Starting from ceph Nautilus, RBD has support for namespaces, allowing
for finer grain ACLs on images inside a pool, and tenant isolation.

In the rbd cli tool documentation, the new image-spec and snap-spec are :
 - [pool-name/[namespace-name/]]image-name
 - [pool-name/[namespace-name/]]image-name@snap-name

When using an non namespace's enabled qemu, it complains about not
finding the image called namespace-name/image-name, thus we only need to
parse the image once again to find if there is a '/' in its name, and if
there is, use what is before it as the name of the namespace to later
pass it to rados_ioctx_set_namespace.
rados_ioctx_set_namespace if called with en empty string or a null
pointer as the namespace parameters pretty much does nothing, as it then
defaults to the default namespace.

The namespace is extracted inside qemu_rbd_parse_filename, stored in the
qdict, and used in qemu_rbd_connect to make it work with both qemu-img,
and qemu itself.

Signed-off-by: Florian Florensa <fflorensa@online.net>
---
 block/rbd.c          | 30 ++++++++++++++++++++++++------
 qapi/block-core.json |  3 +++
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 027cbcc695..e43099fc75 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -104,6 +104,7 @@ typedef struct BDRVRBDState {
     rbd_image_t image;
     char *image_name;
     char *snap;
+    char *nspace;
     uint64_t image_size;
 } BDRVRBDState;
 
@@ -152,7 +153,7 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
     const char *start;
     char *p, *buf;
     QList *keypairs = NULL;
-    char *found_str;
+    char *found_str, *image_name;
 
     if (!strstart(filename, "rbd:", &start)) {
         error_setg(errp, "File name must start with 'rbd:'");
@@ -171,18 +172,24 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
     qdict_put_str(options, "pool", found_str);
 
     if (strchr(p, '@')) {
-        found_str = qemu_rbd_next_tok(p, '@', &p);
-        qemu_rbd_unescape(found_str);
-        qdict_put_str(options, "image", found_str);
+        image_name = qemu_rbd_next_tok(p, '@', &p);
 
         found_str = qemu_rbd_next_tok(p, ':', &p);
         qemu_rbd_unescape(found_str);
         qdict_put_str(options, "snapshot", found_str);
     } else {
-        found_str = qemu_rbd_next_tok(p, ':', &p);
+        image_name = qemu_rbd_next_tok(p, ':', &p);
+    }
+    /* Check for namespace in the image_name */
+    if (strchr(image_name, '/')) {
+        found_str = qemu_rbd_next_tok(image_name, '/', &image_name);
         qemu_rbd_unescape(found_str);
-        qdict_put_str(options, "image", found_str);
+        qdict_put_str(options, "nspace", found_str);
+    } else {
+        qdict_put_str(options, "nspace", "");
     }
+    qemu_rbd_unescape(image_name);
+    qdict_put_str(options, "image", image_name);
     if (!p) {
         goto done;
     }
@@ -343,6 +350,11 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "Rados pool name",
         },
