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
Attachment:
pgpNomgnh_qIy.pgp
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list