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

2012-02-27 Thread Markus Armbruster 
Kevin Wolf kw...@redhat.com writes:

 Am 24.02.2012 12:37, schrieb Federico Simoncelli:
[...]
 diff --git a/blockdev.c b/blockdev.c
 index 2c132a3..1df2542 100644
 --- a/blockdev.c
 +++ b/blockdev.c
[...]
 +void qmp_blockdev_migrate(const char *device, BlockMigrateOp mode,
 +  const char *destination, bool has_new_image_file,
 +  const char *new_image_file, Error **errp)
 +{
 +BlockDriverState *bs;
 +BlockDriver *drv;
 +int i, j, escape;
 +char filename[2048];
 +
 +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 (mode == BLOCK_MIGRATE_OP_MIRROR) {

 Move into a separate function?

 +drv = bdrv_find_format(blkmirror);
 +if (!drv) {
 +error_set(errp, QERR_INVALID_BLOCK_FORMAT, blkmirror);
 +return;
 +}
 +
 +if (!has_new_image_file) {
 +new_image_file = bs-filename;
 +}
 +
 +pstrcpy(filename, sizeof(filename), blkmirror:);
 +i = strlen(blkmirror:);
 +
 +escape = 0;
 +for (j = 0; j  strlen(new_image_file); j++) {
 + loop:
 +if (!(i  sizeof(filename) - 2)) {
 +error_set(errp, QERR_INVALID_PARAMETER_VALUE,
 +  new-image-file, shorter filename);
 +return;
 +}
 +
 +if (new_image_file[j] == ':' || new_image_file[j] == '\\') {

 Markus suggested that using comma for the separator is better even
 though it requires escaping. It would allow to parse the option string
 with QemuOpts.

Yes, I did.  While I'm no particular friend of QemuOpts, inventing more
ad hoc syntaxes is even worse.

On the other hand, many block drivers already do, often a colon syntax
similar to yours.  But not all.  I guess consistency is a lost cause
there, as is generality (support for arbitrary filenames).  Maybe it's
best to concede defeat, do another ad hoc syntax now, and hope we can
replace the whole -drive mess by something saner eventually.

 +if (!escape) {
 +filename[i++] = '\\', escape = 1;
 +goto loop;
 +} else {
 +escape = 0;
 +}
 +}
 +
 +filename[i++] = new_image_file[j];
 +}

 Looks like a string helper for qemu-option.c (it contains the parser, so
 keeping the escaping nearby would make sense).

 +
 +if (i + strlen(destination) + 2  sizeof(filename)) {
 +error_set(errp, QERR_INVALID_PARAMETER_VALUE,
 +  destination, shorter filename);
 +return;
 +}
 +
 +filename[i++] = ':';
 +pstrcpy(filename + i, sizeof(filename) - i - 2, destination);
 +
 +change_blockdev_image(device, filename, blkmirror, false, errp);
 +} else if (mode == BLOCK_MIGRATE_OP_STREAM) {
 +error_set(errp, QERR_NOT_SUPPORTED);

 Why even define it then?

 +}
 +}
 +
  static void eject_device(BlockDriverState *bs, int force, Error **errp)
  {
  if (bdrv_in_use(bs)) {
 diff --git a/hmp-commands.hx b/hmp-commands.hx
 index 573b823..ccb1f62 100644
 --- a/hmp-commands.hx
 +++ b/hmp-commands.hx
 @@ -886,6 +886,42 @@ Snapshot device, using snapshot file as target if 
 provided
  ETEXI
  
  {
 +.name   = blkdev_reopen,

 NACK on the name. Let's reserve blkdev_*/blockdev_* for the proper API

Yes, please.

 (that we'll release with the qemu version that comes with Hurd 1.0).

/me hides.

 drive_* would align well with the existing drive_add/del commands.

Agree.

[...]
 diff --git a/qapi-schema.json b/qapi-schema.json
 index d02ee86..c86796a 100644
 --- a/qapi-schema.json
 +++ b/qapi-schema.json
 @@ -1136,6 +1136,69 @@
'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
  
  ##
 +# @blockdev-reopen
 +#
 +# Assigns a new image file to a device.
 +#
 +# @device: the name of the device for which we are chainging the image file.
 +#
 +# @new-image-file: the target of the new image. If the file doesn't exists 
 the
 +#  command will fail.
 +#
 +# @format: #optional the format of the new image, default is 'qcow2'.
 +#
 +# Returns: nothing on success
 +#  If @device is not a valid block device, DeviceNotFound
 +#  If @new-image-file can't be opened, OpenFileFailed
 +#  If @format is invalid, InvalidBlockFormat
 +#
 +# Since 1.1
 +##
 +
 +{ 'command': 'blockdev-reopen',
 +  'data': { 'device': 'str', 'new-image-file': 'str', '*format': 'str' } }

 Same consideration on the name.

 Also I think we should immediately mark the command as deprecated (I
 think there is precedence for it, though I can't remember which command
 it was). This is not an interface we'll want to keep long term.

Deprecated isn't

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

2012-02-24 Thread Kevin Wolf 
Am 24.02.2012 12:37, schrieb Federico Simoncelli:
 Signed-off-by: Federico Simoncelli fsimo...@redhat.com
 ---
  block/blkmirror.c |2 +-
  blockdev.c|  109 
 +++--
  hmp-commands.hx   |   36 +
  hmp.c |   30 ++
  hmp.h |2 +
  qapi-schema.json  |   63 ++
  6 files changed, 229 insertions(+), 13 deletions(-)
 
 diff --git a/block/blkmirror.c b/block/blkmirror.c
 index 39927c8..49e3381 100644
 --- a/block/blkmirror.c
 +++ b/block/blkmirror.c
 @@ -81,7 +81,7 @@ static int blkmirror_open(BlockDriverState *bs, const char 
 *filename, int flags)
  bdrv_delete(m-bs[0]);
  return -ENOMEM;
  }
 -ret = bdrv_open(m-bs[1], filename, flags, NULL);
 +ret = bdrv_open(m-bs[1], filename, flags | BDRV_O_NO_BACKING, NULL);
  if (ret  0) {
  bdrv_delete(m-bs[0]);
  return ret;

Was this hunk meant to be in patch 1?

 diff --git a/blockdev.c b/blockdev.c
 index 2c132a3..1df2542 100644
 --- a/blockdev.c
 +++ b/blockdev.c
 @@ -646,9 +646,8 @@ void do_commit(Monitor *mon, const QDict *qdict)
  }
  }
  
 -void qmp_blockdev_snapshot_sync(const char *device, const char 
 *snapshot_file,
 -bool has_format, const char *format,
 -Error **errp)
 +static void change_blockdev_image(const char *device, const char 
 *new_image_file,
 +  const char *format, bool create, Error 
 **errp)
  {
  BlockDriverState *bs;
  BlockDriver *drv, *old_drv, *proto_drv;
 @@ -671,7 +670,7 @@ void qmp_blockdev_snapshot_sync(const char *device, const 
 char *snapshot_file,
  old_drv = bs-drv;
  flags = bs-open_flags;
  
 -if (!has_format) {
 +if (!format) {
  format = qcow2;
  }
  
 @@ -681,24 +680,26 @@ void qmp_blockdev_snapshot_sync(const char *device, 
 const char *snapshot_file,
  return;
  }
  
 -proto_drv = bdrv_find_protocol(snapshot_file);
 +proto_drv = bdrv_find_protocol(new_image_file);
  if (!proto_drv) {
  error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
  return;
  }
  
 -ret = bdrv_img_create(snapshot_file, format, bs-filename,
 -  bs-drv-format_name, NULL, -1, flags);
 -if (ret) {
 -error_set(errp, QERR_UNDEFINED_ERROR);
 -return;
 +if (create) {
 +ret = bdrv_img_create(new_image_file, format, bs-filename,
 +  bs-drv-format_name, NULL, -1, flags);
 +if (ret) {
 +error_set(errp, QERR_UNDEFINED_ERROR);
 +return;
 +}
  }
  
  bdrv_drain_all();
  bdrv_flush(bs);
  
  bdrv_close(bs);
 -ret = bdrv_open(bs, snapshot_file, flags, drv);
 +ret = bdrv_open(bs, new_image_file, flags, drv);
  /*
   * If reopening the image file we just created fails, fall back
   * and try to re-open the original image. If that fails too, we
 @@ -709,11 +710,95 @@ void qmp_blockdev_snapshot_sync(const char *device, 
 const char *snapshot_file,
  if (ret != 0) {
  error_set(errp, QERR_OPEN_FILE_FAILED, old_filename);
  } else {
 -error_set(errp, QERR_OPEN_FILE_FAILED, snapshot_file);
 +error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
  }
  }
  }
  
 +void qmp_blockdev_reopen(const char *device, const char *new_image_file,
 + bool has_format, const char *format, Error **errp)
 +{
 +change_blockdev_image(device, new_image_file,
 +  has_format ? format : NULL, false, errp);
 +}
 +
 +void qmp_blockdev_snapshot_sync(const char *device, const char 
 *snapshot_file,
 +bool has_format, const char *format,
 +Error **errp)
 +{
 +change_blockdev_image(device, snapshot_file,
 +  has_format ? format : NULL, true, errp);
 +}
 +
 +void qmp_blockdev_migrate(const char *device, BlockMigrateOp mode,
 +  const char *destination, bool has_new_image_file,
 +  const char *new_image_file, Error **errp)
 +{
 +BlockDriverState *bs;
 +BlockDriver *drv;
 +int i, j, escape;
 +char filename[2048];
 +
 +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 (mode == BLOCK_MIGRATE_OP_MIRROR) {

Move into a separate function?

 +drv = bdrv_find_format(blkmirror);
 +if (!drv) {
 +error_set(errp, QERR_INVALID_BLOCK_FORMAT, blkmirror);
 +return;
 +}
 +
 +if (!has_new_image_file) {
 +new_image_file = bs-filename;
 +}
 +
 +

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

2012-02-24 Thread Federico Simoncelli 
- Original Message -
 From: Kevin Wolf kw...@redhat.com
 To: Federico Simoncelli fsimo...@redhat.com
 Cc: qemu-devel@nongnu.org, Marcelo Tosatti mtosa...@redhat.com, 
 lcapitul...@redhat.com, Paolo Bonzini
 pbonz...@redhat.com, Markus Armbruster arm...@redhat.com
 Sent: Friday, February 24, 2012 1:03:08 PM
 Subject: Re: [PATCH 2/2] Add the blockdev-reopen and blockdev-migrate commands
 
 Am 24.02.2012 12:37, schrieb Federico Simoncelli:
  Signed-off-by: Federico Simoncelli fsimo...@redhat.com
  ---
   block/blkmirror.c |2 +-
   blockdev.c|  109
   +++--
   hmp-commands.hx   |   36 +
   hmp.c |   30 ++
   hmp.h |2 +
   qapi-schema.json  |   63 ++
   6 files changed, 229 insertions(+), 13 deletions(-)
  
  diff --git a/block/blkmirror.c b/block/blkmirror.c
  index 39927c8..49e3381 100644
  --- a/block/blkmirror.c
  +++ b/block/blkmirror.c
  @@ -81,7 +81,7 @@ static int blkmirror_open(BlockDriverState *bs,
  const char *filename, int flags)
   bdrv_delete(m-bs[0]);
   return -ENOMEM;
   }
  -ret = bdrv_open(m-bs[1], filename, flags, NULL);
  +ret = bdrv_open(m-bs[1], filename, flags | BDRV_O_NO_BACKING,
  NULL);
   if (ret  0) {
   bdrv_delete(m-bs[0]);
   return ret;
 
 Was this hunk meant to be in patch 1?

Not necessarily, I thought a lot about it. I didn't want to modify Marcelo's
patch too much. This flag is actually mandatory only to make blockdev-migrate
work with a destination that doesn't have a backing file yet, so I thought it
was more appropriate to squash it here.

-- 
Federico

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

2012-02-24 Thread Paolo Bonzini 
On 02/24/2012 01:03 PM, Kevin Wolf wrote:
 + loop:
 +if (!(i  sizeof(filename) - 2)) {
 +error_set(errp, QERR_INVALID_PARAMETER_VALUE,
 +  new-image-file, shorter filename);
 +return;
 +}
 +
 +if (new_image_file[j] == ':' || new_image_file[j] == '\\') {
 
 Markus suggested that using comma for the separator is better even
 though it requires escaping. It would allow to parse the option string
 with QemuOpts.

Isn't that a bit overengineering for an internal interface?

 +if (!escape) {
 +filename[i++] = '\\', escape = 1;
 +goto loop;
 +} else {
 +escape = 0;
 +}
 +}
 +
 +filename[i++] = new_image_file[j];
 +}
 
 Looks like a string helper for qemu-option.c (it contains the parser, so
 keeping the escaping nearby would make sense).
 
 +
 +if (i + strlen(destination) + 2  sizeof(filename)) {
 +error_set(errp, QERR_INVALID_PARAMETER_VALUE,
 +  destination, shorter filename);
 +return;
 +}
 +
 +filename[i++] = ':';
 +pstrcpy(filename + i, sizeof(filename) - i - 2, destination);
 +
 +change_blockdev_image(device, filename, blkmirror, false, errp);
 +} else if (mode == BLOCK_MIGRATE_OP_STREAM) {
 +error_set(errp, QERR_NOT_SUPPORTED);
 
 Why even define it then?

Because it's the default for the HMP.  Until we have live merging,
mirror mode cannot be consider anything more than a hack.

  ##
 +# @blockdev-reopen
 +#
 +# Assigns a new image file to a device.
 +#
 +# @device: the name of the device for which we are chainging the image file.
 +#
 +# @new-image-file: the target of the new image. If the file doesn't exists 
 the
 +#  command will fail.
 +#
 +# @format: #optional the format of the new image, default is 'qcow2'.
 +#
 +# Returns: nothing on success
 +#  If @device is not a valid block device, DeviceNotFound
 +#  If @new-image-file can't be opened, OpenFileFailed
 +#  If @format is invalid, InvalidBlockFormat
 +#
 +# Since 1.1
 +##
 +
 +{ 'command': 'blockdev-reopen',
 +  'data': { 'device': 'str', 'new-image-file': 'str', '*format': 'str' } }
 
 Same consideration on the name.
 
 Also I think we should immediately mark the command as deprecated (I
 think there is precedence for it, though I can't remember which command
 it was). This is not an interface we'll want to keep long term.

What about blockdev-snapshot-sync/snapshot_blkdev?

Paolo

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

2012-02-24 Thread Luiz Capitulino 
On Fri, 24 Feb 2012 13:03:08 +0100
Kevin Wolf kw...@redhat.com wrote:

   static void eject_device(BlockDriverState *bs, int force, Error **errp)
   {
   if (bdrv_in_use(bs)) {
  diff --git a/hmp-commands.hx b/hmp-commands.hx
  index 573b823..ccb1f62 100644
  --- a/hmp-commands.hx
  +++ b/hmp-commands.hx
  @@ -886,6 +886,42 @@ Snapshot device, using snapshot file as target if 
  provided
   ETEXI
   
   {
  +.name   = blkdev_reopen,
 
 NACK on the name. Let's reserve blkdev_*/blockdev_* for the proper API
 (that we'll release with the qemu version that comes with Hurd 1.0).

lol.

   ##
  +# @blockdev-reopen
  +#
  +# Assigns a new image file to a device.
  +#
  +# @device: the name of the device for which we are chainging the image 
  file.
  +#
  +# @new-image-file: the target of the new image. If the file doesn't exists 
  the
  +#  command will fail.
  +#
  +# @format: #optional the format of the new image, default is 'qcow2'.
  +#
  +# Returns: nothing on success
  +#  If @device is not a valid block device, DeviceNotFound
  +#  If @new-image-file can't be opened, OpenFileFailed
  +#  If @format is invalid, InvalidBlockFormat
  +#
  +# Since 1.1
  +##
  +
  +{ 'command': 'blockdev-reopen',
  +  'data': { 'device': 'str', 'new-image-file': 'str', '*format': 'str' } }
 
 Same consideration on the name.
 
 Also I think we should immediately mark the command as deprecated (I
 think there is precedence for it, though I can't remember which command
 it was). This is not an interface we'll want to keep long term.

We can only mark an interface as deprecated when there's a substitute for it.