* [PATCH 1/2] hw/virtio/vhost-user: don't use uninitialized variable
@ 2022-05-25 12:55 Changpeng Liu
2022-05-25 12:55 ` [PATCH 2/2] hw/vhost-user-scsi|blk: set `supports_config` flag correctly Changpeng Liu
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Changpeng Liu @ 2022-05-25 12:55 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.bennee, changpeng.liu
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(-)
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 2/2] hw/vhost-user-scsi|blk: set `supports_config` flag correctly
2022-05-25 12:55 [PATCH 1/2] hw/virtio/vhost-user: don't use uninitialized variable Changpeng Liu
@ 2022-05-25 12:55 ` Changpeng Liu
2022-05-31 5:40 ` Raphael Norwitz
2022-05-31 5:21 ` [PATCH 1/2] hw/virtio/vhost-user: don't use uninitialized variable Raphael Norwitz
2022-05-31 14:45 ` Alex Bennée
2 siblings, 1 reply; 7+ messages in thread
From: Changpeng Liu @ 2022-05-25 12:55 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.bennee, changpeng.liu
Currently vhost-user-scsi driver doesn't allow to change
the configuration space of virtio_scsi, while vhost-user-blk
support that, so here we set the flag in vhost-user-blk driver
and unset it in vhost-user-scsi.
Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
---
hw/block/vhost-user-blk.c | 1 +
hw/scsi/vhost-user-scsi.c | 1 -
2 files changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 5dca4eab09..9117222456 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -337,6 +337,7 @@ static int vhost_user_blk_connect(DeviceState *dev, Error **errp)
vhost_dev_set_config_notifier(&s->dev, &blk_ops);
+ s->vhost_user.supports_config = true;
ret = vhost_dev_init(&s->dev, &s->vhost_user, VHOST_BACKEND_TYPE_USER, 0,
errp);
if (ret < 0) {
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index 9be21d07ee..1b2f7eed98 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -121,7 +121,6 @@ static void vhost_user_scsi_realize(DeviceState *dev, Error **errp)
vsc->dev.backend_features = 0;
vqs = vsc->dev.vqs;
- s->vhost_user.supports_config = true;
ret = vhost_dev_init(&vsc->dev, &s->vhost_user,
VHOST_BACKEND_TYPE_USER, 0, errp);
if (ret < 0) {
--
2.21.3
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] hw/vhost-user-scsi|blk: set `supports_config` flag correctly
2022-05-25 12:55 ` [PATCH 2/2] hw/vhost-user-scsi|blk: set `supports_config` flag correctly Changpeng Liu
@ 2022-05-31 5:40 ` Raphael Norwitz
0 siblings, 0 replies; 7+ messages in thread
From: Raphael Norwitz @ 2022-05-31 5:40 UTC (permalink / raw)
To: Changpeng Liu; +Cc: qemu-devel@nongnu.org, alex.bennee@linaro.org
On Wed, May 25, 2022 at 08:55:40PM +0800, Changpeng Liu wrote:
> Currently vhost-user-scsi driver doesn't allow to change
> the configuration space of virtio_scsi, while vhost-user-blk
> support that, so here we set the flag in vhost-user-blk driver
> and unset it in vhost-user-scsi.
>
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> ---
> hw/block/vhost-user-blk.c | 1 +
> hw/scsi/vhost-user-scsi.c | 1 -
> 2 files changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 5dca4eab09..9117222456 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -337,6 +337,7 @@ static int vhost_user_blk_connect(DeviceState *dev, Error **errp)
>
> vhost_dev_set_config_notifier(&s->dev, &blk_ops);
>
> + s->vhost_user.supports_config = true;
> ret = vhost_dev_init(&s->dev, &s->vhost_user, VHOST_BACKEND_TYPE_USER, 0,
> errp);
> if (ret < 0) {
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index 9be21d07ee..1b2f7eed98 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -121,7 +121,6 @@ static void vhost_user_scsi_realize(DeviceState *dev, Error **errp)
> vsc->dev.backend_features = 0;
> vqs = vsc->dev.vqs;
>
> - s->vhost_user.supports_config = true;
> ret = vhost_dev_init(&vsc->dev, &s->vhost_user,
> VHOST_BACKEND_TYPE_USER, 0, errp);
> if (ret < 0) {
> --
> 2.21.3
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] hw/virtio/vhost-user: don't use uninitialized variable
2022-05-25 12:55 [PATCH 1/2] hw/virtio/vhost-user: don't use uninitialized variable Changpeng Liu
2022-05-25 12:55 ` [PATCH 2/2] hw/vhost-user-scsi|blk: set `supports_config` flag correctly Changpeng Liu
@ 2022-05-31 5:21 ` Raphael Norwitz
2022-05-31 14:45 ` Alex Bennée
2 siblings, 0 replies; 7+ messages in thread
From: Raphael Norwitz @ 2022-05-31 5:21 UTC (permalink / raw)
To: Changpeng Liu; +Cc: qemu-devel@nongnu.org, alex.bennee@linaro.org
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
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] hw/virtio/vhost-user: don't use uninitialized variable
2022-05-25 12:55 [PATCH 1/2] hw/virtio/vhost-user: don't use uninitialized variable Changpeng Liu
2022-05-25 12:55 ` [PATCH 2/2] hw/vhost-user-scsi|blk: set `supports_config` flag correctly Changpeng Liu
2022-05-31 5:21 ` [PATCH 1/2] hw/virtio/vhost-user: don't use uninitialized variable Raphael Norwitz
@ 2022-05-31 14:45 ` Alex Bennée
2022-06-01 0:58 ` Liu, Changpeng
2 siblings, 1 reply; 7+ messages in thread
From: Alex Bennée @ 2022-05-31 14:45 UTC (permalink / raw)
To: Changpeng Liu; +Cc: qemu-devel
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);
> }
> }
--
Alex Bennée
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [PATCH 1/2] hw/virtio/vhost-user: don't use uninitialized variable
2022-05-31 14:45 ` Alex Bennée
@ 2022-06-01 0:58 ` Liu, Changpeng
2022-06-02 8:27 ` Alex Bennée
0 siblings, 1 reply; 7+ messages in thread
From: Liu, Changpeng @ 2022-06-01 0:58 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel@nongnu.org
> -----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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] hw/virtio/vhost-user: don't use uninitialized variable
2022-06-01 0:58 ` Liu, Changpeng
@ 2022-06-02 8:27 ` Alex Bennée
0 siblings, 0 replies; 7+ messages in thread
From: Alex Bennée @ 2022-06-02 8:27 UTC (permalink / raw)
To: Liu, Changpeng; +Cc: qemu-devel@nongnu.org, Jason Wang, Michael S. Tsirkin
"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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-06-02 8:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-05-25 12:55 [PATCH 1/2] hw/virtio/vhost-user: don't use uninitialized variable Changpeng Liu
2022-05-25 12:55 ` [PATCH 2/2] hw/vhost-user-scsi|blk: set `supports_config` flag correctly Changpeng Liu
2022-05-31 5:40 ` Raphael Norwitz
2022-05-31 5:21 ` [PATCH 1/2] hw/virtio/vhost-user: don't use uninitialized variable Raphael Norwitz
2022-05-31 14:45 ` Alex Bennée
2022-06-01 0:58 ` Liu, Changpeng
2022-06-02 8:27 ` Alex Bennée
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).