Re: [PATCH v6 1/4] slirp: Advance libslirp submodule to add ipv6 host-forward support

From: Doug Evans <dje@google.com>
To: "Marc-André Lureau" <marcandre.lureau@gmail.com>
Cc: QEMU <qemu-devel@nongnu.org>,
	"Samuel Thibault" <samuel.thibault@ens-lyon.org>,
	"Daniel P . Berrangé" <berrange@redhat.com>,
	"Jason Wang" <jasowang@redhat.com>
Subject: Re: [PATCH v6 1/4] slirp: Advance libslirp submodule to add ipv6 host-forward support
Date: Wed, 12 May 2021 12:50:56 -0700	[thread overview]
Message-ID: <CADPb22SGrvWe5+Aonkwq1pcrBENMTMJetzR7WZN6zDkjxjGJNw@mail.gmail.com> (raw)
In-Reply-To: <CAJ+F1C+TzUNq7BOCn7RaKKOztMymY6=+BPxmAYYV8Md=5MQU0A@mail.gmail.com>

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

On Wed, May 12, 2021 at 10:18 AM Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:

> Hi
>
> On Wed, May 12, 2021 at 8:43 PM Doug Evans <dje@google.com> wrote:
>
>> Ping.
>>
>> On Fri, May 7, 2021 at 8:46 AM Doug Evans <dje@google.com> wrote:
>>
>>> On Fri, May 7, 2021 at 8:23 AM Marc-André Lureau <
>>> marcandre.lureau@gmail.com> wrote:
>>>
>>>> Hi
>>>>
>>>> [...]
>>>>>
>>>>>
>>>>> Changes from v5:
>>>>>
>>>>> 1/4 slirp: Advance libslirp submodule to current master
>>>>> NOTE TO REVIEWERS: It may be a better use of everyone's time if a
>>>>> maintainer takes on advancing QEMU's libslirp to libslirp's master.
>>>>> Beyond that, I really don't know what to do except submit this patch as
>>>>> is currently provided.
>>>>>
>>>>>
>>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>
>>>> It can do, but it should rather be a diff of the commits that are new,
>>>> those that were not in the stable branch.
>>>>
>>>
>>>
>>> Can you elaborate?
>>> E.g., Should a new libslirp release be made first, and then update
>>> qemu-master to that new release?
>>>
>>
>>
>> Hey Marc-André and Samuel,
>> What's the next step here (if any)?
>>
>
> There isn't much point in bumping qemu dependency if it doesn't make use
> of the new API. Thus first step is to get the rest of the series
> reviewed/approved imho.
>

I'm not suggesting the dependency be bumped prior to the entire series
being approved.
By "next step" I meant whether this patch (1/4) in the series is done.
The question I asked: "Should a new libslirp release be made and then have
qemu-master use that (*1)?" was outstanding as it wasn't(isn't) clear
whether the plan was indeed to first make a new libslirp release (even
taking into account the comments on patch 3/4).

(*1): When using QEMU-provided libslirp. When not using QEMU-provided
slirp, of course, be compatible with the libslirp that is being used.
Thanks for bringing that to my attention btw, it's easy to forget given
that QEMU provides its own copy of libslirp and I have completely left that
out of v1 to v5.

> Would it be preferable to make a new libslirp release instead?
>>
>
> yes, as I said in some other path review, we would need a libslirp release
> AND compatiblity with older slirp releases.
>

Indeed! I was, perhaps errantly, treating them as orthogonal discussions.
[On the theory that:
- if we resolve all issues for each piece of the current iteration then
that will reduce the number of iterations,
and I'm not sure patch 1/4 is complete and what happens next for it (given
that a separate repo is involved)
- discussions of making a new libslirp release can proceed independent of
updating patch 3/4
That was the theory anyway.]

> [I also don't understand the comment "it should rather be a diff of the
>> commits that are new, ...".
>> I haven't seen this request before (apologies if I missed it), but more
>> importantly
>> isn't it trivial to generate such diffs, without adding them to the email?
>> I could be missing something of course.]
>>
>> At some point a new libslirp release needs to be made, and at some point
>> QEMU needs to be updated to use it.
>> Seems like now is a good time, but maybe there are reasons to prefer not
>> doing so now.
>> I can imagine QEMU wanting to always use a stable branch of libslirp,
>> so I just want to be absolutely clear that switching QEMU to use
>> libslirp's master branch is desired,
>> and if anything more is needed from me.
>> I'm happy with whatever you decide, but I don't want to waste your time
>> guessing and then having to iterate when I guess wrong.
>>
>
> You need to rework the series to be compatible with current libslirp. That
> may be one of the reason why you don't get more reviews, Jason, the
> maintainer, may expect you to do that based on feedback received earlier.
>

I'm indeed updating v7 in this series to be compatible with current
libslirp, but let's get the issue of a new libslirp release resolved too.

Btw, can you elaborate on "should rather be a diff of the commits that are
new"?
Up until now I've been told to provide "git shortlog old..new" output.
The patch itself is just a one-liner to update the subproject sha1.

[-- Attachment #2: Type: text/html, Size: 8820 bytes --]