1:55 p.m.
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(a)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
1:55 p.m.
New subject: [libvirt] [PATCH 2/3] qemu: add support for multiple gluster hosts/servers
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(a)redhat.com> and
"Deepak C Shetty" <deepakcs(a)redhat.com> for their support
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever(a)redhat.com>
---
v1: add support to gluster json formatter
v2: addressing "Peter Krempa" <pkrempa(a)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
2:57 p.m.
New subject: [libvirt] [PATCH 2/3] qemu: add support for multiple gluster hosts/servers
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(a)redhat.com> and
"Deepak C Shetty" <deepakcs(a)redhat.com> for their support
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever(a)redhat.com>
---
v1: add support to gluster json formatter
v2: addressing "Peter Krempa" <pkrempa(a)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 :|
2:43 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: add support for multiple gluster hosts/servers
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(a)redhat.com> and
"Deepak C Shetty" <deepakcs(a)redhat.com> for their support
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever(a)redhat.com>
---
v1: add support to gluster json formatter
v2: addressing "Peter Krempa" <pkrempa(a)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
1:55 p.m.
New subject: [libvirt] [PATCH 3/3] util: add backing store parser support for gluster protocol
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(a)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
9:48 a.m.
New subject: [libvirt] [PATCH 3/3] util: add backing store parser support for gluster protocol
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(a)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
4:28 p.m.
New subject: [libvirt] [PATCH 3/3] util: add backing store parser support for gluster protocol
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(a)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
3:14 p.m.
New subject: [libvirt] [PATCH 3/3] util: add backing store parser support for gluster protocol
> 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(a)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
>
5:32 p.m.
New subject: [libvirt] [PATCH 3/3] util: add backing store parser support for gluster protocol
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
2:30 a.m.
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(a)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
3265
days inactive
3270
days old
9 comments
5 participants
participants (5)
-
Daniel P. Berrange -
Peter Krempa -
Prasanna Kalever -
Prasanna Kumar Kalever
-
Prasanna Kumar Kalever