+1 don't see any issue with this patch. Will apply and test later. -Sharad On Mon, 2012-01-30 at 21:57 -0200, Eduardo Lima (Etrunko) wrote:

From: "Eduardo Lima (Etrunko)" <eblima@br.ibm.com>

According to libvirt network filters specification, since version 0.9.8, priority values can accept negative values within the range -1000 to 1000. Thus the FilterEntry property value type went from unsigned int to signed int.

Signed-off-by: Eduardo Lima (Etrunko) <eblima@br.ibm.com> --- schema/FilterEntry.mof | 18 +++++++++--------- src/Virt_FilterEntry.c | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/schema/FilterEntry.mof b/schema/FilterEntry.mof index b51150c..f60abae 100644 --- a/schema/FilterEntry.mof +++ b/schema/FilterEntry.mof @@ -30,11 +30,11 @@ class KVM_Hdr8021Filter : CIM_Hdr8021Filter "the rule will be, instantiated relative to other rules. " "Rules with lower value will be instantiated and therefore " "evaluated before rules with higher value. Valid values are " - "in the range of 0 to 1000. If this attribute is not " + "in the range of -1000 to 1000. If this attribute is not " "provided, the value 500 will automatically be assigned."), - MinValue(0), + MinValue(-1000), MaxValue(1000)] - uint16 Priority = 500; + sint16 Priority = 500; };

[Provider("cmpi::Virt_FilterEntry")] @@ -68,11 +68,11 @@ class KVM_IPHeadersFilter : CIM_IPHeadersFilter "the rule will be, instantiated relative to other rules. " "Rules with lower value will be instantiated and therefore " "evaluated before rules with higher value. Valid values are " - "in the range of 0 to 1000. If this attribute is not " + "in the range of -1000 to 1000. If this attribute is not " "provided, the value 500 will automatically be assigned."), - MinValue(0), + MinValue(-1000), MaxValue(1000)] - uint16 Priority = 500; + sint16 Priority = 500; };

