[PATCH v2 0/4] block: minor refactoring in preparation to the block layer API split

[PATCH v2 0/4] block: minor refactoring in preparation to the block layer API split

[Emanuele Giuseppe Esposito]

These patches are taken from my old patches and feedback of
my series "block layer: split block APIs in global state and I/O".

The reason for a separate series is that the original one is
already too long, and these patches are just refactoring the code,
mainly deleting or moving functions in blockdev.h and block_int.h.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
v2:
* Apply Philippe comments, instead of renaming a make if_name
  public, create a getter method (discard old patch 2).

Emanuele Giuseppe Esposito (4):
  block_int: make bdrv_backing_overridden static
  include/sysemu/blockdev.c: introduce block_if_name
  include/sysemu/blockdev.h: move drive_add and inline drive_def
  include/sysemu/blockdev.h: remove drive_get_max_devs

 block.c                        |  4 ++-
 block/monitor/block-hmp-cmds.c |  2 +-
 blockdev.c                     | 47 ++++------------------------------
 include/block/block_int.h      |  3 ---
 include/sysemu/blockdev.h      |  6 +----
 softmmu/vl.c                   | 25 +++++++++++++++++-
 6 files changed, 34 insertions(+), 53 deletions(-)

-- 
2.31.1

[PATCH v2 1/4] block_int: make bdrv_backing_overridden static

[Emanuele Giuseppe Esposito]

bdrv_backing_overridden is only used in block.c, so there is
no need to leave it in block_int.h

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block.c                   | 4 +++-
 include/block/block_int.h | 3 ---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 0ac5b163d2..10346b5011 100644
--- a/block.c
+++ b/block.c
@@ -103,6 +103,8 @@ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
 static void bdrv_reopen_commit(BDRVReopenState *reopen_state);
 static void bdrv_reopen_abort(BDRVReopenState *reopen_state);
 
+static bool bdrv_backing_overridden(BlockDriverState *bs);
+
 /* If non-zero, use only whitelisted block drivers */
 static int use_bdrv_whitelist;
 
@@ -7475,7 +7477,7 @@ static bool append_strong_runtime_options(QDict *d, BlockDriverState *bs)
 /* Note: This function may return false positives; it may return true
  * even if opening the backing file specified by bs's image header
  * would result in exactly bs->backing. */
