qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v3 RFC] block/vxhs: Initial commit to add Verita


From: ashish mittal
Subject: Re: [Qemu-devel] [PATCH v3 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support
Date: Wed, 17 Aug 2016 14:58:14 -0700

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 <address@hidden> 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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]