From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38383) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHa5Y-0005sW-KM for qemu-devel@nongnu.org; Fri, 08 Jan 2016 11:40:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aHa5V-0004dL-6h for qemu-devel@nongnu.org; Fri, 08 Jan 2016 11:40:32 -0500 Received: from mx2.parallels.com ([199.115.105.18]:36622) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHa5V-0004d0-0S for qemu-devel@nongnu.org; Fri, 08 Jan 2016 11:40:29 -0500 References: <1449240275-26196-1-git-send-email-den@openvz.org> <1449240275-26196-2-git-send-email-den@openvz.org> <567B11BA.4090901@redhat.com> <568F9D24.6090803@openvz.org> <568FE068.9040203@redhat.com> From: "Denis V. Lunev" Message-ID: <568FE672.3040401@openvz.org> Date: Fri, 8 Jan 2016 19:40:18 +0300 MIME-Version: 1.0 In-Reply-To: <568FE068.9040203@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 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