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

> + 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