Drop the crap deprecated in commit a1b40bda08 "blockdev: Deprecate
-drive with bogus interface type" (v5.1.0).

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/system/deprecated.rst       |  7 ------
 docs/system/removed-features.rst |  7 ++++++
 include/sysemu/blockdev.h        |  1 -
 blockdev.c                       | 37 +++++++++++++-------------------
 softmmu/vl.c                     |  8 +------
 5 files changed, 23 insertions(+), 37 deletions(-)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 601e9647a5..664ed60e9f 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -94,13 +94,6 @@ QEMU 5.1 has three options:
       to the user to load all the images they need.
  3. ``-bios <file>`` - Tells QEMU to load the specified file as the firmwrae.
 
-``-drive`` with bogus interface type (since 5.1)
-''''''''''''''''''''''''''''''''''''''''''''''''
-
-Drives with interface types other than ``if=none`` are for onboard
-devices.  It is possible to use drives the board doesn't pick up with
--device.  This usage is now deprecated.  Use ``if=none`` instead.
-
 Short-form boolean options (since 6.0)
 ''''''''''''''''''''''''''''''''''''''
 
diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index 77e7ba1339..e6d2fbe798 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -87,6 +87,13 @@ becomes
     -device isa-fdc,...
     -device floppy,unit=1,drive=...
 
