| Message ID | 6e22db8528d4a801af11b37d42a350eab9633636.1366127809.git.phrdina@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/16/2013 10:05 AM, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > block.c | 22 ++++++++++++++-------- > block/qcow2-snapshot.c | 21 +++++++++++++++------ > block/qcow2.h | 4 +++- > block/rbd.c | 11 ++++++++--- > block/sheepdog.c | 6 ++++-- > include/block/block.h | 4 +++- > include/block/block_int.h | 4 +++- > qemu-img.c | 9 ++++----- > savevm.c | 26 ++++++++++---------------- > 9 files changed, 64 insertions(+), 43 deletions(-) > > @@ -539,7 +541,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) > /* Search the snapshot */ > snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id); > if (snapshot_index < 0) { > - return -ENOENT; > + error_setg(errp, "qcow2: failed to find snapshot '%s' by id or name", > + snapshot_id); > + return; I haven't looked if you changed this later in the series to have find_snapshot_by_id_or_name take an errp parameter, at which point you could refactor the error reporting to that point in the stack. That might be a nice followup, but this patch is okay as-is. > +++ b/block/sheepdog.c > @@ -1908,10 +1908,12 @@ out: > return ret; > } > > -static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) > +static void sd_snapshot_delete(BlockDriverState *bs, > + const char *snapshot_id, > + Error **errp) > { > /* FIXME: Delete specified snapshot id. */ > - return 0; > + return; No semantic change; but a trailing return; in a void function is not necessary, and an empty body would suffice. On the other hand, claiming success when we didn't delete anything feels wrong. Should we change this function to call error_setg() and warn the user that deletion is not supported at this time? If so, that's probably better done as a separate commit. As my findings are best addressed in other patches, I'm okay with this patch as-is, and you can use: Reviewed-by: Eric Blake <eblake@redhat.com>
Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben: > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > block.c | 22 ++++++++++++++-------- > block/qcow2-snapshot.c | 21 +++++++++++++++------ > block/qcow2.h | 4 +++- > block/rbd.c | 11 ++++++++--- > block/sheepdog.c | 6 ++++-- > include/block/block.h | 4 +++- > include/block/block_int.h | 4 +++- > qemu-img.c | 9 ++++----- > savevm.c | 26 ++++++++++---------------- > 9 files changed, 64 insertions(+), 43 deletions(-) > > diff --git a/block.c b/block.c > index 4ad663d..fb065e6 100644 > --- a/block.c > +++ b/block.c > @@ -3391,16 +3391,22 @@ int bdrv_snapshot_goto(BlockDriverState *bs, > return -ENOTSUP; > } > > -int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) > +void bdrv_snapshot_delete(BlockDriverState *bs, > + const char *snapshot_id, > + Error **errp) > { > BlockDriver *drv = 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)); I don't think the device name is useful here. It's always the device that the user has specified in the monitor command. Also, please start error messages with a capital letter. (This applies to the whole patch, and probably also other patches in this series) > + } 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)); Same thing with the device name here. > + } > } > > int bdrv_snapshot_list(BlockDriverState *bs, > diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c > index 992a5c8..2ebeadc 100644 > --- a/block/qcow2-snapshot.c > +++ b/block/qcow2-snapshot.c > @@ -530,7 +530,9 @@ fail: > return ret; > } > > -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) > +void qcow2_snapshot_delete(BlockDriverState *bs, > + const char *snapshot_id, > + Error **errp) > { > BDRVQcowState *s = bs->opaque; > QCowSnapshot sn; > @@ -539,7 +541,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) > /* Search the snapshot */ > snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id); > if (snapshot_index < 0) { > - return -ENOENT; > + error_setg(errp, "qcow2: failed to find snapshot '%s' by id or name", > + snapshot_id); > + return; > } > sn = s->snapshots[snapshot_index]; > > @@ -550,7 +554,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) > s->nb_snapshots--; > ret = qcow2_write_snapshots(bs); > if (ret < 0) { > - return ret; > + error_setg_errno(errp, -ret, "qcow2: failed to remove snapshot '%s' " > + "from snapshot list", sn.name); > + return; > } > > /* > @@ -567,14 +573,18 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) > ret = qcow2_update_snapshot_refcount(bs, sn.l1_table_offset, > sn.l1_size, -1); > if (ret < 0) { > - return ret; > + error_setg_errno(errp, -ret, > + "qcow2: failed to update snapshot refcount"); I'd make it "qcow2: Failed to update refcounts". It's the refcounts of all clusters referred to by the snapshot's L1 table, not of the snapshot itself. > + return; > } > qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t)); > > /* must update the copied flag on the current cluster offsets */ > ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 0); > if (ret < 0) { > - return ret; > + error_setg_errno(errp, -ret, > + "qcow2: failed to update snapshot refcount"); "qcow2: Failed to update cluster flags" > + return; > } > > #ifdef DEBUG_ALLOC > @@ -583,7 +593,6 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) > qcow2_check_refcounts(bs, &result, 0); > } > #endif > - return 0; > } > > int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) > diff --git a/block/qcow2.h b/block/qcow2.h > index 9421843..dbd332d 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -384,7 +384,9 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors); > /* qcow2-snapshot.c functions */ > int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info); > int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id); > -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id); > +void qcow2_snapshot_delete(BlockDriverState *bs, > + const char *snapshot_id, > + Error **errp); > int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab); > int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name); > > diff --git a/block/rbd.c b/block/rbd.c > index 141b488..c10edbf 100644 > --- a/block/rbd.c > +++ b/block/rbd.c > @@ -871,14 +871,19 @@ static int qemu_rbd_snap_create(BlockDriverState *bs, > return 0; > } > > -static int qemu_rbd_snap_remove(BlockDriverState *bs, > - const char *snapshot_name) > +static void qemu_rbd_snap_remove(BlockDriverState *bs, > + const char *snapshot_name, > + Error **errp) > { > BDRVRBDState *s = bs->opaque; > int r; > > r = rbd_snap_remove(s->image, snapshot_name); > - return r; > + if (r < 0) { > + error_setg_errno(errp, -r, "rbd: failed to remove snapshot '%s' on " > + "device '%s'", snapshot_name, > + bdrv_get_device_name(bs)); Remove the device name. You didn't have it in the qcow2 errors either. > + } > } > > static int qemu_rbd_snap_rollback(BlockDriverState *bs, > diff --git a/block/sheepdog.c b/block/sheepdog.c > index 1c5b532..270fa64 100644 > --- a/block/sheepdog.c > +++ b/block/sheepdog.c > @@ -1908,10 +1908,12 @@ out: > return ret; > } > > -static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) > +static void sd_snapshot_delete(BlockDriverState *bs, > + const char *snapshot_id, > + Error **errp) > { > /* FIXME: Delete specified snapshot id. */ > - return 0; > + return; > } Looks like there should be some error_setg("Deleting snapshots not supported yet by sheepdog"), possibly in a separate patch. > static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) > diff --git a/include/block/block.h b/include/block/block.h > index ebd9512..db8c6aa 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -337,7 +337,9 @@ int bdrv_snapshot_create(BlockDriverState *bs, > QEMUSnapshotInfo *sn_info); > int bdrv_snapshot_goto(BlockDriverState *bs, > const char *snapshot_id); > -int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id); > +void bdrv_snapshot_delete(BlockDriverState *bs, > + const char *snapshot_id, > + Error **errp); > int bdrv_snapshot_list(BlockDriverState *bs, > QEMUSnapshotInfo **psn_info); > int bdrv_snapshot_load_tmp(BlockDriverState *bs, > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 458cde3..53c52bb 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -157,7 +157,9 @@ struct BlockDriver { > QEMUSnapshotInfo *sn_info); > int (*bdrv_snapshot_goto)(BlockDriverState *bs, > const char *snapshot_id); > - int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id); > + void (*bdrv_snapshot_delete)(BlockDriverState *bs, > + const char *snapshot_id, > + Error **errp); > int (*bdrv_snapshot_list)(BlockDriverState *bs, > QEMUSnapshotInfo **psn_info); > int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs, > diff --git a/qemu-img.c b/qemu-img.c > index dbacdb7..4828fe4 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1936,6 +1936,7 @@ static int img_snapshot(int argc, char **argv) > { > BlockDriverState *bs; > QEMUSnapshotInfo sn; > + Error *local_err = NULL; > char *filename, *snapshot_name = NULL; > int c, ret = 0, bdrv_oflags; > int action = 0; > @@ -2033,11 +2034,9 @@ static int img_snapshot(int argc, char **argv) > break; > > case SNAPSHOT_DELETE: > - ret = bdrv_snapshot_delete(bs, snapshot_name); > - if (ret) { > - error_report("Could not delete snapshot '%s': %d (%s)", > - snapshot_name, ret, strerror(-ret)); > - } > + bdrv_snapshot_delete(bs, snapshot_name, &local_err); > + ret = qemu_img_handle_error("qemu-img snapshot delete failed", > + local_err); Here again "qemu-img snapshot delete failed" is redundant. If this is the command that I typed and I get an error, then it's obvious that it's this command that failed. > break; > } > > diff --git a/savevm.c b/savevm.c > index 53515cb..6af84fd 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -2225,18 +2225,17 @@ static int del_existing_snapshots(Monitor *mon, const char *name) > { > BlockDriverState *bs; > QEMUSnapshotInfo sn1, *snapshot = &sn1; > - int ret; > + Error *local_err = NULL; > > bs = NULL; > while ((bs = bdrv_next(bs))) { > if (bdrv_can_snapshot(bs) && > bdrv_snapshot_find(bs, snapshot, name) >= 0) > { > - ret = bdrv_snapshot_delete(bs, name); > - if (ret < 0) { > - monitor_printf(mon, > - "Error while deleting snapshot on '%s'\n", > - bdrv_get_device_name(bs)); > + bdrv_snapshot_delete(bs, name, &local_err); > + if (error_is_set(&local_err)) { > + monitor_printf(mon, "%s\n", error_get_pretty(local_err)); Here the additional monitor_printf() actually had meaningful additional information. Deleting an old snapshot is an implicitly taken action and not explicitly requested, so an error message should indicate that it happened during the deletion. Maybe something like: qerror_report(ERROR_CLASS_GENERIC_ERROR, "Error while deleting old snapshot on device '%s': %s", bdrv_get_device_name(bs), error_get_pretty(local_err)); > + error_free(local_err); > return -1; > } > } > @@ -2450,7 +2449,7 @@ int load_vmstate(const char *name) > void do_delvm(Monitor *mon, const QDict *qdict) > { > BlockDriverState *bs, *bs1; > - int ret; > + Error *local_err = NULL; > const char *name = qdict_get_str(qdict, "name"); > > bs = bdrv_snapshots(); > @@ -2462,15 +2461,10 @@ void do_delvm(Monitor *mon, const QDict *qdict) > bs1 = NULL; > while ((bs1 = bdrv_next(bs1))) { > if (bdrv_can_snapshot(bs1)) { > - ret = bdrv_snapshot_delete(bs1, name); > - if (ret < 0) { > - if (ret == -ENOTSUP) > - monitor_printf(mon, > - "Snapshots not supported on device '%s'\n", > - bdrv_get_device_name(bs1)); > - else > - monitor_printf(mon, "Error %d while deleting snapshot on " > - "'%s'\n", ret, bdrv_get_device_name(bs1)); > + bdrv_snapshot_delete(bs1, name, &local_err); > + if (error_is_set(&local_err)) { > + monitor_printf(mon, "%s\n", error_get_pretty(local_err)); Either something like above, indicating the device name on which bdrv_snapshot_delete() failed, or qerror_report_err(). > + error_free(local_err); > } > } > } Kevin
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 <phrdina@redhat.com> >> --- >> >> -int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) >> +void bdrv_snapshot_delete(BlockDriverState *bs, >> + const char *snapshot_id, >> + Error **errp) >> { >> BlockDriver *drv = 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)); > > 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 name. > > 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. > >> + } 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)); > > 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.
On 18.4.2013 14:55, Kevin Wolf wrote: > Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben: >> >> /* >> @@ -567,14 +573,18 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) >> ret = qcow2_update_snapshot_refcount(bs, sn.l1_table_offset, >> sn.l1_size, -1); >> if (ret < 0) { >> - return ret; >> + error_setg_errno(errp, -ret, >> + "qcow2: failed to update snapshot refcount"); > > I'd make it "qcow2: Failed to update refcounts". It's the refcounts of > all clusters referred to by the snapshot's L1 table, not of the snapshot > itself. Thanks for correction. I'm not that much familiar with qcow2 internals. > >> + return; >> } >> qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t)); >> >> /* must update the copied flag on the current cluster offsets */ >> ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 0); >> if (ret < 0) { >> - return ret; >> + error_setg_errno(errp, -ret, >> + "qcow2: failed to update snapshot refcount"); > > "qcow2: Failed to update cluster flags" Again thanks for correction. > >> + return; >> } >> >> #ifdef DEBUG_ALLOC >> @@ -583,7 +593,6 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) >> qcow2_check_refcounts(bs, &result, 0); >> } >> #endif >> - return 0; >> } >> >> int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) >> diff --git a/block/qcow2.h b/block/qcow2.h >> index 9421843..dbd332d 100644 >> --- a/block/qcow2.h >> +++ b/block/qcow2.h >> @@ -384,7 +384,9 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors); >> /* qcow2-snapshot.c functions */ >> int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info); >> int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id); >> -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id); >> +void qcow2_snapshot_delete(BlockDriverState *bs, >> + const char *snapshot_id, >> + Error **errp); >> int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab); >> int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name); >> >> diff --git a/block/rbd.c b/block/rbd.c >> index 141b488..c10edbf 100644 >> --- a/block/rbd.c >> +++ b/block/rbd.c >> @@ -871,14 +871,19 @@ static int qemu_rbd_snap_create(BlockDriverState *bs, >> return 0; >> } >> >> -static int qemu_rbd_snap_remove(BlockDriverState *bs, >> - const char *snapshot_name) >> +static void qemu_rbd_snap_remove(BlockDriverState *bs, >> + const char *snapshot_name, >> + Error **errp) >> { >> BDRVRBDState *s = bs->opaque; >> int r; >> >> r = rbd_snap_remove(s->image, snapshot_name); >> - return r; >> + if (r < 0) { >> + error_setg_errno(errp, -r, "rbd: failed to remove snapshot '%s' on " >> + "device '%s'", snapshot_name, >> + bdrv_get_device_name(bs)); > > Remove the device name. You didn't have it in the qcow2 errors either. Or maybe I should also add the device name in the qcow2 errors, because as Eric write to you these function are used also for vm-snapshot-delete and devlm and knowing which device failed is important. > >> + } >> } >> >> static int qemu_rbd_snap_rollback(BlockDriverState *bs, >> diff --git a/block/sheepdog.c b/block/sheepdog.c >> index 1c5b532..270fa64 100644 >> --- a/block/sheepdog.c >> +++ b/block/sheepdog.c >> @@ -1908,10 +1908,12 @@ out: >> return ret; >> } >> --- >> diff --git a/savevm.c b/savevm.c >> index 53515cb..6af84fd 100644 >> --- a/savevm.c >> +++ b/savevm.c >> @@ -2225,18 +2225,17 @@ static int del_existing_snapshots(Monitor *mon, const char *name) >> { >> BlockDriverState *bs; >> QEMUSnapshotInfo sn1, *snapshot = &sn1; >> - int ret; >> + Error *local_err = NULL; >> >> bs = NULL; >> while ((bs = bdrv_next(bs))) { >> if (bdrv_can_snapshot(bs) && >> bdrv_snapshot_find(bs, snapshot, name) >= 0) >> { >> - ret = bdrv_snapshot_delete(bs, name); >> - if (ret < 0) { >> - monitor_printf(mon, >> - "Error while deleting snapshot on '%s'\n", >> - bdrv_get_device_name(bs)); >> + bdrv_snapshot_delete(bs, name, &local_err); >> + if (error_is_set(&local_err)) { >> + monitor_printf(mon, "%s\n", error_get_pretty(local_err)); > > Here the additional monitor_printf() actually had meaningful additional > information. Deleting an old snapshot is an implicitly taken action and > not explicitly requested, so an error message should indicate that it > happened during the deletion. Maybe something like: Function del_existing_snapshots will be anyway dropped later in patch series so this has no actual value. But I should probably make something similar to this for HMP command savevm. Thanks > > qerror_report(ERROR_CLASS_GENERIC_ERROR, > "Error while deleting old snapshot on device '%s': %s", > bdrv_get_device_name(bs), error_get_pretty(local_err)); > >> + error_free(local_err); >> return -1; >> } >> } >> @@ -2450,7 +2449,7 @@ int load_vmstate(const char *name) >> void do_delvm(Monitor *mon, const QDict *qdict) >> { >> BlockDriverState *bs, *bs1; >> - int ret; >> + Error *local_err = NULL; >> const char *name = qdict_get_str(qdict, "name"); >> >> bs = bdrv_snapshots(); >> @@ -2462,15 +2461,10 @@ void do_delvm(Monitor *mon, const QDict *qdict) >> bs1 = NULL; >> while ((bs1 = bdrv_next(bs1))) { >> if (bdrv_can_snapshot(bs1)) { >> - ret = bdrv_snapshot_delete(bs1, name); >> - if (ret < 0) { >> - if (ret == -ENOTSUP) >> - monitor_printf(mon, >> - "Snapshots not supported on device '%s'\n", >> - bdrv_get_device_name(bs1)); >> - else >> - monitor_printf(mon, "Error %d while deleting snapshot on " >> - "'%s'\n", ret, bdrv_get_device_name(bs1)); >> + bdrv_snapshot_delete(bs1, name, &local_err); >> + if (error_is_set(&local_err)) { >> + monitor_printf(mon, "%s\n", error_get_pretty(local_err)); > > Either something like above, indicating the device name on which > bdrv_snapshot_delete() failed, or qerror_report_err(). Like I wrote few lines above, I should add the device name into all errors also in qcow2, rbd and sheepdog. This also applies for bdrv_snapshot_goto/create/list. Pavel > >> + error_free(local_err); >> } >> } >> } > > Kevin >
Am 18.04.2013 um 15:19 hat Pavel Hrdina geschrieben: > On 18.4.2013 14:55, Kevin Wolf wrote: > >Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben: > >>--- a/block/rbd.c > >>+++ b/block/rbd.c > >>@@ -871,14 +871,19 @@ static int qemu_rbd_snap_create(BlockDriverState *bs, > >> return 0; > >> } > >> > >>-static int qemu_rbd_snap_remove(BlockDriverState *bs, > >>- const char *snapshot_name) > >>+static void qemu_rbd_snap_remove(BlockDriverState *bs, > >>+ const char *snapshot_name, > >>+ Error **errp) > >> { > >> BDRVRBDState *s = bs->opaque; > >> int r; > >> > >> r = rbd_snap_remove(s->image, snapshot_name); > >>- return r; > >>+ if (r < 0) { > >>+ error_setg_errno(errp, -r, "rbd: failed to remove snapshot '%s' on " > >>+ "device '%s'", snapshot_name, > >>+ bdrv_get_device_name(bs)); > > > >Remove the device name. You didn't have it in the qcow2 errors either. > > Or maybe I should also add the device name in the qcow2 errors, > because as Eric write to you these function are used also for > vm-snapshot-delete and devlm and knowing which device failed is > important. I don't think it's the right approach to let the lowest layer add all information that is potentially useful in some contexts. It should only consider what information is essential for its direct caller. You are right that in delvm multiple devices can be affected. This is why delvm should amend the error message with any information that is required for the error message to be helpful in the context of delvm. This is essentially what I suggested here: > >>@@ -2225,18 +2225,17 @@ static int del_existing_snapshots(Monitor *mon, const char *name) > >> { > >> BlockDriverState *bs; > >> QEMUSnapshotInfo sn1, *snapshot = &sn1; > >>- int ret; > >>+ Error *local_err = NULL; > >> > >> bs = NULL; > >> while ((bs = bdrv_next(bs))) { > >> if (bdrv_can_snapshot(bs) && > >> bdrv_snapshot_find(bs, snapshot, name) >= 0) > >> { > >>- ret = bdrv_snapshot_delete(bs, name); > >>- if (ret < 0) { > >>- monitor_printf(mon, > >>- "Error while deleting snapshot on '%s'\n", > >>- bdrv_get_device_name(bs)); > >>+ bdrv_snapshot_delete(bs, name, &local_err); > >>+ if (error_is_set(&local_err)) { > >>+ monitor_printf(mon, "%s\n", error_get_pretty(local_err)); > > > >Here the additional monitor_printf() actually had meaningful additional > >information. Deleting an old snapshot is an implicitly taken action and > >not explicitly requested, so an error message should indicate that it > >happened during the deletion. Maybe something like: > > Function del_existing_snapshots will be anyway dropped later in > patch series so this has no actual value. But I should probably make > something similar to this for HMP command savevm. > > Thanks > > > > >qerror_report(ERROR_CLASS_GENERIC_ERROR, > > "Error while deleting old snapshot on device '%s': %s", > > bdrv_get_device_name(bs), error_get_pretty(local_err)); See how I added the device name and the context of the error ("while deleteing the old snapshot") only in the place where it's actually necessary for the caller to understand the error? By the way, I'm also unsure if the "qcow2:" or "rbd:" prefix is actually helpful. Kevin
Am 18.04.2013 um 15:09 hat Eric Blake geschrieben: > 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 <phrdina@redhat.com> > >> --- > >> > >> -int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) > >> +void bdrv_snapshot_delete(BlockDriverState *bs, > >> + const char *snapshot_id, > >> + Error **errp) > >> { > >> BlockDriver *drv = 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)); > > > > 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 name. No, we're talking about bdrv_snapshot_delete(). Talking about vm-snapshot-delete in this patch would be a layering violation. If it's important for vm-snapshot-delete to have the device name in the error message (and I agree it is when deleting multiple snapshots), then qmp_vm_snapshot_delete() should add that information. > > > > 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 don't pipe it through wc but look at the output, you'll see that many of these start with a lower case device, module or parameter name, which of course wouldn't be right with upper case. So far I think the block layer messages are pretty consistently upper case where it makes sense. Kevin
diff --git a/block.c b/block.c index 4ad663d..fb065e6 100644 --- a/block.c +++ b/block.c @@ -3391,16 +3391,22 @@ int bdrv_snapshot_goto(BlockDriverState *bs, return -ENOTSUP; } -int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) +void bdrv_snapshot_delete(BlockDriverState *bs, + const char *snapshot_id, + Error **errp) { BlockDriver *drv = 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)); + } 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)); + } } int bdrv_snapshot_list(BlockDriverState *bs, diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index 992a5c8..2ebeadc 100644 --- a/block/qcow2-snapshot.c +++ b/block/qcow2-snapshot.c @@ -530,7 +530,9 @@ fail: return ret; } -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) +void qcow2_snapshot_delete(BlockDriverState *bs, + const char *snapshot_id, + Error **errp) { BDRVQcowState *s = bs->opaque; QCowSnapshot sn; @@ -539,7 +541,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) /* Search the snapshot */ snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id); if (snapshot_index < 0) { - return -ENOENT; + error_setg(errp, "qcow2: failed to find snapshot '%s' by id or name", + snapshot_id); + return; } sn = s->snapshots[snapshot_index]; @@ -550,7 +554,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) s->nb_snapshots--; ret = qcow2_write_snapshots(bs); if (ret < 0) { - return ret; + error_setg_errno(errp, -ret, "qcow2: failed to remove snapshot '%s' " + "from snapshot list", sn.name); + return; } /* @@ -567,14 +573,18 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) ret = qcow2_update_snapshot_refcount(bs, sn.l1_table_offset, sn.l1_size, -1); if (ret < 0) { - return ret; + error_setg_errno(errp, -ret, + "qcow2: failed to update snapshot refcount"); + return; } qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t)); /* must update the copied flag on the current cluster offsets */ ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 0); if (ret < 0) { - return ret; + error_setg_errno(errp, -ret, + "qcow2: failed to update snapshot refcount"); + return; } #ifdef DEBUG_ALLOC @@ -583,7 +593,6 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) qcow2_check_refcounts(bs, &result, 0); } #endif - return 0; } int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) diff --git a/block/qcow2.h b/block/qcow2.h index 9421843..dbd332d 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -384,7 +384,9 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors); /* qcow2-snapshot.c functions */ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info); int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id); -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id); +void qcow2_snapshot_delete(BlockDriverState *bs, + const char *snapshot_id, + Error **errp); int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab); int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name); diff --git a/block/rbd.c b/block/rbd.c index 141b488..c10edbf 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -871,14 +871,19 @@ static int qemu_rbd_snap_create(BlockDriverState *bs, return 0; } -static int qemu_rbd_snap_remove(BlockDriverState *bs, - const char *snapshot_name) +static void qemu_rbd_snap_remove(BlockDriverState *bs, + const char *snapshot_name, + Error **errp) { BDRVRBDState *s = bs->opaque; int r; r = rbd_snap_remove(s->image, snapshot_name); - return r; + if (r < 0) { + error_setg_errno(errp, -r, "rbd: failed to remove snapshot '%s' on " + "device '%s'", snapshot_name, + bdrv_get_device_name(bs)); + } } static int qemu_rbd_snap_rollback(BlockDriverState *bs, diff --git a/block/sheepdog.c b/block/sheepdog.c index 1c5b532..270fa64 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -1908,10 +1908,12 @@ out: return ret; } -static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) +static void sd_snapshot_delete(BlockDriverState *bs, + const char *snapshot_id, + Error **errp) { /* FIXME: Delete specified snapshot id. */ - return 0; + return; } static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) diff --git a/include/block/block.h b/include/block/block.h index ebd9512..db8c6aa 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -337,7 +337,9 @@ int bdrv_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info); int bdrv_snapshot_goto(BlockDriverState *bs, const char *snapshot_id); -int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id); +void bdrv_snapshot_delete(BlockDriverState *bs, + const char *snapshot_id, + Error **errp); int bdrv_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_info); int bdrv_snapshot_load_tmp(BlockDriverState *bs, diff --git a/include/block/block_int.h b/include/block/block_int.h index 458cde3..53c52bb 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -157,7 +157,9 @@ struct BlockDriver { QEMUSnapshotInfo *sn_info); int (*bdrv_snapshot_goto)(BlockDriverState *bs, const char *snapshot_id); - int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id); + void (*bdrv_snapshot_delete)(BlockDriverState *bs, + const char *snapshot_id, + Error **errp); int (*bdrv_snapshot_list)(BlockDriverState *bs, QEMUSnapshotInfo **psn_info); int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs, diff --git a/qemu-img.c b/qemu-img.c index dbacdb7..4828fe4 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1936,6 +1936,7 @@ static int img_snapshot(int argc, char **argv) { BlockDriverState *bs; QEMUSnapshotInfo sn; + Error *local_err = NULL; char *filename, *snapshot_name = NULL; int c, ret = 0, bdrv_oflags; int action = 0; @@ -2033,11 +2034,9 @@ static int img_snapshot(int argc, char **argv) break; case SNAPSHOT_DELETE: - ret = bdrv_snapshot_delete(bs, snapshot_name); - if (ret) { - error_report("Could not delete snapshot '%s': %d (%s)", - snapshot_name, ret, strerror(-ret)); - } + bdrv_snapshot_delete(bs, snapshot_name, &local_err); + ret = qemu_img_handle_error("qemu-img snapshot delete failed", + local_err); break; } diff --git a/savevm.c b/savevm.c index 53515cb..6af84fd 100644 --- a/savevm.c +++ b/savevm.c @@ -2225,18 +2225,17 @@ static int del_existing_snapshots(Monitor *mon, const char *name) { BlockDriverState *bs; QEMUSnapshotInfo sn1, *snapshot = &sn1; - int ret; + Error *local_err = NULL; bs = NULL; while ((bs = bdrv_next(bs))) { if (bdrv_can_snapshot(bs) && bdrv_snapshot_find(bs, snapshot, name) >= 0) { - ret = bdrv_snapshot_delete(bs, name); - if (ret < 0) { - monitor_printf(mon, - "Error while deleting snapshot on '%s'\n", - bdrv_get_device_name(bs)); + bdrv_snapshot_delete(bs, name, &local_err); + if (error_is_set(&local_err)) { + monitor_printf(mon, "%s\n", error_get_pretty(local_err)); + error_free(local_err); return -1; } } @@ -2450,7 +2449,7 @@ int load_vmstate(const char *name) void do_delvm(Monitor *mon, const QDict *qdict) { BlockDriverState *bs, *bs1; - int ret; + Error *local_err = NULL; const char *name = qdict_get_str(qdict, "name"); bs = bdrv_snapshots(); @@ -2462,15 +2461,10 @@ void do_delvm(Monitor *mon, const QDict *qdict) bs1 = NULL; while ((bs1 = bdrv_next(bs1))) { if (bdrv_can_snapshot(bs1)) { - ret = bdrv_snapshot_delete(bs1, name); - if (ret < 0) { - if (ret == -ENOTSUP) - monitor_printf(mon, - "Snapshots not supported on device '%s'\n", - bdrv_get_device_name(bs1)); - else - monitor_printf(mon, "Error %d while deleting snapshot on " - "'%s'\n", ret, bdrv_get_device_name(bs1)); + bdrv_snapshot_delete(bs1, name, &local_err); + if (error_is_set(&local_err)) { + monitor_printf(mon, "%s\n", error_get_pretty(local_err)); + error_free(local_err); } } }
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- block.c | 22 ++++++++++++++-------- block/qcow2-snapshot.c | 21 +++++++++++++++------ block/qcow2.h | 4 +++- block/rbd.c | 11 ++++++++--- block/sheepdog.c | 6 ++++-- include/block/block.h | 4 +++- include/block/block_int.h | 4 +++- qemu-img.c | 9 ++++----- savevm.c | 26 ++++++++++---------------- 9 files changed, 64 insertions(+), 43 deletions(-)