Re: [libvirt] [PATCH 1/2] interface: Introduce netcfInterfaceObjIsActive
On 12/11/2013 11:16 AM, Michal Privoznik wrote:
> This function barely wraps ncf_if_status() and error handling code.
>
> Signed-off-by: Michal Privoznik
> ---
> src/interface/interface_backend_netcf.c | 57
> +++--
> 1 file changed, 33 insertions(+), 24 deletions(-)
>
> diff --git a/src/interface/interface_backend_netcf.c
> b/src/interface/interface_backend_netcf.c
> index c4e18c4..2e681ec 100644
> --- a/src/interface/interface_backend_netcf.c
> +++ b/src/interface/interface_backend_netcf.c
> @@ -238,6 +238,32 @@ static struct netcf_if *interfaceDriverGetNetcfIF(struct
> netcf *ncf, virInterfac
> return iface;
> }
>
> +static int
> +netcfInterfaceObjIsActive(struct netcf_if *iface,
> + bool *active)
> +{
> +int ret = -1;
> +unsigned int flags = 0;
> +
> +virObjectRef(driverState);
> +if (ncf_if_status(iface, &flags) < 0) {
> +const char *errmsg, *details;
> +int errcode = ncf_error(driverState->netcf, &errmsg, &details);
> +virReportError(netcf_to_vir_err(errcode),
> + _("failed to get status of interface %s: %s%s%s"),
> + ncf_if_name(iface), errmsg, details ? " - " : "",
> + details ? details : "");
> +goto cleanup;
> +}
> +
> +*active = flags & NETCF_IFACE_ACTIVE;
> +ret = 0;
> +
> +cleanup:
> +virObjectUnref(driverState);
> +return ret;
> +}
> +
> static virDrvOpenStatus
> netcfInterfaceOpen(virConnectPtr conn,
> virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> @@ -539,7 +565,7 @@ netcfConnectListAllInterfaces(virConnectPtr conn,
> struct netcf_if *iface = NULL;
> virInterfacePtr *tmp_iface_objs = NULL;
> virInterfacePtr iface_obj = NULL;
> -unsigned int status;
> +bool active;
> int niface_objs = 0;
> int ret = -1;
> char **names = NULL;
> @@ -611,15 +637,8 @@ netcfConnectListAllInterfaces(virConnectPtr conn,
> }
> }
>
> -if (ncf_if_status(iface, &status) < 0) {
> -const char *errmsg, *details;
> -int errcode = ncf_error(driver->netcf, &errmsg, &details);
> -virReportError(netcf_to_vir_err(errcode),
> - _("failed to get status of interface %s: %s%s%s"),
> - names[i], errmsg, details ? " - " : "",
> - details ? details : "");
> +if (netcfInterfaceObjIsActive(iface, &active) < 0)
> goto cleanup;
> -}
>
> if (!(def = netcfGetMinimalDefForDevice(iface)))
> goto cleanup;
> @@ -636,10 +655,8 @@ netcfConnectListAllInterfaces(virConnectPtr conn,
> * except active|inactive are supported.
> */
> if (MATCH(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE) &&
> -!((MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) &&
> - (status & NETCF_IFACE_ACTIVE)) ||
> - (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) &&
> - (status & NETCF_IFACE_INACTIVE {
> +!((MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) && active) ||
> + (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) && !active))) {
> ncf_if_free(iface);
> iface = NULL;
> continue;
> @@ -1010,9 +1027,9 @@ static int netcfInterfaceIsActive(virInterfacePtr
> ifinfo)
> {
> virNetcfDriverStatePtr driver = ifinfo->conn->interfacePrivateData;
> struct netcf_if *iface = NULL;
> -unsigned int flags = 0;
> virInterfaceDefPtr def = NULL;
> int ret = -1;
> +bool active;
>
> virObjectLock(driver);
>
> @@ -1022,24 +1039,16 @@ static int netcfInterfaceIsActive(virInterfacePtr
> ifinfo)
> goto cleanup;
> }
>
> -
> if (!(def = netcfGetMinimalDefForDevice(iface)))
> goto cleanup;
>
> if (virInterfaceIsActiveEnsureACL(ifinfo->conn, def) < 0)
> goto cleanup;
>
> -if (ncf_if_status(iface, &flags) < 0) {
> -const char *errmsg, *details;
> -int errcode = ncf_error(driver->netcf, &errmsg, &details);
> -virReportError(netcf_to_vir_err(errcode),
> - _("failed to get status of interface %s: %s%s%s"),
> - ifinfo->name, errmsg, details ? " - " : "",
> - details ? details : "");
> +if (netcfInterfaceObjIsActive(iface, &active) < 0)
> goto cleanup;
> -}
>
> -ret = flags & NETCF_IFACE_ACTIVE ? 1 : 0;
> +ret = active ? 1 : 0;
>
> cleanup:
> ncf_if_free(iface);
This looks fine other than the extra red/unref of the driver state
object that Dan already mentioned. ACK with that removed.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] interface: Introduce netcfInterfaceObjIsActive
On 12/11/2013 05:41 AM, Michal Privoznik wrote:
>>> +static int
>>> +netcfInterfaceObjIsActive(struct netcf_if *iface,
>>> + bool *active)
>>> +{
>>> +int ret = -1;
>>> +unsigned int flags = 0;
>>> +
>>> +virObjectRef(driverState);
>>
>> What's the ref / unref of driverState for ? The code you're
>> replacing doesn't do that, and AFAICT this shouldn't be
>> required since we're always calling this from public API
>> context
>
> Well, for now it is not required, true. But I was thinking if somebody
> rewrites something this part wouldn't need any change as it's already
> foolproof. But I don't mind dropping ref and unref.
Spurious locking now for future code patches is counter-productive; if
someone adds code that needs locking, we can add the locking at that
point (and ideally keeping things lockless is the better approach). So
yes, let's drop the ref and unref.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] interface: Introduce netcfInterfaceObjIsActive
On 11.12.2013 13:19, Daniel P. Berrange wrote:
> On Wed, Dec 11, 2013 at 10:16:37AM +0100, Michal Privoznik wrote:
>> This function barely wraps ncf_if_status() and error handling code.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/interface/interface_backend_netcf.c | 57
>> +++--
>> 1 file changed, 33 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/interface/interface_backend_netcf.c
>> b/src/interface/interface_backend_netcf.c
>> index c4e18c4..2e681ec 100644
>> --- a/src/interface/interface_backend_netcf.c
>> +++ b/src/interface/interface_backend_netcf.c
>> @@ -238,6 +238,32 @@ static struct netcf_if
>> *interfaceDriverGetNetcfIF(struct netcf *ncf, virInterfac
>> return iface;
>> }
>>
>> +static int
>> +netcfInterfaceObjIsActive(struct netcf_if *iface,
>> + bool *active)
>> +{
>> +int ret = -1;
>> +unsigned int flags = 0;
>> +
>> +virObjectRef(driverState);
>
> What's the ref / unref of driverState for ? The code you're
> replacing doesn't do that, and AFAICT this shouldn't be
> required since we're always calling this from public API
> context
Well, for now it is not required, true. But I was thinking if somebody
rewrites something this part wouldn't need any change as it's already
foolproof. But I don't mind dropping ref and unref.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] interface: Introduce netcfInterfaceObjIsActive
On Wed, Dec 11, 2013 at 10:16:37AM +0100, Michal Privoznik wrote:
> This function barely wraps ncf_if_status() and error handling code.
>
> Signed-off-by: Michal Privoznik
> ---
> src/interface/interface_backend_netcf.c | 57
> +++--
> 1 file changed, 33 insertions(+), 24 deletions(-)
>
> diff --git a/src/interface/interface_backend_netcf.c
> b/src/interface/interface_backend_netcf.c
> index c4e18c4..2e681ec 100644
> --- a/src/interface/interface_backend_netcf.c
> +++ b/src/interface/interface_backend_netcf.c
> @@ -238,6 +238,32 @@ static struct netcf_if *interfaceDriverGetNetcfIF(struct
> netcf *ncf, virInterfac
> return iface;
> }
>
> +static int
> +netcfInterfaceObjIsActive(struct netcf_if *iface,
> + bool *active)
> +{
> +int ret = -1;
> +unsigned int flags = 0;
> +
> +virObjectRef(driverState);
What's the ref / unref of driverState for ? The code you're
replacing doesn't do that, and AFAICT this shouldn't be
required since we're always calling this from public API
context
> +if (ncf_if_status(iface, &flags) < 0) {
> +const char *errmsg, *details;
> +int errcode = ncf_error(driverState->netcf, &errmsg, &details);
> +virReportError(netcf_to_vir_err(errcode),
> + _("failed to get status of interface %s: %s%s%s"),
> + ncf_if_name(iface), errmsg, details ? " - " : "",
> + details ? details : "");
> +goto cleanup;
> +}
> +
> +*active = flags & NETCF_IFACE_ACTIVE;
> +ret = 0;
> +
> +cleanup:
> +virObjectUnref(driverState);
> +return ret;
> +}
> +
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] interface: Introduce netcfInterfaceObjIsActive
This function barely wraps ncf_if_status() and error handling code.
Signed-off-by: Michal Privoznik
---
src/interface/interface_backend_netcf.c | 57 +++--
1 file changed, 33 insertions(+), 24 deletions(-)
diff --git a/src/interface/interface_backend_netcf.c
b/src/interface/interface_backend_netcf.c
index c4e18c4..2e681ec 100644
--- a/src/interface/interface_backend_netcf.c
+++ b/src/interface/interface_backend_netcf.c
@@ -238,6 +238,32 @@ static struct netcf_if *interfaceDriverGetNetcfIF(struct
netcf *ncf, virInterfac
return iface;
}
+static int
+netcfInterfaceObjIsActive(struct netcf_if *iface,
+ bool *active)
+{
+int ret = -1;
+unsigned int flags = 0;
+
+virObjectRef(driverState);
+if (ncf_if_status(iface, &flags) < 0) {
+const char *errmsg, *details;
+int errcode = ncf_error(driverState->netcf, &errmsg, &details);
+virReportError(netcf_to_vir_err(errcode),
+ _("failed to get status of interface %s: %s%s%s"),
+ ncf_if_name(iface), errmsg, details ? " - " : "",
+ details ? details : "");
+goto cleanup;
+}
+
+*active = flags & NETCF_IFACE_ACTIVE;
+ret = 0;
+
+cleanup:
+virObjectUnref(driverState);
+return ret;
+}
+
static virDrvOpenStatus
netcfInterfaceOpen(virConnectPtr conn,
virConnectAuthPtr auth ATTRIBUTE_UNUSED,
@@ -539,7 +565,7 @@ netcfConnectListAllInterfaces(virConnectPtr conn,
struct netcf_if *iface = NULL;
virInterfacePtr *tmp_iface_objs = NULL;
virInterfacePtr iface_obj = NULL;
-unsigned int status;
+bool active;
int niface_objs = 0;
int ret = -1;
char **names = NULL;
@@ -611,15 +637,8 @@ netcfConnectListAllInterfaces(virConnectPtr conn,
}
}
-if (ncf_if_status(iface, &status) < 0) {
-const char *errmsg, *details;
-int errcode = ncf_error(driver->netcf, &errmsg, &details);
-virReportError(netcf_to_vir_err(errcode),
- _("failed to get status of interface %s: %s%s%s"),
- names[i], errmsg, details ? " - " : "",
- details ? details : "");
+if (netcfInterfaceObjIsActive(iface, &active) < 0)
goto cleanup;
-}
if (!(def = netcfGetMinimalDefForDevice(iface)))
goto cleanup;
@@ -636,10 +655,8 @@ netcfConnectListAllInterfaces(virConnectPtr conn,
* except active|inactive are supported.
*/
if (MATCH(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE) &&
-!((MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) &&
- (status & NETCF_IFACE_ACTIVE)) ||
- (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) &&
- (status & NETCF_IFACE_INACTIVE {
+!((MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) && active) ||
+ (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) && !active))) {
ncf_if_free(iface);
iface = NULL;
continue;
@@ -1010,9 +1027,9 @@ static int netcfInterfaceIsActive(virInterfacePtr ifinfo)
{
virNetcfDriverStatePtr driver = ifinfo->conn->interfacePrivateData;
struct netcf_if *iface = NULL;
-unsigned int flags = 0;
virInterfaceDefPtr def = NULL;
int ret = -1;
+bool active;
virObjectLock(driver);
@@ -1022,24 +1039,16 @@ static int netcfInterfaceIsActive(virInterfacePtr
ifinfo)
goto cleanup;
}
-
if (!(def = netcfGetMinimalDefForDevice(iface)))
goto cleanup;
if (virInterfaceIsActiveEnsureACL(ifinfo->conn, def) < 0)
goto cleanup;
-if (ncf_if_status(iface, &flags) < 0) {
-const char *errmsg, *details;
-int errcode = ncf_error(driver->netcf, &errmsg, &details);
-virReportError(netcf_to_vir_err(errcode),
- _("failed to get status of interface %s: %s%s%s"),
- ifinfo->name, errmsg, details ? " - " : "",
- details ? details : "");
+if (netcfInterfaceObjIsActive(iface, &active) < 0)
goto cleanup;
-}
-ret = flags & NETCF_IFACE_ACTIVE ? 1 : 0;
+ret = active ? 1 : 0;
cleanup:
ncf_if_free(iface);
--
1.8.5.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
