[Qemu-devel] [PATCH 0/7] introduce BSD-licensed block driver for "raw"

[Qemu-devel] [PATCH 0/7] introduce BSD-licensed block driver for "raw"

[Laszlo Ersek]

Paolo asked me to write such a driver based on his textual specification
alone. The first patch captures his email in full, the rest re-quotes
parts that are being implemented.

The tree compiles at each patch. The series passes "make check-block".

"block/raw.c" is not removed because I wanted to keep it out of my
series and out of my brain.

Disclaimer: I couldn't care less if the raw block driver was public
domain or AGPLv3+, as long as it qualifies as free software. I'm only
trying to do what Paolo asked of me.

Laszlo Ersek (7):
  add skeleton for BSD licensed "raw" BlockDriver
  raw_bsd: emit debug events in bdrv_co_readv() and bdrv_co_writev()
  raw_bsd: add raw_create()
  raw_bsd: introduce "special members"
  raw_bsd: add raw_create_options
  raw_bsd: register bdrv_raw
  switch raw block driver from "raw.o" to "raw_bsd.o"

 block/Makefile.objs |    2 +-
 block/raw_bsd.c     |  186 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 187 insertions(+), 1 deletions(-)
 create mode 100644 block/raw_bsd.c

[Qemu-devel] [PATCH 1/7] add skeleton for BSD licensed "raw" BlockDriver

[Laszlo Ersek]

On 08/05/13 15:03, Paolo Bonzini wrote:
>
>
> ----- Original Message -----
>> From: "Laszlo Ersek" <lersek@redhat.com>
>> To: "Paolo Bonzini" <pbonzini@redhat.com>
>> Sent: Monday, August 5, 2013 2:43:46 PM
>> Subject: Re: [PATCH 1/2] raw: add license header
>>
>> On 08/02/13 00:27, Paolo Bonzini wrote:
>>> On 08/01/2013 10:13 AM, Christoph Hellwig wrote:
>>>> On Wed, Jul 31, 2013 at 08:19:51AM +0200, Paolo Bonzini wrote:
>>>>> Most of the block layer is under the BSD license, thus it is
>>>>> reasonable to license block/raw.c the same way.  CCed people should
>>>>> ACK by replying with a Signed-off-by line.
>>>>
>>>> The coded was intended to be GPLv2.
>>>
>>> Laszlo, would you be willing to do clean-room reverse engineering?
>>>
>>> (No rants, please. :))
>>
>> What's the scope exactly?
>
> It's quite small, it's a file full of forwarders like
>
> static void raw_foo(BlockDriverState *bs)
> {
>     return bdrv_foo(bs->file);
> }
>
> It's 170 lines of code, all as boring as this.  I only picked you
> because I'm quite certain you have never seen the file (and the answer
> confirmed it).
>
> Basically:
>
> 1) BlockDriver is a struct in which these function members are
> interesting:
>
>     .bdrv_reopen_prepare
>     .bdrv_co_readv
>     .bdrv_co_writev
>     .bdrv_co_is_allocated
>     .bdrv_co_write_zeroes
>     .bdrv_co_discard
>     .bdrv_getlength
>     .bdrv_get_info
>     .bdrv_truncate
>     .bdrv_is_inserted
>     .bdrv_media_changed
>     .bdrv_eject
>     .bdrv_lock_medium
>     .bdrv_ioctl
>     .bdrv_aio_ioctl
>     .bdrv_has_zero_init
>
> They should be implemented as simple forwarders (see above).
> There are 16 functions listed here, you can easily see how this
> already accounts for 100+ SLOC roughly...
>
> The implementations of bdrv_co_readv and bdrv_co_writev should also
> call BLKDBG_EVENT on bs->file too, before forwarding to bs->file.  The
> events to be generated are BLKDBG_READ_AIO and BLKDBG_WRITE_AIO.
>
> 2) This is also a simple forwarder function:
>
>     .bdrv_create
>
> but there is no BlockDriverState argument so the forwarded-to function
> does not have a bs->file argument either.  The forwarded-to function
> is bdrv_create_file.
>
> 3) These members are special
>
>     .format_name   is the string "raw"
>     .bdrv_open     raw_open should set bs->sg to bs->file->sg and return 0
>     .bdrv_close    raw_close should do nothing
>     .bdrv_probe    raw_probe should just return 1.
>
> 4) There is another member, .create_options, which is an array of
> QEMUOptionParameter structs, terminated by an all-zero item.  The only
> option you need is for the virtual disk size.  You will find something
> to copy from in other block drivers, for example block/qcow2.c.
>
> 5) Formats are registered with bdrv_register (takes a BlockDriver*).
> You also need to pass the caller of bdrv_register to block_init.
>
> 6) I'm not sure how to organize the patch series, so I'll leave this to
> your creativity.  I guess in this case move/copy detection of git should
> be disabled.  I would definitely include this spec in the commit
> message as a proof of clean-room reverse engineering.
>
> 7) Remember a BSD header like the one in block.c.
>
> Paolo

