* [Qemu-devel] RFC: handling image options with drive-mirror/drive-backup
@ 2016-09-29 8:34 Daniel P. Berrange
2016-09-29 8:43 ` Fam Zheng
0 siblings, 1 reply; 11+ messages in thread
From: Daniel P. Berrange @ 2016-09-29 8:34 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, eblake, kwolf, Stefan Hajnoczi
I got a report that the LUKS block driver was not working in combination
with the drive-mirror command and have been investigating possible fixes
for this.
The core problem here is dealing with the target image. If you try to
run with a pre-created target that is a LUKS image, it will fail because
we have no way to provide the "key-secret" option required to open the
target. If you try to tell drive-mirror to create a new target with
LUKS format, it will fail trying to create the image, again because
no "key-secret" option can be provided.
While this is a fundamental blocker problem for LUKS, it also affects
other image formats. For example, if you're telling drive-mirror to
create a new qcow2 volume, its impossible to control desirable attributes
like cluster-size, or compat-level. If you're mirroring a qcow2 file to
a new qcow2 file, it is impossible to maintain any custom runtimes opts
yuou might have set on the source - eg 'lazy-refcounts', or the various
discard settings will all be stuck on defaults for the target.
You can workaround the problem of being able to create new volumes by
just creating them using qemu-img ahead of time instead.
Dealing with the problem of opening images, requires that we have some
way to provide block options to the drive-mirror command. The naive
approach would to just add a new parameter
'options': ['str']
but IMHO this is just perpetuating the broken design of drive-mirror.
The core problem is that this command should not have been using a
plain target + format pair of strings in the first place. Instead it
should have had a single
"target": "BlockdevOptions"
So my suggestion is that we deprecate "drive-mirror" and define a fixed
command "drive-mirror-blockdev" (or "blockdev-mirror" ?) that accepts
the proper BlockdevOptions QAPI type for the target as above.
This only solves the "open an existing image" case - if we want to
have drive-mirror be able to create new images, then we need to have
a new struct "BlockdevCreateOptions", since creation options are a
superset of the "BlockdevOptions". I'm inclined to say that the
"drive-mirror" command should *not* have the ability to create new
images, as I can't see a compelling reason to overload that functionality
in the same command.
Instead we should add a "blockdev-create" QMP command for doing that
action explicitly, that apps can invoke just prior to running the
drive-mirror command. We'd also want "blockdev-delete" to allow it
to be deleted on failure.
It looks like the same design problem of drive-mirror also applies
to drive-backup.
Co-incidentally I had already been experimenting with the creation
of a "BlockdevCreateOptions" QAPI type, in order to support the
fully-nested block device creation with 'qemu-img create'.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] RFC: handling image options with drive-mirror/drive-backup
2016-09-29 8:34 [Qemu-devel] RFC: handling image options with drive-mirror/drive-backup Daniel P. Berrange
@ 2016-09-29 8:43 ` Fam Zheng
2016-09-29 8:51 ` Daniel P. Berrange
0 siblings, 1 reply; 11+ messages in thread
From: Fam Zheng @ 2016-09-29 8:43 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: qemu-devel, kwolf, Stefan Hajnoczi, qemu-block
On Thu, 09/29 09:34, Daniel P. Berrange wrote:
> I got a report that the LUKS block driver was not working in combination
> with the drive-mirror command and have been investigating possible fixes
> for this.
>
> The core problem here is dealing with the target image. If you try to
> run with a pre-created target that is a LUKS image, it will fail because
> we have no way to provide the "key-secret" option required to open the
> target. If you try to tell drive-mirror to create a new target with
> LUKS format, it will fail trying to create the image, again because
> no "key-secret" option can be provided.
>
> While this is a fundamental blocker problem for LUKS, it also affects
> other image formats. For example, if you're telling drive-mirror to
> create a new qcow2 volume, its impossible to control desirable attributes
> like cluster-size, or compat-level. If you're mirroring a qcow2 file to
> a new qcow2 file, it is impossible to maintain any custom runtimes opts
> yuou might have set on the source - eg 'lazy-refcounts', or the various
> discard settings will all be stuck on defaults for the target.
>
> You can workaround the problem of being able to create new volumes by
> just creating them using qemu-img ahead of time instead.
>
> Dealing with the problem of opening images, requires that we have some
> way to provide block options to the drive-mirror command. The naive
> approach would to just add a new parameter
>
> 'options': ['str']
>
> but IMHO this is just perpetuating the broken design of drive-mirror.
>
> The core problem is that this command should not have been using a
> plain target + format pair of strings in the first place. Instead it
> should have had a single
>
> "target": "BlockdevOptions"
>
> So my suggestion is that we deprecate "drive-mirror" and define a fixed
> command "drive-mirror-blockdev" (or "blockdev-mirror" ?) that accepts
> the proper BlockdevOptions QAPI type for the target as above.
Are you aware that there is already a blockdev-mirror command? Supposedly it
can do what you need, together with blockdev-add once the latter is deemed
ready.
>
> This only solves the "open an existing image" case - if we want to
> have drive-mirror be able to create new images, then we need to have
> a new struct "BlockdevCreateOptions", since creation options are a
> superset of the "BlockdevOptions". I'm inclined to say that the
> "drive-mirror" command should *not* have the ability to create new
> images, as I can't see a compelling reason to overload that functionality
> in the same command.
Images can be created out of bound with qemu-img, and opened with blockdev-add.
>
> Instead we should add a "blockdev-create" QMP command for doing that
> action explicitly, that apps can invoke just prior to running the
> drive-mirror command. We'd also want "blockdev-delete" to allow it
> to be deleted on failure.
>
> It looks like the same design problem of drive-mirror also applies
> to drive-backup.
There is also blockdev-backup already.
Fam
>
> Co-incidentally I had already been experimenting with the creation
> of a "BlockdevCreateOptions" QAPI type, in order to support the
> fully-nested block device creation with 'qemu-img create'.
>
> Regards,
> Daniel
> --
> |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org -o- http://virt-manager.org :|
> |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] RFC: handling image options with drive-mirror/drive-backup
2016-09-29 8:43 ` Fam Zheng
@ 2016-09-29 8:51 ` Daniel P. Berrange
2016-09-29 9:09 ` Fam Zheng
2016-09-29 9:47 ` Kashyap Chamarthy
0 siblings, 2 replies; 11+ messages in thread
From: Daniel P. Berrange @ 2016-09-29 8:51 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel, kwolf, Stefan Hajnoczi, qemu-block
On Thu, Sep 29, 2016 at 04:43:25PM +0800, Fam Zheng wrote:
> On Thu, 09/29 09:34, Daniel P. Berrange wrote:
> > I got a report that the LUKS block driver was not working in combination
> > with the drive-mirror command and have been investigating possible fixes
> > for this.
> >
> > The core problem here is dealing with the target image. If you try to
> > run with a pre-created target that is a LUKS image, it will fail because
> > we have no way to provide the "key-secret" option required to open the
> > target. If you try to tell drive-mirror to create a new target with
> > LUKS format, it will fail trying to create the image, again because
> > no "key-secret" option can be provided.
> >
> > While this is a fundamental blocker problem for LUKS, it also affects
> > other image formats. For example, if you're telling drive-mirror to
> > create a new qcow2 volume, its impossible to control desirable attributes
> > like cluster-size, or compat-level. If you're mirroring a qcow2 file to
> > a new qcow2 file, it is impossible to maintain any custom runtimes opts
> > yuou might have set on the source - eg 'lazy-refcounts', or the various
> > discard settings will all be stuck on defaults for the target.
> >
> > You can workaround the problem of being able to create new volumes by
> > just creating them using qemu-img ahead of time instead.
> >
> > Dealing with the problem of opening images, requires that we have some
> > way to provide block options to the drive-mirror command. The naive
> > approach would to just add a new parameter
> >
> > 'options': ['str']
> >
> > but IMHO this is just perpetuating the broken design of drive-mirror.
> >
> > The core problem is that this command should not have been using a
> > plain target + format pair of strings in the first place. Instead it
> > should have had a single
> >
> > "target": "BlockdevOptions"
> >
> > So my suggestion is that we deprecate "drive-mirror" and define a fixed
> > command "drive-mirror-blockdev" (or "blockdev-mirror" ?) that accepts
> > the proper BlockdevOptions QAPI type for the target as above.
>
> Are you aware that there is already a blockdev-mirror command? Supposedly it
> can do what you need, together with blockdev-add once the latter is deemed
> ready.
Clearly I'm not aware of that :-) It seems libvirt does not yet use
blockdev-mirror either, which is where I got the original bug report
about drive-mirror from.
I'll submit a patch to mark drive-mirror as deprecated and point the
otherwise ignorant reader towards blockdev-mirror instead....
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] RFC: handling image options with drive-mirror/drive-backup
2016-09-29 8:51 ` Daniel P. Berrange
@ 2016-09-29 9:09 ` Fam Zheng
2016-09-29 9:15 ` Daniel P. Berrange
2016-09-29 9:17 ` Kevin Wolf
2016-09-29 9:47 ` Kashyap Chamarthy
1 sibling, 2 replies; 11+ messages in thread
From: Fam Zheng @ 2016-09-29 9:09 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: qemu-devel, kwolf, Stefan Hajnoczi, qemu-block
On Thu, 09/29 09:51, Daniel P. Berrange wrote:
> On Thu, Sep 29, 2016 at 04:43:25PM +0800, Fam Zheng wrote:
> > On Thu, 09/29 09:34, Daniel P. Berrange wrote:
> > > So my suggestion is that we deprecate "drive-mirror" and define a fixed
> > > command "drive-mirror-blockdev" (or "blockdev-mirror" ?) that accepts
> > > the proper BlockdevOptions QAPI type for the target as above.
> >
> > Are you aware that there is already a blockdev-mirror command? Supposedly it
> > can do what you need, together with blockdev-add once the latter is deemed
> > ready.
>
> Clearly I'm not aware of that :-) It seems libvirt does not yet use
> blockdev-mirror either, which is where I got the original bug report
> about drive-mirror from.
Libvirt doesn't support blockdev-add yet, because the command is still being
actively worked on at QEMU side, and is therefore thought to be not "stable"
yet. Though, I think blockdev-add + blockdev-{mirror,backup} are already useful
for common tasks (like your use case with LUKS).
>
> I'll submit a patch to mark drive-mirror as deprecated and point the
> otherwise ignorant reader towards blockdev-mirror instead....
For above reasons, not sure whether we want to advertise it already.
Anyway deprecating drive-mirror is a bit too early.
Fam
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] RFC: handling image options with drive-mirror/drive-backup
2016-09-29 9:09 ` Fam Zheng
@ 2016-09-29 9:15 ` Daniel P. Berrange
2016-09-29 9:25 ` Fam Zheng
2016-09-29 9:17 ` Kevin Wolf
1 sibling, 1 reply; 11+ messages in thread
From: Daniel P. Berrange @ 2016-09-29 9:15 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel, kwolf, Stefan Hajnoczi, qemu-block
On Thu, Sep 29, 2016 at 05:09:20PM +0800, Fam Zheng wrote:
> On Thu, 09/29 09:51, Daniel P. Berrange wrote:
> > On Thu, Sep 29, 2016 at 04:43:25PM +0800, Fam Zheng wrote:
> > > On Thu, 09/29 09:34, Daniel P. Berrange wrote:
> > > > So my suggestion is that we deprecate "drive-mirror" and define a fixed
> > > > command "drive-mirror-blockdev" (or "blockdev-mirror" ?) that accepts
> > > > the proper BlockdevOptions QAPI type for the target as above.
> > >
> > > Are you aware that there is already a blockdev-mirror command? Supposedly it
> > > can do what you need, together with blockdev-add once the latter is deemed
> > > ready.
> >
> > Clearly I'm not aware of that :-) It seems libvirt does not yet use
> > blockdev-mirror either, which is where I got the original bug report
> > about drive-mirror from.
>
> Libvirt doesn't support blockdev-add yet, because the command is still being
> actively worked on at QEMU side, and is therefore thought to be not "stable"
> yet. Though, I think blockdev-add + blockdev-{mirror,backup} are already useful
> for common tasks (like your use case with LUKS).
In what way is it "unstable", aside from the obvious limitation that some
of the blockdev backends are not yet represented in BlockdevOptions QAPI
schema ?
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] RFC: handling image options with drive-mirror/drive-backup
2016-09-29 9:09 ` Fam Zheng
2016-09-29 9:15 ` Daniel P. Berrange
@ 2016-09-29 9:17 ` Kevin Wolf
2016-09-29 9:43 ` Daniel P. Berrange
1 sibling, 1 reply; 11+ messages in thread
From: Kevin Wolf @ 2016-09-29 9:17 UTC (permalink / raw)
To: Fam Zheng; +Cc: Daniel P. Berrange, qemu-devel, Stefan Hajnoczi, qemu-block
Am 29.09.2016 um 11:09 hat Fam Zheng geschrieben:
> On Thu, 09/29 09:51, Daniel P. Berrange wrote:
> > On Thu, Sep 29, 2016 at 04:43:25PM +0800, Fam Zheng wrote:
> > > On Thu, 09/29 09:34, Daniel P. Berrange wrote:
> > > > So my suggestion is that we deprecate "drive-mirror" and define a fixed
> > > > command "drive-mirror-blockdev" (or "blockdev-mirror" ?) that accepts
> > > > the proper BlockdevOptions QAPI type for the target as above.
> > >
> > > Are you aware that there is already a blockdev-mirror command? Supposedly it
> > > can do what you need, together with blockdev-add once the latter is deemed
> > > ready.
> >
> > Clearly I'm not aware of that :-) It seems libvirt does not yet use
> > blockdev-mirror either, which is where I got the original bug report
> > about drive-mirror from.
>
> Libvirt doesn't support blockdev-add yet, because the command is still being
> actively worked on at QEMU side, and is therefore thought to be not "stable"
> yet. Though, I think blockdev-add + blockdev-{mirror,backup} are already useful
> for common tasks (like your use case with LUKS).
Just to clarify what "not stable" means: Literally none of the
blockdev-add commands that used to work when it was originally merged
are still working. And we're considering another similar change
(removing the "options" indirection) that will change the command for
all users. So while I would encourage libvirt to write prototyp code for
supporting blockdev-add now, I would advise against enabling it in a
release yet.
> > I'll submit a patch to mark drive-mirror as deprecated and point the
> > otherwise ignorant reader towards blockdev-mirror instead....
>
> For above reasons, not sure whether we want to advertise it already.
> Anyway deprecating drive-mirror is a bit too early.
That was my first thought, too. However, blockdev-{mirror,backup} can
also work with drives added with -drive or drive_add, so it might be
useful enough even without blockdev-add.
Kevin
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] RFC: handling image options with drive-mirror/drive-backup
2016-09-29 9:15 ` Daniel P. Berrange
@ 2016-09-29 9:25 ` Fam Zheng
0 siblings, 0 replies; 11+ messages in thread
From: Fam Zheng @ 2016-09-29 9:25 UTC (permalink / raw)
To: kwolf, Daniel P. Berrange; +Cc: qemu-devel, Stefan Hajnoczi, qemu-block
On Thu, 09/29 10:15, Daniel P. Berrange wrote:
> On Thu, Sep 29, 2016 at 05:09:20PM +0800, Fam Zheng wrote:
> > On Thu, 09/29 09:51, Daniel P. Berrange wrote:
> > > On Thu, Sep 29, 2016 at 04:43:25PM +0800, Fam Zheng wrote:
> > > > On Thu, 09/29 09:34, Daniel P. Berrange wrote:
> > > > > So my suggestion is that we deprecate "drive-mirror" and define a fixed
> > > > > command "drive-mirror-blockdev" (or "blockdev-mirror" ?) that accepts
> > > > > the proper BlockdevOptions QAPI type for the target as above.
> > > >
> > > > Are you aware that there is already a blockdev-mirror command? Supposedly it
> > > > can do what you need, together with blockdev-add once the latter is deemed
> > > > ready.
> > >
> > > Clearly I'm not aware of that :-) It seems libvirt does not yet use
> > > blockdev-mirror either, which is where I got the original bug report
> > > about drive-mirror from.
> >
> > Libvirt doesn't support blockdev-add yet, because the command is still being
> > actively worked on at QEMU side, and is therefore thought to be not "stable"
> > yet. Though, I think blockdev-add + blockdev-{mirror,backup} are already useful
> > for common tasks (like your use case with LUKS).
>
> In what way is it "unstable", aside from the obvious limitation that some
> of the blockdev backends are not yet represented in BlockdevOptions QAPI
> schema ?
I think another risk is that it is so flexible in terms of building up the
block node graph (especially with BdrvChild referencing) that things may go
loose unexpectedly if the user is doing things we haven't really expected.
I don't have the full list in mind. Maybe Kevin can explain more.
Fam
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] RFC: handling image options with drive-mirror/drive-backup
2016-09-29 9:17 ` Kevin Wolf
@ 2016-09-29 9:43 ` Daniel P. Berrange
2016-09-29 10:29 ` Kashyap Chamarthy
2016-09-29 10:37 ` Kevin Wolf
0 siblings, 2 replies; 11+ messages in thread
From: Daniel P. Berrange @ 2016-09-29 9:43 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Fam Zheng, qemu-devel, Stefan Hajnoczi, qemu-block
On Thu, Sep 29, 2016 at 11:17:52AM +0200, Kevin Wolf wrote:
> Am 29.09.2016 um 11:09 hat Fam Zheng geschrieben:
> > On Thu, 09/29 09:51, Daniel P. Berrange wrote:
> > > On Thu, Sep 29, 2016 at 04:43:25PM +0800, Fam Zheng wrote:
> > > > On Thu, 09/29 09:34, Daniel P. Berrange wrote:
> > > > > So my suggestion is that we deprecate "drive-mirror" and define a fixed
> > > > > command "drive-mirror-blockdev" (or "blockdev-mirror" ?) that accepts
> > > > > the proper BlockdevOptions QAPI type for the target as above.
> > > >
> > > > Are you aware that there is already a blockdev-mirror command? Supposedly it
> > > > can do what you need, together with blockdev-add once the latter is deemed
> > > > ready.
> > >
> > > Clearly I'm not aware of that :-) It seems libvirt does not yet use
> > > blockdev-mirror either, which is where I got the original bug report
> > > about drive-mirror from.
> >
> > Libvirt doesn't support blockdev-add yet, because the command is still being
> > actively worked on at QEMU side, and is therefore thought to be not "stable"
> > yet. Though, I think blockdev-add + blockdev-{mirror,backup} are already useful
> > for common tasks (like your use case with LUKS).
>
> Just to clarify what "not stable" means: Literally none of the
> blockdev-add commands that used to work when it was originally merged
> are still working. And we're considering another similar change
> (removing the "options" indirection) that will change the command for
> all users. So while I would encourage libvirt to write prototyp code for
> supporting blockdev-add now, I would advise against enabling it in a
> release yet.
Urgh, arbitrarily changing behaviour of existing commands is really
very bad for libvirt, as it means we have to now write special case
logic to detect whether we can use the command or not, instead of
merely detecting whether it exists.
If commands are expected to change, they should have an 'x-' prefix
and once that's removed they should never be changed in an incompatible
manner again.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] RFC: handling image options with drive-mirror/drive-backup
2016-09-29 8:51 ` Daniel P. Berrange
2016-09-29 9:09 ` Fam Zheng
@ 2016-09-29 9:47 ` Kashyap Chamarthy
1 sibling, 0 replies; 11+ messages in thread
From: Kashyap Chamarthy @ 2016-09-29 9:47 UTC (permalink / raw)
To: Daniel P. Berrange
Cc: Fam Zheng, kwolf, qemu-block, qemu-devel, Stefan Hajnoczi
On Thu, Sep 29, 2016 at 09:51:09AM +0100, Daniel P. Berrange wrote:
> On Thu, Sep 29, 2016 at 04:43:25PM +0800, Fam Zheng wrote:
> > On Thu, 09/29 09:34, Daniel P. Berrange wrote:
[...]
> > > So my suggestion is that we deprecate "drive-mirror" and define a fixed
> > > command "drive-mirror-blockdev" (or "blockdev-mirror" ?) that accepts
> > > the proper BlockdevOptions QAPI type for the target as above.
> >
> > Are you aware that there is already a blockdev-mirror command? Supposedly it
> > can do what you need, together with blockdev-add once the latter is deemed
> > ready.
>
> Clearly I'm not aware of that :-) It seems libvirt does not yet use
> blockdev-mirror either, which is where I got the original bug report
> about drive-mirror from.
>
> I'll submit a patch to mark drive-mirror as deprecated and point the
> otherwise ignorant reader towards blockdev-mirror instead....
You might also want to see this short thread I started last month to
clarify the semantics of `drive-mirror` & `blockdev-mirror`
https://lists.nongnu.org/archive/html/qemu-block/2016-08/msg00472.html
-- 'drive-mirror' vs. 'blockdev-mirror' semantics
Kevin's response to the above:
https://lists.nongnu.org/archive/html/qemu-block/2016-08/msg00493.html
Some addendum to Kevin's response above:
https://lists.nongnu.org/archive/html/qemu-block/2016-08/msg00503.html
And FWIW, here's some tests of `blockdev-add` + `blockdev-backup`
(`blockdev-mirror` is similar, just a different point-in-time --
point-in-time for 'drive-mirror' is when you break the synchronization;
and the point-in-time for 'drive-backup' is when you _start_ the
operation):
https://kashyapc.fedorapeople.org/virt/qemu/blockdev-add-and-blockdev-backup-success-tests.txt
--
/kashyap
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] RFC: handling image options with drive-mirror/drive-backup
2016-09-29 9:43 ` Daniel P. Berrange
@ 2016-09-29 10:29 ` Kashyap Chamarthy
2016-09-29 10:37 ` Kevin Wolf
1 sibling, 0 replies; 11+ messages in thread
From: Kashyap Chamarthy @ 2016-09-29 10:29 UTC (permalink / raw)
To: Daniel P. Berrange
Cc: Kevin Wolf, qemu-block, Fam Zheng, qemu-devel, Stefan Hajnoczi
On Thu, Sep 29, 2016 at 10:43:37AM +0100, Daniel P. Berrange wrote:
> On Thu, Sep 29, 2016 at 11:17:52AM +0200, Kevin Wolf wrote:
[...]
> > Just to clarify what "not stable" means: Literally none of the
> > blockdev-add commands that used to work when it was originally merged
> > are still working. And we're considering another similar change
> > (removing the "options" indirection) that will change the command for
> > all users. So while I would encourage libvirt to write prototyp code for
> > supporting blockdev-add now, I would advise against enabling it in a
> > release yet.
>
> Urgh, arbitrarily changing behaviour of existing commands is really
> very bad for libvirt, as it means we have to now write special case
> logic to detect whether we can use the command or not, instead of
> merely detecting whether it exists.
>
> If commands are expected to change, they should have an 'x-' prefix
> and once that's removed they should never be changed in an incompatible
> manner again.
I too wondered about the "x-" prefix for 'blockdev-add' when I was
digging into the details a month ago. I came to the conclusion that it
was just an inadvertant mistake that it wasn't marked as such --
because, you could see the experimental prefix for rest of them
commands: x-blockdev-change x-blockdev-del, x-blockdev-insert-medium,
x-blockdev-remove-medium
--
/kashyap
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] RFC: handling image options with drive-mirror/drive-backup
2016-09-29 9:43 ` Daniel P. Berrange
2016-09-29 10:29 ` Kashyap Chamarthy
@ 2016-09-29 10:37 ` Kevin Wolf
1 sibling, 0 replies; 11+ messages in thread
From: Kevin Wolf @ 2016-09-29 10:37 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: Fam Zheng, qemu-devel, Stefan Hajnoczi, qemu-block
Am 29.09.2016 um 11:43 hat Daniel P. Berrange geschrieben:
> On Thu, Sep 29, 2016 at 11:17:52AM +0200, Kevin Wolf wrote:
> > Am 29.09.2016 um 11:09 hat Fam Zheng geschrieben:
> > > On Thu, 09/29 09:51, Daniel P. Berrange wrote:
> > > > On Thu, Sep 29, 2016 at 04:43:25PM +0800, Fam Zheng wrote:
> > > > > On Thu, 09/29 09:34, Daniel P. Berrange wrote:
> > > > > > So my suggestion is that we deprecate "drive-mirror" and define a fixed
> > > > > > command "drive-mirror-blockdev" (or "blockdev-mirror" ?) that accepts
> > > > > > the proper BlockdevOptions QAPI type for the target as above.
> > > > >
> > > > > Are you aware that there is already a blockdev-mirror command? Supposedly it
> > > > > can do what you need, together with blockdev-add once the latter is deemed
> > > > > ready.
> > > >
> > > > Clearly I'm not aware of that :-) It seems libvirt does not yet use
> > > > blockdev-mirror either, which is where I got the original bug report
> > > > about drive-mirror from.
> > >
> > > Libvirt doesn't support blockdev-add yet, because the command is still being
> > > actively worked on at QEMU side, and is therefore thought to be not "stable"
> > > yet. Though, I think blockdev-add + blockdev-{mirror,backup} are already useful
> > > for common tasks (like your use case with LUKS).
> >
> > Just to clarify what "not stable" means: Literally none of the
> > blockdev-add commands that used to work when it was originally merged
> > are still working. And we're considering another similar change
> > (removing the "options" indirection) that will change the command for
> > all users. So while I would encourage libvirt to write prototyp code for
> > supporting blockdev-add now, I would advise against enabling it in a
> > release yet.
>
> Urgh, arbitrarily changing behaviour of existing commands is really
> very bad for libvirt, as it means we have to now write special case
> logic to detect whether we can use the command or not, instead of
> merely detecting whether it exists.
>
> If commands are expected to change, they should have an 'x-' prefix
> and once that's removed they should never be changed in an incompatible
> manner again.
Yes, we messed up this one. Don't use blockdev-add until x-blockdev-del
has been renamed.
Kevin
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-09-29 10:37 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-29 8:34 [Qemu-devel] RFC: handling image options with drive-mirror/drive-backup Daniel P. Berrange
2016-09-29 8:43 ` Fam Zheng
2016-09-29 8:51 ` Daniel P. Berrange
2016-09-29 9:09 ` Fam Zheng
2016-09-29 9:15 ` Daniel P. Berrange
2016-09-29 9:25 ` Fam Zheng
2016-09-29 9:17 ` Kevin Wolf
2016-09-29 9:43 ` Daniel P. Berrange
2016-09-29 10:29 ` Kashyap Chamarthy
2016-09-29 10:37 ` Kevin Wolf
2016-09-29 9:47 ` Kashyap Chamarthy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.