Thanks Paolo, and everyone else for the corrections :) I will try to fix it in a patch this week or the next. I referred to gluster.c implementation as it was closer to what we wanted to achieve i.e. passing multiple servers for the block device. I picked up the idea of referring to gluster.c from the following email that I received on libvirt mailing list. I did test my implementation to make sure that it worked fine, but did not realize that it was not clean. I will fix it.
/====================/ Hmm, unless I'm mis-reading this, there is no separator between the two URIs you're providing - is there a missing ',' or something similar. Slightly off topic for the libvirt list, but I would be pretty surprised if the QEMU developers accepted patches using this URI syntax for providing multiple servers. A similar approach was originally proposed for the glusterfs driver and the submitter was told to write it a different way, not using the legacy URI syntax at all, but instead a QAPI based syntax: https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg04126.html So the sooner you can submit your QEMU patches to the qemu-devel mailing list the better, as we'll need to get clarity on the accepted QEMU syntax in order to proceed further with libvirt patch review. Broadly speaking I think the patch you've proposed looks pretty much fine now. I'd have a slight preference for it to be done as two patches. One patch adds the XML schema, parser changes, etc. The second patch just does the QEMU driver integration & unit tests. ... /====================/ Regards, Ashish On Wed, Aug 17, 2016 at 4:22 AM, Paolo Bonzini <pbonz...@redhat.com> wrote: > > > On 15/08/2016 18:29, ashish mittal wrote: >>>> +/* >>>> + * Convert the json formatted command line into qapi. >>>> +*/ >>>> + >>>> +static int vxhs_parse_json(BlockdevOptionsVxHS *conf, >>>> + QDict *options, Error **errp) >>>> +{ >> ... >>>> + errno = EINVAL; >>>> + return -errno; >>>> +} >>> >>> Ewww this is really horrible code. It is open-coding a special purpose >>> conversion of QemuOpts -> QDict -> QAPI scheme. We should really put >>> my qdict_crumple() API impl as a pre-requisite of this, so you can then >>> use qdict_crumple + qmp_input_visitor to do this conversion in a generic >>> manner removing all this code. >>> >>> https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg03118.html >>> >> >> Thanks for the suggestions. I will try to incorporate them in the next >> version. Actually, I used block/gluster.c for reference (as advised). >> This is exactly how it parses json CLI options. It also implements the >> *parse_uri() in a similar way. > > I think I pointed you to block/nbd.c instead, which works as Kevin > suggested. > > Paolo
