Messages in this thread | | | Date | Tue, 8 May 2007 05:06:57 +0200 | From | Björn Steinbrink <> | Subject | Re: [PATCH] DMI-based module autoloading |
| |
On 2007.05.08 02:54:29 +0200, Lennart Poettering wrote: > From: Lennart Poettering <mzxreary@0pointer.de> > +#define DEFINE_DMI_ATTR_WITH_SHOW(_name, _mode, _field) \
Hm, too many tabs here? Wraps for me with tabsize=8.
> +static ssize_t sys_dmi_##_name##_show(struct device *dev, \ > + struct device_attribute *attr, char *page) \ > +{ \ > + ssize_t len; \ > + len = snprintf(page, PAGE_SIZE, "%s\n", dmi_get_system_info(_field)); \ > + page[PAGE_SIZE-2] = '\n'; \ > + page[PAGE_SIZE-1] = 0; \ > + return min_t(ssize_t, len, PAGE_SIZE); \ > +} \
If you return PAGE_SIZE here, that includes the trailing 0, while len does not. Seems not to cause any problems, but scnprintf already handles that, so how about this?
ssize_t len; len = scnprintf(page, PAGE_SIZE, "%s\n", dmi_get_system_info(_field)); page[len - 1] = '\n'; return len;
> +DEFINE_DMI_ATTR(_name, _mode, sys_dmi_##_name##_show); > + > +DEFINE_DMI_ATTR_WITH_SHOW(bios_vendor, 0444, DMI_BIOS_VENDOR); > +DEFINE_DMI_ATTR_WITH_SHOW(bios_version, 0444, DMI_BIOS_VERSION); > +DEFINE_DMI_ATTR_WITH_SHOW(bios_date, 0444, DMI_BIOS_DATE); > +DEFINE_DMI_ATTR_WITH_SHOW(sys_vendor, 0444, DMI_SYS_VENDOR); > +DEFINE_DMI_ATTR_WITH_SHOW(product_name, 0444, DMI_PRODUCT_NAME); > +DEFINE_DMI_ATTR_WITH_SHOW(product_version, 0444, DMI_PRODUCT_VERSION); > +DEFINE_DMI_ATTR_WITH_SHOW(product_serial, 0400, DMI_PRODUCT_SERIAL); > +DEFINE_DMI_ATTR_WITH_SHOW(product_uuid, 0400, DMI_PRODUCT_UUID); > +DEFINE_DMI_ATTR_WITH_SHOW(board_vendor, 0444, DMI_BOARD_VENDOR); > +DEFINE_DMI_ATTR_WITH_SHOW(board_name, 0444, DMI_BOARD_NAME); > +DEFINE_DMI_ATTR_WITH_SHOW(board_version, 0444, DMI_BOARD_VERSION); > +DEFINE_DMI_ATTR_WITH_SHOW(board_serial, 0400, DMI_BOARD_SERIAL); > +DEFINE_DMI_ATTR_WITH_SHOW(board_asset_tag, 0444, DMI_BOARD_ASSET_TAG); > +DEFINE_DMI_ATTR_WITH_SHOW(chassis_vendor, 0444, DMI_CHASSIS_VENDOR); > +DEFINE_DMI_ATTR_WITH_SHOW(chassis_type, 0444, DMI_CHASSIS_TYPE); > +DEFINE_DMI_ATTR_WITH_SHOW(chassis_version, 0444, DMI_CHASSIS_VERSION); > +DEFINE_DMI_ATTR_WITH_SHOW(chassis_serial, 0400, DMI_CHASSIS_SERIAL); > +DEFINE_DMI_ATTR_WITH_SHOW(chassis_asset_tag, 0444, DMI_CHASSIS_ASSET_TAG);
Mixed tab/space alignment.
> +static void ascii_filter(char *d, const char *s) > +{ > + /* Filter out characters we don't want to see in the modalias string */ > + for (; *s; s++) > + if (*s > ' ' && *s < 127 && *s != ':') > + *(d++) = *s; > + > + *d = 0; > +} > + > +static ssize_t get_modalias(char *buffer, size_t buffer_size) > +{ > + struct mafield { > + const char *prefix; > + int field; > + }; > +
Trailing whitespace (also in a few more places)
> + static const struct mafield fields[] = { > + { "bvn", DMI_BIOS_VENDOR }, > + { "bvr", DMI_BIOS_VERSION }, > + { "bd", DMI_BIOS_DATE }, > + { "svn", DMI_SYS_VENDOR }, > + { "pn", DMI_PRODUCT_NAME }, > + { "pvr", DMI_PRODUCT_VERSION }, > + { "rvn", DMI_BOARD_VENDOR }, > + { "rn", DMI_BOARD_NAME }, > + { "rvr", DMI_BOARD_VERSION }, > + { "cvn", DMI_CHASSIS_VENDOR }, > + { "ct", DMI_CHASSIS_TYPE }, > + { "cvr", DMI_CHASSIS_VERSION }, > + { NULL, DMI_NONE }
Mixed tab/space alignment.
> + }; > + > + ssize_t l, left; > + char *p; > + const struct mafield *f; > + > + strcpy(buffer, "dmi"); > + p = buffer + 3; left = buffer_size-5; > + > + for (f = fields; f->prefix && left > 0; f++) { > + const char *c; > + char *t; > + > + c = dmi_get_system_info(f->field); > + if (!c) > + continue; > + > + t = kmalloc(strlen(c), GFP_KERNEL); > + if (!t) > + break; > + ascii_filter(t, c); > + l = snprintf(p, left, ":%s%s", f->prefix, t);
Looks like a candidate for scnprintf, too, avoids the l > left comparison below.
> + kfree(t); > + > + if (l > left) > + l = left; > + > + p += l; > + left -= l; > + } > + > + snprintf(p, left, ":");
I assume that the colon is strictly required as -5 is used above to calculate how much space is left. But "left" might be 0 if the above snprintf had to truncate a string, thus the colon would not be written, although there's space left. Fails for left == 1, too.
Björn - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |