qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] block: nbd: Fix dirty bitmap context name


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH] block: nbd: Fix dirty bitmap context name
Date: Thu, 19 Dec 2019 14:23:49 +0000

19.12.2019 17:04, Kevin Wolf wrote:
> Am 19.12.2019 um 14:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Ahh, I see, it's documented as
>>
>> +# @bitmap: Also export the dirty bitmap reachable from @device, so the
>> +#          NBD client can use NBD_OPT_SET_META_CONTEXT with
>> +#          "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0)
>>
>> and it is logical to assume that export name (which is @name argument) is
>> mentioned. But we never mentioned it. This is just documented after
>> removed experimenatl command x-nbd-server-add-bitmap,
>>
>> look at
>>
>> commit 7dc570b3806e5b0a4c9219061556ed5a4a0de80c
>> Author: Eric Blake <address@hidden>
>> Date:   Fri Jan 11 13:47:18 2019 -0600
>>
>>       nbd: Remove x-nbd-server-add-bitmap
>>
>> ...
>>
>> -# @bitmap-export-name: How the bitmap will be seen by nbd clients
>> -#                      (default @bitmap)
>> -#
>> -# Note: the client must use NBD_OPT_SET_META_CONTEXT with a query of
>> -# "qemu:dirty-bitmap:NAME" (where NAME matches @bitmap-export-name) to 
>> access
>> -# the exposed bitmap.
>>
>>
>> So, this "NAME" is saved and now looks incorrect. What should be fixed, is 
>> Qapi
>> documentation.
> 
> Hm, I don't know these interfaces very well, but from you explanation it
> looks to me as if having a bitmap name made sense with
> x-nbd-server-add-bitmap because it could be called more than once for
> exporting multiple bitmaps.
> 
> But now, we have only nbd-server-add, which takes a single bitmap name.
> As we don't have to distinguish multiple bitmaps any more, wouldn't it
> make more sense to use "qemu:dirty-bitmap" without any other
> information? Both export name and bitmap name are already identified by
> the connection.

I think, it will a bit in conflict with already documented

* "qemu:dirty-bitmap:" - returns list of all available dirty-bitmap
                          metadata contexts.

So, if we want some "default dirty bitmap", we'd better use something different
from just dirty-bitmap. For example,

    "qemu:default-dirty-bitmap"

> But if we have to have something there, using the bitmap name (which may
> or may not be the same as used in the image file) makes a little more
> sense because it makes the interface extensible for the case that we
> ever want to re-introduce an nbd-server-add-bitmap.

Agree


> 
> (By the way, even if the patch were correct, it lacks a Signed-off-by.)
> 
> Kevin
> 


-- 
Best regards,
Vladimir

reply via email to

[Prev in Thread] Current Thread [Next in Thread]