Re: [PATCH 3/3] qapi/monitor: Handle missing vhost-user protocol features

[Markus Armbruster <armbru@redhat.com>]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> Commit f9a8f5812607 moved protocol_features to the vhost-user backend.
> As a result, VhostStatus.protocol_features is only populated for
> vhost-user devices.
>
> For an active kernel vhost device, HMP info virtio-status and QMP
> x-query-virtio-status crashes now because of unset protocol-features.
>
> Make protocol-features optional in VhostStatus and skip dumping a
> missing object in HMP.
>
> For QMP we can change the x-query-virtio-status command semantics
> because it is unstable.

Suggest

  This is a compatibility break.  Okay, since VhostStatus is only used
  by x-query-virtio-status, which is unstable.

> Enable corresponding test, it works now.
>
> Fixes: f9a8f5812607 ("vhost: move protocol_features to vhost_user")
> Co-authored-by: Nikolay Kuratov <kniv@yandex-team.ru>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  hw/virtio/virtio-hmp-cmds.c                   | 6 ++++--
>  qapi/virtio.json                              | 5 +++--
>  tests/functional/x86_64/test_virtio_status.py | 1 -
>  3 files changed, 7 insertions(+), 5 deletions(-)
>

The actual bug is in qmp_x_query_virtio_status(), which can leave
status->vhost_dev->protocol null:

           if (hdev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER) {
               vhost_user_qmp_status(hdev, status);
           }

A non-optional struct pointer must not be null.  I believe the visitor
detects this programming error and crashes.

An optional struct pointer may be null: it means "absent".  Making the
struct optional fixes the crash.

> diff --git a/hw/virtio/virtio-hmp-cmds.c b/hw/virtio/virtio-hmp-cmds.c
> index e5da6f00699..5b08a06ec42 100644
> --- a/hw/virtio/virtio-hmp-cmds.c
> +++ b/hw/virtio/virtio-hmp-cmds.c
> @@ -176,8 +176,10 @@ void hmp_virtio_status(MonitorHMP *hmp, const QDict *qdict)
>          hmp_virtio_dump_features(hmp, s->vhost_dev->features);
>          monitor_hmp_printf(hmp, "    Acked features:\n");
>          hmp_virtio_dump_features(hmp, s->vhost_dev->acked_features);
> -        monitor_hmp_printf(hmp, "    Protocol features:\n");
> -        hmp_virtio_dump_protocols(hmp, s->vhost_dev->protocol_features);
> +        if (s->vhost_dev->protocol_features) {
> +            monitor_hmp_printf(hmp, "    Protocol features:\n");
> +            hmp_virtio_dump_protocols(hmp, s->vhost_dev->protocol_features);
> +        }

This isn't the bug fix, it's the adjustment for optional, as you write
in the commit message.

>      }
>  
>      qapi_free_VirtioStatus(s);
> diff --git a/qapi/virtio.json b/qapi/virtio.json
> index 1fc4e38a44d..f8e699a0e4e 100644
> --- a/qapi/virtio.json
> +++ b/qapi/virtio.json
> @@ -85,7 +85,8 @@
>  #
>  # @acked-features: vhost_dev acked_features
>  #
> -# @protocol-features: vhost_dev protocol_features
> +# @protocol-features: vhost-user protocol features.  Present only for
> +#     vhost-user devices.
>  #
>  # @max-queues: vhost_dev max_queues
>  #
> @@ -104,7 +105,7 @@
>              'vq-index': 'int',
>              'features': 'VirtioDeviceFeatures',
>              'acked-features': 'VirtioDeviceFeatures',
> -            'protocol-features': 'VhostDeviceProtocols',
> +            '*protocol-features': 'VhostDeviceProtocols',
>              'max-queues': 'uint64',
>              'backend-cap': 'uint64',
>              'log-enabled': 'bool',
> diff --git a/tests/functional/x86_64/test_virtio_status.py b/tests/functional/x86_64/test_virtio_status.py
> index 1e72c713cc6..a88823004f8 100755
> --- a/tests/functional/x86_64/test_virtio_status.py
> +++ b/tests/functional/x86_64/test_virtio_status.py
> @@ -45,7 +45,6 @@ def check_commands(self, vm):
>          vm.cmd("x-query-virtio-status", path=self.VIRTIO_PATH)
>  
>      @skipIfMissingCommands("unshare")
> -    @skipIfMissingEnv("QEMU_FAILING_TESTS")
>      def test_vhost(self):
>          self.require_netdev("tap")
>          for device in ("/dev/net/tun", "/dev/vhost-net"):

Reviewed-by: Markus Armbruster <armbru@redhat.com>