On Thu, Jul 17, 2014 at 10:02:45PM +0400, Roman Bogorodskiy wrote:
Eric Blake wrote:On 07/17/2014 10:57 AM, Roman Bogorodskiy wrote: > Roman Bogorodskiy wrote: > >> Looks like this breaks build with clang: >> >> gmake[3]: Entering directory `/usr/home/novel/code/libvirt/src' >> CC util/libvirt_util_la-virclosecallbacks.lo >> In file included from util/virclosecallbacks.c:28: >> In file included from ../src/util/virclosecallbacks.h:28: >> ../src/conf/domain_conf.h:70:35: error: redefinition of typedef 'virDomainNumatune' is a C11 feature [-Werror,-Wtypedef-redefinition] >> typedef struct _virDomainNumatune virDomainNumatune; >> ^ >> ../src/conf/numatune_conf.h:43:35: note: previous definition is here >> typedef struct _virDomainNumatune virDomainNumatune; >> ^ > > I got it fixed by the following diff: > > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 4c9b7e8..e4d7988 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -67,9 +67,6 @@ typedef virDomainFSDef *virDomainFSDefPtr; > typedef struct _virDomainNetDef virDomainNetDef; > typedef virDomainNetDef *virDomainNetDefPtr; > > -typedef struct _virDomainNumatune virDomainNumatune; > -typedef virDomainNumatune *virDomainNumatunePtr; > - > typedef struct _virDomainInputDef virDomainInputDef; > typedef virDomainInputDef *virDomainInputDefPtr; ACK to this hunk. > > @@ -1854,8 +1851,6 @@ struct _virDomainResourceDef { > * NB: if adding to this struct, virDomainDefCheckABIStability > * may well need an update > */ > -typedef struct _virDomainDef virDomainDef; > -typedef virDomainDef *virDomainDefPtr; > struct _virDomainDef { But this hunk feels fishy. Why does numatune_conf.h need virDomainDef? It's a leaky abstraction - virDomainNumatuneNodeParseXML is accessing def->cpu directly instead of limiting itself to just def->numatune. Also, virDomainNumatuneParseXML is accessing def->placement_mode, which argues that placement_mode should be part of def->numatune rather than an independent variable.
I tried abstracting as much as possible, and the numatune parsing code was really a mess. Now that it's abstracted a bit, I could rework it so that it doesn't require virDomainDef at all. It also needs to change something in the virDomainDef, but that could be some kind of output of the function.
Yes, this hunk solves the compiler fix, but it means that we are just cementing that we didn't abstract things into a single object. That is, my ideal setup would be that numatune has access to all the pieces that it reads/modifies as parameters, but not access the entire virDomainDef, and then we don't have a circular referencing situation and don't need numatune_conf.h to be the source of our typedef declaration of virDomainDef. > I didn't check it beyond build and check/syntax-check though. Anyway, it > doesn't look quite clean to have typedefs in numatune_conf.h for the > struct declared in domain_conf.h.
It wasn't problem with newer gcc since the pre-definition is the same as the actual typedef. And because all the gcc versions me and Michal compiled this on were newer, we didn't hit the bug and therefore I thought circular dependencies can be solved like this, so I haven't try that hard to abstract the whole numatune all the way :).
I'd be fine with your patch going in as a stop-gap compilation fix, even if I still think that we could restructure the code better to make numatune_conf.h self-contained and move the typedef for virDomainDef back to domain_conf.h.Good, so I pushed the patch.
And I'm reworking it so it looks and works as we want. Martin
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list