This patch implements the email up to the paragraph ending with "100+ SLOC
roughly". The skeleton is generated from the list there, with a simple
shell loop using "sed" and the raw_foo() template.

The BSD license block is copied (and reflowed) from
"util/qemu-progress.c".

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 block/raw_bsd.c |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 108 insertions(+), 0 deletions(-)
 create mode 100644 block/raw_bsd.c

diff --git a/block/raw_bsd.c b/block/raw_bsd.c
new file mode 100644
index 0000000..5c17d53
--- /dev/null
+++ b/block/raw_bsd.c
@@ -0,0 +1,108 @@
+/* BlockDriver implementation for "raw"
+ *
+ * Copyright (C) 2013, Red Hat, Inc.
+ *
+ * Author:
+ *   Laszlo Ersek <lersek@redhat.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include "block/block_int.h"
+
+static TYPE raw_reopen_prepare(BlockDriverState *bs)
+{
+    return bdrv_reopen_prepare(bs->file);
+}
+
+static TYPE raw_co_readv(BlockDriverState *bs)
+{
+    return bdrv_co_readv(bs->file);
+}
+
+static TYPE raw_co_writev(BlockDriverState *bs)
+{
+    return bdrv_co_writev(bs->file);
+}
+
+static TYPE raw_co_is_allocated(BlockDriverState *bs)
+{
+    return bdrv_co_is_allocated(bs->file);
+}
+
+static TYPE raw_co_write_zeroes(BlockDriverState *bs)
+{
+    return bdrv_co_write_zeroes(bs->file);
+}
+
+static TYPE raw_co_discard(BlockDriverState *bs)
+{
+    return bdrv_co_discard(bs->file);
+}
+
+static TYPE raw_getlength(BlockDriverState *bs)
+{
+    return bdrv_getlength(bs->file);
+}
+
+static TYPE raw_get_info(BlockDriverState *bs)
+{
+    return bdrv_get_info(bs->file);
+}
+
+static TYPE raw_truncate(BlockDriverState *bs)
+{
+    return bdrv_truncate(bs->file);
+}
+
+static TYPE raw_is_inserted(BlockDriverState *bs)
+{
+    return bdrv_is_inserted(bs->file);
+}
+
+static TYPE raw_media_changed(BlockDriverState *bs)
+{
+    return bdrv_media_changed(bs->file);
+}
+
+static TYPE raw_eject(BlockDriverState *bs)
+{
+    return bdrv_eject(bs->file);
+}
+
+static TYPE raw_lock_medium(BlockDriverState *bs)
+{
+    return bdrv_lock_medium(bs->file);
+}
+
+static TYPE raw_ioctl(BlockDriverState *bs)
+{
+    return bdrv_ioctl(bs->file);
+}
+
+static TYPE raw_aio_ioctl(BlockDriverState *bs)
+{
+    return bdrv_aio_ioctl(bs->file);
+}
+
+static TYPE raw_has_zero_init(BlockDriverState *bs)
+{
+    return bdrv_has_zero_init(bs->file);
+}
+
-- 
1.7.1

[Qemu-devel] [PATCH 2/7] raw_bsd: emit debug events in bdrv_co_readv() and bdrv_co_writev()

[Laszlo Ersek]

On 08/05/13 15:03, Paolo Bonzini wrote:
>
> [...]
>
> 1) BlockDriver is a struct in which these function members are
> interesting:
>
>     .bdrv_reopen_prepare
>     .bdrv_co_readv
>     .bdrv_co_writev
>     .bdrv_co_is_allocated
>     .bdrv_co_write_zeroes
>     .bdrv_co_discard
>     .bdrv_getlength
>     .bdrv_get_info
>     .bdrv_truncate
>     .bdrv_is_inserted
>     .bdrv_media_changed
>     .bdrv_eject
>     .bdrv_lock_medium
>     .bdrv_ioctl
>     .bdrv_aio_ioctl
>     .bdrv_has_zero_init
>
> They should be implemented as simple forwarders (see above). There are
> 16 functions listed here, you can easily see how this already accounts
> for 100+ SLOC roughly...
>
> The implementations of bdrv_co_readv and bdrv_co_writev should also call
> BLKDBG_EVENT on bs->file too, before forwarding to bs->file.  The events
> to be generated are BLKDBG_READ_AIO and BLKDBG_WRITE_AIO.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 block/raw_bsd.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 5c17d53..19091a3 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -33,11 +33,13 @@ static TYPE raw_reopen_prepare(BlockDriverState *bs)
 
 static TYPE raw_co_readv(BlockDriverState *bs)
 {
+    BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
     return bdrv_co_readv(bs->file);
 }
 
 static TYPE raw_co_writev(BlockDriverState *bs)
 {
+    BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
     return bdrv_co_writev(bs->file);
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH 3/7] raw_bsd: add raw_create()

[Laszlo Ersek]

On 08/05/13 15:03, Paolo Bonzini wrote:
>
> [...]
>
> 2) This is also a simple forwarder function:
>
>     .bdrv_create
>
> but there is no BlockDriverState argument so the forwarded-to function
> does not have a bs->file argument either.  The forwarded-to function is
> bdrv_create_file.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 block/raw_bsd.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 19091a3..5bcbe71 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -108,3 +108,7 @@ static TYPE raw_has_zero_init(BlockDriverState *bs)
     return bdrv_has_zero_init(bs->file);
 }
 
+static TYPE raw_create(void)
+{
+    return bdrv_create_file();
+}
-- 
1.7.1

[Qemu-devel] [PATCH 4/7] raw_bsd: introduce "special members"

[Laszlo Ersek]

On 08/05/13 15:03, Paolo Bonzini wrote:
>
> [...]
>
> 3) These members are special
>
>     .format_name   is the string "raw"
>     .bdrv_open     raw_open should set bs->sg to bs->file->sg and return 0
>     .bdrv_close    raw_close should do nothing
>     .bdrv_probe    raw_probe should just return 1.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 block/raw_bsd.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 5bcbe71..86e018d 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -112,3 +112,23 @@ static TYPE raw_create(void)
 {
     return bdrv_create_file();
 }
+
+static const char *raw_format_name(void)
+{
+    return "raw";
+}
+
+static int raw_open(BlockDriverState *bs)
+{
+    bs->sg = bs->file->sg;
+    return 0;
+}
+
+static void raw_close(void)
+{
+}
+
+static int raw_probe(void)
+{
+    return 1;
+}
-- 
1.7.1

Re: [Qemu-devel] [PATCH 4/7] raw_bsd: introduce "special members"

[Kevin Wolf]

Am 16.08.2013 um 16:15 hat Laszlo Ersek geschrieben:
> On 08/05/13 15:03, Paolo Bonzini wrote:
> >
> > [...]
> >
> > 3) These members are special
> >
> >     .format_name   is the string "raw"
> >     .bdrv_open     raw_open should set bs->sg to bs->file->sg and return 0
> >     .bdrv_close    raw_close should do nothing
> >     .bdrv_probe    raw_probe should just return 1.
> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  block/raw_bsd.c |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index 5bcbe71..86e018d 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -112,3 +112,23 @@ static TYPE raw_create(void)
>  {
>      return bdrv_create_file();
>  }
> +
> +static const char *raw_format_name(void)
> +{
> +    return "raw";
> +}
> +
> +static int raw_open(BlockDriverState *bs)
> +{
> +    bs->sg = bs->file->sg;

I know that Paolo explicitly made this requirement, but I think it is
ugly. We should instead fix bdrv_is_sg() to look at bs->file if it
exists, like many other functions already do, and change the two readers
of bs->sg in block.c to use bdrv_is_sg().

> +    return 0;
> +}
> +
> +static void raw_close(void)
> +{
> +}
> +
> +static int raw_probe(void)
> +{
> +    return 1;
> +}

Maybe add a comment here like "smallest possible positive score so that
raw is used if and only if no other block driver works".

Kevin

[Qemu-devel] [PATCH 5/7] raw_bsd: add raw_create_options

[Laszlo Ersek]

On 08/05/13 15:03, Paolo Bonzini wrote:
>
> [...]
>
> 4) There is another member, .create_options, which is an array of
> QEMUOptionParameter structs, terminated by an all-zero item.  The only
> option you need is for the virtual disk size.  You will find something
> to copy from in other block drivers, for example block/qcow2.c.

Code taken and adapted from "block/qcow2.c", as suggested. The code being
copied/modified is blamed on

    commit 20d97356c9df6d68fbd37d6334fdb7063f24eab6
    Author: Blue Swirl <blauwirbel@gmail.com>
    Date:   Fri Apr 23 20:19:47 2010 +0000

        Fix OpenBSD build

and

    commit 7c80ab3f21f0b1342f23057d4345ae266c7348d9
    Author: Jes Sorensen <Jes.Sorensen@redhat.com>
    Date:   Fri Dec 17 16:02:39 2010 +0100

        block/qcow2.c: rename qcow_ functions to qcow2_

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 block/raw_bsd.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 86e018d..12debb2 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -1,6 +1,7 @@
 /* BlockDriver implementation for "raw"
  *
- * Copyright (C) 2013, Red Hat, Inc.
+ * Copyright (C) 2010, 2013, Red Hat, Inc.
+ * Copyright (C) 2010, Blue Swirl <blauwirbel@gmail.com>
  *
  * Author:
  *   Laszlo Ersek <lersek@redhat.com>
@@ -25,6 +26,16 @@
  */
 
 #include "block/block_int.h"
