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.gmei...@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
