[PATCH] qapi/block: fix nbd-server-add spec

[PATCH] qapi/block: fix nbd-server-add spec

[Vladimir Sementsov-Ogievskiy]

"NAME" here may be interpreted like it should match @name, which is
export name. But it was never mentioned in such way. Make it obvious,
that actual "<dirty-bitmap-export-name>" (see docs/interop/nbd.txt)
will match @bitmap parameter.

Fixes: 5fcbeb06812685a2
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

Hi all.

This patch follows discussion on Nir's patch
 [PATCH] block: nbd: Fix dirty bitmap context name
 ( https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg04309.html )

Let's just fix qapi spec now. 

 qapi/block.json | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qapi/block.json b/qapi/block.json
index 145c268bb6..8042ef78f0 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -255,7 +255,8 @@
 
 # @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)
+#          "qemu:dirty-bitmap:BITMAP" to inspect the bitmap (BITMAP here
+#          matches @bitmap parameter). (since 4.0)
 #
 # Returns: error if the server is not running, or export with the same name
 #          already exists.
-- 
2.21.0

Re: [PATCH] qapi/block: fix nbd-server-add spec

[Nir Soffer]

On Thu, Dec 19, 2019 at 4:34 PM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
>
> "NAME" here may be interpreted like it should match @name, which is
> export name. But it was never mentioned in such way. Make it obvious,
> that actual "<dirty-bitmap-export-name>" (see docs/interop/nbd.txt)
> will match @bitmap parameter.

But this is wrong, dirty-bitmap-export-name does not mean the actual bitmap
name but the name exposed to the NBD client, which can be anything.

> Fixes: 5fcbeb06812685a2
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> Hi all.
>
> This patch follows discussion on Nir's patch
>  [PATCH] block: nbd: Fix dirty bitmap context name
>  ( https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg04309.html )
>
> Let's just fix qapi spec now.

But qapi documents a better behavior for users. We should fix the code instead
to mach the docs.

With this we still have the issue of leaking internal bitmap name to
users who do not
control the name, and do not care about it.

>  qapi/block.json | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/block.json b/qapi/block.json
> index 145c268bb6..8042ef78f0 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -255,7 +255,8 @@
>
>  # @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)
> +#          "qemu:dirty-bitmap:BITMAP" to inspect the bitmap (BITMAP here
> +#          matches @bitmap parameter). (since 4.0)
>  #
>  # Returns: error if the server is not running, or export with the same name
>  #          already exists.
> --
> 2.21.0
>

Re: [PATCH] qapi/block: fix nbd-server-add spec

[Vladimir Sementsov-Ogievskiy]

19.12.2019 17:42, Nir Soffer wrote:
> On Thu, Dec 19, 2019 at 4:34 PM Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com> wrote:
>>
>> "NAME" here may be interpreted like it should match @name, which is
>> export name. But it was never mentioned in such way. Make it obvious,
>> that actual "<dirty-bitmap-export-name>" (see docs/interop/nbd.txt)
>> will match @bitmap parameter.
> 
> But this is wrong, dirty-bitmap-export-name does not mean the actual bitmap
> name but the name exposed to the NBD client, which can be anything.

Yes. What is wrong? It can be enything. Currently by default it is bitmap name.
It purely documented (okay, even confusingly documented), but it was so since
4.0. And existing users obviously knows how it work (otherwise, they can't use
the feature)

So, I think it's OK to fix spec to directly show implementation, that was here
since feature introducing.

> 
>> Fixes: 5fcbeb06812685a2
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> Hi all.
>>
>> This patch follows discussion on Nir's patch
>>   [PATCH] block: nbd: Fix dirty bitmap context name
>>   ( https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg04309.html )
>>
>> Let's just fix qapi spec now.
> 
> But qapi documents a better behavior for users. We should fix the code instead
> to mach the docs.

