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 <phrd...@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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature