Re: [PATCH] block/rbd: Add support for ceph namespaces

[Jason Dillaman]

On Fri, Dec 20, 2019 at 9:11 AM Florian Florensa <address@hidden> 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.

> 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.

> > 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".

> > With those fixed:
> >
> > Reviewed-by: Stefano Garzarella <address@hidden>
> >
> > Thanks,
> > Stefano
>
> Regards,
> Florian

-- 
Jason