From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60760) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHVCq-0007LU-8k for qemu-devel@nongnu.org; Fri, 08 Jan 2016 06:27:45 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aHVCn-0006k5-34 for qemu-devel@nongnu.org; Fri, 08 Jan 2016 06:27:44 -0500 Received: from mx2.parallels.com ([199.115.105.18]:53509) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHVCm-0006jz-TK for qemu-devel@nongnu.org; Fri, 08 Jan 2016 06:27:41 -0500 References: <1449240275-26196-1-git-send-email-den@openvz.org> <1449240275-26196-2-git-send-email-den@openvz.org> <567B11BA.4090901@redhat.com> From: "Denis V. Lunev" Message-ID: <568F9D24.6090803@openvz.org> Date: Fri, 8 Jan 2016 14:27:32 +0300 MIME-Version: 1.0 In-Reply-To: <567B11BA.4090901@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/5] migration: split hmp_savevm to do_savevm and hmp_savevm wrapper List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Amit Shah , Markus Armbruster , qemu-devel@nongnu.org, quintela@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 >> Reviewed-by: Juan Quintela >> CC: Amit Shah >> CC: Markus Armbruster >> CC: Eric Blake >> --- >> 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 >