[libvirt] [PATCH 1/3] storage: add gluster backend initialization support for multiple servers
This patch adds support for initialization of gluster backend with multiple servers which acts as gluster volfile servers for the gluster storage backend. This will help in achieving high availability of gluster backend connectivity via libgfapi i.e. when the first volfile server fails, then other servers specified are used as volfile server to re-establish the backend gluster connectivity. Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- src/storage/storage_backend_gluster.c | 77 +++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 36 deletions(-) diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index d2e79bc..53474c1 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -32,6 +32,9 @@ #include "virstring.h" #include "viruri.h" +#define QEMU_DEFAULT_GLUSTER_PORT 24007 +#define QEMU_DEFAULT_GLUSTER_TRANSPORT "tcp" + #define VIR_FROM_THIS VIR_FROM_STORAGE VIR_LOG_INIT("storage.storage_backend_gluster"); @@ -570,62 +573,64 @@ static int virStorageFileBackendGlusterInit(virStorageSourcePtr src) { virStorageFileBackendGlusterPrivPtr priv = NULL; - virStorageNetHostDefPtr host = &(src->hosts[0]); - const char *hostname; + const char *transport = NULL; int port = 0; + size_t i = 0; - if (src->nhosts != 1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Expected exactly 1 host for the gluster volume")); - return -1; - } - - hostname = host->name; - - VIR_DEBUG("initializing gluster storage file %p (gluster://%s:%s/%s%s)[%u:%u]", - src, hostname, host->port ? host->port : "0", - NULLSTR(src->volume), src->path, - (unsigned int)src->drv->uid, (unsigned int)src->drv->gid); + VIR_DEBUG("Initializing gluster volume=%s image-path=%s with uid=%u gid=%u", + NULLSTR(src->volume), src->path, + (unsigned int)src->drv->uid, (unsigned int)src->drv->gid); if (!src->volume) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("missing gluster volume name for path '%s'"), - src->path); + _("missing gluster volume name for path '%s'"), + src->path); return -1; } if (VIR_ALLOC(priv) < 0) return -1; - if (host->port && - virStrToLong_i(host->port, NULL, 10, &port) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to parse port number '%s'"), - host->port); - goto error; - } - - if (host->transport == VIR_STORAGE_NET_HOST_TRANS_UNIX) - hostname = host->socket; - if (!(priv->vol = glfs_new(src->volume))) { virReportOOMError(); goto error; } - if (glfs_set_volfile_server(priv->vol, - virStorageNetHostTransportTypeToString(host->transport), - hostname, port) < 0) { - virReportSystemError(errno, - _("failed to set gluster volfile server '%s'"), - hostname); - goto error; + for (i = 0; i < src->nhosts; i++) { + if (!src->hosts[i].port) { + port = QEMU_DEFAULT_GLUSTER_PORT; + } else if (virStrToLong_i(src->hosts[i].port, NULL, 10, &port) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to parse port number '%s'"), src->hosts[i].port); + goto error; + } + + if (src->hosts[i].transport == VIR_STORAGE_NET_HOST_TRANS_UNIX) { + transport = src->hosts[i].socket; + } else if (!src->hosts[i].transport) { + transport = QEMU_DEFAULT_GLUSTER_TRANSPORT; + } else { + transport = virStorageNetHostTransportTypeToString(src->hosts[i].transport); + } + + if (glfs_set_volfile_server(priv->vol, transport, src->hosts[i].name, + port) < 0) { + virReportSystemError(errno, + _("failed to set gluster volfile server '%s'"), + src->hosts[i].name); + goto error; + } else { + VIR_DEBUG("Successfully set volfile server with server:'%s' " + "port:'%d' transport:'%s'", src->hosts[i].name, + port, transport); + } } if (glfs_init(priv->vol) < 0) { virReportSystemError(errno, - _("failed to initialize gluster connection to " - "server: '%s'"), hostname); + _("failed to initialize gluster connection with given hosts " + "volume : '%s' image-path : '%s'"), + NULLSTR(src->volume), src->path); goto error; } -- 2.1.0
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@redhat.com> and "Deepak C Shetty" <deepakcs@redhat.com> for their support Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- v1: add support to gluster json formatter v2: addressing "Peter Krempa" <pkrempa@redhat.com> 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; 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) +{ + 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; +} + +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; +} + #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: @@ -3473,7 +3582,6 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, cleanup: virBufferFreeAndReset(&buf); - virURIFree(uri); return ret; } @@ -3530,7 +3638,7 @@ qemuGetDriveSourceString(virStorageSourcePtr src, break; case VIR_STORAGE_TYPE_NETWORK: - if (!(*source = qemuBuildNetworkDriveURI(src, username, secret))) + if (!(*source = qemuBuildNetworkDriveStr(src, username, secret))) goto cleanup; break; @@ -6391,7 +6499,7 @@ qemuBuildSCSIiSCSIHostdevDrvStr(virConnectPtr conn, src.nhosts = iscsisrc->nhosts; /* Rather than pull what we think we want - use the network disk code */ - source = qemuBuildNetworkDriveURI(&src, username, secret); + source = qemuBuildNetworkDriveStr(&src, username, secret); cleanup: VIR_FREE(secret); -- 2.1.0
On Thu, Oct 08, 2015 at 05:25:52PM +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@redhat.com> and "Deepak C Shetty" <deepakcs@redhat.com> for their support
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- v1: add support to gluster json formatter
v2: addressing "Peter Krempa" <pkrempa@redhat.com> comments --- src/qemu/qemu_command.c | 216 ++++++++++++++++++++++++++++++++++++------------
Any time qemu_command.c is extended, you need to add to the test suite qemuxml2argvtest.c at the very minimum, ideally qemuxml2xmltest and qemuargv2xmltest too if poissible Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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@redhat.com> and "Deepak C Shetty" <deepakcs@redhat.com> for their support
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- v1: add support to gluster json formatter
v2: addressing "Peter Krempa" <pkrempa@redhat.com> 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
This patch adds support for gluster specific JSON parser functionality This will help in parsing the backing store which uses JSON syntax and update the meta-data in the domain specific objects while taking snapshots which inturn helps in successful creation/updation of backing store information in domain xml Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- src/util/virstoragefile.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2aa1d90..c122d3a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -41,6 +41,7 @@ #include "virstring.h" #include "virutil.h" #include "viruri.h" +#include "virjson.h" #include "dirname.h" #include "virbuffer.h" @@ -2124,6 +2125,132 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, goto cleanup; } +static int +virStorageSourceParseBackingJSON(virStorageSourcePtr src, + const char *path) +{ +#if WITH_STORAGE_GLUSTER + + virJSONValuePtr json = NULL; + virJSONValuePtr file = NULL; + virJSONValuePtr volfile_server = NULL; + char *transport = NULL; + int port = 0; + size_t i = 0; + + /* The string may look like: + * json:{ 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" } + */ + + json = virJSONValueFromString(path+strlen("json:")); + + if (virJSONValueObjectHasKey(json, "driver")) { + if (!(src->format = virStorageFileFormatTypeFromString( + virJSONValueObjectGetString(json, "driver")))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get Format type of key 'driver' from JSON")); + goto error; + } + } + + if (virJSONValueObjectHasKey(json, "file")) { + if (!(file = virJSONValueObjectGet(json, "file"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unbale to get Object from key 'file'")); + goto error; + } + } + + if (virJSONValueObjectHasKey(file, "driver")) { + if (!(src->protocol = virStorageNetProtocolTypeFromString( + virJSONValueObjectGetString(file, "driver")))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get Protocol type of key 'driver' from JSON")); + goto error; + } + } + + if (virJSONValueObjectHasKey(file, "volname")) { + if (!(src->volume = (char *) virJSONValueObjectGetString(file, + "volname"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unbale to get String from key 'volname'")); + goto error; + } + } + + if (virJSONValueObjectHasKey(file, "image-path")) { + if (!(src->path = (char *) virJSONValueObjectGetString(file, + "image-path"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get String from key 'image-path'")); + goto error; + } + } + + if (virJSONValueObjectHasKey(file, "volfile-servers")) { + if (!(src->nhosts = virJSONValueArraySize(virJSONValueObjectGet(file, + "volfile-servers")))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get Size of Array of key 'volfile-servers'")); + goto error; + } + } + + /* null-terminated list */ + if (VIR_ALLOC_N(src->hosts, src->nhosts + 1) < 0) + goto error; + + for (i = 0; i < src->nhosts; i++) { + volfile_server = virJSONValueArrayGet(virJSONValueObjectGetArray(file, + "volfile-servers"), i); + + if (virJSONValueObjectHasKey(volfile_server, "server")) { + if (VIR_STRDUP(src->hosts[i].name, + virJSONValueObjectGetString(volfile_server, "server")) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get String from key 'server'")); + goto error; + } + } + + if (virJSONValueObjectHasKey(volfile_server, "port")) { + if (virJSONValueObjectGetNumberInt(volfile_server, "port", &port) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get NumberInt from key 'port'")); + goto error; + } + if (virAsprintf(&src->hosts[i].port, "%d", port) < 0) + goto error; + } + + if (virJSONValueObjectHasKey(volfile_server, "transport")) { + if (VIR_STRDUP(transport, virJSONValueObjectGetString(volfile_server, + "transport")) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get String from key 'transport'")); + goto error; + } + src->hosts[i].transport = virStorageNetHostTransportTypeFromString(transport); + VIR_FREE(transport); + } + } + + return 0; + + error: + VIR_FREE(transport); + virJSONValueFree(volfile_server); + virJSONValueFree(file); + virJSONValueFree(json); + +#endif /* WITH_STORAGE_GLUSTER */ + + return -1; +} static int virStorageSourceParseBackingURI(virStorageSourcePtr src, @@ -2535,6 +2662,9 @@ virStorageSourceNewFromBackingAbsolute(const char *path) if (strstr(path, "://")) { if (virStorageSourceParseBackingURI(ret, path) < 0) goto error; + } else if (strstr(path, "json:")) { + if (virStorageSourceParseBackingJSON(ret, path) < 0) + goto error; } else { if (virStorageSourceParseBackingColon(ret, path) < 0) goto error; -- 2.1.0
On Thu, Oct 08, 2015 at 17:25:53 +0530, Prasanna Kumar Kalever wrote:
This patch adds support for gluster specific JSON parser functionality
This patch needs to go in before we actually add the support, so as 1/1 of this series.
This will help in parsing the backing store which uses JSON syntax and update the meta-data in the domain specific objects while taking snapshots which inturn helps in successful creation/updation of backing store information in domain xml
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- src/util/virstoragefile.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2aa1d90..c122d3a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -41,6 +41,7 @@ #include "virstring.h" #include "virutil.h" #include "viruri.h" +#include "virjson.h" #include "dirname.h" #include "virbuffer.h"
@@ -2124,6 +2125,132 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, goto cleanup; }
+static int +virStorageSourceParseBackingJSON(virStorageSourcePtr src, + const char *path)
Weird alignment. Also you probably missed a part in my previous review where I'd requested that this is put into the qemu specific code rather than the generic util code since this is too qemu specific. On the other hand, it might be worth checking where the backing chain functions are used and just move all the backing store parsers into qemu code as probably all of it is very qemu specific too.
+{ +#if WITH_STORAGE_GLUSTER
Either the whole function is in the #if block and you have an else block with a stub function that reports an error, or preferred is to have this even if we don't compile with gluster since this doesn't use any gluster api so it can be compiled always
+ + virJSONValuePtr json = NULL; + virJSONValuePtr file = NULL; + virJSONValuePtr volfile_server = NULL; + char *transport = NULL; + int port = 0; + size_t i = 0; + + /* The string may look like: + * json:{ 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" } + */ + + json = virJSONValueFromString(path+strlen("json:")); + + if (virJSONValueObjectHasKey(json, "driver")) { + if (!(src->format = virStorageFileFormatTypeFromString( + virJSONValueObjectGetString(json, "driver")))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get Format type of key 'driver' from JSON"));
virStorageFileFormatTypeFromString returns -1 if it fails to parse the format type. Format '0' might be valid.
+ goto error; + } + } + + if (virJSONValueObjectHasKey(json, "file")) { + if (!(file = virJSONValueObjectGet(json, "file"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unbale to get Object from key 'file'")); + goto error; + } + } + + if (virJSONValueObjectHasKey(file, "driver")) { + if (!(src->protocol = virStorageNetProtocolTypeFromString( + virJSONValueObjectGetString(file, "driver")))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get Protocol type of key 'driver' from JSON"));
Same comment as above for usage of the enum->string helpers. Additionally the assumption that 'drive' will contain something that can be used successfully with virStorageNetProtocolTypeFromString is wrong. The json syntax can be used with all the remote or local files and the virStorageNetProtocolTypeFromString certainly won't recognize local files. That would result in a rather unhelpful error.
+ goto error; + } + } + + if (virJSONValueObjectHasKey(file, "volname")) {
This uses the old naming in qemu, that I've pointed out is not what should be done in qemu. Additionally my comment about the naming was done prior to this submission. I'd suggest you tackle the qemu part first at this point. Additionally this is a gluster specific field. And the parser at least according to the function name sounds rather generic. I think this would badly fail on anything non'gluster.
+ if (!(src->volume = (char *) virJSONValueObjectGetString(file, + "volname"))) {
You are extracting a string but you are not copying it. virJSONValueObjectGetString returns just the pointer to the string in the json data structure. This either crashes or leaks the json array so that it accidentally remains valid. Did you actually test it?
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unbale to get String from key 'volname'")); + goto error; + } + } + + if (virJSONValueObjectHasKey(file, "image-path")) {
Again ... old field name.
+ if (!(src->path = (char *) virJSONValueObjectGetString(file, + "image-path"))) {
And wrong extraction of it.
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get String from key 'image-path'")); + goto error; + } + } + + if (virJSONValueObjectHasKey(file, "volfile-servers")) {
Wrong field name. This is also the reason why "volfile-servers" is a terrible name. Any other protocol that certainly won't call it's server field volfile-servers. We would need to add a different code path here. Gluster isn't the only storage technology on the planet, so this code needs to be extensible
+ if (!(src->nhosts = virJSONValueArraySize(virJSONValueObjectGet(file, + "volfile-servers")))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get Size of Array of key 'volfile-servers'")); + goto error; + } + } + + /* null-terminated list */
Why? We store the countof elements.
+ if (VIR_ALLOC_N(src->hosts, src->nhosts + 1) < 0) + goto error; + + for (i = 0; i < src->nhosts; i++) { + volfile_server = virJSONValueArrayGet(virJSONValueObjectGetArray(file, + "volfile-servers"), i);
Wrong name. Weird alignment.
+ + if (virJSONValueObjectHasKey(volfile_server, "server")) {
Wrong field name.
+ if (VIR_STRDUP(src->hosts[i].name, + virJSONValueObjectGetString(volfile_server, "server")) < 0) {
Wow, here you copy the string. Didn't that seem suspicious compared to the avoce usage?
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get String from key 'server'"));
You've validated that "server" key exists, so the only error here is if VIR_STRDUP fails. VIR_STRDUP also reports it's own errors.
+ goto error; + } + } + + if (virJSONValueObjectHasKey(volfile_server, "port")) { + if (virJSONValueObjectGetNumberInt(volfile_server, "port", &port) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get NumberInt from key 'port'")); + goto error; + } + if (virAsprintf(&src->hosts[i].port, "%d", port) < 0) + goto error; + } + + if (virJSONValueObjectHasKey(volfile_server, "transport")) { + if (VIR_STRDUP(transport, virJSONValueObjectGetString(volfile_server, + "transport")) < 0) {
Here you duplicate the string containing the transport ....
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get String from key 'transport'")); + goto error; + } + src->hosts[i].transport = virStorageNetHostTransportTypeFromString(transport);
Convert it without actually checking that the conversion was successful...
+ VIR_FREE(transport);
And free it. That's a waste since you didn't use the 'transport' string out of the context here. Seems like you should use approach like this above but certainly not here.
+ } + }
Oh right, this leaks all the stuff on success, so that's why it doesn't crash. That's obviously not right.
+ + return 0; + + error: + VIR_FREE(transport); + virJSONValueFree(volfile_server); + virJSONValueFree(file); + virJSONValueFree(json); + +#endif /* WITH_STORAGE_GLUSTER */
You'd get compilation errors if WITH_STORAGE_GLUSTER is not enabled due to unused variables.
+ + return -1; +}
static int virStorageSourceParseBackingURI(virStorageSourcePtr src,
Peter
On Thu, Oct 08, 2015 at 17:25:53 +0530, Prasanna Kumar Kalever wrote:
This patch adds support for gluster specific JSON parser functionality
This patch needs to go in before we actually add the support, so as 1/1 of this series.
This will help in parsing the backing store which uses JSON syntax and update the meta-data in the domain specific objects while taking snapshots which inturn helps in successful creation/updation of backing store information in domain xml
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- src/util/virstoragefile.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2aa1d90..c122d3a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -41,6 +41,7 @@ #include "virstring.h" #include "virutil.h" #include "viruri.h" +#include "virjson.h" #include "dirname.h" #include "virbuffer.h"
@@ -2124,6 +2125,132 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, goto cleanup; }
+static int +virStorageSourceParseBackingJSON(virStorageSourcePtr src, + const char *path)
Weird alignment. Also you probably missed a part in my previous review where I'd requested that this is put into the qemu specific code rather than the generic util code since this is too qemu specific.
On the other hand, it might be worth checking where the backing chain functions are used and just move all the backing store parsers into qemu code as probably all of it is very qemu specific too.
+{ +#if WITH_STORAGE_GLUSTER
Either the whole function is in the #if block and you have an else block with a stub function that reports an error, or preferred is to have this even if we don't compile with gluster since this doesn't use any gluster api so it can be compiled always
+ + virJSONValuePtr json = NULL; + virJSONValuePtr file = NULL; + virJSONValuePtr volfile_server = NULL; + char *transport = NULL; + int port = 0; + size_t i = 0; + + /* The string may look like: + * json:{ 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" } + */ + + json = virJSONValueFromString(path+strlen("json:")); + + if (virJSONValueObjectHasKey(json, "driver")) { + if (!(src->format = virStorageFileFormatTypeFromString( + virJSONValueObjectGetString(json, "driver")))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get Format type of key 'driver' from JSON"));
virStorageFileFormatTypeFromString returns -1 if it fails to parse the format type. Format '0' might be valid.
+ goto error; + } + } + + if (virJSONValueObjectHasKey(json, "file")) { + if (!(file = virJSONValueObjectGet(json, "file"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unbale to get Object from key 'file'")); + goto error; + } + } + + if (virJSONValueObjectHasKey(file, "driver")) { + if (!(src->protocol = virStorageNetProtocolTypeFromString( + virJSONValueObjectGetString(file, "driver")))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get Protocol type of key 'driver' from JSON"));
Same comment as above for usage of the enum->string helpers.
Additionally the assumption that 'drive' will contain something that can be used successfully with virStorageNetProtocolTypeFromString is wrong.
The json syntax can be used with all the remote or local files and the virStorageNetProtocolTypeFromString certainly won't recognize local files. That would result in a rather unhelpful error.
+ goto error; + } + } + + if (virJSONValueObjectHasKey(file, "volname")) {
This uses the old naming in qemu, that I've pointed out is not what should be done in qemu. Additionally my comment about the naming was done prior to this submission.
I'd suggest you tackle the qemu part first at this point.
Additionally this is a gluster specific field. And the parser at least according to the function name sounds rather generic. I think this would badly fail on anything non'gluster.
+ if (!(src->volume = (char *) virJSONValueObjectGetString(file, + "volname"))) {
You are extracting a string but you are not copying it. virJSONValueObjectGetString returns just the pointer to the string in the json data structure. This either crashes or leaks the json array so that it accidentally remains valid. Did you actually test it?
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unbale to get String from key 'volname'")); + goto error; + } + } + + if (virJSONValueObjectHasKey(file, "image-path")) {
Again ... old field name.
+ if (!(src->path = (char *) virJSONValueObjectGetString(file, + "image-path"))) {
And wrong extraction of it.
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get String from key 'image-path'")); + goto error; + } + } + + if (virJSONValueObjectHasKey(file, "volfile-servers")) {
Wrong field name. This is also the reason why "volfile-servers" is a terrible name. Any other protocol that certainly won't call it's server field volfile-servers. We would need to add a different code path here.
Gluster isn't the only storage technology on the planet, so this code needs to be extensible
Peter, I have send the patch to qemu just before with the naming conventions yourself and Kevin suggested on v5 patch of qemu. But before sending this patch qemu v5 patch-set have the older naming conventions, so obviously if somebody want to test this patch which depends on qemu patch, the naming conventions must be as qemu v5 path-set, so knowingly I followed the older naming conventions. I hope you understand that part. Anyways I know this patch-set is not the final one that goes-in Coz of some of holes in my understanding of being very new user to libvirt, I shall update this patch with qemu v6 naming conventions very soon. I shall also consider the valuable points you mention in this mail. Thank you.
+ if (!(src->nhosts = virJSONValueArraySize(virJSONValueObjectGet(file, + "volfile-servers")))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get Size of Array of key 'volfile-servers'")); + goto error; + } + } + + /* null-terminated list */
Why? We store the countof elements.
+ if (VIR_ALLOC_N(src->hosts, src->nhosts + 1) < 0) + goto error; + + for (i = 0; i < src->nhosts; i++) { + volfile_server = virJSONValueArrayGet(virJSONValueObjectGetArray(file, + "volfile-servers"), i);
Wrong name. Weird alignment.
+ + if (virJSONValueObjectHasKey(volfile_server, "server")) {
Wrong field name.
+ if (VIR_STRDUP(src->hosts[i].name, + virJSONValueObjectGetString(volfile_server, "server")) < 0) {
Wow, here you copy the string. Didn't that seem suspicious compared to the avoce usage?
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get String from key 'server'"));
You've validated that "server" key exists, so the only error here is if VIR_STRDUP fails. VIR_STRDUP also reports it's own errors.
+ goto error; + } + } + + if (virJSONValueObjectHasKey(volfile_server, "port")) { + if (virJSONValueObjectGetNumberInt(volfile_server, "port", &port) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get NumberInt from key 'port'")); + goto error; + } + if (virAsprintf(&src->hosts[i].port, "%d", port) < 0) + goto error; + } + + if (virJSONValueObjectHasKey(volfile_server, "transport")) { + if (VIR_STRDUP(transport, virJSONValueObjectGetString(volfile_server, + "transport")) < 0) {
Here you duplicate the string containing the transport ....
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get String from key 'transport'")); + goto error; + } + src->hosts[i].transport = virStorageNetHostTransportTypeFromString(transport);
Convert it without actually checking that the conversion was successful...
+ VIR_FREE(transport);
And free it. That's a waste since you didn't use the 'transport' string out of the context here. Seems like you should use approach like this above but certainly not here.
+ } + }
Oh right, this leaks all the stuff on success, so that's why it doesn't crash. That's obviously not right.
+ + return 0; + + error: + VIR_FREE(transport); + virJSONValueFree(volfile_server); + virJSONValueFree(file); + virJSONValueFree(json); + +#endif /* WITH_STORAGE_GLUSTER */
You'd get compilation errors if WITH_STORAGE_GLUSTER is not enabled due to unused variables.
+ + return -1; +}
static int virStorageSourceParseBackingURI(virStorageSourcePtr src,
Peter
On Thu, Oct 08, 2015 at 17:25:53 +0530, Prasanna Kumar Kalever wrote:
This patch adds support for gluster specific JSON parser functionality
This patch needs to go in before we actually add the support, so as 1/1 of this series.
This will help in parsing the backing store which uses JSON syntax and update the meta-data in the domain specific objects while taking snapshots which inturn helps in successful creation/updation of backing store information in domain xml
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- src/util/virstoragefile.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2aa1d90..c122d3a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -41,6 +41,7 @@ #include "virstring.h" #include "virutil.h" #include "viruri.h" +#include "virjson.h" #include "dirname.h" #include "virbuffer.h"
@@ -2124,6 +2125,132 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, goto cleanup; }
+static int +virStorageSourceParseBackingJSON(virStorageSourcePtr src, + const char *path)
Weird alignment. Also you probably missed a part in my previous review where I'd requested that this is put into the qemu specific code rather than the generic util code since this is too qemu specific.
On the other hand, it might be worth checking where the backing chain functions are used and just move all the backing store parsers into qemu code as probably all of it is very qemu specific too.
Can you please tell me into which file is it meaningful to push "virStorageSourceParseBackingJSON" under src/qemu or do you prefer to create a new file with name src/qemu/qemu_parse_json.c ?
+{ +#if WITH_STORAGE_GLUSTER
Either the whole function is in the #if block and you have an else block with a stub function that reports an error, or preferred is to have this even if we don't compile with gluster since this doesn't use any gluster api so it can be compiled always
+ + virJSONValuePtr json = NULL; + virJSONValuePtr file = NULL; + virJSONValuePtr volfile_server = NULL; + char *transport = NULL; + int port = 0; + size_t i = 0; + + /* The string may look like: + * json:{ 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" } + */ + + json = virJSONValueFromString(path+strlen("json:")); + + if (virJSONValueObjectHasKey(json, "driver")) { + if (!(src->format = virStorageFileFormatTypeFromString( + virJSONValueObjectGetString(json, "driver")))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get Format type of key 'driver' from JSON"));
virStorageFileFormatTypeFromString returns -1 if it fails to parse the format type. Format '0' might be valid.
+ goto error; + } + } + + if (virJSONValueObjectHasKey(json, "file")) { + if (!(file = virJSONValueObjectGet(json, "file"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unbale to get Object from key 'file'")); + goto error; + } + } + + if (virJSONValueObjectHasKey(file, "driver")) { + if (!(src->protocol = virStorageNetProtocolTypeFromString( + virJSONValueObjectGetString(file, "driver")))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get Protocol type of key 'driver' from JSON"));
Same comment as above for usage of the enum->string helpers.
Additionally the assumption that 'drive' will contain something that can be used successfully with virStorageNetProtocolTypeFromString is wrong.
The json syntax can be used with all the remote or local files and the virStorageNetProtocolTypeFromString certainly won't recognize local files. That would result in a rather unhelpful error.
+ goto error; + } + } + + if (virJSONValueObjectHasKey(file, "volname")) {
This uses the old naming in qemu, that I've pointed out is not what should be done in qemu. Additionally my comment about the naming was done prior to this submission.
I'd suggest you tackle the qemu part first at this point.
Additionally this is a gluster specific field. And the parser at least according to the function name sounds rather generic. I think this would badly fail on anything non'gluster.
+ if (!(src->volume = (char *) virJSONValueObjectGetString(file, + "volname"))) {
You are extracting a string but you are not copying it. virJSONValueObjectGetString returns just the pointer to the string in the json data structure. This either crashes or leaks the json array so that it accidentally remains valid. Did you actually test it?
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unbale to get String from key 'volname'")); + goto error; + } + } + + if (virJSONValueObjectHasKey(file, "image-path")) {
Again ... old field name.
+ if (!(src->path = (char *) virJSONValueObjectGetString(file, + "image-path"))) {
And wrong extraction of it.
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get String from key 'image-path'")); + goto error; + } + } + + if (virJSONValueObjectHasKey(file, "volfile-servers")) {
Wrong field name. This is also the reason why "volfile-servers" is a terrible name. Any other protocol that certainly won't call it's server field volfile-servers. We would need to add a different code path here.
Gluster isn't the only storage technology on the planet, so this code needs to be extensible
Peter, I have send the patch to qemu just before with the naming conventions yourself and Kevin suggested on v5 patch of qemu. But before sending this patch qemu v5 patch-set have the older naming conventions, so obviously if somebody want to test this patch which depends on qemu patch, the naming conventions must be as qemu v5 path-set, so knowingly I followed the older naming conventions. I hope you understand that part.
Anyways I know this patch-set is not the final one that goes-in Coz of some of holes in my understanding of being very new user to libvirt, I shall update this patch with qemu v6 naming conventions very soon.
I shall also consider the valuable points you mention in this mail.
Thank you.
+ if (!(src->nhosts = virJSONValueArraySize(virJSONValueObjectGet(file, + "volfile-servers")))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get Size of Array of key 'volfile-servers'")); + goto error; + } + } + + /* null-terminated list */
Why? We store the countof elements.
+ if (VIR_ALLOC_N(src->hosts, src->nhosts + 1) < 0) + goto error; + + for (i = 0; i < src->nhosts; i++) { + volfile_server = virJSONValueArrayGet(virJSONValueObjectGetArray(file, + "volfile-servers"), i);
Wrong name. Weird alignment.
+ + if (virJSONValueObjectHasKey(volfile_server, "server")) {
Wrong field name.
+ if (VIR_STRDUP(src->hosts[i].name, + virJSONValueObjectGetString(volfile_server, "server")) < 0) {
Wow, here you copy the string. Didn't that seem suspicious compared to the avoce usage?
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get String from key 'server'"));
You've validated that "server" key exists, so the only error here is if VIR_STRDUP fails. VIR_STRDUP also reports it's own errors.
+ goto error; + } + } + + if (virJSONValueObjectHasKey(volfile_server, "port")) { + if (virJSONValueObjectGetNumberInt(volfile_server, "port", &port) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get NumberInt from key 'port'")); + goto error; + } + if (virAsprintf(&src->hosts[i].port, "%d", port) < 0) + goto error; + } + + if (virJSONValueObjectHasKey(volfile_server, "transport")) { + if (VIR_STRDUP(transport, virJSONValueObjectGetString(volfile_server, + "transport")) < 0) {
Here you duplicate the string containing the transport ....
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get String from key 'transport'")); + goto error; + } + src->hosts[i].transport = virStorageNetHostTransportTypeFromString(transport);
Convert it without actually checking that the conversion was successful...
+ VIR_FREE(transport);
And free it. That's a waste since you didn't use the 'transport' string out of the context here. Seems like you should use approach like this above but certainly not here.
+ } + }
Oh right, this leaks all the stuff on success, so that's why it doesn't crash. That's obviously not right.
+ + return 0; + + error: + VIR_FREE(transport); + virJSONValueFree(volfile_server); + virJSONValueFree(file); + virJSONValueFree(json); + +#endif /* WITH_STORAGE_GLUSTER */
You'd get compilation errors if WITH_STORAGE_GLUSTER is not enabled due to unused variables.
+ + return -1; +}
static int virStorageSourceParseBackingURI(virStorageSourcePtr src,
Peter
On Mon, Oct 12, 2015 at 09:14:33 -0400, Prasanna Kumar Kalever wrote: (Missing reply header?)
On Thu, Oct 08, 2015 at 17:25:53 +0530, Prasanna Kumar Kalever wrote:
(I'd like to strongly suggest that you trim down the responses to the relevant portions only ... ) [...]
On the other hand, it might be worth checking where the backing chain functions are used and just move all the backing store parsers into qemu code as probably all of it is very qemu specific too.
Can you please tell me into which file is it meaningful to push "virStorageSourceParseBackingJSON" under src/qemu or do you prefer to create a new file with name src/qemu/qemu_parse_json.c ?
I've thought about that a bit more and for now we can go with leaving it in the common utils file. Despite being qemu specific, the storage driver uses the APIs too and we should be able to use the backing files created by qemu in the storage driver too so please disregard this point for now.
+{ +#if WITH_STORAGE_GLUSTER
[...]
Wrong field name. This is also the reason why "volfile-servers" is a terrible name. Any other protocol that certainly won't call it's server field volfile-servers. We would need to add a different code path here.
Gluster isn't the only storage technology on the planet, so this code needs to be extensible
Peter, I have send the patch to qemu just before with the naming conventions yourself and Kevin suggested on v5 patch of qemu. But before sending this patch qemu v5 patch-set have the older naming conventions, so obviously if somebody want to test this patch which depends on qemu patch, the naming conventions must be as qemu v5 path-set, so knowingly I followed the older naming conventions. I hope you understand that part.
Well, since you've posted this after you've received the review on the qemu part where it was almost certain that the design of the qemu part would change it didn't make much sense to post this since it wasted your time writing the code and even if the rest of the code would be in good shape you'd still need to post another version with the changed naming. Peter
On Thu, Oct 08, 2015 at 17:25:51 +0530, Prasanna Kumar Kalever wrote:
This patch adds support for initialization of gluster backend with multiple servers which acts as gluster volfile servers for the gluster storage backend.
This will help in achieving high availability of gluster backend connectivity via libgfapi i.e. when the first volfile server fails, then other servers specified are used as volfile server to re-establish the backend gluster connectivity.
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- src/storage/storage_backend_gluster.c | 77 +++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 36 deletions(-)
diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index d2e79bc..53474c1 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -32,6 +32,9 @@ #include "virstring.h" #include "viruri.h"
+#define QEMU_DEFAULT_GLUSTER_PORT 24007 +#define QEMU_DEFAULT_GLUSTER_TRANSPORT "tcp" + #define VIR_FROM_THIS VIR_FROM_STORAGE
VIR_LOG_INIT("storage.storage_backend_gluster"); @@ -570,62 +573,64 @@ static int virStorageFileBackendGlusterInit(virStorageSourcePtr src) { virStorageFileBackendGlusterPrivPtr priv = NULL; - virStorageNetHostDefPtr host = &(src->hosts[0]); - const char *hostname; + const char *transport = NULL;
'transport' doesn't need to be initialized here since it's used in a loop. This might cover up bugs.
int port = 0; + size_t i = 0;
- if (src->nhosts != 1) {
This still needs to check that we have at least one host.
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Expected exactly 1 host for the gluster volume")); - return -1; - } - - hostname = host->name; - - VIR_DEBUG("initializing gluster storage file %p (gluster://%s:%s/%s%s)[%u:%u]", - src, hostname, host->port ? host->port : "0", - NULLSTR(src->volume), src->path, - (unsigned int)src->drv->uid, (unsigned int)src->drv->gid); + VIR_DEBUG("Initializing gluster volume=%s image-path=%s with uid=%u gid=%u", + NULLSTR(src->volume), src->path, + (unsigned int)src->drv->uid, (unsigned int)src->drv->gid);
Wrong alignment ...
if (!src->volume) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("missing gluster volume name for path '%s'"), - src->path); + _("missing gluster volume name for path '%s'"), + src->path);
... and here you deliberately break alignment of existing code without any useful change. Please adhere to the libvirt coding style when posting patches. I will not complain further in this review about this.
return -1; }
if (VIR_ALLOC(priv) < 0) return -1;
- if (host->port && - virStrToLong_i(host->port, NULL, 10, &port) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to parse port number '%s'"), - host->port); - goto error; - } - - if (host->transport == VIR_STORAGE_NET_HOST_TRANS_UNIX) - hostname = host->socket; - if (!(priv->vol = glfs_new(src->volume))) { virReportOOMError(); goto error; }
- if (glfs_set_volfile_server(priv->vol, - virStorageNetHostTransportTypeToString(host->transport), - hostname, port) < 0) { - virReportSystemError(errno, - _("failed to set gluster volfile server '%s'"), - hostname); - goto error; + for (i = 0; i < src->nhosts; i++) { + if (!src->hosts[i].port) { + port = QEMU_DEFAULT_GLUSTER_PORT; + } else if (virStrToLong_i(src->hosts[i].port, NULL, 10, &port) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to parse port number '%s'"), src->hosts[i].port); + goto error; + } + + if (src->hosts[i].transport == VIR_STORAGE_NET_HOST_TRANS_UNIX) { + transport = src->hosts[i].socket; + } else if (!src->hosts[i].transport) { + transport = QEMU_DEFAULT_GLUSTER_TRANSPORT; + } else { + transport = virStorageNetHostTransportTypeToString(src->hosts[i].transport); + }
The whole above condition is breaking code for unix socket transport, sice you assign the socket path as the transport string. Additionally the case for default transport should not be necessary since AFAIK the parser code sets the default to TCP.
+ + if (glfs_set_volfile_server(priv->vol, transport, src->hosts[i].name, + port) < 0) { + virReportSystemError(errno, + _("failed to set gluster volfile server '%s'"), + src->hosts[i].name); + goto error; + } else { + VIR_DEBUG("Successfully set volfile server with server:'%s' " + "port:'%d' transport:'%s'", src->hosts[i].name, + port, transport); + } }
if (glfs_init(priv->vol) < 0) { virReportSystemError(errno, - _("failed to initialize gluster connection to " - "server: '%s'"), hostname); + _("failed to initialize gluster connection with given hosts " + "volume : '%s' image-path : '%s'"),
I'd prefer that the error message contained at least the first server.
+ NULLSTR(src->volume), src->path); goto error; }
Peter
participants (5)
-
Daniel P. Berrange -
Peter Krempa -
Prasanna Kalever -
Prasanna Kumar Kalever
-
Prasanna Kumar Kalever