The docs describing the <host> element that are under the <source> element in the XML document were incorrectly placed under the <disk> element. Move them to the correct place. --- docs/formatdomain.html.in | 104 ++++++++++++++++++++++++---------------------- 1 file changed, 54 insertions(+), 50 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d4189e6..4f44bc0 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1953,6 +1953,60 @@ may have zero or more <code>host</code> sub-elements used to specify the hosts to connect. </p> + + <dl> + <dt><code>host</code></dt> + <dd>The <code>host</code> element supports 4 attributes, viz. "name", + "port", "transport" and "socket", which specify the hostname, + the port number, transport type and path to socket, respectively. + The meaning of this element and the number of the elements depend + on the protocol attribute. + <table class="top_table"> + <tr> + <th> Protocol </th> + <th> Meaning </th> + <th> Number of hosts </th> + <th> Default port </th> + </tr> + <tr> + <td> nbd </td> + <td> a server running nbd-server </td> + <td> only one </td> + <td> 10809 </td> + </tr> + <tr> + <td> iscsi </td> + <td> an iSCSI server </td> + <td> only one </td> + <td> 3260 </td> + </tr> + <tr> + <td> rbd </td> + <td> monitor servers of RBD </td> + <td> one or more </td> + <td> 6789 </td> + </tr> + <tr> + <td> sheepdog </td> + <td> one of the sheepdog servers (default is localhost:7000) </td> + <td> zero or one </td> + <td> 7000 </td> + </tr> + <tr> + <td> gluster </td> + <td> a server running glusterd daemon </td> + <td> only one </td> + <td> 24007 </td> + </tr> + </table> + gluster supports "tcp", "rdma", "unix" as valid values for the + transport attribute. nbd supports "tcp" and "unix". Others only + support "tcp". If nothing is specified, "tcp" is assumed. If the + transport is "unix", the socket attribute specifies the path to an + AF_UNIX socket. + </dd> + </dl> + <p> For a "file" or "volume" disk type which represents a cdrom or floppy (the <code>device</code> attribute), it is possible to define @@ -2308,56 +2362,6 @@ characters. <span class='since'>Since 1.0.1</span> </dd> - <dt><code>host</code></dt> - <dd>The <code>host</code> element supports 4 attributes, viz. "name", - "port", "transport" and "socket", which specify the hostname, the port - number, transport type and path to socket, respectively. The meaning - of this element and the number of the elements depend on the protocol - attribute. - <table class="top_table"> - <tr> - <th> Protocol </th> - <th> Meaning </th> - <th> Number of hosts </th> - <th> Default port </th> - </tr> - <tr> - <td> nbd </td> - <td> a server running nbd-server </td> - <td> only one </td> - <td> 10809 </td> - </tr> - <tr> - <td> iscsi </td> - <td> an iSCSI server </td> - <td> only one </td> - <td> 3260 </td> - </tr> - <tr> - <td> rbd </td> - <td> monitor servers of RBD </td> - <td> one or more </td> - <td> 6789 </td> - </tr> - <tr> - <td> sheepdog </td> - <td> one of the sheepdog servers (default is localhost:7000) </td> - <td> zero or one </td> - <td> 7000 </td> - </tr> - <tr> - <td> gluster </td> - <td> a server running glusterd daemon </td> - <td> only one </td> - <td> 24007 </td> - </tr> - </table> - gluster supports "tcp", "rdma", "unix" as valid values for the - transport attribute. nbd supports "tcp" and "unix". Others only - support "tcp". If nothing is specified, "tcp" is assumed. If the - transport is "unix", the socket attribute specifies the path to an - AF_UNIX socket. - </dd> <dt><code>address</code></dt> <dd>If present, the <code>address</code> element ties the disk to a given slot of a controller (the -- 2.1.0

On 11/20/14 16:08, John Ferlan wrote: I'm going to add <p>aragraphs around the text blocks. That will automagically add a break before the table and will avoid having empty paragraph block.

The escaping is applied only to the string, not the formating argument. State this fact in the docs. --- src/util/virbuffer.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 52ffa08..30a4183 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -506,8 +506,9 @@ virBufferEscapeSexpr(virBufferPtr buf, * @str: the string argument which needs to be escaped * * Do a formatted print with a single string to a buffer. Any characters - * in the provided list are escaped with the given escape. Auto indentation - * may be applied. + * in the provided list that are contained in @str are escaped with the + * given escape. Escaping is not applied to characters specified in @format. + * Auto indentation may be applied. */ void virBufferEscape(virBufferPtr buf, char escape, const char *toescape, -- 2.1.0

As we now have a deep copy function for struct virStorageSource add a notice that extensions of the structure require also appropriate changes to the virStorageSourceCopy func. --- src/util/virstoragefile.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 2583e10..7f3f353 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -228,9 +228,11 @@ typedef virStorageDriverData *virStorageDriverDataPtr; typedef struct _virStorageSource virStorageSource; typedef virStorageSource *virStorageSourcePtr; -/* Stores information related to a host resource. In the case of - * backing chains, multiple source disks join to form a single guest - * view. */ +/* Stores information related to a host resource. In the case of backing + * chains, multiple source disks join to form a single guest view. + * + * IMPORTANT: When adding fields to this struct it's also necessary to add + * apropriate code to thevirStorageSourceCopy deep copy function */ struct _virStorageSource { int type; /* virStorageType */ char *path; -- 2.1.0

If there are no hosts for a storage source virStorageSourceCopy would try to copy them anyways. As the success of virStorageNetHostDefCopy is determined by returning a pointer and malloc of 0 elements might return NULL according to the implementation, the result of the copy function may vary. Fix this by copying the hosts array only if there are hosts defined. --- src/util/virstoragefile.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 8e9d115..c2d5b46 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1852,9 +1852,12 @@ virStorageSourceCopy(const virStorageSource *src, VIR_STRDUP(ret->compat, src->compat) < 0) goto error; - if (!(ret->hosts = virStorageNetHostDefCopy(src->nhosts, src->hosts))) - goto error; - ret->nhosts = src->nhosts; + if (src->nhosts) { + if (!(ret->hosts = virStorageNetHostDefCopy(src->nhosts, src->hosts))) + goto error; + + ret->nhosts = src->nhosts; + } if (src->srcpool && !(ret->srcpool = virStorageSourcePoolDefCopy(src->srcpool))) -- 2.1.0

On 11/20/14 16:10, John Ferlan wrote: Avoiding the allocation would inhibit checking for failed allocation. This would require to rewrite it to return an int and return the allocated array as an argument. I'll stick with adding an explicit check for now.

To allow reuse this non-trivial parser code in the backing store parser this part of the command line parser needs to be split out into a separate funciton. --- src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 133 +++--------------------------------------- src/util/virstoragefile.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 3 + 4 files changed, 156 insertions(+), 125 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b8f35e8..b33722e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1991,6 +1991,7 @@ virStorageSourceInitChainElement; virStorageSourceIsEmpty; virStorageSourceIsLocalStorage; virStorageSourceNewFromBacking; +virStorageSourceParseRBDColonString; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; virStorageSourcePoolModeTypeToString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 19e8f9d..021ec07 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2540,137 +2540,20 @@ qemuGetSecretString(virConnectPtr conn, } -static int qemuAddRBDHost(virDomainDiskDefPtr disk, char *hostport) +static int +qemuParseRBDString(virDomainDiskDefPtr disk) { - char *port; - size_t skip; - char **parts; - - if (VIR_EXPAND_N(disk->src->hosts, disk->src->nhosts, 1) < 0) - return -1; - - if ((port = strchr(hostport, ']'))) { - /* ipv6, strip brackets */ - hostport += 1; - skip = 3; - } else { - port = strstr(hostport, "\\:"); - skip = 2; - } - - if (port) { - *port = '\0'; - port += skip; - if (VIR_STRDUP(disk->src->hosts[disk->src->nhosts - 1].port, port) < 0) - goto error; - } else { - if (VIR_STRDUP(disk->src->hosts[disk->src->nhosts - 1].port, "6789") < 0) - goto error; - } - - parts = virStringSplit(hostport, "\\:", 0); - if (!parts) - goto error; - disk->src->hosts[disk->src->nhosts-1].name = virStringJoin((const char **)parts, ":"); - virStringFreeList(parts); - if (!disk->src->hosts[disk->src->nhosts-1].name) - goto error; + char *source = disk->src->path; + int ret; - disk->src->hosts[disk->src->nhosts-1].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - disk->src->hosts[disk->src->nhosts-1].socket = NULL; + disk->src->path = NULL; - return 0; + ret = virStorageSourceParseRBDColonString(source, disk->src); - error: - VIR_FREE(disk->src->hosts[disk->src->nhosts-1].port); - VIR_FREE(disk->src->hosts[disk->src->nhosts-1].name); - return -1; + VIR_FREE(source); + return ret; } -/* disk->src initially has everything after the rbd: prefix */ -static int qemuParseRBDString(virDomainDiskDefPtr disk) -{ - char *options = NULL; - char *p, *e, *next; - virStorageAuthDefPtr authdef = NULL; - - p = strchr(disk->src->path, ':'); - if (p) { - if (VIR_STRDUP(options, p + 1) < 0) - goto error; - *p = '\0'; - } - - /* options */ - if (!options) - return 0; /* all done */ - - p = options; - while (*p) { - /* find : delimiter or end of string */ - for (e = p; *e && *e != ':'; ++e) { - if (*e == '\\') { - e++; - if (*e == '\0') - break; - } - } - if (*e == '\0') { - next = e; /* last kv pair */ - } else { - next = e + 1; - *e = '\0'; - } - - if (STRPREFIX(p, "id=")) { - const char *secrettype; - /* formulate authdef for disk->src->auth */ - if (VIR_ALLOC(authdef) < 0) - goto error; - - if (VIR_STRDUP(authdef->username, p + strlen("id=")) < 0) - goto error; - secrettype = virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_CEPH); - if (VIR_STRDUP(authdef->secrettype, secrettype) < 0) - goto error; - disk->src->auth = authdef; - authdef = NULL; - - /* Cannot formulate a secretType (eg, usage or uuid) given - * what is provided. - */ - } - if (STRPREFIX(p, "mon_host=")) { - char *h, *sep; - - h = p + strlen("mon_host="); - while (h < e) { - for (sep = h; sep < e; ++sep) { - if (*sep == '\\' && (sep[1] == ',' || - sep[1] == ';' || - sep[1] == ' ')) { - *sep = '\0'; - sep += 2; - break; - } - } - if (qemuAddRBDHost(disk, h) < 0) - goto error; - - h = sep; - } - } - - p = next; - } - VIR_FREE(options); - return 0; - - error: - VIR_FREE(options); - virStorageAuthDefFree(authdef); - return -1; -} static int qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri, diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index c2d5b46..f267d1a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2209,6 +2209,150 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, static int +virStorageSourceRBDAddHost(virStorageSourcePtr src, + char *hostport) +{ + char *port; + size_t skip; + char **parts; + + if (VIR_EXPAND_N(src->hosts, src->nhosts, 1) < 0) + return -1; + + if ((port = strchr(hostport, ']'))) { + /* ipv6, strip brackets */ + hostport += 1; + skip = 3; + } else { + port = strstr(hostport, "\\:"); + skip = 2; + } + + if (port) { + *port = '\0'; + port += skip; + if (VIR_STRDUP(src->hosts[src->nhosts - 1].port, port) < 0) + goto error; + } else { + if (VIR_STRDUP(src->hosts[src->nhosts - 1].port, "6789") < 0) + goto error; + } + + parts = virStringSplit(hostport, "\\:", 0); + if (!parts) + goto error; + src->hosts[src->nhosts-1].name = virStringJoin((const char **)parts, ":"); + virStringFreeList(parts); + if (!src->hosts[src->nhosts-1].name) + goto error; + + src->hosts[src->nhosts-1].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + src->hosts[src->nhosts-1].socket = NULL; + + return 0; + + error: + VIR_FREE(src->hosts[src->nhosts-1].port); + VIR_FREE(src->hosts[src->nhosts-1].name); + return -1; +} + + +int +virStorageSourceParseRBDColonString(const char *rbdstr, + virStorageSourcePtr src) +{ + char *options = NULL; + char *p, *e, *next; + virStorageAuthDefPtr authdef = NULL; + + /* optionally skip the "rbd:" prefix if provided */ + if (STRPREFIX(rbdstr, "rbd:")) + rbdstr += strlen("rbd:"); + + if (VIR_STRDUP(src->path, rbdstr) < 0) + goto error; + + p = strchr(src->path, ':'); + if (p) { + if (VIR_STRDUP(options, p + 1) < 0) + goto error; + *p = '\0'; + } + + /* options */ + if (!options) + return 0; /* all done */ + + p = options; + while (*p) { + /* find : delimiter or end of string */ + for (e = p; *e && *e != ':'; ++e) { + if (*e == '\\') { + e++; + if (*e == '\0') + break; + } + } + if (*e == '\0') { + next = e; /* last kv pair */ + } else { + next = e + 1; + *e = '\0'; + } + + if (STRPREFIX(p, "id=")) { + /* formulate authdef for src->auth */ + if (VIR_ALLOC(authdef) < 0) + goto error; + + if (VIR_STRDUP(authdef->username, p + strlen("id=")) < 0) + goto error; + + if (VIR_STRDUP(authdef->secrettype, "ceph") < 0) + goto error; + src->auth = authdef; + authdef = NULL; + + /* Cannot formulate a secretType (eg, usage or uuid) given + * what is provided. + */ + } + if (STRPREFIX(p, "mon_host=")) { + char *h, *sep; + + h = p + strlen("mon_host="); + while (h < e) { + for (sep = h; sep < e; ++sep) { + if (*sep == '\\' && (sep[1] == ',' || + sep[1] == ';' || + sep[1] == ' ')) { + *sep = '\0'; + sep += 2; + break; + } + } + + if (virStorageSourceRBDAddHost(src, h) < 0) + goto error; + + h = sep; + } + } + + p = next; + } + VIR_FREE(options); + return 0; + + error: + VIR_FREE(options); + virStorageAuthDefFree(authdef); + return -1; +} + + +static int virStorageSourceParseBackingColon(virStorageSourcePtr src, const char *path) { diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 7f3f353..b1ba73a 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -363,6 +363,9 @@ virStorageSourcePtr virStorageSourceCopy(const virStorageSource *src, bool backingChain) ATTRIBUTE_NONNULL(1); +int virStorageSourceParseRBDColonString(const char *rbdstr, + virStorageSourcePtr src); + typedef int (*virStorageFileSimplifyPathReadlinkCallback)(const char *path, char **link, -- 2.1.0

Split out the code so that the function looks homogenous after adding more protocol specific parsers. --- src/util/virstoragefile.c | 141 ++++++++++++++++++++++++++++------------------ 1 file changed, 86 insertions(+), 55 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index f267d1a..58be237 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2353,77 +2353,109 @@ virStorageSourceParseRBDColonString(const char *rbdstr, static int -virStorageSourceParseBackingColon(virStorageSourcePtr src, - const char *path) +virStorageSourceParseNBDColonString(const char *nbdstr, + virStorageSourcePtr src) { char **backing = NULL; int ret = -1; - if (!(backing = virStringSplit(path, ":", 0))) + if (!(backing = virStringSplit(nbdstr, ":", 0))) goto cleanup; - if (!backing[0] || - (src->protocol = virStorageNetProtocolTypeFromString(backing[0])) < 0) { + /* we know that backing[0] now equals to "nbd" */ + + if (VIR_ALLOC_N(src->hosts, 1) < 0) + goto cleanup; + + src->nhosts = 1; + src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + + /* format: [] denotes optional sections, uppercase are variable strings + * nbd:unix:/PATH/TO/SOCKET[:exportname=EXPORTNAME] + * nbd:HOSTNAME:PORT[:exportname=EXPORTNAME] + */ + if (!backing[1]) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("invalid backing protocol '%s'"), - NULLSTR(backing[0])); + _("missing remote information in '%s' for protocol nbd"), + nbdstr); goto cleanup; - } + } else if (STREQ(backing[1], "unix")) { + if (!backing[2]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing unix socket path in nbd backing string %s"), + nbdstr); + goto cleanup; + } - switch ((virStorageNetProtocol) src->protocol) { - case VIR_STORAGE_NET_PROTOCOL_NBD: - if (VIR_ALLOC_N(src->hosts, 1) < 0) + if (VIR_STRDUP(src->hosts->socket, backing[2]) < 0) goto cleanup; - src->nhosts = 1; - src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - /* format: [] denotes optional sections, uppercase are variable strings - * nbd:unix:/PATH/TO/SOCKET[:exportname=EXPORTNAME] - * nbd:HOSTNAME:PORT[:exportname=EXPORTNAME] - */ + } else { if (!backing[1]) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("missing remote information in '%s' for protocol nbd"), - path); + _("missing host name in nbd string '%s'"), + nbdstr); goto cleanup; - } else if (STREQ(backing[1], "unix")) { - if (!backing[2]) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("missing unix socket path in nbd backing string %s"), - path); - goto cleanup; - } + } - if (VIR_STRDUP(src->hosts->socket, backing[2]) < 0) - goto cleanup; + if (VIR_STRDUP(src->hosts->name, backing[1]) < 0) + goto cleanup; - } else { - if (!backing[1]) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("missing host name in nbd string '%s'"), - path); - goto cleanup; - } + if (!backing[2]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing port in nbd string '%s'"), + nbdstr); + goto cleanup; + } - if (VIR_STRDUP(src->hosts->name, backing[1]) < 0) - goto cleanup; + if (VIR_STRDUP(src->hosts->port, backing[2]) < 0) + goto cleanup; + } - if (!backing[2]) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("missing port in nbd string '%s'"), - path); - goto cleanup; - } + if (backing[3] && STRPREFIX(backing[3], "exportname=")) { + if (VIR_STRDUP(src->path, backing[3] + strlen("exportname=")) < 0) + goto cleanup; + } - if (VIR_STRDUP(src->hosts->port, backing[2]) < 0) - goto cleanup; - } + ret = 0; - if (backing[3] && STRPREFIX(backing[3], "exportname=")) { - if (VIR_STRDUP(src->path, backing[3] + strlen("exportname=")) < 0) - goto cleanup; - } - break; + cleanup: + virStringFreeList(backing); + + return ret; +} + + +static int +virStorageSourceParseBackingColon(virStorageSourcePtr src, + const char *path) +{ + char *protocol = NULL; + const char *p; + int ret = -1; + + if (!(p = strchr(path, ':'))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid backing protocol string '%s'"), + path); + goto cleanup; + } + + if (VIR_STRNDUP(protocol, path, p - path) < 0) + goto cleanup; + + if ((src->protocol = virStorageNetProtocolTypeFromString(protocol)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid backing protocol '%s'"), + protocol); + goto cleanup; + } + + switch ((virStorageNetProtocol) src->protocol) { + case VIR_STORAGE_NET_PROTOCOL_NBD: + if (virStorageSourceParseNBDColonString(path, src) < 0) + goto cleanup; + break; case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: case VIR_STORAGE_NET_PROTOCOL_RBD: @@ -2431,7 +2463,7 @@ virStorageSourceParseBackingColon(virStorageSourcePtr src, case VIR_STORAGE_NET_PROTOCOL_NONE: virReportError(VIR_ERR_INTERNAL_ERROR, _("backing store parser is not implemented for protocol %s"), - backing[0]); + protocol); goto cleanup; case VIR_STORAGE_NET_PROTOCOL_HTTP: @@ -2443,16 +2475,15 @@ virStorageSourceParseBackingColon(virStorageSourcePtr src, case VIR_STORAGE_NET_PROTOCOL_GLUSTER: virReportError(VIR_ERR_INTERNAL_ERROR, _("malformed backing store path for protocol %s"), - backing[0]); + protocol); goto cleanup; } ret = 0; cleanup: - virStringFreeList(backing); + VIR_FREE(protocol); return ret; - } -- 2.1.0

