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@redhat.com>
---
 src/util/virmdev.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index 124933506..688f2efb5 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -505,10 +505,13 @@ virMediatedDeviceTypeReadAttrs(const char *sysfspath,
     int ret = -1;
     virMediatedDeviceTypePtr tmp = NULL;
 
-#define MDEV_GET_SYSFS_ATTR(attr, dst, cb) \
+#define MDEV_GET_SYSFS_ATTR(attr, dst, cb, optional) \
     do { \
-        if (cb(dst, "%s/%s", sysfspath, attr) < 0) \
-            goto cleanup; \
+        errno = 0; \
+        if (cb(dst, "%s/%s", sysfspath, attr) < 0) { \
+            if (errno != ENOENT || !optional) \
+                goto cleanup; \
+        } \
     } while (0)
 
     if (VIR_ALLOC(tmp) < 0)
@@ -517,10 +520,12 @@ virMediatedDeviceTypeReadAttrs(const char *sysfspath,
     if (VIR_STRDUP(tmp->id, last_component(sysfspath)) < 0)
         goto cleanup;
 
-    MDEV_GET_SYSFS_ATTR("name", &tmp->name, virFileReadValueString);
-    MDEV_GET_SYSFS_ATTR("device_api", &tmp->device_api, virFileReadValueString);
+    /* @name sysfs attribute is optional, so getting ENOENT is fine */
+    MDEV_GET_SYSFS_ATTR("name", &tmp->name, virFileReadValueString, true);
+    MDEV_GET_SYSFS_ATTR("device_api", &tmp->device_api,
+                        virFileReadValueString, false);
     MDEV_GET_SYSFS_ATTR("available_instances", &tmp->available_instances,
-                        virFileReadValueUint);
+                        virFileReadValueUint, false);
 
 #undef MDEV_GET_SYSFS_ATTR
 
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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.

John


> diff --git a/src/util/virmdev.c b/src/util/virmdev.c
> index 124933506..688f2efb5 100644
> --- a/src/util/virmdev.c
> +++ b/src/util/virmdev.c
> @@ -505,10 +505,13 @@ virMediatedDeviceTypeReadAttrs(const char *sysfspath,
>      int ret = -1;
>      virMediatedDeviceTypePtr tmp = NULL;
>  
> -#define MDEV_GET_SYSFS_ATTR(attr, dst, cb) \
> +#define MDEV_GET_SYSFS_ATTR(attr, dst, cb, optional) \
>      do { \
> -        if (cb(dst, "%s/%s", sysfspath, attr) < 0) \
> -            goto cleanup; \
> +        errno = 0; \
> +        if (cb(dst, "%s/%s", sysfspath, attr) < 0) { \
> +            if (errno != ENOENT || !optional) \
> +                goto cleanup; \
> +        } \
>      } while (0)
>  
>      if (VIR_ALLOC(tmp) < 0)
> @@ -517,10 +520,12 @@ virMediatedDeviceTypeReadAttrs(const char *sysfspath,
>      if (VIR_STRDUP(tmp->id, last_component(sysfspath)) < 0)
>          goto cleanup;
>  
> -    MDEV_GET_SYSFS_ATTR("name", &tmp->name, virFileReadValueString);
> -    MDEV_GET_SYSFS_ATTR("device_api", &tmp->device_api, virFileReadValueString);
> +    /* @name sysfs attribute is optional, so getting ENOENT is fine */
> +    MDEV_GET_SYSFS_ATTR("name", &tmp->name, virFileReadValueString, true);
> +    MDEV_GET_SYSFS_ATTR("device_api", &tmp->device_api,
> +                        virFileReadValueString, false);
>      MDEV_GET_SYSFS_ATTR("available_instances", &tmp->available_instances,
> -                        virFileReadValueUint);
> +                        virFileReadValueUint, false);
>  
>  #undef MDEV_GET_SYSFS_ATTR
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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@redhat.com>

The preference is to use the -2, but I'm not going to be the blocker on
using errno.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list