+``-drive`` with bogus interface type (removed in 6.0)
+'''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Drives with interface types other than ``if=none`` are for onboard
+devices.  Drives the board doesn't pick up can no longer be used with
+-device.  Use ``if=none`` instead.
+
 QEMU Machine Protocol (QMP) commands
 ------------------------------------
 
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 3b5fcda08d..32c2d6023c 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -35,7 +35,6 @@ struct DriveInfo {
     bool is_default;            /* Added by default_drive() ?  */
     int media_cd;
     QemuOpts *opts;
-    bool claimed_by_board;
     QTAILQ_ENTRY(DriveInfo) next;
 };
 
diff --git a/blockdev.c b/blockdev.c
index cd438e60e3..2e01889cff 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -240,19 +240,10 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
     return NULL;
 }
 
-void drive_mark_claimed_by_board(void)
-{
-    BlockBackend *blk;
-    DriveInfo *dinfo;
-
-    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
-        dinfo = blk_legacy_dinfo(blk);
-        if (dinfo && blk_get_attached_dev(blk)) {
-            dinfo->claimed_by_board = true;
-        }
-    }
-}
-
+/*
+ * Check board claimed all -drive that are meant to be claimed.
+ * Fatal error if any remain unclaimed.
+ */
 void drive_check_orphaned(void)
 {
     BlockBackend *blk;
@@ -262,7 +253,17 @@ void drive_check_orphaned(void)
 
     for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
         dinfo = blk_legacy_dinfo(blk);
-        if (dinfo->is_default || dinfo->type == IF_NONE) {
+        /*
+         * Ignore default drives, because we create certain default
+         * drives unconditionally, then leave them unclaimed.  Not the
+         * users fault.
+         * Ignore IF_VIRTIO, because it gets desugared into -device,
+         * so we can leave failing to -device.
+         * Ignore IF_NONE, because leaving unclaimed IF_NONE remains
+         * available for device_add is a feature.
+         */
+        if (dinfo->is_default || dinfo->type == IF_VIRTIO
+            || dinfo->type == IF_NONE) {
             continue;
         }
         if (!blk_get_attached_dev(blk)) {
@@ -273,14 +274,6 @@ void drive_check_orphaned(void)
                          if_name[dinfo->type], dinfo->bus, dinfo->unit);
             loc_pop(&loc);
             orphans = true;
-            continue;
-        }
-        if (!dinfo->claimed_by_board && dinfo->type != IF_VIRTIO) {
-            loc_push_none(&loc);
-            qemu_opts_loc_restore(dinfo->opts);
-            warn_report("bogus if=%s is deprecated, use if=none",
-                        if_name[dinfo->type]);
-            loc_pop(&loc);
         }
     }
 
diff --git a/softmmu/vl.c b/softmmu/vl.c
index ff488ea3e7..7453611152 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2460,13 +2460,7 @@ static void qemu_init_board(void)
     /* From here on we enter MACHINE_PHASE_INITIALIZED.  */
     machine_run_board_init(current_machine);
 
-    /*
-     * TODO To drop support for deprecated bogus if=..., move
-     * drive_check_orphaned() here, replacing this call.  Also drop
-     * its deprecation warning, along with DriveInfo member
-     * @claimed_by_board.
-     */
-    drive_mark_claimed_by_board();
+    drive_check_orphaned();
 
     realtime_init();
 
-- 
2.26.2

On 3/9/21 11:12 AM, Markus Armbruster wrote:
> Drop the crap deprecated in commit a1b40bda08 "blockdev: Deprecate
> -drive with bogus interface type" (v5.1.0).
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   docs/system/deprecated.rst       |  7 ------
>   docs/system/removed-features.rst |  7 ++++++
>   include/sysemu/blockdev.h        |  1 -
>   blockdev.c                       | 37 +++++++++++++-------------------
>   softmmu/vl.c                     |  8 +------
>   5 files changed, 23 insertions(+), 37 deletions(-)
> 
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 601e9647a5..664ed60e9f 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -94,13 +94,6 @@ QEMU 5.1 has three options:
>         to the user to load all the images they need.
>    3. ``-bios <file>`` - Tells QEMU to load the specified file as the firmwrae.
>   
> -``-drive`` with bogus interface type (since 5.1)
> -''''''''''''''''''''''''''''''''''''''''''''''''
> -
> -Drives with interface types other than ``if=none`` are for onboard
> -devices.  It is possible to use drives the board doesn't pick up with
> --device.  This usage is now deprecated.  Use ``if=none`` instead.
> -
>   Short-form boolean options (since 6.0)
>   ''''''''''''''''''''''''''''''''''''''
>   
> diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
> index 77e7ba1339..e6d2fbe798 100644
> --- a/docs/system/removed-features.rst
> +++ b/docs/system/removed-features.rst
> @@ -87,6 +87,13 @@ becomes
>       -device isa-fdc,...
>       -device floppy,unit=1,drive=...
>   
> +``-drive`` with bogus interface type (removed in 6.0)
> +'''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +Drives with interface types other than ``if=none`` are for onboard
> +devices.  Drives the board doesn't pick up can no longer be used with
> +-device.  Use ``if=none`` instead.
> +
>   QEMU Machine Protocol (QMP) commands
>   ------------------------------------
>   
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 3b5fcda08d..32c2d6023c 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -35,7 +35,6 @@ struct DriveInfo {
>       bool is_default;            /* Added by default_drive() ?  */
>       int media_cd;
>       QemuOpts *opts;
> -    bool claimed_by_board;
>       QTAILQ_ENTRY(DriveInfo) next;
>   };
>   
> diff --git a/blockdev.c b/blockdev.c
> index cd438e60e3..2e01889cff 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -240,19 +240,10 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
>       return NULL;
>   }
>   
> -void drive_mark_claimed_by_board(void)
> -{
> -    BlockBackend *blk;
> -    DriveInfo *dinfo;
> -
> -    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
> -        dinfo = blk_legacy_dinfo(blk);
> -        if (dinfo && blk_get_attached_dev(blk)) {
> -            dinfo->claimed_by_board = true;
> -        }
> -    }
> -}
> -
> +/*
> + * Check board claimed all -drive that are meant to be claimed.
> + * Fatal error if any remain unclaimed.
> + */
>   void drive_check_orphaned(void)
>   {
>       BlockBackend *blk;
> @@ -262,7 +253,17 @@ void drive_check_orphaned(void)
>   
>       for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
>           dinfo = blk_legacy_dinfo(blk);
> -        if (dinfo->is_default || dinfo->type == IF_NONE) {
> +        /*
> +         * Ignore default drives, because we create certain default
> +         * drives unconditionally, then leave them unclaimed.  Not the
> +         * users fault.

"user's" ?

> +         * Ignore IF_VIRTIO, because it gets desugared into -device,
> +         * so we can leave failing to -device.
> +         * Ignore IF_NONE, because leaving unclaimed IF_NONE remains
> +         * available for device_add is a feature.

Do you mean "as a feature" ?

> +         */
> +        if (dinfo->is_default || dinfo->type == IF_VIRTIO
> +            || dinfo->type == IF_NONE) {
>               continue;
>           }
>           if (!blk_get_attached_dev(blk)) {
> @@ -273,14 +274,6 @@ void drive_check_orphaned(void)
>                            if_name[dinfo->type], dinfo->bus, dinfo->unit);
>               loc_pop(&loc);
>               orphans = true;
> -            continue;
> -        }
> -        if (!dinfo->claimed_by_board && dinfo->type != IF_VIRTIO) {
> -            loc_push_none(&loc);
> -            qemu_opts_loc_restore(dinfo->opts);
> -            warn_report("bogus if=%s is deprecated, use if=none",
> -                        if_name[dinfo->type]);
> -            loc_pop(&loc);
>           }
>       }
>   
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index ff488ea3e7..7453611152 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2460,13 +2460,7 @@ static void qemu_init_board(void)
>       /* From here on we enter MACHINE_PHASE_INITIALIZED.  */
>       machine_run_board_init(current_machine);
>   
> -    /*
> -     * TODO To drop support for deprecated bogus if=..., move
> -     * drive_check_orphaned() here, replacing this call.  Also drop
> -     * its deprecation warning, along with DriveInfo member
> -     * @claimed_by_board.
> -     */
> -    drive_mark_claimed_by_board();
> +    drive_check_orphaned();
>   
>       realtime_init();
>   
> 

