On Thu, Oct 08, 2015 at 17:25:52 +0530, Prasanna Kumar Kalever wrote:
> This patch adds support for gluster specific JSON formatter functionality
>
> 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
>
> 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
>
> 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
>
> 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@xxxxxxxxxx> and
> "Deepak C Shetty" <deepakcs@xxxxxxxxxx> for their support
>
> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@xxxxxxxxxx>
> ---
> v1: add support to gluster json formatter
>
> v2: addressing "Peter Krempa" <pkrempa@xxxxxxxxxx> comments
> ---
> src/qemu/qemu_command.c | 216 ++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 162 insertions(+), 54 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index bb1835c..63217cd 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3243,8 +3243,7 @@ qemuNetworkDriveGetPort(int protocol,
>
> case VIR_STORAGE_NET_PROTOCOL_ISCSI:
> case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
> - /* no default port specified */
> - return 0;
> + return 24007;
This would add the gluster port for ISCSI too ...
>
> case VIR_STORAGE_NET_PROTOCOL_RBD:
> case VIR_STORAGE_NET_PROTOCOL_LAST:
> @@ -3256,16 +3255,160 @@ qemuNetworkDriveGetPort(int protocol,
> return -1;
> }
>
> +static char*
> +qemuBuildNetworkDriveURI(virStorageSourcePtr src,
> + const char *username,
> + const char *secret)
As stated in previous patch, please adhere to the libvirt coding style.
> +{
> + virURIPtr uri = NULL;
> + char *str = NULL;
> +
> + 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)
> + 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)
> + goto cleanup;
> + } else {
> + if (virAsprintf(&uri->path, "%s%s",
> + src->path[0] == '/' ? "" : "/",
> + src->path) < 0)
> + goto cleanup;
> + }
> + }
> +
> + if (src->hosts->socket &&
> + virAsprintf(&uri->query, "socket=%s", src->hosts->socket) < 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 (VIR_STRDUP(uri->server, src->hosts->name) < 0)
> + goto cleanup;
> +
> + str = virURIFormat(uri);
> +
> + cleanup:
> + virURIFree(uri);
> +
> + return str;
> +}
Looks okay apart from totaly breaking coding style.
> +
> +static char *
> +qemuBuildGlusterDriveJSON(virStorageSourcePtr src)
> +{
> + virJSONValuePtr file = NULL;
> + virJSONValuePtr object = NULL;
> + virJSONValuePtr volfile_servers = NULL;
> + char *ret = NULL;
> + size_t i;
> +
> + if (!(file = virJSONValueNewObject()))
> + return NULL;
> +
> + /* 1. prepare { driver:"gluster", volname:"testvol", image-path:"/a.img"} */
> + if (virJSONValueObjectAdd(file,
> + "s:driver", virStorageNetProtocolTypeToString(src->protocol),
> + "s:volname", src->volume,
> + "s:image-path", src->path,
> + NULL) < 0)
> + goto cleanup;
> +
> + if (!(volfile_servers = 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;
> + if (virJSONValueObjectAdd(object, "s:server", src->hosts[i].name,
> + "i:port",
> + qemuNetworkDriveGetPort(src->protocol, src->hosts[i].port),
> + "s:transport",
> + virStorageNetHostTransportTypeToString(src->hosts[i].transport),
> + NULL) < 0)
> + goto cleanup;
> + if (virJSONValueArrayAppend(volfile_servers, object) < 0)
> + 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(file, "volfile-servers", volfile_servers) < 0)
> + 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", file) < 0)
> + 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;
> +
> + if (virAsprintf(&ret, "json:%s", virJSONValueToString(object, false)) < 0)
> + ret = NULL;
> +
> + cleanup:
> + virJSONValueFree(volfile_servers);
> + virJSONValueFree(file);
> + virJSONValueFree(object);
> +
> + return ret;
Wrong coding style and wrong parameter names.
> +}
> +
> #define QEMU_DEFAULT_NBD_PORT "10809"
>
> static char *
> -qemuBuildNetworkDriveURI(virStorageSourcePtr src,
> +qemuBuildNetworkDriveStr(virStorageSourcePtr src,
> const char *username,
> const char *secret)
> {
> char *ret = NULL;
> virBuffer buf = VIR_BUFFER_INITIALIZER;
> - virURIPtr uri = NULL;
> size_t i;
>
> switch ((virStorageNetProtocol) src->protocol) {
> @@ -3330,63 +3473,29 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src,
> case VIR_STORAGE_NET_PROTOCOL_FTPS:
> case VIR_STORAGE_NET_PROTOCOL_TFTP:
> case VIR_STORAGE_NET_PROTOCOL_ISCSI:
> - case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
> - if (src->nhosts != 1) {
> + if (src->nhosts == 1) {
> + if (!(ret = qemuBuildNetworkDriveURI(src, username, secret)))
> + goto cleanup;
> + } else {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("protocol '%s' accepts only one host"),
> - virStorageNetProtocolTypeToString(src->protocol));
> + _("protocol '%s' accepts only one host"),
> + virStorageNetProtocolTypeToString(src->protocol));
> goto cleanup;
> }
>
> - if (VIR_ALLOC(uri) < 0)
> - goto cleanup;
> + break;
>
> - if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) {
> - if (VIR_STRDUP(uri->scheme,
> - virStorageNetProtocolTypeToString(src->protocol)) < 0)
> + case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
> + if (src->nhosts == 1) {
> + /* URI formatted syntax */
> + if (!(ret = qemuBuildNetworkDriveURI(src, username, secret)))
> goto cleanup;
> } else {
> - if (virAsprintf(&uri->scheme, "%s+%s",
> - virStorageNetProtocolTypeToString(src->protocol),
> - virStorageNetHostTransportTypeToString(src->hosts->transport)) < 0)
> + /* switch to new json formatted syntax */
> + if (!(ret = qemuBuildGlusterDriveJSON(src)))
> 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)
> - goto cleanup;
> - } else {
> - if (virAsprintf(&uri->path, "%s%s",
> - src->path[0] == '/' ? "" : "/",
> - src->path) < 0)
> - goto cleanup;
> - }
> - }
> -
> - if (src->hosts->socket &&
> - virAsprintf(&uri->query, "socket=%s", src->hosts->socket) < 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 (VIR_STRDUP(uri->server, src->hosts->name) < 0)
> - goto cleanup;
> -
> - ret = virURIFormat(uri);
> -
> break;
>
> case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
Few more coding style issues.
Also as Dan pointed out, you'll need some tests (qemuxml2argv,
qemuxml2xml) for the new stuff. I'd suggest that you wait until the qemu
design settles at this point since you'd need to change them if it
changes.
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list