On Tue, Aug 08, 2017 at 09:51:17PM +0800, JingPiao Chen wrote:
> On Tue, Aug 08, 2017 at 03:22:31PM +0300, Dmitry V. Levin wrote:
> > On Tue, Aug 08, 2017 at 03:07:40PM +0300, Dmitry V. Levin wrote:
> > > On Sun, Aug 06, 2017 at 07:52:17AM +0800, JingPiao Chen wrote:
> > > > On Sun, Aug 06, 2017 at 02:10:16AM +0300, Dmitry V. Levin wrote:
> > > > > On Tue, Aug 01, 2017 at 07:48:41AM +0800, JingPiao Chen wrote:
> > > > > > Prepare for NETLINK_KOBJECT_UEVENT decode. The messages
> > > > > > of NETLINK_KOBJECT_UEVENT do not contain nlmsghdr.
> > > > > [...]
> > > > > > (decode_nlmsghdr_with_payload): Skip family specific decoders
> > > > > > for type < NLMSG_MIN_TYPE && type != NLMSG_DONE.
> > > > > [...]
> > > > > > @@ -182,7 +177,7 @@ decode_nlmsg_type(const uint16_t type, const 
> > > > > > unsigned int family)
> > > > > >     const struct xlat *xlat = netlink_types;
> > > > > >     const char *dflt = "NLMSG_???";
> > > > > >  
> > > > > > -   if (type != NLMSG_DONE && family < ARRAY_SIZE(nlmsg_types)) {
> > > > > > +   if (type >= NLMSG_MIN_TYPE && family < ARRAY_SIZE(nlmsg_types)) 
> > > > > > {
> > > > > 
> > > > > Why?  How this is related to the rest of NETLINK_KOBJECT_UEVENT 
> > > > > change?
> > > > 
> > > > Previous code when type < NLMSG_MIN_TYPE && type != NLMSG_DONE,
> > > > family = -2 (NL_FAMILY_DEFAULT), family < ARRAY_SIZE(nlmsg_types) filter
> > > > this case. Now get family ignore the nlmsg_type.
> > > > When type < NLMSG_MIN_TYPE && type != NLMSG_DONE, family is not
> > > > a negative. family < ARRAY_SIZE(nlmsg_types) can not filter this case.
> > > > 
> > > > Related commit:
> > > > v4.17-43-g1b63425, v4.17-44-g8700030: These commit introduce 
> > > > get_fd_nl_family.
> > > > And get family only when type >= NLMSG_MIN_TYPE.
> > > > 
> > > > v4.17-123-g54aed21: This commit changed get family when
> > > > type >= NLMSG_MIN_TYPE || type == NLMSG_DONE.
> > > 
> > > Now that you change things to call get_fd_nl_family from decode_netlink
> > > unconditionally, family is available unconditionally, too (although it
> > > can be -1).  This is all clear enough but doesn't answer my question:
> > > 
> > > Why do you think it means that decode_nlmsg_type should not use
> > > family-specific decoders for message type < NLMSG_MIN_TYPE?
> > > Is decoding of these message types 100% family-agnostic?
> > > 
> > > It's so far from being obvious that if it's true, there must be a comment
> > > explaining why it's true.
> >
> > Now that you change things to call get_fd_nl_family from decode_netlink
> > unconditionally, family is available unconditionally, too (although it
> > can be -1).  This is all clear enough but doesn't answer my question:
> > 
> > Why do you think it means that decode_nlmsg_type should not use
> > family-specific decoders for message type < NLMSG_MIN_TYPE?
> > Is decoding of these message types 100% family-agnostic?
> > 
> > It's so far from being obvious that if it's true, there must be a comment
> > explaining why it's true.
> 
> include/linux/netlink.h:
> #define NLMSG_MIN_TYPE          0x10    /* < 0x10: reserved control messages 
> */
> 
> If use decode_nlmsg_type family-specific decoders for message
> type < NLMSG_MIN_TYPE, test will fail:
> -sendto(3, {len=16, type=NLMSG_ERROR, ...
> +sendto(3, {len=16, type=0x2 /* SOCK_DIAG_??? */, ...
> 
> comment:
>       /* type < NLMSG_MIN_TYPE are reserved control messages. */
> 
> > The same issue is with message type checks in decode_payload:
> > there has to be a comment explaining why family-specific netlink decoders
> > are not invoked for (nlmsg_type < 0 && nlmsg_type != NLMSG_DONE).
> 
> v4.17-123-g54aed21 commit message:
> While many NLMSG_DONE messages indeed have payload containing
> just one integer, there are exceptions. Handle this by passing
> payloads of NLMSG_DONE messages to family specific netlink
> payload decoders.
> 
> I shorten it as comment:
>       /*
>        * While many NLMSG_DONE messages indeed have payload
>        * containing just one integer, there are exceptions.
>        * Passing payloads of NLMSG_DONE messages to family
>        * specific netlink payload decoders.
>        */
> This comment only explain why invoke family-specific netlink decoders
> for nlmsg_type == NLMSG_DONE.
> 
> Are you prefer to handle type < NLMSG_MIN_TYPE in family-specific
> netlink decoders? (decode_netlink_sock_diag should make a change,
> decode_netlink_selinux and decode_netlink_crypto can handle now.)

Only if there is something family-specific to decode, otherwise no.

> Need I send version 2?

Yes, please send v2, this is the cheapest way to ensure that
none of your comments are lost.


-- 
ldv

Attachment: signature.asc
Description: PGP signature

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Strace-devel mailing list
Strace-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/strace-devel

Reply via email to