[lm-sensors] libsensors / libsysfs patches for review

[lm-sensors] libsensors / libsysfs patches for review

[Mark M. Hoffman]

Hi everyone:

As I mentioned on IRC, I have some libsensors patches ready for review here
(as a quilt series):

http://members.dca.net/mhoffman/sensors/patches/lm_sensors2/20050910/

The fifth patch is slightly edited since I first posted them, and the sixth
patch is now ready also.  I tested them against each of the following kernels:
2.4.31 (/proc), 2.6.13-rc6 (old sysfs), and 2.6.13-rc6-mm2 (sysfs/hwmon).

Known caveats: 

1) To build libsensors on a box w/ Linux 2.4.x kernel, you still need to have
built and installed libsysfs first.  That's awkward for sure, but it's only
temporary.  The long-term plan is for lib/sysfs.c to be optional.  Likely
I'll add more automagic to the makefile.

2) I haven't prepared a documentation patch yet.

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

[lm-sensors] libsensors / libsysfs patches for review

[Mark M. Hoffman]

Hi again:

* Mark M. Hoffman <mhoffman@lightlink.com> [2005-09-11 00:06:00 -0400]:
> Known caveats: 
> 
> 1) To build libsensors on a box w/ Linux 2.4.x kernel, you still need to have
> built and installed libsysfs first.  That's awkward for sure, but it's only
> temporary.  The long-term plan is for lib/sysfs.c to be optional.  Likely
> I'll add more automagic to the makefile.

Decided to do this sooner rather than later - the seventh patch in the series
addresses it.  The other six haven't changed; all seven are here:

http://members.dca.net/mhoffman/sensors/patches/lm_sensors2/20050913/

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

[lm-sensors] libsensors / libsysfs patches for review

[Jean Delvare]

Hi Mark,

> http://members.dca.net/mhoffman/sensors/patches/lm_sensors2/20050913/

I finally got around to reviewing your work. Unsurprisingly, I only have
minor comments. The whole idea and your implementation are just right.

I have no comment at all on the first three patches.

4th patch: found_sysfs_libsysfs

> --- /dev/null
> +++ lm_sensors2/lib/sysfs.c
> (...)
> +#include <string.h>

I don't think we actually need it at this point - but this admittedly
doesn't matter, later patches need it anyway.

> --- /dev/null
> +++ lm_sensors2/lib/sysfs.h
> (...)
> +#ifndef SENSORS_LIB_SYSFS_H
> +#define SENSORS_LIB_SYSFS_H
> (...)
> +#endif
> +

I'd suggest a /* !SENSORS_LIB_SYSFS_H */ after the #endif. Also, you
have an extra blank line at the end of this file.

> --- lm_sensors2.orig/lib/init.c
> +++ lm_sensors2/lib/init.c
> (...)
>    sensors_cleanup();
> +  sensors_init_sysfs();

Unrelated with your changes, but why are we calling sensors_cleanup()
here? Looks like a waste of time to me.

