Hi Bjorn,
Thank you for the review!
[...]
> > Only expose the value of the "driver_override" variable through the
> > corresponding sysfs object when a value is actually set.
>
> What's the reason for this change? The above tells me what it *does*,
> but not *why* or whether it affects users.
Good point! I am going to update the commit message with more details
about why this change in the next version. Sorry about this!
> Is this to avoid trying to print a NULL pointer as %s? Do we print
> "(null)" or something in that case now? I assume sprintf() doesn't
> actually oops. If we change what appears in sysfs, we should mention
> that here. And maybe consider whether there's any chance of breaking
> user code that might know what to do with "(null)" but not with an
> empty string.
Yes, sprintf() deals with a NULL pointer and prints "(null)", and
I personally think that this is not very useful, especially in the case
of this particular sysfs object - I would also prefer not to expose the
notion of a NULL-value to the userspace this way, something we ought to
handle properly, see for example how the "resource_alignment" sysfs
object behaves when there is no value set, as per:
# wc /sys/bus/pci/resource_alignment
0 0 0 /sys/bus/pci/resource_alignment
> There are six other driver_override_show() methods. Five don't check
> the ->driver_override pointer at all; one (spi.c) checks like this:
>
> len = snprintf(buf, PAGE_SIZE, "%s\n", spi->driver_override ? : "");
For an empty sysfs object, as per the sysfs recommendations, I believe
that having 0 bytes might be more appropriate to indicate lack of any
sensible value. Using "" (empty string) like in the example above would
still warrant adding a trailing newline character, which would make it
empty (technically), but certainly not 0 bytes. Having said that, some
drivers opt not to add a particular sysfs object at all should there be
no value for it.
> Do the others need similar fixes? Most of them still use sprintf()
> also.
[...]
I would like to think that we should handle NULL-value in sysfs objects
sensibly everywhere, so the other cases you were able to find could be
updated.
Krzysztof
