Re: [PATCH 1/2] hw/virtio/vhost-user: don't use uninitialized variable

["Alex Bennée" <alex.bennee@linaro.org>]

"Liu, Changpeng" <changpeng.liu@intel.com> writes:

>> -----Original Message-----
>> From: Alex Bennée <alex.bennee@linaro.org>
>> Sent: Tuesday, May 31, 2022 10:46 PM
>> To: Liu, Changpeng <changpeng.liu@intel.com>
>> Cc: qemu-devel@nongnu.org
>> Subject: Re: [PATCH 1/2] hw/virtio/vhost-user: don't use uninitialized variable
>> 
>> 
>> Changpeng Liu <changpeng.liu@intel.com> writes:
>> 
>> > Variable `vdev` in `struct vhost_dev` will not be ready
>> > until start the device, so let's not use it for the error
>> > output here.
>> 
>> This seems to be one of the areas where vhost_user_backend_dev_init and
>> vhost_dev_init do things differently. Is there any particular reason why
>> we couldn't initialise hdev->vdev consistently at init time?
> vhost_dev_init() set hdev->vdev to NULL, and vhost_dev_start() set it to VirtIODevice,
> it's consistent, they are common APIs designed for vhost-kernel and
> vhost-user.

Ahh vhost_user_backend_dev_init actually sets VhostUserBackend->vdev
right before calling vhost_dev_init during it's realize phase.

There is definitely some scope for rationalisation here given we know
the type at realize. Why delay setting it?

It looks like this all came in with c471ad0e9b (vhost_net: device IOTLB
support).

>> 
>> >
>> > Fixes: 5653493 ("hw/virtio/vhost-user: don't suppress F_CONFIG when
>> supported")
>> >
>> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
>> > ---
>> >  hw/virtio/vhost-user.c | 8 +++-----
>> >  1 file changed, 3 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> > index b040c1ad2b..0594178224 100644
>> > --- a/hw/virtio/vhost-user.c
>> > +++ b/hw/virtio/vhost-user.c
>> > @@ -2031,18 +2031,16 @@ static int vhost_user_backend_init(struct
>> vhost_dev *dev, void *opaque,
>> >          if (supports_f_config) {
>> >              if (!virtio_has_feature(protocol_features,
>> >                                      VHOST_USER_PROTOCOL_F_CONFIG)) {
>> > -                error_setg(errp, "vhost-user device %s expecting "
>> > +                error_setg(errp, "vhost-user device expecting "
>> >                             "VHOST_USER_PROTOCOL_F_CONFIG but the vhost-user
>> backend does "
>> > -                           "not support it.", dev->vdev->name);
>> > +                           "not support it.");
>> >                  return -EPROTO;
>> >              }
>> >          } else {
>> >              if (virtio_has_feature(protocol_features,
>> >                                     VHOST_USER_PROTOCOL_F_CONFIG)) {
>> >                  warn_reportf_err(*errp, "vhost-user backend supports "
>> > -                                 "VHOST_USER_PROTOCOL_F_CONFIG for "
>> > -                                 "device %s but QEMU does not.",
>> > -                                 dev->vdev->name);
>> > +                                 "VHOST_USER_PROTOCOL_F_CONFIG but QEMU does not.");
>> >                  protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_CONFIG);
>> >              }
>> >          }
>> 
>> 
>> --
>> Alex Bennée


-- 
Alex Bennée