Re: [Qemu-devel] [PATCH 02/11] block: update error reporting for bdrv_snapshot_delete() and related functions

[Kevin Wolf]

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 <address@hidden>
> >> ---
> >>  
> >> -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