From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:42600) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKUm7-0008Ml-0w for qemu-devel@nongnu.org; Tue, 26 Mar 2013 10:23:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UKUm5-0007Or-9E for qemu-devel@nongnu.org; Tue, 26 Mar 2013 10:22:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:1671) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKUm5-0007Nt-0n for qemu-devel@nongnu.org; Tue, 26 Mar 2013 10:22:53 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r2QEMqRF010391 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 26 Mar 2013 10:22:52 -0400 Message-ID: <5151AF3A.6070801@redhat.com> Date: Tue, 26 Mar 2013 08:22:50 -0600 From: Eric Blake MIME-Version: 1.0 References: <5752796f1573f8ccd8de9224a5ce15f71908ac2d.1363957855.git.phrdina@redhat.com> In-Reply-To: <5752796f1573f8ccd8de9224a5ce15f71908ac2d.1363957855.git.phrdina@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2AFSBLJFIDPJGJHJWLHMG" Subject: Re: [Qemu-devel] [PATCH v2 01/12] block: add error parameter to bdrv_snapshot_create() and related functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Hrdina Cc: lcapitulino@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2AFSBLJFIDPJGJHJWLHMG Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 03/22/2013 07:16 AM, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina > --- > block.c | 22 ++++++++++++++++------ > block/qcow2-snapshot.c | 9 ++++++++- > block/qcow2.h | 4 +++- > block/rbd.c | 8 ++++++-- > block/sheepdog.c | 17 +++++++++-------- > include/block/block.h | 3 ++- > include/block/block_int.h | 3 ++- > qemu-img.c | 2 +- > savevm.c | 2 +- > 9 files changed, 48 insertions(+), 22 deletions(-) >=20 > int bdrv_snapshot_create(BlockDriverState *bs, > - QEMUSnapshotInfo *sn_info) > + QEMUSnapshotInfo *sn_info, > + Error **errp) > { > BlockDriver *drv =3D bs->drv; > - if (!drv) > + > + if (!drv) { > + error_setg(errp, "Device '%s' has no medium.", In general, error_setg() should not print a trailing '.' (only two offenders to git grep 'error_setg.*\."'). I think we also tend to start messages with lower case, although that's not as obvious (49 cases of 'error_setg[^"*"[A-Z]' vs. 121 of error_setg[^"]*"[a-z]'). > + > + error_setg(errp, "Snapshot is not supported for '%s'.", And again, and probably throughout your series (although I'll quit pointing it out). > @@ -830,16 +832,18 @@ static int qemu_rbd_snap_create(BlockDriverState = *bs, > */ > if (sn_info->id_str[0] !=3D '\0' && > strcmp(sn_info->id_str, sn_info->name) !=3D 0) { > + error_setg(errp, "ID and name have to be equal."); > return -EINVAL; > } > =20 > if (strlen(sn_info->name) >=3D sizeof(sn_info->id_str)) { > + error_setg(errp, "Parameter 'name' has to be shorter that 127 = chars."); s/that/than/ > return -ERANGE; > } > =20 > r =3D rbd_snap_create(s->image, sn_info->name); > if (r < 0) { > - error_report("failed to create snap: %s", strerror(-r)); > + error_setg(errp, "Failed to create snapshot: '%s'.", strerror(= -r)); Use error_setg_errno(errp, -r, "failed to create snapshot"). > @@ -1779,9 +1781,8 @@ static int sd_snapshot_create(BlockDriverState *b= s, QEMUSnapshotInfo *sn_info) > s->name, sn_info->vm_state_size, s->is_snapshot); > =20 > if (s->is_snapshot) { > - error_report("You can't create a snapshot of a snapshot VDI, "= > - "%s (%" PRIu32 ").", s->name, s->inode.vdi_id); > - > + error_setg(errp, "You can't create a snapshot '%s' of a VDI sn= apshot.", > + sn_info->name); Why are you losing information from the previous version of this error message? > if (ret < 0) { > - error_report("failed to create inode for snapshot. %s", > - strerror(errno)); > + error_setg(errp, "Failed to create inode for snapshot."); Another case of losing information; error_setg_errno would be better here, and anywhere else the old message included a strerror() call. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2AFSBLJFIDPJGJHJWLHMG 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/ iQEcBAEBCAAGBQJRUa86AAoJEKeha0olJ0NqdwgH/0V+BOCRFUyAoqOJaeGf/wgR ppHOTVn/OFSSaEPvDEQ1cQ5b0x3FQlf7LS54OytYRgJ7Hf1X23U41etjnFXc6gvU HvyLoj7m45hlwN36cffpQVzyphjSS2bZCbm8XBitY/K/DKanSl14yG3Y0z4cfxID qVHEZQ78Mw6KX03MqomrYI5kCfbbspqSqb42ExyLvAcTJe4MGWvEXUpokYG/sEQT IaTs8LH8xg586BQrbzo0da80uWLjV8P+jGwiqecE3zVLPwQ0Ha3RjpynhccGofLA pEOPxhkBRLIsYIV0pFK4JZG9f5dAwzAj7ddMpkoaVEij73i8m4uzLr4QRSx5MQM= =PsDo -----END PGP SIGNATURE----- ------enig2AFSBLJFIDPJGJHJWLHMG--