Re: [PATCH v6 5/6] PCI/sysfs: Only show value when driver_override is not NULL

["Krzysztof Wilczyński" <kw@linux.com>]

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