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