| Message ID | 1275497729-13120-8-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 02.06.2010 18:55, schrieb Markus Armbruster: > All drives are still made that way. They get destroyed along with > their device. That's inappropriate for the alternative way to make > blockdevs that will appear later in this series. These won't have a > DriveInfo. > > blockdev_detach() destroys the blockdev only if it has a DriveInfo. > > blockdev_attach() does nothing for now. It'll be fleshed out later. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > blockdev.c | 35 +++++++++++++++++++++++++++++++++++ > blockdev.h | 7 +++++++ > 2 files changed, 42 insertions(+), 0 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index ace74e4..f90d4fc 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1,8 +1,12 @@ > /* > * QEMU host block devices > * > + * Copyright (C) 2010 Red Hat Inc. > * Copyright (c) 2003-2008 Fabrice Bellard > * > + * Authors: > + * Markus Armbruster <armbru@redhat.com>, > + * > * This work is licensed under the terms of the GNU GPL, version 2 or > * later. See the COPYING file in the top-level directory. > */ > @@ -17,6 +21,37 @@ > > static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives); > > +static int blockdev_del_dinfo(BlockDriverState *bs) > +{ > + DriveInfo *dinfo, *next_dinfo; > + int res = 0; > + > + QTAILQ_FOREACH_SAFE(dinfo, &drives, next, next_dinfo) { > + if (dinfo->bdrv == bs) { > + qemu_opts_del(dinfo->opts); > + QTAILQ_REMOVE(&drives, dinfo, next); > + qemu_free(dinfo); > + res = 1; > + } > + } > + > + return res; Can it happen that a BlockDriverState belongs to multiple DriveInfos? If no, why not returning in the loop? Wouldn't need a FOREACH_SAFE then, too. It's not worth respinning because of this one, but there were more comments and I think you'll send a v2 for the actual -blockdev option anyway once we have decided how to do it. I have applied patches 1 to 6 now, and I think I could safely go on until patch 9 if the minor improvements that were mentioned in comments are made. I'd ignore patch 10 to 13 for now. Is this what you would have expected or should I handle anything in a different way? Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 02.06.2010 18:55, schrieb Markus Armbruster: >> All drives are still made that way. They get destroyed along with >> their device. That's inappropriate for the alternative way to make >> blockdevs that will appear later in this series. These won't have a >> DriveInfo. >> >> blockdev_detach() destroys the blockdev only if it has a DriveInfo. >> >> blockdev_attach() does nothing for now. It'll be fleshed out later. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> blockdev.c | 35 +++++++++++++++++++++++++++++++++++ >> blockdev.h | 7 +++++++ >> 2 files changed, 42 insertions(+), 0 deletions(-) >> >> diff --git a/blockdev.c b/blockdev.c >> index ace74e4..f90d4fc 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -1,8 +1,12 @@ >> /* >> * QEMU host block devices >> * >> + * Copyright (C) 2010 Red Hat Inc. >> * Copyright (c) 2003-2008 Fabrice Bellard >> * >> + * Authors: >> + * Markus Armbruster <armbru@redhat.com>, >> + * >> * This work is licensed under the terms of the GNU GPL, version 2 or >> * later. See the COPYING file in the top-level directory. >> */ >> @@ -17,6 +21,37 @@ >> >> static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives); >> >> +static int blockdev_del_dinfo(BlockDriverState *bs) >> +{ >> + DriveInfo *dinfo, *next_dinfo; >> + int res = 0; >> + >> + QTAILQ_FOREACH_SAFE(dinfo, &drives, next, next_dinfo) { >> + if (dinfo->bdrv == bs) { >> + qemu_opts_del(dinfo->opts); >> + QTAILQ_REMOVE(&drives, dinfo, next); >> + qemu_free(dinfo); >> + res = 1; >> + } >> + } >> + >> + return res; > > Can it happen that a BlockDriverState belongs to multiple DriveInfos? If > no, why not returning in the loop? Wouldn't need a FOREACH_SAFE then, too. No, that shouldn't happen. Defensive coding, I don't want to leave dinfos with dangling dinfo->bdrv around. Maybe I should put an assert(!res) before the qemu_opts_del(). Or just forget about it, and simplify like you suggest. > It's not worth respinning because of this one, but there were more > comments and I think you'll send a v2 for the actual -blockdev option > anyway once we have decided how to do it. > > I have applied patches 1 to 6 now, and I think I could safely go on > until patch 9 if the minor improvements that were mentioned in comments > are made. I'd ignore patch 10 to 13 for now. > > Is this what you would have expected or should I handle anything in a > different way? No, that suits me fine. I definitely need to respin from part 8 on (commit message too terse).
diff --git a/blockdev.c b/blockdev.c index ace74e4..f90d4fc 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1,8 +1,12 @@ /* * QEMU host block devices * + * Copyright (C) 2010 Red Hat Inc. * Copyright (c) 2003-2008 Fabrice Bellard * + * Authors: + * Markus Armbruster <armbru@redhat.com>, + * * This work is licensed under the terms of the GNU GPL, version 2 or * later. See the COPYING file in the top-level directory. */ @@ -17,6 +21,37 @@ static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives); +static int blockdev_del_dinfo(BlockDriverState *bs) +{ + DriveInfo *dinfo, *next_dinfo; + int res = 0; + + QTAILQ_FOREACH_SAFE(dinfo, &drives, next, next_dinfo) { + if (dinfo->bdrv == bs) { + qemu_opts_del(dinfo->opts); + QTAILQ_REMOVE(&drives, dinfo, next); + qemu_free(dinfo); + res = 1; + } + } + + return res; +} + +int blockdev_attach(BlockDriverState *bs, DeviceState *qdev) +{ + return 0; +} + +void blockdev_detach(BlockDriverState *bs, DeviceState *qdev) +{ + /* Backward compatibility: auto-destroy block device made with + * drive_init() when its guest device detaches */ + if (blockdev_del_dinfo(bs)) { + bdrv_delete(bs); + } +} + QemuOpts *drive_add(const char *file, const char *fmt, ...) { va_list ap; diff --git a/blockdev.h b/blockdev.h index 23ea576..8607086 100644 --- a/blockdev.h +++ b/blockdev.h @@ -1,8 +1,12 @@ /* * QEMU host block devices * + * Copyright (C) 2010 Red Hat Inc. * Copyright (c) 2003-2008 Fabrice Bellard * + * Authors: + * Markus Armbruster <armbru@redhat.com>, + * * This work is licensed under the terms of the GNU GPL, version 2 or * later. See the COPYING file in the top-level directory. */ @@ -13,6 +17,9 @@ #include "block.h" #include "qemu-queue.h" +int blockdev_attach(BlockDriverState *, DeviceState *); +void blockdev_detach(BlockDriverState *, DeviceState *); + typedef enum { IF_NONE, IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
All drives are still made that way. They get destroyed along with their device. That's inappropriate for the alternative way to make blockdevs that will appear later in this series. These won't have a DriveInfo. blockdev_detach() destroys the blockdev only if it has a DriveInfo. blockdev_attach() does nothing for now. It'll be fleshed out later. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- blockdev.c | 35 +++++++++++++++++++++++++++++++++++ blockdev.h | 7 +++++++ 2 files changed, 42 insertions(+), 0 deletions(-)