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