[libvirt] [PATCH v4 1/4] util: change the virStorageNetHostDef type

[Prasanna Kalever]

On Mon, Dec 12, 2016 at 8:00 PM, Peter Krempa <pkrempa at redhat.com> wrote:
> On Mon, Dec 12, 2016 at 19:54:47 +0530, Prasanna Kalever wrote:
>> On Wed, Dec 7, 2016 at 7:31 PM, Peter Krempa <pkrempa at redhat.com> wrote:
>> > On Tue, Dec 06, 2016 at 22:51:58 +0530, Prasanna Kumar Kalever wrote:
>> >> Currently, the Host object looks like
>> >>
>> >> struct _virStorageNetHostDef {
>> >>         char *name;
>> >>         char *port;
>> >>         int transport; /* virStorageNetHostTransport */
>> >>         char *socket;  /* path to unix socket */
>> >> }
>> >>
>> >> We don't actually need a 'name' and 'port' if the transport type is unix
>> >> domain sockets, and if the transport is inet tcp/rdma we don't actually
>> >> care for socket path.
>> >>
>> >> With a simple union discrimination we can make this much better
>> >>
>> >> struct _virStorageNetHostUnixSockAddr {
>> >>     char *path;
>> >> };
>> >>
>> >> struct _virStorageNetHostInetSockAddr {
>> >>     char *addr;
>> >>     char *port;
>> >> };
>> >>
>> >> struct _virStorageNetHostDef {
>> >>         virStorageNetHostTransport type;
>> >>         union { /* union tag is @type */
>> >>             virStorageNetHostUnixSockAddr uds;
>> >>             virStorageNetHostInetSockAddr inet;
>> >>         } u;
>> >> };
>> >
>> > I'm not really persuaded that this is much better than the current
>> > state. Data-wise you save one char *. Code wise you add the complexity
>> > of accessing the enum everywhere.
>> >
>> >>
>> >> This patch performs the required changes in transforming _virStorageNetHostDef
>> >> to fit union discrimination.
>> >
>> > On a machine with xen installed this fails to compile:
>> >
>> > xenconfig/xen_xl.c: In function 'xenFormatXLDiskSrcNet':
>> > xenconfig/xen_xl.c:954:41: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'name'
>> >                  if (strchr(src->hosts[i].name, ':'))
>> >                                          ^
>> > xenconfig/xen_xl.c:956:50: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'name'
>> >                                      src->hosts[i].name);
>> >                                                   ^
>> > xenconfig/xen_xl.c:958:64: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'name'
>> >                      virBufferAsprintf(&buf, "%s", src->hosts[i].name);
>> >                                                                 ^
>> > xenconfig/xen_xl.c:960:34: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'port'
>> >                  if (src->hosts[i].port)
>> >                                   ^
>> > xenconfig/xen_xl.c:961:69: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'port'
>> >                      virBufferAsprintf(&buf, "\\\\:%s", src->hosts[i].port);
>> >
>> > I suspect there are more than just these in other hypervisor drivers.
>>
>> HUhhh...
>> How to make sure that I have covered all the drivers ?
>> Any long list of all drivers in libvirt or config to compile all the modules ?
>
> Output of 'configure' lists which ones are enabled and which are
> disabled.
>
> I'd suggest that you drop this patch though as the complexity of
> accessing the union is not worth the gain of removing one pointer.

IMO, this is not just about saving a pointer. It is preferred to have
the transport elements on the type.

Please excuse me, if it doesn't make sense to mention about other
projects approaches here but, recently after qemu add supports to qapi
union discrimination it now follows similar union.
In terms of all the gluster api users does use the union
discrimination (there are another bunch of projects)

If we cannot migrate to this now, Its very unfortunate we get this chance again.

I shall post a patch addressing all the review comments in your
previous email, lets give a final try ?

Apologies for stressing this out.

Thanks,
--
Prasanna