1. Using disk name as a bitmap name is a bad behavior, as they are completely
different concepts. Especially keeping in mind that user already knows disk name anyway
and no reason to write this export name inside metadata context of this export.

2. It's not directly documented. You assume that NAME == @name. I understand that
it may be assumed.. But it's not documented.

3. It's never worked like you write. So if we change the behavior, we'll break
existing users.


> 
> With this we still have the issue of leaking internal bitmap name to
> users who do not
> control the name, and do not care about it.
> 
>>   qapi/block.json | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/block.json b/qapi/block.json
>> index 145c268bb6..8042ef78f0 100644
>> --- a/qapi/block.json
>> +++ b/qapi/block.json
>> @@ -255,7 +255,8 @@
>>
>>   # @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)
>> +#          "qemu:dirty-bitmap:BITMAP" to inspect the bitmap (BITMAP here
>> +#          matches @bitmap parameter). (since 4.0)
>>   #
>>   # Returns: error if the server is not running, or export with the same name
>>   #          already exists.
>> --
>> 2.21.0
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH] qapi/block: fix nbd-server-add spec

[Nir Soffer]

On Thu, Dec 19, 2019 at 5:00 PM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
>
> 19.12.2019 17:42, Nir Soffer wrote:
> > On Thu, Dec 19, 2019 at 4:34 PM Vladimir Sementsov-Ogievskiy
> > <vsementsov@virtuozzo.com> wrote:
> >>
> >> "NAME" here may be interpreted like it should match @name, which is
> >> export name. But it was never mentioned in such way. Make it obvious,
> >> that actual "<dirty-bitmap-export-name>" (see docs/interop/nbd.txt)
> >> will match @bitmap parameter.
> >
> > But this is wrong, dirty-bitmap-export-name does not mean the actual bitmap
> > name but the name exposed to the NBD client, which can be anything.
>
> Yes. What is wrong? It can be enything. Currently by default it is bitmap name.
> It purely documented (okay, even confusingly documented), but it was so since
> 4.0. And existing users obviously knows how it work (otherwise, they can't use
> the feature)
>
> So, I think it's OK to fix spec to directly show implementation, that was here
> since feature introducing.
>
> >
> >> Fixes: 5fcbeb06812685a2
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >> ---
> >>
> >> Hi all.
> >>
> >> This patch follows discussion on Nir's patch
> >>   [PATCH] block: nbd: Fix dirty bitmap context name
> >>   ( https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg04309.html )
> >>
> >> Let's just fix qapi spec now.
> >
> > But qapi documents a better behavior for users. We should fix the code instead
> > to mach the docs.
>
> 1. Using disk name as a bitmap name is a bad behavior, as they are completely
> different concepts. Especially keeping in mind that user already knows disk name anyway
> and no reason to write this export name inside metadata context of this export.

The different concept is expressed by the "qemu:dirty-bitmap:" prefix.
"qemu:dirty-bitmap:export-name" means the dirty bitmap for this export.

> 2. It's not directly documented. You assume that NAME == @name. I understand that
> it may be assumed.. But it's not documented.

But NAME is likely to be understood as the name argument, and unlikely to be the
bitmap name.

> 3. It's never worked like you write. So if we change the behavior, we'll break
> existing users.

Do we have existing users? isn't this new feature in 4.2?

Before we had experimental x-block-dirty-bitmap APIs, which are stable, so users
could not depend on them.

> > With this we still have the issue of leaking internal bitmap name to
> > users who do not
> > control the name, and do not care about it.
> >
> >>   qapi/block.json | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/qapi/block.json b/qapi/block.json
> >> index 145c268bb6..8042ef78f0 100644
> >> --- a/qapi/block.json
> >> +++ b/qapi/block.json
> >> @@ -255,7 +255,8 @@
> >>
> >>   # @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)
> >> +#          "qemu:dirty-bitmap:BITMAP" to inspect the bitmap (BITMAP here
> >> +#          matches @bitmap parameter). (since 4.0)
> >>   #
> >>   # Returns: error if the server is not running, or export with the same name
> >>   #          already exists.
> >> --
> >> 2.21.0
> >>
> >
>
>
> --
> Best regards,
> Vladimir

