Re: [RFC PATCH 04/18] stubs: Add blk_by_qdev_id()

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> blockdev.c uses the blk_by_qdev_id() function, so before we can use the
> file in tools (i.e. outside of the system emulator), we need to add a
> stub for it. The function always returns an error.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  stubs/blk-by-qdev-id.c | 9 +++++++++
>  stubs/Makefile.objs    | 1 +
>  2 files changed, 10 insertions(+)
>  create mode 100644 stubs/blk-by-qdev-id.c
>
> diff --git a/stubs/blk-by-qdev-id.c b/stubs/blk-by-qdev-id.c
> new file mode 100644
> index 0000000000..0b6160fefa
> --- /dev/null
> +++ b/stubs/blk-by-qdev-id.c
> @@ -0,0 +1,9 @@
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "sysemu/block-backend.h"
> +
> +BlockBackend *blk_by_qdev_id(const char *id, Error **errp)
> +{
> +    error_setg(errp, "qdev IDs/QOM paths exist only in the system emulator");
> +    return NULL;
> +}

Is this function meant to be called?  Let me check...  it seems to be
used just for QMP commands that accept either a qdev ID or QOM path (by
convention 'id') or a block backend name (by convention 'device').
Evidence:

   static BlockBackend *qmp_get_blk(const char *blk_name, const char *qdev_id,
                                    Error **errp)
   {
       BlockBackend *blk;

       if (!blk_name == !qdev_id) {
           error_setg(errp, "Need exactly one of 'device' and 'id'");
           return NULL;
       }

       if (qdev_id) {
           blk = blk_by_qdev_id(qdev_id, errp);
       } else {
           blk = blk_by_name(blk_name);
           if (blk == NULL) {
               error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                         "Device '%s' not found", blk_name);
           }
       }

       return blk;
   }

Users:

* eject, blockdev-open-tray, blockdev-change-medium via do_open_tray()

* blockdev-close-tray

* eject, blockdev-remove-medium via blockdev_remove_medium()

* blockdev-insert-medium via blockdev_insert_medium()

  Aside: blockdev_insert_medium() could be inlined.

* blockdev-change-medium

* block_set_io_throttle

* block-latency-histogram-set

The newer ones them don't provide 'device', and pass a null @blk_name to
qmp_get_blk().  Using blk_by_qdev_id() directly would be clearer.

Where 'device' is provided, it's been deprecated since v2.8.0.  Not
documented in qemu-deprecated.texi, though.

As far as I understand the storage daemon's intended purpose, none of
these commands make sense there.  Testing.... aha: it provides them all
anyway.  Is that a good idea?

> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 9f4eb25e02..77fbf72576 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -1,5 +1,6 @@
>  stub-obj-y += arch_type.o
>  stub-obj-y += bdrv-next-monitor-owned.o
> +stub-obj-y += blk-by-qdev-id.o
>  stub-obj-y += blk-commit-all.o
>  stub-obj-y += blockdev-close-all-bdrv-states.o
>  stub-obj-y += clock-warp.o