On 22/02/2014 16:12, Guillem Jover wrote: > Hi, > > Just a quick review skimming over. The C code has a mix of indentation > by tab and spaces, not just 4 spaces, no cudled braces, etc. I've > ignored other style issues too.
I just went through the style rules in http://www.x.org/wiki/CodingStyle/ and applied them to devd.c. Hopefully all style issues are solved now. >> +++ b/config/devd.c >> @@ -0,0 +1,365 @@ >> +/* >> + * Copyright ?? 2012 Baptiste Daroussin > > Encoding issue? Should be ©, I guess. It's © in the actual file - just checked. >> + if (sysctlbyname(name, NULL, &len, NULL, 0) == 0) { >> + dest = malloc(len + 1); > > malloc() return value unchecked. Fixed, thanks. >> + walk = strchr(line, ' '); >> + if (walk != NULL) >> + walk[0] = '\0'; > > You could do this (and the one from device_remove()) in the single > caller where it seems to matter, that is wakeup_handler(), and pass > a devicename (or similar) as argument instead of the generic line. Done. >> + if (asprintf(&path, "/dev/%s", line) == -1) >> + return; >> + >> + options = input_option_new(NULL, "_source", "server/devd"); >> + if (!options) >> + return; > > Leaks path. Fixed, thanks. >> + vendor = sysctl_get_str("dev.%s.%s.%%desc", hw_types[i].driver, line + >> strlen(hw_types[i].driver)); >> + if (vendor == NULL) { >> + attrs.vendor = strdup("(unnamed)"); >> + } else { >> + if ((product = strchr(vendor, ' ')) != NULL) { >> + product[0] = '\0'; >> + product++; >> + } >> + attrs.vendor = strdup(vendor); >> + if (product != NULL && (walk = strchr(product, ',')) != NULL) >> + walk[0] = '\0'; >> + attrs.product = strdup(product != NULL ? product : "(unnamed)"); >> + options = input_option_new(options, "name", product != NULL ? >> product : "(unnamed)"); > > You could compute the product once before the conditional (as it's > used later on too), Done. >> + if (rc != Success) >> + goto unwind; > > A bit useless, although future-proof. I suppose that's what Baptiste had in mind. The compiler clears this out anyway. >> + return; > > Useless return. Removed. >> +static void >> +device_removed(char *line) >> +{ >> + char *walk; >> + char *value; >> + >> + walk = strchr(line, ' '); >> + if (walk != NULL) >> + walk[0] = '\0'; > > Same comment here as in device_added. Fixed likewise. >> +static ssize_t >> +socket_getline(int fd, char **out) >> +{ >> + char *buf; >> + ssize_t ret, cap, sz = 0; >> + char c; >> + >> + cap = 1024; >> + buf = malloc(cap * sizeof(char)); > > Useless sizeof(), Useless to the compiler, like many other things, but not necessarily useless to the reader. As far as I can see this is an established practice in xserver codebase. Some cases go as far as specifying signedness: config/hal.c: ret = calloc(sizeof(char), len + i); /* i - 1 commas, 1 NULL */ dix/gc.c: dash = malloc(2 * sizeof(unsigned char)); dix/gc.c: dash = malloc(pgcSrc->numInDashList * sizeof(unsigned char)); dix/gc.c: p = malloc(2 * ndash * sizeof(unsigned char)); dix/gc.c: p = malloc(ndash * sizeof(unsigned char)); . . etc > realloc() can do the initial alloc if buf is NULL. What would be the point in that? This change pessimizes on the common case by adding two extra in-loop branches, without actually saving any cost since the allocation needs to happen anyway. >> + for (;;) { >> + ret = read(sock_devd, &c, 1); >> + if (ret < 1) { >> + free(buf); >> + return -1; >> + } > > You might want to check for EINTR here. Fixed, thanks. >> + if (c == '\n') >> + break; >> + >> + if (sz + 1 >= cap) { >> + cap *= 2; >> + buf = realloc(buf, cap *sizeof(char)); > > Leaks on failure, Fixed, thanks. >> +int >> +config_devd_init(void) >> +{ >> + struct sockaddr_un devd; >> + char devicename[1024]; >> + int i, j; >> + >> + /* first scan the sysctl to determine the hardware if needed */ >> + >> + for (i = 0; hw_types[i].driver != NULL; i++) { >> + for (j = 0; sysctl_exists("dev.%s.%i.%%desc", hw_types[i].driver, >> j); j++) { >> + snprintf(devicename, 1024, "%s%i", hw_types[i].driver, j); > > sizeof(devicename) instead of hard-coding 1024, which can easily get > out of sync. Fixed, thanks. >> + if (connect(sock_devd, (struct sockaddr *) &devd, sizeof(struct >> sockaddr_un)) < 0) { > > Better sizeof(devd), it will always match the variable type, > and it's shorter so the line fits in 80 chars. Fixed, thanks. >> -#if defined(CONFIG_HAL) || defined(CONFIG_UDEV) || defined(CONFIG_WSCONS) >> +#if defined(CONFIG_HAL) || defined(CONFIG_UDEV) || defined(CONFIG_WSCONS) >> || defined(CONFIG_DEVD) >> .forceInputDevices = FALSE, >> .autoAddDevices = TRUE, >> .autoEnableDevices = TRUE, > > You might want to wrap these long lines to 80 chars. Done. On 22/02/2014 17:15, Guillem Jover wrote: >>> +static void >>> +device_added(char *line) >>> +{ > >>> + vendor = sysctl_get_str("dev.%s.%s.%%desc", hw_types[i].driver, line + >>> strlen(hw_types[i].driver)); >>> + if (vendor == NULL) { >>> + attrs.vendor = strdup("(unnamed)"); >>> + } else { >>> + if ((product = strchr(vendor, ' ')) != NULL) { >>> + product[0] = '\0'; >>> + product++; >>> + } >>> + attrs.vendor = strdup(vendor); >>> + if (product != NULL && (walk = strchr(product, ',')) != NULL) >>> + walk[0] = '\0'; >>> + attrs.product = strdup(product != NULL ? product : "(unnamed)"); >>> + options = input_option_new(options, "name", product != NULL ? >>> product : "(unnamed)"); >>> + } >>> + attrs.usb_id = NULL; >>> + attrs.device = strdup(path); >>> + options = input_option_new(options, "driver", hw_types[i].xdriver); > >>> + rc = NewInputDeviceRequest(options, &attrs, &dev); > > Ah, missed this one, you might want to check how NewInputDeviceRequest() > copes with NULL attributes if strdup() fails, and if you might need to > bail on one of those strdup() failures. Looks safe to me. The codepaths lead to MatchAttrToken() and DuplicateInputAttributes(), neither of which assumes any of the attributes to be non-NULL. -- Robert Millan
>From d96e2bd2a2b48ede527ad7071d3e0eeda9861b73 Mon Sep 17 00:00:00 2001 From: Robert Millan <[email protected]> Date: Mon, 24 Feb 2014 23:22:57 +0100 Subject: [PATCH] Add devd config backend for FreeBSD (and GNU/kFreeBSD) Based on original code by Baptiste Daroussin, with some fixes made by Koop Mast and myself. Signed-off-by: Robert Millan <[email protected]> --- config/Makefile.am | 4 + config/config-backends.h | 5 + config/config.c | 5 + config/devd.c | 387 +++++++++++++++++++++++++++++++++++++++ configure.ac | 16 ++ hw/xfree86/common/xf86Config.c | 7 +- hw/xfree86/common/xf86Globals.c | 3 +- include/dix-config.h.in | 3 + 8 files changed, 427 insertions(+), 3 deletions(-) create mode 100644 config/devd.c diff --git a/config/Makefile.am b/config/Makefile.am index 0e20e8b..16f8aaa 100644 --- a/config/Makefile.am +++ b/config/Makefile.am @@ -34,6 +34,10 @@ if CONFIG_WSCONS libconfig_la_SOURCES += wscons.c endif # CONFIG_WSCONS +if CONFIG_DEVD +libconfig_la_SOURCES += devd.c +endif + endif # !CONFIG_HAL endif # !CONFIG_UDEV diff --git a/config/config-backends.h b/config/config-backends.h index 5f07557..889e2ad 100644 --- a/config/config-backends.h +++ b/config/config-backends.h @@ -44,3 +44,8 @@ void config_hal_fini(void); int config_wscons_init(void); void config_wscons_fini(void); #endif + +#ifdef CONFIG_DEVD +int config_devd_init(void); +void config_devd_fini(void); +#endif diff --git a/config/config.c b/config/config.c index 760cf19..cca5a31 100644 --- a/config/config.c +++ b/config/config.c @@ -53,6 +53,9 @@ config_init(void) #elif defined(CONFIG_WSCONS) if (!config_wscons_init()) ErrorF("[config] failed to initialise wscons\n"); +#elif defined(CONFIG_DEVD) + if (!config_devd_init()) + ErrorF("[config] failed to initialise devd\n"); #endif } @@ -65,6 +68,8 @@ config_fini(void) config_hal_fini(); #elif defined(CONFIG_WSCONS) config_wscons_fini(); +#elif defined(CONFIG_DEVD) + config_devd_fini(); #endif } diff --git a/config/devd.c b/config/devd.c new file mode 100644 index 0000000..92a66c4 --- /dev/null +++ b/config/devd.c @@ -0,0 +1,387 @@ +/* + * Copyright © 2012 Baptiste Daroussin + * Copyright © 2014 Robert Millan + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + * + * Author: Baptiste Daroussin <[email protected]> + */ + +#ifdef HAVE_DIX_CONFIG_H +#include <dix-config.h> +#endif + +#include <sys/types.h> +#include <sys/socket.h> +#include <sys/sysctl.h> +#include <sys/un.h> + +#include <ctype.h> +#include <errno.h> +#include <fcntl.h> +#include <stdlib.h> +#include <stdio.h> +#include <stdarg.h> +#include <stdbool.h> +#include <unistd.h> + +#include "input.h" +#include "inputstr.h" +#include "hotplug.h" +#include "config-backends.h" +#include "os.h" + +#define DEVD_SOCK_PATH "/var/run/devd.pipe" + +#define DEVD_EVENT_ADD '+' +#define DEVD_EVENT_REMOVE '-' + +static int sock_devd = -1; + +struct hw_type { + const char *driver; + int flag; + const char *xdriver; +}; + +static struct hw_type hw_types[] = { + {"ukbd", ATTR_KEYBOARD, "kbd"}, + {"atkbd", ATTR_KEYBOARD, "kbd"}, + {"ums", ATTR_POINTER, "mouse"}, + {"psm", ATTR_POINTER, "mouse"}, + {"uhid", ATTR_POINTER, "mouse"}, + {"joy", ATTR_JOYSTICK, NULL}, + {"atp", ATTR_TOUCHPAD, NULL}, + {"uep", ATTR_TOUCHSCREEN, NULL}, + {NULL, -1, NULL}, +}; + +static bool +sysctl_exists(const char *format, ...) +{ + va_list args; + char *name = NULL; + size_t len; + int ret; + + if (format == NULL) + return false; + + va_start(args, format); + vasprintf(&name, format, args); + va_end(args); + + ret = sysctlbyname(name, NULL, &len, NULL, 0); + + if (ret == -1) + len = 0; + + free(name); + return (len > 0); +} + +static char * +sysctl_get_str(const char *format, ...) +{ + va_list args; + char *name = NULL; + char *dest = NULL; + size_t len; + + if (format == NULL) + return NULL; + + va_start(args, format); + vasprintf(&name, format, args); + va_end(args); + + if (sysctlbyname(name, NULL, &len, NULL, 0) == 0) { + dest = malloc(len + 1); + if (!dest) + goto unwind; + if (sysctlbyname(name, dest, &len, NULL, 0) == 0) + dest[len] = '\0'; + else { + free(dest); + dest = NULL; + } + } + + unwind: + free(name); + return dest; +} + +static void +device_added(char *devname) +{ + char path[PATH_MAX]; + char *vendor; + char *product = NULL; + char *config_info = NULL; + char *walk; + InputOption *options = NULL; + InputAttributes attrs = { }; + DeviceIntPtr dev = NULL; + int i, rc; + int fd; + + for (i = 0; hw_types[i].driver != NULL; i++) { + if (strncmp(devname, hw_types[i].driver, + strlen(hw_types[i].driver)) == 0 && + isdigit(*(devname + strlen(hw_types[i].driver)))) { + attrs.flags |= hw_types[i].flag; + break; + } + } + if (hw_types[i].driver == NULL) { + LogMessageVerb(X_INFO, 10, "config/devd: ignoring device %s\n", + devname); + return; + } + if (hw_types[i].xdriver == NULL) { + LogMessageVerb(X_INFO, 10, "config/devd: ignoring device %s\n", + devname); + return; + } + snprintf(path, sizeof(path), "/dev/%s", devname); + + options = input_option_new(NULL, "_source", "server/devd"); + if (!options) + return; + + vendor = + sysctl_get_str("dev.%s.%s.%%desc", hw_types[i].driver, + devname + strlen(hw_types[i].driver)); + if (vendor == NULL) { + attrs.vendor = strdup("(unnamed)"); + attrs.product = strdup("(unnamed)"); + } + else { + if ((walk = strchr(vendor, ' ')) != NULL) { + walk[0] = '\0'; + walk++; + product = walk; + if ((walk = strchr(product, ',')) != NULL) + walk[0] = '\0'; + } + + attrs.vendor = strdup(vendor); + if (product) + attrs.product = strdup(product); + else + attrs.product = strdup("(unnamed)"); + + options = input_option_new(options, "name", xstrdup(attrs.product)); + + free(vendor); + } + attrs.usb_id = NULL; + attrs.device = strdup(path); + options = input_option_new(options, "driver", hw_types[i].xdriver); + if (attrs.flags & ATTR_KEYBOARD) { + /* + * Don't pass device option if keyboard is attached to console (open fails), + * thus activating special logic in xf86-input-keyboard. + */ + fd = open(path, O_RDONLY | O_NONBLOCK | O_EXCL); + if (fd > 0) { + close(fd); + options = input_option_new(options, "device", xstrdup(path)); + } + } + else { + options = input_option_new(options, "device", xstrdup(path)); + } + + if (asprintf(&config_info, "devd:%s", devname) == -1) { + config_info = NULL; + goto unwind; + } + + if (device_is_duplicate(config_info)) { + LogMessage(X_WARNING, "config/devd: device %s already added. " + "Ignoring.\n", attrs.product); + goto unwind; + } + + options = input_option_new(options, "config_info", config_info); + LogMessage(X_INFO, "config/devd: adding input device %s (%s)\n", + attrs.product, path); + + rc = NewInputDeviceRequest(options, &attrs, &dev); + + if (rc != Success) + goto unwind; + + unwind: + free(config_info); + input_option_free_list(&options); + + free(attrs.usb_id); + free(attrs.product); + free(attrs.device); + free(attrs.vendor); +} + +static void +device_removed(char *devname) +{ + char *value; + + if (asprintf(&value, "devd:%s", devname) == -1) + return; + + remove_devices("devd", value); + + free(value); +} + +static ssize_t +socket_getline(int fd, char **out) +{ + char *buf, *newbuf; + ssize_t ret, cap, sz = 0; + char c; + + cap = 1024; + buf = malloc(cap * sizeof(char)); + if (!buf) + return -1; + + for (;;) { + ret = read(sock_devd, &c, 1); + if (ret < 1) { + if (errno == EINTR) + continue; + free(buf); + return -1; + } + + if (c == '\n') + break; + + if (sz + 1 >= cap) { + cap *= 2; + newbuf = realloc(buf, cap * sizeof(char)); + if (!newbuf) { + free(buf); + return -1; + } + buf = newbuf; + } + buf[sz] = c; + sz++; + } + + buf[sz] = '\0'; + if (sz >= 0) + *out = buf; + else + free(buf); + + return sz; /* number of bytes in the line, not counting the line break */ +} + +static void +wakeup_handler(void *data, int err, void *read_mask) +{ + char *line = NULL; + char *walk; + + if (err < 0) + return; + + if (FD_ISSET(sock_devd, (fd_set *) read_mask)) { + if (socket_getline(sock_devd, &line) < 0) + return; + + walk = strchr(line + 1, ' '); + if (walk != NULL) + walk[0] = '\0'; + + switch (*line) { + case DEVD_EVENT_ADD: + device_added(line + 1); + break; + case DEVD_EVENT_REMOVE: + device_removed(line + 1); + break; + default: + break; + } + free(line); + } +} + +static void +block_handler(void *data, struct timeval **tv, void *read_mask) +{ +} + +int +config_devd_init(void) +{ + struct sockaddr_un devd; + char devicename[1024]; + int i, j; + + /* first scan the sysctl to determine the hardware if needed */ + + for (i = 0; hw_types[i].driver != NULL; i++) { + for (j = 0; sysctl_exists("dev.%s.%i.%%desc", hw_types[i].driver, j); + j++) { + snprintf(devicename, sizeof(devicename), "%s%i", hw_types[i].driver, + j); + device_added(devicename); + } + + } + sock_devd = socket(AF_UNIX, SOCK_STREAM, 0); + if (sock_devd < 0) { + ErrorF("config/devd: Fail opening stream socket"); + return 0; + } + + devd.sun_family = AF_UNIX; + strlcpy(devd.sun_path, DEVD_SOCK_PATH, sizeof(devd.sun_path)); + + if (connect(sock_devd, (struct sockaddr *) &devd, sizeof(devd)) < 0) { + close(sock_devd); + ErrorF("config/devd: Fail to connect to devd"); + return 0; + } + + RegisterBlockAndWakeupHandlers(block_handler, wakeup_handler, NULL); + AddGeneralSocket(sock_devd); + + return 1; +} + +void +config_devd_fini(void) +{ + if (sock_devd < 0) + return; + + RemoveGeneralSocket(sock_devd); + RemoveBlockAndWakeupHandlers(block_handler, wakeup_handler, NULL); + close(sock_devd); +} diff --git a/configure.ac b/configure.ac index 21a6591..27452e3 100644 --- a/configure.ac +++ b/configure.ac @@ -615,6 +615,7 @@ AC_ARG_ENABLE(dpms, AS_HELP_STRING([--disable-dpms], [Build DPMS exten AC_ARG_ENABLE(config-udev, AS_HELP_STRING([--enable-config-udev], [Build udev support (default: auto)]), [CONFIG_UDEV=$enableval], [CONFIG_UDEV=auto]) AC_ARG_ENABLE(config-udev-kms, AS_HELP_STRING([--enable-config-udev-kms], [Build udev kms support (default: auto)]), [CONFIG_UDEV_KMS=$enableval], [CONFIG_UDEV_KMS=auto]) AC_ARG_ENABLE(config-hal, AS_HELP_STRING([--disable-config-hal], [Build HAL support (default: auto)]), [CONFIG_HAL=$enableval], [CONFIG_HAL=auto]) +AC_ARG_ENABLE(config-devd, AS_HELP_STRING([--disable-config-devd], [Build devd support (default: auto)]), [CONFIG_DEVD=$enableval], [CONFIG_DEVD=auto]) AC_ARG_ENABLE(config-wscons, AS_HELP_STRING([--enable-config-wscons], [Build wscons config support (default: auto)]), [CONFIG_WSCONS=$enableval], [CONFIG_WSCONS=auto]) AC_ARG_ENABLE(xfree86-utils, AS_HELP_STRING([--enable-xfree86-utils], [Build xfree86 DDX utilities (default: enabled)]), [XF86UTILS=$enableval], [XF86UTILS=yes]) AC_ARG_ENABLE(vgahw, AS_HELP_STRING([--enable-vgahw], [Build Xorg with vga access (default: enabled)]), [VGAHW=$enableval], [VGAHW=yes]) @@ -921,6 +922,21 @@ if test "x$CONFIG_WSCONS" = xyes; then AC_DEFINE(CONFIG_WSCONS, 1, [Use wscons for input auto configuration]) fi +if test "x$CONFIG_DEVD" = xauto; then + case $host_os in + freebsd* | kfreebsd*-gnu) + CONFIG_DEVD=yes; + ;; + *) + CONFIG_DEVD=no; + ;; + esac +fi +AM_CONDITIONAL(CONFIG_DEVD, [test "x$CONFIG_DEVD" = xyes]) +if test "x$CONFIG_DEVD" = xyes; then + AC_DEFINE(CONFIG_DEVD, 1, [Use devd for input auto configuration]) +fi + if test "x$USE_SIGIO_BY_DEFAULT" = xyes; then USE_SIGIO_BY_DEFAULT_VALUE=TRUE else diff --git a/hw/xfree86/common/xf86Config.c b/hw/xfree86/common/xf86Config.c index 542d5ab..d012584 100644 --- a/hw/xfree86/common/xf86Config.c +++ b/hw/xfree86/common/xf86Config.c @@ -1379,15 +1379,18 @@ checkCoreInputDevices(serverLayoutPtr servlayoutp, Bool implicitLayout) } if (!xf86Info.forceInputDevices && !(foundPointer && foundKeyboard)) { -#if defined(CONFIG_HAL) || defined(CONFIG_UDEV) || defined(CONFIG_WSCONS) +#if defined(CONFIG_HAL) || defined(CONFIG_UDEV) || defined(CONFIG_WSCONS) || \ + defined(CONFIG_DEVD) const char *config_backend; #if defined(CONFIG_HAL) config_backend = "HAL"; #elif defined(CONFIG_UDEV) config_backend = "udev"; -#else +#elif defined(CONFIG_WSCONS) config_backend = "wscons"; +#elif defined(CONFIG_DEVD) + config_backend = "devd"; #endif xf86Msg(X_INFO, "The server relies on %s to provide the list of " "input devices.\n\tIf no devices become available, " diff --git a/hw/xfree86/common/xf86Globals.c b/hw/xfree86/common/xf86Globals.c index 7df7a80..4119e8b 100644 --- a/hw/xfree86/common/xf86Globals.c +++ b/hw/xfree86/common/xf86Globals.c @@ -123,7 +123,8 @@ xf86InfoRec xf86Info = { .log = LogNone, .disableRandR = FALSE, .randRFrom = X_DEFAULT, -#if defined(CONFIG_HAL) || defined(CONFIG_UDEV) || defined(CONFIG_WSCONS) +#if defined(CONFIG_HAL) || defined(CONFIG_UDEV) || defined(CONFIG_WSCONS) || \ + defined(CONFIG_DEVD) .forceInputDevices = FALSE, .autoAddDevices = TRUE, .autoEnableDevices = TRUE, diff --git a/include/dix-config.h.in b/include/dix-config.h.in index 7c77956..72df572 100644 --- a/include/dix-config.h.in +++ b/include/dix-config.h.in @@ -420,6 +420,9 @@ /* Support HAL for hotplug */ #undef CONFIG_HAL +/* Support devd for hotplug */ +#undef CONFIG_DEVD + /* Have a monotonic clock from clock_gettime() */ #undef MONOTONIC_CLOCK -- 1.7.10.4
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
