[PATCH] Implement ncf_if_status()

[Laine Stump]

Here's a first draft of a suggested new API, subject to change based
on comments.

ncf_if_status returns various status bits about the given interface. As a
start, only a single bit is defined:

       NETCF_IFACE_STATUS_UP

This bit is set if the interface is currently up (active) and not set if it
is currently down (inactive).

The value of all other bits should be considered random by an
application program, as later versions of netcf may use them. In other
words, you should always mask out all the other bits.
---
 src/drv_initscripts.c |   16 ++++++++++++++++
 src/internal.h        |    1 +
 src/netcf.c           |    8 ++++++++
 src/netcf.h           |    9 +++++++++
 src/netcf_public.syms |    5 +++++
 5 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/src/drv_initscripts.c b/src/drv_initscripts.c
index 6d03baa..17fad00 100644
--- a/src/drv_initscripts.c
+++ b/src/drv_initscripts.c
@@ -807,6 +807,22 @@ char *drv_xml_state(struct netcf_if *nif) {
     goto done;
 }
 
+/* Report various status info about the interface as bits in
+ * "flags". Returns 0 on success, -1 on failure
+ */
+int drv_if_status(struct netcf_if *nif, ncf_if_status_flags *flags) {
+    int is_active;
+
+    ERR_THROW(flags == NULL, nif->ncf, EOTHER, "NULL pointer for flags in
ncf_if_status");
+    *flags = 0;
+    is_active = if_is_active(nif->ncf, nif->name);
+    if (is_active)
+        *flags |= NETCF_IFACE_STATUS_UP;
+    return 0;
+error:
+    return -1;
+}
+
 /* Get the content of /interface/@name. Result must be freed with xmlFree()
  *
  * The name on VLAN interfaces is optional; if there is no
diff --git a/src/internal.h b/src/internal.h
index ca9a0ef..abdc01d 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -180,6 +180,7 @@ int drv_lookup_by_mac_string(struct netcf *, const char *mac,
                              int maxifaces, struct netcf_if **ifaces);
 char *drv_xml_desc(struct netcf_if *);
 char *drv_xml_state(struct netcf_if *);
+int drv_if_status(struct netcf_if *nif, ncf_if_status_flags *flags);
 const char *drv_mac_string(struct netcf_if *nif);
 struct netcf_if *drv_define(struct netcf *ncf, const char *xml);
 int drv_undefine(struct netcf_if *nif);
diff --git a/src/netcf.c b/src/netcf.c
index 1cd7aed..3cf045c 100644
--- a/src/netcf.c
+++ b/src/netcf.c
@@ -212,6 +212,14 @@ char *ncf_if_xml_state(struct netcf_if *nif) {
     return drv_xml_state(nif);
 }
 
+/* Report various status info about the interface as bits in
+ * "flags". Returns 0 on success, -1 on failure
+ */
+int ncf_if_status(struct netcf_if *nif, ncf_if_status_flags *flags) {
+    API_ENTRY(nif->ncf);
+    return drv_if_status(nif, flags);
+}
+
 /* Release any resources used by this NETCF_IF; the pointer is invalid
  * after this call
  */
diff --git a/src/netcf.h b/src/netcf.h
index a8619f5..0e1d4bd 100644
--- a/src/netcf.h
+++ b/src/netcf.h
@@ -163,6 +163,15 @@ char *ncf_if_xml_desc(struct netcf_if *);
  */
 char *ncf_if_xml_state(struct netcf_if *);
 
+typedef enum {
+    NETCF_IFACE_STATUS_UP = 1,
+} ncf_if_status_flags;
+
+/* Report various status info about the interface as bits in
+ * "flags". Returns 0 on success, -1 on failure
+ */
+int ncf_if_status(struct netcf_if *nif, ncf_if_status_flags *flags);
+
 /* Release any resources used by this NETCF_IF; the pointer is invalid
  * after this call
  */
diff --git a/src/netcf_public.syms b/src/netcf_public.syms
index 61464a5..9180614 100644
--- a/src/netcf_public.syms
+++ b/src/netcf_public.syms
@@ -23,3 +23,8 @@ NETCF_1.2.0 {
     global:
       ncf_if_xml_state;
 } NETCF_1.0.0;
+
+NETCF_1.3.0 {
+    global:
+      ncf_if_status;
+} NETCF_1.2.0;
-- 
1.6.5.15.gc274d

[Laine Stump]

A couple of questions on this topic:

1) I didn't add anything to ncftool that exercises this new 
functionality.  Do you see any use to this? (The information is given 
implicitly by "list").