+#include "qemu/option.h"
+
+static const QEMUOptionParameter raw_create_options[] = {
+    {
+        .name = BLOCK_OPT_SIZE,
+        .type = OPT_SIZE,
+        .help = "Virtual disk size"
+    },
+    { 0 }
+};
 
 static TYPE raw_reopen_prepare(BlockDriverState *bs)
 {
-- 
1.7.1

[Qemu-devel] [PATCH 6/7] raw_bsd: register bdrv_raw

[Laszlo Ersek]

On 08/05/13 15:03, Paolo Bonzini wrote:
>
> [...]
>
> 5) Formats are registered with bdrv_register (takes a BlockDriver*). You
> also need to pass the caller of bdrv_register to block_init.

Fill in the BlockDriver structure with the raw_*() functions that have
been added to "block/raw_bsd.c", in the order the fields are defined in
"include/block/block_int.h".

I needed more explanation / naming examples for registering the driver
than what Paolo gave me, so I copied / adapted from "block/qcow2.c". The
parts I took as basis for modification are blamed on

    commit 5efa9d5a8b18841c9c62208a494d7f519238979a
    Author: Anthony Liguori <aliguori@us.ibm.com>
    Date:   Sat May 9 17:03:42 2009 -0500

        Convert block infrastructure to use new module init functionality

    commit 20d97356c9df6d68fbd37d6334fdb7063f24eab6
    Author: Blue Swirl <blauwirbel@gmail.com>
    Date:   Fri Apr 23 20:19:47 2010 +0000

        Fix OpenBSD build

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 block/raw_bsd.c |   38 +++++++++++++++++++++++++++++++++-----
 1 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 12debb2..79916c4 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -2,6 +2,7 @@
  *
  * Copyright (C) 2010, 2013, Red Hat, Inc.
  * Copyright (C) 2010, Blue Swirl <blauwirbel@gmail.com>
