[libvirt] [PATCH 3/3] util: add backing store parser support for gluster protocol
Prasanna Kumar Kalever
pkalever at redhat.com
Mon Oct 12 13:14:33 UTC 2015
> > 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 at redhat.com>
> > > ---
> > > 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
> >
>
More information about the libvir-list
mailing list