Re: [PATCH 1/2] lockable: use QLNULL for a null lockable

[Eric Blake]

On 6/3/20 5:49 PM, Joe Slater wrote:
> Allows us to build with -Og and optimizations that
> do not clean up dead-code.
>
> Signed-off-by: Joe Slater <joe.slater@windriver.com>
>
> to be squished
>
> Signed-off-by: Joe Slater <joe.slater@windriver.com>

These last two lines can be elided (looks like a rebase mishap).

> ---
>   block/block-backend.c          | 4 ++--
>   block/block-copy.c             | 2 +-
>   block/mirror.c                 | 5 +++--
>   fsdev/qemu-fsdev-throttle.c    | 6 +++---
>   hw/9pfs/9p.c                   | 2 +-
>   include/qemu/lockable.h        | 3 +++
>   util/qemu-co-shared-resource.c | 2 +-
>   7 files changed, 14 insertions(+), 10 deletions(-)

If you use scripts/git.orderfile, your diff would show with the .h 
changes first, which are arguably the meat of this patch.

> +++ b/block/mirror.c
> @@ -28,6 +28,7 @@
>   #define MAX_IO_BYTES (1 << 20) /* 1 Mb */
>   #define DEFAULT_MIRROR_BUF_SIZE (MAX_IN_FLIGHT * MAX_IO_BYTES)
>   
> +
>   /* The mirroring buffer is a list of granularity-sized chunks.
>    * Free chunks are organized in a list.
>    */

This hunk looks spurious.


> +++ b/include/qemu/lockable.h
> @@ -24,6 +24,9 @@ struct QemuLockable {
>       QemuLockUnlockFunc *unlock;
>   };
>   
> +#define QLNULL ((QemuLockable *)0)

Why not ((QemuLocakable *)NULL) ?
Why no comments why this type-safe NULL is useful?

> +
> +
>   /* This function gives an error if an invalid, non-NULL pointer type is passed
>    * to QEMU_MAKE_LOCKABLE.  For optimized builds, we can rely on dead-code 
> elimination
>    * from the compiler, and give the errors already at link time.
> diff --git a/util/qemu-co-shared-resource.c b/util/qemu-co-shared-resource.c
> index 1c83cd9..7423ea4 100644
> --- a/util/qemu-co-shared-resource.c
> +++ b/util/qemu-co-shared-resource.c
> @@ -64,7 +64,7 @@ void coroutine_fn co_get_from_shres(SharedResource *s, 
> uint64_t n)
>   {
>       assert(n <= s->total);
>       while (!co_try_get_from_shres(s, n)) {
> -        qemu_co_queue_wait(&s->queue, NULL);
> +        qemu_co_queue_wait(&s->queue, QLNULL);

It looks like your macro is added to give the compiler some type-safety, 
but it is not obvious how it matters from just the commit message, 
without also looking at patch 2/2.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org