| Message ID | 20171107030236.23633-4-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/11/2017 04:02, Eric Blake wrote: > We forbid operations like a zero-length write zero or a discard > at the protocol layer when it is marked read-only, but those > same operations were succeeding at the format layer because the > raw format was not reflecting the underlying read-only status > to the block layer, which then took short circuit paths on > zero-length operations. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > block/raw-format.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/block/raw-format.c b/block/raw-format.c > index 830243a8e4..717b8eff65 100644 > --- a/block/raw-format.c > +++ b/block/raw-format.c > @@ -418,6 +418,12 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags, > bs->file->bs->supported_write_flags; > bs->supported_zero_flags = (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) & > bs->file->bs->supported_zero_flags; > + if (bdrv_is_read_only(bs->file->bs)) { > + ret = bdrv_set_read_only(bs, true, errp); > + if (ret < 0) { > + return ret; > + } > + } > > if (bs->probed && !bdrv_is_read_only(bs)) { > fprintf(stderr, > Kevin, perhaps this should be done straight in block.c? Thanks, Paolo
Am 07.11.2017 um 12:00 hat Paolo Bonzini geschrieben: > On 07/11/2017 04:02, Eric Blake wrote: > > We forbid operations like a zero-length write zero or a discard > > at the protocol layer when it is marked read-only, but those > > same operations were succeeding at the format layer because the > > raw format was not reflecting the underlying read-only status > > to the block layer, which then took short circuit paths on > > zero-length operations. > > > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > > block/raw-format.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/block/raw-format.c b/block/raw-format.c > > index 830243a8e4..717b8eff65 100644 > > --- a/block/raw-format.c > > +++ b/block/raw-format.c > > @@ -418,6 +418,12 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags, > > bs->file->bs->supported_write_flags; > > bs->supported_zero_flags = (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) & > > bs->file->bs->supported_zero_flags; > > + if (bdrv_is_read_only(bs->file->bs)) { > > + ret = bdrv_set_read_only(bs, true, errp); > > + if (ret < 0) { > > + return ret; > > + } > > + } > > > > if (bs->probed && !bdrv_is_read_only(bs)) { > > fprintf(stderr, > > > > Kevin, perhaps this should be done straight in block.c? No, I just discussed this with Eric on IRC, and it shouldn't be done anywhere. Basically, the way qemu works with read-only images is that you need to be explicit about it and specify read-only=on. Drivers are not supposed to magically set the read-only flag if the user didn't request it. So what NBD needs to do is to error out if you try to open a read-write connection to a read-only NBD server. There's a second part, related specifically to this patch, that was a bit surprising to me at first, but it actually makes sense. I can successfully create a read-write raw node on top of a read-only file-posix node: -blockdev driver=file,filename=/tmp/test.qcow2,node-name=proto,read-only=on -blockdev driver=raw,file=proto,node-name=format This is because in this state without a user, the raw node doesn't actually want to write to the file-posix node. However, if I add a disk: -device ide-hd,drive=format That will actually request write permissions throughout the whole chain and cause an error: qemu-system-x86_64: -device ide-hd,drive=format: Block node is read-only Somewhat surprising at first, but it does make sense. Kevin
diff --git a/block/raw-format.c b/block/raw-format.c index 830243a8e4..717b8eff65 100644 --- a/block/raw-format.c +++ b/block/raw-format.c @@ -418,6 +418,12 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags, bs->file->bs->supported_write_flags; bs->supported_zero_flags = (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) & bs->file->bs->supported_zero_flags; + if (bdrv_is_read_only(bs->file->bs)) { + ret = bdrv_set_read_only(bs, true, errp); + if (ret < 0) { + return ret; + } + } if (bs->probed && !bdrv_is_read_only(bs)) { fprintf(stderr,
We forbid operations like a zero-length write zero or a discard at the protocol layer when it is marked read-only, but those same operations were succeeding at the format layer because the raw format was not reflecting the underlying read-only status to the block layer, which then took short circuit paths on zero-length operations. Signed-off-by: Eric Blake <eblake@redhat.com> --- block/raw-format.c | 6 ++++++ 1 file changed, 6 insertions(+)