On 11/20/14 16:16, John Ferlan wrote: Yep, that doesn't make sense. That is probably an artifact from the time I wrote the func :/. I'll remove it in a follow up to keep the split-out patch intact in the semantic way. Yes we could do a better job here. Additionally there is a second function doing similar stuff in the qemu_command file. I'll post a patch to unify and clean up them as a follow up as this would be out of scope of the split-out patch. Peter

Some storage systems have internal support for snapshots. Libvirt should be able to select a correct snapshot when starting a VM. This patch adds a XML element to select a storage source snapshot for the RBD protocol which supports this feature. --- docs/formatdomain.html.in | 18 +++++++--- docs/schemas/domaincommon.rng | 8 +++++ src/conf/domain_conf.c | 38 +++++++++++++++++++--- src/conf/domain_conf.h | 1 + src/conf/snapshot_conf.c | 6 ++-- src/qemu/qemu_command.c | 3 ++ src/util/virstoragefile.c | 8 +++++ src/util/virstoragefile.h | 1 + .../qemuxml2argv-disk-drive-network-rbd.args | 4 +++ .../qemuxml2argv-disk-drive-network-rbd.xml | 17 ++++++++++ 10 files changed, 94 insertions(+), 10 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 4f44bc0..fc35c5a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1676,6 +1676,7 @@ &lt;driver name="qemu" type="raw"/&gt; &lt;source protocol="rbd" name="image_name2"&gt; &lt;host name="hostname" port="7000"/&gt; + &lt;snapshot name="snapname"/&gt; &lt;/source&gt; &lt;target dev="hdc" bus="ide"/&gt; &lt;auth username='myuser'&gt; @@ -1949,14 +1950,16 @@ is only valid when the specified storage volume is of 'file' or 'block' type). <p> - When the disk <code>type</code> is "network", the <code>source</code> - may have zero or more <code>host</code> sub-elements used to - specify the hosts to connect. + The <code>source</code> element may contain the following sub elements: </p> <dl> <dt><code>host</code></dt> - <dd>The <code>host</code> element supports 4 attributes, viz. "name", + <dd>When the disk <code>type</code> is "network", the <code>source</code> + may have zero or more <code>host</code> sub-elements used to + specify the hosts to connect. + + The <code>host</code> element supports 4 attributes, viz. "name", "port", "transport" and "socket", which specify the hostname, the port number, transport type and path to socket, respectively. The meaning of this element and the number of the elements depend @@ -2005,6 +2008,13 @@ transport is "unix", the socket attribute specifies the path to an AF_UNIX socket. </dd> + <dt><code>snapshot</code></dt> + <dd> + The <code>name</code> attribute of <code>snapshot</code> element can + optionally specify an internal snapshot name to be used as the + source for storage systems such as rbd. + (<span class="since">Since 1.2.11 for the qemu driver.</span>) + </dd> </dl> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 6863ec6..154d222 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1452,6 +1452,14 @@ </choice> </element> </zeroOrMore> + <optional> + <element name="snapshot"> + <attribute name="name"> + <ref name="genericName"/> + </attribute> + <empty/> + </element> + </optional> <empty/> </element> </interleave> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2c65276..37a8042 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3162,6 +3162,22 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, return -1; } + /* verify disk source */ + if (dev->type == VIR_DOMAIN_DEVICE_DISK) { + virDomainDiskDefPtr disk = dev->data.disk; + + /* internal snapshots are currently supported only with rbd: */ + if (virStorageSourceGetActualType(disk->src) != VIR_STORAGE_TYPE_NETWORK && + disk->src->protocol != VIR_STORAGE_NET_PROTOCOL_RBD) { + if (disk->src->snapshot) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("<snapshot> element is currently supported " + "only with 'rbd' disks")); + return -1; + } + } + } + return 0; } @@ -5316,10 +5332,14 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node, int virDomainDiskSourceParse(xmlNodePtr node, + xmlXPathContextPtr ctxt, virStorageSourcePtr src) { int ret = -1; char *protocol = NULL; + xmlNodePtr saveNode = ctxt->node; + + ctxt->node = node; switch ((virStorageType)src->type) { case VIR_STORAGE_TYPE_FILE: @@ -5372,6 +5392,9 @@ virDomainDiskSourceParse(xmlNodePtr node, tmp[0] = '\0'; } + /* snapshot currently works only for remote disks */ + src->snapshot = virXPathString("string(./snapshot/@name)", ctxt); + if (virDomainStorageHostParse(node, &src->hosts, &src->nhosts) < 0) goto cleanup; break; @@ -5397,6 +5420,7 @@ virDomainDiskSourceParse(xmlNodePtr node, cleanup: VIR_FREE(protocol); + ctxt->node = saveNode; return ret; } @@ -5452,7 +5476,7 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, goto cleanup; } - if (virDomainDiskSourceParse(source, backingStore) < 0 || + if (virDomainDiskSourceParse(source, ctxt, backingStore) < 0 || virDomainDiskBackingStoreParse(ctxt, backingStore) < 0) goto cleanup; @@ -5562,7 +5586,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, xmlStrEqual(cur->name, BAD_CAST "source")) { sourceNode = cur; - if (virDomainDiskSourceParse(cur, def->src) < 0) + if (virDomainDiskSourceParse(cur, ctxt, def->src) < 0) goto error; source = def->src->path; @@ -5728,7 +5752,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, _("mirror requires source element")); goto error; } - if (virDomainDiskSourceParse(mirrorNode, def->mirror) < 0) + if (virDomainDiskSourceParse(mirrorNode, ctxt, + def->mirror) < 0) goto error; } ready = virXMLPropString(cur, "ready"); @@ -16154,11 +16179,12 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, VIR_FREE(path); - if (src->nhosts == 0) { + if (src->nhosts == 0 && !src->snapshot) { virBufferAddLit(buf, "/>\n"); } else { virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); + for (n = 0; n < src->nhosts; n++) { virBufferAddLit(buf, "<host"); virBufferEscapeString(buf, " name='%s'", @@ -16175,6 +16201,10 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } + + virBufferEscapeString(buf, "<snapshot name='%s'/>\n", + src->snapshot); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</source>\n"); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 530a3ca..acbf13b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2477,6 +2477,7 @@ virDomainDiskRemove(virDomainDefPtr def, size_t i); virDomainDiskDefPtr virDomainDiskRemoveByName(virDomainDefPtr def, const char *name); int virDomainDiskSourceParse(xmlNodePtr node, + xmlXPathContextPtr ctxt, virStorageSourcePtr src); bool virDomainHasDiskMirror(virDomainObjPtr vm); diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 1f83b2c..66fc2e6 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -107,6 +107,7 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) static int virDomainSnapshotDiskDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt, virDomainSnapshotDiskDefPtr def) { int ret = -1; @@ -154,7 +155,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, if (!def->src->path && xmlStrEqual(cur->name, BAD_CAST "source")) { - if (virDomainDiskSourceParse(cur, def->src) < 0) + if (virDomainDiskSourceParse(cur, ctxt, def->src) < 0) goto cleanup; } else if (!def->src->format && @@ -352,7 +353,8 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, goto cleanup; def->ndisks = n; for (i = 0; i < def->ndisks; i++) { - if (virDomainSnapshotDiskDefParseXML(nodes[i], &def->disks[i]) < 0) + if (virDomainSnapshotDiskDefParseXML(nodes[i], ctxt, + &def->disks[i]) < 0) goto cleanup; } VIR_FREE(nodes); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 021ec07..7923842 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2988,6 +2988,9 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, virBufferStrcat(&buf, "rbd:", src->path, NULL); + if (src->snapshot) + virBufferEscape(&buf, '\\', ":", "@%s", src->snapshot); + if (username) { virBufferEscape(&buf, '\\', ":", ":id=%s", username); virBufferEscape(&buf, '\\', ":", diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index bc7c51d..efd51d2 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1849,6 +1849,7 @@ virStorageSourceCopy(const virStorageSource *src, VIR_STRDUP(ret->driverName, src->driverName) < 0 || VIR_STRDUP(ret->relPath, src->relPath) < 0 || VIR_STRDUP(ret->backingStoreRaw, src->backingStoreRaw) < 0 || + VIR_STRDUP(ret->snapshot, src->snapshot) < 0 || VIR_STRDUP(ret->compat, src->compat) < 0) goto error; @@ -2280,6 +2281,13 @@ virStorageSourceParseRBDColonString(const char *rbdstr, *p = '\0'; } + /* snapshot name */ + if ((p = strchr(src->path, '@'))) { + if (VIR_STRDUP(src->snapshot, p + 1) < 0) + goto error; + *p = '\0'; + } + /* options */ if (!options) return 0; /* all done */ diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index b1ba73a..caab0b4 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -238,6 +238,7 @@ struct _virStorageSource { char *path; int protocol; /* virStorageNetProtocol */ char *volume; /* volume name for remote storage */ + char *snapshot; /* for storage systems supporting internal snapshots */ size_t nhosts; virStorageNetHostDefPtr hosts; virStorageSourcePoolDefPtr srcpool; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args index 21d7b64..e4f1389 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args @@ -5,4 +5,8 @@ unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ -drive 'file=rbd:pool/image:auth_supported=none:\ mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;\ mon3.example.org\:6322,if=virtio,format=raw' \ +-drive file=rbd:pool/image@asdf:auth_supported=none,if=virtio,format=raw \ +-drive 'file=rbd:pool/image@foo:auth_supported=none:\ +mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;\ +mon3.example.org\:6322,if=virtio,format=raw' \ -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.xml index 37e9db5..f6accd8 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.xml @@ -29,6 +29,23 @@ </source> <target dev='vda' bus='virtio'/> </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='rbd' name='pool/image'> + <snapshot name='asdf'/> + </source> + <target dev='vdb' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + <snapshot name='foo'/> + </source> + <target dev='vdc' bus='virtio'/> + </disk> <controller type='usb' index='0'/> <controller type='ide' index='0'/> <controller type='pci' index='0' model='pci-root'/> -- 2.1.0

To be able to express some use cases of the RBD backing with libvirt, we need to be able to specify a config file for the RBD client to qemu as that is one of the commonly used options. --- docs/formatdomain.html.in | 8 ++++++++ docs/schemas/domaincommon.rng | 8 ++++++++ src/conf/domain_conf.c | 18 ++++++++++++++++-- src/qemu/qemu_command.c | 3 +++ src/util/virstoragefile.c | 5 +++++ src/util/virstoragefile.h | 2 ++ .../qemuxml2argv-disk-drive-network-rbd.args | 2 ++ .../qemuxml2argv-disk-drive-network-rbd.xml | 8 ++++++++ 8 files changed, 52 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index fc35c5a..62bca45 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1677,6 +1677,7 @@ &lt;source protocol="rbd" name="image_name2"&gt; &lt;host name="hostname" port="7000"/&gt; &lt;snapshot name="snapname"/&gt; + &lt;config file="/path/to/file"/&gt; &lt;/source&gt; &lt;target dev="hdc" bus="ide"/&gt; &lt;auth username='myuser'&gt; @@ -2015,6 +2016,13 @@ source for storage systems such as rbd. (<span class="since">Since 1.2.11 for the qemu driver.</span>) </dd> + <dt><code>config</code></dt> + <dd> + The <code>file</code> attribute of <code>config</code> element + allows to specify a configuration file for storage backends. + + (<span class="since">Since 1.2.11</span> for 'rbd' disks in qemu) + </dd> </dl> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 154d222..6b2845a 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1460,6 +1460,14 @@ <empty/> </element> </optional> + <optional> + <element name="config"> + <attribute name="file"> + <ref name="absFilePath"/> + </attribute> + <empty/> + </element> + </optional> <empty/> </element> </interleave> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 37a8042..a9a26a4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3166,7 +3166,8 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, if (dev->type == VIR_DOMAIN_DEVICE_DISK) { virDomainDiskDefPtr disk = dev->data.disk; - /* internal snapshots are currently supported only with rbd: */ + /* internal snapshots and config files are currently supported + * only with rbd: */ if (virStorageSourceGetActualType(disk->src) != VIR_STORAGE_TYPE_NETWORK && disk->src->protocol != VIR_STORAGE_NET_PROTOCOL_RBD) { if (disk->src->snapshot) { @@ -3175,6 +3176,13 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, "only with 'rbd' disks")); return -1; } + + if (disk->src->configFile) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("<config> element is currently supported " + "only with 'rbd' disks")); + return -1; + } } } @@ -5395,6 +5403,9 @@ virDomainDiskSourceParse(xmlNodePtr node, /* snapshot currently works only for remote disks */ src->snapshot = virXPathString("string(./snapshot/@name)", ctxt); + /* config file currently only works with remote disks */ + src->configFile = virXPathString("string(./config/@file)", ctxt); + if (virDomainStorageHostParse(node, &src->hosts, &src->nhosts) < 0) goto cleanup; break; @@ -16179,7 +16190,7 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, VIR_FREE(path); - if (src->nhosts == 0 && !src->snapshot) { + if (src->nhosts == 0 && !src->snapshot && !src->configFile) { virBufferAddLit(buf, "/>\n"); } else { virBufferAddLit(buf, ">\n"); @@ -16205,6 +16216,9 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, virBufferEscapeString(buf, "<snapshot name='%s'/>\n", src->snapshot); + virBufferEscapeString(buf, "<config file='%s'/>\n", + src->configFile); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</source>\n"); } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7923842..a19ad7e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3018,6 +3018,9 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, } } + if (src->configFile) + virBufferEscape(&buf, '\\', ":", ":conf=%s", src->configFile); + if (virBufferCheckError(&buf) < 0) goto cleanup; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index efd51d2..c5a309d 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1850,6 +1850,7 @@ virStorageSourceCopy(const virStorageSource *src, VIR_STRDUP(ret->relPath, src->relPath) < 0 || VIR_STRDUP(ret->backingStoreRaw, src->backingStoreRaw) < 0 || VIR_STRDUP(ret->snapshot, src->snapshot) < 0 || + VIR_STRDUP(ret->configFile, src->configFile) < 0 || VIR_STRDUP(ret->compat, src->compat) < 0) goto error; @@ -2348,6 +2349,10 @@ virStorageSourceParseRBDColonString(const char *rbdstr, } } + if (STRPREFIX(p, "conf=") && + VIR_STRDUP(src->configFile, p + strlen("conf=")) < 0) + goto error; + p = next; } VIR_FREE(options); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index caab0b4..578fbf2 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -239,6 +239,8 @@ struct _virStorageSource { int protocol; /* virStorageNetProtocol */ char *volume; /* volume name for remote storage */ char *snapshot; /* for storage systems supporting internal snapshots */ + char *configFile; /* some storage systems use config file as part of + the source definition */ size_t nhosts; virStorageNetHostDefPtr hosts; virStorageSourcePoolDefPtr srcpool; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args index e4f1389..f634e8d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args @@ -9,4 +9,6 @@ mon3.example.org\:6322,if=virtio,format=raw' \ -drive 'file=rbd:pool/image@foo:auth_supported=none:\ mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;\ mon3.example.org\:6322,if=virtio,format=raw' \ +-drive file=rbd:pool/image@foo:auth_supported=none:\ +conf=/blah/test.conf,if=virtio,format=raw \ -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.xml index f6accd8..9ffe633 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.xml @@ -46,6 +46,14 @@ </source> <target dev='vdc' bus='virtio'/> </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='rbd' name='pool/image'> + <snapshot name='foo'/> + <config file='/blah/test.conf'/> + </source> + <target dev='vdd' bus='virtio'/> + </disk> <controller type='usb' index='0'/> <controller type='ide' index='0'/> <controller type='pci' index='0' model='pci-root'/> -- 2.1.0

On 11/20/14 16:17, John Ferlan wrote: virBufferEscapeString automagically doesn't do anything if the third argument is NULL. The design is to avoid the need to wrap the calls by if in the XML formatter :) Thanks. Peter