Re: [PATCH] qapi/block: fix nbd-server-add spec

[Vladimir Sementsov-Ogievskiy]

19.12.2019 18:08, Nir Soffer wrote:
> On Thu, Dec 19, 2019 at 5:00 PM Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com> wrote:
>>
>> 19.12.2019 17:42, Nir Soffer wrote:
>>> On Thu, Dec 19, 2019 at 4:34 PM Vladimir Sementsov-Ogievskiy
>>> <vsementsov@virtuozzo.com> wrote:
>>>>
>>>> "NAME" here may be interpreted like it should match @name, which is
>>>> export name. But it was never mentioned in such way. Make it obvious,
>>>> that actual "<dirty-bitmap-export-name>" (see docs/interop/nbd.txt)
>>>> will match @bitmap parameter.
>>>
>>> But this is wrong, dirty-bitmap-export-name does not mean the actual bitmap
>>> name but the name exposed to the NBD client, which can be anything.
>>
>> Yes. What is wrong? It can be enything. Currently by default it is bitmap name.
>> It purely documented (okay, even confusingly documented), but it was so since
>> 4.0. And existing users obviously knows how it work (otherwise, they can't use
>> the feature)
>>
>> So, I think it's OK to fix spec to directly show implementation, that was here
>> since feature introducing.
>>
>>>
>>>> Fixes: 5fcbeb06812685a2
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>
>>>> Hi all.
>>>>
>>>> This patch follows discussion on Nir's patch
>>>>    [PATCH] block: nbd: Fix dirty bitmap context name
>>>>    ( https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg04309.html )
>>>>
>>>> Let's just fix qapi spec now.
>>>
>>> But qapi documents a better behavior for users. We should fix the code instead
>>> to mach the docs.
>>
>> 1. Using disk name as a bitmap name is a bad behavior, as they are completely
>> different concepts. Especially keeping in mind that user already knows disk name anyway
>> and no reason to write this export name inside metadata context of this export.
> 
> The different concept is expressed by the "qemu:dirty-bitmap:" prefix.
> "qemu:dirty-bitmap:export-name" means the dirty bitmap for this export.

Why do you think so? Did you read NBD specification?

Metadata context is always owned by some export. Do you mean that there will be
metadata contexts

qemu:dirty-bitmap:export-A
qemu:dirty-bitmap:export-B

both defined for export-A?

> 
>> 2. It's not directly documented. You assume that NAME == @name. I understand that
>> it may be assumed.. But it's not documented.
> 
> But NAME is likely to be understood as the name argument, and unlikely to be the
> bitmap name.

Yes likely. But it's still bad specification, which should be fixed.

> 
>> 3. It's never worked like you write. So if we change the behavior, we'll break
>> existing users.
> 
> Do we have existing users? isn't this new feature in 4.2?

No, it's since 4.0

> 
> Before we had experimental x-block-dirty-bitmap APIs, which are stable, so users
> could not depend on them.
> 
>>> With this we still have the issue of leaking internal bitmap name to
>>> users who do not
>>> control the name, and do not care about it.
>>>
>>>>    qapi/block.json | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/qapi/block.json b/qapi/block.json
>>>> index 145c268bb6..8042ef78f0 100644
>>>> --- a/qapi/block.json
>>>> +++ b/qapi/block.json
>>>> @@ -255,7 +255,8 @@
>>>>
>>>>    # @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)
>>>> +#          "qemu:dirty-bitmap:BITMAP" to inspect the bitmap (BITMAP here
>>>> +#          matches @bitmap parameter). (since 4.0)
>>>>    #
>>>>    # Returns: error if the server is not running, or export with the same name
>>>>    #          already exists.
>>>> --
>>>> 2.21.0
>>>>
>>>
>>
>>
>> --
>> Best regards,
>> Vladimir
> 


