Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@intel.com>
To:     Jakub Kicinski <kuba@kernel.org>
CC:     "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "edumazet@google.com" <edumazet@google.com>,
        "chuck.lever@oracle.com" <chuck.lever@oracle.com>
Subject: RE: [PATCH net-next] tools: ynl-gen: fix parse multi-attr enum
 attribute
Thread-Topic: [PATCH net-next] tools: ynl-gen: fix parse multi-attr enum
 attribute
Thread-Index: AQHZs93dRsBnvsQ2oE6qwQQo1+9ekK+1guSAgABffbA=
Date:   Wed, 12 Jul 2023 09:47:43 +0000
Message-ID: <DM6PR11MB465780867DE4C45F977A06D39B36A@DM6PR11MB4657.namprd11.prod.outlook.com>
References: <20230711095323.121131-1-arkadiusz.kubalewski@intel.com>
 <20230711205953.346e883b@kernel.org>
In-Reply-To: <20230711205953.346e883b@kernel.org>
Accept-Language: pl-PL, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?IjqvDaaeKIvF8j5rPjFnw5ks8pMDbnHPDt6wl7EYr4o+ONftm+8pbSM50oFS?=
 =?us-ascii?Q?tlJ04luNlE1DoCcatGNs6QIfqiu+bR17Lgie0i32eqoRHjRxF0b2vknz3iCW?=
 =?us-ascii?Q?xWFuqUgoeWAcc4iacTSJQRl+7l05lttEUNSa7+Tz+4zbbjn6xuCBCvGHS+ip?=
 =?us-ascii?Q?Zaa/yF3hNbRjbt3OYkmiUc/Ni6b9mucjONaVGCG4ORTmA025sYOOd5QJJ646?=
 =?us-ascii?Q?BZw2XYNRZFR2afO7kAEDGil9Ro6b7ZzCjvqpD/f8u3/CaeIgUvSvvJNyulEQ?=
 =?us-ascii?Q?7Ik0tbwQgS/U5kZ/16GrAx83yS///H+MWLfY4QYlLThVoRE2nRS8URZ4Vte9?=
 =?us-ascii?Q?xHo7a3wIT6Y4adzj40s6YqqxlACC1hdpLt7V2F3wUEjkC4mK1fW8TboHmMxu?=
 =?us-ascii?Q?xv9EReEzZ/LzRm8rV4f6I0lBNXlJVj1tdPUFve6HhWHiPxxRL4uVofno3Zap?=
 =?us-ascii?Q?yuj5uuyArKJLIjUPwqgYAGjxiEdF0TyNCPbKHMYs8LF8W2fhzKLoqpAq/GS9?=
 =?us-ascii?Q?x2UQXx9CCXQClgPw4tWQGCPh/9GnJcH1BL3+CK5xLXNjAmjzFkKCxS7y1+cq?=
 =?us-ascii?Q?RMfJxeh4pNO9njVX5oGP4IlVgCa6WFJx1mnKukUrw4HEWAuE/cZQENjf5Q0M?=
 =?us-ascii?Q?HMtYB3mwIgiJ8RJ6TDUNEmVzsBxL/6G+Me9qiQKCSaGm9+CHGWGW4gTggE4m?=
 =?us-ascii?Q?stbNzOmIiH0Fdr5NdAxaW9hD3K1OyKesRRev4vB1YHU/geeiDerawdBLdhYl?=
 =?us-ascii?Q?kHeLTg1Ez89Z9cOSJBvyHN5cAEEuziSQkruMLcT7Ed47jSOGadDIFrz70jVC?=
 =?us-ascii?Q?/SVX1ei7nZeskRRDkdo2/a51D/5DtluD6VD36bcxcaRq34MzOuGwYEm89Vkj?=
 =?us-ascii?Q?zjmQPbd67NQbfSKaPLuGD2/co+Y+EQQs1z342It4ZfdZzVjF3GglUS97qfez?=
 =?us-ascii?Q?i3jhVfG1GIiOBRtZHyIhGiO8uz6x6RAxR4O7Icw/bj7g0qleQ9jo03jYzrhj?=
 =?us-ascii?Q?oLhKpPtVN+wDo/rdOIPr6vlVf6fayFzy0KalR2bblCtvdNxlLL7jujeltLwq?=
 =?us-ascii?Q?T0nly8nBV+m1JT8taD/0s/Y3tLS2MzpEEToJZPbfWG7eP4ly/oLRo1GlEKDW?=
 =?us-ascii?Q?DzmaPB2XTJqhfN8WG0vbMJUgNzIpeVQ5sa3OWhGMsLOGhLod19exTtitQCU8?=
 =?us-ascii?Q?RfdgFmhsyHJRfRp/w+cSbmiIqHn+/mqExRoYksWcKYJ/nE610LjlcIVvpG7V?=
 =?us-ascii?Q?wg3FrhHp9dlgFGM+auUTZTZwOJGuSdLtiwtuA2DYXR7jd1oerOssvM2m2pO9?=
 =?us-ascii?Q?6c6uOFfXBwgcuXk/MgLlOI9WmSo/+iQP/1P+u0EdRmpKAXx0WwdXEke12xv3?=
 =?us-ascii?Q?U7TC0RpKeJr4wZzHg49WRZZfcXNm9fI/NLtf+wHGnGU+uU1BlxOTIw789Sfk?=
 =?us-ascii?Q?29YnCvQNJA8rVHHzhk3OsaYuwLxBh3JO/0evctbLCQZ86/j4WFnqdDoHBFTK?=
 =?us-ascii?Q?WLHCqeM2zQb/XjC6Gtf8GdFo7iLo0gvm50ZBuCyz4QA4DmGnRmU2Er/Ubfv9?=
 =?us-ascii?Q?WorBqjNkaqTkU1qmo3zNL1r0eBqQvDjkoUfGw1hf9D5T4hW/A9obj/7INHUe?=
 =?us-ascii?Q?Kw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB4657.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2c769c60-a9c5-4193-cc6c-08db82bd0a6a
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Jul 2023 09:47:43.7675
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 9tu2Sp7E1boQIhxa6GHrmw5X9cF0GYws7yayKY0F2RaL7kSm3kpOXadvCAKTME9mt5hcJ2rk8rTHg24ZYWn1yR1b+DwWeEmoStVU0TkN9yI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4999
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,
        RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Wednesday, July 12, 2023 6:00 AM
