[PATCH] netlink: decode libudev netlink header

[Harsha Sharma]

On Thu, Jan 4, 2018 at 7:09 AM, Chen Jingpiao <chenjingpiao at gmail.com> wrote:
> On Wed, 3 Jan 2018 18:33:46 +0530, Harsha Sharma wrote:
>> * netlink.c: decode family NETLINK_KOBJECT_UEVENT if prefix is libudev
>> * linux/netlink_kobject_uevent.h: New struct (udev_monitor_netlink_header)
>
> I think this header should not in linux directory.
> When adding a new file, we should add it in Makefile.am.
>
>>
>> Signed-off-by: Harsha Sharma <harshasharmaiitr at gmail.com>
>> ---
>>  linux/netlink_kobject_uevent.h | 16 ++++++++++++++++
>>  netlink.c                      | 27 ++++++++++++++++++++++++++-
>>  2 files changed, 42 insertions(+), 1 deletion(-)
>>  create mode 100644 linux/netlink_kobject_uevent.h
>>
>> diff --git a/linux/netlink_kobject_uevent.h
>> b/linux/netlink_kobject_uevent.h
>> new file mode 100644
>> index 00000000..d83ccad5
>> --- /dev/null
>> +++ b/linux/netlink_kobject_uevent.h
>> @@ -0,0 +1,16 @@
>> +#ifndef STRACE_LINUX_NETLINK_KOBJECT_UEVENT_H
>> +#define STRACE_LINUX_NETLINK_KOBJECT_UEVENT_H
>> +
>> +struct udev_monitor_netlink_header {
>> + char prefix[8];
>
> Reserve useful comment.
>
>> + unsigned int magic;
>> + unsigned int header_size;
>> + unsigned int properties_off;
>> + unsigned int properties_len;
>> + unsigned int filter_subsystem_hash;
>> + unsigned int filter_devtype_hash;
>> + unsigned int filter_tag_bloom_hi;
>> + unsigned int filter_tag_bloom_lo;
>> +};
>> +
>> +#endif /* !STRACE_LINUX_NETLINK_KOBJECT_UEVENT_H */
>> diff --git a/netlink.c b/netlink.c
>> index 6b9a1f5c..e991ed86 100644
>> --- a/netlink.c
>> +++ b/netlink.c
>> @@ -32,6 +32,7 @@
>>  #include "nlattr.h"
>>  #include <linux/audit.h>
>>  #include <linux/rtnetlink.h>
>> +#include <linux/netlink_kobject_uevent.h>
>>  #include <linux/xfrm.h>
>>  #include "xlat/netlink_ack_flags.h"
>>  #include "xlat/netlink_delete_flags.h"
>> @@ -183,6 +184,29 @@ decode_nlmsg_type_netfilter(const struct xlat *const
>> xlat,
>>   tprintf("%#x", msg_type);
>>  }
>>
>> +static bool
>> +decode_nlmsg_type_kobject_uevent(struct tcb *tcp, kernel_ulong_t addr,
>> + kernel_ulong_t len,
>> + const void *const opaque_data)
>> +{
>> + struct udev_monitor_netlink_header uh;
>> + const char *prefix = "libudev";
>> +
>> + if (len < sizeof(uh))
>> + return false;
>> + if (!umove_or_printaddr(tcp, addr, &uh) &&
>> +    strcmp(uh.prefix, prefix) == 0) {
>> + tprintf("{prefix=%s, magic=%u, header_size=%u, properties_off=%u,
>> properties_len=%u, filter_subsystem_hash=%u, filter_devtype_hash=%u,
>> filter_tag_bloom_hi=%u, filter_tag_bloom_lo=%u}", uh.prefix,
>
> More than 80 character.
>
>> +    uh.magic, uh.header_size, uh.properties_off,
>> +    uh.properties_len, uh.filter_subsystem_hash,
>> +    uh.filter_devtype_hash, uh.filter_tag_bloom_hi,
>> +    uh.filter_tag_bloom_lo);
>
> We can use macros in print_fields.h.
> I think magic, filter_tag_bloom_hi and filter_tag_bloom_lo sholud use other
> format.
>
> After the header have other messages.
>
>> + return true;
>> + }
>> + return false;
>
> If umove_or_printaddr return -1, you print address and string.
> If prefix is not "libudev", address addr to (addr + sizeof(uh)) will call
> umove
> again, we should reduce call syscall.
>
>> +}
>
> I think this function should separate into a single file.
>
>> +
>> +
>
> To more empty line.
>
>>  typedef void (*nlmsg_types_decoder_t)(const struct xlat *,
>>        uint16_t type,
>>        const char *dflt);
>> @@ -628,7 +652,8 @@ decode_netlink(struct tcb *const tcp,
>>   const int family = get_fd_nl_family(tcp, fd);
>>
>>   if (family == NETLINK_KOBJECT_UEVENT) {
>> - printstrn(tcp, addr, len);
>> + if (!decode_nlmsg_type_kobject_uevent(tcp, addr, len, NULL))
>
> I think this should be named decode_netlink_kobject_uevent, then ...
>
>> + printstrn(tcp, addr, len);
>
> ... this problem handle in this function.
>
> if (family == NETLINK_KOBJECT_UEVENT) {
> decode_netlink_kobject_uevent(tcp, addr, len);
> return;
> }
>
>>   return;
>>   }
>>
>> --
>> 2.11.0
>>
>
> Without a test.
>

Hello,
I have sent a patch v2 for the changes requested. Is it ok if I add
the tests for this in next patch ?
Thanks a lot for your review.

Regards,
Harsha Sharma

> --
> Chen Jingpiao