On Wed, Jul 24, 2013 at 02:32:53PM -0600, Eric Blake wrote: > On 07/24/2013 04:55 AM, Kevin Wolf wrote: > > > Unconditionally overriding format for NEW_IMAGE_MODE_EXISTING is > > definitely wrong. It's the user's choice which COW format to use for the > > backup image. There's no reason why it has to be the same format as the > > image that is being backed up. > > > > Before, bs->drv->format_name was a default for the case where a new > > image had to be created and no format was given; and the format of > > existing images could be probed. This is still what makes most sense to > > me. What's even the goal with this change?
Actually I think that code is wrong. If we are using NEW_IMAGE_MODE_EXISTING then format doesn't get used. We just end up using bdrv_open() below to open the existing image. Format should not be specified for an existing image. > Furthermore, I'm proposing that for 1.6, we should make the format > argument mandatory for drive-backup. We made it optional for > drive-mirror, to allow for probing, but there have been CVEs in the past > due to probing of a raw file gone wrong. We can always relax a > mandatory argument into an optional one in 1.7, if we decide that > probing can be done safely, but we can never turn an optional argument > into a mandatory one once the initial release bakes in the option. It > would make the code a lot simpler to just have a mandatory format > argument, instead of having to bake in and document hueristics on which > format is picked when the caller doesn't provide one. So I made format mandatory in the last patch but only for NEW_IMAGE_MODE_ABSOLUTE_PATHS. It actually doesn't make sense to specify the format of an existing image so I left it optional as an argument, but it will throw an error if it's not specified for the case where we create a new image. That make sense? Ian