| Message ID | 1279642177-21229-1-git-send-email-Jes.Sorensen@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/20/10 10:09, Jes.Sorensen@redhat.com wrote: > From: Jes Sorensen <Jes.Sorensen@redhat.com> > > O_DIRECT (cache=none) requires sector alignment, however the physical > sector size of CDROM/DVD drives is 2048, as opposed to most disk > devices which use 512. QEMU is hard coding 512 all over the place, so > allowing O_DIRECT for CDROM/DVD devices does not work. Exit with an > error! > > Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com> > --- > block/raw-posix.c | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 291699f..1b840c4 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -1139,6 +1139,11 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags) > BDRVRawState *s = bs->opaque; > > s->type = FTYPE_CD; > + if (flags & BDRV_O_NOCACHE) { > + fprintf(stderr, "O_DIRECT (cache=none) for CDROM/DVD device (%s) " > + "is unsupported\n", filename); > + exit(1); > + } If a cdrom is added via the monitor this would abruptly terminate the VM - which is not good. David > > /* open will not fail even if no CD is inserted, so add O_NONBLOCK */ > return raw_open_common(bs, filename, flags, O_NONBLOCK);
On 07/20/10 18:35, David S. Ahern wrote: > On 07/20/10 10:09, Jes.Sorensen@redhat.com wrote: >> diff --git a/block/raw-posix.c b/block/raw-posix.c >> index 291699f..1b840c4 100644 >> --- a/block/raw-posix.c >> +++ b/block/raw-posix.c >> @@ -1139,6 +1139,11 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags) >> BDRVRawState *s = bs->opaque; >> >> s->type = FTYPE_CD; >> + if (flags & BDRV_O_NOCACHE) { >> + fprintf(stderr, "O_DIRECT (cache=none) for CDROM/DVD device (%s) " >> + "is unsupported\n", filename); >> + exit(1); >> + } > > If a cdrom is added via the monitor this would abruptly terminate the VM > - which is not good. True, but this would only happen if the user explicitly marks it cache=none which is not the default value for a new drive, so it would be a corner case. Cheers, Jes
Jes Sorensen <Jes.Sorensen@redhat.com> writes: > On 07/20/10 18:35, David S. Ahern wrote: >> On 07/20/10 10:09, Jes.Sorensen@redhat.com wrote: >>> diff --git a/block/raw-posix.c b/block/raw-posix.c >>> index 291699f..1b840c4 100644 >>> --- a/block/raw-posix.c >>> +++ b/block/raw-posix.c >>> @@ -1139,6 +1139,11 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags) >>> BDRVRawState *s = bs->opaque; >>> >>> s->type = FTYPE_CD; >>> + if (flags & BDRV_O_NOCACHE) { >>> + fprintf(stderr, "O_DIRECT (cache=none) for CDROM/DVD device (%s) " >>> + "is unsupported\n", filename); >>> + exit(1); >>> + } >> >> If a cdrom is added via the monitor this would abruptly terminate the VM >> - which is not good. > > True, but this would only happen if the user explicitly marks it > cache=none which is not the default value for a new drive, so it would > be a corner case. Why not return -ENOTSUP?
On 07/20/10 19:19, Markus Armbruster wrote: > Jes Sorensen <Jes.Sorensen@redhat.com> writes: > >> On 07/20/10 18:35, David S. Ahern wrote: >>> On 07/20/10 10:09, Jes.Sorensen@redhat.com wrote: >>> If a cdrom is added via the monitor this would abruptly terminate the VM >>> - which is not good. >> >> True, but this would only happen if the user explicitly marks it >> cache=none which is not the default value for a new drive, so it would >> be a corner case. > > Why not return -ENOTSUP? Good point, just posted v3 of the patch that does this instead of exit() Jes
diff --git a/block/raw-posix.c b/block/raw-posix.c index 291699f..1b840c4 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -1139,6 +1139,11 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags) BDRVRawState *s = bs->opaque; s->type = FTYPE_CD; + if (flags & BDRV_O_NOCACHE) { + fprintf(stderr, "O_DIRECT (cache=none) for CDROM/DVD device (%s) " + "is unsupported\n", filename); + exit(1); + } /* open will not fail even if no CD is inserted, so add O_NONBLOCK */ return raw_open_common(bs, filename, flags, O_NONBLOCK);