[PATCH v8 2/3] conf: Support to parse rbd namespace from source name

[Peter Krempa]

On Wed, May 26, 2021 at 21:35:11 +0800, Han Han wrote:
> Signed-off-by: Han Han <hhan at redhat.com>
> ---
>  docs/formatdomain.rst          | 16 ++++++++++++
>  src/conf/domain_conf.c         | 47 +++++++++++++++++++++++++++++++---
>  src/conf/storage_source_conf.c |  2 ++
>  src/conf/storage_source_conf.h |  1 +
>  4 files changed, 62 insertions(+), 4 deletions(-)

[...]

> @@ -2570,6 +2580,12 @@ paravirtualized driver is specified via the ``disk`` element.
>        the host by the ``nbd_tls`` and ``nbd_tls_x509_cert_dir`` in
>        /etc/libvirt/qemu.conf. ('tls' :since:`Since 4.5.0` )
>  
> +      For "rbd", the ``name`` attribute could be two formats: the format of
> +      ``pool_name/image_name`` includes the rbd pool name and image name with
> +      default rbd pool namespace; for the customized namespace, the format is
> +      ``pool_name/namespace/image_name`` ( :since:`Since 6.9.0 and QEMU 5.0` ).
> +      The pool name, namespace and image are separated by slash.

7.5.0 at this point.

> +
>        For protocols ``http`` and ``https`` an optional attribute ``query``
>        specifies the query string. ( :since:`Since 6.2.0` )
>  
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6d90041bf8..eac59dd00e 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8197,6 +8197,7 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
>  {
>      virStorageNetProtocol protocol;
>      xmlNodePtr tmpnode;
> +    char **tmp_split_paths;
>  
>      if (virXMLPropEnum(node, "protocol", virStorageNetProtocolTypeFromString,
>                         VIR_XML_PROP_REQUIRED, &protocol) < 0)
> @@ -8226,8 +8227,7 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
>      /* for historical reasons we store the volume and image name in one XML
>       * element although it complicates thing when attempting to access them. */
>      if (src->path &&
> -        (src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER ||
> -         src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) {
> +        src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER) {
>          char *tmp;
>          if (!(tmp = strchr(src->path, '/')) ||
>              tmp == src->path) {
> @@ -8244,6 +8244,41 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
>          tmp[0] = '\0';
>      }
>  
> +    /* the name of rbd could be <pool>/<image> or <pool>/<namespace>/<image> */
> +    if (src->path &&
> +        src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD &&

This algorithm is still needlessly obscure and very hard to follow.


> +        (tmp_split_paths = g_strsplit(src->path, "/", 3))) {
> +        if (virStringIsEmpty(tmp_split_paths[0]) ||
> +            !tmp_split_paths[1] ||
> +            STREQ_NULLABLE(tmp_split_paths[2], "") ||

Firstly on 3 lines you have 3 distinct empty sting checks.

> +            (virStringIsEmpty(tmp_split_paths[1]) &&
> +             !tmp_split_paths[2])) {
> +            virStringListFreeCount(tmp_split_paths, 3);

Then there's no need for explicit freeing just declare the helper
variable as g_auto(GStrv).

Additionally put it into the scope here by making the top level
condition just: 
    if (src->path && src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
    ...

> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("can't split path '%s' into pool name, pool "
> +                             "namespace, image name OR pool name, image name"),

error string should be a signle line. Also the error format is not using
the expected format that you require. (it should contain the slashes).

> +                             src->path);
> +            return -1;
> +        }
> +
> +        VIR_FREE(src->path);
> +        src->volume = g_strdup(tmp_split_paths[0]);
> +        /* the format of <pool>/<image> */
> +        if (!tmp_split_paths[2])
> +            src->path = g_strdup(tmp_split_paths[1]);
> +
> +        if (tmp_split_paths[2]) {

this is the 'else' section of the above condition.

> +            /* the format of <pool>/<ns>/<image> */
> +            if (STRNEQ_NULLABLE(tmp_split_paths[1], ""))

This check is duplicated.

> +                src->ns = g_strdup(tmp_split_paths[1]);
> +
> +            /* the format of <pool>//<image> */
> +            src->path = g_strdup(tmp_split_paths[2]);

Well, so the qemu parser for the stringified name actually supports
escape sequences.

> +        }
> +
> +        virStringListFreeCount(tmp_split_paths, 3);


I suggest the following implementation which should have the same logic
as what you've proposed:

    /* the name of rbd could be <pool>/<image> or <pool>/<namespace>/<image> */
    if (src->path && src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
        g_auto(GStrv) path_components = g_strsplit(src->path, "/", 3);
        size_t npath_components = g_strv_length(path_components);

        if (npath_components < 2 ||
            virStringIsEmpty(path_components[0]) ||
            virStringIsEmpty(path_components[npath_components - 1])) {
            virReportError(VIR_ERR_XML_ERROR,
                           _("path '%s' doesn't consist of 'pool_name/image_name', or 'pool_name/namespace_name/image_name'"),
                             src->path);
            return -1;
        }

        VIR_FREE(src->path);
        src->volume = g_strdup(path_components[0]);

        if (npath_components == 2 ||
            virStringIsEmpty(path_components[1])) {
            src->path = g_strdup(path_components[1]);
        } else {
            src->ns = g_strdup(path_components[1]);
            src->path = g_strdup(path_components[2]);
        }
    }


> +    }
> +
>      /* snapshot currently works only for remote disks */
>      src->snapshot = virXPathString("string(./snapshot/@name)", ctxt);
>  
> @@ -22948,8 +22983,12 @@ virDomainDiskSourceFormatNetwork(virBuffer *attrBuf,
>      virBufferAsprintf(attrBuf, " protocol='%s'",
>                        virStorageNetProtocolTypeToString(src->protocol));
>  
> -    if (src->volume)
> -        path = g_strdup_printf("%s/%s", src->volume, src->path);
> +    if (src->volume) {
> +        if (src->ns)
> +            path = g_strdup_printf("%s/%s/%s", src->volume, src->ns, src->path);
> +        else
> +            path = g_strdup_printf("%s/%s", src->volume, src->path);
> +    }
>  
>      virBufferEscapeString(attrBuf, " name='%s'", path ? path : src->path);
>      virBufferEscapeString(attrBuf, " query='%s'", src->query);

The series is also missing the parsers for the backing store strings
which we parse from the image metadata in cases when the backing chain
is not fully defined in the XML.

There's also a possibility that a similar treatment will be needed in
the URI parser for the RBD case, but I'm not sure whether qemu accepts
URIs as RBD source.

Namely:

virStorageSourceParseRBDColonString and TEST_BACKING_PARSE test cases
in tests/virstoragetest.c

and

virStorageSourceParseBackingJSONRBD and also a test case in the above
mentioned test file.