| Message ID | 20200813164637.58904-1-liq3ea@163.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw: virtio-mem: detach the element fromt the virtqueue when error occurs | expand |
On 13.08.20 18:46, Li Qiang wrote: For now we use "virtio-mem:" for the subject, without the "hw: "part. > If error occurs while processing the virtio request we should call > 'virtqueue_detach_element' to detach the element from the virtqueue > before free the elem. What's the effect of this? In all cases we trigger a virtio_error(), so do we really have to bother? > > Signed-off-by: Li Qiang <liq3ea@163.com> > --- > hw/virtio/virtio-mem.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c > index 7740fc613f..5ac6c3ec67 100644 > --- a/hw/virtio/virtio-mem.c > +++ b/hw/virtio/virtio-mem.c > @@ -318,8 +318,7 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, VirtQueue *vq) > if (iov_to_buf(elem->out_sg, elem->out_num, 0, &req, len) < len) { > virtio_error(vdev, "virtio-mem protocol violation: invalid request" > " size: %d", len); > - g_free(elem); > - return; > + goto out_free; > } > > if (iov_size(elem->in_sg, elem->in_num) < > @@ -327,8 +326,7 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, VirtQueue *vq) > virtio_error(vdev, "virtio-mem protocol violation: not enough space" > " for response: %zu", > iov_size(elem->in_sg, elem->in_num)); > - g_free(elem); > - return; > + goto out_free; > } > > type = le16_to_cpu(req.type); > @@ -348,12 +346,15 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, VirtQueue *vq) > default: > virtio_error(vdev, "virtio-mem protocol violation: unknown request" > " type: %d", type); > - g_free(elem); > - return; > + goto out_free; > } > > g_free(elem); > } > + > +out_free: > + virtqueue_detach_element(vq, elem, 0); > + g_free(elem); > } > > static void virtio_mem_get_config(VirtIODevice *vdev, uint8_t *config_data) >
David Hildenbrand <david@redhat.com> 于2020年8月14日周五 上午1:15写道： > > On 13.08.20 18:46, Li Qiang wrote: > > For now we use "virtio-mem:" for the subject, without the "hw: "part. > > > If error occurs while processing the virtio request we should call > > 'virtqueue_detach_element' to detach the element from the virtqueue > > before free the elem. > > What's the effect of this? In all cases we trigger a virtio_error(), so > do we really have to bother? > Though the 'in_use' will be reset to 0 while reseting the virtio device. The mapped sglist will not be unammped. There maybe some undesired behavior. CC Paolo to make a confirmation. Thanks, Li Qiang > > > > Signed-off-by: Li Qiang <liq3ea@163.com> > > --- > > hw/virtio/virtio-mem.c | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c > > index 7740fc613f..5ac6c3ec67 100644 > > --- a/hw/virtio/virtio-mem.c > > +++ b/hw/virtio/virtio-mem.c > > @@ -318,8 +318,7 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, VirtQueue *vq) > > if (iov_to_buf(elem->out_sg, elem->out_num, 0, &req, len) < len) { > > virtio_error(vdev, "virtio-mem protocol violation: invalid request" > > " size: %d", len); > > - g_free(elem); > > - return; > > + goto out_free; > > } > > > > if (iov_size(elem->in_sg, elem->in_num) < > > @@ -327,8 +326,7 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, VirtQueue *vq) > > virtio_error(vdev, "virtio-mem protocol violation: not enough space" > > " for response: %zu", > > iov_size(elem->in_sg, elem->in_num)); > > - g_free(elem); > > - return; > > + goto out_free; > > } > > > > type = le16_to_cpu(req.type); > > @@ -348,12 +346,15 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, VirtQueue *vq) > > default: > > virtio_error(vdev, "virtio-mem protocol violation: unknown request" > > " type: %d", type); > > - g_free(elem); > > - return; > > + goto out_free; > > } > > > > g_free(elem); > > } > > + > > +out_free: > > + virtqueue_detach_element(vq, elem, 0); > > + g_free(elem); > > } > > > > static void virtio_mem_get_config(VirtIODevice *vdev, uint8_t *config_data) > > > > > -- > Thanks, > > David / dhildenb >
On 14.08.20 03:01, Li Qiang wrote: > David Hildenbrand <david@redhat.com> 于2020年8月14日周五 上午1:15写道： >> >> On 13.08.20 18:46, Li Qiang wrote: >> >> For now we use "virtio-mem:" for the subject, without the "hw: "part. >> >>> If error occurs while processing the virtio request we should call >>> 'virtqueue_detach_element' to detach the element from the virtqueue >>> before free the elem. >> >> What's the effect of this? In all cases we trigger a virtio_error(), so >> do we really have to bother? >> > > Though the 'in_use' will be reset to 0 while reseting the virtio device. > The mapped sglist will not be unammped. > There maybe some undesired behavior. CC Paolo to make a confirmation. Looking at hw/virtio/virtio-crypto.c, this seems to be the right thing to do. Can you please respin, avoiding adding the label, only inserting the 3 separate virtqueue_detach_element() calls? Thanks!
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c index 7740fc613f..5ac6c3ec67 100644 --- a/hw/virtio/virtio-mem.c +++ b/hw/virtio/virtio-mem.c @@ -318,8 +318,7 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, VirtQueue *vq) if (iov_to_buf(elem->out_sg, elem->out_num, 0, &req, len) < len) { virtio_error(vdev, "virtio-mem protocol violation: invalid request" " size: %d", len); - g_free(elem); - return; + goto out_free; } if (iov_size(elem->in_sg, elem->in_num) < @@ -327,8 +326,7 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, VirtQueue *vq) virtio_error(vdev, "virtio-mem protocol violation: not enough space" " for response: %zu", iov_size(elem->in_sg, elem->in_num)); - g_free(elem); - return; + goto out_free; } type = le16_to_cpu(req.type); @@ -348,12 +346,15 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, VirtQueue *vq) default: virtio_error(vdev, "virtio-mem protocol violation: unknown request" " type: %d", type); - g_free(elem); - return; + goto out_free; } g_free(elem); } + +out_free: + virtqueue_detach_element(vq, elem, 0); + g_free(elem); } static void virtio_mem_get_config(VirtIODevice *vdev, uint8_t *config_data)
If error occurs while processing the virtio request we should call 'virtqueue_detach_element' to detach the element from the virtqueue before free the elem. Signed-off-by: Li Qiang <liq3ea@163.com> --- hw/virtio/virtio-mem.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)