On Mon, 27 Feb 2012 06:29:39 -0500 (EST)
Federico Simoncelli <fsimo...@redhat.com> wrote:
> ----- Original Message -----
> > From: "Luiz Capitulino" <lcapitul...@redhat.com>
> > To: "Federico Simoncelli" <fsimo...@redhat.com>
> > Cc: qemu-devel@nongnu.org, mtosa...@redhat.com, pbonz...@redhat.com,
> > kw...@redhat.com, arm...@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 <fsimo...@redhat.com> wrote:
> >
> > > Signed-off-by: Federico Simoncelli <fsimo...@redhat.com>
> >
> > 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?
