Re: [Qemu-devel] [RHEL-7.3 qemu-kvm-rhev PATCH] qmp: Report drive_add error to monitor

[Markus Armbruster <armbru@redhat.com>]

Fam Zheng <famz@redhat.com> writes:

> BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1337100
> Brew: 11051815
> Upstream: N/A
>
> In other error cases of this function we use error_setg, the same should
> be done with drive_new() failures. This is useful for libvirt to
> correctly detect the failure and report proper error message when a
> specified image is not available.
>
> This bug cames from the forward porting from qemu-kvm, at which point we
> overlooked the difference in QMP reporting between qerror_report and
> error_report.

Commit 524403b in master-2.4, 9a612fc in master-2.5, 0806196 in
master-2.6.  The commit message is unhelpful / misleading on the
commit's history.  Please understand that I'm *not* pointing this out to
assign blame!  I don't care for blaming people, and I've made enough
similar mistakes to know how easy they are to make.  I care because I
want to figure out how this happened, and what we can do to better avoid
such mistakes in the future.

This is the forward port from RHEL-6 that went into 7.0:

    commit 75ad257a1d23dcbde364ad736770d1bd01f157b6
    Author:     Markus Armbruster <armbru@redhat.com>
    AuthorDate: Tue Dec 17 06:46:36 2013 +0100
    Commit:     Michal Novotny <minovotn@redhat.com>
    CommitDate: Wed Dec 18 17:59:19 2013 +0100

        QMP: Forward-port __com.redhat_drive_add from RHEL-6

        RH-Author: Markus Armbruster <armbru@redhat.com>
        Message-id: <1387262799-10350-4-git-send-email-armbru@redhat.com>
        Patchwork-id: 56294
        O-Subject: [PATCH v2 3/6] QMP: Forward-port __com.redhat_drive_add from RHEL-6
        Bugzilla: 889051
        RH-Acked-by: Fam Zheng <famz@redhat.com>
        RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
        RH-Acked-by: Luiz Capitulino <lcapitulino@redhat.com>

        From: Markus Armbruster <armbru@redhat.com>

        Code taken from RHEL-6 as of qemu-kvm-0.12.1.2-2.418.el6, backported
        and fixed up as follows:

        * Update simple_drive_add() for commit 4e89978 "qemu-option:
          qemu_opts_from_qdict(): use error_set()".

        * Update simple_drive_add() for commit 2d0d283 "Support default block
          interfaces per QEMUMachine".

        * Add comment explaining drive_init() error reporting hacks to
          simple_drive_add().

        * qemu-monitor.hx has been split into qmp-commands.hx and
          hmp-commands.hx.  Copy the QMP parts to qmp-commands.hx.  Clean up
          second example slightly.

        * Trailing whitespace cleaned up.

        Signed-off-by: Markus Armbruster <armbru@redhat.com>
        ---
         device-hotplug.c          | 73 +++++++++++++++++++++++++++++++++++++++++++++++
         include/sysemu/blockdev.h |  2 ++
         qmp-commands.hx           | 46 +++++++++++++++++++++++++++++
         3 files changed, 121 insertions(+)

        Signed-off-by: Michal Novotny <minovotn@redhat.com>

It's unchanged in current qemu-kvm.  For qemu-kvm-rhev, we went through
a series of rebases.  Here's 7.1's to upstream 2.1.0:

    commit 5e207eb5cf52288913f345bc91ace6e854f63160
    Author:     Markus Armbruster <armbru@redhat.com>
    AuthorDate: Tue Dec 17 06:46:36 2013 +0100
    Commit:     Miroslav Rezanina <mrezanin@redhat.com>
    CommitDate: Fri Sep 26 13:51:59 2014 +0200

Since it's a rebase, it doesn't carry a (cherry picked from commit ...)
line, but remembering about the rebase and finding the commit that was
rebased is easy enough.

The commit message is unchanged, i.e. it carries no rebase notes.  The
commit applies cleanly, but doesn't compile.  The commit resolves the
semantic conflicts correctly.  The resolution should have been
documented in the commit message.

Here's 7.2's to upstream 2.3.0:

    commit c466d0a410e2618c6a69ec4d7179ea5be97121a5
    Author:     Markus Armbruster <armbru@redhat.com>
    AuthorDate: Tue Dec 17 06:46:36 2013 +0100
    Commit:     Miroslav Rezanina <mrezanin@redhat.com>
    CommitDate: Tue Apr 28 07:52:38 2015 +0200

This time there are conflicts.  The only change to the commit message is
loss of the part below the '---' line.  Again, the commit resolves the
conflicts correctly, and again it fails to document them.

Digression on '---' lines, feel free to skip.  git-am interprets such a
line as "end of commit message, start of patch".  This effectively drops
everything between this line and the patch proper.  Useful to tack notes
to a patch that shouldn't go into the repository.

Until relatively recently, our maintainer tools failed to implement this
convention, and because of that we now got tons of old commits with
'---' lines in their messages.  Looks like they get dropped on rebase.
That's wrong, but it's probably not wrong enough to do anything about
it.  End of digression.

Here's the rebase to upstream 2.4.0:

    commit 524403b863dfdb4c97297230bbede2ca54314fcf
    Author:     Markus Armbruster <armbru@redhat.com>
    AuthorDate: Tue Dec 17 06:46:36 2013 +0100
    Commit:     Miroslav Rezanina <mrezanin@redhat.com>
    CommitDate: Fri Nov 6 09:54:28 2015 +0100

        QMP: Forward-port __com.redhat_drive_add from RHEL-6
    [No changes here...]
        * Trailing whitespace cleaned up.

        Rebase notes (2.4.0):
        - removed qerror_report
        - removed user_print
        Signed-off-by: Markus Armbruster <armbru@redhat.com>

Good: it comes with rebase notes.  But they're in the wrong place, which
makes them needlessly confusing.  Never *change* a commit message on
rebase or cherry-pick!  *Append* to it, so the commit message serves as
a *log*, like this:

        Signed-off-by: Markus Armbruster <armbru@redhat.com>

        Rebase notes (2.4.0):
        - removed qerror_report
        - removed user_print

        Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>

Bad: there was no review.  This is a *process* mistake, not a human
mistake: we require formal review (three ACKs) for all patches,
including forward ports from previous RHEL versions (e.g. the 7.0
version of this patch), *except* for rebases.  There, we dump the
responsibility entirely on the maintainer doing the rebase.

This isn't the first time a rebase had problems of the sort review can
catch.  It's time to mend our ways and route rebases through our the
tried-and-true review process.

I think there are no additional lessons to learn from this commit's
rebases to upstream 2.5.0 and 2.6.0, so I'm skipping them.

> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  device-hotplug.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/device-hotplug.c b/device-hotplug.c
> index 12c17e5..883346e 100644
> --- a/device-hotplug.c
> +++ b/device-hotplug.c
> @@ -145,8 +145,7 @@ void simple_drive_add(QDict *qdict, QObject **ret_data, Error **errp)
>      mc = MACHINE_GET_CLASS(current_machine);
>      dinfo = drive_new(opts, mc->block_default_type);
>      if (!dinfo) {
> -        error_report(QERR_DEVICE_INIT_FAILED,
> -                      qemu_opts_id(opts));
> +        error_setg(errp, QERR_DEVICE_INIT_FAILED, qemu_opts_id(opts));
>          qemu_opts_del(opts);
>          return;
>      }

ACK