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
> + 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list