[PATCH] Add devd config backend for FreeBSD (and GNU/kFreeBSD)
Guillem Jover
guillem at debian.org
Sat Feb 22 08:12:34 PST 2014
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 <rmh at freebsd.org>
> 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 <rmh at freebsd.org>
> 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
More information about the xorg-devel
mailing list