From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37783) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USCnn-0007me-BP for qemu-devel@nongnu.org; Tue, 16 Apr 2013 16:48:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1USCnl-0003Tm-Ge for qemu-devel@nongnu.org; Tue, 16 Apr 2013 16:48:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:31965) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USCnl-0003Tb-7w for qemu-devel@nongnu.org; Tue, 16 Apr 2013 16:48:29 -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 r3GKmSmj017529 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 16 Apr 2013 16:48:28 -0400 Message-ID: <516DB91A.1050900@redhat.com> Date: Tue, 16 Apr 2013 14:48:26 -0600 From: Eric Blake MIME-Version: 1.0 References: In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2EGWFICDSCJCHGHVPUERW" Subject: Re: [Qemu-devel] [PATCH 05/11] block: update error reporting for bdrv_snapshot_goto() 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) ------enig2EGWFICDSCJCHGHVPUERW Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 04/16/2013 10:05 AM, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina > --- > block.c | 55 ++++++++++++++++++++++++++-------------= -------- > block/qcow2-snapshot.c | 32 +++++++++++++++++++-------- > block/qcow2.h | 8 +++++-- > block/rbd.c | 19 +++++++++++----- > block/sheepdog.c | 33 ++++++++++++++++------------ > include/block/block.h | 8 ++++--- > include/block/block_int.h | 8 ++++--- > qemu-img.c | 20 ++++++++++------- > savevm.c | 21 ++++++++++-------- > 9 files changed, 127 insertions(+), 77 deletions(-) >=20 > + } else if (bs->file) { > drv->bdrv_close(bs); > - ret =3D bdrv_snapshot_goto(bs->file, snapshot_id); > - open_ret =3D drv->bdrv_open(bs, NULL, bs->open_flags); > - if (open_ret < 0) { > + bdrv_snapshot_goto(bs->file, snapshot_id, errp); > + ret =3D drv->bdrv_open(bs, NULL, bs->open_flags); > + if (ret < 0) { > bdrv_delete(bs->file); > bs->drv =3D NULL; > - return open_ret; > + error_setg(errp, "failed to open '%s'", bdrv_get_device_na= me(bs)); Do we still want to try bdrv_open() if bdrv_snapshot_goto() set errp? For that matter, if bdrv_snapshot_goto() _did_ set errp, does error_setg() allow you to overwrite errors, or are you violating constraints? This is another case of partial failure; I'm wondering if we need to eventually tidy this code up to use transaction semantics, so that a loadvm is all-or-none, rather than risking partial failure. > @@ -3410,16 +3410,23 @@ void bdrv_snapshot_delete(BlockDriverState *bs,= > } > =20 > int bdrv_snapshot_list(BlockDriverState *bs, > - QEMUSnapshotInfo **psn_info) > + QEMUSnapshotInfo **psn_info, > + Error **errp) > { > BlockDriver *drv =3D bs->drv; > - if (!drv) > - return -ENOMEDIUM; > - if (drv->bdrv_snapshot_list) > - return drv->bdrv_snapshot_list(bs, psn_info); > - if (bs->file) > - return bdrv_snapshot_list(bs->file, psn_info); > - return -ENOTSUP; > + > + if (!drv) { > + error_setg(errp, "device '%s' has no medium", bdrv_get_device_= name(bs)); > + return 0; > + } else if (drv->bdrv_snapshot_list) { > + return drv->bdrv_snapshot_list(bs, psn_info, errp); > + } else if (bs->file) { > + return bdrv_snapshot_list(bs->file, psn_info, errp); > + } else { > + error_setg(errp, "snapshots are not supported on device '%s'",= > + bdrv_get_device_name(bs)); > + return 0; Why 0 for error? Don't you want to reserve 0 for 'snapshots are supported, but none exist', and use -1 for error instead? Or are we safe treating no medium and no support in the same was as supported but the list is 0-length? > @@ -447,6 +450,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const= char *snapshot_id) > */ > ret =3D qcow2_grow_l1_table(bs, sn->l1_size, true); > if (ret < 0) { > + error_setg_errno(errp, -ret, "fqcow2: ailed to grow L1 table")= ; s/ailed/failed/ > @@ -595,7 +606,9 @@ void qcow2_snapshot_delete(BlockDriverState *bs, > #endif > } > =20 > -int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_t= ab) > +int qcow2_snapshot_list(BlockDriverState *bs, > + QEMUSnapshotInfo **psn_tab, > + Error **errp) > { > BDRVQcowState *s =3D bs->opaque; > QEMUSnapshotInfo *sn_tab, *sn_info; > @@ -604,7 +617,8 @@ int qcow2_snapshot_list(BlockDriverState *bs, QEMUS= napshotInfo **psn_tab) > =20 > if (!s->nb_snapshots) { > *psn_tab =3D NULL; > - return s->nb_snapshots; > + error_setg(errp, "qcow2: there is no snapshot available"); > + return 0; Note that inside the 'if' block, s->nb_snapshots =3D=3D 0, so there is no= change in the return value, but now you are declaring this an error where it previously just left a NULL *psn_tab. Does the caller care? > @@ -913,7 +917,12 @@ static int qemu_rbd_snap_list(BlockDriverState *bs= , > } > } while (snap_count =3D=3D -ERANGE); > =20 > - if (snap_count <=3D 0) { > + if (snap_count < 0) { > + error_setg_errno(errp, -snap_count, "rbd: failed to find snaps= hots"); > + snap_count =3D 0; This is changing the return value from negative to 0. Does the caller ca= re? > +static int sd_snapshot_list(BlockDriverState *bs, > + QEMUSnapshotInfo **psn_tab, > + Error **errp) > { > BDRVSheepdogState *s =3D bs->opaque; > SheepdogReq req; > - int fd, nr =3D 1024, ret, max =3D BITS_TO_LONGS(SD_NR_VDIS) * size= of(long); > + int fd, nr =3D 1024, ret =3D 0, max =3D BITS_TO_LONGS(SD_NR_VDIS) = * sizeof(long); > QEMUSnapshotInfo *sn_tab =3D NULL; > unsigned wlen, rlen; > int found =3D 0; > @@ -1934,7 +1937,7 @@ static int sd_snapshot_list(BlockDriverState *bs,= QEMUSnapshotInfo **psn_tab) > =20 > fd =3D connect_to_sdog(s); > if (fd < 0) { > - ret =3D fd; > + error_setg_errno(errp, -fd, "sd: failed to connect to sdog"); > goto out; Another case of changing the result from -1 to 0; does the caller care? > @@ -2001,7 +2005,8 @@ out: > g_free(vdi_inuse); > =20 > if (ret < 0) { > - return ret; > + error_setg_errno(errp, -ret, "sd: failed to read VDI object");= > + return 0; and again, in the same function. > +++ b/include/block/block_int.h > @@ -155,13 +155,15 @@ struct BlockDriver { > =20 > int (*bdrv_snapshot_create)(BlockDriverState *bs, > QEMUSnapshotInfo *sn_info); > - int (*bdrv_snapshot_goto)(BlockDriverState *bs, > - const char *snapshot_id); > + void (*bdrv_snapshot_goto)(BlockDriverState *bs, > + const char *snapshot_id, > + Error **errp); It would be really nice if we documented the contracts of all these callback functions. > +++ b/qemu-img.c > @@ -1563,10 +1563,13 @@ static void dump_snapshots(BlockDriverState *bs= ) > QEMUSnapshotInfo *sn_tab, *sn; > int nb_sns, i; > char buf[256]; > + Error *local_err =3D NULL; > =20 > - nb_sns =3D bdrv_snapshot_list(bs, &sn_tab); > - if (nb_sns <=3D 0) > + nb_sns =3D bdrv_snapshot_list(bs, &sn_tab, &local_err); > + if (qemu_img_handle_error("qemu-img dump snapshots failed", local_= err) > + || nb_sns =3D=3D 0) { Interesting - you are stating that if no error was reported in local_err, then a return of 0 short-circuits. I was asking about all the places where you changed return semantics; if this is the only caller, then when local_err is set you ignore nb_sns and therefore the return value is irrelevant (and changing from -1 to 0 makes no difference to this code doing a short-circuit). But again, calling that out as a contract, where we can verify that each implementation of the callback function obeys the contract, would make me feel a bit better. > +++ b/savevm.c > @@ -2206,7 +2206,7 @@ static int bdrv_snapshot_find(BlockDriverState *b= s, QEMUSnapshotInfo *sn_info, > return found; > } > =20 > - nb_sns =3D bdrv_snapshot_list(bs, &sn_tab); > + nb_sns =3D bdrv_snapshot_list(bs, &sn_tab, NULL); > if (nb_sns < 0) { > return found; Oops. qemu-img wasn't the only caller. Here, changing -1 to 0 on error return means we fall through instead of returning 0 early. Did you mean for that to happen? Calling with NULL says you don't care about error reporting - is that right? --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2EGWFICDSCJCHGHVPUERW 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/ iQEcBAEBCAAGBQJRbbkaAAoJEKeha0olJ0Nq5+AIAIkvyCKD1EnIuKaxiBLXLh/L HoFfeCz0ynriD4pyfygUH+NanjHv/EN6W40CCbYzXsaXmrilN4RyktxDt1oyF0Vt 73G+Ho2vOibTnaxPFH2iJIizIPdqPpnZavGwdOAvE3rgjBfv85sPur1L1NJ7hCal WWd1b1UnBiARgwtKTkzoClIsg4cAmj1v+24snLesgI2z7CHd13wMW0hqeUtfArYT nVkajLJxjtqgD3Et9bZBvvc1pUsJv5dS3n3l/79Pkd/ej4TrWEVR2AFSwKbGi6pr 6BQqb8AjjKNmLq5mlibMARCGBqD3337NUTEMEmUN4kq5rambahVgVvpK8vG5QGM= =xSkZ -----END PGP SIGNATURE----- ------enig2EGWFICDSCJCHGHVPUERW--