Re: [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&gt; > > --- > > 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 >