[PATCH v2 4/6] Add GPIO v2 ioctl decoding

[Kent Gibson]

On Tue, Jan 19, 2021 at 11:14:32PM +0300, Dmitry V. Levin wrote:
> On Mon, Jan 11, 2021 at 11:09:10PM +0800, Kent Gibson wrote:
> > Add decoding of GPIO uAPI v2 ioctls added in Linux v5.10.
> > 
> > Signed-off-by: Kent Gibson <warthog618 at gmail.com>
> > ---
[snip]

> > +
> > +static void
> > +print_gpio_v2_line_attribute(struct tcb *const tcp,
> > +			     struct gpio_v2_line_attribute *attr)
> > +{
> > +	switch (attr->id) {
> > +	case GPIO_V2_LINE_ATTR_ID_FLAGS:
> > +		PRINT_FIELD_FLAGS("", *attr, flags, gpio_v2_line_flags,
> > +				  "GPIO_V2_LINE_FLAG_???");
> > +		break;
> > +	case GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES:
> > +		PRINT_FIELD_X("", *attr, values);
> > +		break;
> > +	case GPIO_V2_LINE_ATTR_ID_DEBOUNCE:
> > +		PRINT_FIELD_U("", *attr, debounce_period_us);
> > +		break;
> > +	default:
> > +		tprintf("(id=%u)=%#" PRIx64, attr->id, (uint64_t)attr->values);
> > +		break;
> 
> We already discussed this before, but I'm still not quite happy with the
> output because (id=%u)=%#x is very odd looking thing indeed, it doesn't
> look similar to any valid C construct I'm aware of, and we strive to
> produce a C-like output.
> 
> What if we invent a non-exiting field and print it instead?  For example,
> 
> 	PRINT_FIELD_U("", *attr, id);
> #define unknown_data values
> 	PRINT_FIELD_X("", *attr, unknown_data);
> #undef unknown_data
> 

I'm ok with effectively decoding it raw, e.g. 
"{id=42, data=0x1234567812345678}", but I'm uncomfortable with the #define
and #undef, particularly if I were to call the field "data".
So I would lean towards a tprintf for this one:

		PRINT_FIELD_U("{", *attr, id);
		tprintf(", data=%#" PRIx64 "}", attr->values);

unless there is some other helper function that could do the same
without requiring preprocessor gymnastics.

Or are you trying to replace all instances of tprintf with a PRINT_xxx
variant?

> > +	}
> > +}
> > +
> > +static void
> > +print_gpio_v2_line_config_attribute(struct tcb *const tcp,
> > +				    struct gpio_v2_line_config_attribute *attr)
> > +{
> > +	tprints("{");
> > +	print_gpio_v2_line_attribute(tcp, &attr->attr);
> > +	PRINT_FIELD_X(", ", *attr, mask);
> > +	tprints("}");
> > +}
> > +
> > +static bool
> > +print_gpio_v2_line_attr_array_member(struct tcb *tcp, void *elem_buf,
> > +				     size_t elem_size, void *data)
> > +{
> > +	struct gpio_v2_line_attribute *la;
> > +
> > +	la = (struct gpio_v2_line_attribute *)elem_buf;
> > +	if (la->padding) {
> > +		PRINT_FIELD_X("{", *la, padding);
> > +		tprints(", ");
> > +	}
> > +	print_gpio_v2_line_attribute(tcp, la);
> > +	if (la->padding)
> > +		tprints("}");
> > +
> > +	return true;
> > +}
> 
> If struct gpio_v2_line_attribute a structure regardless of the value
> stored in la->padding, then I suggest to print "{" and "}"
> unconditionally.
> 

Logically, the array of line attributes is equivalent to a struct.
Each element in the array corresponds to one field in the logical struct.
The uAPI is written that way to allow the line configuration to be
flexible - only the fields of interest need to be passed, and new fields
can be defined in the future without breaking the ABI. 
Each struct gpio_v2_line_attribute is decoded as a single field=value,
based on the id field.  It is only decoded as a struct if the id is
unknown to the decoder - see the comment above.

The padding should be zeroed.  If not then it probably means the
protocol has been extended with fields this decoder doesn't understand,
and so how about we also fallback to raw decoding in this case, so
something like "{id=42, padding=0x12345678, data=0x8765432112345678}"?

The padding being non-zero may also mean the user doesn't understand
the protocol - which requires the padding to be zero or the request will
be rejected by the kernel - and I'm ok with decoding to a raw struct in
that case as well.

No problems with your other comments.

Cheers,
Kent.