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@xxxxxxxxxx> and
> "Deepak C Shetty" <deepakcs@xxxxxxxxxx> for their support
>
> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@xxxxxxxxxx>
> ---
> 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.)
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list