From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40938) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V277j-0007ZH-Cw for qemu-devel@nongnu.org; Wed, 24 Jul 2013 18:01:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V277g-0002Eb-V0 for qemu-devel@nongnu.org; Wed, 24 Jul 2013 18:01:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52169) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V277g-0002EW-MA for qemu-devel@nongnu.org; Wed, 24 Jul 2013 18:01:28 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r6OM1Rcx031871 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 24 Jul 2013 18:01:27 -0400 Date: Wed, 24 Jul 2013 15:01:24 -0700 From: Ian Main Message-ID: <20130724220124.GF2744@gate.mains.priv> References: <1374530960-22031-1-git-send-email-imain@redhat.com> <1374530960-22031-2-git-send-email-imain@redhat.com> <20130724105543.GA3623@dhcp-200-207.str.redhat.com> <51F039F5.90109@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <51F039F5.90109@redhat.com> Subject: Re: [Qemu-devel] [PATCH V6 1/3] Implement sync modes for drive-backup. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Kevin Wolf , famz@redhat.com, qemu-devel@nongnu.org, rjones@redhat.com, stefanha@redhat.com, pbonzini@redhat.com 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