2) Would it be useful to also add this information to the xml of 
ncf_if_xml_state()? If so, any suggestions on what it should look like?

On 10/22/2009 05:37 PM, Laine Stump wrote:
>  Here's a first draft of a suggested new API, subject to change
> based
>  on comments.
>
>  ncf_if_status returns various status bits about the given interface. As a
>  start, only a single bit is defined:
>
>          NETCF_IFACE_STATUS_UP
>
>  This bit is set if the interface is currently up (active) and not set if it
>  is currently down (inactive).
>
>  The value of all other bits should be considered random by an
>  application program, as later versions of netcf may use them. In other
>  words, you should always mask out all the other bits.
>  ---
>    src/drv_initscripts.c |   16 ++++++++++++++++
>    src/internal.h        |    1 +
>    src/netcf.c           |    8 ++++++++
>    src/netcf.h           |    9 +++++++++
>    src/netcf_public.syms |    5 +++++
>    5 files changed, 39 insertions(+), 0 deletions(-)
>
>  diff --git a/src/drv_initscripts.c b/src/drv_initscripts.c
>  index 6d03baa..17fad00 100644
>  --- a/src/drv_initscripts.c
>  +++ b/src/drv_initscripts.c
>  @@ -807,6 +807,22 @@ char *drv_xml_state(struct netcf_if *nif) {
>        goto done;
>    }
>
>  +/* Report various status info about the interface as bits in
>  + * "flags". Returns 0 on success, -1 on failure
>  + */
>  +int drv_if_status(struct netcf_if *nif, ncf_if_status_flags *flags) {
>  +    int is_active;
>  +
>  +    ERR_THROW(flags == NULL, nif->ncf, EOTHER, "NULL pointer for flags in
> ncf_if_status");
>  +    *flags = 0;
>  +    is_active = if_is_active(nif->ncf, nif->name);
>  +    if (is_active)
>  +        *flags |= NETCF_IFACE_STATUS_UP;
>  +    return 0;
>  +error:
>  +    return -1;
>  +}
>  +
>    /* Get the content of /interface/@name. Result must be freed with xmlFree()
>     *
>     * The name on VLAN interfaces is optional; if there is no
>  diff --git a/src/internal.h b/src/internal.h
>  index ca9a0ef..abdc01d 100644
>  --- a/src/internal.h
>  +++ b/src/internal.h
>  @@ -180,6 +180,7 @@ int drv_lookup_by_mac_string(struct netcf *, const char *mac,
>                                 int maxifaces, struct netcf_if **ifaces);
>    char *drv_xml_desc(struct netcf_if *);
>    char *drv_xml_state(struct netcf_if *);
>  +int drv_if_status(struct netcf_if *nif, ncf_if_status_flags *flags);
>    const char *drv_mac_string(struct netcf_if *nif);
>    struct netcf_if *drv_define(struct netcf *ncf, const char *xml);
>    int drv_undefine(struct netcf_if *nif);
>  diff --git a/src/netcf.c b/src/netcf.c
>  index 1cd7aed..3cf045c 100644
>  --- a/src/netcf.c
>  +++ b/src/netcf.c
>  @@ -212,6 +212,14 @@ char *ncf_if_xml_state(struct netcf_if *nif) {
>        return drv_xml_state(nif);
>    }
>
>  +/* Report various status info about the interface as bits in
>  + * "flags". Returns 0 on success, -1 on failure
>  + */
>  +int ncf_if_status(struct netcf_if *nif, ncf_if_status_flags *flags) {
>  +    API_ENTRY(nif->ncf);
>  +    return drv_if_status(nif, flags);
>  +}
>  +
>    /* Release any resources used by this NETCF_IF; the pointer is invalid
>     * after this call
>     */
>  diff --git a/src/netcf.h b/src/netcf.h
>  index a8619f5..0e1d4bd 100644
>  --- a/src/netcf.h
>  +++ b/src/netcf.h
>  @@ -163,6 +163,15 @@ char *ncf_if_xml_desc(struct netcf_if *);
>     */
>    char *ncf_if_xml_state(struct netcf_if *);
>
>  +typedef enum {
>  +    NETCF_IFACE_STATUS_UP = 1,
>  +} ncf_if_status_flags;
>  +
>  +/* Report various status info about the interface as bits in
>  + * "flags". Returns 0 on success, -1 on failure
>  + */
>  +int ncf_if_status(struct netcf_if *nif, ncf_if_status_flags *flags);
>  +
>    /* Release any resources used by this NETCF_IF; the pointer is invalid
>     * after this call
>     */
>  diff --git a/src/netcf_public.syms b/src/netcf_public.syms
>  index 61464a5..9180614 100644
>  --- a/src/netcf_public.syms
>  +++ b/src/netcf_public.syms
>  @@ -23,3 +23,8 @@ NETCF_1.2.0 {
>        global:
>          ncf_if_xml_state;
>    } NETCF_1.0.0;
>  +
>  +NETCF_1.3.0 {
>  +    global:
>  +      ncf_if_status;
>  +} NETCF_1.2.0;
>

[Daniel P. Berrange]

On Fri, Oct 23, 2009 at 10:34:54AM -0400, Laine Stump wrote:
>  A couple of questions on this topic:
>  
>  1) I didn't add anything to ncftool that exercises this new 
>  functionality.  Do you see any use to this? (The information is given 
>  implicitly by "list"). 
It could be useful to include it in the list output, eg
instead of

