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

[Federico Simoncelli]

----- Original Message -----
> From: "Luiz Capitulino" <address@hidden>
> To: "Federico Simoncelli" <address@hidden>
> Cc: address@hidden, address@hidden, address@hidden, address@hidden, 
> address@hidden
> 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 <address@hidden> wrote:
> 
> > Signed-off-by: Federico Simoncelli <address@hidden>
> 
> 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.

> 
> > +    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).

> > +    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?
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.

-- 
Federico