Re: [PATCH net-next 07/10] genetlink: add genlmsg_iput() API

[Jiri Pirko <jiri@resnulli.us>]

Thu, Aug 10, 2023 at 06:13:36PM CEST, kuba@kernel.org wrote:
>On Thu, 10 Aug 2023 11:07:41 +0200 Jiri Pirko wrote:
>> >+#ifdef __LITTLE_ENDIAN
>> >+#define __GENL_PTR_LOW(byte)	((void *)(unsigned long)(byte))
>> >+#else
>> >+#define __GENL_PTR_LOW(byte)	\
>> >+	((void *)((unsigned long)(byte) << (BITS_PER_LONG - 8)))
>> >+#endif
>> >+
>> >+/**
>> >+ * GENL_INFO_NTF() - define genl_info for notifications
>> >+ * @__name: name of declared variable
>> >+ * @__family: pointer to the genetlink family
>> >+ * @__cmd: command to be used in the notification
>> >+ */
>> >+#define GENL_INFO_NTF(__name, __family, __cmd)			\
>> >+	struct genl_info __name = {				\
>> >+		.family = (__family),				\
>> >+		.genlhdr = (void *)&(__name.user_ptr[0]),	\
>> >+		.user_ptr[0] = __GENL_PTR_LOW(__cmd),		\  
>> 
>> Ugh. Took me some time to decypher what you do here. Having endian
>> specific code here seems quite odd to me. Why don't you have this as
>> static inline initializer function instead and use struct genlmsghdr
>> pointer to store cmd where it belong?
>> 
>> static inline void genl_info_ntf(struct genl_info *info,
>> 				 const struct genl_family *family, u8 cmd)
>> }
>> 	struct genlmsghdr *hdr = (void *) &info->user_ptr[0];
>> 
>> 	info->family = family;
>> 	info->genlhdr = hdr;
>> 	hdr->cmd = cmd;
>> }
>
>Nice! The endian magic is easily the nastiest part of this series.
>I considered making genlhdr a struct (rather than a pointer) because
>it's actually smaller than a pointer on 64b. But dunno, feels kinda
>weird to have a copy of the struct and a pointer to nlh. Hence the
>magic.
>
>And I was trying to save the 2 LoC and provide the DEFINE_ style macro
>but on second thought your init helper is cleaner. I don't like the
>DEFINE_ shit myself. I'll just throw in a memset(0) in there, and maybe
>add a verb to the name - genl_info_init_nft()?

Yep, looks fine to me. Thanks!