Re: [Qemu-devel] [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate commands

[Luiz Capitulino <lcapitulino@redhat.com>]

On Mon, 27 Feb 2012 06:29:39 -0500 (EST)
Federico Simoncelli <fsimonce@redhat.com> wrote:

> ----- Original Message -----
> > From: "Luiz Capitulino" <lcapitulino@redhat.com>
> > To: "Federico Simoncelli" <fsimonce@redhat.com>
> > 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 <fsimonce@redhat.com> wrote:
> > 
> > > Signed-off-by: Federico Simoncelli <fsimonce@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?