ncftool> list --all
eth0
eth1
lo
ppp0

have

ncftool> list --all
eth0   UP
eth1   UP
lo     UP
ppp0   DOWN

or some less crude formatting

>  2) Would it be useful to also add this information to the xml of 
>  ncf_if_xml_state()? If so, any suggestions on what it should look like? 
I don't think it is relevant there, because whether an interface is up
or down is not configuration information.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

[Laine Stump]

On 10/23/2009 12:32 PM, Daniel P. Berrange wrote:
>  On Fri, Oct 23, 2009 at 10:34:54AM -0400, Laine Stump wrote:
>     
> > A couple of questions on this topic:
> >
> > 1) I didn't add anything to ncftool that exercises this new
> > functionality.  Do you see any use to this? (The information is given
> > implicitly by "list").
> >      
>  It could be useful to include it in the list output, eg
>  instead of
>
>  ncftool>  list --all
>  eth0
>  eth1
>  lo
>  ppp0
>
>  have
>
>  ncftool>  list --all
>  eth0   UP
>  eth1   UP
>  lo     UP
>  ppp0   DOWN
>
>  or some less crude formatting
>      
Okay, I'll take a whack at that momentarily.

I'm also adding in reporting of mac address, which will hopefully work 
as easily as it seems to be getting written...

>     
> > 2) Would it be useful to also add this information to the xml of
> > ncf_if_xml_state()? If so, any suggestions on what it should look like?
> >      
>  I don't think it is relevant there, because whether an interface is up
>  or down is not configuration information.
>      
ncf_if_xml_desc() is the config, ncf_if_xml_state() is only the 
information we got directly from the kernel.

[Daniel P. Berrange]

On Fri, Oct 23, 2009 at 12:58:12PM -0400, Laine Stump wrote:
>  On 10/23/2009 12:32 PM, Daniel P. Berrange wrote:
>  >>2) Would it be useful to also add this information to the xml of
>  >>ncf_if_xml_state()? If so, any suggestions on what it should look like?
>  >>     
>  >I don't think it is relevant there, because whether an interface is up
>  >or down is not configuration information.
>  >   
>  
>  ncf_if_xml_desc() is the config, ncf_if_xml_state() is only the 
>  information we got directly from the kernel. 
Sorry, terminology mixup. By  'configuration information' here, I meant
stuff about IP addresses, routes, type of interfaces, etc, regardless
of whether the information comes from a config file, or the kernel

Whether an interface is up or down isn't a configuration property in 
this sense, so is outside scope of the XML description IMHO.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

[Daniel P. Berrange]

On Thu, Oct 22, 2009 at 05:37:40PM -0400, Laine Stump wrote:
>  Here's a first draft of a suggested new API, subject to change
> based
>  on comments.
>  
>  ncf_if_status returns various status bits about the given interface. As a
>  start, only a single bit is defined:
>  
>         NETCF_IFACE_STATUS_UP
>  
>  This bit is set if the interface is currently up (active) and not set if it
>  is currently down (inactive).
>  
>  The value of all other bits should be considered random by an
>  application program, as later versions of netcf may use them. In other
>  words, you should always mask out all the other bits. 

ACK, this looks sufficient for the usage I was wanting in libvirt.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

[David Lutterkort]

On Thu, 2009-10-22 at 17:37 -0400, Laine Stump wrote:
>  Here's a first draft of a suggested new API, subject to change
> based
>  on comments.
>  
>  ncf_if_status returns various status bits about the given interface. As a
>  start, only a single bit is defined:
>  
>         NETCF_IFACE_STATUS_UP
>  
>  This bit is set if the interface is currently up (active) and not set if it
>  is currently down (inactive).
>  
>  The value of all other bits should be considered random by an
>  application program, as later versions of netcf may use them. In other
>  words, you should always mask out all the other bits. 
This all looks good, one question though:

