[RFC PATCH v2 2/4] Add eventfd option to --decode-fds

[Sahil]

Hi,

Thank you for the review.

On Thursday, May 2, 2024 3:25:24 AM IST Dmitry V. Levin wrote:
> On Wed, May 01, 2024 at 10:57:59PM +0530, Sahil Siddiq wrote:
> [...]
> 
> > +static bool
> > +parse_fdinfo_efd_counter(const char *value, void *data)
> > +{
> > +	uint64_t *efd_counter = data;
> > +	*efd_counter = string_to_uint_ex(value, NULL, ULLONG_MAX, "\n", 16);
> 
> string_to_uint_ex() internally handles it as a "signed long long" value
> and rejects negative values as invalid, so even though ULLONG_MAX was
> specified, the actual limit imposed by this implementation is LLONG_MAX.

Oh, I missed this. Also, given that the name of the function is "string_to_uint_ex",
would it be better to use strtoull in this function and return an unsigned long long.
And then another function "string_to_int_ex" could be introduced that uses strtoll
and returns a long long?

> That is, you have either to open-code string_to_uint_ex here, or just
> treat it as a string.

In this case, shall I discard the first patch (passing base 10 in every invocation of
string_to_uint_ex)?

> > +	return true;
> > +}
> > +
> > +static bool
> > +printeventfd(pid_t pid_of_fd, int fd, const char *path)
> > +{
> > +	static const char eventfd_path[] = "anon_inode:[eventfd]";
> > +	static const char efd_counter_pfx[] = "eventfd-count:";
> > +	static const char efd_id_pfx[] = "eventfd-id:";
> > +
> > +	if (strcmp(path, eventfd_path))
> > +		return false;
> > +
> > +	uint64_t efd_counter = -1ULL;
> > +	int efd_id = -1;
> > +
> > +	scan_fdinfo(pid_of_fd, fd, efd_counter_pfx, sizeof(efd_counter_pfx) - 1,
> > +			   parse_fdinfo_efd_counter, &efd_counter);
> > +
> > +	scan_fdinfo(pid_of_fd, fd, efd_id_pfx, sizeof(efd_id_pfx) - 1,
> > +			   parse_fdinfo_efd_id, &efd_id);
> > +
> > +	if (efd_counter != -1ULL && efd_id != -1) {
> > +		tprint_associated_info_begin();
> > +		tprints_string("eventfd-count:");
> > +		PRINT_VAL_U(efd_counter);
> > +		tprint_arg_next();
> 
> tprint_arg_next() is for syscall and function arguments, here we probably
> need to introduce tprint_associated_info_next().

Got it.

> > +		tprints_string("eventfd-id:");
> > +		PRINT_VAL_U(efd_id);
> 
> For the record, "eventfd-count:" was introduced in v3.8-rc1~74^2~8, while
> "eventfd-id:" is available since v5.2-rc1~62^2~38.  It seems to me that it
> is worth printing eventfd-count if the kernel is not fresh enough.
> 
> I would even considered printing eventfd-semaphore that was introduced
> much later in v6.5-rc1~246^2~5.

One approach would be to check for all three values and print whatever it
finds. In such a case, the condition:

> if (efd_counter != -1ULL && efd_id != -1) {...}

could be split into two independent conditions. Something like:

> if (efd_counter != -1ULL) {...}
> if (efd_id != -1) {
> 	// continue printing within angle brackets
> } else { // nothing else to print }

Instead of unconditionally checking for all three values, would it be better
to use KERNEL_VERSION(a, b, c) defined in version.h?

Thanks,
Sahil