| Message ID | 1311179069-27882-9-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 20.07.2011 18:23, schrieb Markus Armbruster: > Callees always return 0, except for FreeBSD's cdrom_eject(), which > returns -ENOTSUP when the device is in a terminally wedged state. > > The only caller is bdrv_eject(), and it maps -ENOTSUP to 0 since > commit 4be9762a. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> What about failed ioctls? Currently we only print an error message but still return 0. Is this the right behaviour? Could callers make use of an error return here or would we end up like with bdrv_set_locked() where we can't really communicate the error to the guest? Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 20.07.2011 18:23, schrieb Markus Armbruster: >> Callees always return 0, except for FreeBSD's cdrom_eject(), which >> returns -ENOTSUP when the device is in a terminally wedged state. >> >> The only caller is bdrv_eject(), and it maps -ENOTSUP to 0 since >> commit 4be9762a. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > What about failed ioctls? Currently we only print an error message but > still return 0. Is this the right behaviour? Could callers make use of > an error return here or would we end up like with bdrv_set_locked() > where we can't really communicate the error to the guest? Note that I'm not removing any error handling. I don't add any either. The series is long enough... bdrv_eject()'s purpose is similar to bdrv_set_locked()'s: synchronize the physical tray to the virtual one. Unsurprisingly, our error handling options are also similar. All bdrv_eject() can do with errors from the BlockDriver method is pass them on. At this point in the series, bdrv_eject() still returns an error of its own: -EBUSY when bs->locked. 28/55 moves that check into callers. bdrv_eject()'s callers are device models: ide-cd and scsi-cd. Each one calls it in three places (with all my patches applied): * Device init: can return failure. Failure makes QEMU refuse to start (-device cold plug), or hot plug fail (device_add). * Device post migration: can return failure, makes migration fail. Given the choice, I figure I'd pick an incorrect physical tray lock over a failed migration. * Guest PREVENT ALLOW MEDIUM REMOVAL: can send error reply to guest. Does send the appropriate error when the virtual tray is locked. What sense to send when the ioctl() fails? How can the ioctl() fail? When the tray is locked, but for that to happen some other process must have interfered with the lock (Bad process! No treat for you! Go away!). We could synchronize the lock again right before the eject, but that merely shrinks the race's window. I doubt adding the error handling is worth our while, but feel free to send patches.
diff --git a/block.c b/block.c index fab766a..594bcab 100644 --- a/block.c +++ b/block.c @@ -2768,25 +2768,16 @@ int bdrv_media_changed(BlockDriverState *bs) int bdrv_eject(BlockDriverState *bs, int eject_flag) { BlockDriver *drv = bs->drv; - int ret; if (bs->locked) { return -EBUSY; } - if (!drv || !drv->bdrv_eject) { - ret = -ENOTSUP; - } else { - ret = drv->bdrv_eject(bs, eject_flag); - } - if (ret == -ENOTSUP) { - ret = 0; + if (drv && drv->bdrv_eject) { + drv->bdrv_eject(bs, eject_flag); } - if (ret >= 0) { - bs->tray_open = eject_flag; - } - - return ret; + bs->tray_open = eject_flag; + return 0; } int bdrv_is_locked(BlockDriverState *bs) diff --git a/block/raw-posix.c b/block/raw-posix.c index 14f8f62..e8a2fc2 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -1239,7 +1239,7 @@ static int floppy_media_changed(BlockDriverState *bs) return ret; } -static int floppy_eject(BlockDriverState *bs, int eject_flag) +static void floppy_eject(BlockDriverState *bs, int eject_flag) { BDRVRawState *s = bs->opaque; int fd; @@ -1254,8 +1254,6 @@ static int floppy_eject(BlockDriverState *bs, int eject_flag) perror("FDEJECT"); close(fd); } - - return 0; } static BlockDriver bdrv_host_floppy = { @@ -1331,7 +1329,7 @@ static int cdrom_is_inserted(BlockDriverState *bs) return 0; } -static int cdrom_eject(BlockDriverState *bs, int eject_flag) +static void cdrom_eject(BlockDriverState *bs, int eject_flag) { BDRVRawState *s = bs->opaque; @@ -1342,8 +1340,6 @@ static int cdrom_eject(BlockDriverState *bs, int eject_flag) if (ioctl(s->fd, CDROMCLOSETRAY, NULL) < 0) perror("CDROMEJECT"); } - - return 0; } static void cdrom_set_locked(BlockDriverState *bs, int locked) @@ -1443,12 +1439,12 @@ static int cdrom_is_inserted(BlockDriverState *bs) return raw_getlength(bs) > 0; } -static int cdrom_eject(BlockDriverState *bs, int eject_flag) +static void cdrom_eject(BlockDriverState *bs, int eject_flag) { BDRVRawState *s = bs->opaque; if (s->fd < 0) - return -ENOTSUP; + return; (void) ioctl(s->fd, CDIOCALLOW); @@ -1460,9 +1456,7 @@ static int cdrom_eject(BlockDriverState *bs, int eject_flag) perror("CDIOCCLOSE"); } - if (cdrom_reopen(bs) < 0) - return -ENOTSUP; - return 0; + cdrom_reopen(bs); } static void cdrom_set_locked(BlockDriverState *bs, int locked) diff --git a/block/raw.c b/block/raw.c index 1398a9c..cb6203e 100644 --- a/block/raw.c +++ b/block/raw.c @@ -75,9 +75,9 @@ static int raw_is_inserted(BlockDriverState *bs) return bdrv_is_inserted(bs->file); } -static int raw_eject(BlockDriverState *bs, int eject_flag) +static void raw_eject(BlockDriverState *bs, int eject_flag) { - return bdrv_eject(bs->file, eject_flag); + bdrv_eject(bs->file, eject_flag); } static void raw_set_locked(BlockDriverState *bs, int locked) diff --git a/block_int.h b/block_int.h index 69cf222..2d9edcf 100644 --- a/block_int.h +++ b/block_int.h @@ -110,7 +110,7 @@ struct BlockDriver { /* removable device specific */ int (*bdrv_is_inserted)(BlockDriverState *bs); int (*bdrv_media_changed)(BlockDriverState *bs); - int (*bdrv_eject)(BlockDriverState *bs, int eject_flag); + void (*bdrv_eject)(BlockDriverState *bs, int eject_flag); void (*bdrv_set_locked)(BlockDriverState *bs, int locked); /* to control generic scsi devices */
Callees always return 0, except for FreeBSD's cdrom_eject(), which returns -ENOTSUP when the device is in a terminally wedged state. The only caller is bdrv_eject(), and it maps -ENOTSUP to 0 since commit 4be9762a. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block.c | 17 ++++------------- block/raw-posix.c | 16 +++++----------- block/raw.c | 4 ++-- block_int.h | 2 +- 4 files changed, 12 insertions(+), 27 deletions(-)