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

Martin Kletzander mkletzan at redhat.com
Fri Jul 18 12:02:14 UTC 2014


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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140718/1941716b/attachment-0001.sig>


More information about the libvir-list mailing list