Message ID | 1971390dc6d0ae9014e796f8ee444dee4a90815a.1259754427.git.quintela@redhat.com |
---|---|
State | New |
Headers | show |
On Wed, Dec 02, 2009 at 01:04:36PM +0100, Juan Quintela wrote: > viltio_blak_dma_restart_bh() was unsafe, it used req->next after having > (possible) put req in another list > > Signed-off-by: Juan Quintela <quintela@redhat.com> Sounds good, but why is this part of vmstate patchset? > --- > hw/virtio-blk.c | 29 ++++++++++++++--------------- > 1 files changed, 14 insertions(+), 15 deletions(-) > > diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c > index b716a36..838ec32 100644 > --- a/hw/virtio-blk.c > +++ b/hw/virtio-blk.c > @@ -26,7 +26,7 @@ typedef struct VirtIOBlock > VirtIODevice vdev; > BlockDriverState *bs; > VirtQueue *vq; > - VirtIOBlockReq *rq; > + QLIST_HEAD (,VirtIOBlockReq) rq; > char serial_str[BLOCK_SERIAL_STRLEN + 1]; > QEMUBH *bh; > size_t config_size; > @@ -81,7 +81,7 @@ struct VirtIOBlockReq > struct virtio_blk_outhdr *out; > struct virtio_scsi_inhdr *scsi; > QEMUIOVector qiov; > - struct VirtIOBlockReq *next; > + QLIST_ENTRY(VirtIOBlockReq) next; > }; > > static void virtio_blk_req_complete(VirtIOBlockReq *req, int status) > @@ -105,8 +105,7 @@ static int virtio_blk_handle_write_error(VirtIOBlockReq *req, int error) > > if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC) > || action == BLOCK_ERR_STOP_ANY) { > - req->next = s->rq; > - s->rq = req; > + QLIST_INSERT_HEAD(&s->rq, req, next); > vm_stop(0); > } else { > virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR); > @@ -366,17 +365,19 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq) > static void virtio_blk_dma_restart_bh(void *opaque) > { > VirtIOBlock *s = opaque; > - VirtIOBlockReq *req = s->rq; > + QLIST_HEAD (, VirtIOBlockReq) rq_copy; > + VirtIOBlockReq *req, *next_req; > > qemu_bh_delete(s->bh); > s->bh = NULL; > > - s->rq = NULL; > + QLIST_COPY_HEAD(&rq_copy, &s->rq); > + QLIST_INIT(&s->rq); > > - while (req) { > + QLIST_FOREACH_SAFE(req, &rq_copy, next, next_req) { > + QLIST_REMOVE(req, next); > bdrv_aio_writev(req->dev->bs, req->out->sector, &req->qiov, > req->qiov.size / 512, virtio_blk_rw_complete, req); > - req = req->next; > } > } > > @@ -451,14 +452,13 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev) > static void virtio_blk_save(QEMUFile *f, void *opaque) > { > VirtIOBlock *s = opaque; > - VirtIOBlockReq *req = s->rq; > + VirtIOBlockReq *req;; > > virtio_save(&s->vdev, f); > - > - while (req) { > + > + QLIST_FOREACH(req, &s->rq, next) { > qemu_put_sbyte(f, 1); > qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem)); > - req = req->next; > } > qemu_put_sbyte(f, 0); > } > @@ -474,8 +474,7 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id) > while (qemu_get_sbyte(f)) { > VirtIOBlockReq *req = virtio_blk_alloc_request(s); > qemu_get_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem)); > - req->next = s->rq; > - s->rq = req->next; > + QLIST_INSERT_HEAD(&s->rq, req, next); > } > > return 0; > @@ -499,7 +498,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo) > s->vdev.get_features = virtio_blk_get_features; > s->vdev.reset = virtio_blk_reset; > s->bs = dinfo->bdrv; > - s->rq = NULL; > + QLIST_INIT(&s->rq); > if (strlen(ps)) > strncpy(s->serial_str, ps, sizeof(s->serial_str)); > else > -- > 1.6.5.2
"Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Dec 02, 2009 at 01:04:36PM +0100, Juan Quintela wrote: >> viltio_blak_dma_restart_bh() was unsafe, it used req->next after having >> (possible) put req in another list >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> > > Sounds good, but why is this part of vmstate patchset? Rest of the patches depend on it. I tried to only do the cleanups needed for vmstate. I really forced me to not doing more cleanups. Later, Juan.
On Wed, Dec 02, 2009 at 07:56:58PM +0100, Juan Quintela wrote: > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > On Wed, Dec 02, 2009 at 01:04:36PM +0100, Juan Quintela wrote: > >> viltio_blak_dma_restart_bh() was unsafe, it used req->next after having > >> (possible) put req in another list > >> > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > > > > Sounds good, but why is this part of vmstate patchset? > > Rest of the patches depend on it. I tried to only do the cleanups needed > for vmstate. I really forced me to not doing more cleanups. > > Later, Juan. But this is bugfix, not cleanup, right? We want bugfixes ASAP, let not mix it with cleanups etc. You can just post 2 series, and note that second one depends on 1'st one.
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index b716a36..838ec32 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -26,7 +26,7 @@ typedef struct VirtIOBlock VirtIODevice vdev; BlockDriverState *bs; VirtQueue *vq; - VirtIOBlockReq *rq; + QLIST_HEAD (,VirtIOBlockReq) rq; char serial_str[BLOCK_SERIAL_STRLEN + 1]; QEMUBH *bh; size_t config_size; @@ -81,7 +81,7 @@ struct VirtIOBlockReq struct virtio_blk_outhdr *out; struct virtio_scsi_inhdr *scsi; QEMUIOVector qiov; - struct VirtIOBlockReq *next; + QLIST_ENTRY(VirtIOBlockReq) next; }; static void virtio_blk_req_complete(VirtIOBlockReq *req, int status) @@ -105,8 +105,7 @@ static int virtio_blk_handle_write_error(VirtIOBlockReq *req, int error) if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC) || action == BLOCK_ERR_STOP_ANY) { - req->next = s->rq; - s->rq = req; + QLIST_INSERT_HEAD(&s->rq, req, next); vm_stop(0); } else { virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR); @@ -366,17 +365,19 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq) static void virtio_blk_dma_restart_bh(void *opaque) { VirtIOBlock *s = opaque; - VirtIOBlockReq *req = s->rq; + QLIST_HEAD (, VirtIOBlockReq) rq_copy; + VirtIOBlockReq *req, *next_req; qemu_bh_delete(s->bh); s->bh = NULL; - s->rq = NULL; + QLIST_COPY_HEAD(&rq_copy, &s->rq); + QLIST_INIT(&s->rq); - while (req) { + QLIST_FOREACH_SAFE(req, &rq_copy, next, next_req) { + QLIST_REMOVE(req, next); bdrv_aio_writev(req->dev->bs, req->out->sector, &req->qiov, req->qiov.size / 512, virtio_blk_rw_complete, req); - req = req->next; } } @@ -451,14 +452,13 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev) static void virtio_blk_save(QEMUFile *f, void *opaque) { VirtIOBlock *s = opaque; - VirtIOBlockReq *req = s->rq; + VirtIOBlockReq *req;; virtio_save(&s->vdev, f); - - while (req) { + + QLIST_FOREACH(req, &s->rq, next) { qemu_put_sbyte(f, 1); qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem)); - req = req->next; } qemu_put_sbyte(f, 0); } @@ -474,8 +474,7 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id) while (qemu_get_sbyte(f)) { VirtIOBlockReq *req = virtio_blk_alloc_request(s); qemu_get_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem)); - req->next = s->rq; - s->rq = req->next; + QLIST_INSERT_HEAD(&s->rq, req, next); } return 0; @@ -499,7 +498,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo) s->vdev.get_features = virtio_blk_get_features; s->vdev.reset = virtio_blk_reset; s->bs = dinfo->bdrv; - s->rq = NULL; + QLIST_INIT(&s->rq); if (strlen(ps)) strncpy(s->serial_str, ps, sizeof(s->serial_str)); else
viltio_blak_dma_restart_bh() was unsafe, it used req->next after having (possible) put req in another list Signed-off-by: Juan Quintela <quintela@redhat.com> --- hw/virtio-blk.c | 29 ++++++++++++++--------------- 1 files changed, 14 insertions(+), 15 deletions(-)