> --- lm_sensors2.orig/lib/access.c
> +++ lm_sensors2/lib/access.c
> (...)
> -  if(foundsysfs) {
> +  if(sensors_found_sysfs) {

Please do not hesitate to fix the coding style in these occasions. There
should be a space between the "if" and the opening parenthesis.

5th patch: read_bus_libsysfs

> --- lm_sensors2.orig/lib/sysfs.c
> +++ lm_sensors2/lib/sysfs.c
> (...)
> +int sensors_read_sysfs_bus(void)
> +{
> +       struct sysfs_class *cls = NULL;
> +       struct dlist *clsdevs = NULL;
> +       struct sysfs_class_device *clsdev = NULL;

These initializations look superfluous to me.

> +       dlist_for_each_data(clsdevs, clsdev, struct sysfs_class_device) {
> +               struct sysfs_device *dev = NULL;
> +               struct sysfs_attribute *attr = NULL;

Ditto.

> +               } else if (!sscanf(clsdev->name, "i2c-%d", &entry.number)) {

I'd prefer an explicit != 1 test. sscanf is said to eventually return
EOF.

> +exit:
> +       /* this frees *i2c_class _and_ *cdevs */
> +       if (cls)
> +               sysfs_close_class(cls);
> +       return ret;
> +}

The comment looks out of sync with the variable names.

Why don't you define a second label after the call to
sysfs_close_class()? This would let you get rid of the if (cls) test
unless I'm mistaken.

> --- lm_sensors2.orig/lib/init.c
> +++ lm_sensors2/lib/init.c
> (...)
> +  if ((res = sensors_read_sysfs_bus()) && (res = sensors_read_proc_bus()))

This looks less than optimal to me. Why not rely on the
sensors_found_sysfs variable to call the right function directly?

6th patch: read_chip_libsysfs

> --- lm_sensors2.orig/lib/init.c
> +++ lm_sensors2/lib/init.c
> (...)
>    sensors_init_sysfs();
> -  if ((res = sensors_read_proc_chips()))
> +  if ((res = sensors_read_sysfs_chips()) && (res = sensors_read_proc_chips()))

Ditto.

Maybe you could have sensors_init_sysfs() return the value of
sensors_found_sysfs. This would lead to the following code:

  if (sensors_init_sysfs()) {
    if ((res = sensors_read_sysfs_chips()) || (res = sensors_read_sysfs_bus()))
      return res;
  } else {
    if ((res = sensors_read_proc_chips()) || (res = sensors_read_proc_bus()))
      return res;
  }

I think it's faster and more readble than the current code.

> --- lm_sensors2.orig/lib/sysfs.c
> +++ lm_sensors2/lib/sysfs.c
> (...)
> +#define _GNU_SOURCE

Add a comment explaining why this is needed?

> +static int sensors_read_one_sysfs_chip(struct sysfs_device *dev)
> (...)
> +       if (!entry.name.prefix)
> +               return -SENSORS_ERR_PARSE;
> +
> +       entry.name.busname = strdup(dev->path);
> +       if (!entry.name.busname)
> +               return -SENSORS_ERR_PARSE;

Shouldn't these be out-of-memory fatal errors like you did elsewhere?
Calling these "parse errors" is rather confusing.

> +       if ((bus_attr = sysfs_open_attribute(bus_path))) {
> +

No blank line here please.

Additionally, the same comments I had about patch 5 (useless
initializations, second exit label) apply to this one too.

7th patch: sysfs_support_optional

> --- lm_sensors2.orig/Makefile
> +++ lm_sensors2/Makefile
> (...)
> +# by uncommenting the line after the next endif.  Keep in mind, *iff* you

Typo.

> +ifdef SYSFS_SUPPORT
> +ARCPPFLAGS := $(ARCPPFLAGS) -DSYSFS_SUPPORT
> +endif
>  ARCFLAGS := $(ALL_CFLAGS)
>  LIBCPPFLAGS := $(ALL_CPPFLAGS)
> +ifdef SYSFS_SUPPORT
> +LIBCPPFLAGS := $(LIBCPPFLAGS) -DSYSFS_SUPPORT
> +endif

Why not +=?

> --- lm_sensors2.orig/lib/Module.mk
> +++ lm_sensors2/lib/Module.mk

> +ifdef SYSFS_SUPPORT
> +       LIBCSOURCES := $(LIBCSOURCES) $(MODULE_DIR)/sysfs.c
> +endif

Ditto.

That's about it. Overall it looks just fine, thanks a lot for working on
this :)

-- 
Jean Delvare

[lm-sensors] libsensors / libsysfs patches for review

[Mark M. Hoffman]

Hi Jean:

* Jean Delvare <khali@linux-fr.org> [2005-09-17 20:50:31 +0200]:
> I finally got around to reviewing your work. Unsurprisingly, I only have
> minor comments. The whole idea and your implementation are just right.

Thanks! ;)

> I have no comment at all on the first three patches.

I'll commit them as is.

> 4th patch: found_sysfs_libsysfs
> 
> > --- /dev/null
> > +++ lm_sensors2/lib/sysfs.c
> > (...)
> > +#include <string.h>
> 
> I don't think we actually need it at this point - but this admittedly
> doesn't matter, later patches need it anyway.
> 
> > --- /dev/null
> > +++ lm_sensors2/lib/sysfs.h
> > (...)
> > +#ifndef SENSORS_LIB_SYSFS_H
> > +#define SENSORS_LIB_SYSFS_H
> > (...)
> > +#endif
> > +
> 
> I'd suggest a /* !SENSORS_LIB_SYSFS_H */ after the #endif. Also, you
> have an extra blank line at the end of this file.

OK

> > --- lm_sensors2.orig/lib/init.c
> > +++ lm_sensors2/lib/init.c
> > (...)
> >    sensors_cleanup();
> > +  sensors_init_sysfs();
> 
> Unrelated with your changes, but why are we calling sensors_cleanup()
> here? Looks like a waste of time to me.
> 
> > --- lm_sensors2.orig/lib/access.c
> > +++ lm_sensors2/lib/access.c
> > (...)
> > -  if(foundsysfs) {
> > +  if(sensors_found_sysfs) {
> 
> Please do not hesitate to fix the coding style in these occasions. There
> should be a space between the "if" and the opening parenthesis.

OK

> 5th patch: read_bus_libsysfs
> 
> > --- lm_sensors2.orig/lib/sysfs.c
> > +++ lm_sensors2/lib/sysfs.c
> > (...)
> > +int sensors_read_sysfs_bus(void)
> > +{
> > +       struct sysfs_class *cls = NULL;
> > +       struct dlist *clsdevs = NULL;
> > +       struct sysfs_class_device *clsdev = NULL;
> 
> These initializations look superfluous to me.
> 
> > +       dlist_for_each_data(clsdevs, clsdev, struct sysfs_class_device) {
> > +               struct sysfs_device *dev = NULL;
> > +               struct sysfs_attribute *attr = NULL;
> 
> Ditto.

OK

> > +               } else if (!sscanf(clsdev->name, "i2c-%d", &entry.number)) {
> 
> I'd prefer an explicit != 1 test. sscanf is said to eventually return
> EOF.

Right, nice catch.

> > +exit:
> > +       /* this frees *i2c_class _and_ *cdevs */
> > +       if (cls)
> > +               sysfs_close_class(cls);
> > +       return ret;
> > +}
> 
> The comment looks out of sync with the variable names.

OK

> Why don't you define a second label after the call to
> sysfs_close_class()? This would let you get rid of the if (cls) test
> unless I'm mistaken.

OK

> > --- lm_sensors2.orig/lib/init.c
> > +++ lm_sensors2/lib/init.c
> > (...)
> > +  if ((res = sensors_read_sysfs_bus()) && (res = sensors_read_proc_bus()))
> 
> This looks less than optimal to me. Why not rely on the
> sensors_found_sysfs variable to call the right function directly?

See below.

> 6th patch: read_chip_libsysfs
> 
> > --- lm_sensors2.orig/lib/init.c
> > +++ lm_sensors2/lib/init.c
> > (...)
> >    sensors_init_sysfs();
> > -  if ((res = sensors_read_proc_chips()))
> > +  if ((res = sensors_read_sysfs_chips()) && (res = sensors_read_proc_chips()))
> 
> Ditto.
> 
> Maybe you could have sensors_init_sysfs() return the value of
> sensors_found_sysfs. This would lead to the following code:
> 
>   if (sensors_init_sysfs()) {
>     if ((res = sensors_read_sysfs_chips()) || (res = sensors_read_sysfs_bus()))
>       return res;
>   } else {
>     if ((res = sensors_read_proc_chips()) || (res = sensors_read_proc_bus()))
>       return res;
>   }
> 
> I think it's faster and more readble than the current code.

You're right.

> > --- lm_sensors2.orig/lib/sysfs.c
> > +++ lm_sensors2/lib/sysfs.c
> > (...)
> > +#define _GNU_SOURCE
> 
> Add a comment explaining why this is needed?

OK

> > +static int sensors_read_one_sysfs_chip(struct sysfs_device *dev)
> > (...)
> > +       if (!entry.name.prefix)
> > +               return -SENSORS_ERR_PARSE;
> > +
> > +       entry.name.busname = strdup(dev->path);
> > +       if (!entry.name.busname)
> > +               return -SENSORS_ERR_PARSE;
> 
> Shouldn't these be out-of-memory fatal errors like you did elsewhere?
> Calling these "parse errors" is rather confusing.

OK

> > +       if ((bus_attr = sysfs_open_attribute(bus_path))) {
> > +
> 
> No blank line here please.

OK

> Additionally, the same comments I had about patch 5 (useless
> initializations, second exit label) apply to this one too.

OK

> 7th patch: sysfs_support_optional
> 
> > --- lm_sensors2.orig/Makefile
> > +++ lm_sensors2/Makefile
> > (...)
> > +# by uncommenting the line after the next endif.  Keep in mind, *iff* you
> 
> Typo.

"iff" is shorthand for "if and only if" - I'll just write it out.

> > +ifdef SYSFS_SUPPORT
> > +ARCPPFLAGS := $(ARCPPFLAGS) -DSYSFS_SUPPORT
> > +endif
> >  ARCFLAGS := $(ALL_CFLAGS)
> >  LIBCPPFLAGS := $(ALL_CPPFLAGS)
> > +ifdef SYSFS_SUPPORT
> > +LIBCPPFLAGS := $(LIBCPPFLAGS) -DSYSFS_SUPPORT
> > +endif
> 
> Why not +=?

We use ':=' all over the place, so I think we should stick with that.
('a += b' doesn't have quite the same meaning as 'a := a b')

> > --- lm_sensors2.orig/lib/Module.mk
> > +++ lm_sensors2/lib/Module.mk
> 
> > +ifdef SYSFS_SUPPORT
> > +       LIBCSOURCES := $(LIBCSOURCES) $(MODULE_DIR)/sysfs.c
> > +endif
> 
> Ditto.
> 
> That's about it. Overall it looks just fine, thanks a lot for working on
> this :)

Thanks for the review.  I'll start committing the (rest of the) patches 
after they're updated and tested.

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com