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

[Dmitry V. Levin]

On Thu, May 02, 2024 at 11:59:34PM +0530, Sahil wrote:
> 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.

To use strtoull, one has to explicitly disallow "-" because strtoull
accepts negative values.

> And then another function "string_to_int_ex" could be introduced that uses strtoll
> and returns a long long?

Given that string_to_uint_ex doesn't accept negative values, I don't see
why would we need string_to_int_ex.

> > 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;

By the way, is eventfd-count allowed, by any chance, to have -1ULL as
a valid value?

> > > +
> > > +	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

I'd consider extending scan_fdinfo.  For example,

struct scan_fdinfo {
	const char *prefix;
	size_t prefix_len;
	scan_fdinfo_fn fn;
	void *data;
};

static size_t
scan_fdinfo_array(pid_t pid_of_fd, int fd, scan_fdinfo_line *search_array,
		  size_t search_array_size);

The new function would lookup all values in a single pass,
and return the number of matches.

> 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?

Given that kernel commits are getting backported, I wouldn't recommend
relying on the kernel version.  However, I think we can safely assume, if
necessary, that if eventfd-count is not supported, then eventfd-id is also
not supported, and if eventfd-id is not supported, then eventfd-semaphore
is also not supported.


-- 
ldv