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@xxxxxxxxxx> > >>> --- > >>> 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@xxxxxxxxxx> > > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
