| Message ID | 1311179069-27882-8-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Wed, 20 Jul 2011 18:23:41 +0200 > Markus Armbruster <armbru@redhat.com> wrote: > >> The only caller is bdrv_set_locked(), and it ignores the value. >> >> Callees always return 0, except for FreeBSD's cdrom_set_locked(), >> which returns -ENOTSUP when the device is in a terminally wedged >> state. > > The fact that we're ignoring ioctl() failures caught my attention. Is it > because the state we store in bs->locked is what really matters? > > Otherwise the right thing to do would be to propagate the error and > change callers to handle it. The only caller is bdrv_set_locked(). Which can't handle it, so it would have to pass it on. The purpose of bdrv_set_locked() is to synchronize the physical lock (if any) with the virtual lock. Worst that can happen is the physical lock fails to track the virtual one. bdrv_set_locked()'s callers are device models: ide-cd and scsi-cd. Each one calls it in four 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 exit: can't return failure. * 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. Recommended errors according to MMC-5 are "unit attention errors" (not a good match), "CDB or parameter list validation errors" (worse) and "hardware failures" (no good choices there either). I doubt adding the error handling is worth our while, but feel free to send patches.
On Thu, 21 Jul 2011 17:07:17 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > On Wed, 20 Jul 2011 18:23:41 +0200 > > Markus Armbruster <armbru@redhat.com> wrote: > > > >> The only caller is bdrv_set_locked(), and it ignores the value. > >> > >> Callees always return 0, except for FreeBSD's cdrom_set_locked(), > >> which returns -ENOTSUP when the device is in a terminally wedged > >> state. > > > > The fact that we're ignoring ioctl() failures caught my attention. Is it > > because the state we store in bs->locked is what really matters? > > > > Otherwise the right thing to do would be to propagate the error and > > change callers to handle it. > > The only caller is bdrv_set_locked(). Which can't handle it, so it > would have to pass it on. > > The purpose of bdrv_set_locked() is to synchronize the physical lock (if > any) with the virtual lock. Worst that can happen is the physical lock > fails to track the virtual one. > > bdrv_set_locked()'s callers are device models: ide-cd and scsi-cd. Each > one calls it in four 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 exit: can't return failure. > > * 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. > Recommended errors according to MMC-5 are "unit attention errors" (not > a good match), "CDB or parameter list validation errors" (worse) and > "hardware failures" (no good choices there either). > > I doubt adding the error handling is worth our while, but feel free to > send patches. I can't tell if the problems you mention above (physical lock out of sync with the virtual one and the post migration) will turn out to be real issues. If they do, we'll have to fix it.
Am 21.07.2011 17:07, schrieb Markus Armbruster: > Luiz Capitulino <lcapitulino@redhat.com> writes: > >> On Wed, 20 Jul 2011 18:23:41 +0200 >> Markus Armbruster <armbru@redhat.com> wrote: >> >>> The only caller is bdrv_set_locked(), and it ignores the value. >>> >>> Callees always return 0, except for FreeBSD's cdrom_set_locked(), >>> which returns -ENOTSUP when the device is in a terminally wedged >>> state. >> >> The fact that we're ignoring ioctl() failures caught my attention. Is it >> because the state we store in bs->locked is what really matters? >> >> Otherwise the right thing to do would be to propagate the error and >> change callers to handle it. > > The only caller is bdrv_set_locked(). Which can't handle it, so it > would have to pass it on. > > The purpose of bdrv_set_locked() is to synchronize the physical lock (if > any) with the virtual lock. Worst that can happen is the physical lock > fails to track the virtual one. > > bdrv_set_locked()'s callers are device models: ide-cd and scsi-cd. Each > one calls it in four 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 exit: can't return failure. > > * 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. > Recommended errors according to MMC-5 are "unit attention errors" (not > a good match), "CDB or parameter list validation errors" (worse) and > "hardware failures" (no good choices there either). > > I doubt adding the error handling is worth our while, but feel free to > send patches. I agree with Luiz that it feels a bit strange to remove one of the rare places in qemu that actually return errors, but if you say that the callers can't handle it anyway, well... We can always add it back if we find a device that could make use of it. Kevin
diff --git a/block/raw-posix.c b/block/raw-posix.c index 34b64aa..14f8f62 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -1346,7 +1346,7 @@ static int cdrom_eject(BlockDriverState *bs, int eject_flag) return 0; } -static int cdrom_set_locked(BlockDriverState *bs, int locked) +static void cdrom_set_locked(BlockDriverState *bs, int locked) { BDRVRawState *s = bs->opaque; @@ -1357,8 +1357,6 @@ static int cdrom_set_locked(BlockDriverState *bs, int locked) */ /* perror("CDROM_LOCKDOOR"); */ } - - return 0; } static BlockDriver bdrv_host_cdrom = { @@ -1467,12 +1465,12 @@ static int cdrom_eject(BlockDriverState *bs, int eject_flag) return 0; } -static int cdrom_set_locked(BlockDriverState *bs, int locked) +static void cdrom_set_locked(BlockDriverState *bs, int locked) { BDRVRawState *s = bs->opaque; if (s->fd < 0) - return -ENOTSUP; + return; if (ioctl(s->fd, (locked ? CDIOCPREVENT : CDIOCALLOW)) < 0) { /* * Note: an error can happen if the distribution automatically @@ -1480,8 +1478,6 @@ static int cdrom_set_locked(BlockDriverState *bs, int locked) */ /* perror("CDROM_LOCKDOOR"); */ } - - return 0; } static BlockDriver bdrv_host_cdrom = { diff --git a/block/raw.c b/block/raw.c index b0f72d6..1398a9c 100644 --- a/block/raw.c +++ b/block/raw.c @@ -80,10 +80,9 @@ static int raw_eject(BlockDriverState *bs, int eject_flag) return bdrv_eject(bs->file, eject_flag); } -static int raw_set_locked(BlockDriverState *bs, int locked) +static void raw_set_locked(BlockDriverState *bs, int locked) { bdrv_set_locked(bs->file, locked); - return 0; } static int raw_ioctl(BlockDriverState *bs, unsigned long int req, void *buf) diff --git a/block_int.h b/block_int.h index f5d67c1..69cf222 100644 --- a/block_int.h +++ b/block_int.h @@ -111,7 +111,7 @@ struct BlockDriver { int (*bdrv_is_inserted)(BlockDriverState *bs); int (*bdrv_media_changed)(BlockDriverState *bs); int (*bdrv_eject)(BlockDriverState *bs, int eject_flag); - int (*bdrv_set_locked)(BlockDriverState *bs, int locked); + void (*bdrv_set_locked)(BlockDriverState *bs, int locked); /* to control generic scsi devices */ int (*bdrv_ioctl)(BlockDriverState *bs, unsigned long int req, void *buf);
The only caller is bdrv_set_locked(), and it ignores the value. Callees always return 0, except for FreeBSD's cdrom_set_locked(), which returns -ENOTSUP when the device is in a terminally wedged state. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block/raw-posix.c | 10 +++------- block/raw.c | 3 +-- block_int.h | 2 +- 3 files changed, 5 insertions(+), 10 deletions(-)