[PATCH] Add devd config backend for FreeBSD (and GNU/kFreeBSD)

[Robert Millan]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-devd-config-backend-for-FreeBSD-and-GNU-kFreeBSD.patch
Type: text/x-patch
Size: 15952 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20140224/352d3557/attachment-0001.bin>