[PATCH] netlink: introduce nla_policy system to parse netlink attribute data

[Dmitry V. Levin]

On Sat, Jun 17, 2017 at 08:55:29PM +0800, JingPiao Chen wrote:
> On Sat, Jun 17, 2017 at 03:27:30PM +0300, Dmitry V. Levin wrote:
> > On Sat, Jun 17, 2017 at 07:18:29PM +0800, JingPiao Chen wrote:
> > > On Sat, Jun 17, 2017 at 12:35:15AM +0300, Dmitry V. Levin wrote:
> > > > On Sat, May 20, 2017 at 07:42:19PM +0800, JingPiao Chen wrote:
> > > [...]
> > > > > diff --git a/netlink.c b/netlink.c
> > > > > index 52f3806..cadfccb 100644
> > > > > --- a/netlink.c
> > > > > +++ b/netlink.c
> > > > > @@ -78,9 +78,138 @@ print_nlattr(const struct nlattr *const nla,
> > > > >  }
> > > > >  
> > > > >  static void
> > > > > -decode_nlattr_with_data(struct tcb *tcp, kernel_ulong_t addr,
> > > > > -			kernel_ulong_t len, const struct xlat *table,
> > > > > -			const char *dflt)
> > > > > +decode_nlattr_data(struct tcb *tcp,
> > > > > +		   kernel_ulong_t addr,
> > > > > +		   kernel_ulong_t len,
> > > > > +		   unsigned int nla_type,
> > > > > +		   unsigned int policy_size,
> > > > > +		   const struct nla_policy *policy,
> > > > > +		   bool(*fallback_parser)(struct tcb *,
> > > > > +					  kernel_ulong_t addr,
> > > > > +					  kernel_ulong_t len,
> > > > > +					  int nla_type,
> > > > > +					  void *opaque_data),
> > > > > +		   void *const opaque_data)
> > > > > +{
> > > > > +	if (!policy || nla_type >= policy_size) {
> > > > > +		printstrn(tcp, addr, len);
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > > > +	switch (policy[nla_type].type) {
> > > > > +	case NLA_U8: {
> > > > > +		uint8_t num;
> > > > > +
> > > > > +		if (len < sizeof(num))
> > > > > +			printstr_ex(tcp, addr, len, QUOTE_FORCE_HEX);
> > > > > +		else if (!umove_or_printaddr(tcp, addr, &num))
> > > > > +			tprintf("%" PRIu8, num);
> > > > > +		break;
> > > > > +	}
> > > > > +	case NLA_U16: {
> > > > > +		uint16_t num;
> > > > > +
> > > > > +		if (len < sizeof(num))
> > > > > +			printstr_ex(tcp, addr, len, QUOTE_FORCE_HEX);
> > > > > +		else if (!umove_or_printaddr(tcp, addr, &num))
> > > > > +			tprintf("%" PRIu16, num);
> > > > > +		break;
> > > > > +	}
> > > > > +	case NLA_U32: {
> > > > > +		uint32_t num;
> > > > > +
> > > > > +		if (len < sizeof(num))
> > > > > +			printstr_ex(tcp, addr, len, QUOTE_FORCE_HEX);
> > > > > +		else if (!umove_or_printaddr(tcp, addr, &num))
> > > > > +			tprintf("%" PRIu32, num);
> > > > > +		break;
> > > > > +	}
> > > > > +	case NLA_MSECS:
> > > > > +	case NLA_U64: {
> > > > > +		uint64_t num;
> > > > > +
> > > > > +		if (len < sizeof(num))
> > > > > +			printstr_ex(tcp, addr, len, QUOTE_FORCE_HEX);
> > > > > +		else if (!umove_or_printaddr(tcp, addr, &num))
> > > > > +			tprintf("%" PRIu64, num);
> > > > > +		break;
> > > > > +	}
> > > > > +	case NLA_STRING: {
> > > > > +		uint16_t str_len = len;
> > > > > +
> > > > > +		if (policy[nla_type].len && policy[nla_type].len < len)
> > > > > +			str_len = policy[nla_type].len;
> > > > > +		printstrn(tcp, addr, str_len);
> > > > > +		break;
> > > > > +	}
> > > > > +	case NLA_NUL_STRING:
> > > > > +		printstr(tcp, addr);
> > > > > +		break;
> > > > > +	case NLA_S8: {
> > > > > +		int8_t num;
> > > > > +
> > > > > +		if (len < sizeof(num))
> > > > > +			printstr_ex(tcp, addr, len, QUOTE_FORCE_HEX);
> > > > > +		else if (!umove_or_printaddr(tcp, addr, &num))
> > > > > +			tprintf("%" PRId8, num);
> > > > > +		break;
> > > > > +	}
> > > > > +	case NLA_S16: {
> > > > > +		int16_t num;
> > > > > +
> > > > > +		if (len < sizeof(num))
> > > > > +			printstr_ex(tcp, addr, len, QUOTE_FORCE_HEX);
> > > > > +		else if (!umove_or_printaddr(tcp, addr, &num))
> > > > > +			tprintf("%" PRId16, num);
> > > > > +		break;
> > > > > +	}
> > > > > +	case NLA_S32: {
> > > > > +		int32_t num;
> > > > > +
> > > > > +		if (len < sizeof(num))
> > > > > +			printstr_ex(tcp, addr, len, QUOTE_FORCE_HEX);
> > > > > +		else if (!umove_or_printaddr(tcp, addr, &num))
> > > > > +			tprintf("%" PRId32, num);
> > > > > +		break;
> > > > > +	}
> > > > > +	case NLA_S64: {
> > > > > +		int64_t num;
> > > > > +
> > > > > +		if (len < sizeof(num))
> > > > > +			printstr_ex(tcp, addr, len, QUOTE_FORCE_HEX);
> > > > > +		else if (!umove_or_printaddr(tcp, addr, &num))
> > > > > +			tprintf("%" PRId64, num);
> > > > > +		break;
> > > > > +	}
> > > > 
> > > > All these numeric cases look very similar, could you think of something
> > > > that would save us from these cut-n-paste lines?
> > > > 
> > > 
> > > Use macro, Is this ok?
> > > 
> > > 	switch (policy[nla_type].type) {
> > > #define CASE_DECODE_INTEGER(type, data_type, fmt)	\
> > > 	case type: {					\
> > > 		data_type num;				\
> > > 							\
> > > 		if (len < sizeof(num))			\
> > > 			printstr_ex(tcp, addr, len, QUOTE_FORCE_HEX); \
> > > 		else if (!umove_or_printaddr(tcp, addr, &num)) \
> > > 			tprintf(fmt, num);		\
> > > 		break;					\
> > > 	}						
> > > 
> > > 	CASE_DECODE_INTEGER(NLA_U8, uint8_t, "%" PRIu8)
> > > 	CASE_DECODE_INTEGER(NLA_U16, uint16_t, "%" PRIu16)
> > > 	CASE_DECODE_INTEGER(NLA_U32, uint32_t, "%" PRIu32)
> > > 	CASE_DECODE_INTEGER(NLA_U64, uint64_t, "%" PRIu64)
> > > 	CASE_DECODE_INTEGER(NLA_S8, int8_t, "%" PRId8)
> > > 	CASE_DECODE_INTEGER(NLA_S16, int16_t, "%" PRId16)
> > > 	CASE_DECODE_INTEGER(NLA_S32, int32_t, "%" PRId32)
> > > 	CASE_DECODE_INTEGER(NLA_S64, int64_t, "%" PRId64)
> > > 	CASE_DECODE_INTEGER(NLA_MSECS, uint64_t, "%" PRIu64)
> > 
> > This is better, but yet you'd create two different parsers
> > for NLA_U64 and NLA_MSECS which are identical.
> > 
> > Would it be better if we introduced functions for parsing each nla type?
> 
> I prefer to introduced functions for parsing each nla type,
> Like this:
> 
> typedef (*nla_decoder_t)(struct tcb *,

Do not forget about the return code type.

> 			 kernel_ulong_t addr,
> 			 kernel_ulong_t len,
> 			 void *opaque_data);
> static const nla_decoder_t unix_nla_decoders[] = {
> 	[UNIX_DIAG_PEER] = decode_nla_u32,
> 	[UNIX_DIAG_ICONS] = decode_unix_diag_icons
> };

Go ahead then. :)


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20170617/8acb3eb8/attachment.bin>