Re: [Qemu-devel] [PATCH 1/2 v2] Add blkmirror block driver

2012-02-27 Thread Federico Simoncelli 
- 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 7:17:22 PM
 Subject: Re: [PATCH 1/2 v2] Add blkmirror block driver
 
 On Fri, 24 Feb 2012 16:49:03 +
 Federico Simoncelli fsimo...@redhat.com wrote:
 
  +/* Parse the raw image filename */
  +filename2 = g_malloc(strlen(filename)+1);
  +escape = 0;
  +for (i = n = 0; i  strlen(filename); i++) {
  +if (!escape  filename[i] == ':') {
  +break;
  +}
  +if (!escape  filename[i] == '\\') {
  +escape = 1;
  +} else {
  +escape = 0;
  +}
  +
  +if (!escape) {
  +filename2[n++] = filename[i];
  +}
  +}
  +filename2[n] = '\0';
 
 You're escaping only the first image name string, is that
 intentional?

Yes, it was also documented here by Marcelo:

  diff --git a/docs/blkmirror.txt b/docs/blkmirror.txt
  new file mode 100644
  index 000..cf73f3f
  --- /dev/null
  +++ b/docs/blkmirror.txt
  @@ -0,0 +1,16 @@
  +Block mirror driver
  +---
  +
  +This driver will mirror writes to two distinct images.
  +It's used internally by live block copy.
  +
  +Format
  +--
  +
  +blkmirror:/image1.img:/image2.img
  +
  +'\' (backslash) can be used to escape colon processing
  +as a separator, in the first image filename.
  +Backslashes themselves also can be escaped as '\\'.
  +
  +

Anyway this format is encapsulated by blockdev-migrate.

-- 
Federico

[Qemu-devel] [PATCH 1/2 v2] Add blkmirror block driver

2012-02-24 Thread Federico Simoncelli 
From: Marcelo Tosatti mtosa...@redhat.com

Mirrored writes are used by live block copy.

Signed-off-by: Marcelo Tosatti mtosa...@redhat.com
Signed-off-by: Federico Simoncelli fsimo...@redhat.com
---
 Makefile.objs  |2 +-
 block/blkmirror.c  |  247 
 docs/blkmirror.txt |   16 
 3 files changed, 264 insertions(+), 1 deletions(-)
 create mode 100644 block/blkmirror.c
 create mode 100644 docs/blkmirror.txt

diff --git a/Makefile.objs b/Makefile.objs
index 67ee3df..6020308 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -34,7 +34,7 @@ block-nested-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o 
dmg.o bochs.o vpc.o vv
 block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o 
qcow2-cache.o
 block-nested-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-nested-y += qed-check.o
-block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
+block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o 
blkmirror.o
 block-nested-y += stream.o
 block-nested-$(CONFIG_WIN32) += raw-win32.o
 block-nested-$(CONFIG_POSIX) += raw-posix.o