+ * Copyright (C) 2009, Anthony Liguori <aliguori@us.ibm.com>
  *
  * Author:
  *   Laszlo Ersek <lersek@redhat.com>
@@ -124,11 +125,6 @@ static TYPE raw_create(void)
     return bdrv_create_file();
 }
 
-static const char *raw_format_name(void)
-{
-    return "raw";
-}
-
 static int raw_open(BlockDriverState *bs)
 {
     bs->sg = bs->file->sg;
@@ -143,3 +139,35 @@ static int raw_probe(void)
 {
     return 1;
 }
+
+static BlockDriver bdrv_raw = {
+    .format_name          = "raw",
+    .bdrv_probe           = &raw_probe,
+    .bdrv_reopen_prepare  = &raw_reopen_prepare,
+    .bdrv_open            = &raw_open,
+    .bdrv_close           = &raw_close,
+    .bdrv_create          = &raw_create,
+    .bdrv_co_readv        = &raw_co_readv,
+    .bdrv_co_writev       = &raw_co_writev,
+    .bdrv_co_write_zeroes = &raw_co_write_zeroes,
+    .bdrv_co_discard      = &raw_co_discard,
+    .bdrv_co_is_allocated = &raw_co_is_allocated,
+    .bdrv_truncate        = &raw_truncate,
+    .bdrv_getlength       = &raw_getlength,
+    .bdrv_get_info        = &raw_get_info,
+    .bdrv_is_inserted     = &raw_is_inserted,
+    .bdrv_media_changed   = &raw_media_changed,
+    .bdrv_eject           = &raw_eject,
+    .bdrv_lock_medium     = &raw_lock_medium,
+    .bdrv_ioctl           = &raw_ioctl,
+    .bdrv_aio_ioctl       = &raw_aio_ioctl,
+    .create_options       = &raw_create_options[0],
+    .bdrv_has_zero_init   = &raw_has_zero_init
+};
+
+static void bdrv_raw_init(void)
+{
+    bdrv_register(&bdrv_raw);
+}
+
+block_init(bdrv_raw_init);
-- 
1.7.1

[Qemu-devel] [PATCH 7/7] switch raw block driver from "raw.o" to "raw_bsd.o"

[Laszlo Ersek]

"Incoming" function prototypes and "outgoing" function calls must match
reality. Implemented using the "struct BlockDriver" definition in
"include/block/block_int.h", and gcc errors & warnings.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 block/Makefile.objs |    2 +-
 block/raw_bsd.c     |   81 +++++++++++++++++++++++++++++---------------------
 2 files changed, 48 insertions(+), 35 deletions(-)

diff --git a/block/Makefile.objs b/block/Makefile.objs
index 4cf9aa4..3bb85b5 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -1,4 +1,4 @@
-block-obj-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vvfat.o
+block-obj-y += raw_bsd.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vvfat.o
 block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o
 block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-obj-y += qed-check.o
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 79916c4..07e6f06 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -29,7 +29,7 @@
 #include "block/block_int.h"
 #include "qemu/option.h"
 
-static const QEMUOptionParameter raw_create_options[] = {
+static QEMUOptionParameter raw_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,
         .type = OPT_SIZE,
@@ -38,104 +38,117 @@ static const QEMUOptionParameter raw_create_options[] = {
     { 0 }
 };
 
-static TYPE raw_reopen_prepare(BlockDriverState *bs)
+static int raw_reopen_prepare(BDRVReopenState *reopen_state,
+                              BlockReopenQueue *queue, Error **errp)
 {
-    return bdrv_reopen_prepare(bs->file);
+    BDRVReopenState tmp = *reopen_state;
+
+    tmp.bs = tmp.bs->file;
+    return bdrv_reopen_prepare(&tmp, queue, errp);
 }
 
-static TYPE raw_co_readv(BlockDriverState *bs)
+static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,
+                                     int nb_sectors, QEMUIOVector *qiov)
 {
     BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
-    return bdrv_co_readv(bs->file);
+    return bdrv_co_readv(bs->file, sector_num, nb_sectors, qiov);
 }
 
-static TYPE raw_co_writev(BlockDriverState *bs)
+static int coroutine_fn raw_co_writev(BlockDriverState *bs, int64_t sector_num,
+                                      int nb_sectors, QEMUIOVector *qiov)
 {
     BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
-    return bdrv_co_writev(bs->file);
+    return bdrv_co_writev(bs->file, sector_num, nb_sectors, qiov);
 }
 
-static TYPE raw_co_is_allocated(BlockDriverState *bs)
+static int coroutine_fn raw_co_is_allocated(BlockDriverState *bs,
+                                            int64_t sector_num, int nb_sectors,
+                                            int *pnum)
 {
-    return bdrv_co_is_allocated(bs->file);
+    return bdrv_co_is_allocated(bs->file, sector_num, nb_sectors, pnum);
 }
 
-static TYPE raw_co_write_zeroes(BlockDriverState *bs)
+static int coroutine_fn raw_co_write_zeroes(BlockDriverState *bs,
+                                            int64_t sector_num, int nb_sectors)
 {
-    return bdrv_co_write_zeroes(bs->file);
+    return bdrv_co_write_zeroes(bs->file, sector_num, nb_sectors);
 }
 
-static TYPE raw_co_discard(BlockDriverState *bs)
+static int coroutine_fn raw_co_discard(BlockDriverState *bs,
+                                       int64_t sector_num, int nb_sectors)
 {
-    return bdrv_co_discard(bs->file);
+    return bdrv_co_discard(bs->file, sector_num, nb_sectors);
 }
 
-static TYPE raw_getlength(BlockDriverState *bs)
+static int64_t raw_getlength(BlockDriverState *bs)
 {
     return bdrv_getlength(bs->file);
 }
 
-static TYPE raw_get_info(BlockDriverState *bs)
+static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
-    return bdrv_get_info(bs->file);
+    return bdrv_get_info(bs->file, bdi);
 }
 
