| Message ID | 2fb3ea05d2e1ee06d578a0f2c53f56df1661401c.1389726691.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01/14/2014 12:12 PM, Jeff Cody wrote: > Having both read-only=on and snapshot=on together does not make sense; > currently, the read-only argument is effectively ignored for the > temporary snapshot. To prevent confusion, disallow the usage of both > 'snapshot=on' and 'read-only=on'. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > blockdev.c | 7 +++++++ > 1 file changed, 7 insertions(+) Reviewed-by: Eric Blake <eblake@redhat.com> No impact to libvirt, which (intentionally) doesn't use snapshot=on.
On Tue, Jan 14, 2014 at 02:12:19PM -0500, Jeff Cody wrote: > Having both read-only=on and snapshot=on together does not make sense; > currently, the read-only argument is effectively ignored for the > temporary snapshot. To prevent confusion, disallow the usage of both > 'snapshot=on' and 'read-only=on'. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > blockdev.c | 7 +++++++ > 1 file changed, 7 insertions(+) Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan
Am 14.01.2014 um 20:12 hat Jeff Cody geschrieben: > Having both read-only=on and snapshot=on together does not make sense; > currently, the read-only argument is effectively ignored for the > temporary snapshot. To prevent confusion, disallow the usage of both > 'snapshot=on' and 'read-only=on'. > > Signed-off-by: Jeff Cody <jcody@redhat.com> I believe the reason why this was allowed was so that you can use a read-only file with -snapshot. It might not be necessary any more since I switched -snapshot implementation to modify the options QDict instead of manually doing a second bdrv_open(). Did you test that this still works now? The other question is about this code in bdrv_open_flags(): /* * Snapshots should be writable. */ if (bs->is_temporary) { open_flags |= BDRV_O_RDWR; } Is this dead code now because the flag is always already set? Kevin
On Thu, Jan 16, 2014 at 10:39:30AM +0100, Kevin Wolf wrote: > Am 14.01.2014 um 20:12 hat Jeff Cody geschrieben: > > Having both read-only=on and snapshot=on together does not make sense; > > currently, the read-only argument is effectively ignored for the > > temporary snapshot. To prevent confusion, disallow the usage of both > > 'snapshot=on' and 'read-only=on'. > > > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > I believe the reason why this was allowed was so that you can use a > read-only file with -snapshot. It might not be necessary any more since > I switched -snapshot implementation to modify the options QDict instead > of manually doing a second bdrv_open(). > > Did you test that this still works now? > Yes, that still works. > The other question is about this code in bdrv_open_flags(): > > /* > * Snapshots should be writable. > */ > if (bs->is_temporary) { > open_flags |= BDRV_O_RDWR; > } > > Is this dead code now because the flag is always already set? > Yes, that ends up being dead code. From later in blockdev_init(): bdrv_flags |= ro ? 0 : BDRV_O_RDWR; QINCREF(bs_opts); ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error); And ro is set from the read-only QemuOpts, that we check in this patch in conjunction with snapshot. So if read-only=on is not specified, it is opened r/w by default.
Am 16.01.2014 um 20:20 hat Jeff Cody geschrieben: > On Thu, Jan 16, 2014 at 10:39:30AM +0100, Kevin Wolf wrote: > > Am 14.01.2014 um 20:12 hat Jeff Cody geschrieben: > > > Having both read-only=on and snapshot=on together does not make sense; > > > currently, the read-only argument is effectively ignored for the > > > temporary snapshot. To prevent confusion, disallow the usage of both > > > 'snapshot=on' and 'read-only=on'. > > > > > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > > > I believe the reason why this was allowed was so that you can use a > > read-only file with -snapshot. It might not be necessary any more since > > I switched -snapshot implementation to modify the options QDict instead > > of manually doing a second bdrv_open(). > > > > Did you test that this still works now? > > Yes, that still works. Great. :-) > > The other question is about this code in bdrv_open_flags(): > > > > /* > > * Snapshots should be writable. > > */ > > if (bs->is_temporary) { > > open_flags |= BDRV_O_RDWR; > > } > > > > Is this dead code now because the flag is always already set? > > > > Yes, that ends up being dead code. From later in blockdev_init(): > > bdrv_flags |= ro ? 0 : BDRV_O_RDWR; > > QINCREF(bs_opts); > ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error); > > > And ro is set from the read-only QemuOpts, that we check in this > patch in conjunction with snapshot. So if read-only=on is not > specified, it is opened r/w by default. Good, that was my impression as well. Can you send a follow-up patch to remove the dead code? Kevin
Am 14.01.2014 um 20:12 hat Jeff Cody geschrieben: > Having both read-only=on and snapshot=on together does not make sense; > currently, the read-only argument is effectively ignored for the > temporary snapshot. To prevent confusion, disallow the usage of both > 'snapshot=on' and 'read-only=on'. > > Signed-off-by: Jeff Cody <jcody@redhat.com> This breaks in a surprising way: $ softmmu/qemu-system-x86_64 -drive file=~/images/hd.img,snapshot=on ... works fine ... $ qemu-system-x86_64 -drive file=~/images/hd.img -snapshot qemu-system-x86_64: invalid option combination: read-only and snapshot qemu-iotests case 051 catches this. I'll have to remove this patch and the follow-up from the queue for now. Kevin
On Fri, Jan 24, 2014 at 02:33:19PM +0100, Kevin Wolf wrote: > Am 14.01.2014 um 20:12 hat Jeff Cody geschrieben: > > Having both read-only=on and snapshot=on together does not make sense; > > currently, the read-only argument is effectively ignored for the > > temporary snapshot. To prevent confusion, disallow the usage of both > > 'snapshot=on' and 'read-only=on'. > > > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > This breaks in a surprising way: > > $ softmmu/qemu-system-x86_64 -drive file=~/images/hd.img,snapshot=on > ... works fine ... > > $ qemu-system-x86_64 -drive file=~/images/hd.img -snapshot > qemu-system-x86_64: invalid option combination: read-only and snapshot > > qemu-iotests case 051 catches this. I'll have to remove this patch and > the follow-up from the queue for now. > Odd - OK, I'll follow up on this and submit a series with both patches (well, likely squashed together), and whatever is needed to fix this as well.
Am 24.01.2014 um 14:48 hat Jeff Cody geschrieben: > On Fri, Jan 24, 2014 at 02:33:19PM +0100, Kevin Wolf wrote: > > Am 14.01.2014 um 20:12 hat Jeff Cody geschrieben: > > > Having both read-only=on and snapshot=on together does not make sense; > > > currently, the read-only argument is effectively ignored for the > > > temporary snapshot. To prevent confusion, disallow the usage of both > > > 'snapshot=on' and 'read-only=on'. > > > > > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > > > This breaks in a surprising way: > > > > $ softmmu/qemu-system-x86_64 -drive file=~/images/hd.img,snapshot=on > > ... works fine ... > > > > $ qemu-system-x86_64 -drive file=~/images/hd.img -snapshot > > qemu-system-x86_64: invalid option combination: read-only and snapshot > > > > qemu-iotests case 051 catches this. I'll have to remove this patch and > > the follow-up from the queue for now. > > > > Odd - OK, I'll follow up on this and submit a series with both patches > (well, likely squashed together), and whatever is needed to fix this > as well. Jeff, what happened with this? I found the two patches in an old git branch and wondered why they didn't disappear in the rebase. But apparently we still allow read-only and snapshot at the same time. Kevin
On Wed, Mar 12, 2014 at 12:16:04PM +0100, Kevin Wolf wrote: > Am 24.01.2014 um 14:48 hat Jeff Cody geschrieben: > > On Fri, Jan 24, 2014 at 02:33:19PM +0100, Kevin Wolf wrote: > > > Am 14.01.2014 um 20:12 hat Jeff Cody geschrieben: > > > > Having both read-only=on and snapshot=on together does not make sense; > > > > currently, the read-only argument is effectively ignored for the > > > > temporary snapshot. To prevent confusion, disallow the usage of both > > > > 'snapshot=on' and 'read-only=on'. > > > > > > > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > > > > > This breaks in a surprising way: > > > > > > $ softmmu/qemu-system-x86_64 -drive file=~/images/hd.img,snapshot=on > > > ... works fine ... > > > > > > $ qemu-system-x86_64 -drive file=~/images/hd.img -snapshot > > > qemu-system-x86_64: invalid option combination: read-only and snapshot > > > > > > qemu-iotests case 051 catches this. I'll have to remove this patch and > > > the follow-up from the queue for now. > > > > > > > Odd - OK, I'll follow up on this and submit a series with both patches > > (well, likely squashed together), and whatever is needed to fix this > > as well. > > Jeff, what happened with this? I found the two patches in an old git > branch and wondered why they didn't disappear in the rebase. But > apparently we still allow read-only and snapshot at the same time. > The test case 051 failed because when -snapshot was specified, it was enabled for all the default drives as well, which included default_cdrom. The solutions for that seemed a little hacky, and I wasn't sure it was actually worth it, given that this is probably not a real problem to begin with. If you think it is worth it, I can resurrect the patch and make sure it works with both snapshot=on and -snapshot.
diff --git a/blockdev.c b/blockdev.c index e457494..845ff8a 100644 --- a/blockdev.c +++ b/blockdev.c @@ -352,6 +352,13 @@ static DriveInfo *blockdev_init(QDict *bs_opts, /* extract parameters */ snapshot = qemu_opt_get_bool(opts, "snapshot", 0); ro = qemu_opt_get_bool(opts, "read-only", 0); + + /* having ro and snapshot together does not make sense */ + if (ro && snapshot) { + error_setg(errp, "invalid option combination: read-only and snapshot"); + goto early_err; + } + copy_on_read = qemu_opt_get_bool(opts, "copy-on-read", false); file = qemu_opt_get(opts, "file");
Having both read-only=on and snapshot=on together does not make sense; currently, the read-only argument is effectively ignored for the temporary snapshot. To prevent confusion, disallow the usage of both 'snapshot=on' and 'read-only=on'. Signed-off-by: Jeff Cody <jcody@redhat.com> --- blockdev.c | 7 +++++++ 1 file changed, 7 insertions(+)