> 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@xxxxxxxxxx>
> > ---
> > 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list