| Message ID | 20201023183652.478921-3-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Exposing backing-chain allocation over NBD | expand |
23.10.2020 21:36, Eric Blake wrote: > We have a useful macro for inserting at the front of any > QAPI-generated list; move it from block.c to qapi/util.h so more > places can use it, including one earlier place in block.c. > > Suggested-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com> > Signed-off-by: Eric Blake<eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> -- Best regards, Vladimir
Eric Blake <eblake@redhat.com> writes: > We have a useful macro for inserting at the front of any > QAPI-generated list; move it from block.c to qapi/util.h so more > places can use it, including one earlier place in block.c. > > Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > include/qapi/util.h | 8 ++++++++ > block.c | 15 +++------------ > 2 files changed, 11 insertions(+), 12 deletions(-) > > diff --git a/include/qapi/util.h b/include/qapi/util.h > index 50201896c7a4..b6083055ce69 100644 > --- a/include/qapi/util.h > +++ b/include/qapi/util.h > @@ -30,4 +30,12 @@ int qapi_enum_parse(const QEnumLookup *lookup, const char *buf, > > int parse_qapi_name(const char *name, bool complete); > > +/* For any GenericList @list, insert @element at the front. */ > +#define QAPI_LIST_ADD(list, element) do { \ > + typeof(list) _tmp = g_new(typeof(*(list)), 1); \ g_new(typeof(*p), 1) is an rather roundabout way to say g_malloc(sizeof(*p). Yes, it returns typeof(p) instead of void *, but the chance of this catching mistakes here rounds to zero. Not this patch's problem. Ignore me :) > + _tmp->value = (element); \ > + _tmp->next = (list); \ > + (list) = _tmp; \ > +} while (0) > + > #endif > diff --git a/block.c b/block.c > index 430edf79bb10..45bd79299611 100644 > --- a/block.c > +++ b/block.c > @@ -39,6 +39,7 @@ > #include "qapi/qmp/qstring.h" > #include "qapi/qobject-output-visitor.h" > #include "qapi/qapi-visit-block-core.h" > +#include "qapi/util.h" > #include "sysemu/block-backend.h" > #include "sysemu/sysemu.h" > #include "qemu/notify.h" > @@ -5211,7 +5212,7 @@ BlockDriverState *bdrv_find_node(const char *node_name) > BlockDeviceInfoList *bdrv_named_nodes_list(bool flat, > Error **errp) > { > - BlockDeviceInfoList *list, *entry; > + BlockDeviceInfoList *list; > BlockDriverState *bs; > > list = NULL; > @@ -5221,22 +5222,12 @@ BlockDeviceInfoList *bdrv_named_nodes_list(bool flat, > qapi_free_BlockDeviceInfoList(list); > return NULL; > } > - entry = g_malloc0(sizeof(*entry)); > - entry->value = info; > - entry->next = list; > - list = entry; > + QAPI_LIST_ADD(list, info); > } > > return list; > } PATCH 12 has more. I wonder whether PATCH 12 should be squashed into this one. You decide. > > -#define QAPI_LIST_ADD(list, element) do { \ > - typeof(list) _tmp = g_new(typeof(*(list)), 1); \ > - _tmp->value = (element); \ > - _tmp->next = (list); \ > - (list) = _tmp; \ > -} while (0) > - > typedef struct XDbgBlockGraphConstructor { > XDbgBlockGraph *graph; > GHashTable *graph_nodes; Reviewed-by: Markus Armbruster <armbru@redhat.com>
On 10/26/20 9:25 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> We have a useful macro for inserting at the front of any >> QAPI-generated list; move it from block.c to qapi/util.h so more >> places can use it, including one earlier place in block.c. >> >> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> include/qapi/util.h | 8 ++++++++ >> block.c | 15 +++------------ >> 2 files changed, 11 insertions(+), 12 deletions(-) >> >> diff --git a/include/qapi/util.h b/include/qapi/util.h >> index 50201896c7a4..b6083055ce69 100644 >> --- a/include/qapi/util.h >> +++ b/include/qapi/util.h >> @@ -30,4 +30,12 @@ int qapi_enum_parse(const QEnumLookup *lookup, const char *buf, >> >> int parse_qapi_name(const char *name, bool complete); >> >> +/* For any GenericList @list, insert @element at the front. */ >> +#define QAPI_LIST_ADD(list, element) do { \ >> + typeof(list) _tmp = g_new(typeof(*(list)), 1); \ > > g_new(typeof(*p), 1) is an rather roundabout way to say > g_malloc(sizeof(*p). Yes, it returns typeof(p) instead of void *, but > the chance of this catching mistakes here rounds to zero. > > Not this patch's problem. Ignore me :) typeof is a gcc/clang extension; using sizeof makes the code slightly more portable (but doesn't affect qemu's usage). I'm happy to change it, although it would require more commit message finesse since that becomes more than just code motion. >> @@ -5221,22 +5222,12 @@ BlockDeviceInfoList *bdrv_named_nodes_list(bool flat, >> qapi_free_BlockDeviceInfoList(list); >> return NULL; >> } >> - entry = g_malloc0(sizeof(*entry)); >> - entry->value = info; >> - entry->next = list; >> - list = entry; >> + QAPI_LIST_ADD(list, info); >> } >> >> return list; >> } > > PATCH 12 has more. I wonder whether PATCH 12 should be squashed into > this one. You decide. Patch 12 has a LOT more. And we're really close to soft freeze. I kept them separate to minimize the risk of landing my QAPI changes (4/12) before 5.2 (that MUST happen, or we've locked in a problematic API with block-export-add), vs. cleanup changes that may require review from a lot more areas in the tree. Given the timeline, I prefer to keep them separate for v6. I also still wonder if it is possible to make Coccinelle identify candidates, rather than my manual audit that produced patch 12. But for v6, I _will_ update the commit message to mention that more conversions are possible, and saved for a later patch. > > Reviewed-by: Markus Armbruster <armbru@redhat.com> Thanks; I think I can keep this even for v6, since all I am changing is an enhanced commit message. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 10/26/20 9:37 AM, Eric Blake wrote: >> PATCH 12 has more. I wonder whether PATCH 12 should be squashed into >> this one. You decide. > > Patch 12 has a LOT more. And we're really close to soft freeze. I kept > them separate to minimize the risk of landing my QAPI changes (4/12) Correction - the QAPI change was 6/12 of v5 (whereas it moves earlier in the series in my upcoming v6 posting) > before 5.2 (that MUST happen, or we've locked in a problematic API with > block-export-add), vs. cleanup changes that may require review from a > lot more areas in the tree. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
diff --git a/include/qapi/util.h b/include/qapi/util.h index 50201896c7a4..b6083055ce69 100644 --- a/include/qapi/util.h +++ b/include/qapi/util.h @@ -30,4 +30,12 @@ int qapi_enum_parse(const QEnumLookup *lookup, const char *buf, int parse_qapi_name(const char *name, bool complete); +/* For any GenericList @list, insert @element at the front. */ +#define QAPI_LIST_ADD(list, element) do { \ + typeof(list) _tmp = g_new(typeof(*(list)), 1); \ + _tmp->value = (element); \ + _tmp->next = (list); \ + (list) = _tmp; \ +} while (0) + #endif diff --git a/block.c b/block.c index 430edf79bb10..45bd79299611 100644 --- a/block.c +++ b/block.c @@ -39,6 +39,7 @@ #include "qapi/qmp/qstring.h" #include "qapi/qobject-output-visitor.h" #include "qapi/qapi-visit-block-core.h" +#include "qapi/util.h" #include "sysemu/block-backend.h" #include "sysemu/sysemu.h" #include "qemu/notify.h" @@ -5211,7 +5212,7 @@ BlockDriverState *bdrv_find_node(const char *node_name) BlockDeviceInfoList *bdrv_named_nodes_list(bool flat, Error **errp) { - BlockDeviceInfoList *list, *entry; + BlockDeviceInfoList *list; BlockDriverState *bs; list = NULL; @@ -5221,22 +5222,12 @@ BlockDeviceInfoList *bdrv_named_nodes_list(bool flat, qapi_free_BlockDeviceInfoList(list); return NULL; } - entry = g_malloc0(sizeof(*entry)); - entry->value = info; - entry->next = list; - list = entry; + QAPI_LIST_ADD(list, info); } return list; } -#define QAPI_LIST_ADD(list, element) do { \ - typeof(list) _tmp = g_new(typeof(*(list)), 1); \ - _tmp->value = (element); \ - _tmp->next = (list); \ - (list) = _tmp; \ -} while (0) - typedef struct XDbgBlockGraphConstructor { XDbgBlockGraph *graph; GHashTable *graph_nodes;
We have a useful macro for inserting at the front of any QAPI-generated list; move it from block.c to qapi/util.h so more places can use it, including one earlier place in block.c. Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- include/qapi/util.h | 8 ++++++++ block.c | 15 +++------------ 2 files changed, 11 insertions(+), 12 deletions(-)