On Mon, Jun 07, 2021 at 16:34:39 +0200, Peter Krempa wrote:
> On Wed, May 26, 2021 at 21:35:11 +0800, Han Han wrote:
> > Signed-off-by: Han Han <hhan@xxxxxxxxxx>
> > ---
> > 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]);
> }
Actually the above hunk should be:
if (npath_components == 2) {
src->path = g_strdup(path_components[1]);
} else if (virStringIsEmpty(path_components[1])) {
src->path = g_strdup(path_components[2]);
} 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.
>
>
>
