Re: [Qemu-devel] [PATCH 2/6] qemu/queue.h: leave head structs anonymous unless necessary

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> Most list head structs need not be given a name.  In most cases the
> name is given just in case one is going to use QTAILQ_LAST, QTAILQ_PREV
> or reverse iteration, but this does not apply to lists of other kinds,
> and even for QTAILQ in practice this is only rarely needed.  In addition,
> we will soon reimplement those macros completely so that they do not
> need a name for the head struct.  So clean up everything, not giving a
> name except in the rare case where it is necessary.
>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
[...]
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 4880a05399..4e1de942ce 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -86,7 +86,7 @@ struct KVMState
>      int robust_singlestep;
>      int debugregs;
>  #ifdef KVM_CAP_SET_GUEST_DEBUG
> -    struct kvm_sw_breakpoint_head kvm_sw_breakpoints;
> +    QTAILQ_HEAD(, kvm_sw_breakpoint) kvm_sw_breakpoints;
>  #endif
>      int many_ioeventfds;
>      int intx_set_mask;
> @@ -102,7 +102,7 @@ struct KVMState
>      int nr_allocated_irq_routes;
>      unsigned long *used_gsi_bitmap;
>      unsigned int gsi_count;
> -    QTAILQ_HEAD(msi_hashtab, KVMMSIRoute) msi_hashtab[KVM_MSI_HASHTAB_SIZE];
> +    QTAILQ_HEAD(, KVMMSIRoute) msi_hashtab[KVM_MSI_HASHTAB_SIZE];
>  #endif
>      KVMMemoryListener memory_listener;
>      QLIST_HEAD(, KVMParkedVcpu) kvm_parked_vcpus;
> diff --git a/block/gluster.c b/block/gluster.c
> index 5e300c96c8..72891060e3 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -72,7 +72,7 @@ typedef struct ListElement {
>      GlfsPreopened saved;
>  } ListElement;
>  
> -static QLIST_HEAD(glfs_list, ListElement) glfs_list;
> +static QLIST_HEAD(, ListElement) glfs_list;
>  
>  static QemuOptsList qemu_gluster_create_opts = {
>      .name = "qemu-gluster-create-opts",
> diff --git a/block/mirror.c b/block/mirror.c
> index 8f52c6215d..6250cc3c87 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -72,7 +72,7 @@ typedef struct MirrorBlockJob {
>      unsigned long *in_flight_bitmap;
>      int in_flight;
>      int64_t bytes_in_flight;
> -    QTAILQ_HEAD(MirrorOpList, MirrorOp) ops_in_flight;
> +    QTAILQ_HEAD(, MirrorOp) ops_in_flight;
>      int ret;
>      bool unmap;
>      int target_cluster_size;
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index accebef4cf..b946301429 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -77,8 +77,6 @@ typedef struct Qcow2BitmapTable {
>      uint32_t size; /* number of 64bit entries */
>      QSIMPLEQ_ENTRY(Qcow2BitmapTable) entry;
>  } Qcow2BitmapTable;
> -typedef QSIMPLEQ_HEAD(Qcow2BitmapTableList, Qcow2BitmapTable)
> -    Qcow2BitmapTableList;
>  
>  typedef struct Qcow2Bitmap {
>      Qcow2BitmapTable table;
> @@ -1316,7 +1314,7 @@ void 
> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>      int ret;
>      Qcow2BitmapList *bm_list;
>      Qcow2Bitmap *bm;
> -    Qcow2BitmapTableList drop_tables;
> +    QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables;
>      Qcow2BitmapTable *tb, *tb_next;
>  
>      if (!bdrv_has_changed_persistent_bitmaps(bs)) {

Thanks for getting rid of typedefs like this one.

> diff --git a/block/qcow2.h b/block/qcow2.h
> index 8662b68575..d747dd14a2 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -281,7 +281,7 @@ typedef struct BDRVQcow2State {
>      uint8_t *cluster_cache;
>      uint8_t *cluster_data;
>      uint64_t cluster_cache_offset;
> -    QLIST_HEAD(QCowClusterAlloc, QCowL2Meta) cluster_allocs;
> +    QLIST_HEAD(, QCowL2Meta) cluster_allocs;
>  
>      uint64_t *refcount_table;
>      uint64_t refcount_table_offset;
[Snip...]

Did you create this patch manually, or (partly) mechanically?

Commit message claims a named struct is not needed for "lists of other
kinds, and even for QTAILQ in practice this is only rarely needed".
Let's see:

    $ git-grep 'Q[A-Z]*_HEAD(,' | wc -l
    246
    $ git-grep 'Q[A-Z]*_HEAD([^,]' | grep -v '#define' | wc -l
    25

Yup, "rarely" is fair.

    $ git-grep 'Q[A-Z]*_HEAD([^,]' | grep -v '#define' | grep -v QTAILQ_HEAD
    block/qcow2-bitmap.c:typedef QSIMPLEQ_HEAD(Qcow2BitmapList, Qcow2Bitmap) 
Qcow2BitmapList;
    include/block/block.h:typedef QSIMPLEQ_HEAD(BlockReopenQueue, 
BlockReopenQueueEntry) BlockReopenQueue;
    include/hw/vfio/vfio-common.h:typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) 
VFIOGroupList;
    qemu-bridge-helper.c:typedef QSIMPLEQ_HEAD(ACLList, ACLRule) ACLList;

Are these four names left in intentionally?

If the name is indeed not needed for lists of other kinds, should we
simplify the macros so their users don't have to supply an empty first
argument?