-static TYPE raw_truncate(BlockDriverState *bs)
+static int raw_truncate(BlockDriverState *bs, int64_t offset)
 {
-    return bdrv_truncate(bs->file);
+    return bdrv_truncate(bs->file, offset);
 }
 
-static TYPE raw_is_inserted(BlockDriverState *bs)
+static int raw_is_inserted(BlockDriverState *bs)
 {
     return bdrv_is_inserted(bs->file);
 }
 
-static TYPE raw_media_changed(BlockDriverState *bs)
+static int raw_media_changed(BlockDriverState *bs)
 {
     return bdrv_media_changed(bs->file);
 }
 
-static TYPE raw_eject(BlockDriverState *bs)
+static void raw_eject(BlockDriverState *bs, bool eject_flag)
 {
-    return bdrv_eject(bs->file);
+    bdrv_eject(bs->file, eject_flag);
 }
 
-static TYPE raw_lock_medium(BlockDriverState *bs)
+static void raw_lock_medium(BlockDriverState *bs, bool locked)
 {
-    return bdrv_lock_medium(bs->file);
+    bdrv_lock_medium(bs->file, locked);
 }
 
-static TYPE raw_ioctl(BlockDriverState *bs)
+static int raw_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
 {
-    return bdrv_ioctl(bs->file);
+    return bdrv_ioctl(bs->file, req, buf);
 }
 
-static TYPE raw_aio_ioctl(BlockDriverState *bs)
+static BlockDriverAIOCB *raw_aio_ioctl(BlockDriverState *bs,
+                                       unsigned long int req, void *buf,
+                                       BlockDriverCompletionFunc *cb,
+                                       void *opaque)
 {
-    return bdrv_aio_ioctl(bs->file);
+    return bdrv_aio_ioctl(bs->file, req, buf, cb, opaque);
 }
 
-static TYPE raw_has_zero_init(BlockDriverState *bs)
+static int raw_has_zero_init(BlockDriverState *bs)
 {
     return bdrv_has_zero_init(bs->file);
 }
 
-static TYPE raw_create(void)
+static int raw_create(const char *filename, QEMUOptionParameter *options)
 {
-    return bdrv_create_file();
+    return bdrv_create_file(filename, options);
 }
 
