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

[Eric Engestrom]

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");`

> +   };

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.

> +
> +   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()

> +    };

(`;` 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

> +
> +   /*
> +   * 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.

> +               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.

> +               } 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