From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45808) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S1zRG-0006fT-Hy for qemu-devel@nongnu.org; Mon, 27 Feb 2012 07:12:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S1zRA-00080s-AS for qemu-devel@nongnu.org; Mon, 27 Feb 2012 07:12:22 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42961) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S1zRA-00080m-2f for qemu-devel@nongnu.org; Mon, 27 Feb 2012 07:12:16 -0500 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q1RCCEpP024433 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 27 Feb 2012 07:12:14 -0500 Date: Mon, 27 Feb 2012 09:12:15 -0300 From: Luiz Capitulino Message-ID: <20120227091215.7849e558@doriath.home> In-Reply-To: <8eaeb022-ea20-4823-886a-e629bce1c776@zmail16.collab.prod.int.phx2.redhat.com> References: <20120224170143.78f55d3e@doriath.home> <8eaeb022-ea20-4823-886a-e629bce1c776@zmail16.collab.prod.int.phx2.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate commands List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Federico Simoncelli Cc: kwolf@redhat.com, pbonzini@redhat.com, mtosatti@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com On Mon, 27 Feb 2012 06:29:39 -0500 (EST) Federico Simoncelli wrote: > ----- Original Message ----- > > From: "Luiz Capitulino" > > To: "Federico Simoncelli" > > Cc: qemu-devel@nongnu.org, mtosatti@redhat.com, pbonzini@redhat.com, kwolf@redhat.com, armbru@redhat.com > > Sent: Friday, February 24, 2012 8:01:43 PM > > Subject: Re: [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate commands > > > > On Fri, 24 Feb 2012 16:49:04 +0000 > > Federico Simoncelli wrote: > > > > > Signed-off-by: Federico Simoncelli > > > > Btw, would be nice to have a 0/2 intro email describing the feature > > and changelog > > info. > > Yes the 0/2 (actually 0/3) was sent at the beginning of the thread so you might > have missed it because you were added later on but I'm sure you can still find > it in the archives. I only found v1 iirc, but this is not important right now, as you're going to post v3 right? And that's going to have the intro :) > > > > > + BlockDriver *drv; > > > + int i, j, escape; > > > + char new_filename[2048], *filename; > > > > I'd use PATH_MAX for new_filename's size. > > Maybe PATH_MAX * 2 + 1? (To handle the case where all the characters should be > escaped). Well, I was discussing this with Eric and he thinks that a value of 4096 should be fine. I personally prefer using PATH_MAX because I don't believe I'm better at choosing a random value for this vs. using what the system provides me. Feel free to choose what you think is the best for this case. > > > + escape = 0; > > > + for (i = 0, j = 0; j < strlen(new_image_file); j++) { > > > + loop: > > > + if (!(i < sizeof(new_filename) - 2)) { > > > + error_set(errp, QERR_INVALID_PARAMETER_VALUE, > > > + "new-image-file", "shorter filename"); > > > + return; > > > + } > > > + > > > + if (new_image_file[j] == ':' || new_image_file[j] == '\\') > > > { > > > + if (!escape) { > > > + new_filename[i++] = '\\', escape = 1; > > > + goto loop; > > > + } else { > > > + escape = 0; > > > + } > > > + } > > > + > > > + new_filename[i++] = new_image_file[j]; > > > + } > > > > IMO, you could require the filename arguments to be escaped already. > > Can you be more explicit, who should escape it? Paolo thinks this should be done by the block layer, fine with me. > The only thing that comes to my mind right now is requiring the input of > blockdev-migrate already escaped but that would expose an internal format. > (I'd not recommend it). > > > > +void qmp_blockdev_migrate(const char *device, bool incremental, > > > + const char *destination, bool > > > has_new_image_file, > > > + const char *new_image_file, Error > > > **errp) > > > +{ > > > + BlockDriverState *bs; > > > + > > > + bs = bdrv_find(device); > > > + if (!bs) { > > > + error_set(errp, QERR_DEVICE_NOT_FOUND, device); > > > + return; > > > + } > > > + if (bdrv_in_use(bs)) { > > > + error_set(errp, QERR_DEVICE_IN_USE, device); > > > + return; > > > + } > > > + > > > + if (incremental) { > > > + if (!has_new_image_file) { > > > + error_set(errp, QERR_INVALID_PARAMETER_VALUE, > > > + "incremental", "a new image file"); > > > + } else { > > > + qmp_blockdev_mirror(device, destination, > > > new_image_file, errp); > > > + } > > > + } else { > > > + error_set(errp, QERR_NOT_SUPPORTED); > > > + } > > > > The command documentation says that 'incremental' and > > 'new_image_file' are > > optionals, but the code makes them required. Why? > > If I didn't make any mistake in the code I'm just enforcing that when > you specify "incremental" you also need a new image. > There are still other valid cases where they are optional. Which operation will be performed if 'incremental' is not passed? If it's passed, which operation will be performed if 'new_image_file' is not?