Messages in this thread | | | Date | Mon, 14 Apr 2008 20:13:40 -0700 | From | "SL Baur" <> | Subject | Re: [PATCH] sysfs: add /sys/dev/{char, block} to lookup sysfs path by major:minor |
| |
On 4/14/08, Dan Williams <dan.j.williams@intel.com> wrote:
> This is the second revision of the patch originally posted here: > http://marc.info/?l=linux-kernel&m=120795638915272&w=2 > > * Fixed up ENOMEM handling in devices_init() > * Added a short blurb in Documentation/filesystems/sysfs.txt > > Documentation/filesystems/sysfs.txt | 6 +++++ > drivers/base/core.c | 46 ++++++++++++++++++++++++++++++++++- > 2 files changed, 51 insertions(+), 1 deletions(-)
I've looked this patch over and I have some comments. The logic looks correct, but there are two ugly lines.
> @@ -775,6 +783,7 @@ int device_add(struct device *dev) > struct device *parent = NULL; > struct class_interface *class_intf; > int error; > + char devt_str[25];
> @@ -925,12 +944,16 @@ void device_del(struct device *dev) > { > struct device *parent = dev->parent; > struct class_interface *class_intf; > + char devt_str[25];
May I ask why `25'? The only other user of format_dev_t that I could find in a quick grep is md (device-mapper) and they used a hardcoded `15' there. The real problem is format_dev_t and print_dev_t.
If the only other user of those macros which want to be C inlines with buffer size parameters is md, perhaps now would be a good time to clean them up before adding more users?
Otherwise, the logic looks O.K.
Add my Reviewed-by: SL Baur <steve@xemacs.org> if that is appropriate.
-sb
| |