Re: [libvirt] [PATCH v3 07/16] numatune: Encapsulate numatune configuration in order to unify results

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. 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. 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.