[Mesa-dev] [PATCH 1/2] loader: add loader_open_name(..)

[Christian Gmeiner]

Hi Eric

Thanks for you code review!

Am Do., 2. Aug. 2018 um 13:09 Uhr schrieb Eric Engestrom
<eric.engestrom at intel.com>:
>
> On Wednesday, 2018-08-01 23:07:02 +0200, Christian Gmeiner wrote:
> > Add an improved drmOpenWithType(..) clone which fixes some serious
> > flaws. Some highlights:
> >  - using busid works only with PCI devices
> >  - open() w/o O_CLOEXEC
> >  - when build w/o udev - it creates a node: mkdir, chown(root), chmod, mknod
> >  - calls back into Xserver/DDX module
> >  - last but no least - borderline hacks with massive documentation [1]
> >    to keep this running.
> >
> > Signed-off-by: Christian Gmeiner <christian.gmeiner at gmail.com>
>
> The idea sounds good, but I have a few remarks below.
>
> > ---
> >  src/loader/loader.c | 79 +++++++++++++++++++++++++++++++++++++++++++++
> >  src/loader/loader.h |  3 ++
> >  2 files changed, 82 insertions(+)
> >
> > diff --git a/src/loader/loader.c b/src/loader/loader.c
> > index 43275484cc..c2d2122bb8 100644
> > --- a/src/loader/loader.c
> > +++ b/src/loader/loader.c
> > @@ -82,6 +82,85 @@ loader_open_device(const char *device_name)
> >     return fd;
> >  }
> >
> > +static int
> > +open_minor(int minor, int type)
> > +{
> > +   const char *dev_name;
> > +   char buf[64];
> > +
> > +   switch (type) {
> > +   case DRM_NODE_PRIMARY:
> > +      dev_name = DRM_DEV_NAME;
> > +      break;
> > +   case DRM_NODE_CONTROL:
> > +      dev_name = DRM_CONTROL_DEV_NAME;
> > +      break;
> > +   case DRM_NODE_RENDER:
> > +      dev_name = DRM_RENDER_DEV_NAME;
> > +      break;
> > +   default:
> > +      return -EINVAL;
>
> This function is returning an fd, so this should be -1.
> That said, giving an invalid type here is a serious misuse of the api,
> so I think you should replace the default return here and below ([1])
> with an `unreachable("invalid DRM node type");`
>

Good idea.

> > +   };
>
> Nit: no need for `;` after a switch
>
> > +
> > +   sprintf(buf, dev_name, DRM_DIR_NAME, minor);
>
> unlikely, but you should probably use snprintf(buf, sizeof(buf), ...)
>
> You should also use the `util_*` versions from u_string.h for
> compatibility with toolchains that don't support them natively.
>

Will be changed in v2.

> > +
> > +   return loader_open_device(buf);
> > +}
> > +
> > +static int minor_base(int type)
> > +{
> > +    switch (type) {
> > +    case DRM_NODE_PRIMARY:
> > +        return 0;
> > +    case DRM_NODE_CONTROL:
> > +        return 64;
> > +    case DRM_NODE_RENDER:
> > +        return 128;
> > +    default:
> > +        return -1;
>
> [1] ^ to replace with unreachable()
>

yep

> > +    };
>
> (`;` again)
>
> > +}
> > +
> > +int
> > +loader_open_name(const char *name, int type)
> > +{
> > +   int base = minor_base(type);
> > +   drmVersionPtr version;
> > +   int i, fd;
> > +   char *id;
> > +
> > +   if (base < 0)
> > +      return -1;
>
> with unreachable() above, this can go
>

correct

> > +
> > +   /*
> > +   * Open the first minor number that matches the driver name and isn't
> > +   * already in use.  If it's in use it will have a busid assigned already.
> > +   */
> > +   for (i = base; i < base + DRM_MAX_MINOR; i++) {
> > +      if ((fd = open_minor(i, type)) >= 0) {
> > +         if ((version = drmGetVersion(fd))) {
> > +            if (!strcmp(version->name, name)) {
>
> I would suggest inverting these `if`s to avoid nesting so deep.
>

Makes lot of sense - yes.

> > +               drmFreeVersion(version);
> > +               id = drmGetBusid(fd);
> > +               drmMsg("drmGetBusid returned '%s'\n", id ? id : "NULL");
> > +               if (!id || !*id) {
> > +                  if (id)
> > +                     drmFreeBusid(id);
> > +                  return fd;
>
> This condition looks wrong; surely you want to keep the one that has
> a bus id, not the one that doesn't?
>
> The `if(!id) if(id)` also looks weird; I suggest rewriting this bit of
> your code. Flattening your logic should also make it more readable.
>

Btw, you have just reviewed big parts of existing stuff I have taken
1:1 which can be found here :)
https://cgit.freedesktop.org/mesa/drm/tree/xf86drm.c

> > +               } else {
> > +                  drmFreeBusid(id);
> > +               }
> > +            } else {
> > +               drmFreeVersion(version);
> > +            }
> > +         }
> > +         close(fd);
> > +      }
> > +   }
> > +
> > +   return -1;
> > +}
> > +
> >  #if defined(HAVE_LIBDRM)
> >  #ifdef USE_DRICONF
> >  static const char __driConfigOptionsLoader[] =
> > diff --git a/src/loader/loader.h b/src/loader/loader.h
> > index 3859b45dc4..7e1612301a 100644
> > --- a/src/loader/loader.h
> > +++ b/src/loader/loader.h
> > @@ -38,6 +38,9 @@ extern "C" {
> >  int
> >  loader_open_device(const char *);
> >
> > +int
> > +loader_open_name(const char *name, int type);
> > +
> >  int
> >  loader_get_pci_id_for_fd(int fd, int *vendor_id, int *chip_id);
> >
> > --
> > 2.17.1
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev



-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info