From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53472) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XvkF4-0002DH-NI for qemu-devel@nongnu.org; Tue, 02 Dec 2014 04:59:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XvkEy-0002rZ-I2 for qemu-devel@nongnu.org; Tue, 02 Dec 2014 04:59:34 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57656) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XvkEy-0002rT-9N for qemu-devel@nongnu.org; Tue, 02 Dec 2014 04:59:28 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sB29xRo8026435 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Tue, 2 Dec 2014 04:59:27 -0500 Message-ID: <547D8D7B.2040804@redhat.com> Date: Tue, 02 Dec 2014 10:59:23 +0100 From: Max Reitz MIME-Version: 1.0 References: <1416503198-17031-1-git-send-email-mreitz@redhat.com> <1416503198-17031-19-git-send-email-mreitz@redhat.com> <20141128141351.GS13631@stefanha-thinkpad.redhat.com> In-Reply-To: <20141128141351.GS13631@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 18/22] qcow2: Use intermediate helper CB for amend List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , qemu-devel@nongnu.org On 2014-11-28 at 15:13, Stefan Hajnoczi wrote: > On Thu, Nov 20, 2014 at 06:06:34PM +0100, Max Reitz wrote: >> @@ -2674,6 +2743,7 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts, >> bool encrypt; >> int ret; >> QemuOptDesc *desc = opts->list->desc; >> + Qcow2AmendHelperCBInfo helper_cb_info; >> >> while (desc && desc->name) { >> if (!qemu_opt_find(opts, desc->name)) { >> @@ -2731,6 +2801,12 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts, >> desc++; >> } >> >> + helper_cb_info = (Qcow2AmendHelperCBInfo){ >> + .original_status_cb = status_cb, >> + .original_cb_opaque = cb_opaque, >> + .total_operations = (new_version < old_version) > If you respin, another way of writing this is without total_operations > here (so it initializes to 0)... > >> + }; >> + >> /* Upgrade first (some features may require compat=1.1) */ >> if (new_version > old_version) { >> s->qcow_version = new_version; >> @@ -2789,7 +2865,9 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts, >> >> /* Downgrade last (so unsupported features can be removed before) */ >> if (new_version < old_version) { >> - ret = qcow2_downgrade(bs, new_version, status_cb, cb_opaque); >> + helper_cb_info.current_operation = QCOW2_DOWNGRADING; > ...and then helper_cb_info.total_operations++ here. > > That way the new_version < old_version check is not duplicated into the > helper_cb_info initializer. > > The code is clearer because we assign current_operation and > total_operations at the same time. > > Just a style suggestion, feel free to ignore if you don't like it. I think helper_cb_info.total_operations should be set right when creating the object. We can only do .total_operations++ once, and that is for the very first operation because after that is executed, we may no longer change .total_operations; and I don't think we should treat the first operation differently than the rest. Max