-- 
Best regards,
Vladimir

Re: [PATCH] qapi/block: fix nbd-server-add spec

[Nir Soffer]

On Thu, Dec 19, 2019 at 5:25 PM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
>
> 19.12.2019 18:08, Nir Soffer wrote:
> > On Thu, Dec 19, 2019 at 5:00 PM Vladimir Sementsov-Ogievskiy
> > <vsementsov@virtuozzo.com> wrote:
> >>
> >> 19.12.2019 17:42, Nir Soffer wrote:
> >>> On Thu, Dec 19, 2019 at 4:34 PM Vladimir Sementsov-Ogievskiy
> >>> <vsementsov@virtuozzo.com> wrote:
> >>>>
> >>>> "NAME" here may be interpreted like it should match @name, which is
> >>>> export name. But it was never mentioned in such way. Make it obvious,
> >>>> that actual "<dirty-bitmap-export-name>" (see docs/interop/nbd.txt)
> >>>> will match @bitmap parameter.
> >>>
> >>> But this is wrong, dirty-bitmap-export-name does not mean the actual bitmap
> >>> name but the name exposed to the NBD client, which can be anything.
> >>
> >> Yes. What is wrong? It can be enything. Currently by default it is bitmap name.
> >> It purely documented (okay, even confusingly documented), but it was so since
> >> 4.0. And existing users obviously knows how it work (otherwise, they can't use
> >> the feature)
> >>
> >> So, I think it's OK to fix spec to directly show implementation, that was here
> >> since feature introducing.
> >>
> >>>
> >>>> Fixes: 5fcbeb06812685a2
> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >>>> ---
> >>>>
> >>>> Hi all.
> >>>>
> >>>> This patch follows discussion on Nir's patch
> >>>>    [PATCH] block: nbd: Fix dirty bitmap context name
> >>>>    ( https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg04309.html )
> >>>>
> >>>> Let's just fix qapi spec now.
> >>>
> >>> But qapi documents a better behavior for users. We should fix the code instead
> >>> to mach the docs.
> >>
> >> 1. Using disk name as a bitmap name is a bad behavior, as they are completely
> >> different concepts. Especially keeping in mind that user already knows disk name anyway
> >> and no reason to write this export name inside metadata context of this export.
> >
> > The different concept is expressed by the "qemu:dirty-bitmap:" prefix.
> > "qemu:dirty-bitmap:export-name" means the dirty bitmap for this export.
>
> Why do you think so? Did you read NBD specification?

Yes - the name of the bitmap does not have any meaning.
But for nbd_server_add we allow only single bitmap for export.

> Metadata context is always owned by some export.

Of course.

> Do you mean that there will bemetadata contexts
>
> qemu:dirty-bitmap:export-A
> qemu:dirty-bitmap:export-B
>
> both defined for export-A?

It does not make sense, but it is valid.

> >> 2. It's not directly documented. You assume that NAME == @name. I understand that
> >> it may be assumed.. But it's not documented.
> >
> > But NAME is likely to be understood as the name argument, and unlikely to be the
> > bitmap name.
>
> Yes likely. But it's still bad specification, which should be fixed.

If we cannot change the current behavior since it will break current users,
I agree fixing the spec to describe the current behavior is a good idea.




