| Message ID | 20201023183652.478921-2-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Exposing backing-chain allocation over NBD | expand |
23.10.2020 21:36, Eric Blake wrote: > Placing GenericList in util.h will make it easier for the next patch > to promote QAPI_LIST_ADD() into a public macro without requiring more > files to include the unrelated visitor.h. > > However, we can't also move GenericAlternate; this is because it would > introduce a circular dependency: qapi-builtin-types.h needs a complete > definition of QEnumLookup (so it includes qapi/util.h), and > GenericAlternate needs a complete definition of QType (declared in > qapi-builtin-types.h). Leaving GenericAlternate in visitor.h breaks > the cycle, and doesn't matter since we don't have any further planned > uses for that type outside of visitors. > > Suggested-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/qapi/visitor.h | 9 +-------- > include/qapi/util.h | 8 ++++++++ > 2 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h > index ebc19ede7fff..8c2e1c29ad8b 100644 > --- a/include/qapi/visitor.h > +++ b/include/qapi/visitor.h > @@ -16,6 +16,7 @@ > #define QAPI_VISITOR_H > > #include "qapi/qapi-builtin-types.h" > +#include "qapi/util.h" > > /* > * The QAPI schema defines both a set of C data types, and a QMP wire > @@ -228,14 +229,6 @@ > > /*** Useful types ***/ > > -/* This struct is layout-compatible with all other *List structs > - * created by the QAPI generator. It is used as a typical > - * singly-linked list. */ > -typedef struct GenericList { > - struct GenericList *next; > - char padding[]; > -} GenericList; > - > /* This struct is layout-compatible with all Alternate types > * created by the QAPI generator. */ > typedef struct GenericAlternate { > diff --git a/include/qapi/util.h b/include/qapi/util.h > index a7c3c6414874..50201896c7a4 100644 > --- a/include/qapi/util.h > +++ b/include/qapi/util.h > @@ -11,6 +11,14 @@ > #ifndef QAPI_UTIL_H > #define QAPI_UTIL_H > > +/* This struct is layout-compatible with all other *List structs > + * created by the QAPI generator. It is used as a typical > + * singly-linked list. */ doesn't checkpatch complain for comment style? > +typedef struct GenericList { > + struct GenericList *next; > + char padding[]; > +} GenericList; > + > typedef struct QEnumLookup { > const char *const *array; > int size; >
Eric Blake <eblake@redhat.com> writes: > Placing GenericList in util.h will make it easier for the next patch > to promote QAPI_LIST_ADD() into a public macro without requiring more > files to include the unrelated visitor.h. Is this true? You don't actually need GenericList to make use of QAPI_LIST_ADD(), do you? Any QAPI list type should do. > However, we can't also move GenericAlternate; this is because it would > introduce a circular dependency: qapi-builtin-types.h needs a complete > definition of QEnumLookup (so it includes qapi/util.h), and > GenericAlternate needs a complete definition of QType (declared in > qapi-builtin-types.h). Leaving GenericAlternate in visitor.h breaks > the cycle, and doesn't matter since we don't have any further planned > uses for that type outside of visitors. > > Suggested-by: Markus Armbruster <armbru@redhat.com> I did suggest to consider moving GenericList and GenericAlternate next to QAPI_LIST_ADD(), because they're (loosely) related. We can't move GenericAlternate. Moving only GenericList brings GenericList and QAPI_LIST_ADD() together, but separates the more closely related GenericList and GenericAlternate. Meh. I'd leave it put. > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > include/qapi/visitor.h | 9 +-------- > include/qapi/util.h | 8 ++++++++ > 2 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h > index ebc19ede7fff..8c2e1c29ad8b 100644 > --- a/include/qapi/visitor.h > +++ b/include/qapi/visitor.h > @@ -16,6 +16,7 @@ > #define QAPI_VISITOR_H > > #include "qapi/qapi-builtin-types.h" > +#include "qapi/util.h" Not necessary, qapi-builtin-types.h must include it for QEnumLookup. > /* > * The QAPI schema defines both a set of C data types, and a QMP wire > @@ -228,14 +229,6 @@ > > /*** Useful types ***/ > > -/* This struct is layout-compatible with all other *List structs > - * created by the QAPI generator. It is used as a typical > - * singly-linked list. */ > -typedef struct GenericList { > - struct GenericList *next; > - char padding[]; > -} GenericList; > - > /* This struct is layout-compatible with all Alternate types > * created by the QAPI generator. */ > typedef struct GenericAlternate { > diff --git a/include/qapi/util.h b/include/qapi/util.h > index a7c3c6414874..50201896c7a4 100644 > --- a/include/qapi/util.h > +++ b/include/qapi/util.h > @@ -11,6 +11,14 @@ > #ifndef QAPI_UTIL_H > #define QAPI_UTIL_H > > +/* This struct is layout-compatible with all other *List structs > + * created by the QAPI generator. It is used as a typical > + * singly-linked list. */ > +typedef struct GenericList { > + struct GenericList *next; > + char padding[]; > +} GenericList; > + > typedef struct QEnumLookup { > const char *const *array; > int size;
On 10/26/20 9:18 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Placing GenericList in util.h will make it easier for the next patch >> to promote QAPI_LIST_ADD() into a public macro without requiring more >> files to include the unrelated visitor.h. > > Is this true? > > You don't actually need GenericList to make use of QAPI_LIST_ADD(), do > you? Any QAPI list type should do. Correct, compilation still works if I drop this patch. > >> However, we can't also move GenericAlternate; this is because it would >> introduce a circular dependency: qapi-builtin-types.h needs a complete >> definition of QEnumLookup (so it includes qapi/util.h), and >> GenericAlternate needs a complete definition of QType (declared in >> qapi-builtin-types.h). Leaving GenericAlternate in visitor.h breaks >> the cycle, and doesn't matter since we don't have any further planned >> uses for that type outside of visitors. >> >> Suggested-by: Markus Armbruster <armbru@redhat.com> > > I did suggest to consider moving GenericList and GenericAlternate next > to QAPI_LIST_ADD(), because they're (loosely) related. We can't move > GenericAlternate. Moving only GenericList brings GenericList and > QAPI_LIST_ADD() together, but separates the more closely related > GenericList and GenericAlternate. Meh. > > I'd leave it put. Agreed. Dropping this patch in v6.
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index ebc19ede7fff..8c2e1c29ad8b 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -16,6 +16,7 @@ #define QAPI_VISITOR_H #include "qapi/qapi-builtin-types.h" +#include "qapi/util.h" /* * The QAPI schema defines both a set of C data types, and a QMP wire @@ -228,14 +229,6 @@ /*** Useful types ***/ -/* This struct is layout-compatible with all other *List structs - * created by the QAPI generator. It is used as a typical - * singly-linked list. */ -typedef struct GenericList { - struct GenericList *next; - char padding[]; -} GenericList; - /* This struct is layout-compatible with all Alternate types * created by the QAPI generator. */ typedef struct GenericAlternate { diff --git a/include/qapi/util.h b/include/qapi/util.h index a7c3c6414874..50201896c7a4 100644 --- a/include/qapi/util.h +++ b/include/qapi/util.h @@ -11,6 +11,14 @@ #ifndef QAPI_UTIL_H #define QAPI_UTIL_H +/* This struct is layout-compatible with all other *List structs + * created by the QAPI generator. It is used as a typical + * singly-linked list. */ +typedef struct GenericList { + struct GenericList *next; + char padding[]; +} GenericList; + typedef struct QEnumLookup { const char *const *array; int size;
Placing GenericList in util.h will make it easier for the next patch to promote QAPI_LIST_ADD() into a public macro without requiring more files to include the unrelated visitor.h. However, we can't also move GenericAlternate; this is because it would introduce a circular dependency: qapi-builtin-types.h needs a complete definition of QEnumLookup (so it includes qapi/util.h), and GenericAlternate needs a complete definition of QType (declared in qapi-builtin-types.h). Leaving GenericAlternate in visitor.h breaks the cycle, and doesn't matter since we don't have any further planned uses for that type outside of visitors. Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- include/qapi/visitor.h | 9 +-------- include/qapi/util.h | 8 ++++++++ 2 files changed, 9 insertions(+), 8 deletions(-)