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.
On Sun, 2014-02-16 at 12:17:01 +0000, Robert Millan wrote: > From 34f6395846f88abec1ae72a74fd0aa35ec1f99a7 Mon Sep 17 00:00:00 2001 > From: Robert Millan <[email protected]> > Date: Sun, 16 Feb 2014 12:14:42 +0000 > 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]> > diff --git a/config/devd.c b/config/devd.c > new file mode 100644 > index 0000000..68e658a > --- /dev/null > +++ b/config/devd.c > @@ -0,0 +1,365 @@ > +/* > + * Copyright ?? 2012 Baptiste Daroussin Encoding issue? Should be ©, I guess. > +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); malloc() return value unchecked. > + if (sysctlbyname(name, dest, &len, NULL, 0) == 0) > + dest[len] = '\0'; > + else { > + free(dest); > + dest = NULL; > + } > + } > + > + free(name); > + return dest; > +} > +static void > +device_added(char *line) > +{ > + char *walk; > + char *path; > + char *vendor; > + char *product = NULL; > + char *config_info = NULL; > + InputOption *options = NULL; > + InputAttributes attrs = {}; > + DeviceIntPtr dev = NULL; > + int i, rc; > + int fd; > + > + 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. > + for (i = 0; hw_types[i].driver != NULL; i++) { > + if (strncmp(line, hw_types[i].driver, > + strlen(hw_types[i].driver)) == 0 && > + isdigit(*(line + 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", > line); > + return; > + } > + if (hw_types[i].xdriver == NULL) { > + LogMessageVerb(X_INFO, 10, "config/devd: ignoring device %s\n", > line); > + return; > + } > + if (asprintf(&path, "/dev/%s", line) == -1) > + return; > + > + options = input_option_new(NULL, "_source", "server/devd"); > + if (!options) > + return; Leaks path. > + 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), passing attrs.product might not be good because I guess it might leak afterwards on input_* teardown. > + } > + 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", path); > + } > + } else { > + options = input_option_new(options, "device", path); > + } > + > + if (asprintf(&config_info, "devd:%s", line) == -1) { > + config_info = NULL; > + goto unwind; > + } > + > + if (device_is_duplicate(config_info)) { > + LogMessage(X_WARNING, "config/devd: device %s already added. " > + "Ignoring.\n", product != NULL ? product : > "(unnamed)"); > + goto unwind; > + } > + > + options = input_option_new(options, "config_info", config_info); > + LogMessage(X_INFO, "config/devd: adding input device %s (%s)\n", > + product != NULL ? product : "(unnamed)", path); > + > + rc = NewInputDeviceRequest(options, &attrs, &dev); > + > + if (rc != Success) > + goto unwind; A bit useless, although future-proof. > + unwind: > + free(config_info); > + input_option_free_list(&options); > + > + free(attrs.usb_id); > + free(attrs.product); > + free(attrs.device); > + free(attrs.vendor); > + > + return; Useless return. > +} > + > +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. > + if (asprintf(&value, "devd:%s", line) == -1) > + return; > + > + remove_devices("devd", value); > + > + free(value); > +} > + > +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(), realloc() can do the initial alloc if buf is NULL. > + if (!buf) > + return -1; > + > + for (;;) { > + ret = read(sock_devd, &c, 1); > + if (ret < 1) { > + free(buf); > + return -1; > + } You might want to check for EINTR here. > + if (c == '\n') > + break; > + > + if (sz + 1 >= cap) { > + cap *= 2; > + buf = realloc(buf, cap *sizeof(char)); Leaks on failure, useless sizeof(). > + } > + buf[sz] = c; > + sz++; > + } > + > + buf[sz] = '\0'; > + if (sz >= 0) > + *out = buf; You could unconditionally set *out, as buf will be initialized to NULL for realloc(), so that the caller always gets a defined value. > + else > + free(buf); > + > + return sz; /* number of bytes in the line, not counting the line break*/ Move this to a paragraph before the function, otherwise there's inconsistency with other returns. > +} > +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. > + 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(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. > + close(sock_devd); > + ErrorF("config/devd: Fail to connect to devd"); > + return 0; > + } > + > + RegisterBlockAndWakeupHandlers(block_handler, wakeup_handler, NULL); > + AddGeneralSocket(sock_devd); > + > + return 1; > +} > diff --git a/hw/xfree86/common/xf86Config.c b/hw/xfree86/common/xf86Config.c > index 542d5ab..3d8b08f 100644 > --- a/hw/xfree86/common/xf86Config.c > +++ b/hw/xfree86/common/xf86Config.c > @@ -1379,15 +1379,17 @@ 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) > diff --git a/hw/xfree86/common/xf86Globals.c b/hw/xfree86/common/xf86Globals.c > index 7df7a80..8b2fb4d 100644 > --- a/hw/xfree86/common/xf86Globals.c > +++ b/hw/xfree86/common/xf86Globals.c > @@ -123,7 +123,7 @@ 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, You might want to wrap these long lines to 80 chars. Thanks, Guillem _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
