Re: [PATCH v3 0/3] tools/accounting: refactor delay fields and share format_timespec()

[wang.yaxin]

>>  CFLAGS := -I../include/uapi/
>>  
>>  PROGS := getdelays procacct delaytop
>> +OBJS := format_timespec.o
>>  
>>  all: $(PROGS)
>>  
>> +getdelays delaytop: %: %.o $(OBJS)
>> +	$(CC) $(LDFLAGS) -o $@ $^ $(LDLIBS)
>> +
>> +procacct: procacct.o
>> +	$(CC) $(LDFLAGS) -o $@ $^ $(LDLIBS)
>Does omitting $(CFLAGS) here break builds that require compiler flags during
>the link phase (such as -m32 for 32-bit cross-compilation)? 
>If CFLAGS is passed by the user or build system, it will be applied during
>object compilation but dropped during this link phase, which could cause
>architecture mismatches or missing symbol errors.

The Makefile's CFLAGS is only `-I../include/uapi/` — a compile-time flag.
The hypothetical `-m32` scenario requires explicit user override, which is
not how this Makefile is used in practice. Other kernel tools
(e.g. tools/mm/, tools/bpf/) also omit CFLAGS from link rules.

In our view, this review comment is not problematic, so no fix is needed.

>> +struct __kernel_old_timeval {
>> +	__kernel_long_t		tv_sec;
>> +	__kernel_long_t		tv_usec;
>> +};
>> +
>> +struct __kernel_old_timespec {
>> +	__kernel_long_t		tv_sec;
>> +	long			tv_nsec;
>Does missing the #ifndef __kernel_old_timeval guard around struct
>__kernel_old_timeval cause struct redefinition errors on architectures like
>sparc64, which define it as a macro in asm/posix_types.h?
>Also, does typing tv_nsec as long instead of __kernel_long_t in struct
>__kernel_old_timespec change the struct layout on x32, where long is 32-bit
>but __kernel_long_t is a 64-bit long long?

The issue pointed out in the first review comment does exist and will be
fixed in the next version.

The kernel's own `include/uapi/linux/time_types.h` uses plain `long`
for `tv_nsec`, not `__kernel_long_t`. The stub matches the kernel header
exactly 

In our view, the second review comment is not problematic, so no fix is needed.

>> @@ -728,10 +728,14 @@ static void fetch_and_fill_task_info(int pid, const char *comm)
>>  	nl_len = GENLMSG_PAYLOAD(&resp.n);
>>  	na = (struct nlattr *) GENLMSG_DATA(&resp);
>>  	while (nl_len > 0) {
>> +		if (na->nla_len < NLA_HDRLEN)
>> +			break;
>Could these netlink attribute header length checks be mentioned in the commit
>message or separated into a different patch?
>The commit message only describes simplifying the time_t overflow check
>and fixing a minor alignment issue, but the patch also silently introduces
>these minimum length checks in the attribute parsing loops across delaytop.c
>and getdelays.c.
>While these checks prevent infinite loops on malformed netlink messages,
>introducing functional changes without documenting them can make the review
>process more difficult.

We used to put modification logs in the changelog (on the cover), which is
not very intuitive. In the next version, we will update functional change
descriptions to the commit messages instead.

Thanks
Yaxin