>  diff --git a/src/netcf.h b/src/netcf.h
>  index a8619f5..0e1d4bd 100644
>  --- a/src/netcf.h
>  +++ b/src/netcf.h
>  @@ -163,6 +163,15 @@ char *ncf_if_xml_desc(struct netcf_if *);
>    */
>   char *ncf_if_xml_state(struct netcf_if *);
>   
>  +typedef enum {
>  +    NETCF_IFACE_STATUS_UP = 1,
>  +} ncf_if_status_flags; 
How about reusing the existing type netcf_if_flag_t ? Longer term, it
seems to me that we would want to filter interface lists by the same
criteria that we list as status for each of them. For consistency, we
should also call this type ncf_if_status_flags_t, since all the other
types have a _t at the end.

David

[Laine Stump]

On 10/26/2009 06:00 AM, David Lutterkort wrote:
>  On Thu, 2009-10-22 at 17:37 -0400, Laine Stump wrote:
>     
> > Here's a first draft of a suggested new API, subject to change based
> > on comments.
> >
> > ncf_if_status returns various status bits about the given interface. As a
> > start, only a single bit is defined:
> >
> >         NETCF_IFACE_STATUS_UP
> >
> > This bit is set if the interface is currently up (active) and not set if it
> > is currently down (inactive).
> >
> > The value of all other bits should be considered random by an
> > application program, as later versions of netcf may use them. In other
> > words, you should always mask out all the other bits.
> >      
>  This all looks good, one question though:
>
>     
> > diff --git a/src/netcf.h b/src/netcf.h
> > index a8619f5..0e1d4bd 100644
> > --- a/src/netcf.h
> > +++ b/src/netcf.h
> > @@ -163,6 +163,15 @@ char *ncf_if_xml_desc(struct netcf_if *);
> >    */
> >   char *ncf_if_xml_state(struct netcf_if *);
> >
> > +typedef enum {
> > +    NETCF_IFACE_STATUS_UP = 1,
> > +} ncf_if_status_flags;
> >      
>  How about reusing the existing type netcf_if_flag_t ? Longer term, it
>  seems to me that we would want to filter interface lists by the same
>  criteria that we list as status for each of them. 

That thought briefly went through my mind too. I don't have any problem 
with it. The semantics are a bit different, though - the flag bit in 
netcf_if_flag_t uses "ACTIVE" instead of "UP", and it has 2 bits, one

for active and one for inactive. So to be consistent, one or the other 
of the bits would need to be set (of course in practice, by definition 
they are always the opposite of each other, so the application would 
only need to test one of the bits).

And of course it's always possible that the set of flags for the two 
won't exactly match, which could lead to some nonsensical bits for one 
or the other. I guess as long as they're documented, that wouldn't be a 
problem, though.

I can easily change it to work that way. Want?

>  For consistency, we
>  should also call this type ncf_if_status_flags_t, since all the other
>  types have a _t at the end.
>      
Sorry, I missed that. If you don't want the 1st change above, I'll at 
least do this.

[David Lutterkort]

On Mon, 2009-10-26 at 09:21 -0400, Laine Stump wrote:
>  On 10/26/2009 06:00 AM, David Lutterkort wrote: 
>  That thought briefly went through my mind too. I don't have any
> problem 
>  with it. The semantics are a bit different, though - the flag bit in 
>  netcf_if_flag_t uses "ACTIVE" instead of "UP", and it has 2 bits, one
>
>  for active and one for inactive. So to be consistent, one or the other 
>  of the bits would need to be set (of course in practice, by definition 
>  they are always the opposite of each other, so the application would 
>  only need to test one of the bits). 
True - we just need to make sure we set one or the other.

>  And of course it's always possible that the set of flags for the
> two 
>  won't exactly match, which could lead to some nonsensical bits for one 
>  or the other. I guess as long as they're documented, that wouldn't be a 
>  problem, though. 
'Nonsensical' in the sense that filtering by them might not be very
important, though there's no harm in allowing that; if we can query the
status of an interface for something, we can always filter by it and
vice versa. So I don't think that that will be an issue.

>  I can easily change it to work that way. Want? 
Yes, please.

David

[Laine Stump]

New version. This one reuses an existing enum (netcf_if_flag_t) rather
than creating a new enum. Also, the API passes the flags as unsigned
int* rather than an enum.

ncf_if_status returns various status bits about the given interface,
as described in the definition of the enum type
netcf_if_flag_t. Currently that defines two bits:

       NETCF_IFACE_INACTIVE
       NETCF_IFACE_ACTIVE

