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

[Sahil]

Hi,

On Friday, May 3, 2024 4:04:33 AM IST Dmitry V. Levin wrote:
> On Thu, May 02, 2024 at 11:59:34PM +0530, Sahil 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.

Sorry, I got a little confused with the naming of the function and that got
me thinking in this direction.

I have another query regarding "string_to_uint_ex". There's the following
condition in "string_to_uint_ex":
> if (str == end || val < 0 || (unsigned long long) val > max_val 
>     || (val == LLONG_MAX && errno == ERANGE)) {...}

Based on what I have understood this condition returns -1 when:
1. there are no digits in the string.
2. the number is negative.
3. ??
4. there's an overflow.

I haven't really understood the type casting part of the third condition.
For values -1 through LLONG_MIN, the second condition takes care of it.
And for 0 through LLONG_MAX, I don't think the explicit type cast would
make much of a difference.

In the case that there's an underflow (eg:, the value in the string is
LLONG_MIN - 1), strtoll would still return LLONG_MIN and set the errno
accordingly.

Could this function be refactored instead to use strtoull. Before calling this
function, a simple pass of "str" could be performed to check if there's a
"-" sign. If this is the case, the function immediately returns -1. Otherwise,
we could apply strtoull and check for overflow. Using strtoull would increase
the range of positive values that one can work with.

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

No, according to the eventfd(2) man page [1]:
> The maximum value that may be stored in the counter is the largest
> unsigned 64-bit value minus 1 (i.e., 0xfffffffffffffffe).

> [...]
> > > 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.

Understood. I'll take this approach.

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

Right, I think having nested "if" conditions should suffice.

Thanks,
Sahil

[1]. https://www.man7.org/linux/man-pages/man2/eventfd.2.html