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

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(a)redhat.com&gt; 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.