-bool bdrv_backing_overridden(BlockDriverState *bs)
+static bool bdrv_backing_overridden(BlockDriverState *bs)
 {
     if (bs->backing) {
         return strcmp(bs->auto_backing_file,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f4c75e8ba9..27008cfb22 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1122,9 +1122,6 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
 void bdrv_parse_filename_strip_prefix(const char *filename, const char *prefix,
                                       QDict *options);
 
-bool bdrv_backing_overridden(BlockDriverState *bs);
-
-
 /**
  * bdrv_add_aio_context_notifier:
  *
-- 
2.31.1

[PATCH v2 2/4] include/sysemu/blockdev.c: introduce block_if_name

[Emanuele Giuseppe Esposito]

Add a getter function for the if_name array, so that also
outside functions can access it.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 blockdev.c                | 5 +++++
 include/sysemu/blockdev.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index b35072644e..1581f0d2f5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -83,6 +83,11 @@ static const char *const if_name[IF_COUNT] = {
     [IF_XEN] = "xen",
 };
 
+const char *block_if_name(BlockInterfaceType iface)
+{
+    return if_name[iface];
+}
+
 static int if_max_devs[IF_COUNT] = {
     /*
      * Do not change these numbers!  They govern how drive option
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 32c2d6023c..b321286dee 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -42,6 +42,7 @@ DriveInfo *blk_legacy_dinfo(BlockBackend *blk);
 DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo);
 BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo);
 
+const char *block_if_name(BlockInterfaceType iface);
 void override_max_devs(BlockInterfaceType type, int max_devs);
 
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
-- 
2.31.1

[PATCH v2 3/4] include/sysemu/blockdev.h: move drive_add and inline drive_def

[Emanuele Giuseppe Esposito]

drive_add is only used in softmmu/vl.c, so it can be a static
function there, and drive_def is only a particular use case of
qemu_opts_parse_noisily, so it can be inlined.

Also remove drive_mark_claimed_by_board, as it is only defined
but not implemented (nor used) anywhere.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/monitor/block-hmp-cmds.c |  2 +-
 blockdev.c                     | 25 -------------------------
 include/sysemu/blockdev.h      |  4 ----
 softmmu/vl.c                   | 25 ++++++++++++++++++++++++-
 4 files changed, 25 insertions(+), 31 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 2ac4aedfff..bfb3c043a0 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -101,7 +101,7 @@ void hmp_drive_add(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    opts = drive_def(optstr);
+    opts = qemu_opts_parse_noisily(qemu_find_opts("drive"), optstr, false);
     if (!opts)
         return;
 
diff --git a/blockdev.c b/blockdev.c
index 1581f0d2f5..e5cb3eb95f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -202,31 +202,6 @@ static int drive_index_to_unit_id(BlockInterfaceType type, int index)
     return max_devs ? index % max_devs : index;
 }
 
-QemuOpts *drive_def(const char *optstr)
-{
-    return qemu_opts_parse_noisily(qemu_find_opts("drive"), optstr, false);
-}
-
-QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
-                    const char *optstr)
-{
-    QemuOpts *opts;
-
-    opts = drive_def(optstr);
-    if (!opts) {
-        return NULL;
-    }
-    if (type != IF_DEFAULT) {
-        qemu_opt_set(opts, "if", if_name[type], &error_abort);
-    }
-    if (index >= 0) {
-        qemu_opt_set_number(opts, "index", index, &error_abort);
-    }
-    if (file)
-        qemu_opt_set(opts, "file", file, &error_abort);
-    return opts;
-}
-
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
 {
     BlockBackend *blk;
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index b321286dee..21ae0b994a 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -46,16 +46,12 @@ const char *block_if_name(BlockInterfaceType iface);
 void override_max_devs(BlockInterfaceType type, int max_devs);
 
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
-void drive_mark_claimed_by_board(void);
 void drive_check_orphaned(void);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);
 int drive_get_max_devs(BlockInterfaceType type);
 DriveInfo *drive_get_next(BlockInterfaceType type);
 
-QemuOpts *drive_def(const char *optstr);
-QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
-                    const char *optstr);
 DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type,
                      Error **errp);
 
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 1159a64bce..4df30bad14 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -650,6 +650,27 @@ static int drive_enable_snapshot(void *opaque, QemuOpts *opts, Error **errp)
     return 0;
 }
 
+static QemuOpts *drive_add(BlockInterfaceType type, int index,
+                           const char *file, const char *optstr)
+{
+    QemuOpts *opts;
+
+    opts = qemu_opts_parse_noisily(qemu_find_opts("drive"), optstr, false);
+    if (!opts) {
+        return NULL;
+    }
+    if (type != IF_DEFAULT) {
+        qemu_opt_set(opts, "if", block_if_name(type), &error_abort);
+    }
+    if (index >= 0) {
+        qemu_opt_set_number(opts, "index", index, &error_abort);
+    }
+    if (file) {
+        qemu_opt_set(opts, "file", file, &error_abort);
+    }
+    return opts;
+}
+
 static void default_drive(int enable, int snapshot, BlockInterfaceType type,
                           int index, const char *optstr)
 {
@@ -2884,7 +2905,9 @@ void qemu_init(int argc, char **argv, char **envp)
                     break;
                 }
             case QEMU_OPTION_drive:
-                if (drive_def(optarg) == NULL) {
+                opts = qemu_opts_parse_noisily(qemu_find_opts("drive"),
+                                               optarg, false);
+                if (opts == NULL) {
                     exit(1);
                 }
                 break;
-- 
2.31.1

[PATCH v2 4/4] include/sysemu/blockdev.h: remove drive_get_max_devs

[Emanuele Giuseppe Esposito]

Remove drive_get_max_devs, as it is not used by anyone.

Last use was removed in commit 8f2d75e81d5
("hw: Drop superfluous special checks for orphaned -drive").

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 blockdev.c                | 17 -----------------
 include/sysemu/blockdev.h |  1 -
 2 files changed, 18 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index e5cb3eb95f..0112f488a4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -173,23 +173,6 @@ void blockdev_auto_del(BlockBackend *blk)
     }
 }
 
-/**
- * Returns the current mapping of how many units per bus
- * a particular interface can support.
- *
- *  A positive integer indicates n units per bus.
- *  0 implies the mapping has not been established.
- * -1 indicates an invalid BlockInterfaceType was given.
- */
-int drive_get_max_devs(BlockInterfaceType type)
-{
-    if (type >= IF_IDE && type < IF_COUNT) {
-        return if_max_devs[type];
-    }
-
-    return -1;
-}
-
 static int drive_index_to_bus_id(BlockInterfaceType type, int index)
 {
     int max_devs = if_max_devs[type];
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 21ae0b994a..430abdbdb8 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -49,7 +49,6 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 void drive_check_orphaned(void);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);
-int drive_get_max_devs(BlockInterfaceType type);
 DriveInfo *drive_get_next(BlockInterfaceType type);
 
 DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type,
-- 
2.31.1

Re: [PATCH v2 1/4] block_int: make bdrv_backing_overridden static

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 441 bytes --]

On Tue, Nov 30, 2021 at 04:46:30AM -0500, Emanuele Giuseppe Esposito wrote:
> bdrv_backing_overridden is only used in block.c, so there is
> no need to leave it in block_int.h
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  block.c                   | 4 +++-
>  include/block/block_int.h | 3 ---
>  2 files changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/4] include/sysemu/blockdev.c: introduce block_if_name

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 419 bytes --]

