Re: [PATCH v2 3/5] thermal: core: Remove old uapi generic netlink

[Amit Kucheria]

On Wed, Jul 1, 2020 at 3:15 PM Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
>
> On 01/07/2020 11:33, Amit Kucheria wrote:
> > On Wed, Jul 1, 2020 at 2:56 PM Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
> >>
> >> On 30/06/2020 13:47, Amit Kucheria wrote:
> >>> On Thu, Jun 25, 2020 at 8:15 PM Daniel Lezcano
> >>> <daniel.lezcano@xxxxxxxxxx> wrote:
> >>>>

> >>>>  /* Adding event notification support elements */
> >>>>  #define THERMAL_GENL_FAMILY_NAME                "thermal_event"
> >>>> -#define THERMAL_GENL_VERSION                    0x01
> >>>> +#define THERMAL_GENL_VERSION                    0x02
> >>>
> >>> This hunk should be removed since you set version back to 1 in the
> >>> next patch and we don't actually intend to bump the version yet.
> >>
> >> Well, I've been very strict here for git-bisecting.
> >>
> >> I move to V2 because of the removal, but when adding the new genetlink
> >> code, the family name changed, so we returned back to the V1 as it is a
> >> new genetlink thermal brand.
> >
> > I don't understand the move to v2 for an empty skeleton UAPI. For the
> > purposes of bisection, couldn't you just remove all the v1 UAPI (w/o
> > bumping to v2) and then add a new UAPI in the next patch?
> >
> >> The name is change because it is no longer event based but also sampling
> >> and commands.
> >
> > In this case, just to avoid any confusion, the new UAPI could be v2
> > making the transition clear in case of bisection.
> >
> > I'm afraid the v1->v2->v1 is a bit more confusing.
>
> Let me elaborate a bit:
>
> Why there is this patch ?
> - By removing this code first, the next patch will just contain
> additions, I thought it would be clearer
>
> Why increase the version here ?
> - Code must continue to compile and as the 'thermal_event' family is now
> different from V1, the version is changed
>
> Why the version goes to V1 in the next patch ?
> - The family name is changed as it is not doing event only, so it is a
> new netlink thermal protocol and we begin at V1
>
> So the main reason of this patch is to be very strict in the iteration
> changes. May be it is too much, in this case I can merge this patch with
> 4/5, the old netlink protocol removal will be lost in the addition of
> the new protocol. I'm fine with that if you think it is simpler.

Considering that there are no users of v1 currently, it feels a bit
over engineered, IMHO.

Also, the new UAPI doesn't need to begin at v1. Just having it start
at v2 will avoid this confusion, no?