> >
> >> 3. It's never worked like you write. So if we change the behavior, we'll break
> >> existing users.
> >
> > Do we have existing users? isn't this new feature in 4.2?
>
> No, it's since 4.0
>
> >
> > Before we had experimental x-block-dirty-bitmap APIs, which are stable, so users
> > could not depend on them.
> >
> >>> With this we still have the issue of leaking internal bitmap name to
> >>> users who do not
> >>> control the name, and do not care about it.
> >>>
> >>>>    qapi/block.json | 3 ++-
> >>>>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/qapi/block.json b/qapi/block.json
> >>>> index 145c268bb6..8042ef78f0 100644
> >>>> --- a/qapi/block.json
> >>>> +++ b/qapi/block.json
> >>>> @@ -255,7 +255,8 @@
> >>>>
> >>>>    # @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)
> >>>> +#          "qemu:dirty-bitmap:BITMAP" to inspect the bitmap (BITMAP here
> >>>> +#          matches @bitmap parameter). (since 4.0)
> >>>>    #
> >>>>    # Returns: error if the server is not running, or export with the same name
> >>>>    #          already exists.
> >>>> --
> >>>> 2.21.0
> >>>>
> >>>
> >>
> >>
> >> --
> >> Best regards,
> >> Vladimir
> >
>
>
> --
> Best regards,
> Vladimir

Re: [PATCH] qapi/block: fix nbd-server-add spec

[Vladimir Sementsov-Ogievskiy]

19.12.2019 19:14, Nir Soffer wrote:
> On Thu, Dec 19, 2019 at 5:25 PM Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com> wrote:
>>
>> 19.12.2019 18:08, Nir Soffer wrote:
>>> On Thu, Dec 19, 2019 at 5:00 PM Vladimir Sementsov-Ogievskiy
>>> <vsementsov@virtuozzo.com> wrote:
>>>>
>>>> 19.12.2019 17:42, Nir Soffer wrote:
>>>>> On Thu, Dec 19, 2019 at 4:34 PM Vladimir Sementsov-Ogievskiy
>>>>> <vsementsov@virtuozzo.com> wrote:
>>>>>>
>>>>>> "NAME" here may be interpreted like it should match @name, which is
>>>>>> export name. But it was never mentioned in such way. Make it obvious,
>>>>>> that actual "<dirty-bitmap-export-name>" (see docs/interop/nbd.txt)
>>>>>> will match @bitmap parameter.
>>>>>
>>>>> But this is wrong, dirty-bitmap-export-name does not mean the actual bitmap
>>>>> name but the name exposed to the NBD client, which can be anything.
>>>>
>>>> Yes. What is wrong? It can be enything. Currently by default it is bitmap name.
>>>> It purely documented (okay, even confusingly documented), but it was so since
>>>> 4.0. And existing users obviously knows how it work (otherwise, they can't use
>>>> the feature)
>>>>
>>>> So, I think it's OK to fix spec to directly show implementation, that was here
>>>> since feature introducing.
>>>>
>>>>>
>>>>>> Fixes: 5fcbeb06812685a2
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>> ---
>>>>>>
>>>>>> Hi all.
>>>>>>
>>>>>> This patch follows discussion on Nir's patch
>>>>>>     [PATCH] block: nbd: Fix dirty bitmap context name
>>>>>>     ( https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg04309.html )
>>>>>>
>>>>>> Let's just fix qapi spec now.
>>>>>
>>>>> But qapi documents a better behavior for users. We should fix the code instead
>>>>> to mach the docs.
>>>>
>>>> 1. Using disk name as a bitmap name is a bad behavior, as they are completely
>>>> different concepts. Especially keeping in mind that user already knows disk name anyway
>>>> and no reason to write this export name inside metadata context of this export.
>>>
>>> The different concept is expressed by the "qemu:dirty-bitmap:" prefix.
>>> "qemu:dirty-bitmap:export-name" means the dirty bitmap for this export.
>>
>> Why do you think so? Did you read NBD specification?
> 
> Yes - the name of the bitmap does not have any meaning.

it does not have any meaning in your scenario. But in some other, when we need
to export several bitmaps, the name will be used.

> But for nbd_server_add we allow only single bitmap for export.

Yes, Qemu just don't use the whole power of NBD spec. But it may change.

> 
>> Metadata context is always owned by some export.
> 
> Of course.
> 
>> Do you mean that there will bemetadata contexts
>>
>> qemu:dirty-bitmap:export-A
>> qemu:dirty-bitmap:export-B
>>
>> both defined for export-A?
> 
> It does not make sense, but it is valid.