[Provider("cmpi::Virt_FilterEntry")] @@ -98,9 +98,9 @@ class KVM_FilterEntry : CIM_FilterEntry "the rule will be, instantiated relative to other rules. " "Rules with lower value will be instantiated and therefore " "evaluated before rules with higher value. Valid values are " - "in the range of 0 to 1000. If this attribute is not " + "in the range of -1000 to 1000. If this attribute is not " "provided, the value 500 will automatically be assigned."), - MinValue(0), + MinValue(-1000), MaxValue(1000)] - uint16 Priority = 500; + sint16 Priority = 500; }; diff --git a/src/Virt_FilterEntry.c b/src/Virt_FilterEntry.c index 16b211e..a4fa4cd 100644 --- a/src/Virt_FilterEntry.c +++ b/src/Virt_FilterEntry.c @@ -554,7 +554,7 @@ static CMPIInstance *convert_rule_to_instance( CMSetProperty(inst, "Direction", (CMPIValue *)&direction, CMPI_uint16);

priority = convert_priority(rule->priority); - CMSetProperty(inst, "Priority", (CMPIValue *)&priority, CMPI_uint16); + CMSetProperty(inst, "Priority", (CMPIValue *)&priority, CMPI_sint16);

if (convert_f) convert_f(rule, inst, broker);

+1 looks okay -Sharad On Mon, 2012-01-30 at 21:57 -0200, Eduardo Lima (Etrunko) wrote:

From: "Eduardo Lima (Etrunko)" <eblima@br.ibm.com>

According to libvirt network filter specification, since verstion 0.9.8, filter lists can be assigned priorities.

Signed-off-by: Eduardo Lima (Etrunko) <eblima@br.ibm.com> --- libxkutil/acl_parsing.c | 2 ++ libxkutil/acl_parsing.h | 1 + schema/FilterList.mof | 9 +++++++++ src/Makefile.am | 4 ++-- src/Virt_FilterEntry.c | 11 ++++------- src/Virt_FilterEntry.h | 6 ++++++ src/Virt_FilterList.c | 5 ++++- 7 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/libxkutil/acl_parsing.c b/libxkutil/acl_parsing.c index 9c4b4b2..2930928 100644 --- a/libxkutil/acl_parsing.c +++ b/libxkutil/acl_parsing.c @@ -134,6 +134,7 @@ void cleanup_filter(struct acl_filter *filter) free(filter->uuid); free(filter->name); free(filter->chain); + free(filter->priority);

for (i = 0; i < filter->rule_ct; i++) cleanup_rule(filter->rules[i]); @@ -345,6 +346,7 @@ static int parse_acl_filter(xmlNode *fnode, struct acl_filter *filter) goto err;

filter->chain = get_attr_value(fnode, "chain"); + filter->priority = get_attr_value(fnode, "priority");

for (child = fnode->children; child != NULL; child = child->next) { if (XSTREQ(child->name, "uuid")) { diff --git a/libxkutil/acl_parsing.h b/libxkutil/acl_parsing.h index 5b99175..5abcb02 100644 --- a/libxkutil/acl_parsing.h +++ b/libxkutil/acl_parsing.h @@ -148,6 +148,7 @@ struct acl_filter { char *uuid; char *name; char *chain; + char *priority;

struct acl_rule **rules; int rule_ct; diff --git a/schema/FilterList.mof b/schema/FilterList.mof index a98c14d..7339db6 100644 --- a/schema/FilterList.mof +++ b/schema/FilterList.mof @@ -1,4 +1,13 @@ // Copyright IBM Corp. 2011 class KVM_FilterList : CIM_FilterList { + [Description("The priority of the filter controls the order in which " + "the filter will be, instantiated relative to other filters. " + "Filters with lower value will be instantiated and therefore " + "evaluated before rules with higher value. Valid values are " + "in the range of -1000 to 1000. If this attribute is not " + "provided, the value 500 will automatically be assigned."), + MinValue(-1000), + MaxValue(1000)] + sint16 Priority = 500; }; diff --git a/src/Makefile.am b/src/Makefile.am index c28dc9a..3f90926 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -255,9 +255,9 @@ libVirt_FilterEntry_la_DEPENDENCIES = libVirt_HostSystem.la libVirt_FilterEntry_la_SOURCES = Virt_FilterEntry.c libVirt_FilterEntry_la_LIBADD = -lVirt_HostSystem

-libVirt_FilterList_la_DEPENDENCIES = libVirt_HostSystem.la +libVirt_FilterList_la_DEPENDENCIES = libVirt_HostSystem.la libVirt_FilterEntry.la libVirt_FilterList_la_SOURCES = Virt_FilterList.c -libVirt_FilterList_la_LIBADD = -lVirt_HostSystem +libVirt_FilterList_la_LIBADD = -lVirt_HostSystem -lVirt_FilterEntry

libVirt_EntriesInFilterList_la_DEPENDENCIES = libVirt_FilterEntry.la libVirt_FilterList.la libVirt_EntriesInFilterList_la_SOURCES = Virt_EntriesInFilterList.c diff --git a/src/Virt_FilterEntry.c b/src/Virt_FilterEntry.c index a4fa4cd..c058b5e 100644 --- a/src/Virt_FilterEntry.c +++ b/src/Virt_FilterEntry.c @@ -192,15 +192,12 @@ static int convert_direction(const char *s) return direction; }

-static int convert_priority(const char *s) +int convert_priority(const char *s) { - int priority = 0; - - if (s != NULL) { - priority = atoi(s); - } + if (s == NULL) + return 0;

- return priority; + return atoi(s); }

static int convert_action(const char *s) diff --git a/src/Virt_FilterEntry.h b/src/Virt_FilterEntry.h index 2fe17c4..a30f46d 100644 --- a/src/Virt_FilterEntry.h +++ b/src/Virt_FilterEntry.h @@ -72,6 +72,12 @@ CMPIStatus instance_from_rule( struct acl_rule *rule, CMPIInstance **instance);

+/** + * Convert a string representing the priority to corresponding integer value + * + * @param s A pointer to a string representing the priority + */ +int convert_priority(const char *s); #endif

/* diff --git a/src/Virt_FilterList.c b/src/Virt_FilterList.c index 5b1b6e8..9b5dbae 100644 --- a/src/Virt_FilterList.c +++ b/src/Virt_FilterList.c @@ -31,6 +31,7 @@

#include "Virt_FilterList.h" #include "Virt_HostSystem.h" +#include "Virt_FilterEntry.h"

const static CMPIBroker *_BROKER;

@@ -44,7 +45,7 @@ static CMPIInstance *convert_filter_to_instance( CMPIInstance *inst = NULL; const char *sys_name = NULL; const char *sys_ccname = NULL; - int direction = 0; + int direction = 0, priority;

inst = get_typed_instance(broker, CLASSNAME(reference), @@ -77,6 +78,8 @@ static CMPIInstance *convert_filter_to_instance( CMPI_chars); CMSetProperty(inst, "Direction", (CMPIValue *)&direction, CMPI_uint16);

+ priority = convert_priority(filter->priority); + CMSetProperty(inst, "Priority", (CMPIValue *)&priority, CMPI_sint16); out: return inst; }

+1 -sharad On Mon, 2012-01-30 at 21:57 -0200, Eduardo Lima (Etrunko) wrote:

From: "Eduardo Lima (Etrunko)" <eblima@br.ibm.com>

According to libvirt network filters specification, new values have been added:

Since 0.9.0: - reject

Since 0.9.7: - return - continue

Signed-off-by: Eduardo Lima (Etrunko) <eblima@br.ibm.com> --- schema/FilterEntry.mof | 8 ++++---- src/Virt_FilterEntry.c | 10 ++++++++-- 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/schema/FilterEntry.mof b/schema/FilterEntry.mof index f60abae..b5f695f 100644 --- a/schema/FilterEntry.mof +++ b/schema/FilterEntry.mof @@ -6,8 +6,8 @@ class KVM_Hdr8021Filter : CIM_Hdr8021Filter "This defines whether the action should be to forward or " "deny traffic meeting the match condition specified in " "this filter." ), - ValueMap { "1", "2" }, - Values { "Permit", "Deny" }] + ValueMap { "1", "2", "3", "4", "5" }, + Values { "Permit", "Deny", "Reject", "Return", "Continue" }] uint16 Action;

[Description("This defines whether the Filter is used for input, " @@ -44,8 +44,8 @@ class KVM_IPHeadersFilter : CIM_IPHeadersFilter "This defines whether the action should be to forward or " "deny traffic meeting the match condition specified in " "this filter." ), - ValueMap { "1", "2" }, - Values { "Permit", "Deny" }] + ValueMap { "1", "2", "3", "4", "5" }, + Values { "Permit", "Deny", "Reject", "Return", "Continue" }] uint16 Action;

[Description("This defines whether the Filter is used for input, " diff --git a/src/Virt_FilterEntry.c b/src/Virt_FilterEntry.c index c058b5e..2ff354a 100644 --- a/src/Virt_FilterEntry.c +++ b/src/Virt_FilterEntry.c @@ -202,13 +202,19 @@ int convert_priority(const char *s)

static int convert_action(const char *s) { - enum {NONE=0, ACCEPT, DENY} action = NONE; + enum {NONE=0, ACCEPT, DENY, REJECT, RETURN, CONTINUE} action = NONE;

if (s != NULL) { if (STREQC(s, "accept")) action = ACCEPT; - else if (STREQC(s, "drop") || STREQC(s, "reject")) + else if (STREQC(s, "drop")) action = DENY; + else if (STREQC(s, "reject")) + action = REJECT; + else if (STREQC(s, "return")) + action = RETURN; + else if (STREQC(s, "continue")) + action = CONTINUE; } return action; }

On Mon, 2012-01-30 at 21:57 -0200, Eduardo Lima (Etrunko) wrote:

From: "Eduardo Lima (Etrunko)" <eblima@br.ibm.com>

The HdrProtocolID8021 property expects an unsigned int value, while libvirt network filters specification allows the corresponding value for this property (protocolid) to be written as a string (ipv4, ipv6, arp, rarp).

The corresponding values for the protocolid strings can be found in: http://www.iana.org/assignments/ethernet-numbers

Signed-off-by: Eduardo Lima (Etrunko) <eblima@br.ibm.com> --- src/Virt_FilterEntry.c | 26 ++++++++++++++++++++++++-- 1 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/src/Virt_FilterEntry.c b/src/Virt_FilterEntry.c index 2ff354a..126615b 100644 --- a/src/Virt_FilterEntry.c +++ b/src/Virt_FilterEntry.c @@ -219,6 +219,24 @@ static int convert_action(const char *s) return action; }

+static unsigned long convert_protocol_id(const char *s) +{ + enum {NONE = 0, IPV4 = 2048, ARP = 2054, RARP = 32821, IPV6 = 34525} id = NONE; + + if (s != NULL) { + if (STREQC(s, "ipv4")) + id = IPV4; + else if (STREQC(s, "arp")) + id = ARP; + else if (STREQC(s, "rarp")) + id = RARP; + else if (STREQC(s, "ipv6")) + id = IPV6; + } + + return id; +} + static void convert_mac_rule_to_instance( struct acl_rule *rule, CMPIInstance *inst, @@ -265,8 +283,12 @@ static void convert_mac_rule_to_instance( (CMPIValue *)&array, CMPI_uint8A);

if (rule->var.mac.protocol_id != NULL) { - unsigned long n = strtoul(rule->var.mac.protocol_id, - NULL, 16); + unsigned long n = convert_protocol_id(rule->var.mac.protocol_id); + + /* Unknown protocolid string. Try converting from hexadecimal value */ + if (n == 0) + n = strtoul(rule->var.mac.protocol_id, NULL, 16); + CMSetProperty(inst, "HdrProtocolID8021", (CMPIValue *)&n, CMPI_uint16);

Why are we converting to hex if unknown protocol? Can we just use "0" to indicate that its 'unknown'? -Sharad

}