[libvirt] [PATCH] Change return value of VIR_DRV_SUPPORTS_FEATURE to bool
virDrvSupportsFeature API is allowed to return -1 on error while all but one uses of VIR_DRV_SUPPORTS_FEATURE only check for (non)zero return value. Let's make this macro return zero on error, which is what everyone expects anyway. --- src/driver.h | 8 ++++---- src/libvirt.c | 5 ++++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/driver.h b/src/driver.h index b770e5e..e797a75 100644 --- a/src/driver.h +++ b/src/driver.h @@ -52,12 +52,12 @@ typedef enum { * Note that you must check for errors. * * Returns: - * >= 1 Feature is supported. + * != 0 Feature is supported. * 0 Feature is not supported. - * -1 Error. */ -# define VIR_DRV_SUPPORTS_FEATURE(drv,conn,feature) \ - ((drv)->supports_feature ? (drv)->supports_feature((conn),(feature)) : 0) +# define VIR_DRV_SUPPORTS_FEATURE(drv,conn,feature) \ + ((drv)->supports_feature ? \ + (drv)->supports_feature((conn), (feature)) > 0 : 0) typedef virDrvOpenStatus (*virDrvOpen) (virConnectPtr conn, diff --git a/src/libvirt.c b/src/libvirt.c index b4951c2..4188b45 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -1605,7 +1605,10 @@ virDrvSupportsFeature (virConnectPtr conn, int feature) return (-1); } - ret = VIR_DRV_SUPPORTS_FEATURE (conn->driver, conn, feature); + if (!conn->driver->supports_feature) + ret = 0; + else + ret = conn->driver->supports_feature(conn, feature); if (ret < 0) virDispatchError(conn); -- 1.7.3.2
On 12/03/2010 01:48 AM, Jiri Denemark wrote:
virDrvSupportsFeature API is allowed to return -1 on error while all but one uses of VIR_DRV_SUPPORTS_FEATURE only check for (non)zero return value. Let's make this macro return zero on error, which is what everyone expects anyway. --- src/driver.h | 8 ++++---- src/libvirt.c | 5 ++++- 2 files changed, 8 insertions(+), 5 deletions(-)
That is, if you care about the possibility of an error, call the function directly; and if you use the macro, then you don't care about errors and are happy treating an error the same as not present. Seems reasonable.
diff --git a/src/driver.h b/src/driver.h index b770e5e..e797a75 100644 --- a/src/driver.h +++ b/src/driver.h @@ -52,12 +52,12 @@ typedef enum { * Note that you must check for errors.
However, this comment is no longer applicable. So you'll need a v2 that fixes the documentation, and make it explicit that the macro ignores errors, as well as a cross-reference to the actual function for someone that cares about errors.
* * Returns: - * >= 1 Feature is supported. + * != 0 Feature is supported. * 0 Feature is not supported. - * -1 Error. */ -# define VIR_DRV_SUPPORTS_FEATURE(drv,conn,feature) \ - ((drv)->supports_feature ? (drv)->supports_feature((conn),(feature)) : 0) +# define VIR_DRV_SUPPORTS_FEATURE(drv,conn,feature) \ + ((drv)->supports_feature ? \ + (drv)->supports_feature((conn), (feature)) > 0 : 0)
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
diff --git a/src/driver.h b/src/driver.h index b770e5e..e797a75 100644 --- a/src/driver.h +++ b/src/driver.h @@ -52,12 +52,12 @@ typedef enum { * Note that you must check for errors.
However, this comment is no longer applicable. So you'll need a v2 that fixes the documentation, and make it explicit that the macro ignores errors, as well as a cross-reference to the actual function for someone that cares about errors.
Ah, I haven't noticed that line... What about the following v2? Jirka
From 592fd277d7a853025d92946701590485c0973205 Mon Sep 17 00:00:00 2001 Message-Id: <592fd277d7a853025d92946701590485c0973205.1291390396.git.jdenemar@redhat.com> From: Jiri Denemark <jdenemar@redhat.com> Date: Fri, 3 Dec 2010 09:31:48 +0100 Subject: [PATCH] Change return value of VIR_DRV_SUPPORTS_FEATURE to bool Mail-Followup-To: libvir-list@redhat.com
virDrvSupportsFeature API is allowed to return -1 on error while all but one uses of VIR_DRV_SUPPORTS_FEATURE only check for (non)zero return value. Let's make this macro return zero on error, which is what everyone expects anyway. --- src/driver.h | 15 +++++++++------ src/libvirt.c | 5 ++++- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/driver.h b/src/driver.h index b770e5e..75305fe 100644 --- a/src/driver.h +++ b/src/driver.h @@ -47,17 +47,20 @@ typedef enum { /* Internal feature-detection macro. Don't call drv->supports_feature - * directly, because it may be NULL, use this macro instead. + * directly if you don't have to, because it may be NULL, use this macro + * instead. * - * Note that you must check for errors. + * Note that this treats a possible error returned by drv->supports_feature + * the same as not supported. If you care about the error, call + * drv->supports_feature directly. * * Returns: - * >= 1 Feature is supported. + * != 0 Feature is supported. * 0 Feature is not supported. - * -1 Error. */ -# define VIR_DRV_SUPPORTS_FEATURE(drv,conn,feature) \ - ((drv)->supports_feature ? (drv)->supports_feature((conn),(feature)) : 0) +# define VIR_DRV_SUPPORTS_FEATURE(drv,conn,feature) \ + ((drv)->supports_feature ? \ + (drv)->supports_feature((conn), (feature)) > 0 : 0) typedef virDrvOpenStatus (*virDrvOpen) (virConnectPtr conn, diff --git a/src/libvirt.c b/src/libvirt.c index b4951c2..4188b45 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -1605,7 +1605,10 @@ virDrvSupportsFeature (virConnectPtr conn, int feature) return (-1); } - ret = VIR_DRV_SUPPORTS_FEATURE (conn->driver, conn, feature); + if (!conn->driver->supports_feature) + ret = 0; + else + ret = conn->driver->supports_feature(conn, feature); if (ret < 0) virDispatchError(conn); -- 1.7.3.2
On 12/03/2010 08:36 AM, Jiri Denemark wrote:
diff --git a/src/driver.h b/src/driver.h index b770e5e..e797a75 100644 --- a/src/driver.h +++ b/src/driver.h @@ -52,12 +52,12 @@ typedef enum { * Note that you must check for errors.
However, this comment is no longer applicable. So you'll need a v2 that fixes the documentation, and make it explicit that the macro ignores errors, as well as a cross-reference to the actual function for someone that cares about errors.
Ah, I haven't noticed that line... What about the following v2?
ACK.
/* Internal feature-detection macro. Don't call drv->supports_feature - * directly, because it may be NULL, use this macro instead. + * directly if you don't have to, because it may be NULL, use this macro + * instead. * - * Note that you must check for errors. + * Note that this treats a possible error returned by drv->supports_feature + * the same as not supported. If you care about the error, call + * drv->supports_feature directly. *
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Jiri Denemark