[Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none.

[Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none.

[Ian Main]

qcow2 supports backing files so it makes sense to default to qcow2
for MIRROR_SYNC_MODE_NONE so that we can use the source as a backing
drive and export it via nbd.  Defaulting FULL and TOP to SYNC_MODE_NONE
breaks tests but that could be fixed if we wanted it.

Signed-off-by: Ian Main <imain@redhat.com>
---
 blockdev.c       | 5 ++++-
 qapi-schema.json | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 000dea6..a56ba08 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1462,7 +1462,10 @@ void qmp_drive_backup(const char *device, const char *target,
     }
 
     if (!has_format) {
-        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name;
+        format = NULL;
+        if (mode != NEW_IMAGE_MODE_EXISTING) {
+            format = sync == MIRROR_SYNC_MODE_NONE ? "qcow2" : bs->drv->format_name;
+        }
     }
     if (format) {
         drv = bdrv_find_format(format);
diff --git a/qapi-schema.json b/qapi-schema.json
index b3f6c2a..e2c86f9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1806,6 +1806,7 @@
 #
 # @format: #optional the format of the new destination, default is to
 #          probe if @mode is 'existing', else the format of the source
+#          drive.  If @sync is 'none' then the default is qcow2.
 #
 # @sync: what parts of the disk image should be copied to the destination
 #        (all the disk, only the sectors allocated in the topmost image, or
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

On 07/18/2013 12:47 PM, Ian Main wrote:
> qcow2 supports backing files so it makes sense to default to qcow2
> for MIRROR_SYNC_MODE_NONE so that we can use the source as a backing
> drive and export it via nbd.  Defaulting FULL and TOP to SYNC_MODE_NONE
> breaks tests but that could be fixed if we wanted it.
> 
> Signed-off-by: Ian Main <imain@redhat.com>
> ---
>  blockdev.c       | 5 ++++-
>  qapi-schema.json | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)

Looks okay, but let's answer the meta-question first of whether we
should just make 'format' mandatory and be done with it.

Also, I've noticed you aren't cc'ing many people; that can slow down
reviews.  http://wiki.qemu.org/Contribute/SubmitAPatch gives hints on
how to determine the right people to send your patches to, by
deciphering MAINTAINERS and running ./scripts/getmaintainer.pl.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none.

[Ian Main]

On Thu, Jul 18, 2013 at 12:56:51PM -0600, Eric Blake wrote:
> On 07/18/2013 12:47 PM, Ian Main wrote:
> > qcow2 supports backing files so it makes sense to default to qcow2
> > for MIRROR_SYNC_MODE_NONE so that we can use the source as a backing
> > drive and export it via nbd.  Defaulting FULL and TOP to SYNC_MODE_NONE
> > breaks tests but that could be fixed if we wanted it.
> > 
> > Signed-off-by: Ian Main <imain@redhat.com>
> > ---
> >  blockdev.c       | 5 ++++-
> >  qapi-schema.json | 1 +
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> Looks okay, but let's answer the meta-question first of whether we
> should just make 'format' mandatory and be done with it.
> 
> Also, I've noticed you aren't cc'ing many people; that can slow down
> reviews.  http://wiki.qemu.org/Contribute/SubmitAPatch gives hints on
> how to determine the right people to send your patches to, by
> deciphering MAINTAINERS and running ./scripts/getmaintainer.pl.

Ah ok, I'll add them next rev.

My take has been to just do a patch that implements the suggestion and
see what people think :).

	Ian

Re: [Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1459 bytes --]

On 07/18/2013 01:13 PM, Ian Main wrote:
> On Thu, Jul 18, 2013 at 12:56:51PM -0600, Eric Blake wrote:
>> On 07/18/2013 12:47 PM, Ian Main wrote:
>>> qcow2 supports backing files so it makes sense to default to qcow2
>>> for MIRROR_SYNC_MODE_NONE so that we can use the source as a backing
>>> drive and export it via nbd.  Defaulting FULL and TOP to SYNC_MODE_NONE
>>> breaks tests but that could be fixed if we wanted it.
>>>
>>> Signed-off-by: Ian Main <imain@redhat.com>
>>> ---
>>>  blockdev.c       | 5 ++++-
>>>  qapi-schema.json | 1 +
>>>  2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> Looks okay, but let's answer the meta-question first of whether we
>> should just make 'format' mandatory and be done with it.
>>
>> Also, I've noticed you aren't cc'ing many people; that can slow down
>> reviews.  http://wiki.qemu.org/Contribute/SubmitAPatch gives hints on
>> how to determine the right people to send your patches to, by
>> deciphering MAINTAINERS and running ./scripts/getmaintainer.pl.
> 
> Ah ok, I'll add them next rev.
> 
> My take has been to just do a patch that implements the suggestion and
> see what people think :).