Rest looks okay as far as I am concerned, pending your response to Dan's 
remark.

(Sorry for being AWOL here. Thank you for taking the time to clean it up 
as you have; it feels like closing a book on some of the first reviews I 
ever got @ RH when I put the first orphan check in aeons ago! I think 
only the whitespace is left by now, haha.)

--js

John Snow <jsnow@redhat.com> writes:

> On 3/9/21 11:12 AM, Markus Armbruster wrote:
>> Drop the crap deprecated in commit a1b40bda08 "blockdev: Deprecate
>> -drive with bogus interface type" (v5.1.0).
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   docs/system/deprecated.rst       |  7 ------
>>   docs/system/removed-features.rst |  7 ++++++
>>   include/sysemu/blockdev.h        |  1 -
>>   blockdev.c                       | 37 +++++++++++++-------------------
>>   softmmu/vl.c                     |  8 +------
>>   5 files changed, 23 insertions(+), 37 deletions(-)
>> 
>> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
>> index 601e9647a5..664ed60e9f 100644
>> --- a/docs/system/deprecated.rst
>> +++ b/docs/system/deprecated.rst
>> @@ -94,13 +94,6 @@ QEMU 5.1 has three options:
>>         to the user to load all the images they need.
>>    3. ``-bios <file>`` - Tells QEMU to load the specified file as the firmwrae.
>>   
>> -``-drive`` with bogus interface type (since 5.1)
>> -''''''''''''''''''''''''''''''''''''''''''''''''
>> -
>> -Drives with interface types other than ``if=none`` are for onboard
>> -devices.  It is possible to use drives the board doesn't pick up with
>> --device.  This usage is now deprecated.  Use ``if=none`` instead.
>> -
>>   Short-form boolean options (since 6.0)
>>   ''''''''''''''''''''''''''''''''''''''
>>   
>> diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
>> index 77e7ba1339..e6d2fbe798 100644
>> --- a/docs/system/removed-features.rst
>> +++ b/docs/system/removed-features.rst
>> @@ -87,6 +87,13 @@ becomes
>>       -device isa-fdc,...
>>       -device floppy,unit=1,drive=...
>>   
>> +``-drive`` with bogus interface type (removed in 6.0)
>> +'''''''''''''''''''''''''''''''''''''''''''''''''''''
>> +
>> +Drives with interface types other than ``if=none`` are for onboard
>> +devices.  Drives the board doesn't pick up can no longer be used with
>> +-device.  Use ``if=none`` instead.
>> +
>>   QEMU Machine Protocol (QMP) commands
>>   ------------------------------------
>>   
>> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
>> index 3b5fcda08d..32c2d6023c 100644
>> --- a/include/sysemu/blockdev.h
>> +++ b/include/sysemu/blockdev.h
>> @@ -35,7 +35,6 @@ struct DriveInfo {
>>       bool is_default;            /* Added by default_drive() ?  */
>>       int media_cd;
>>       QemuOpts *opts;
>> -    bool claimed_by_board;
>>       QTAILQ_ENTRY(DriveInfo) next;
>>   };
>>   
>> diff --git a/blockdev.c b/blockdev.c
>> index cd438e60e3..2e01889cff 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -240,19 +240,10 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
>>       return NULL;
>>   }
>>   
>> -void drive_mark_claimed_by_board(void)
>> -{
>> -    BlockBackend *blk;
>> -    DriveInfo *dinfo;
>> -
>> -    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
>> -        dinfo = blk_legacy_dinfo(blk);
>> -        if (dinfo && blk_get_attached_dev(blk)) {
>> -            dinfo->claimed_by_board = true;
>> -        }
>> -    }
>> -}
>> -
>> +/*
>> + * Check board claimed all -drive that are meant to be claimed.
>> + * Fatal error if any remain unclaimed.
>> + */
>>   void drive_check_orphaned(void)
>>   {
>>       BlockBackend *blk;
>> @@ -262,7 +253,17 @@ void drive_check_orphaned(void)
>>   
>>       for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
>>           dinfo = blk_legacy_dinfo(blk);
>> -        if (dinfo->is_default || dinfo->type == IF_NONE) {
>> +        /*
>> +         * Ignore default drives, because we create certain default
>> +         * drives unconditionally, then leave them unclaimed.  Not the
>> +         * users fault.
>
> "user's" ?

Yes.

>> +         * Ignore IF_VIRTIO, because it gets desugared into -device,
>> +         * so we can leave failing to -device.
>> +         * Ignore IF_NONE, because leaving unclaimed IF_NONE remains
>> +         * available for device_add is a feature.
>
> Do you mean "as a feature" ?

I mean "because leaving unclaimed IF_NONE available for device_add is a
feature."

I'm embarrassingly prone to accidents when rewriting my own prose.

>> +         */
>> +        if (dinfo->is_default || dinfo->type == IF_VIRTIO
>> +            || dinfo->type == IF_NONE) {
>>               continue;
>>           }
>>           if (!blk_get_attached_dev(blk)) {
>> @@ -273,14 +274,6 @@ void drive_check_orphaned(void)
>>                            if_name[dinfo->type], dinfo->bus, dinfo->unit);
>>               loc_pop(&loc);
>>               orphans = true;
>> -            continue;
>> -        }
>> -        if (!dinfo->claimed_by_board && dinfo->type != IF_VIRTIO) {
>> -            loc_push_none(&loc);
>> -            qemu_opts_loc_restore(dinfo->opts);
>> -            warn_report("bogus if=%s is deprecated, use if=none",
>> -                        if_name[dinfo->type]);
>> -            loc_pop(&loc);
>>           }
>>       }
>>   
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index ff488ea3e7..7453611152 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -2460,13 +2460,7 @@ static void qemu_init_board(void)
>>       /* From here on we enter MACHINE_PHASE_INITIALIZED.  */
>>       machine_run_board_init(current_machine);
>>   
>> -    /*
>> -     * TODO To drop support for deprecated bogus if=..., move
>> -     * drive_check_orphaned() here, replacing this call.  Also drop
>> -     * its deprecation warning, along with DriveInfo member
>> -     * @claimed_by_board.
>> -     */
>> -    drive_mark_claimed_by_board();
>> +    drive_check_orphaned();
>>   
>>       realtime_init();
>>   
>> 
>
> Rest looks okay as far as I am concerned, pending your response to Dan's 
> remark.
>
> (Sorry for being AWOL here. Thank you for taking the time to clean it up 
> as you have; it feels like closing a book on some of the first reviews I 
> ever got @ RH when I put the first orphan check in aeons ago! I think 
> only the whitespace is left by now, haha.)

It has served us well :)

Thank you, v4 coming!

On Tue, Mar 09, 2021 at 05:12:13PM +0100, Markus Armbruster wrote:
> Drop the crap deprecated in commit a1b40bda08 "blockdev: Deprecate
> -drive with bogus interface type" (v5.1.0).
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  docs/system/deprecated.rst       |  7 ------
>  docs/system/removed-features.rst |  7 ++++++
>  include/sysemu/blockdev.h        |  1 -
>  blockdev.c                       | 37 +++++++++++++-------------------
>  softmmu/vl.c                     |  8 +------
>  5 files changed, 23 insertions(+), 37 deletions(-)
> 
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 601e9647a5..664ed60e9f 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -94,13 +94,6 @@ QEMU 5.1 has three options:
>        to the user to load all the images they need.
>   3. ``-bios <file>`` - Tells QEMU to load the specified file as the firmwrae.
>  
> -``-drive`` with bogus interface type (since 5.1)
> -''''''''''''''''''''''''''''''''''''''''''''''''
> -
> -Drives with interface types other than ``if=none`` are for onboard
> -devices.  It is possible to use drives the board doesn't pick up with
> --device.  This usage is now deprecated.  Use ``if=none`` instead.
> -
>  Short-form boolean options (since 6.0)
>  ''''''''''''''''''''''''''''''''''''''
>  
> diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
> index 77e7ba1339..e6d2fbe798 100644
> --- a/docs/system/removed-features.rst
> +++ b/docs/system/removed-features.rst
> @@ -87,6 +87,13 @@ becomes
>      -device isa-fdc,...
>      -device floppy,unit=1,drive=...
>  
> +``-drive`` with bogus interface type (removed in 6.0)
> +'''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +Drives with interface types other than ``if=none`` are for onboard
> +devices.  Drives the board doesn't pick up can no longer be used with
> +-device.  Use ``if=none`` instead.
> +
>  QEMU Machine Protocol (QMP) commands
>  ------------------------------------
>  
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 3b5fcda08d..32c2d6023c 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -35,7 +35,6 @@ struct DriveInfo {
>      bool is_default;            /* Added by default_drive() ?  */
>      int media_cd;
>      QemuOpts *opts;
> -    bool claimed_by_board;
>      QTAILQ_ENTRY(DriveInfo) next;
>  };
>  
> diff --git a/blockdev.c b/blockdev.c
> index cd438e60e3..2e01889cff 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -240,19 +240,10 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
>      return NULL;
>  }
>  
> -void drive_mark_claimed_by_board(void)
> -{
> -    BlockBackend *blk;
> -    DriveInfo *dinfo;
> -
> -    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
> -        dinfo = blk_legacy_dinfo(blk);
> -        if (dinfo && blk_get_attached_dev(blk)) {
> -            dinfo->claimed_by_board = true;
> -        }
> -    }
> -}
> -
> +/*
> + * Check board claimed all -drive that are meant to be claimed.
> + * Fatal error if any remain unclaimed.
> + */
>  void drive_check_orphaned(void)
>  {
>      BlockBackend *blk;
> @@ -262,7 +253,17 @@ void drive_check_orphaned(void)
>  
>      for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
>          dinfo = blk_legacy_dinfo(blk);
> -        if (dinfo->is_default || dinfo->type == IF_NONE) {
> +        /*
> +         * Ignore default drives, because we create certain default
> +         * drives unconditionally, then leave them unclaimed.  Not the
> +         * users fault.
> +         * Ignore IF_VIRTIO, because it gets desugared into -device,
> +         * so we can leave failing to -device.
> +         * Ignore IF_NONE, because leaving unclaimed IF_NONE remains
> +         * available for device_add is a feature.
> +         */
> +        if (dinfo->is_default || dinfo->type == IF_VIRTIO
> +            || dinfo->type == IF_NONE) {
>              continue;
>          }
>          if (!blk_get_attached_dev(blk)) {
> @@ -273,14 +274,6 @@ void drive_check_orphaned(void)
>                           if_name[dinfo->type], dinfo->bus, dinfo->unit);
>              loc_pop(&loc);
>              orphans = true;
> -            continue;
> -        }
> -        if (!dinfo->claimed_by_board && dinfo->type != IF_VIRTIO) {
> -            loc_push_none(&loc);
> -            qemu_opts_loc_restore(dinfo->opts);
> -            warn_report("bogus if=%s is deprecated, use if=none",
> -                        if_name[dinfo->type]);
> -            loc_pop(&loc);
>          }
>      }
>  
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index ff488ea3e7..7453611152 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2460,13 +2460,7 @@ static void qemu_init_board(void)
>      /* From here on we enter MACHINE_PHASE_INITIALIZED.  */
>      machine_run_board_init(current_machine);
>  
> -    /*
> -     * TODO To drop support for deprecated bogus if=..., move
> -     * drive_check_orphaned() here, replacing this call.  Also drop
> -     * its deprecation warning, along with DriveInfo member
> -     * @claimed_by_board.
> -     */
> -    drive_mark_claimed_by_board();
> +    drive_check_orphaned();