It conflicts with the fact that both contexts are owned by export-A. The idea
was that metadata is bound to the export. We should not see metadata of export
B in metadata of export A.

Moreover, consider that these two exports may have different size. Then how NBD_CMD_BLOCK_STATUS
will work?

We don't have global nbd server metadata, like you consider it. We only have per-export metadata,
which is bound (at least by size) to the export. So it's theoretically possible to export bitmap
of another nbd-export (if it has the same size), but it breaks the concept.

> 
>>>> 2. It's not directly documented. You assume that NAME == @name. I understand that
>>>> it may be assumed.. But it's not documented.
>>>
>>> But NAME is likely to be understood as the name argument, and unlikely to be the
>>> bitmap name.
>>
>> Yes likely. But it's still bad specification, which should be fixed.
> 
> If we cannot change the current behavior since it will break current users,
> I agree fixing the spec to describe the current behavior is a good idea.
> 
> 
> 
> 
>>>
>>>> 3. It's never worked like you write. So if we change the behavior, we'll break
>>>> existing users.
>>>
>>> Do we have existing users? isn't this new feature in 4.2?
>>
>> No, it's since 4.0
>>
>>>
>>> Before we had experimental x-block-dirty-bitmap APIs, which are stable, so users
>>> could not depend on them.
>>>
>>>>> With this we still have the issue of leaking internal bitmap name to
>>>>> users who do not
>>>>> control the name, and do not care about it.
>>>>>
>>>>>>     qapi/block.json | 3 ++-
>>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/qapi/block.json b/qapi/block.json
>>>>>> index 145c268bb6..8042ef78f0 100644
>>>>>> --- a/qapi/block.json
>>>>>> +++ b/qapi/block.json
>>>>>> @@ -255,7 +255,8 @@
>>>>>>
>>>>>>     # @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)
>>>>>> +#          "qemu:dirty-bitmap:BITMAP" to inspect the bitmap (BITMAP here
>>>>>> +#          matches @bitmap parameter). (since 4.0)
>>>>>>     #
>>>>>>     # Returns: error if the server is not running, or export with the same name
>>>>>>     #          already exists.
>>>>>> --
>>>>>> 2.21.0
>>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Best regards,
>>>> Vladimir
>>>
>>
>>
>> --
>> Best regards,
>> Vladimir
> 


-- 
Best regards,
Vladimir

Re: [PATCH] qapi/block: fix nbd-server-add spec

[Eric Blake]

On 12/19/19 10:14 AM, Nir Soffer wrote:

>>>> 1. Using disk name as a bitmap name is a bad behavior, as they are completely
>>>> different concepts. Especially keeping in mind that user already knows disk name anyway
>>>> and no reason to write this export name inside metadata context of this export.
>>>
>>> The different concept is expressed by the "qemu:dirty-bitmap:" prefix.
>>> "qemu:dirty-bitmap:export-name" means the dirty bitmap for this export.
>>
>> Why do you think so? Did you read NBD specification?
> 
> Yes - the name of the bitmap does not have any meaning.
> But for nbd_server_add we allow only single bitmap for export.

Just because qemu is currently limited to only exposing one bitmap at 
the moment does not mean that a future version can't expose multiple 
bitmaps. It may very well be that we have reason to expose both 
"qemu:dirty-bitmap:timeA" and "qemu:dirty-bitmap:timeB" on the same 
export, for exposing two bitmaps at once.  To get to that point, we'd 
have to refactor the QAPI command to allow attaching more than one 
bitmap at the time of creating the NBD export, but it's not impossible.

> 
>> Metadata context is always owned by some export.
> 
> Of course.
> 
>> Do you mean that there will bemetadata contexts
>>
>> qemu:dirty-bitmap:export-A
>> qemu:dirty-bitmap:export-B
>>
>> both defined for export-A?
> 
> It does not make sense, but it is valid.

