On Fri, Dec 20, 2019 at 9:11 AM Florian Florensa <fflore...@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. > 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 <sgarz...@redhat.com> > > > > Thanks, > > Stefano > > Regards, > Florian -- Jason
