[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