qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: John Snow <jsnow@redhat.com>
To: Markus Armbruster <armbru@redhat.com>, qemu-devel@nongnu.org
Cc: kwolf@redhat.com, libvir-list@redhat.com, berrange@redhat.com,
	qemu-block@nongnu.org, mreitz@redhat.com
Subject: Re: [PATCH v3 4/4] blockdev: Drop deprecated bogus -drive interface type
Date: Thu, 11 Mar 2021 00:00:10 -0500	[thread overview]
Message-ID: <11ec870c-9b72-8b87-bfb9-b296d6762ccc@redhat.com> (raw)
In-Reply-To: <20210309161214.1402527-5-armbru@redhat.com>

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



  parent reply	other threads:[~2021-03-11  5:02 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-09 16:12 [PATCH v3 0/4] Drop deprecated floppy config & bogus -drive if=T Markus Armbruster
2021-03-09 16:12 ` [PATCH v3 1/4] docs/system/deprecated: Fix note on fdc drive properties Markus Armbruster
2021-03-09 16:13   ` Daniel P. Berrangé
2021-03-09 16:12 ` [PATCH v3 2/4] fdc: Drop deprecated floppy configuration Markus Armbruster
2021-03-09 16:14   ` Daniel P. Berrangé
2021-03-09 16:12 ` [PATCH v3 3/4] fdc: Inline fdctrl_connect_drives() into fdctrl_realize_common() Markus Armbruster
2021-03-09 16:15   ` Daniel P. Berrangé
2021-03-09 18:25   ` Richard Henderson
2021-03-10  8:13     ` Markus Armbruster
2021-03-09 16:12 ` [PATCH v3 4/4] blockdev: Drop deprecated bogus -drive interface type Markus Armbruster
2021-03-09 16:18   ` Daniel P. Berrangé
2021-03-10  8:10     ` Markus Armbruster
2021-03-11  5:00   ` John Snow [this message]
2021-03-11  6:29     ` Markus Armbruster

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=11ec870c-9b72-8b87-bfb9-b296d6762ccc@redhat.com \
    --to=jsnow@redhat.com \
    --cc=armbru@redhat.com \
    --cc=berrange@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=libvir-list@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).