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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 604 bytes)