-static int raw_open(BlockDriverState *bs)
+static int raw_open(BlockDriverState *bs, QDict *options, int flags)
 {
     bs->sg = bs->file->sg;
     return 0;
 }
 
-static void raw_close(void)
+static void raw_close(BlockDriverState *bs)
 {
 }
 
-static int raw_probe(void)
+static int raw_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
     return 1;
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 7/7] switch raw block driver from "raw.o" to "raw_bsd.o"

[Paolo Bonzini]

Il 16/08/2013 16:15, Laszlo Ersek ha scritto:
> +static int raw_reopen_prepare(BDRVReopenState *reopen_state,
> +                              BlockReopenQueue *queue, Error **errp)
>  {
> -    return bdrv_reopen_prepare(bs->file);
> +    BDRVReopenState tmp = *reopen_state;
> +
> +    tmp.bs = tmp.bs->file;
> +    return bdrv_reopen_prepare(&tmp, queue, errp);
>  }

This should just return zero, my fault.

Paolo

Re: [Qemu-devel] [PATCH 7/7] switch raw block driver from "raw.o" to "raw_bsd.o"

[Kevin Wolf]

Am 18.08.2013 um 16:29 hat Paolo Bonzini geschrieben:
> Il 16/08/2013 16:15, Laszlo Ersek ha scritto:
> > +static int raw_reopen_prepare(BDRVReopenState *reopen_state,
> > +                              BlockReopenQueue *queue, Error **errp)
> >  {
> > -    return bdrv_reopen_prepare(bs->file);
> > +    BDRVReopenState tmp = *reopen_state;
> > +
> > +    tmp.bs = tmp.bs->file;
> > +    return bdrv_reopen_prepare(&tmp, queue, errp);
> >  }
> 
> This should just return zero, my fault.

Which is because bdrv_reopen_queue() already queues bs->file for reopen.
The simple return 0; implementation is shared by all other format drivers
that support reopening images.

Kevin

Re: [Qemu-devel] [PATCH 0/7] introduce BSD-licensed block driver for "raw"

[Anthony Liguori]

Laszlo Ersek <lersek@redhat.com> writes:

> Paolo asked me to write such a driver based on his textual specification
> alone. The first patch captures his email in full, the rest re-quotes
> parts that are being implemented.
>
> The tree compiles at each patch. The series passes "make check-block".
>
> "block/raw.c" is not removed because I wanted to keep it out of my
> series and out of my brain.
>
> Disclaimer: I couldn't care less if the raw block driver was public
> domain or AGPLv3+, as long as it qualifies as free software. I'm only
> trying to do what Paolo asked of me.

Generally speaking, rewriting parts of QEMU to be !GPL is something I
would strongly, strongly oppose.

I believe that Paolo had a good reason for this though.  I suppose the
logic is that we want to expose a "libqemublock" that libvirt can use such
that it can stop parsing qcow2 files.

Now libvirt just needs LGPLv2+, right?

Is the JSON mode of qemu-img info not sufficient for libvirt's purposes?

Is there additional logic behind having a libqemublock?

Regards,

Anthony Liguori

>
> Laszlo Ersek (7):
>   add skeleton for BSD licensed "raw" BlockDriver
>   raw_bsd: emit debug events in bdrv_co_readv() and bdrv_co_writev()
>   raw_bsd: add raw_create()
>   raw_bsd: introduce "special members"
>   raw_bsd: add raw_create_options
>   raw_bsd: register bdrv_raw
>   switch raw block driver from "raw.o" to "raw_bsd.o"
>
>  block/Makefile.objs |    2 +-
>  block/raw_bsd.c     |  186 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 187 insertions(+), 1 deletions(-)
>  create mode 100644 block/raw_bsd.c

Re: [Qemu-devel] [PATCH 0/7] introduce BSD-licensed block driver for "raw"

[Daniel P. Berrange]

On Fri, Aug 16, 2013 at 09:59:07AM -0500, Anthony Liguori wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
> 
> > Paolo asked me to write such a driver based on his textual specification
> > alone. The first patch captures his email in full, the rest re-quotes
> > parts that are being implemented.
> >
> > The tree compiles at each patch. The series passes "make check-block".
> >
> > "block/raw.c" is not removed because I wanted to keep it out of my
> > series and out of my brain.
> >
> > Disclaimer: I couldn't care less if the raw block driver was public
> > domain or AGPLv3+, as long as it qualifies as free software. I'm only
> > trying to do what Paolo asked of me.
> 
> Generally speaking, rewriting parts of QEMU to be !GPL is something I
> would strongly, strongly oppose.
> 
> I believe that Paolo had a good reason for this though.  I suppose the
> logic is that we want to expose a "libqemublock" that libvirt can use such
> that it can stop parsing qcow2 files.
> 
> Now libvirt just needs LGPLv2+, right?

LGPLv2+ is fine, but regardless of license, libvirt won't use any
libqemublock.so library as long as it links to glib with abort on
OOM behaviour.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 0/7] introduce BSD-licensed block driver for "raw"

