From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51553) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UNCKf-0005na-N9 for qemu-devel@nongnu.org; Tue, 02 Apr 2013 21:17:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UNCKc-0005tr-0U for qemu-devel@nongnu.org; Tue, 02 Apr 2013 21:17:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:5546) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UNCKb-0005tg-ON for qemu-devel@nongnu.org; Tue, 02 Apr 2013 21:17:41 -0400 Message-ID: <515B832F.7050103@redhat.com> Date: Tue, 02 Apr 2013 19:17:35 -0600 From: Eric Blake MIME-Version: 1.0 References: <1364903250-10429-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1364903250-10429-6-git-send-email-xiawenc@linux.vnet.ibm.com> In-Reply-To: <1364903250-10429-6-git-send-email-xiawenc@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2NADFRAHMINGJTPLWFTDN" Subject: Re: [Qemu-devel] [PATCH V11 05/17] block: add snapshot info query function bdrv_query_snapshot_info_list() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: kwolf@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, stefanha@gmail.com, pbonzini@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2NADFRAHMINGJTPLWFTDN Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 04/02/2013 05:47 AM, Wenchao Xia wrote: > This patch adds function bdrv_query_snapshot_info_list(), which will > retrieve snapshot info of an image in qmp object format. The implementa= tion > is based on the code moved from qemu-img.c with modification to fit mor= e > for qmp based block layer API. >=20 > Signed-off-by: Wenchao Xia > --- > block/qapi.c | 55 ++++++++++++++++++++++++++++++++++++++----= ------- > include/block/qapi.h | 4 ++- > qemu-img.c | 5 +++- > 3 files changed, 49 insertions(+), 15 deletions(-) > +/* > + * return 0 on success, @p_list will be set only on success, and calle= r need to s/need/needs/ > + * check *p_list on success. I wonder if this wording would be any better: Returns 0 on success, with *p_list either set to describe snapshot information, or NULL because there are no snapshots. Returns -1 on error, with *p_list untouched. > + */ > +int bdrv_query_snapshot_info_list(BlockDriverState *bs, > + SnapshotInfoList **p_list, > + Error **errp) > { At any rate, my only commentary was on grammar and a possible wording for a comment, while the code itself is fine from my viewpoint; so feel free to add: Reviewed-by: Eric Blake > +++ b/qemu-img.c > @@ -1735,7 +1735,10 @@ static ImageInfoList *collect_image_info_list(co= nst char *filename, > =20 > info =3D g_new0(ImageInfo, 1); > bdrv_collect_image_info(bs, info, filename); > - bdrv_collect_snapshots(bs, info); > + if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL)= && > + info->snapshots) { > + info->has_snapshots =3D true; > + } Hmm. info->snapshots starts life as NULL (thanks to g_new0), and is untouched on error. Since you are ignoring any errors, you technically could write: bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL); if (info->snapshots) { info->has_snapshots =3D true; } for the same semantics. That means that as of this commit, no caller cares about the return value of bdrv_query_snapshot_info_list (they only care about whether info->snapshots was changed to non-null), so it could return void for a slightly simpler implementation. But I don't know if any later patches in the series start to care about which error was returned. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2NADFRAHMINGJTPLWFTDN Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJRW4MvAAoJEKeha0olJ0NqRDIH/20pgWaMrcAJ+5HW2GpJUJ+x 1YUbYa0Ujjav2+XT5tnwqgDYRr1VKCmyvk1iRWsxAhnnXt59hQwRILd3ursTrkQW w2sX2f+LkVQLOTy/wVJFhZjZQmTdIBX407ZEUv6pl1hvdPkZWLbtLsrccmmw1Muh l0Vwncj9htT4os8jWz93j+sWv8CFWq2dYM5JYCyJHEE6VysI7VlUljEn1GC0uCun N5OQB6U3NaTOn2ZhXWQLMrAoXbZWGsXZ6Sjt/58IeXvmJAsG0qW9ufl/L8N8Z3Ci UcXukKTpSMl13BhyGlhxJKX4MmR1L1paSMW7tbYpA1Fp9E/TPuCZ7wO1s5elfJc= =V955 -----END PGP SIGNATURE----- ------enig2NADFRAHMINGJTPLWFTDN--