This method is already called by qemu_machine_creation_done(), which is
invoked shortly after this qemu_init_board() is run.

So either this added instance, or the later call to drive_check_orphaned
feels redundant

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Mar 09, 2021 at 05:12:13PM +0100, Markus Armbruster wrote:
>> Drop the crap deprecated in commit a1b40bda08 "blockdev: Deprecate
>> -drive with bogus interface type" (v5.1.0).
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  docs/system/deprecated.rst       |  7 ------
>>  docs/system/removed-features.rst |  7 ++++++
>>  include/sysemu/blockdev.h        |  1 -
>>  blockdev.c                       | 37 +++++++++++++-------------------
>>  softmmu/vl.c                     |  8 +------
>>  5 files changed, 23 insertions(+), 37 deletions(-)
>> 
>> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
>> index 601e9647a5..664ed60e9f 100644
>> --- a/docs/system/deprecated.rst
>> +++ b/docs/system/deprecated.rst
>> @@ -94,13 +94,6 @@ QEMU 5.1 has three options:
>>        to the user to load all the images they need.
>>   3. ``-bios <file>`` - Tells QEMU to load the specified file as the firmwrae.
>>  
>> -``-drive`` with bogus interface type (since 5.1)
>> -''''''''''''''''''''''''''''''''''''''''''''''''
>> -
>> -Drives with interface types other than ``if=none`` are for onboard
>> -devices.  It is possible to use drives the board doesn't pick up with
>> --device.  This usage is now deprecated.  Use ``if=none`` instead.
>> -
>>  Short-form boolean options (since 6.0)
>>  ''''''''''''''''''''''''''''''''''''''
>>  
>> diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
>> index 77e7ba1339..e6d2fbe798 100644
>> --- a/docs/system/removed-features.rst
>> +++ b/docs/system/removed-features.rst
>> @@ -87,6 +87,13 @@ becomes
>>      -device isa-fdc,...
>>      -device floppy,unit=1,drive=...
>>  
>> +``-drive`` with bogus interface type (removed in 6.0)
>> +'''''''''''''''''''''''''''''''''''''''''''''''''''''
>> +
>> +Drives with interface types other than ``if=none`` are for onboard
>> +devices.  Drives the board doesn't pick up can no longer be used with
>> +-device.  Use ``if=none`` instead.
>> +
>>  QEMU Machine Protocol (QMP) commands
>>  ------------------------------------
>>  
>> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
>> index 3b5fcda08d..32c2d6023c 100644
>> --- a/include/sysemu/blockdev.h
>> +++ b/include/sysemu/blockdev.h
>> @@ -35,7 +35,6 @@ struct DriveInfo {
>>      bool is_default;            /* Added by default_drive() ?  */
>>      int media_cd;
>>      QemuOpts *opts;
>> -    bool claimed_by_board;
>>      QTAILQ_ENTRY(DriveInfo) next;
>>  };
>>  
>> diff --git a/blockdev.c b/blockdev.c
>> index cd438e60e3..2e01889cff 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -240,19 +240,10 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
>>      return NULL;
>>  }
>>  
>> -void drive_mark_claimed_by_board(void)
>> -{
>> -    BlockBackend *blk;
>> -    DriveInfo *dinfo;
>> -
>> -    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
>> -        dinfo = blk_legacy_dinfo(blk);
>> -        if (dinfo && blk_get_attached_dev(blk)) {
>> -            dinfo->claimed_by_board = true;
>> -        }
>> -    }
>> -}
>> -
>> +/*
>> + * Check board claimed all -drive that are meant to be claimed.
>> + * Fatal error if any remain unclaimed.
>> + */
>>  void drive_check_orphaned(void)
>>  {
>>      BlockBackend *blk;
>> @@ -262,7 +253,17 @@ void drive_check_orphaned(void)
>>  
>>      for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
>>          dinfo = blk_legacy_dinfo(blk);
>> -        if (dinfo->is_default || dinfo->type == IF_NONE) {
>> +        /*
>> +         * Ignore default drives, because we create certain default
>> +         * drives unconditionally, then leave them unclaimed.  Not the
>> +         * users fault.
>> +         * Ignore IF_VIRTIO, because it gets desugared into -device,
>> +         * so we can leave failing to -device.
>> +         * Ignore IF_NONE, because leaving unclaimed IF_NONE remains
>> +         * available for device_add is a feature.
>> +         */
>> +        if (dinfo->is_default || dinfo->type == IF_VIRTIO
>> +            || dinfo->type == IF_NONE) {
>>              continue;
>>          }
>>          if (!blk_get_attached_dev(blk)) {
>> @@ -273,14 +274,6 @@ void drive_check_orphaned(void)
>>                           if_name[dinfo->type], dinfo->bus, dinfo->unit);
>>              loc_pop(&loc);
>>              orphans = true;
>> -            continue;
>> -        }
>> -        if (!dinfo->claimed_by_board && dinfo->type != IF_VIRTIO) {
>> -            loc_push_none(&loc);
>> -            qemu_opts_loc_restore(dinfo->opts);
>> -            warn_report("bogus if=%s is deprecated, use if=none",
>> -                        if_name[dinfo->type]);
>> -            loc_pop(&loc);
>>          }
>>      }
>>  
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index ff488ea3e7..7453611152 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -2460,13 +2460,7 @@ static void qemu_init_board(void)
>>      /* From here on we enter MACHINE_PHASE_INITIALIZED.  */
>>      machine_run_board_init(current_machine);
>>  
>> -    /*
>> -     * TODO To drop support for deprecated bogus if=..., move
>> -     * drive_check_orphaned() here, replacing this call.  Also drop
>> -     * its deprecation warning, along with DriveInfo member
>> -     * @claimed_by_board.
>> -     */
>> -    drive_mark_claimed_by_board();
>> +    drive_check_orphaned();
>
> This method is already called by qemu_machine_creation_done(), which is
> invoked shortly after this qemu_init_board() is run.
>
> So either this added instance, or the later call to drive_check_orphaned
> feels redundant

You're right.

I'll drop the one in qemu_machine_creation_done().  Rationale:

* Before the patch, we reject -drive if=T when T != none and nothing
  claimed it.  We warn unless the board claimed it.  "Nothing claimed
  it" is known only after qemu_create_cli_devices().

* After the patch, we reject -drive if=T when T != none and the board
  didn't claim it.  This is known earlier, after
  machine_run_board_init().