Re: [PATCH 3/3] util: add backing store parser support for gluster protocol

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]