[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [Qemu-devel] [PATCHv5 0/3] DEVICE_DELETED event



Anthony asked for a space between "PATCH" and "v<N>" in the subject.
Please try to remember next time.

"Michael S. Tsirkin" <mst redhat com> writes:

> libvirt has a long-standing bug: when removing the device,
> it can request removal but does not know when the
> removal completes. Add an event so we can fix this in a robust way.
>
> First patch only adds the event for devices with ID, second path adds it
> for all devices and adds a path fields. Split this way for ease of
> backport (stable downstreams without QOM would want to only take the
> first patch),

I'd *strongly* advice downstreams to take the first patch plus the part
of the third patch that changes the event trigger.

Let's compare the difference to upstream for both approaches.

    Just the first patch: event fires only when device has an ID.
    Upstream: event fires always.
    Subtle semantic difference.

    First patch + changed trigger: event doesn't have member path.
    Upstream: event has member path.
    Blatantly obvious syntactic difference.

I'd take syntactic over semantic any day.

Note that even without member path a QMP client can still find which
device is gone, by polling.  Any client designed to keep track of state
accurately across disconnect/reconnect (such as libvirt) needs such
polling code anyway.

If you want to make backporters' lives easier, move the event trigger
change from patch 3 to patch 1.

>                and also because the 2nd/3rd patches might turn out to be
> more controversial - if they really turn
> out such I'd like just the 1st one to get applied while we discuss 2 and
> 3.

I don't expect more controversy.  If I'm wrong, I don't want just patch
1 applied while we discuss, because that creates an longer stretch in
git where the event is there, but doesn't work like we want it to, for
no appreciable gain.  We're in no particular hurry here, so let's do it
right.

> Signed-off-by: Michael S. Tsirkin <mst redhat com>

Options in decreasing order of preference, pick one that suits you:

1. Go the extra mile for backporters, and move the event trigger change
   from PATCH 3 into 1.

2. Squash PATCH 1 into 3.  Backporting trouble is obvious and easy to
   resolve: just drop member path.  Feel free to point that out in the
   commit message.

3. Let the series stand as is.  Backporting trouble is subtle and easy
   to miss: backporting just PATCH 1 is tempting, but screws up the
   event trigger.  I wouldn't do it that way myself, but I'm not going
   to NAK usable upstream work because of such downstream concerns.

In case you pick 3:

Acked-by: Markus Armbruster <armbru redhat com>


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]