If the interface is currently up (active), NETCF_IFACE_ACTIVE is set,
otherwise NETCF_IFACE_INACTIVE is set. By definition, one or the other
of these bits will always be set (unless an error is encountered),
never both, so you can safely just check one of them.

The value of all other bits should be considered random by an
application program, as later versions of netcf may use them. In other
words, you should always mask out all the other bits when checking.
---
 src/drv_initscripts.c |   18 ++++++++++++++++++
 src/internal.h        |    1 +
 src/netcf.c           |    8 ++++++++
 src/netcf.h           |    6 ++++++
 src/netcf_public.syms |    5 +++++
 5 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/src/drv_initscripts.c b/src/drv_initscripts.c
index 6d03baa..8e082b6 100644
--- a/src/drv_initscripts.c
+++ b/src/drv_initscripts.c
@@ -807,6 +807,24 @@ char *drv_xml_state(struct netcf_if *nif) {
     goto done;
 }
 
+/* Report various status info about the interface as bits in
+ * "flags". Returns 0 on success, -1 on failure
+ */
+int drv_if_status(struct netcf_if *nif, unsigned int *flags) {
+    int is_active;
+
+    ERR_THROW(flags == NULL, nif->ncf, EOTHER, "NULL pointer for flags in
ncf_if_status");
+    *flags = 0;
+    is_active = if_is_active(nif->ncf, nif->name);
+    if (is_active)
+        *flags |= NETCF_IFACE_ACTIVE;
+    else
+        *flags |= NETCF_IFACE_INACTIVE;
+    return 0;
+error:
+    return -1;
+}
+
 /* Get the content of /interface/@name. Result must be freed with xmlFree()
  *
  * The name on VLAN interfaces is optional; if there is no
diff --git a/src/internal.h b/src/internal.h
index ca9a0ef..d06665d 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -180,6 +180,7 @@ int drv_lookup_by_mac_string(struct netcf *, const char *mac,
                              int maxifaces, struct netcf_if **ifaces);
 char *drv_xml_desc(struct netcf_if *);
 char *drv_xml_state(struct netcf_if *);
+int drv_if_status(struct netcf_if *nif, unsigned int *flags);
 const char *drv_mac_string(struct netcf_if *nif);
 struct netcf_if *drv_define(struct netcf *ncf, const char *xml);
 int drv_undefine(struct netcf_if *nif);
diff --git a/src/netcf.c b/src/netcf.c
index 1cd7aed..7c4969b 100644
--- a/src/netcf.c
+++ b/src/netcf.c
@@ -212,6 +212,14 @@ char *ncf_if_xml_state(struct netcf_if *nif) {
     return drv_xml_state(nif);
 }
 
+/* Report various status info about the interface as bits in
+ * "flags". Returns 0 on success, -1 on failure
+ */
+int ncf_if_status(struct netcf_if *nif, unsigned int *flags) {
+    API_ENTRY(nif->ncf);
+    return drv_if_status(nif, flags);
+}
+
 /* Release any resources used by this NETCF_IF; the pointer is invalid
  * after this call
  */
diff --git a/src/netcf.h b/src/netcf.h
index a8619f5..bf5e4e2 100644
--- a/src/netcf.h
+++ b/src/netcf.h
@@ -163,6 +163,12 @@ char *ncf_if_xml_desc(struct netcf_if *);
  */
 char *ncf_if_xml_state(struct netcf_if *);
 
+/* Report various status info about the interface as bits in
+ * "flags". The meaning of the bits is in the enum type netcf_if_flag_t.
+ * Returns 0 on success, -1 on failure
+ */
+int ncf_if_status(struct netcf_if *nif, unsigned int *flags);
+
 /* Release any resources used by this NETCF_IF; the pointer is invalid
  * after this call
  */
diff --git a/src/netcf_public.syms b/src/netcf_public.syms
index 61464a5..9180614 100644
--- a/src/netcf_public.syms
+++ b/src/netcf_public.syms
@@ -23,3 +23,8 @@ NETCF_1.2.0 {
     global:
       ncf_if_xml_state;
 } NETCF_1.0.0;
+
+NETCF_1.3.0 {
+    global:
+      ncf_if_status;
+} NETCF_1.2.0;
-- 
1.6.5.15.gc274d

[David Lutterkort]

On Mon, 2009-10-26 at 13:28 -0400, Laine Stump wrote:
>  New version. This one reuses an existing enum (netcf_if_flag_t)
> rather
>  than creating a new enum. Also, the API passes the flags as unsigned
>  int* rather than an enum. 
ACK. Committed.

David