| Message ID | 1374091462-18391-5-git-send-email-imain@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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
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
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.
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);
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(-)