>
>On Tue, 11 Jul 2023 11:53:23 +0200 Arkadiusz Kubalewski wrote:
>> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index
>> 3b343d6cbbc0..553d82dd6382 100644
>> --- a/tools/net/ynl/lib/ynl.py
>> +++ b/tools/net/ynl/lib/ynl.py
>> @@ -407,7 +407,14 @@ class YnlFamily(SpecFamily):
>>                  raw >>=3D 1
>>                  i +=3D 1
>>          else:
>> -            value =3D enum.entries_by_val[raw - i].name
>> +            if attr_spec.is_multi:
>> +                for index in range(len(raw)):
>> +                    if (type(raw[index]) =3D=3D int):
>> +                        enum_name =3D enum.entries_by_val[raw[index] -
>>i].name
>> +                        rsp[attr_spec['name']][index] =3D enum_name
>> +                return
>> +            else:
>> +                value =3D enum.entries_by_val[raw - i].name
>>          rsp[attr_spec['name']] =3D value
>
>Two asks:
>
>First this function stupidly looks at value-start. Best I can tell this is
>a leftover from when enum set was an array, but potentially "indexed with
>an offset" (ie. if value start =3D 10, first elem would have value 11, sec=
ond
>12 etc.). When we added support for sparse enums this was carried forward,
>but it's actually incorrect. entries_by_val is indexed with the real value=
,
>we should not subtract the start-value. So please send a patch to set i to
>0 at the start and ignore start-value here (or LMK if I should send one).
>
>Second, instead of fixing the value up here, after already putting it in
>the rsp - can we call this function to decode the enum before?
>A bit hard to explain, let me show you the diff of what I have in mind for
>the call site:
>
>diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index
>1b3a36fbb1c3..e2e8a8c5fb6b 100644
>--- a/tools/net/ynl/lib/ynl.py
>+++ b/tools/net/ynl/lib/ynl.py
>@@ -466,15 +466,15 @@ genl_family_name_to_id =3D None
>             else:
>                 raise Exception(f'Unknown {attr_spec["type"]} with name
>{attr_spec["name"]}')
>
>+            if 'enum' in attr_spec:
>+                decoded =3D self._decode_enum(rsp, attr_spec)
>+
>             if not attr_spec.is_multi:
>                 rsp[attr_spec['name']] =3D decoded
>             elif attr_spec.name in rsp:
>                 rsp[attr_spec.name].append(decoded)
>             else:
>                 rsp[attr_spec.name] =3D [decoded]
>-
>-            if 'enum' in attr_spec:
>-                self._decode_enum(rsp, attr_spec)
>         return rsp
>
>     def _decode_extack_path(self, attrs, attr_set, offset, target):
>
>Then _decode_enum() only has to ever deal with single values, and the
>caller will take care of mutli_attr like it would for any other type?

Sure, I will try to implement your proposal and send update here.

Thank you!
Arkadiusz

>--
>pw-bot: cr