diff --git a/block/blkmirror.c b/block/blkmirror.c
new file mode 100644
index 000..49e3381
--- /dev/null
+++ b/block/blkmirror.c
@@ -0,0 +1,247 @@
+/*
+ * Block driver for mirrored writes.
+ *
+ * Copyright (C) 2011 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include stdarg.h
+#include block_int.h
+
+typedef struct {
+BlockDriverState *bs[2];
+} BdrvMirrorState;
+
+typedef struct DupAIOCB DupAIOCB;
+
+typedef struct SingleAIOCB {
+BlockDriverAIOCB *aiocb;
+int finished;
+DupAIOCB *parent;
+} SingleAIOCB;
+
+struct DupAIOCB {
+BlockDriverAIOCB common;
+int count;
+
+BlockDriverCompletionFunc *cb;
+SingleAIOCB aios[2];
+int ret;
+};
+
+/* Valid blkmirror filenames look like
+ * blkmirror:path/to/image1:path/to/image2 */
+static int blkmirror_open(BlockDriverState *bs, const char *filename, int 
flags)
+{
+BdrvMirrorState *m = bs-opaque;
+int ret, escape, i, n;
+char *filename2;
+
+/* Parse the blkmirror: prefix */
+if (strncmp(filename, blkmirror:, strlen(blkmirror:))) {
+return -EINVAL;
+}
+filename += strlen(blkmirror:);
+
+/* Parse the raw image filename */
+filename2 = g_malloc(strlen(filename)+1);
+escape = 0;
+for (i = n = 0; i  strlen(filename); i++) {
+if (!escape  filename[i] == ':') {
+break;
+}
+if (!escape  filename[i] == '\\') {
+escape = 1;
+} else {
+escape = 0;
+}
+
+if (!escape) {
+filename2[n++] = filename[i];
+}
+}
+filename2[n] = '\0';
+
+m-bs[0] = bdrv_new();
+if (m-bs[0] == NULL) {
+g_free(filename2);
+return -ENOMEM;
+}
+ret = bdrv_open(m-bs[0], filename2, flags, NULL);
+g_free(filename2);
+if (ret  0) {
+return ret;
+}
+filename += i + 1;
+
+m-bs[1] = bdrv_new();
+if (m-bs[1] == NULL) {
+bdrv_delete(m-bs[0]);
+return -ENOMEM;
+}
+ret = bdrv_open(m-bs[1], filename, flags | BDRV_O_NO_BACKING, NULL);
+if (ret  0) {
+bdrv_delete(m-bs[0]);
+return ret;
+}
+
+return 0;
+}
+
+static void blkmirror_close(BlockDriverState *bs)
+{
+BdrvMirrorState *m = bs-opaque;
+int i;
+
+for (i = 0; i  2; i++) {
+bdrv_delete(m-bs[i]);
+m-bs[i] = NULL;
+}
+}
+
+static coroutine_fn int blkmirror_co_flush(BlockDriverState *bs)
+{
+BdrvMirrorState *m = bs-opaque;
+int ret;
+
+ret = bdrv_co_flush(m-bs[0]);
+if (ret  0) {
+return ret;
+}
+
+return bdrv_co_flush(m-bs[1]);
+}
+
+static int64_t blkmirror_getlength(BlockDriverState *bs)
+{
+BdrvMirrorState *m = bs-opaque;
+
+return bdrv_getlength(m-bs[0]);
+}
+
+static BlockDriverAIOCB *blkmirror_aio_readv(BlockDriverState *bs,
+ int64_t sector_num,
+ QEMUIOVector *qiov,
+ int nb_sectors,
+ BlockDriverCompletionFunc *cb,
+ void *opaque)
+{
+BdrvMirrorState *m = bs-opaque;
+return bdrv_aio_readv(m-bs[0], sector_num, qiov, nb_sectors, cb, opaque);
+}
+
+static void dup_aio_cancel(BlockDriverAIOCB *blockacb)
+{
+DupAIOCB *acb = container_of(blockacb, DupAIOCB, common);
+int i;
+
+for (i = 0 ; i  2; i++) {
+if (!acb-aios[i].finished) {
+bdrv_aio_cancel(acb-aios[i].aiocb);
+}
+}
+qemu_aio_release(acb);
+}
+
+static AIOPool dup_aio_pool = {
+.aiocb_size = sizeof(DupAIOCB),
+.cancel = dup_aio_cancel,
+};
+
+static void blkmirror_aio_cb(void

Re: [Qemu-devel] [PATCH 1/2 v2] Add blkmirror block driver

2012-02-24 Thread Eric Blake 
On 02/24/2012 09:49 AM, Federico Simoncelli wrote:
 From: Marcelo Tosatti mtosa...@redhat.com
 
 Mirrored writes are used by live block copy.
 
 Signed-off-by: Marcelo Tosatti mtosa...@redhat.com
 Signed-off-by: Federico Simoncelli fsimo...@redhat.com
 ---

 +++ b/block/blkmirror.c
 @@ -0,0 +1,247 @@
 +/*
 + * Block driver for mirrored writes.
 + *
 + * Copyright (C) 2011 Red Hat, Inc.

It's 2012 now.

 +++ b/docs/blkmirror.txt
 @@ -0,0 +1,16 @@
 +Block mirror driver
 +---
 +
 +This driver will mirror writes to two distinct images.
 +It's used internally by live block copy.
 +
 +Format
 +--
 +
 +blkmirror:/image1.img:/image2.img
 +
 +'\' (backslash) can be used to escape colon processing
 +as a separator, in the first image filename.
 +Backslashes themselves also can be escaped as '\\'.

Is the escaping of : and \ only necessary for image1.img, or for both
image1 and image2?  I need to know if the parser is consistent for both
arguments, but this wording makes it sound like it is only for the first
argument.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 1/2 v2] Add blkmirror block driver

2012-02-24 Thread Federico Simoncelli 
- Original Message -
 From: Eric Blake ebl...@redhat.com
 To: Federico Simoncelli fsimo...@redhat.com
 Cc: qemu-devel@nongnu.org, kw...@redhat.com, mtosa...@redhat.com, 
 arm...@redhat.com, lcapitul...@redhat.com,
 pbonz...@redhat.com
 Sent: Friday, February 24, 2012 6:02:36 PM
 Subject: Re: [Qemu-devel] [PATCH 1/2 v2] Add blkmirror block driver
  +++ b/docs/blkmirror.txt
  @@ -0,0 +1,16 @@
  +Block mirror driver
  +---
  +
  +This driver will mirror writes to two distinct images.
  +It's used internally by live block copy.
  +
  +Format
  +--
  +
  +blkmirror:/image1.img:/image2.img
  +
  +'\' (backslash) can be used to escape colon processing
  +as a separator, in the first image filename.
  +Backslashes themselves also can be escaped as '\\'.
 
 Is the escaping of : and \ only necessary for image1.img, or for both
 image1 and image2?  I need to know if the parser is consistent for
 both
 arguments, but this wording makes it sound like it is only for the
 first
 argument.

Yes it is only for the first argument.

-- 
Federico

Re: [Qemu-devel] [PATCH 1/2 v2] Add blkmirror block driver

2012-02-24 Thread Luiz Capitulino 
On Fri, 24 Feb 2012 16:49:03 +
Federico Simoncelli fsimo...@redhat.com wrote:

 From: Marcelo Tosatti mtosa...@redhat.com
 
 Mirrored writes are used by live block copy.
 
 Signed-off-by: Marcelo Tosatti mtosa...@redhat.com
 Signed-off-by: Federico Simoncelli fsimo...@redhat.com
 ---
  Makefile.objs  |2 +-
  block/blkmirror.c  |  247 
 
  docs/blkmirror.txt |   16 
  3 files changed, 264 insertions(+), 1 deletions(-)
  create mode 100644 block/blkmirror.c
  create mode 100644 docs/blkmirror.txt
 
 diff --git a/Makefile.objs b/Makefile.objs
 index 67ee3df..6020308 100644
 --- a/Makefile.objs
 +++ b/Makefile.objs
 @@ -34,7 +34,7 @@ block-nested-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o 
 dmg.o bochs.o vpc.o vv
  block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o 
 qcow2-cache.o
  block-nested-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
  block-nested-y += qed-check.o
 -block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
 +block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o 
 blkmirror.o
  block-nested-y += stream.o
  block-nested-$(CONFIG_WIN32) += raw-win32.o
  block-nested-$(CONFIG_POSIX) += raw-posix.o
 diff --git a/block/blkmirror.c b/block/blkmirror.c
 new file mode 100644
 index 000..49e3381
 --- /dev/null
 +++ b/block/blkmirror.c
 @@ -0,0 +1,247 @@
 +/*
 + * Block driver for mirrored writes.
 + *
 + * Copyright (C) 2011 Red Hat, Inc.
 + *
 + * This work is licensed under the terms of the GNU GPL, version 2 or later.
 + * See the COPYING file in the top-level directory.
 + */
 +
 +#include stdarg.h
 +#include block_int.h
 +
 +typedef struct {
 +BlockDriverState *bs[2];
 +} BdrvMirrorState;
 +
 +typedef struct DupAIOCB DupAIOCB;
 +
 +typedef struct SingleAIOCB {
 +BlockDriverAIOCB *aiocb;
 +int finished;
 +DupAIOCB *parent;
 +} SingleAIOCB;
 +
 +struct DupAIOCB {
 +BlockDriverAIOCB common;
 +int count;
 +
 +BlockDriverCompletionFunc *cb;
 +SingleAIOCB aios[2];
 +int ret;
 +};
 +
 +/* Valid blkmirror filenames look like
 + * blkmirror:path/to/image1:path/to/image2 */
 +static int blkmirror_open(BlockDriverState *bs, const char *filename, int 
 flags)
 +{
 +BdrvMirrorState *m = bs-opaque;
 +int ret, escape, i, n;
 +char *filename2;
 +
 +/* Parse the blkmirror: prefix */
 +if (strncmp(filename, blkmirror:, strlen(blkmirror:))) {
 +return -EINVAL;
 +}
 +filename += strlen(blkmirror:);

Minor: you can use strstart() here.

 +
 +/* Parse the raw image filename */
 +filename2 = g_malloc(strlen(filename)+1);
 +escape = 0;
 +for (i = n = 0; i  strlen(filename); i++) {
 +if (!escape  filename[i] == ':') {
 +break;
 +}
 +if (!escape  filename[i] == '\\') {
 +escape = 1;
 +} else {
 +escape = 0;
 +}
 +
 +if (!escape) {
 +filename2[n++] = filename[i];
 +}
 +}
 +filename2[n] = '\0';

You're escaping only the first image name string, is that intentional?

 +
 +m-bs[0] = bdrv_new();
 +if (m-bs[0] == NULL) {
 +g_free(filename2);
 +return -ENOMEM;
 +}
 +ret = bdrv_open(m-bs[0], filename2, flags, NULL);
 +g_free(filename2);
 +if (ret  0) {

Leaking m-bs[0]?

 +return ret;
 +}
 +filename += i + 1;
 +
 +m-bs[1] = bdrv_new();
 +if (m-bs[1] == NULL) {
 +bdrv_delete(m-bs[0]);
 +return -ENOMEM;
 +}
 +ret = bdrv_open(m-bs[1], filename, flags | BDRV_O_NO_BACKING, NULL);
 +if (ret  0) {
 +bdrv_delete(m-bs[0]);

What about m-bs[1]?

 +return ret;
 +}
 +
 +return 0;
 +}
 +
 +static void blkmirror_close(BlockDriverState *bs)
 +{
 +BdrvMirrorState *m = bs-opaque;
 +int i;
 +
 +for (i = 0; i  2; i++) {
 +bdrv_delete(m-bs[i]);
 +m-bs[i] = NULL;
 +}
 +}
 +
 +static coroutine_fn int blkmirror_co_flush(BlockDriverState *bs)
 +{
 +BdrvMirrorState *m = bs-opaque;
 +int ret;
 +
 +ret = bdrv_co_flush(m-bs[0]);
 +if (ret  0) {
 +return ret;
 +}
 +
 +return bdrv_co_flush(m-bs[1]);
 +}
 +
 +static int64_t blkmirror_getlength(BlockDriverState *bs)
 +{
 +BdrvMirrorState *m = bs-opaque;
 +
 +return bdrv_getlength(m-bs[0]);
 +}
 +
 +static BlockDriverAIOCB *blkmirror_aio_readv(BlockDriverState *bs,
 + int64_t sector_num,
 + QEMUIOVector *qiov,
 + int nb_sectors,
 + BlockDriverCompletionFunc *cb,
 + void *opaque)
 +{
 +BdrvMirrorState *m = bs-opaque;
 +return bdrv_aio_readv(m-bs[0], sector_num, qiov, nb_sectors, cb, 
 opaque);
 +}
 +
 +static void dup_aio_cancel(BlockDriverAIOCB *blockacb)
 +{

Re: [Qemu-devel] [PATCH 1/2 v2] Add blkmirror block driver

2012-02-24 Thread Paolo Bonzini 
On 02/24/2012 06:02 PM, Eric Blake wrote:
 Is the escaping of : and \ only necessary for image1.img, or for both
 image1 and image2?  I need to know if the parser is consistent for both
 arguments, but this wording makes it sound like it is only for the first
 argument.

libvirt is completely insulated from this.

Paolo