[libvirt] [PATCH] util: mdev: Treat the 'name' sysfs attribute as optional
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
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
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
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
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
participants (2)
-
Erik Skultety -
John Ferlan