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

[Eric Blake]

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.

> 
> 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