+        {
+            .name = "nspace",
+            .type = QEMU_OPT_STRING,
+            .help = "Rados namespace name in the pool",
+        },
         {
             .name = "image",
             .type = QEMU_OPT_STRING,
@@ -472,6 +484,7 @@ static int coroutine_fn qemu_rbd_co_create_opts(const char *filename,
     loc->has_conf = !!loc->conf;
     loc->user     = g_strdup(qdict_get_try_str(options, "user"));
     loc->has_user = !!loc->user;
+    loc->nspace   = g_strdup(qdict_get_try_str(options, "nspace"));
     loc->image    = g_strdup(qdict_get_try_str(options, "image"));
     keypairs      = qdict_get_try_str(options, "=keyvalue-pairs");
 
@@ -648,6 +661,11 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
         error_setg_errno(errp, -r, "error opening pool %s", opts->pool);
         goto failed_shutdown;
     }
+    /*
+     * Set the namespace after opening the io context on the pool,
+     * if nspace == NULL or if nspace == "", it is just as we did nothing
+     */
+    rados_ioctx_set_namespace(*io_ctx, opts->nspace);
 
     return 0;
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0cf68fea14..9ebc020e93 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3657,6 +3657,8 @@
 #
 # @pool:               Ceph pool name.
 #
+# @nspace:             Rados namespace name in the Ceph pool.
+#
 # @image:              Image name in the Ceph pool.
 #
 # @conf:               path to Ceph configuration file.  Values
@@ -3683,6 +3685,7 @@
 ##
 { 'struct': 'BlockdevOptionsRbd',
   'data': { 'pool': 'str',
+            'nspace': 'str',
             'image': 'str',
             '*conf': 'str',
             '*snapshot': 'str',
-- 
2.24.1

On Thu, Dec 19, 2019 at 8:44 AM Florian Florensa <fflorensa@online.net> wrote:
>
> Starting from ceph Nautilus, RBD has support for namespaces, allowing
> for finer grain ACLs on images inside a pool, and tenant isolation.
>
> In the rbd cli tool documentation, the new image-spec and snap-spec are :
>  - [pool-name/[namespace-name/]]image-name
>  - [pool-name/[namespace-name/]]image-name@snap-name
>
> When using an non namespace's enabled qemu, it complains about not
> finding the image called namespace-name/image-name, thus we only need to
> parse the image once again to find if there is a '/' in its name, and if
> there is, use what is before it as the name of the namespace to later
> pass it to rados_ioctx_set_namespace.
> rados_ioctx_set_namespace if called with en empty string or a null
> pointer as the namespace parameters pretty much does nothing, as it then
> defaults to the default namespace.
>
> The namespace is extracted inside qemu_rbd_parse_filename, stored in the
> qdict, and used in qemu_rbd_connect to make it work with both qemu-img,
> and qemu itself.
>
> Signed-off-by: Florian Florensa <fflorensa@online.net>
> ---
>  block/rbd.c          | 30 ++++++++++++++++++++++++------
>  qapi/block-core.json |  3 +++
>  2 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index 027cbcc695..e43099fc75 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -104,6 +104,7 @@ typedef struct BDRVRBDState {
>      rbd_image_t image;
>      char *image_name;
>      char *snap;
> +    char *nspace;
>      uint64_t image_size;
>  } BDRVRBDState;
>
> @@ -152,7 +153,7 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
>      const char *start;
>      char *p, *buf;
>      QList *keypairs = NULL;
> -    char *found_str;
> +    char *found_str, *image_name;
>
>      if (!strstart(filename, "rbd:", &start)) {
>          error_setg(errp, "File name must start with 'rbd:'");
> @@ -171,18 +172,24 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
>      qdict_put_str(options, "pool", found_str);
>
>      if (strchr(p, '@')) {
> -        found_str = qemu_rbd_next_tok(p, '@', &p);
> -        qemu_rbd_unescape(found_str);
> -        qdict_put_str(options, "image", found_str);
> +        image_name = qemu_rbd_next_tok(p, '@', &p);
>
>          found_str = qemu_rbd_next_tok(p, ':', &p);
>          qemu_rbd_unescape(found_str);
>          qdict_put_str(options, "snapshot", found_str);
>      } else {
> -        found_str = qemu_rbd_next_tok(p, ':', &p);
> +        image_name = qemu_rbd_next_tok(p, ':', &p);
> +    }
> +    /* Check for namespace in the image_name */
> +    if (strchr(image_name, '/')) {
> +        found_str = qemu_rbd_next_tok(image_name, '/', &image_name);
>          qemu_rbd_unescape(found_str);
> -        qdict_put_str(options, "image", found_str);
> +        qdict_put_str(options, "nspace", found_str);
> +    } else {
> +        qdict_put_str(options, "nspace", "");
>      }
> +    qemu_rbd_unescape(image_name);
> +    qdict_put_str(options, "image", image_name);
>      if (!p) {
>          goto done;
>      }
> @@ -343,6 +350,11 @@ static QemuOptsList runtime_opts = {
>              .type = QEMU_OPT_STRING,
>              .help = "Rados pool name",
>          },
> +        {
> +            .name = "nspace",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Rados namespace name in the pool",
> +        },
>          {
>              .name = "image",
>              .type = QEMU_OPT_STRING,
> @@ -472,6 +484,7 @@ static int coroutine_fn qemu_rbd_co_create_opts(const char *filename,
>      loc->has_conf = !!loc->conf;
>      loc->user     = g_strdup(qdict_get_try_str(options, "user"));
>      loc->has_user = !!loc->user;
> +    loc->nspace   = g_strdup(qdict_get_try_str(options, "nspace"));
>      loc->image    = g_strdup(qdict_get_try_str(options, "image"));
>      keypairs      = qdict_get_try_str(options, "=keyvalue-pairs");
>
> @@ -648,6 +661,11 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
>          error_setg_errno(errp, -r, "error opening pool %s", opts->pool);
>          goto failed_shutdown;
>      }
> +    /*
> +     * Set the namespace after opening the io context on the pool,
> +     * if nspace == NULL or if nspace == "", it is just as we did nothing
> +     */
> +    rados_ioctx_set_namespace(*io_ctx, opts->nspace);
>
>      return 0;
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0cf68fea14..9ebc020e93 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3657,6 +3657,8 @@
>  #
>  # @pool:               Ceph pool name.
>  #
> +# @nspace:             Rados namespace name in the Ceph pool.
> +#
>  # @image:              Image name in the Ceph pool.
>  #
>  # @conf:               path to Ceph configuration file.  Values
> @@ -3683,6 +3685,7 @@
>  ##
>  { 'struct': 'BlockdevOptionsRbd',
>    'data': { 'pool': 'str',
> +            'nspace': 'str',
>              'image': 'str',
>              '*conf': 'str',
>              '*snapshot': 'str',
> --
> 2.24.1
>

Thanks for tackling this. I had this and msgr v2 support on my todo
list for QEMU but I haven't had a chance to work on them yet. The
changes look good to me and it works as expected during CLI
play-testing.

Reviewed-by: Jason Dillaman <dillaman@redhat.com>

Hi Florian,

On Thu, Dec 19, 2019 at 02:34:16PM +0100, Florian Florensa wrote:
> Starting from ceph Nautilus, RBD has support for namespaces, allowing
> for finer grain ACLs on images inside a pool, and tenant isolation.
> 
> In the rbd cli tool documentation, the new image-spec and snap-spec are :
>  - [pool-name/[namespace-name/]]image-name
>  - [pool-name/[namespace-name/]]image-name@snap-name
> 
> When using an non namespace's enabled qemu, it complains about not
> finding the image called namespace-name/image-name, thus we only need to
> parse the image once again to find if there is a '/' in its name, and if
> there is, use what is before it as the name of the namespace to later
> pass it to rados_ioctx_set_namespace.
> rados_ioctx_set_namespace if called with en empty string or a null
> pointer as the namespace parameters pretty much does nothing, as it then
> defaults to the default namespace.
> 
> The namespace is extracted inside qemu_rbd_parse_filename, stored in the
> qdict, and used in qemu_rbd_connect to make it work with both qemu-img,
> and qemu itself.
> 
> Signed-off-by: Florian Florensa <fflorensa@online.net>
> ---
>  block/rbd.c          | 30 ++++++++++++++++++++++++------
>  qapi/block-core.json |  3 +++
>  2 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 027cbcc695..e43099fc75 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -104,6 +104,7 @@ typedef struct BDRVRBDState {
>      rbd_image_t image;
>      char *image_name;
>      char *snap;
> +    char *nspace;
>      uint64_t image_size;
>  } BDRVRBDState;
>  
> @@ -152,7 +153,7 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
>      const char *start;
>      char *p, *buf;
>      QList *keypairs = NULL;
> -    char *found_str;
> +    char *found_str, *image_name;
>  
>      if (!strstart(filename, "rbd:", &start)) {
>          error_setg(errp, "File name must start with 'rbd:'");
> @@ -171,18 +172,24 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
>      qdict_put_str(options, "pool", found_str);
>  
>      if (strchr(p, '@')) {
> -        found_str = qemu_rbd_next_tok(p, '@', &p);
> -        qemu_rbd_unescape(found_str);
> -        qdict_put_str(options, "image", found_str);
> +        image_name = qemu_rbd_next_tok(p, '@', &p);
>  
>          found_str = qemu_rbd_next_tok(p, ':', &p);
>          qemu_rbd_unescape(found_str);
>          qdict_put_str(options, "snapshot", found_str);
>      } else {
> -        found_str = qemu_rbd_next_tok(p, ':', &p);
> +        image_name = qemu_rbd_next_tok(p, ':', &p);
> +    }
> +    /* Check for namespace in the image_name */
> +    if (strchr(image_name, '/')) {
> +        found_str = qemu_rbd_next_tok(image_name, '/', &image_name);
>          qemu_rbd_unescape(found_str);
> -        qdict_put_str(options, "image", found_str);
> +        qdict_put_str(options, "nspace", found_str);
> +    } else {
> +        qdict_put_str(options, "nspace", "");
>      }
> +    qemu_rbd_unescape(image_name);
> +    qdict_put_str(options, "image", image_name);
>      if (!p) {
>          goto done;
>      }
> @@ -343,6 +350,11 @@ static QemuOptsList runtime_opts = {
>              .type = QEMU_OPT_STRING,
>              .help = "Rados pool name",
>          },
> +        {
> +            .name = "nspace",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Rados namespace name in the pool",
> +        },
>          {
>              .name = "image",
>              .type = QEMU_OPT_STRING,
> @@ -472,6 +484,7 @@ static int coroutine_fn qemu_rbd_co_create_opts(const char *filename,
>      loc->has_conf = !!loc->conf;
>      loc->user     = g_strdup(qdict_get_try_str(options, "user"));
>      loc->has_user = !!loc->user;
> +    loc->nspace   = g_strdup(qdict_get_try_str(options, "nspace"));
>      loc->image    = g_strdup(qdict_get_try_str(options, "image"));
>      keypairs      = qdict_get_try_str(options, "=keyvalue-pairs");
>  
> @@ -648,6 +661,11 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
>          error_setg_errno(errp, -r, "error opening pool %s", opts->pool);
>          goto failed_shutdown;
>      }
> +    /*
> +     * Set the namespace after opening the io context on the pool,
> +     * if nspace == NULL or if nspace == "", it is just as we did nothing
> +     */
> +    rados_ioctx_set_namespace(*io_ctx, opts->nspace);
>  
>      return 0;
>  
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0cf68fea14..9ebc020e93 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3657,6 +3657,8 @@
>  #
>  # @pool:               Ceph pool name.
>  #
> +# @nspace:             Rados namespace name in the Ceph pool.
> +#

I think we need to add (Since: 5.0).

The patch LGTM, but I'd like to use 'namespace' instead of cryptic
'nspace'. (as BlockdevOptionsNVMe did)
What do you think?


With those fixed:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks,
Stefano

>  # @image:              Image name in the Ceph pool.
>  #
>  # @conf:               path to Ceph configuration file.  Values
> @@ -3683,6 +3685,7 @@
>  ##
>  { 'struct': 'BlockdevOptionsRbd',
>    'data': { 'pool': 'str',
> +            'nspace': 'str',
>              'image': 'str',
>              '*conf': 'str',
>              '*snapshot': 'str',
> -- 
> 2.24.1
> 
> 

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

> 
> 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 ?
> 
> With those fixed:
> 
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> 
> Thanks,
> Stefano

Regards,
Florian

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.

> 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 <sgarzare@redhat.com>
> >
> > Thanks,
> > Stefano
>
> Regards,
> Florian

-- 
Jason

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 ?

Exactly, as Jason suggested is part of documentation,

Following the file, I mean something like that:

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0cf68fea14..9ebc020e93 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3657,6 +3657,8 @@
 #
 # @pool:               Ceph pool name.
 #
+# @namespace:          Rados namespace name in the Ceph pool. (Since: 5.0)
+#
 # @image:              Image name in the Ceph pool.
 #
 # @conf:               path to Ceph configuration file.  Values


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

Agree, I'd avoid the 'rbd_' prefix.

Thanks,
Stefano

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

On 12/20/19 11:17 AM, Florian Florensa wrote:

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

The whole point of the mangling of 'q_namespace' in the C code is so 
that you can have a SANE name in the qapi, without tripping up 
compilation in a C++ compiler where 'namespace' is a reserved word 
(since we do have parts of qemu compiled by c++).  I'd go with just 
'namespace', rather than 'pool-namespace' (note that if you DO go with a 
longer name, we prefer - over _ in qapi names).

>>>> With those fixed:
>>>>
>>>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

But see my other comment upthread about making the new parameter 
optional, to avoid breaks with older qapi clients.

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

On 12/19/19 7:34 AM, Florian Florensa wrote:
> Starting from ceph Nautilus, RBD has support for namespaces, allowing
> for finer grain ACLs on images inside a pool, and tenant isolation.
> 
> In the rbd cli tool documentation, the new image-spec and snap-spec are :
>   - [pool-name/[namespace-name/]]image-name
>   - [pool-name/[namespace-name/]]image-name@snap-name
> 
> When using an non namespace's enabled qemu, it complains about not
> finding the image called namespace-name/image-name, thus we only need to
> parse the image once again to find if there is a '/' in its name, and if
> there is, use what is before it as the name of the namespace to later
> pass it to rados_ioctx_set_namespace.
> rados_ioctx_set_namespace if called with en empty string or a null
> pointer as the namespace parameters pretty much does nothing, as it then
> defaults to the default namespace.
> 
> The namespace is extracted inside qemu_rbd_parse_filename, stored in the
> qdict, and used in qemu_rbd_connect to make it work with both qemu-img,
> and qemu itself.
> 
> Signed-off-by: Florian Florensa <fflorensa@online.net>
> ---

> +++ b/qapi/block-core.json
> @@ -3657,6 +3657,8 @@
>   #
>   # @pool:               Ceph pool name.
>   #
> +# @nspace:             Rados namespace name in the Ceph pool.
> +#

Needs a '(since 5.0)' tag.

>   # @image:              Image name in the Ceph pool.
>   #
>   # @conf:               path to Ceph configuration file.  Values
> @@ -3683,6 +3685,7 @@
>   ##
>   { 'struct': 'BlockdevOptionsRbd',
>     'data': { 'pool': 'str',
> +            'nspace': 'str',

This makes a new argument mandatory, which breaks expectations of older 
clients that failed to provide it. You probably want to make it 
'*nspace', and have a sane default when the argument is not present.

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