But this list is so high volume that the people that matter won't check
your email unless they are cc'd :)  If you want opinions fast, it pays
to follow the directions.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

On 07/18/2013 12:03 PM, Ian Main wrote:
>>
>> Or we could simplify life by making 'format' mandatory for drive-backup;
>> it was optional for 'drive-mirror' due to incremental implementation,
>> but for 'drive-backup', we still have the opportunity to do things right
>> from the first release.
> 
> Ah, I did make a doc change, I must have forgotten to add it.
> 
> I'm all for making format mandatory if that is ok with everyone.  Maybe
> that is the best solution.

We can always change our mind in 1.7 to make it optional if we change
our minds, but I'd definitely like to see patches that make 'format'
mandatory for DriveBackup for 1.6 - simpler all around.  Converting
mandatory to optional is discoverable via introspection; while
converting optional to mandatory is an API break.  And since we can
argue that optional formats is relatively risky, it's better to have our
initial release be conservative by requiring the field until (and
unless) someone comes up with a use case why leaving it unspecified
makes a difference.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none.

[Ian Main]

On Thu, Jul 18, 2013 at 11:27:21AM -0600, Eric Blake wrote:
> On 07/17/2013 02:04 PM, Ian Main wrote:
> > qcow2 supports backing files so it makes sense to default to qcow2
> > for MIRROR_SYNC_MODE_NONE so that we can use the source as a backing
> > drive and export it via nbd.  Defaulting FULL and TOP to SYNC_MODE_NONE
> > breaks tests but that could be fixed if we wanted it.
> > 
> > Signed-off-by: Ian Main <imain@redhat.com>
> > ---
> >  blockdev.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index 000dea6..805b0e5 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -1462,7 +1462,7 @@ void qmp_drive_backup(const char *device, const char *target,
> >      }
> >  
> >      if (!has_format) {
> > -        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name;
> > +        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : "qcow2";
> 
> Is this the right thing to do?  Or should we do:
> 
> if (!has_format) {
>     if (mode == NEW_IMAGE_MODE_EXISTING) {
>         format = NULL;
>     } else {
>         format = bs->drv->format_name ?: "qcow2";
>     }
> }
> 
> That is, I think we should default to doing a backup in the format given
> by the original (what if the original is qed, which also supports
> backing files), and only use qcow2 when there is no guidance whatsoever.
> 
> But in practice, I don't care - format probing is a security hole, so
> libvirt should always be passing a format, at which point the entire
> !has_format condition should be skipped when called by libvirt.

Heh, actually that is basically what I have now, as with the doc change
I forgot to git add it.  Doh!  I'll repost.. I'm also not opposed to
format being non-optional.

	Ian

Re: [Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none.

[Ian Main]

On Thu, Jul 18, 2013 at 11:32:52AM -0600, Eric Blake wrote:
> On 07/18/2013 11:27 AM, Eric Blake wrote:
> 
> >>      if (!has_format) {
> >> -        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name;
> >> +        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : "qcow2";
> > 
> > Is this the right thing to do?  Or should we do:
> > 
> > if (!has_format) {
> >     if (mode == NEW_IMAGE_MODE_EXISTING) {
> >         format = NULL;
> >     } else {
> >         format = bs->drv->format_name ?: "qcow2";
> >     }
> > }
> > 
> > That is, I think we should default to doing a backup in the format given
> > by the original (what if the original is qed, which also supports
> > backing files), and only use qcow2 when there is no guidance whatsoever.
> > 
> > But in practice, I don't care
> 
> Well, I _DO_ care about one thing - make sure that the qapi-schema.json
> page accurately documents how this variable is defaulted for callers
> that don't care about the implications of omitting a format.
> 
> Or we could simplify life by making 'format' mandatory for drive-backup;
> it was optional for 'drive-mirror' due to incremental implementation,
> but for 'drive-backup', we still have the opportunity to do things right
> from the first release.

Ah, I did make a doc change, I must have forgotten to add it.

I'm all for making format mandatory if that is ok with everyone.  Maybe
that is the best solution.

	Ian

Re: [Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]

On 07/18/2013 11:27 AM, Eric Blake wrote:

>>      if (!has_format) {
>> -        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name;
>> +        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : "qcow2";
> 
> Is this the right thing to do?  Or should we do:
> 
> if (!has_format) {
>     if (mode == NEW_IMAGE_MODE_EXISTING) {
>         format = NULL;
>     } else {
>         format = bs->drv->format_name ?: "qcow2";
>     }
> }
> 
> That is, I think we should default to doing a backup in the format given
> by the original (what if the original is qed, which also supports
> backing files), and only use qcow2 when there is no guidance whatsoever.
> 
> But in practice, I don't care

Well, I _DO_ care about one thing - make sure that the qapi-schema.json
page accurately documents how this variable is defaulted for callers
that don't care about the implications of omitting a format.

Or we could simplify life by making 'format' mandatory for drive-backup;
it was optional for 'drive-mirror' due to incremental implementation,
but for 'drive-backup', we still have the opportunity to do things right
from the first release.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1598 bytes --]

On 07/17/2013 02:04 PM, Ian Main wrote:
> qcow2 supports backing files so it makes sense to default to qcow2
> for MIRROR_SYNC_MODE_NONE so that we can use the source as a backing
> drive and export it via nbd.  Defaulting FULL and TOP to SYNC_MODE_NONE
> breaks tests but that could be fixed if we wanted it.
> 
> Signed-off-by: Ian Main <imain@redhat.com>
> ---
>  blockdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 000dea6..805b0e5 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1462,7 +1462,7 @@ void qmp_drive_backup(const char *device, const char *target,
>      }
>  
>      if (!has_format) {
> -        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name;
> +        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : "qcow2";

Is this the right thing to do?  Or should we do:

if (!has_format) {
    if (mode == NEW_IMAGE_MODE_EXISTING) {
        format = NULL;
    } else {
        format = bs->drv->format_name ?: "qcow2";
    }
}

That is, I think we should default to doing a backup in the format given
by the original (what if the original is qed, which also supports
backing files), and only use qcow2 when there is no guidance whatsoever.

But in practice, I don't care - format probing is a security hole, so
libvirt should always be passing a format, at which point the entire
!has_format condition should be skipped when called by libvirt.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCH V4 4/4] Change default to qcow2 for sync mode none.

[Ian Main]

qcow2 supports backing files so it makes sense to default to qcow2
for MIRROR_SYNC_MODE_NONE so that we can use the source as a backing
drive and export it via nbd.  Defaulting FULL and TOP to SYNC_MODE_NONE
breaks tests but that could be fixed if we wanted it.

Signed-off-by: Ian Main <imain@redhat.com>
---
 blockdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 000dea6..805b0e5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1462,7 +1462,7 @@ void qmp_drive_backup(const char *device, const char *target,
     }
 
     if (!has_format) {
-        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name;
+        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : "qcow2";
     }
     if (format) {
         drv = bdrv_find_format(format);
-- 
1.8.1.4