Re: [libgpiod] libgpiod v2.0 API discussion

[Helmut Grohne <helmut.grohne@xxxxxxxxxx>]

On Tue, Nov 03, 2020 at 09:42:23PM +0100, Bartosz Golaszewski wrote:
> * Line lookup by name
> 
> One thing that was omitted in the first version was the fact that GPIO
> line names are not unique in the kernel. Functions that do name lookup
> from a single chip should return bulk objects. Functions that can
> lookup several lines with different names should probably be removed
> because it's difficult to design an elegant API that would take this
> non-uniqueness into account. Returning arrays of bulks sounds like a
> bad idea.

While there is no guarantuee that line names are unique in general, that
tends to be the case in a number of practical cases. Notably, embedded
system developers (which seem like a big part of the intended audience)
are in control of line names and can ensure their uniqueness. It
certainly makes sense to offer an API that deals with non-unique line
names.  However, I think that keeping a less flexible convenience helper
for looking up a line by supposedly unique name would be good. In case
the uniqueness is violated, I'd expect an error.

There is ENOTUNIQ. It is rarely used and while the description says that
it is network-related, it is used for inifiband, nvme, virtualbox guest
functionality and cifs beyond decnet and 802.11.

> * Reading line events
> 
> Since we now can have a single file descriptor for multiple monitored
> lines - reading of the events needs to be modified. Event structure
> needs to reference the line object with which it's associated. Adding
> additional fields for the seq numbers is a no-brainer of course.
> 
> I'm wondering if we should still be using struct timespec for
> timestamps. On one hand it's used everywhere in libc but it's also
> subject to ABI change due to year 2036 problem. Maybe we should switch
> to nanoseconds in 64-bit integer just like what we get from the kernel
> but then user-space won't have standard tools to deal with such
> timestamps.

Aside: It's 2038, not 2036. The key is that you thought of this. Thanks.

The story for timespec is a little difficult for some time to come:
 * On 64bit architectures and some 32bit architectures (e.g. x32), using
   timespec doesn't pose a 2038 problem.
 * On the kernel side, using timespec has become hard. However there now
   is a timespec64 which is not encumbered with 2038 problems on any
   architecture on the kernel ABI side.
 * On the userspace side, opting into timespec64 is still quite hard. I
   think that the idea was to follow the process of widening off_t to
   64 bits. So userspace won't get a timespec64, but instead will be
   able to choose between 2038-buggy timespec and timespec64 using a
   macro. glibc will do the translation (truncation) for many syscalls
   to retain backwards compatibility. Unless I am mistaken, the macro is
   supposed to be _TIME_BITS=64, but that doesn't seem to have
   progressed very far.

A real implication here is that unless all of your users enable this
extra macro, they'll be affected by the problem on most 32bit
architectures when you use timespec even if you use timespec64 on the
kernel side.

On the C++ side, ktime_t values can be stored in a suitable std::chrono
clock without the need for a conversion. The released libgpiod however
transforms it to timespec and then reconstructs the original value for
std::chrono usage. In doing so it introduces a 2038 problem entirely on
the userspace side besides incurring integer arithmetic that can be slow
on certain arm processors. Please consider offering a libgpiod C-API
that allows using the kernel time represntation without conversion.

> * Opaque data types
> 
> For events the reason is simple: with opaque types we'd face an
> overhead with malloc() and I think it's important that we can read
> them as fast as possible.

+1 to this part

Helmut