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

[Dmitry V. Levin]

On Wed, Jan 20, 2021 at 03:34:34PM +0800, Kent Gibson wrote:
> 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?

Yes, we are trying to replace all tprintf instances, but we don't have to
replace all of them right now, so let it remain tprintf here for a while.

You can add a short comment in the code telling why the field is printed
with a different name.

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

If this is the case, then I suggest turning
print_gpio_v2_line_attr_array_member into a trivial wrapper around
print_gpio_v2_line_attribute, similar to its neighbour
print_gpio_v2_line_config_attr_array_member being a trivial wrapper around
print_gpio_v2_line_config_attribute.

I suppose it's still worth printing the structure in
print_gpio_v2_line_attribute even if the padding is non-zero, this would
be consistent with print_gpio_v2_line_config, print_gpio_v2_line_info,
and print_gpio_v2_line_request.


-- 
ldv