From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50404) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USoab-0006ij-Bg for qemu-devel@nongnu.org; Thu, 18 Apr 2013 09:09:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1USoaZ-0002Pr-TM for qemu-devel@nongnu.org; Thu, 18 Apr 2013 09:09:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:61216) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USoaZ-0002Pk-Lj for qemu-devel@nongnu.org; Thu, 18 Apr 2013 09:09:23 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3ID9NM4017489 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 18 Apr 2013 09:09:23 -0400 Message-ID: <516FF082.4050804@redhat.com> Date: Thu, 18 Apr 2013 07:09:22 -0600 From: Eric Blake MIME-Version: 1.0 References: <6e22db8528d4a801af11b37d42a350eab9633636.1366127809.git.phrdina@redhat.com> <20130418125543.GD2723@dhcp-200-207.str.redhat.com> In-Reply-To: <20130418125543.GD2723@dhcp-200-207.str.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2FSMIMAMHLVDMFOKBDJKS" Subject: Re: [Qemu-devel] [PATCH 02/11] block: update error reporting for bdrv_snapshot_delete() and related functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: armbru@redhat.com, Pavel Hrdina , qemu-devel@nongnu.org, lcapitulino@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2FSMIMAMHLVDMFOKBDJKS Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 04/18/2013 06:55 AM, Kevin Wolf wrote: > Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben: >> >> Signed-off-by: Pavel Hrdina >> --- >> =20 >> -int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_i= d) >> +void bdrv_snapshot_delete(BlockDriverState *bs, >> + const char *snapshot_id, >> + Error **errp) >> { >> BlockDriver *drv =3D bs->drv; >> - if (!drv) >> - return -ENOMEDIUM; >> - if (drv->bdrv_snapshot_delete) >> - return drv->bdrv_snapshot_delete(bs, snapshot_id); >> - if (bs->file) >> - return bdrv_snapshot_delete(bs->file, snapshot_id); >> - return -ENOTSUP; >> + >> + if (!drv) { >> + error_setg(errp, "device '%s' has no medium", bdrv_get_device= _name(bs)); >=20 > I don't think the device name is useful here. It's always the device > that the user has specified in the monitor command. Huh? We're talking about vm-snapshot-delete, which removes the snapshot for multiple devices at a time. Knowing _which_ device failed is important in the context of a command where you don't specify a device na= me. >=20 > Also, please start error messages with a capital letter. (This applies > to the whole patch, and probably also other patches in this series) Qemu is inconsistent on this front. The code base actually favors lower case at the moment: $ git grep 'error_setg.*"[a-z]' | wc 119 957 10361 $ git grep 'error_setg.*"[A-Z]' | wc 60 510 4996 Monitor output, on the other hand, favor uppercase: $ git grep 'monitor_pr.*"[A-Z]' | wc 88 576 6611 $ git grep 'monitor_pr.*"[a-z]' | wc 108 789 8566 If you want to enforce a particular style, I think it would be best to patch HACKING to document a preferred style. If it helps as a tie breaker, GNU Coding Standards recommend lowercase: https://www.gnu.org/prep/standards/standards.html#Errors Personally, I don't care which way we go. >=20 >> + } else if (drv->bdrv_snapshot_delete) { >> + drv->bdrv_snapshot_delete(bs, snapshot_id, errp); >> + } else if (bs->file) { >> + bdrv_snapshot_delete(bs->file, snapshot_id, errp); >> + } else { >> + error_setg(errp, "snapshots are not supported on device '%s'"= , >> + bdrv_get_device_name(bs)); >=20 > Same thing with the device name here. Same thing about this function being reached via vm-snapshot-delete where we aren't passing in a device name, so knowing which device failed is important. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2FSMIMAMHLVDMFOKBDJKS 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/ iQEcBAEBCAAGBQJRb/CCAAoJEKeha0olJ0NquuAH/3TSJjsWW+r9eHZxd4yZ0f5W bMfTSqJcCE3QNilTHFUsnKm+5CVQUg2hDP8ZsB+VG8WVS/dJhMXcLiuYc8Y8eJwt xxuNXMQTnCA1GgY1SiBpEFcc5UjZTu3eZNjPSkL8JHRIGQFI5nUs3RsdKK57NHwa q3tUuF/3yk3lWeaZUyH75aP1pUzMGwlhV9WvRKNqIQDNgBZ2rha8spTouWzqEQa4 FouBaH29eUhIsUcpebTV2JRfhfOrVMTU5TQvrsyO0hcVjnPxlUync2FxVYF6jyC2 JOAYjKJn6my5AoIg6L1OLZh6dP1NQTlJSOMPaDQGkNnUA3LzQDA5JkHPGZjLPTc= =Z0Q+ -----END PGP SIGNATURE----- ------enig2FSMIMAMHLVDMFOKBDJKS--