On Tue, Nov 30, 2021 at 04:46:31AM -0500, Emanuele Giuseppe Esposito wrote:
> Add a getter function for the if_name array, so that also
> outside functions can access it.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  blockdev.c                | 5 +++++
>  include/sysemu/blockdev.h | 1 +
>  2 files changed, 6 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/4] include/sysemu/blockdev.h: move drive_add and inline drive_def

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

On Tue, Nov 30, 2021 at 04:46:32AM -0500, Emanuele Giuseppe Esposito wrote:
> drive_add is only used in softmmu/vl.c, so it can be a static
> function there, and drive_def is only a particular use case of
> qemu_opts_parse_noisily, so it can be inlined.
> 
> Also remove drive_mark_claimed_by_board, as it is only defined
> but not implemented (nor used) anywhere.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  block/monitor/block-hmp-cmds.c |  2 +-
>  blockdev.c                     | 25 -------------------------
>  include/sysemu/blockdev.h      |  4 ----
>  softmmu/vl.c                   | 25 ++++++++++++++++++++++++-
>  4 files changed, 25 insertions(+), 31 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 4/4] include/sysemu/blockdev.h: remove drive_get_max_devs

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 568 bytes --]

On Tue, Nov 30, 2021 at 04:46:33AM -0500, Emanuele Giuseppe Esposito wrote:
> Remove drive_get_max_devs, as it is not used by anyone.
> 
> Last use was removed in commit 8f2d75e81d5
> ("hw: Drop superfluous special checks for orphaned -drive").
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  blockdev.c                | 17 -----------------
>  include/sysemu/blockdev.h |  1 -
>  2 files changed, 18 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/4] block: minor refactoring in preparation to the block layer API split

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

On Tue, Nov 30, 2021 at 04:46:29AM -0500, Emanuele Giuseppe Esposito wrote:
> These patches are taken from my old patches and feedback of
> my series "block layer: split block APIs in global state and I/O".
> 
> The reason for a separate series is that the original one is
> already too long, and these patches are just refactoring the code,
> mainly deleting or moving functions in blockdev.h and block_int.h.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
> v2:
> * Apply Philippe comments, instead of renaming a make if_name
>   public, create a getter method (discard old patch 2).
> 
> Emanuele Giuseppe Esposito (4):
>   block_int: make bdrv_backing_overridden static
>   include/sysemu/blockdev.c: introduce block_if_name
>   include/sysemu/blockdev.h: move drive_add and inline drive_def
>   include/sysemu/blockdev.h: remove drive_get_max_devs
> 
>  block.c                        |  4 ++-
>  block/monitor/block-hmp-cmds.c |  2 +-
>  blockdev.c                     | 47 ++++------------------------------
>  include/block/block_int.h      |  3 ---
>  include/sysemu/blockdev.h      |  6 +----
>  softmmu/vl.c                   | 25 +++++++++++++++++-
>  6 files changed, 34 insertions(+), 53 deletions(-)
> 
> -- 
> 2.31.1
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/4] include/sysemu/blockdev.h: move drive_add and inline drive_def

[Kevin Wolf]

Am 30.11.2021 um 10:46 hat Emanuele Giuseppe Esposito geschrieben:
> drive_add is only used in softmmu/vl.c, so it can be a static
> function there, and drive_def is only a particular use case of
> qemu_opts_parse_noisily, so it can be inlined.
> 
> Also remove drive_mark_claimed_by_board, as it is only defined
> but not implemented (nor used) anywhere.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

I don't think moving drive_add() actually improves anything. Yes, you
can make it static, but in order to do that you had to introduce
block_if_name() as a new public function and you're moving an obviously
block related function to common code in vl.c.

So this part doesn't look like a net win to me. The rest of the series
looks good to me.

