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

[Eric Blake]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140717/ddadd9af/attachment-0001.sig>