If an image has multiple bitmaps, exposing all of those as separate 
contexts at the same time for a single export can indeed make sense.

> 
>>>> 2. It's not directly documented. You assume that NAME == @name. I understand that
>>>> it may be assumed.. But it's not documented.
>>>
>>> But NAME is likely to be understood as the name argument, and unlikely to be the
>>> bitmap name.
>>
>> Yes likely. But it's still bad specification, which should be fixed.
> 
> If we cannot change the current behavior since it will break current users,
> I agree fixing the spec to describe the current behavior is a good idea.

We need the doc fix. Whether we also want an additional fix adding an 
optional parameter allowing user control over the export name is also 
under debate (the fact that the old x-nbd-server-add-bitmap supported it 
shows that it may be useful, but it is not minimal, and as I pointed out 
at the time of removing x-, libvirt can always control what name is 
exposed by creating a temporary bitmap and merging from other bitmaps 
into the temporary).

We also have to think about a future of parallel backup jobs: libvirt 
can create a single temporary bitmap to expose whatever name it wants 
under one job, but if libvirt wants to expose the SAME user-visible name 
to two parallel jobs, it cannot create two bitmaps with the same name, 
so having a way to set the user-visible name of an arbitrary bitmap when 
producing the NBD export makes sense on that front.


>>>
>>>> 3. It's never worked like you write. So if we change the behavior, we'll break
>>>> existing users.
>>>
>>> Do we have existing users? isn't this new feature in 4.2?
>>
>> No, it's since 4.0

As long as altering the exported name is controlled by a new optional 
parameter, it does not hurt older 4.0 clients that do not use the new 
parameter.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] qapi/block: fix nbd-server-add spec

[Vladimir Sementsov-Ogievskiy]

21.12.2019 1:04, Eric Blake wrote:
> On 12/19/19 10:14 AM, Nir Soffer wrote:
> 
>>>>> 1. Using disk name as a bitmap name is a bad behavior, as they are completely
>>>>> different concepts. Especially keeping in mind that user already knows disk name anyway
>>>>> and no reason to write this export name inside metadata context of this export.
>>>>
>>>> The different concept is expressed by the "qemu:dirty-bitmap:" prefix.
>>>> "qemu:dirty-bitmap:export-name" means the dirty bitmap for this export.
>>>
>>> Why do you think so? Did you read NBD specification?
>>
>> Yes - the name of the bitmap does not have any meaning.
>> But for nbd_server_add we allow only single bitmap for export.
> 
> Just because qemu is currently limited to only exposing one bitmap at the moment does not mean that a future version can't expose multiple bitmaps. It may very well be that we have reason to expose both "qemu:dirty-bitmap:timeA" and "qemu:dirty-bitmap:timeB" on the same export, for exposing two bitmaps at once.  To get to that point, we'd have to refactor the QAPI command to allow attaching more than one bitmap at the time of creating the NBD export, but it's not impossible.
> 
>>
>>> Metadata context is always owned by some export.
>>
>> Of course.
>>
>>> Do you mean that there will bemetadata contexts
>>>
>>> qemu:dirty-bitmap:export-A
>>> qemu:dirty-bitmap:export-B
>>>
>>> both defined for export-A?
>>
>> It does not make sense, but it is valid.
> 
> If an image has multiple bitmaps, exposing all of those as separate contexts at the same time for a single export can indeed make sense.
> 
>>
>>>>> 2. It's not directly documented. You assume that NAME == @name. I understand that
>>>>> it may be assumed.. But it's not documented.
>>>>
>>>> But NAME is likely to be understood as the name argument, and unlikely to be the
>>>> bitmap name.
>>>
>>> Yes likely. But it's still bad specification, which should be fixed.
>>
>> If we cannot change the current behavior since it will break current users,
>> I agree fixing the spec to describe the current behavior is a good idea.
> 
> We need the doc fix. 

