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

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(a)redhat.com&gt; >>> --- >>> 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(a)redhat.com&gt; The preference is to use the -2, but I'm not going to be the blocker on using errno.