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

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

> 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. Good, so I pushed the patch.