Re: [Qemu-devel] [2.3 PATCH v7 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[Markus Armbruster]

John Snow <address@hidden> writes:

> On 11/26/2014 07:19 AM, Max Reitz wrote:
>> On 2014-11-25 at 20:46, John Snow wrote:
>>> From: Fam Zheng <address@hidden>
>>>
>>> The new command pair is added to manage user created dirty bitmap. The
>>> dirty bitmap's name is mandatory and must be unique for the same device,
>>> but different devices can have bitmaps with the same names.
>>>
>>> The types added to block-core.json will be re-used in future patches
>>> in this series, see:
>>> 'qapi: Add transaction support to block-dirty-bitmap-{add, enable,
>>> disable}'
[...]
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 57910b8..e2fe687 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -1810,6 +1810,69 @@ void qmp_block_set_io_throttle(const char
>>> *device, int64_t bps, int64_t bps_rd,
>>>       aio_context_release(aio_context);
>>>   }
>>> +void qmp_block_dirty_bitmap_add(const char *device, const char *name,
>>> +                                bool has_granularity, int64_t
>>> granularity,
>>> +                                Error **errp)
>>> +{
>>> +    BlockDriverState *bs;
>>> +    Error *local_err = NULL;
>>> +
>>> +    if (!device) {
>>> +        error_setg(errp, "Device to add dirty bitmap to must not be
>>> null");
>>> +        return;
>>> +    }
>>
>> I don't know if checking for that case makes sense, but of course it
>> won't hurt. But...[1]
>>
>>> +
>>> +    bs = bdrv_lookup_bs(device, NULL, &local_err);
>>
>> Fair enough, I'd still like blk_by_name() and blk_bs() more
>> (bdrv_lookup_bs() uses blk_bs(blk_by_name()) for the device name just as
>> bdrv_find() did), but this is at least not a completely trivial wrapper.

When a QMP command deals with a backend as a whole, then it should use
the blk_ API.  When it deals with individual (possibly interior) nodes,
then it has to use the bdrv_ API.  Since blk_ is new, most existing code
doesn't use it, yet.  Beginnings:

6007cdd blockdev: Convert qmp_eject(), qmp_change_blockdev() to BlockBackend
d829a21 block/qapi: Convert qmp_query_block() to BlockBackend

>>> +    if (!bs) {
>>> +        error_propagate(errp, local_err);
>>
>> Simply calling bdrv_lookup_bs(device, NULL, errp); suffices, no need for
>> a local Error object and error_propagate(). But I'm fine with it either
>> way.
>
> I found other cases where we do this in the code, I actually thought
> it was a "style thing," which is why I did it so often.

Cargo cult programming ;-P

Back to serious: I believe a chief reason for bad or ugly error handling
in new code is the many bad and ugly examples in existing code, from
where they get copied into new code by sensible people.

I've been chipping away at the bad examples.  Don't hold your breath.

> I'll happily cut it out.

Yes, please.

[...]