[Paolo Bonzini]

Il 16/08/2013 16:59, Anthony Liguori ha scritto:
> Laszlo Ersek <lersek@redhat.com> writes:
> 
>> Paolo asked me to write such a driver based on his textual specification
>> alone. The first patch captures his email in full, the rest re-quotes
>> parts that are being implemented.
>>
>> The tree compiles at each patch. The series passes "make check-block".
>>
>> "block/raw.c" is not removed because I wanted to keep it out of my
>> series and out of my brain.
>>
>> Disclaimer: I couldn't care less if the raw block driver was public
>> domain or AGPLv3+, as long as it qualifies as free software. I'm only
>> trying to do what Paolo asked of me.
> 
> Generally speaking, rewriting parts of QEMU to be !GPL is something I
> would strongly, strongly oppose.
> 
> I believe that Paolo had a good reason for this though.

The reason is that Christoph said his original version of block/raw.c
was meant to be GPLv2-only.  I don't care if the file is BSD or LGPLv2+,
but most of the block layer is BSD, which is why I went for BSD.

It's been a while since I audited the files that would go into
libqemublock, but I remember the only problematic files were block/raw.c
(unlicensed) and block/vdi.c (GPLv2+).  Not having VirtualBox support
wouldn't be a big deal.

Paolo

Re: [Qemu-devel] [PATCH 0/7] introduce BSD-licensed block driver for "raw"

[Laszlo Ersek]

On 08/18/13 16:25, Paolo Bonzini wrote:
> Il 16/08/2013 16:59, Anthony Liguori ha scritto:
>> Laszlo Ersek <lersek@redhat.com> writes:
>>
>>> Paolo asked me to write such a driver based on his textual specification
>>> alone. The first patch captures his email in full, the rest re-quotes
>>> parts that are being implemented.
>>>
>>> The tree compiles at each patch. The series passes "make check-block".
>>>
>>> "block/raw.c" is not removed because I wanted to keep it out of my
>>> series and out of my brain.
>>>
>>> Disclaimer: I couldn't care less if the raw block driver was public
>>> domain or AGPLv3+, as long as it qualifies as free software. I'm only
>>> trying to do what Paolo asked of me.
>>
>> Generally speaking, rewriting parts of QEMU to be !GPL is something I
>> would strongly, strongly oppose.
>>
>> I believe that Paolo had a good reason for this though.
> 
> The reason is that Christoph said his original version of block/raw.c
> was meant to be GPLv2-only.  I don't care if the file is BSD or LGPLv2+,
> but most of the block layer is BSD, which is why I went for BSD.

So, is it OK if, after fixing the small problem in 7/7, I post v2 again
under the BSDL?

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH 0/7] introduce BSD-licensed block driver for "raw"

[Kevin Wolf]

Am 16.08.2013 um 16:15 hat Laszlo Ersek geschrieben:
> Paolo asked me to write such a driver based on his textual specification
> alone. The first patch captures his email in full, the rest re-quotes
> parts that are being implemented.
> 
> The tree compiles at each patch. The series passes "make check-block".
> 
> "block/raw.c" is not removed because I wanted to keep it out of my
> series and out of my brain.
> 
> Disclaimer: I couldn't care less if the raw block driver was public
> domain or AGPLv3+, as long as it qualifies as free software. I'm only
> trying to do what Paolo asked of me.
> 
> Laszlo Ersek (7):
>   add skeleton for BSD licensed "raw" BlockDriver
>   raw_bsd: emit debug events in bdrv_co_readv() and bdrv_co_writev()
>   raw_bsd: add raw_create()
>   raw_bsd: introduce "special members"
>   raw_bsd: add raw_create_options
>   raw_bsd: register bdrv_raw
>   switch raw block driver from "raw.o" to "raw_bsd.o"
> 
>  block/Makefile.objs |    2 +-
>  block/raw_bsd.c     |  186 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 187 insertions(+), 1 deletions(-)
>  create mode 100644 block/raw_bsd.c

Reviewed if the individual added functions make sense, whether all
necessary function from struct BlockDriver are implemented, and which
fields from BlockDriverState need special handling (it's only bs->sg,
and we should probably get rid of that requirement)

Looks good in general, but please CC Stefan and me for v2 (like for all
block patches).

Kevin

Re: [Qemu-devel] [PATCH 0/7] introduce BSD-licensed block driver for "raw"

[Laszlo Ersek]