Do you like the patch starting this thread? It's a fix we are talking about..

> Whether we also want an additional fix adding an optional parameter allowing user control over the export name is also under debate (the fact that the old x-nbd-server-add-bitmap supported it shows that it may be useful, but it is not minimal, and as I pointed out at the time of removing x-, libvirt can always control what name is exposed by creating a temporary bitmap and merging from other bitmaps into the temporary).
> 
> We also have to think about a future of parallel backup jobs: libvirt can create a single temporary bitmap to expose whatever name it wants under one job, but if libvirt wants to expose the SAME user-visible name to two parallel jobs, it cannot create two bitmaps with the same name, so having a way to set the user-visible name of an arbitrary bitmap when producing the NBD export makes sense on that front.
> 
> 
>>>>
>>>>> 3. It's never worked like you write. So if we change the behavior, we'll break
>>>>> existing users.
>>>>
>>>> Do we have existing users? isn't this new feature in 4.2?
>>>
>>> No, it's since 4.0
> 
> As long as altering the exported name is controlled by a new optional parameter, it does not hurt older 4.0 clients that do not use the new parameter.
> 


-- 
Best regards,
Vladimir

Re: [PATCH] qapi/block: fix nbd-server-add spec

[Eric Blake]

On 12/19/19 9:08 AM, Nir Soffer wrote:

>>>> Let's just fix qapi spec now.
>>>
>>> But qapi documents a better behavior for users. We should fix the code instead
>>> to mach the docs.
>>
>> 1. Using disk name as a bitmap name is a bad behavior, as they are completely
>> different concepts. Especially keeping in mind that user already knows disk name anyway
>> and no reason to write this export name inside metadata context of this export.
> 
> The different concept is expressed by the "qemu:dirty-bitmap:" prefix.
> "qemu:dirty-bitmap:export-name" means the dirty bitmap for this export.
> 
>> 2. It's not directly documented. You assume that NAME == @name. I understand that
>> it may be assumed.. But it's not documented.
> 
> But NAME is likely to be understood as the name argument, and unlikely to be the
> bitmap name.

That's a misunderstanding due to poor docs.  The bitmap name has always 
been what was exposed, ever since we promoted things to stable by 
getting rid of x-.

> 
>> 3. It's never worked like you write. So if we change the behavior, we'll break
>> existing users.
> 
> Do we have existing users? isn't this new feature in 4.2?

No, the feature stems back to 4.0, when we got rid of x-.  There are 
other reasons that dirty bitmaps aren't really usable for incremental 
backups without qemu 4.2, but qemu 4.0 was the first time we exposed a 
stable interface for a bitmap over an NBD export, and that release used 
the bitmap name (and not the export name), so at this point, a code 
change would break expectations of any 4.0 client using bitmaps for 
other reasons.  Libvirt currently has absolute control over the bitmap 
name (my initial code in libvirt created a temporary bitmap with my 
desired name, then merged the contents from the permanent bitmaps 
corresponding to the actual libvirt Checkpoint objects into the 
temporary, so that it could then call nbd-export-add with the temporary 
bitmap name).  But, as you point out...

> 
> Before we had experimental x-block-dirty-bitmap APIs, which are stable, so users
> could not depend on them.

The unstable x-block-dirty-bitmap APIs _did_ have a way to export a 
user-controlled name SEPARATE from the bitmap name.  At the time I was 
removing the x- prefix, I asked if anyone had a strong use case for 
keeping that functionality.  No one spoke up in favor of keeping it 
(Nikolay mentioned using the old interface, but not being stumped by its 
removal), so we nuked it at the time.  We can always add it back (now 
that it sounds like you have a use case where it may be more 
compelling), but it was easier to stabilize less and add more later as 
needed, than to stabilize too much and regret that we had to support the 
flexibility that no one needed.
https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg02373.html
https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg01970.html

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org