Hi Michael,
On Tue, Aug 20, 2024 at 06:55:29AM -0400, Michael S. Tsirkin wrote:
[snip]
> > diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> > index 64b96b226c39..8a1e16fce3de 100644
> > --- a/hw/virtio/vdpa-dev.c
> > +++ b/hw/virtio/vdpa-dev.c
> > @@ -63,19 +63,19 @@ static void vhost_vdpa_device_realize(DeviceState *dev,
> > Error **errp)
> > }
> >
> > v->vhostfd = qemu_open(v->vhostdev, O_RDWR, errp);
> > - if (*errp) {
> > + if (v->vhostfd < 0) {
> > return;
> > }
> >
> > v->vdev_id = vhost_vdpa_device_get_u32(v->vhostfd,
> > VHOST_VDPA_GET_DEVICE_ID, errp);
> > - if (*errp) {
> > + if (v->vdev_id < 0) {
> > goto out;
> > }
>
> vdev_id is unsigned, no idea how is this supposed to work.
>
> >
> > max_queue_size = vhost_vdpa_device_get_u32(v->vhostfd,
> > VHOST_VDPA_GET_VRING_NUM,
> > errp);
> > - if (*errp) {
> > + if (max_queue_size < 0) {
> > goto out;
> > }
> >
> max_queue_size is unsigned, too.
>
> > @@ -89,7 +89,7 @@ static void vhost_vdpa_device_realize(DeviceState *dev,
> > Error **errp)
> >
> > v->num_queues = vhost_vdpa_device_get_u32(v->vhostfd,
> > VHOST_VDPA_GET_VQS_COUNT,
> > errp);
> > - if (*errp) {
> > + if (v->num_queues < 0) {
> > goto out;
> > }
> >
>
> num_queues is unsigned, too.
Oops, yes. The correct way is to check whether vhost_vdpa_device_get_u32
returns "(uint32_t)-1".
I can add a new macro like this:
#define VDPA_DEVICE_U32_VALUE_NONE ((uint32_t)-1)
Is this okay with you?
Thanks,
Zhao
> > @@ -127,7 +127,7 @@ static void vhost_vdpa_device_realize(DeviceState *dev,
> > Error **errp)
> > v->config_size = vhost_vdpa_device_get_u32(v->vhostfd,
> > VHOST_VDPA_GET_CONFIG_SIZE,
> > errp);
> > - if (*errp) {
> > + if (v->config_size < 0) {
> > goto vhost_cleanup;
> > }
> >
> > --
> > 2.34.1
>
