currently libvirt has the capability to parse only one host and convert that into URI formatted string, with the help of this patch libvirt will be able to parse multiple hosts from the domain xml and can convert that into JSON formatted string before: ------ example.xml: ... <disk type='network' device='disk'> <driver name='qemu' type='qcow2' cache='none'/> <source protocol='gluster' name='testvol/a.qcow2'> <host name='1.2.3.4' port='24007' transport="tcp"/> </source> <target dev='vda' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </disk> ... resultant string: file=gluster://1.2.3.4:24007/testvol/a.qcow2, \ if=none,id=drive-virtio-disk0,format=qcow2,cache=none after: ----- example.xml: ... <disk type='network' device='disk'> <driver name='qemu' type='qcow2' cache='none'/> <source protocol='gluster' name='testvol/a.qcow2'> <host name='1.2.3.4' port='24009' transport="tcp"/> <host name='3.4.5.6' port="24008"/> <host name='5.6.7.8' /> </source> <target dev='vda' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </disk> ... resultant string: -drive file=json:{

"file": { "driver": "gluster",, "volname": "testvol",, "image-path": "/a.qcow2",, "volfile-servers": [ { "server": "1.2.3.4",, "port": 24009,, "transport": "tcp" },, { "server": "3.4.5.6",, "port": 24008,, "transport": "tcp" },, { "server": "5.6.7.8",, "port": 24007,,

"transport": "tcp" } ] },, "driver": "qcow2" } ,if=none,id=drive-virtio-disk0,cache=none if the number of hosts supplied is only one, then we use existing URI format for backward compatibility and if number of hosts is greater than one we switch to the new JSON format this patch requires qemu latest patch https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg07062.html

Credits: Sincere thanks to Kevin Wolf <kwolf(a)redhat.com&gt; and "Deepak C Shetty" <deepakcs(a)redhat.com&gt; for their support Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever(a)redhat.com&gt; --- src/qemu/qemu_command.c | 192 ++++++++++++++++++++++++++++++++++++------------ 1 file changed, 145 insertions(+), 47 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bb1835c..8c1bf1a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3256,16 +3256,106 @@ qemuNetworkDriveGetPort(int protocol, return -1; } +#define QEMU_DEFAULT_GLUSTER_PORT 24007

+ +static virJSONValuePtr +qemuBuildGlusterDriveJSON(virStorageSourcePtr src) +{ + virJSONValuePtr parent = NULL; + virJSONValuePtr object = NULL; + virJSONValuePtr dict_array = NULL; + int port; + int i;

+ + if (!(parent = virJSONValueNewObject())) + return NULL; + /* 1. prepare { driver:"gluster", volname:"testvol", image-path:"/a.img"} */ + if (virJSONValueObjectAdd(parent, + "s:driver", virStorageNetProtocolTypeToString(src->protocol), + "s:volname", src->volume, + "s:image-path", src->path, + NULL) < 0) + goto cleanup; + + if (!(dict_array = virJSONValueNewArray())) + goto cleanup; + /* 2. prepare array [ {server:"1.2.3.4", port:24007, transport:"tcp"} , + * {server:"5.6.7.8", port:24008, transport:"rdma"}, + * {}, ... ] */ + for (i = 0; i < src->nhosts; i++) { + if (!(object = virJSONValueNewObject())) + goto cleanup; + port = qemuNetworkDriveGetPort(src->protocol, src->hosts[i].port); + if (virJSONValueObjectAdd(object, "s:server", src->hosts[i].name, + "i:port", port ? port : QEMU_DEFAULT_GLUSTER_PORT ,

+ "s:transport", + virStorageNetHostTransportTypeToString(src->hosts[i].transport), + NULL) < 0) + goto cleanup; + if (virJSONValueArrayAppend(dict_array, object) < 0) { + virJSONValueFree(object);

+ goto cleanup; + } + } + /* 3. append 1 and 2 + * { driver:"gluster", volname:"testvol", image-path:"/a.img", + * volfile-servers :[ {server:"1.2.3.4", port:24007, transport:"tcp"} , + * {server:"5.6.7.8", port:24008, transport:"rdma"}, + * {}, ... ] } + */ + if (virJSONValueObjectAppend(parent, "volfile-servers", dict_array) < 0) { + virJSONValueFree(dict_array);

+ goto cleanup; + } + /* 4. assign key to 3 + * { file: { driver:"gluster", volname:"testvol", image-path:"/a.img", + * volfile-servers :[ {server:"1.2.3.4", port:24007, transport:"tcp"} , + * {server:"5.6.7.8", port:24008, transport:"rdma"}, + * {}, ... ] } } + */ + if (!(object = virJSONValueNewObject())) + goto cleanup; + if (virJSONValueObjectAppend(object, "file", parent) < 0) { + virJSONValueFree(parent);

+ goto cleanup; + } + /* 5. append format to 4 + * { file: { driver:"gluster", volname:"testvol", image-path:"/a.img", + * volfile-servers :[ {server:"1.2.3.4", port:24007, transport:"tcp"} , + * {server:"5.6.7.8", port:24008, transport:"rdma"}, + * {}, ... ] }, driver:"qcow2" } + */ + if (virJSONValueObjectAdd(object, + "s:driver", virStorageFileFormatTypeToString(src->format), + NULL) < 0) + goto cleanup; + else + /* since we have already captured the format type, let's make it '0' to + * avoid further checks for format information

+ */ + src->format = 0; + + return object; + +cleanup: + virJSONValueFree(dict_array); + virJSONValueFree(parent); + virJSONValueFree(object); + return NULL; +} + #define QEMU_DEFAULT_NBD_PORT "10809" static char * -qemuBuildNetworkDriveURI(virStorageSourcePtr src, +qemuBuildNetworkDriveStr(virStorageSourcePtr src, const char *username, const char *secret) { char *ret = NULL; + char *str = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; virURIPtr uri = NULL; + virJSONValuePtr json = NULL; size_t i; switch ((virStorageNetProtocol) src->protocol) { @@ -3331,61 +3421,67 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, case VIR_STORAGE_NET_PROTOCOL_TFTP: case VIR_STORAGE_NET_PROTOCOL_ISCSI: case VIR_STORAGE_NET_PROTOCOL_GLUSTER:

- if (src->nhosts != 1) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("protocol '%s' accepts only one host"), - virStorageNetProtocolTypeToString(src->protocol)); - goto cleanup; - } - - if (VIR_ALLOC(uri) < 0) - goto cleanup; - - if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) { - if (VIR_STRDUP(uri->scheme, - virStorageNetProtocolTypeToString(src->protocol)) < 0) + if (src->nhosts == 1) { + /* URI syntax generation */ + if (VIR_ALLOC(uri) < 0) goto cleanup; - } else { - if (virAsprintf(&uri->scheme, "%s+%s", - virStorageNetProtocolTypeToString(src->protocol), - virStorageNetHostTransportTypeToString(src->hosts->transport)) < 0) - goto cleanup; - } - - if ((uri->port = qemuNetworkDriveGetPort(src->protocol, src->hosts->port)) < 0) - goto cleanup; - if (src->path) { - if (src->volume) { - if (virAsprintf(&uri->path, "/%s%s", - src->volume, src->path) < 0) + if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) { + if (VIR_STRDUP(uri->scheme, + virStorageNetProtocolTypeToString(src->protocol)) < 0) goto cleanup; } else { - if (virAsprintf(&uri->path, "%s%s", - src->path[0] == '/' ? "" : "/", - src->path) < 0) + if (virAsprintf(&uri->scheme, "%s+%s", + virStorageNetProtocolTypeToString(src->protocol), + virStorageNetHostTransportTypeToString(src->hosts->transport)) < 0) goto cleanup; } - } - if (src->hosts->socket && - virAsprintf(&uri->query, "socket=%s", src->hosts->socket) < 0) - goto cleanup; + if ((uri->port = qemuNetworkDriveGetPort(src->protocol, src->hosts->port)) < 0) + goto cleanup; - if (username) { - if (secret) { - if (virAsprintf(&uri->user, "%s:%s", username, secret) < 0) - goto cleanup; - } else { - if (VIR_STRDUP(uri->user, username) < 0) - goto cleanup; + if (src->path) { + if (src->volume) { + if (virAsprintf(&uri->path, "/%s%s", + src->volume, src->path) < 0) + goto cleanup; + } else { + if (virAsprintf(&uri->path, "%s%s", + src->path[0] == '/' ? "" : "/", + src->path) < 0) + goto cleanup; + } } - } - if (VIR_STRDUP(uri->server, src->hosts->name) < 0) - goto cleanup; + if (src->hosts->socket && + virAsprintf(&uri->query, "socket=%s", src->hosts->socket) < 0) + goto cleanup; - ret = virURIFormat(uri); + if (username) { + if (secret) { + if (virAsprintf(&uri->user, "%s:%s", username, secret) < 0) + goto cleanup; + } else { + if (VIR_STRDUP(uri->user, username) < 0) + goto cleanup; + } + } + + if (VIR_STRDUP(uri->server, src->hosts->name) < 0) + goto cleanup; + + ret = virURIFormat(uri); + } else { + /* switch to new json formated syntax */

+ if(!(json = qemuBuildGlusterDriveJSON(src))) + goto cleanup; + + if (!(str = virJSONValueToString(json, false))) + goto cleanup; + + if (virAsprintf (&ret, "json:%s", str) < 0) + goto cleanup; + } break;

On Mon, Oct 05, 2015 at 15:30:42 +0530, Prasanna Kumar Kalever wrote: > currently libvirt has the capability to parse only one host and convert that > into URI formatted string, with the help of this patch libvirt will be able > to parse multiple hosts from the domain xml and can convert that into JSON > formatted string > > before: > ------ > example.xml: > ... > <disk type='network' device='disk'> > <driver name='qemu' type='qcow2' cache='none'/> > <source protocol='gluster' name='testvol/a.qcow2'> > <host name='1.2.3.4' port='24007' transport="tcp"/> > </source> > <target dev='vda' bus='virtio'/> > <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> > </disk> > ... > > resultant string: > file=gluster://1.2.3.4:24007/testvol/a.qcow2, \ > if=none,id=drive-virtio-disk0,format=qcow2,cache=none > > after: > ----- > example.xml: > ... > <disk type='network' device='disk'> > <driver name='qemu' type='qcow2' cache='none'/> > <source protocol='gluster' name='testvol/a.qcow2'> > <host name='1.2.3.4' port='24009' transport="tcp"/> > <host name='3.4.5.6' port="24008"/> > <host name='5.6.7.8' /> > </source> > <target dev='vda' bus='virtio'/> > <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> > </disk> > ... > > resultant string: > -drive file=json:{ > "file": { > "driver": "gluster",, > "volname": "testvol",, > "image-path": "/a.qcow2",, > "volfile-servers": [ > { > "server": "1.2.3.4",, > "port": 24009,, > "transport": "tcp" > },, > { > "server": "3.4.5.6",, > "port": 24008,, > "transport": "tcp" > },, > { > "server": "5.6.7.8",, > "port": 24007,, The double commas look like a result of our command line escaping function. Are they actually required with 'json:' sources? If no, we will need probably a way to avoid them. > "transport": "tcp" > } > ] > },, > "driver": "qcow2" > } > ,if=none,id=drive-virtio-disk0,cache=none > > if the number of hosts supplied is only one, then we use existing URI format > for backward compatibility and if number of hosts is greater than one we switch > to the new JSON format > > this patch requires qemu latest patch > https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg07062.html So this patch, if it will be acked needs to wait until qemu accepts the patch as final. > Credits: Sincere thanks to Kevin Wolf <kwolf(a)redhat.com&gt; and > "Deepak C Shetty" <deepakcs(a)redhat.com&gt; for their support > > Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever(a)redhat.com&gt; > --- > src/qemu/qemu_command.c | 192 ++++++++++++++++++++++++++++++++++++------------ > 1 file changed, 145 insertions(+), 47 deletions(-) > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index bb1835c..8c1bf1a 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -3256,16 +3256,106 @@ qemuNetworkDriveGetPort(int protocol, > return -1; > } > > +#define QEMU_DEFAULT_GLUSTER_PORT 24007 qemuNetworkDriveGetPort can be possibly improved to include this too, instead of the define ... [1] > + > +static virJSONValuePtr > +qemuBuildGlusterDriveJSON(virStorageSourcePtr src) > +{ > + virJSONValuePtr parent = NULL; > + virJSONValuePtr object = NULL; > + virJSONValuePtr dict_array = NULL; > + int port; > + int i; This won't pass syntax-check. Please make sure that you'll run it before posting patches. > + > + if (!(parent = virJSONValueNewObject())) > + return NULL; > + /* 1. prepare { driver:"gluster", volname:"testvol", image-path:"/a.img"} */ > + if (virJSONValueObjectAdd(parent, > + "s:driver", virStorageNetProtocolTypeToString(src->protocol), > + "s:volname", src->volume, > + "s:image-path", src->path, > + NULL) < 0) > + goto cleanup; > + > + if (!(dict_array = virJSONValueNewArray())) > + goto cleanup; > + /* 2. prepare array [ {server:"1.2.3.4", port:24007, transport:"tcp"} , > + * {server:"5.6.7.8", port:24008, transport:"rdma"}, > + * {}, ... ] */ > + for (i = 0; i < src->nhosts; i++) { > + if (!(object = virJSONValueNewObject())) > + goto cleanup; > + port = qemuNetworkDriveGetPort(src->protocol, src->hosts[i].port); > + if (virJSONValueObjectAdd(object, "s:server", src->hosts[i].name, > + "i:port", port ? port : QEMU_DEFAULT_GLUSTER_PORT , [1] and a ugly ternary operator. > + "s:transport", > + virStorageNetHostTransportTypeToString(src->hosts[i].transport), > + NULL) < 0) > + goto cleanup; > + if (virJSONValueArrayAppend(dict_array, object) < 0) { > + virJSONValueFree(object); The above statement is already in the cleanup section. > + goto cleanup; > + } > + } > + /* 3. append 1 and 2 > + * { driver:"gluster", volname:"testvol", image-path:"/a.img", > + * volfile-servers :[ {server:"1.2.3.4", port:24007, transport:"tcp"} , > + * {server:"5.6.7.8", port:24008, transport:"rdma"}, > + * {}, ... ] } > + */ > + if (virJSONValueObjectAppend(parent, "volfile-servers", dict_array) < 0) { > + virJSONValueFree(dict_array); The above statement is already in the cleanup section. > + goto cleanup; > + } > + /* 4. assign key to 3 > + * { file: { driver:"gluster", volname:"testvol", image-path:"/a.img", > + * volfile-servers :[ {server:"1.2.3.4", port:24007, transport:"tcp"} , > + * {server:"5.6.7.8", port:24008, transport:"rdma"}, > + * {}, ... ] } } > + */ > + if (!(object = virJSONValueNewObject())) > + goto cleanup; > + if (virJSONValueObjectAppend(object, "file", parent) < 0) { > + virJSONValueFree(parent); Same thing. > + goto cleanup; > + } > + /* 5. append format to 4 > + * { file: { driver:"gluster", volname:"testvol", image-path:"/a.img", > + * volfile-servers :[ {server:"1.2.3.4", port:24007, transport:"tcp"} , > + * {server:"5.6.7.8", port:24008, transport:"rdma"}, > + * {}, ... ] }, driver:"qcow2" } > + */ > + if (virJSONValueObjectAdd(object, > + "s:driver", virStorageFileFormatTypeToString(src->format), > + NULL) < 0) > + goto cleanup; > + else > + /* since we have already captured the format type, let's make it '0' to > + * avoid further checks for format information NACK to this, this would remove it from the live definition and it would disappear from the live XML. That can't happen. Various block job operations and a lot of other code depend on knowing the format. Also this violates the code style since the "body" of the else statement is multi-line due to the comment so both paths need a block. Also, since the true part jumps to 'cleanup' you can remove else completely to make the code more unambiguous. > + */ > + src->format = 0; > + > + return object; > + > +cleanup: > + virJSONValueFree(dict_array); > + virJSONValueFree(parent); > + virJSONValueFree(object); > + return NULL; > +} > + > #define QEMU_DEFAULT_NBD_PORT "10809" > > static char * > -qemuBuildNetworkDriveURI(virStorageSourcePtr src, > +qemuBuildNetworkDriveStr(virStorageSourcePtr src, > const char *username, > const char *secret) > { > char *ret = NULL; > + char *str = NULL; > virBuffer buf = VIR_BUFFER_INITIALIZER; > virURIPtr uri = NULL; > + virJSONValuePtr json = NULL; > size_t i; > > switch ((virStorageNetProtocol) src->protocol) { > @@ -3331,61 +3421,67 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, > case VIR_STORAGE_NET_PROTOCOL_TFTP: > case VIR_STORAGE_NET_PROTOCOL_ISCSI: > case VIR_STORAGE_NET_PROTOCOL_GLUSTER: So this is done for all protocols that originally use an URI ... > - if (src->nhosts != 1) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("protocol '%s' accepts only one host"), > - virStorageNetProtocolTypeToString(src->protocol)); > - goto cleanup; > - } > - > - if (VIR_ALLOC(uri) < 0) > - goto cleanup; > - > - if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) { > - if (VIR_STRDUP(uri->scheme, > - virStorageNetProtocolTypeToString(src->protocol)) < 0) > + if (src->nhosts == 1) { > + /* URI syntax generation */ > + if (VIR_ALLOC(uri) < 0) > goto cleanup; > - } else { > - if (virAsprintf(&uri->scheme, "%s+%s", > - virStorageNetProtocolTypeToString(src->protocol), > - virStorageNetHostTransportTypeToString(src->hosts->transport)) < 0) > - goto cleanup; > - } > - > - if ((uri->port = qemuNetworkDriveGetPort(src->protocol, src->hosts->port)) < 0) > - goto cleanup; > > - if (src->path) { > - if (src->volume) { > - if (virAsprintf(&uri->path, "/%s%s", > - src->volume, src->path) < 0) > + if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) { > + if (VIR_STRDUP(uri->scheme, > + virStorageNetProtocolTypeToString(src->protocol)) < 0) > goto cleanup; > } else { > - if (virAsprintf(&uri->path, "%s%s", > - src->path[0] == '/' ? "" : "/", > - src->path) < 0) > + if (virAsprintf(&uri->scheme, "%s+%s", > + virStorageNetProtocolTypeToString(src->protocol), > + virStorageNetHostTransportTypeToString(src->hosts->transport)) < 0) > goto cleanup; > } > - } > > - if (src->hosts->socket && > - virAsprintf(&uri->query, "socket=%s", src->hosts->socket) < 0) > - goto cleanup; > + if ((uri->port = qemuNetworkDriveGetPort(src->protocol, src->hosts->port)) < 0) > + goto cleanup; > > - if (username) { > - if (secret) { > - if (virAsprintf(&uri->user, "%s:%s", username, secret) < 0) > - goto cleanup; > - } else { > - if (VIR_STRDUP(uri->user, username) < 0) > - goto cleanup; > + if (src->path) { > + if (src->volume) { > + if (virAsprintf(&uri->path, "/%s%s", > + src->volume, src->path) < 0) > + goto cleanup; > + } else { > + if (virAsprintf(&uri->path, "%s%s", > + src->path[0] == '/' ? "" : "/", > + src->path) < 0) > + goto cleanup; > + } > } > - } > > - if (VIR_STRDUP(uri->server, src->hosts->name) < 0) > - goto cleanup; > + if (src->hosts->socket && > + virAsprintf(&uri->query, "socket=%s", src->hosts->socket) < 0) > + goto cleanup; > > - ret = virURIFormat(uri); > + if (username) { > + if (secret) { > + if (virAsprintf(&uri->user, "%s:%s", username, secret) < 0) > + goto cleanup; > + } else { > + if (VIR_STRDUP(uri->user, username) < 0) > + goto cleanup; > + } > + } > + > + if (VIR_STRDUP(uri->server, src->hosts->name) < 0) > + goto cleanup; > + > + ret = virURIFormat(uri); > + } else { > + /* switch to new json formated syntax */ ... but you do Gluster here unconditionally. This would actually turn a HTTP (or other) disk definition into a gluster source. That can't happen, this either needs to go into a separate section under the VIR_STORAGE_NET_PROTOCOL_GLUSTER or the JSON generator needs to be able to generate json for all the protocols. > + if(!(json = qemuBuildGlusterDriveJSON(src))) > + goto cleanup; > + > + if (!(str = virJSONValueToString(json, false))) > + goto cleanup; > + > + if (virAsprintf (&ret, "json:%s", str) < 0) > + goto cleanup; > + } > > break; > So while all the above looks pretty okay at this point (except for clearing the image type) this patch is incomplete. Since libvirt is loading the backing chain to be able to traverse it and set correct permissions on individual images. This equals to two additional parts of the code that need modification: 1) virStorageFileBackendGlusterInit has the same "src->nhosts != 1" condition and is not prepared to work with multiple protocols

2) I presume (I didn't test it yet) that the qemu patch that adds this stuff will result into using the "json:{" protocol string in the 'backing_store' field once you do a snapshot of a gluster disk that uses this new syntax. If that happens the VM will not be able to start in libvirt any more, as libvirt does not have a 'json:{' protocol parser to parse the backing store.

So in order to accept this patch, the helpers in src/util/virstoragefile.c (virStorageSourceParseBackingURI) will need to be made modular so that they can accept a driver specific callback, that will then parse the 'json:{' source definitions. (We can't just hardcode it there, since src/util "should" not contain any qemu specific code.)