On 08/20/13 10:21, Kevin Wolf wrote:
> Am 16.08.2013 um 16:15 hat Laszlo Ersek geschrieben:
>> Paolo asked me to write such a driver based on his textual specification
>> alone. The first patch captures his email in full, the rest re-quotes
>> parts that are being implemented.
>>
>> The tree compiles at each patch. The series passes "make check-block".
>>
>> "block/raw.c" is not removed because I wanted to keep it out of my
>> series and out of my brain.
>>
>> Disclaimer: I couldn't care less if the raw block driver was public
>> domain or AGPLv3+, as long as it qualifies as free software. I'm only
>> trying to do what Paolo asked of me.
>>
>> Laszlo Ersek (7):
>>   add skeleton for BSD licensed "raw" BlockDriver
>>   raw_bsd: emit debug events in bdrv_co_readv() and bdrv_co_writev()
>>   raw_bsd: add raw_create()
>>   raw_bsd: introduce "special members"
>>   raw_bsd: add raw_create_options
>>   raw_bsd: register bdrv_raw
>>   switch raw block driver from "raw.o" to "raw_bsd.o"
>>
>>  block/Makefile.objs |    2 +-
>>  block/raw_bsd.c     |  186 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 187 insertions(+), 1 deletions(-)
>>  create mode 100644 block/raw_bsd.c
> 
> Reviewed if the individual added functions make sense, whether all
> necessary function from struct BlockDriver are implemented, and which
> fields from BlockDriverState need special handling (it's only bs->sg,
> and we should probably get rid of that requirement)
> 
> Looks good in general, but please CC Stefan and me for v2 (like for all
> block patches).

Thanks for the review.

Regarding your comments for 4/7: can we postpone the bdrv_is_sg() change
to another series?

Because, I can't just rebase / update this series as a "normal" series
-- v2 will still be a "clean room reimplementation", and I must keep
full history (basically, a documentation of the "clean room process") in
the commit log.

So, Paolo's suggestion for 7/7 (ie. raw_reopen_prepare() should just
return 0) will be a separate 8/8, with his email quoted as commit
message. (Normally I would just squash the change and add a short v2
note *outside* the commit log, but that's exactly what we can't do here.)

... Maybe I can still squash the change into 7/7, and extend only the
commit message with Paolo's email, since that includes the wrong v1 code
too.

Anyway, under this MO, I have trouble implementing what you want, if
only from the "how to structure the series" aspect. I think we shouldn't
mix in any changes that are unrelated to the BSDL reimplementation
*purpose* (as opposed to, related to the code).

Once the series is applied, in a minimal, barely working form, under the
BSDL, y'all who actually have a clue about the block layer can very
quickly improve it.

Does that sound acceptable?

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 0/7] introduce BSD-licensed block driver for "raw"

[Paolo Bonzini]

Il 21/08/2013 10:20, Laszlo Ersek ha scritto:
> On 08/20/13 10:21, Kevin Wolf wrote:
>> Am 16.08.2013 um 16:15 hat Laszlo Ersek geschrieben:
>>> Paolo asked me to write such a driver based on his textual specification
>>> alone. The first patch captures his email in full, the rest re-quotes
>>> parts that are being implemented.
>>>
>>> The tree compiles at each patch. The series passes "make check-block".
>>>
>>> "block/raw.c" is not removed because I wanted to keep it out of my
>>> series and out of my brain.
>>>
>>> Disclaimer: I couldn't care less if the raw block driver was public
>>> domain or AGPLv3+, as long as it qualifies as free software. I'm only
>>> trying to do what Paolo asked of me.
>>>
>>> Laszlo Ersek (7):
>>>   add skeleton for BSD licensed "raw" BlockDriver
>>>   raw_bsd: emit debug events in bdrv_co_readv() and bdrv_co_writev()
>>>   raw_bsd: add raw_create()
>>>   raw_bsd: introduce "special members"
>>>   raw_bsd: add raw_create_options
>>>   raw_bsd: register bdrv_raw
>>>   switch raw block driver from "raw.o" to "raw_bsd.o"
>>>
>>>  block/Makefile.objs |    2 +-
>>>  block/raw_bsd.c     |  186 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 187 insertions(+), 1 deletions(-)
>>>  create mode 100644 block/raw_bsd.c
>>
>> Reviewed if the individual added functions make sense, whether all
>> necessary function from struct BlockDriver are implemented, and which
>> fields from BlockDriverState need special handling (it's only bs->sg,
>> and we should probably get rid of that requirement)
>>
>> Looks good in general, but please CC Stefan and me for v2 (like for all
>> block patches).
> 
> Thanks for the review.
> 
> Regarding your comments for 4/7: can we postpone the bdrv_is_sg() change
> to another series?

I think it should be done like that.

> Because, I can't just rebase / update this series as a "normal" series
> -- v2 will still be a "clean room reimplementation", and I must keep
> full history (basically, a documentation of the "clean room process") in
> the commit log.
> 
> So, Paolo's suggestion for 7/7 (ie. raw_reopen_prepare() should just
> return 0) will be a separate 8/8, with his email quoted as commit
> message. (Normally I would just squash the change and add a short v2
> note *outside* the commit log, but that's exactly what we can't do here.)
> 
> ... Maybe I can still squash the change into 7/7, and extend only the
> commit message with Paolo's email, since that includes the wrong v1 code
> too.

Yes, just squash it.

Paolo