Re: [PATCH v4 11/25] migration: Add Error** argument to .save_setup() handler
On Fri, Mar 08, 2024 at 03:11:04PM +0800, Peter Xu wrote: > On Thu, Mar 07, 2024 at 02:39:31PM +0300, Vladimir Sementsov-Ogievskiy wrote: > > > I would be glad to have most of this series merged in QEMU 9.0. So, > > > unless there is something major, I will keep that for followups. > > Unfortunately I found this series won't apply to master.. starting from > "migration: Always report an error in ram_save_setup()". Perhaps forgot to > pull before the repost? Scratch this. It's myself who forgot to pull... :-( It applies all fine. > > It'll also be nice if we can get an ACK for the s390 patch from a > maintainer. I'll ping on the patch. -- Peter Xu
Re: [PATCH v4 11/25] migration: Add Error** argument to .save_setup() handler
On Thu, Mar 07, 2024 at 02:39:31PM +0300, Vladimir Sementsov-Ogievskiy wrote: > > I would be glad to have most of this series merged in QEMU 9.0. So, > > unless there is something major, I will keep that for followups. Unfortunately I found this series won't apply to master.. starting from "migration: Always report an error in ram_save_setup()". Perhaps forgot to pull before the repost? It'll also be nice if we can get an ACK for the s390 patch from a maintainer. Cedric, would you prefer a repost before this weekend, or we just wait for 9.1? IMHO we don't need to rush this in 9.0 if it's still partially done, so the latter option isn't that bad (I've already queued the initial four irrelevant of that). Thanks, -- Peter Xu
Re: [PATCH v4 11/25] migration: Add Error** argument to .save_setup() handler
On 07.03.24 13:31, Cédric Le Goater wrote:
On 3/7/24 10:53, Vladimir Sementsov-Ogievskiy wrote:
On 06.03.24 16:34, Cédric Le Goater wrote:
The purpose is to record a potential error in the migration stream if
qemu_savevm_state_setup() fails. Most of the current .save_setup()
handlers can be modified to use the Error argument instead of managing
their own and calling locally error_report().
Cc: Nicholas Piggin
Cc: Harsh Prateek Bora
Cc: Halil Pasic
Cc: Thomas Huth
Cc: Eric Blake
Cc: Vladimir Sementsov-Ogievskiy
Cc: John Snow
Cc: Stefan Hajnoczi
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Peter Xu
Reviewed-by: Thomas Huth
Signed-off-by: Cédric Le Goater
Reviewed-by: Vladimir Sementsov-Ogievskiy
Still, if you resend, please add error_prepend in the case below:
diff --git a/migration/savevm.c b/migration/savevm.c
index
63fdbb5ad7d4dbfaef1d2094350bf302cc677602..52d35b2a72c6238bfe5dcb4d81c1af8d2bf73013
100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1342,11 +1342,9 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
}
save_section_header(f, se, QEMU_VM_SECTION_START);
- ret = se->ops->save_setup(f, se->opaque);
+ ret = se->ops->save_setup(f, se->opaque, errp);
save_section_footer(f, se);
if (ret < 0) {
- error_setg(errp, "failed to setup SaveStateEntry with id(name): "
- "%d(%s): %d", se->section_id, se->idstr, ret);
You drop a good bit of information, let's use error_prepend to save it.
I kind of agree but the call stack is quite deep and the callees also use
error_prepend. The error becomes quite long. Here's an example of what we
get today :
(qemu) migrate -d tcp:10.8.3.15:1234
(qemu)
(qemu) qemu-system-x86_64: vfio: Could not start dirty page tracking -
:b1:00.2: Failed to start DMA logging: Invalid argument
If the subsystems implementing a .save_setup() handler use a component
identifier, the failure should be fairly easy to identify.
What's the best practice for such cases ?
Can we use multiline errors maybe ? Less practical for grep though.
May be a verbose error mode would help getting more information ?
Anyhow, I can add a new trace event for "failed to setup SaveStateEntry ... "
or use error_prepend() as you suggested.
Let's see what the others have to say.
qemu_file_set_error(f, ret);
break;
Not about this patch:
Better do explicit "return ret" instead of this "break" (and one more break
above in that loop):
1. making a jump to do just do "return ret" seems overkill. It would make sense if we had some more
"cleanup" code than just a "return ret", and if so, more classic and readable thing is "goto
fail;".
2. "break" make me think, that there may be more logic after it, which will probably
fail, and I should be careful, as errp is already set (and second attempt to set it will crash).
Again, "goto fail;" is better, as I don't expect more failures when see it.
Sure. If I respin, I can drop the break and simply return.
If so, you should also make simple return instead of one another break in same loop. And
drop "if (ret < 0) { return ret }" after loop.
Although,
I would be glad to have most of this series merged in QEMU 9.0. So,
unless there is something major, I will keep that for followups.
Agree
Thanks for the review,
C.
--
Best regards,
Vladimir
Re: [PATCH v4 11/25] migration: Add Error** argument to .save_setup() handler
On 3/7/24 10:53, Vladimir Sementsov-Ogievskiy wrote:
On 06.03.24 16:34, Cédric Le Goater wrote:
The purpose is to record a potential error in the migration stream if
qemu_savevm_state_setup() fails. Most of the current .save_setup()
handlers can be modified to use the Error argument instead of managing
their own and calling locally error_report().
Cc: Nicholas Piggin
Cc: Harsh Prateek Bora
Cc: Halil Pasic
Cc: Thomas Huth
Cc: Eric Blake
Cc: Vladimir Sementsov-Ogievskiy
Cc: John Snow
Cc: Stefan Hajnoczi
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Peter Xu
Reviewed-by: Thomas Huth
Signed-off-by: Cédric Le Goater
Reviewed-by: Vladimir Sementsov-Ogievskiy
Still, if you resend, please add error_prepend in the case below:
diff --git a/migration/savevm.c b/migration/savevm.c
index
63fdbb5ad7d4dbfaef1d2094350bf302cc677602..52d35b2a72c6238bfe5dcb4d81c1af8d2bf73013
100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1342,11 +1342,9 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
}
save_section_header(f, se, QEMU_VM_SECTION_START);
- ret = se->ops->save_setup(f, se->opaque);
+ ret = se->ops->save_setup(f, se->opaque, errp);
save_section_footer(f, se);
if (ret < 0) {
- error_setg(errp, "failed to setup SaveStateEntry with id(name): "
- "%d(%s): %d", se->section_id, se->idstr, ret);
You drop a good bit of information, let's use error_prepend to save it.
I kind of agree but the call stack is quite deep and the callees also use
error_prepend. The error becomes quite long. Here's an example of what we
get today :
(qemu) migrate -d tcp:10.8.3.15:1234
(qemu)
(qemu) qemu-system-x86_64: vfio: Could not start dirty page tracking -
:b1:00.2: Failed to start DMA logging: Invalid argument
If the subsystems implementing a .save_setup() handler use a component
identifier, the failure should be fairly easy to identify.
What's the best practice for such cases ?
Can we use multiline errors maybe ? Less practical for grep though.
May be a verbose error mode would help getting more information ?
Anyhow, I can add a new trace event for "failed to setup SaveStateEntry ... "
or use error_prepend() as you suggested.
Let's see what the others have to say.
qemu_file_set_error(f, ret);
break;
Not about this patch:
Better do explicit "return ret" instead of this "break" (and one more break
above in that loop):
1. making a jump to do just do "return ret" seems overkill. It would make sense if we had some more
"cleanup" code than just a "return ret", and if so, more classic and readable thing is "goto
fail;".
2. "break" make me think, that there may be more logic after it, which will probably
fail, and I should be careful, as errp is already set (and second attempt to set it will crash).
Again, "goto fail;" is better, as I don't expect more failures when see it.
Sure. If I respin, I can drop the break and simply return. Although,
I would be glad to have most of this series merged in QEMU 9.0. So,
unless there is something major, I will keep that for followups.
Thanks for the review,
C.
Re: [PATCH v4 11/25] migration: Add Error** argument to .save_setup() handler
On 06.03.24 16:34, Cédric Le Goater wrote:
The purpose is to record a potential error in the migration stream if
qemu_savevm_state_setup() fails. Most of the current .save_setup()
handlers can be modified to use the Error argument instead of managing
their own and calling locally error_report().
Cc: Nicholas Piggin
Cc: Harsh Prateek Bora
Cc: Halil Pasic
Cc: Thomas Huth
Cc: Eric Blake
Cc: Vladimir Sementsov-Ogievskiy
Cc: John Snow
Cc: Stefan Hajnoczi
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Peter Xu
Reviewed-by: Thomas Huth
Signed-off-by: Cédric Le Goater
Reviewed-by: Vladimir Sementsov-Ogievskiy
Still, if you resend, please add error_prepend in the case below:
diff --git a/migration/savevm.c b/migration/savevm.c
index
63fdbb5ad7d4dbfaef1d2094350bf302cc677602..52d35b2a72c6238bfe5dcb4d81c1af8d2bf73013
100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1342,11 +1342,9 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
}
save_section_header(f, se, QEMU_VM_SECTION_START);
-ret = se->ops->save_setup(f, se->opaque);
+ret = se->ops->save_setup(f, se->opaque, errp);
save_section_footer(f, se);
if (ret < 0) {
-error_setg(errp, "failed to setup SaveStateEntry with id(name): "
- "%d(%s): %d", se->section_id, se->idstr, ret);
You drop a good bit of information, let's use error_prepend to save it.
qemu_file_set_error(f, ret);
break;
Not about this patch:
Better do explicit "return ret" instead of this "break" (and one more break
above in that loop):
1. making a jump to do just do "return ret" seems overkill. It would make sense if we had some more
"cleanup" code than just a "return ret", and if so, more classic and readable thing is "goto
fail;".
2. "break" make me think, that there may be more logic after it, which will probably
fail, and I should be careful, as errp is already set (and second attempt to set it will crash).
Again, "goto fail;" is better, as I don't expect more failures when see it.
--
Best regards,
Vladimir
