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

[Roman Bogorodskiy]

  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.

Roman Bogorodskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140717/1a55b4a6/attachment-0001.sig>