Kevin

Re: [PATCH v2 3/4] include/sysemu/blockdev.h: move drive_add and inline drive_def

[Emanuele Giuseppe Esposito]

On 14/12/2021 15:35, Kevin Wolf wrote:
> Am 30.11.2021 um 10:46 hat Emanuele Giuseppe Esposito geschrieben:
>> drive_add is only used in softmmu/vl.c, so it can be a static
>> function there, and drive_def is only a particular use case of
>> qemu_opts_parse_noisily, so it can be inlined.
>>
>> Also remove drive_mark_claimed_by_board, as it is only defined
>> but not implemented (nor used) anywhere.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> I don't think moving drive_add() actually improves anything. Yes, you
> can make it static, but in order to do that you had to introduce
> block_if_name() as a new public function and you're moving an obviously
> block related function to common code in vl.c.
> 
> So this part doesn't look like a net win to me. The rest of the series
> looks good to me.
> 

So are we going to drop patch 2 and 3? For me it is fine either way, and 
I saw Stefan added r-b to all patches.

If we are, Kevin are you going to apply only patch 1 and 4, or do you 
want me to send v3?

Thank you,
Emanuele

Re: [PATCH v2 3/4] include/sysemu/blockdev.h: move drive_add and inline drive_def

[Kevin Wolf]

Am 15.12.2021 um 10:19 hat Emanuele Giuseppe Esposito geschrieben:
> 
> 
> On 14/12/2021 15:35, Kevin Wolf wrote:
> > Am 30.11.2021 um 10:46 hat Emanuele Giuseppe Esposito geschrieben:
> > > drive_add is only used in softmmu/vl.c, so it can be a static
> > > function there, and drive_def is only a particular use case of
> > > qemu_opts_parse_noisily, so it can be inlined.
> > > 
> > > Also remove drive_mark_claimed_by_board, as it is only defined
> > > but not implemented (nor used) anywhere.
> > > 
> > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > 
> > I don't think moving drive_add() actually improves anything. Yes, you
> > can make it static, but in order to do that you had to introduce
> > block_if_name() as a new public function and you're moving an obviously
> > block related function to common code in vl.c.
> > 
> > So this part doesn't look like a net win to me. The rest of the series
> > looks good to me.
> > 
> 
> So are we going to drop patch 2 and 3? For me it is fine either way, and I
> saw Stefan added r-b to all patches.
> 
> If we are, Kevin are you going to apply only patch 1 and 4, or do you want
> me to send v3?

This patch does a bit more than just moving drive_add(). It also inlines
drive_def() and deletes drive_mark_claimed_by_board(), which are both
things that make sense to me. So this suggests a v3.

But if you think I should just apply patches 1 and 4, I'm happy to do
that, too.

Kevin

Re: [PATCH v2 3/4] include/sysemu/blockdev.h: move drive_add and inline drive_def

[Emanuele Giuseppe Esposito]

On 15/12/2021 11:00, Kevin Wolf wrote:
> Am 15.12.2021 um 10:19 hat Emanuele Giuseppe Esposito geschrieben:
>>
>>
>> On 14/12/2021 15:35, Kevin Wolf wrote:
>>> Am 30.11.2021 um 10:46 hat Emanuele Giuseppe Esposito geschrieben:
>>>> drive_add is only used in softmmu/vl.c, so it can be a static
>>>> function there, and drive_def is only a particular use case of
>>>> qemu_opts_parse_noisily, so it can be inlined.
>>>>
>>>> Also remove drive_mark_claimed_by_board, as it is only defined
>>>> but not implemented (nor used) anywhere.
>>>>
>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>
>>> I don't think moving drive_add() actually improves anything. Yes, you
>>> can make it static, but in order to do that you had to introduce
>>> block_if_name() as a new public function and you're moving an obviously
>>> block related function to common code in vl.c.
>>>
>>> So this part doesn't look like a net win to me. The rest of the series
>>> looks good to me.
>>>
>>
>> So are we going to drop patch 2 and 3? For me it is fine either way, and I
>> saw Stefan added r-b to all patches.
>>
>> If we are, Kevin are you going to apply only patch 1 and 4, or do you want
>> me to send v3?
> 
> This patch does a bit more than just moving drive_add(). It also inlines
> drive_def() and deletes drive_mark_claimed_by_board(), which are both
> things that make sense to me. So this suggests a v3.
> 
> But if you think I should just apply patches 1 and 4, I'm happy to do
> that, too.

You're right, I forgot about that. I will send v3, thanks.

Emanuele