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

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

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