On Fri, Dec 20, 2019 at 09:56:51AM -0500, Jason Dillaman wrote:
> On Fri, Dec 20, 2019 at 9:11 AM Florian Florensa <fflorensa@online.net> wrote:
> >
> > Hello Stefano and Jason,
> >
> > First of all thanks for the quick reply,
> > Response inline belowe
> > > Hi Florian,
> > >
> > > I think we need to add (Since: 5.0).
> >
> > Are you implying by that (Since: 5.0) that we need to specify its
> > availability target is qemu 5.0 ?
> 
> FWIW, I took this as just a comment to add some documentation that the
> field is only valid starting w/ qemu v5.
> 
Works for me, will add this in v2.
> > I guess that maybe a version check would be better ? Like try to do
> > namespaces stuff only if we have a recent enough librbd in the system ?
> > Using something like :
> >
> > int rbd_major;
> >
> > rbd_version(&rbd_major, NULL, NULL);
> > /*
> >  * Target only nautilus+ librbd for namespace support
> > */
> > if (rbd_major >= 14) // tar
> >  <process namespace>
> 
> Unfortunately, those versions weren't updated in the Mimic nor
> Nautilus release so it would still return 1/12 (whoops). I think that
> means you would need to add a probe in "configure" to test for librbd
> namespace support (e.g. test for the existence of the `rbd_list2`
> function or the `rbd_linked_image_spec_t` structure). I'll fix this
> before the forthcoming Octopus release.
Will see to do this, I originally wanted to do this at runtime so a Qemu
built against an older librbd would work if the library was updated.
Else some dlopen + dlsym trickery would work by checking for the
existence of rbd_list2 in librbd.so, but I guess this might be a bad
idea, as it would add code that would be useless in sometime
> 
> > > The patch LGTM, but I'd like to use 'namespace' instead of cryptic
> > > 'nspace'. (as BlockdevOptionsNVMe did)
> > > What do you think?
> > >
> > Yes no worries, I can rename it to 'rbd_namespace' to avoid any possible
> > confusion, is this Ok for you ?
> 
> We use "pool_namespace" in the rbd CLI if you are trying to avoid the
> word "namespace".
> 
Yes I wanted to avoid namespace because it looks like the qapi generated
code changes the name to something like q_namespace, will use
pool_namespace in the v2.
> > > With those fixed:
> > >
> > > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> > >
> > > Thanks,
> > > Stefano
> >
> > Regards,
> > Florian
> 
> -- 
> Jason
> 

Regards,
Florian