Re: [libgpiod][PATCH 2/3] libgpiosim: new library for controlling the gpio-sim module

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Thu, Apr 29, 2021 at 1:23 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Thu, Apr 29, 2021 at 11:47:33AM +0200, Bartosz Golaszewski wrote:
> > Add a C library for controlling the gpio-sim kernel module from various
> > libgpiod test suites. This aims at replacing the old gpio-mockup module
> > and its user-space library - libgpio-mockup - in the project's tree.
>
> ...
>
> > +     rv = uname(&un);
> > +     if (rv)
> > +             return -1;
>
> Wondering why in all such cases instead of returning error code from upper
> layer we shadow it with -1.
>

Unlike the linux kernel - the error codes in libc are not returned as
return values of functions. Instead standard routines return -1 in
almost all error cases and set errno to indicate the error code. This
is why it's not necessary to propagate error numbers.

> ...
>
> > +GPIOSIM_API void gpiosim_ctx_unref(struct gpiosim_ctx *ctx)
> > +{
> > +     if (--ctx->refcnt == 0) {
>
>         if (--refcnt)
>                 return;
>
> ?
>

Matter of taste I suppose but whatever, I can change it.

>
> > +             close(ctx->pending_dir_fd);
> > +             close(ctx->live_dir_fd);
> > +
> > +             if (ctx->cfs_mnt_dir) {
> > +                     umount(ctx->cfs_mnt_dir);
> > +                     rmdir(ctx->cfs_mnt_dir);
> > +                     free(ctx->cfs_mnt_dir);
> > +             }
> > +
> > +             free(ctx);
> > +     }
> > +}
>
> ...
>
> > +/* We don't have mkdtempat()... :( */
>
> But we have tmpnam() / tmpnam_r(), why to reinvent it below?
>

Because of this:

$man tmpnam_r
...
The created pathname has a directory prefix P_tmpdir.
...

And this:

./stdio.h:120:# define P_tmpdir "/tmp"

> > +static char *make_random_dir_at(int at)
> > +{
> > +     static const char chars[] = "abcdefghijklmnoprstquvwxyz"
> > +                                 "ABCDEFGHIJKLMNOPRSTQUVWXYZ"
> > +                                 "0123456789";
> > +     static const unsigned int namelen = 16;
> > +
> > +     unsigned int idx, i;
> > +     char *name;
> > +     int ret;
> > +
> > +     name = malloc(namelen);
> > +     if (!name)
> > +             return NULL;
> > +
> > +again:
> > +     for (i = 0; i < namelen; i++) {
> > +             ret = getrandom(&idx, sizeof(idx), GRND_NONBLOCK);
> > +             if (ret != sizeof(idx)) {
> > +                     if (ret >= 0)
> > +                             errno = EAGAIN;
> > +
> > +                     free(name);
> > +                     return NULL;
> > +             }
> > +
> > +             name[i] = chars[idx % (GPIOSIM_ARRAY_SIZE(chars) - 1)];
> > +     }
> > +
> > +     ret = mkdirat(at, name, 0600);
> > +     if (ret) {
> > +             if (errno == EEXIST)
> > +                     goto again;
> > +
> > +             free(name);
> > +             return NULL;
> > +     }
> > +
> > +     return name;
> > +}
>
> ...
>
> > +     for (i = 0; i < num_names; i++) {
> > +             len = names[i] ? strlen(names[i]) : 0;
> > +             /* Length of the name + '"'x2 + ', '. */
>
> This x2 is  kinda hard to get on the first glance, perhaps:
>
>                 /* Length of the '"' + name + '"' + ', '. */
>
> ?
>
> > +             size += len + 4;
> > +     }
> > +
> > +     buf = malloc(size);
> > +     if (!buf)
> > +             return -1;
> > +
> > +     memset(buf, 0, size);
>
> calloc() ?
>


Nah, I prefer malloc() for single block allocs even if it needs a memset().

>
> > +     for (i = 0; i < num_names; i++)
> > +             written += snprintf(buf + written, size - written,
> > +                                 "\"%s\", ", names[i] ?: "");
> > +     buf[size - 2] = '\0';
>
> Dunno if you can use asprintf() and actually replace NULL by "" in the original
> array. Ah, see you already using it somewhere else, why not here?
>

Not sure what you mean, we can't use asprintf() to create a composite
string like what is needed here. Can you give me an example?

> ...
>
> > +     while (--tries) {
>
> I consider such loops better in a form of
>
>         do {
>                 ...
>         } while (--tries);
>

Sure.

Bart

>
> > +     }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>