[PATCH v2 2/6] Add GPIO ioctl decoding

[Kent Gibson]

On Tue, Jan 19, 2021 at 11:01:45PM +0300, Dmitry V. Levin wrote:
> On Mon, Jan 11, 2021 at 11:09:08PM +0800, Kent Gibson wrote:
> > Decode the GPIO character device ioctls first introduced in Linux v4.8,
> > as well as those added in subsequent kernel releases up to Linux v5.7.
> > 
> > Signed-off-by: Kent Gibson <warthog618 at gmail.com>
> > ---
[snip]

> > +static int
> > +print_gpioline_info_unwatch(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > +	tprintf(", {offset=%u}", (uint32_t)arg);
> 
> I'm not sure there is a real structure with an offset field, the kernel does
> 	__u32 offset;
> 	...
> 	if (copy_from_user(&offset, ip, sizeof(offset)))
> 		return -EFAULT;
> 
> Anyway, printing the uint32_t part of the address itself doesn't look
> particularly useful, maybe printnum_int(tcp, arg, "%u") on exiting syscall
> would be more appropriate here.
> 

Fair point on skipping the umove and printing the address here.
Not sure what I was thinking there.

Wrt the formatting, I'm interpreting the u32 as an offset field in a
struct as that is more informative than a bare u32.  I'm assuming
Bartosz defined that ioctl to pass the u32 rather than a struct to avoid
defining a single field struct, but I think that was wrong and I'm going
with the more informative formatting that is consistent with how that
value is presented in the other ioctls.

> > +
> > +	return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +#include "xlat/gpio_handle_flags.h"
> > +
> > +static int
> > +print_gpiohandle_request(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > +	struct gpiohandle_request hr;
> > +
> > +	if (entering(tcp))
> > +		tprints(", ");
> > +	else if (syserror(tcp))
> > +		return RVAL_IOCTL_DECODED;
> > +	else
> > +		tprints(" => ");
> > +
> > +	if (umove_or_printaddr(tcp, arg, &hr))
> > +		return RVAL_IOCTL_DECODED;
> > +
> > +	if (entering(tcp)) {
> > +		PRINT_FIELD_U("{", hr, lines);
> > +		tprints(", lineoffsets=");
> > +		if (hr.lines > GPIOHANDLES_MAX)
> > +			hr.lines = GPIOHANDLES_MAX;
> > +		print_local_array_ex(tcp, hr.lineoffsets, hr.lines,
> > +			sizeof(hr.lineoffsets[0]), print_uint32_array_member,
> > +			NULL, 0, NULL, NULL);
> 
> We've added PRINT_FIELD_ARRAY_UPTO recently, please consider using it
> instead, not only it would look simpler:
> 
> 		PRINT_FIELD_ARRAY_UPTO(", ", hr, lineoffsets, hr.lines, tcp,
> 				       print_uint32_array_member);
> 
> it would also help us in achieving our big hope to implement structured
> output in the future.
> 

Yay - not using code that didn't exist at the time.
Will certainly update to use this, and the others, now they are
available.

No issues with your other comments.

Cheers,
Kent.