| Message ID | 20220525125540.50979-1-changpeng.liu@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] hw/virtio/vhost-user: don't use uninitialized variable | expand |
On Wed, May 25, 2022 at 08:55:39PM +0800, Changpeng Liu wrote: > 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. > > Fixes: 5653493 ("hw/virtio/vhost-user: don't suppress F_CONFIG when supported") > > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com> Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.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); > } > } > -- > 2.21.3 > >
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? > > 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); > } > }
> -----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. > > > > > 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
"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
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); } }
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. 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(-)