From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36191) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBUJT-0003tG-Ux for qemu-devel@nongnu.org; Tue, 22 Dec 2015 16:17:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aBUJS-0003PI-QY for qemu-devel@nongnu.org; Tue, 22 Dec 2015 16:17:43 -0500 References: <1450802786-20893-1-git-send-email-kwolf@redhat.com> <1450802786-20893-8-git-send-email-kwolf@redhat.com> From: Eric Blake Message-ID: <5679BDEC.4040100@redhat.com> Date: Tue, 22 Dec 2015 14:17:32 -0700 MIME-Version: 1.0 In-Reply-To: <1450802786-20893-8-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="kxaDXOwnjfthoR9d8ORoFNnC7kRtUNwL1" Subject: Re: [Qemu-devel] [PATCH 07/10] qcow2: Implement .bdrv_inactivate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-block@nongnu.org Cc: qemu-devel@nongnu.org, mreitz@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --kxaDXOwnjfthoR9d8ORoFNnC7kRtUNwL1 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 12/22/2015 09:46 AM, Kevin Wolf wrote: > The callback has to ensure that closing or flushing the image afterward= s > wouldn't cause a write access to the image files. This means that just > the caches have to be written out, which is part of the existing > .bdrv_close implementation. >=20 > Signed-off-by: Kevin Wolf > --- > block/qcow2.c | 45 ++++++++++++++++++++++++++++----------------- > 1 file changed, 28 insertions(+), 17 deletions(-) >=20 Mostly code motion, but I still ended up with questions. > =20 > +static int qcow2_inactivate(BlockDriverState *bs) > +{ > + BDRVQcow2State *s =3D bs->opaque; > + int ret, result =3D 0; > + > + ret =3D qcow2_cache_flush(bs, s->l2_table_cache); > + if (ret) { > + result =3D ret; > + error_report("Failed to flush the L2 table cache: %s", > + strerror(-ret)); I asked Markus if we want error_report_errno() - and ever since then, I keep finding more and more uses that would benefit from it :) > + } > + > + ret =3D qcow2_cache_flush(bs, s->refcount_block_cache); > + if (ret) { > + result =3D ret; > + error_report("Failed to flush the refcount block cache: %s", > + strerror(-ret)); > + } > + If the media fails in between these two statements,... > + if (result =3D=3D 0) { > + qcow2_mark_clean(bs); =2E..can't qcow2_mark_clean() fail due to an EIO or other write error? D= o we care? I guess the worst is that we didn't mark the image clean after all, which is no worse than if qemu[-img] had been SIGKILL'd at the same point where I hypothesized that the media could fail. > + } > + > + return result; If both flushes failed, you return the result set to the return value of the second flush. Is it ever possible that the return value of the first flush might be more useful? > @@ -1693,23 +1719,7 @@ static void qcow2_close(BlockDriverState *bs) > s->l1_table =3D NULL; > =20 > if (!(bs->open_flags & BDRV_O_INCOMING)) { > - if (!ret1 && !ret2) { > - qcow2_mark_clean(bs); > - } > + qcow2_inactivate(bs); > } Then again, the lone existing caller in this addition doesn't even care about the return value. The only other caller was your new code added earlier in the series; in 5/10 migration_completion(), which uses the value as a conditional but doesn't try to call strerror(-ret). Since this is mostly code motion, any semantic changes you would want to make based on my questions above probably belong in their own patches. Therefore, for this patch as written, Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --kxaDXOwnjfthoR9d8ORoFNnC7kRtUNwL1 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWeb3sAAoJEKeha0olJ0Nq42AH+gIKedrAvHF1vdj/E6/QXe4z BycayPJ9PIAGqF0msg8+/FRhDodOlik3aysnPslX/x6UB3cqdp+Ke6ldeg1ZXzj2 HKIqwLedDMB3q2aCq8twOeRno4P58x3AG9MKmV1KLTkeQlBOdXst4oycJH38vzck dhXZpll3d5WLoT+sCFgy1LDV2f/XlMXIKb0mmuWxIdry11eTaztlq03aDPOD40k0 s6/ei6b/gYqjxx3cJ0w2/JwE0f1kjk7GtGkyoT/RO7rQuenbatHOcXOd592W7VKr jH3fzqdrKSF75VFcsrSma5DPAeWTdNdbQX5UWlm2jV/tY9kz8Mu3i6Upjg4Y3jc= =JxpO -----END PGP SIGNATURE----- --kxaDXOwnjfthoR9d8ORoFNnC7kRtUNwL1--