| Message ID | 1449240275-26196-2-git-send-email-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
On 12/04/2015 07:44 AM, Denis V. Lunev wrote: > This would be useful in the next step when QMP version of this call will > be introduced. > > Signed-off-by: Denis V. Lunev <den@openvz.org> > Reviewed-by: Juan Quintela <quintela@redhat.com> > CC: Amit Shah <amit.shah@redhat.com> > CC: Markus Armbruster <armbru@redhat.com> > CC: Eric Blake <eblake@redhat.com> > --- > migration/savevm.c | 38 +++++++++++++++++++++++--------------- > 1 file changed, 23 insertions(+), 15 deletions(-) > > @@ -1915,28 +1915,27 @@ void hmp_savevm(Monitor *mon, const QDict *qdict) > uint64_t vm_state_size; > qemu_timeval tv; > struct tm tm; > - const char *name = qdict_get_try_str(qdict, "name"); > Error *local_err = NULL; > AioContext *aio_context; > > if (!bdrv_all_can_snapshot(&bs)) { > - monitor_printf(mon, "Device '%s' is writable but does not " > - "support snapshots.\n", bdrv_get_device_name(bs)); > + error_setg(errp, > + "Device '%s' is writable but does not support snapshots.", No trailing '.' in error_setg() calls. > + bdrv_get_device_name(bs)); > return; > } > > /* Delete old snapshots of the same name */ > if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) { > - monitor_printf(mon, > - "Error while deleting snapshot on device '%s': %s\n", > - bdrv_get_device_name(bs1), error_get_pretty(local_err)); > + error_setg(errp, "Error while deleting snapshot on device '%s': %s", > + bdrv_get_device_name(bs1), error_get_pretty(local_err)); Markus' series to add a prefixing notation would be better to use here (although I didn't check if he caught this one in that series already): https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html > > +void hmp_savevm(Monitor *mon, const QDict *qdict) > +{ > + Error *local_err = NULL; > + > + do_savevm(qdict_get_try_str(qdict, "name"), &local_err); > + > + if (local_err != NULL) { I would have just written 'if (local_err) {'; but that's minor style. Looks like a clean refactoring, other than the nit about the trailing '.', so with that fixed: Reviewed-by: Eric Blake <eblake@redhat.com>
On 12/24/2015 12:27 AM, Eric Blake wrote: > On 12/04/2015 07:44 AM, Denis V. Lunev wrote: >> This would be useful in the next step when QMP version of this call will >> be introduced. >> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> Reviewed-by: Juan Quintela <quintela@redhat.com> >> CC: Amit Shah <amit.shah@redhat.com> >> CC: Markus Armbruster <armbru@redhat.com> >> CC: Eric Blake <eblake@redhat.com> >> --- >> migration/savevm.c | 38 +++++++++++++++++++++++--------------- >> 1 file changed, 23 insertions(+), 15 deletions(-) >> >> @@ -1915,28 +1915,27 @@ void hmp_savevm(Monitor *mon, const QDict *qdict) >> uint64_t vm_state_size; >> qemu_timeval tv; >> struct tm tm; >> - const char *name = qdict_get_try_str(qdict, "name"); >> Error *local_err = NULL; >> AioContext *aio_context; >> >> if (!bdrv_all_can_snapshot(&bs)) { >> - monitor_printf(mon, "Device '%s' is writable but does not " >> - "support snapshots.\n", bdrv_get_device_name(bs)); >> + error_setg(errp, >> + "Device '%s' is writable but does not support snapshots.", > No trailing '.' in error_setg() calls. > >> + bdrv_get_device_name(bs)); >> return; >> } >> >> /* Delete old snapshots of the same name */ >> if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) { >> - monitor_printf(mon, >> - "Error while deleting snapshot on device '%s': %s\n", >> - bdrv_get_device_name(bs1), error_get_pretty(local_err)); >> + error_setg(errp, "Error while deleting snapshot on device '%s': %s", >> + bdrv_get_device_name(bs1), error_get_pretty(local_err)); > Markus' series to add a prefixing notation would be better to use here > (although I didn't check if he caught this one in that series already): > https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html this series is not yet merged. I think that we could do this refactoring later on. This thing could be considered independent. Anyway, this series has its own value and it takes a lot of time to push it in. Could we do error setting improvement later on? Messages are not changed etc. I'll change them as you suggest. >> >> +void hmp_savevm(Monitor *mon, const QDict *qdict) >> +{ >> + Error *local_err = NULL; >> + >> + do_savevm(qdict_get_try_str(qdict, "name"), &local_err); >> + >> + if (local_err != NULL) { > I would have just written 'if (local_err) {'; but that's minor style. from my point of view explicit != NULL exposes that local_err is a pointer rather than a boolean value. > Looks like a clean refactoring, other than the nit about the trailing > '.', so with that fixed: > Reviewed-by: Eric Blake <eblake@redhat.com> >
On 01/08/2016 04:27 AM, Denis V. Lunev wrote: >>> /* Delete old snapshots of the same name */ >>> if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < >>> 0) { >>> - monitor_printf(mon, >>> - "Error while deleting snapshot on device >>> '%s': %s\n", >>> - bdrv_get_device_name(bs1), >>> error_get_pretty(local_err)); >>> + error_setg(errp, "Error while deleting snapshot on device >>> '%s': %s", >>> + bdrv_get_device_name(bs1), >>> error_get_pretty(local_err)); >> Markus' series to add a prefixing notation would be better to use here >> (although I didn't check if he caught this one in that series already): >> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html > > this series is not yet merged. I think that we could do this refactoring > later on. > This thing could be considered independent. Anyway, this series has its > own value > and it takes a lot of time to push it in. Could we do error setting > improvement later on? I don't care who rebases on top of the other, but maybe Markus will have an opinion when he gets back online next week. >>> + >>> + if (local_err != NULL) { >> I would have just written 'if (local_err) {'; but that's minor style. > from my point of view explicit != NULL exposes that local_err is a > pointer rather than a boolean value. But the code base already overwhelmingly relies on C's implicit conversion of pointer to a boolean context, as it requires less typing; being verbose doesn't make the code base any easier to read. However, since HACKING doesn't say one way or the other, I won't make you change.
On 01/08/2016 07:14 PM, Eric Blake wrote: > On 01/08/2016 04:27 AM, Denis V. Lunev wrote: > >>>> /* Delete old snapshots of the same name */ >>>> if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < >>>> 0) { >>>> - monitor_printf(mon, >>>> - "Error while deleting snapshot on device >>>> '%s': %s\n", >>>> - bdrv_get_device_name(bs1), >>>> error_get_pretty(local_err)); >>>> + error_setg(errp, "Error while deleting snapshot on device >>>> '%s': %s", >>>> + bdrv_get_device_name(bs1), >>>> error_get_pretty(local_err)); >>> Markus' series to add a prefixing notation would be better to use here >>> (although I didn't check if he caught this one in that series already): >>> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html >> this series is not yet merged. I think that we could do this refactoring >> later on. >> This thing could be considered independent. Anyway, this series has its >> own value >> and it takes a lot of time to push it in. Could we do error setting >> improvement later on? > I don't care who rebases on top of the other, but maybe Markus will have > an opinion when he gets back online next week. > why we have to wait with this set due to this reason? The code with error_prepend and current code are BOTH correct. One is a bit shorter then other. Yes, it would be nice to switch to it, but why this should be done in this set? This set solves real problem which has not been addressed for a long time. Let's proceed, cool and shiny stuff could be done later on, when it will be merged. Moreover, merging this set will make my life easier with merging these changes to our downstream. Fixes will be merged while improvements will stay in upstream only. >>>> + >>>> + if (local_err != NULL) { >>> I would have just written 'if (local_err) {'; but that's minor style. >> from my point of view explicit != NULL exposes that local_err is a >> pointer rather than a boolean value. > But the code base already overwhelmingly relies on C's implicit > conversion of pointer to a boolean context, as it requires less typing; > being verbose doesn't make the code base any easier to read. However, > since HACKING doesn't say one way or the other, I won't make you change. > I do not understand your last words. I am not agitating you with one approach or another. This is a reason why I am writing code this way. The code written this way looks better to me. This code is NEW and this does not contradict any written rule in coding style policy. If the code is working and correct, can we just move on with it? Den
On 01/08/2016 09:40 AM, Denis V. Lunev wrote: >>>> Markus' series to add a prefixing notation would be better to use here >>>> (although I didn't check if he caught this one in that series already): >>>> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html >>> this series is not yet merged. I think that we could do this refactoring >>> later on. >>> This thing could be considered independent. Anyway, this series has its >>> own value >>> and it takes a lot of time to push it in. Could we do error setting >>> improvement later on? >> I don't care who rebases on top of the other, but maybe Markus will have >> an opinion when he gets back online next week. >> > why we have to wait with this set due to this reason? One of you will have to rebase on the other - either you wait for Markus' error_prepend to go in and you use it, or your patch goes in and Markus updates his error_prepend patch to cover your additional instance that will be benefitted by it. I don't care which, and the timing is really up to the maintainers and how fast they send pull requests. > The code with error_prepend and current code are BOTH > correct. One is a bit shorter then other. Yes, it would > be nice to switch to it, but why this should be done in > this set? Exactly, we're saying the same things. >>>>> + if (local_err != NULL) { >>>> I would have just written 'if (local_err) {'; but that's minor style. >>> from my point of view explicit != NULL exposes that local_err is a >>> pointer rather than a boolean value. >> But the code base already overwhelmingly relies on C's implicit >> conversion of pointer to a boolean context, as it requires less typing; >> being verbose doesn't make the code base any easier to read. However, >> since HACKING doesn't say one way or the other, I won't make you change. >> > I do not understand your last words. > > I am not agitating you with one approach or another. This > is a reason why I am writing code this way. The code written > this way looks better to me. This code is NEW and this does > not contradict any written rule in coding style policy. > > If the code is working and correct, can we just move on with it? Once again, we are saying the same thing. I pointed out a cosmetic issue, but one where I do not have a strong enough leg to stand on to force you to change your style, so what you did is fine as is.
On 01/08/2016 08:54 PM, Eric Blake wrote: > On 01/08/2016 09:40 AM, Denis V. Lunev wrote: > >>>>> Markus' series to add a prefixing notation would be better to use here >>>>> (although I didn't check if he caught this one in that series already): >>>>> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html >>>> this series is not yet merged. I think that we could do this refactoring >>>> later on. >>>> This thing could be considered independent. Anyway, this series has its >>>> own value >>>> and it takes a lot of time to push it in. Could we do error setting >>>> improvement later on? >>> I don't care who rebases on top of the other, but maybe Markus will have >>> an opinion when he gets back online next week. >>> >> why we have to wait with this set due to this reason? > One of you will have to rebase on the other - either you wait for > Markus' error_prepend to go in and you use it, or your patch goes in and > Markus updates his error_prepend patch to cover your additional instance > that will be benefitted by it. I don't care which, and the timing is > really up to the maintainers and how fast they send pull requests. > >> The code with error_prepend and current code are BOTH >> correct. One is a bit shorter then other. Yes, it would >> be nice to switch to it, but why this should be done in >> this set? > Exactly, we're saying the same things. > >>>>>> + if (local_err != NULL) { >>>>> I would have just written 'if (local_err) {'; but that's minor style. >>>> from my point of view explicit != NULL exposes that local_err is a >>>> pointer rather than a boolean value. >>> But the code base already overwhelmingly relies on C's implicit >>> conversion of pointer to a boolean context, as it requires less typing; >>> being verbose doesn't make the code base any easier to read. However, >>> since HACKING doesn't say one way or the other, I won't make you change. >>> >> I do not understand your last words. >> >> I am not agitating you with one approach or another. This >> is a reason why I am writing code this way. The code written >> this way looks better to me. This code is NEW and this does >> not contradict any written rule in coding style policy. >> >> If the code is working and correct, can we just move on with it? > Once again, we are saying the same thing. I pointed out a cosmetic > issue, but one where I do not have a strong enough leg to stand on to > force you to change your style, so what you did is fine as is. > ok. perfect to be on the same page :) I'll promise to switch to error_prepend code when it will be merged. I hope that v4 of the set is good enough to proceed. Den
diff --git a/migration/savevm.c b/migration/savevm.c index 0ad1b93..b7278ac 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f) return ret; } -void hmp_savevm(Monitor *mon, const QDict *qdict) +static void do_savevm(const char *name, Error **errp) { BlockDriverState *bs, *bs1; QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1; @@ -1915,28 +1915,27 @@ void hmp_savevm(Monitor *mon, const QDict *qdict) uint64_t vm_state_size; qemu_timeval tv; struct tm tm; - const char *name = qdict_get_try_str(qdict, "name"); Error *local_err = NULL; AioContext *aio_context; if (!bdrv_all_can_snapshot(&bs)) { - monitor_printf(mon, "Device '%s' is writable but does not " - "support snapshots.\n", bdrv_get_device_name(bs)); + error_setg(errp, + "Device '%s' is writable but does not support snapshots.", + bdrv_get_device_name(bs)); return; } /* Delete old snapshots of the same name */ if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) { - monitor_printf(mon, - "Error while deleting snapshot on device '%s': %s\n", - bdrv_get_device_name(bs1), error_get_pretty(local_err)); + error_setg(errp, "Error while deleting snapshot on device '%s': %s", + bdrv_get_device_name(bs1), error_get_pretty(local_err)); error_free(local_err); return; } bs = bdrv_all_find_vmstate_bs(); if (bs == NULL) { - monitor_printf(mon, "No block device can accept snapshots\n"); + error_setg(errp, "No block device can accept snapshots"); return; } aio_context = bdrv_get_aio_context(bs); @@ -1945,7 +1944,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict) ret = global_state_store(); if (ret) { - monitor_printf(mon, "Error saving global state\n"); + error_setg(errp, "Error saving global state"); return; } vm_stop(RUN_STATE_SAVE_VM); @@ -1977,22 +1976,20 @@ void hmp_savevm(Monitor *mon, const QDict *qdict) /* save the VM state */ f = qemu_fopen_bdrv(bs, 1); if (!f) { - monitor_printf(mon, "Could not open VM state file\n"); + error_setg(errp, "Could not open VM state file"); goto the_end; } - ret = qemu_savevm_state(f, &local_err); + ret = qemu_savevm_state(f, errp); vm_state_size = qemu_ftell(f); qemu_fclose(f); if (ret < 0) { - monitor_printf(mon, "%s\n", error_get_pretty(local_err)); - error_free(local_err); goto the_end; } ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs); if (ret < 0) { - monitor_printf(mon, "Error while creating snapshot on '%s'\n", - bdrv_get_device_name(bs)); + error_setg(errp, "Error while creating snapshot on '%s'", + bdrv_get_device_name(bs)); } the_end: @@ -2002,6 +1999,17 @@ void hmp_savevm(Monitor *mon, const QDict *qdict) } } +void hmp_savevm(Monitor *mon, const QDict *qdict) +{ + Error *local_err = NULL; + + do_savevm(qdict_get_try_str(qdict, "name"), &local_err); + + if (local_err != NULL) { + error_report_err(local_err); + } +} + void qmp_xen_save_devices_state(const char *filename, Error **errp) { QEMUFile *f;