Hi Eric Thanks for you code review!
Am Do., 2. Aug. 2018 um 13:09 Uhr schrieb Eric Engestrom <eric.engest...@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.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");` > 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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev -- greets -- Christian Gmeiner, MSc https://christian-gmeiner.info _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev