On 11/12/2014 08:47 AM, Peter Krempa wrote:

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.

<p></p>

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

<p></p> ACK - adding the <p></p> is mostly a visual thing - it separates the table from the text a bit... not sure if there's another way to do it as my html skills are spartan. John

+ 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

On 11/12/2014 08:47 AM, Peter Krempa wrote:

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 */

s/apropriate/appropriate s/thevir.../the vir.../ Too bad there wasn't some way to automagically detect this... ACK John

struct _virStorageSource { int type; /* virStorageType */ char *path;

On 11/12/2014 08:47 AM, Peter Krempa wrote:

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(-)

I see 'virStorageSourceNewFromBackingRelative' also has the same issue. Could use this opportunity to fix that - or modify the API to adjust the return and only do the alloc if source->nhosts > 0. I also note that virStorageNetHostDefCopy is only used in virstoragefile.c too - cause for statification? ACK - I'll let you figure the best option... Definitely need a check in both callers though. John

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

On 11/12/2014 08:47 AM, Peter Krempa wrote:

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;

Mostly code motion, but I see here in order to avoid the memory leak of 'secrettype' in the old code, you're just using "ceph" here (or perhaps dragging in the former definition...) Not that it could ever change, but you could use virStorageAuthTypeFromString(VIR_STORAGE_AUTH_TYPE_CEPHX) since it's defined in here... Of course the secrettype would need to be promoted to the top of the function and then VIR_FREE()'d appropriately

+ 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); +

I assume "ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ACK John

typedef int (*virStorageFileSimplifyPathReadlinkCallback)(const char *path, char **link,

On 11/12/2014 08:47 AM, Peter Krempa wrote:

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]) {

IOW: If someone provided just "nbd:", right?

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);

How could this ever have happened? We have : if (!backing[1]) error else if (STREQ(backing[1], "unix")) ... else if (!backing[1]) different error

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 someone provides "nbd:HOSTNAME:PORT:exportnam=EXPORTNAME" by mistake are they never told of their error? I know - we're not in the business of validating mistakes, but it seems a shame to avoid the opportunity... ACK anyway, John

- 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; - }

On 11/12/2014 08:47 AM, Peter Krempa wrote:

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 @@ <driver name="qemu" type="raw"/> <source protocol="rbd" name="image_name2"> <host name="hostname" port="7000"/> + <snapshot name="snapname"/> </source> <target dev="hdc" bus="ide"/> <auth username='myuser'> @@ -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>)

To follow other usages (adjust last 2 lines): source for storage protocols. Supported for 'rbd' <span class="since">since 1.2.11 (QEMU only).</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"); } + if (src->snapshot)

Rest is fine, ACK John

+ 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'/>

On 11/12/2014 08:47 AM, Peter Krempa wrote:

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 @@ <source protocol="rbd" name="image_name2"> <host name="hostname" port="7000"/> <snapshot name="snapname"/> + <config file="/path/to/file"/> </source> <target dev="hdc" bus="ide"/> <auth username='myuser'> @@ -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)

So this reads funny/strange... In particular, "...attribute of config element allows to specify..." Perhaps, "The <code>file</code> attribute for the <code>config</code> element provides a fully qualified path to a configuration file to be provided as a parameter to the client of a networked storage protocol. Supported for 'rbd' <span class="since">since 1.2.11 (QEMU only).</span>" NOTE: Going for consistent usage w/ 11/12... previously snapshot had "storage systems" and this has "storage backends". Since both seem to be keyed of which storage protocol is used, I went with storage protocol.

+ </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);

if (src->configfile) Rest is fine, ACK John

+ 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'/>