[libvirt] [PATCH] util: mdev: Treat the 'name' sysfs attribute as optional

[Erik Skultety]

On Wed, Mar 07, 2018 at 10:31:27AM -0500, John Ferlan wrote:
>
>
> On 03/07/2018 02:46 AM, Erik Skultety wrote:
> > On Tue, Mar 06, 2018 at 11:47:51AM -0500, John Ferlan wrote:
> >>
> >>
> >> On 03/05/2018 09:43 AM, Erik Skultety wrote:
> >>> When commit 3545cbef moved the sysfs attribute reading logic from
> >>> _udev.c module to virmdev.c, it had to replace our udev read wrappers
> >>> with the ones available from virfile.c. The problem is that the original
> >>> logic worked correctly with udev read wrappers which don't return an
> >>> error code for a missing attribute, virfile.c readers however - not so
> >>> much. Therefore add another parameter to the macro, so we can again
> >>> accept the fact that optional attributes may be missing.
> >>>
> >>> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> >>> ---
> >>>  src/util/virmdev.c | 17 +++++++++++------
> >>>  1 file changed, 11 insertions(+), 6 deletions(-)
> >>>
> >>
> >> The virFileReadValue* API's return -2 for non existing file, so instead
> >> of messing with errno, you should be able to
> >>
> >>     rc = cb();
> >>     if (rc == -2 && optional)
> >>         rc = 0;
> >>     if (rc < 0)
> >>         goto cleanup;
> >>
> >> As it seems to be the more common way to use the functions.
> >
> > Honestly, that was my first approach, but then I told myself that rather than
> > comparing against a "magic" value which in order to understand the caller has
> > to go and read the function being called, so I went for the errno and I liked it
> > more, it's standardized (you don't care what the function does and under what
> > circumstances it returns, you just want the errno), there was less lines of code
> > involved, I can change it if you insist, but I wanted to express my intentions
> > first.
> >
>
> I don't insist, but since the function notes it returns a magic -2 on
> non-existing files I just figured that is no different...  BTW: The same
> logic can be applied in reverse - you know that virFileExists would
> return ENOENT and are using that knowledge for the magic errno comparison.
>
> In the long run, it's just an "implementation detail" whether you use
> ret == -2 or errno == ENOENT. The code is technically correct. A long
> time ago in a former project I was encouraged to stay away from trusting
> errno because something in between where it gets set (in this case by
> access()) and the calling code a few levels back up the stack the errno
> could be changed and then you're hosed. In this case it's not, so no big
> deal.
>
> So for the concept in general, you can have my R-b
>
> Reviewed-by: John Ferlan <jferlan at redhat.com>
>
> The preference is to use the -2, but I'm not going to be the blocker on
> using errno.

Changed it to a version testing the numerical value rather than relying on
errno and pushed.

Thanks,
Erik