[libvirt] [PATCH v2 0/5] nwfilter: Support comment attribute in filter rule descriptions
V2: - work on the iptables instantiation patch (2/5) - work on the parser patch (1/5) - small changes to the test cases (5/5) The following patch series adds support for a comment node to the XML attributes of all protocols. If possible, as for example in case of iptables, the comments are instantiated (iptables ... -m comment --comment ...). The patches do the following: - extend the parser and XML generator to parse and create XML with the comment attribute - instantiate the comment in case of ip(6)tables - extend the nwfilter.rng schema with the comment attribute - add the information to the web docs - add a test case for the XML parser/generator to be run during 'make check' Regards, Stefan
V2: - introducing a field 'maxstrlen' to control the length of accepted string - removed function validating comment string assuming the XML parser already sinity checked the string The patch below extends the XML parser and generator so that every protocol now can have a comment node. Comments are limited to 256 characters and are tested to only contain printable characters or spaces. Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- src/conf/nwfilter_conf.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/nwfilter_conf.h | 10 +++++- 2 files changed, 87 insertions(+), 1 deletion(-) Index: libvirt-acl/src/conf/nwfilter_conf.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -181,6 +181,8 @@ static const char dscp_str[] = " #define DSCP dscp_str +#define MAX_COMMENT_LENGTH 256 + /** * intMapGetByInt: * @intmap: Pointer to int-to-string map @@ -246,7 +248,11 @@ virNWFilterRuleDefFree(virNWFilterRuleDe for (i = 0; i < def->nvars; i++) VIR_FREE(def->vars[i]); + for (i = 0; i < def->nstrings; i++) + VIR_FREE(def->strings[i]); + VIR_FREE(def->vars); + VIR_FREE(def->strings); VIR_FREE(def); } @@ -350,6 +356,29 @@ virNWFilterRuleDefAddVar(virNWFilterRule } +static char * +virNWFilterRuleDefAddString(virNWFilterRuleDefPtr nwf, + const char *string, + size_t maxstrlen) +{ + if (VIR_REALLOC_N(nwf->strings, nwf->nstrings+1) < 0) { + virReportOOMError(); + return NULL; + } + + nwf->strings[nwf->nstrings] = strndup(string, maxstrlen); + + if (!nwf->strings[nwf->nstrings]) { + virReportOOMError(); + return NULL; + } + + nwf->nstrings++; + + return nwf->strings[nwf->nstrings-1]; +} + + void virNWFilterPoolObjRemove(virNWFilterPoolObjListPtr pools, virNWFilterPoolObjPtr pool) @@ -399,6 +428,7 @@ struct _virXMLAttr2Struct int dataIdx; // offset of the hasXYZ boolean valueValidator validator; // beyond-standard checkers valueFormatter formatter; // beyond-standard formatter + size_t maxstrlen; }; @@ -655,6 +685,18 @@ dscpValidator(enum attrDatatype datatype } +#define COMMENT_PROP(STRUCT) \ + {\ + .name = "comment",\ + .datatype = DATATYPE_STRINGCOPY,\ + .dataIdx = offsetof(virNWFilterRuleDef, p.STRUCT.dataComment),\ + .maxstrlen = MAX_COMMENT_LENGTH,\ + } + +#define COMMENT_PROP_IPHDR(STRUCT) \ + COMMENT_PROP(STRUCT.ipHdr) + + static const virXMLAttr2Struct macAttributes[] = { COMMON_MAC_PROPS(ethHdrFilter), { @@ -664,6 +706,7 @@ static const virXMLAttr2Struct macAttrib .validator= checkMacProtocolID, .formatter= macProtocolIDFormatter, }, + COMMENT_PROP(ethHdrFilter), { .name = NULL, } @@ -702,6 +745,7 @@ static const virXMLAttr2Struct arpAttrib .datatype = DATATYPE_IPADDR, .dataIdx = offsetof(virNWFilterRuleDef, p.arpHdrFilter.dataARPDstIPAddr), }, + COMMENT_PROP(arpHdrFilter), { .name = NULL, } @@ -762,6 +806,7 @@ static const virXMLAttr2Struct ipAttribu .dataIdx = offsetof(virNWFilterRuleDef, p.ipHdrFilter.ipHdr.dataDSCP), .validator = dscpValidator, }, + COMMENT_PROP_IPHDR(ipHdrFilter), { .name = NULL, } @@ -817,6 +862,7 @@ static const virXMLAttr2Struct ipv6Attri .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX, .dataIdx = offsetof(virNWFilterRuleDef, p.ipv6HdrFilter.portData.dataDstPortEnd), }, + COMMENT_PROP_IPHDR(ipv6HdrFilter), { .name = NULL, } @@ -914,6 +960,7 @@ static const virXMLAttr2Struct tcpAttrib .datatype = DATATYPE_UINT8 | DATATYPE_UINT8_HEX, .dataIdx = offsetof(virNWFilterRuleDef, p.tcpHdrFilter.dataTCPOption), }, + COMMENT_PROP_IPHDR(tcpHdrFilter), { .name = NULL, } @@ -922,6 +969,7 @@ static const virXMLAttr2Struct tcpAttrib static const virXMLAttr2Struct udpAttributes[] = { COMMON_IP_PROPS(udpHdrFilter, DATATYPE_IPADDR, DATATYPE_IPMASK), COMMON_PORT_PROPS(udpHdrFilter), + COMMENT_PROP_IPHDR(udpHdrFilter), { .name = NULL, } @@ -929,6 +977,7 @@ static const virXMLAttr2Struct udpAttrib static const virXMLAttr2Struct udpliteAttributes[] = { COMMON_IP_PROPS(udpliteHdrFilter, DATATYPE_IPADDR, DATATYPE_IPMASK), + COMMENT_PROP_IPHDR(udpliteHdrFilter), { .name = NULL, } @@ -936,6 +985,7 @@ static const virXMLAttr2Struct udpliteAt static const virXMLAttr2Struct espAttributes[] = { COMMON_IP_PROPS(espHdrFilter, DATATYPE_IPADDR, DATATYPE_IPMASK), + COMMENT_PROP_IPHDR(espHdrFilter), { .name = NULL, } @@ -943,6 +993,7 @@ static const virXMLAttr2Struct espAttrib static const virXMLAttr2Struct ahAttributes[] = { COMMON_IP_PROPS(ahHdrFilter, DATATYPE_IPADDR, DATATYPE_IPMASK), + COMMENT_PROP_IPHDR(ahHdrFilter), { .name = NULL, } @@ -951,6 +1002,7 @@ static const virXMLAttr2Struct ahAttribu static const virXMLAttr2Struct sctpAttributes[] = { COMMON_IP_PROPS(sctpHdrFilter, DATATYPE_IPADDR, DATATYPE_IPMASK), COMMON_PORT_PROPS(sctpHdrFilter), + COMMENT_PROP_IPHDR(sctpHdrFilter), { .name = NULL, } @@ -969,6 +1021,7 @@ static const virXMLAttr2Struct icmpAttri .datatype = DATATYPE_UINT8 | DATATYPE_UINT8_HEX, .dataIdx = offsetof(virNWFilterRuleDef, p.icmpHdrFilter.dataICMPCode), }, + COMMENT_PROP_IPHDR(icmpHdrFilter), { .name = NULL, } @@ -977,6 +1030,7 @@ static const virXMLAttr2Struct icmpAttri static const virXMLAttr2Struct allAttributes[] = { COMMON_IP_PROPS(allHdrFilter, DATATYPE_IPADDR, DATATYPE_IPMASK), + COMMENT_PROP_IPHDR(allHdrFilter), { .name = NULL, } @@ -985,6 +1039,7 @@ static const virXMLAttr2Struct allAttrib static const virXMLAttr2Struct igmpAttributes[] = { COMMON_IP_PROPS(igmpHdrFilter, DATATYPE_IPADDR, DATATYPE_IPMASK), + COMMENT_PROP_IPHDR(igmpHdrFilter), { .name = NULL, } @@ -999,6 +1054,7 @@ static const virXMLAttr2Struct tcpipv6At .datatype = DATATYPE_UINT8 | DATATYPE_UINT8_HEX, .dataIdx = offsetof(virNWFilterRuleDef, p.tcpHdrFilter.dataTCPOption), }, + COMMENT_PROP_IPHDR(tcpHdrFilter), { .name = NULL, } @@ -1007,6 +1063,7 @@ static const virXMLAttr2Struct tcpipv6At static const virXMLAttr2Struct udpipv6Attributes[] = { COMMON_IP_PROPS(udpHdrFilter, DATATYPE_IPV6ADDR, DATATYPE_IPV6MASK), COMMON_PORT_PROPS(udpHdrFilter), + COMMENT_PROP_IPHDR(udpHdrFilter), { .name = NULL, } @@ -1015,6 +1072,7 @@ static const virXMLAttr2Struct udpipv6At static const virXMLAttr2Struct udpliteipv6Attributes[] = { COMMON_IP_PROPS(udpliteHdrFilter, DATATYPE_IPV6ADDR, DATATYPE_IPV6MASK), + COMMENT_PROP_IPHDR(udpliteHdrFilter), { .name = NULL, } @@ -1023,6 +1081,7 @@ static const virXMLAttr2Struct udpliteip static const virXMLAttr2Struct espipv6Attributes[] = { COMMON_IP_PROPS(espHdrFilter, DATATYPE_IPV6ADDR, DATATYPE_IPV6MASK), + COMMENT_PROP_IPHDR(espHdrFilter), { .name = NULL, } @@ -1031,6 +1090,7 @@ static const virXMLAttr2Struct espipv6At static const virXMLAttr2Struct ahipv6Attributes[] = { COMMON_IP_PROPS(ahHdrFilter, DATATYPE_IPV6ADDR, DATATYPE_IPV6MASK), + COMMENT_PROP_IPHDR(ahHdrFilter), { .name = NULL, } @@ -1040,6 +1100,7 @@ static const virXMLAttr2Struct ahipv6Att static const virXMLAttr2Struct sctpipv6Attributes[] = { COMMON_IP_PROPS(sctpHdrFilter, DATATYPE_IPV6ADDR, DATATYPE_IPV6MASK), COMMON_PORT_PROPS(sctpHdrFilter), + COMMENT_PROP_IPHDR(sctpHdrFilter), { .name = NULL, } @@ -1058,6 +1119,7 @@ static const virXMLAttr2Struct icmpv6Att .datatype = DATATYPE_UINT8 | DATATYPE_UINT8_HEX, .dataIdx = offsetof(virNWFilterRuleDef, p.icmpHdrFilter.dataICMPCode), }, + COMMENT_PROP_IPHDR(icmpHdrFilter), { .name = NULL, } @@ -1066,6 +1128,7 @@ static const virXMLAttr2Struct icmpv6Att static const virXMLAttr2Struct allipv6Attributes[] = { COMMON_IP_PROPS(allHdrFilter, DATATYPE_IPV6ADDR, DATATYPE_IPV6MASK), + COMMENT_PROP_IPHDR(allHdrFilter), { .name = NULL, } @@ -1332,6 +1395,17 @@ virNWFilterRuleDetailsParse(xmlNodePtr n found = 1; break; + case DATATYPE_STRINGCOPY: + if (!(item->u.string = + virNWFilterRuleDefAddString(nwf, prop, + att[idx].maxstrlen))) { + rc = -1; + break; + } + data.c = item->u.string; + found = 1; + break; + case DATATYPE_LAST: default: break; @@ -2510,6 +2584,10 @@ virNWFilterRuleDefDetailsFormat(virBuffe (j < 5) ? ":" : ""); break; + case DATATYPE_STRINGCOPY: + virBufferEscapeString(buf, "%s", item->u.string); + break; + case DATATYPE_STRING: default: virBufferVSprintf(buf, Index: libvirt-acl/src/conf/nwfilter_conf.h =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.h +++ libvirt-acl/src/conf/nwfilter_conf.h @@ -92,8 +92,9 @@ enum attrDatatype { DATATYPE_STRING = (1 << 8), DATATYPE_IPV6ADDR = (1 << 9), DATATYPE_IPV6MASK = (1 << 10), + DATATYPE_STRINGCOPY = (1 << 11), - DATATYPE_LAST = (1 << 11), + DATATYPE_LAST = (1 << 12), }; @@ -123,6 +124,7 @@ struct _nwItemDesc { uint8_t u8; uint16_t u16; char protocolID[10]; + char *string; } u; }; @@ -142,6 +144,7 @@ typedef ethHdrFilterDef *ethHdrFilterDef struct _ethHdrFilterDef { ethHdrDataDef ethHdr; nwItemDesc dataProtocolID; + nwItemDesc dataComment; }; @@ -156,6 +159,7 @@ struct _arpHdrFilterDef { nwItemDesc dataARPSrcIPAddr; nwItemDesc dataARPDstMACAddr; nwItemDesc dataARPDstIPAddr; + nwItemDesc dataComment; }; @@ -174,6 +178,7 @@ struct _ipHdrDataDef { nwItemDesc dataDstIPTo; nwItemDesc dataDSCP; nwItemDesc dataConnlimitAbove; + nwItemDesc dataComment; }; @@ -376,6 +381,9 @@ struct _virNWFilterRuleDef { int nvars; char **vars; + + int nstrings; + char **strings; };
On 09/27/2010 12:40 PM, Stefan Berger wrote:
V2: - introducing a field 'maxstrlen' to control the length of accepted string - removed function validating comment string assuming the XML parser already sinity checked the string
s/sinity/sanity/, if you leave this in your commit message
The patch below extends the XML parser and generator so that every protocol now can have a comment node. Comments are limited to 256 characters and are tested to only contain printable characters or spaces.
Literal spaces, or generic blanks (space and tab for sure, but what about newline, not to mention vertical tab, form feed, ...), all of which are technically printable according to c_isprint()? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake <eblake@redhat.com> wrote on 09/27/2010 04:02:54 PM:
On 09/27/2010 12:40 PM, Stefan Berger wrote:
V2: - introducing a field 'maxstrlen' to control the length of accepted
string
- removed function validating comment string assuming the XML parser already sinity checked the string
The patch below extends the XML parser and generator so that every
protocol
now can have a comment node. Comments are limited to 256 characters and are tested to only contain printable characters or spaces.
Literal spaces, or generic blanks (space and tab for sure, but what about newline, not to mention vertical tab, form feed, ...), all of which are technically printable according to c_isprint()?
I have been doing some testing using 'virsh nwfilter-edit'. I edited a comment and put horizontal tabs into it or newlines or a combination of both. Both of these seem to automatically be converted to space, meaning that when the XML is generated after the parsing, the tab or newline now are shown as simple space. I also wrote the XML into a file using 'virsh nwfilter-dumpxml', added a newline in the comment, ran unix2dos on it to get '\r' and used 'virsh nwfilter-define' on it again, and the '\r' disappeared. From that I took that the XML parser does the correct replacement already and I don't need to worry about it -- assuming it would do the same for vertical tab as well. Stefan
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 09/27/2010 02:26 PM, Stefan Berger wrote:
The patch below extends the XML parser and generator so that every protocol now can have a comment node. Comments are limited to 256 characters and are tested to only contain printable characters or spaces.
Literal spaces, or generic blanks (space and tab for sure, but what about newline, not to mention vertical tab, form feed, ...), all of which are technically printable according to c_isprint()?
I have been doing some testing using 'virsh nwfilter-edit'. I edited a comment and put horizontal tabs into it or newlines or a combination of both. Both of these seem to automatically be converted to space, meaning that when the XML is generated after the parsing, the tab or newline now are shown as simple space. I also wrote the XML into a file using 'virsh nwfilter-dumpxml', added a newline in the comment, ran unix2dos on it to get '\r' and used 'virsh nwfilter-define' on it again, and the '\r' disappeared. From that I took that the XML parser does the correct replacement already and I don't need to worry about it -- assuming it would do the same for vertical tab as well.
Tab seems like it would be most likely for people to want, but I'm okay with leaving it unsupported for now until someone complains (and even then, it depends on whether XML parsing can even preserve tabs). Patch 1 #defines MAX_COMMENT_LENGTH 256 in nwfilter_conf.c, patch 2 #defines IPTABLES_MAX_COMMENT_SIZE 256 MAX_COMMENT_LENGTH 256 in nwfilter_ebiptables_driver.h. Should these two values be consolidated into a single name in a common .h file in patch 1? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake <eblake@redhat.com> wrote on 09/28/2010 03:19:42 PM:
On 09/27/2010 02:26 PM, Stefan Berger wrote:
The patch below extends the XML parser and generator so that every protocol now can have a comment node. Comments are limited to 256 characters and are tested to only contain printable characters or spaces.
Literal spaces, or generic blanks (space and tab for sure, but what about newline, not to mention vertical tab, form feed, ...), all of which are technically printable according to c_isprint()?
I have been doing some testing using 'virsh nwfilter-edit'. I edited a comment and put horizontal tabs into it or newlines or a combination
of
both. Both of these seem to automatically be converted to space, meaning that when the XML is generated after the parsing, the tab or newline now are shown as simple space. I also wrote the XML into a file using 'virsh nwfilter-dumpxml', added a newline in the comment, ran unix2dos on it to get '\r' and used 'virsh nwfilter-define' on it again, and the '\r' disappeared. From that I took that the XML parser does the correct replacement already and I don't need to worry about it -- assuming it would do the same for vertical tab as well.
Tab seems like it would be most likely for people to want, but I'm okay with leaving it unsupported for now until someone complains (and even then, it depends on whether XML parsing can even preserve tabs).
Patch 1 #defines MAX_COMMENT_LENGTH 256 in nwfilter_conf.c, patch 2 #defines IPTABLES_MAX_COMMENT_SIZE 256 MAX_COMMENT_LENGTH 256 in nwfilter_ebiptables_driver.h. Should these two values be consolidated into a single name in a common .h file in patch 1?
I chose two independent constants to reflect that the parser and the (iptables) driver should be somewhat loosely coupled. The iptables driver could cut the size of the comment down to what it can handle, though in this case the sizes match. Stefan
V2 changes: following Eric's comments: - commands in the script are assigned using cmd='...' rather than cmd="..." - invoke commands using eval res=... rather than res=eval ... - rewrote function escaping ` and single quotes (which became more tricky) In this patch I am extending the rule instantiator to create the comment node where supported, which is the case for iptables and ip6tables. Since commands are written in the format cmd='iptables ...-m comment --comment \"\" ' certain characters ('`) in the comment need to be escaped to prevent comments from becoming commands themselves or cause other forms of (bash) substitutions. I have tested this with various input and in my tests the input made it straight into the comment. A test case for TCK will be provided separately that tests this. Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- src/nwfilter/nwfilter_ebiptables_driver.c | 57 ++++++++++++++++++++++++++++-- src/nwfilter/nwfilter_ebiptables_driver.h | 2 + 2 files changed, 56 insertions(+), 3 deletions(-) Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -23,6 +23,7 @@ #include <config.h> +#include <string.h> #include <sys/stat.h> #include <fcntl.h> @@ -52,10 +53,10 @@ #define CMD_SEPARATOR "\n" -#define CMD_DEF_PRE "cmd=\"" -#define CMD_DEF_POST "\"" +#define CMD_DEF_PRE "cmd='" +#define CMD_DEF_POST "'" #define CMD_DEF(X) CMD_DEF_PRE X CMD_DEF_POST -#define CMD_EXEC "res=`${cmd}`" CMD_SEPARATOR +#define CMD_EXEC "eval res=\\`\"${cmd}\"\\`" CMD_SEPARATOR #define CMD_STOPONERR(X) \ X ? "if [ $? -ne 0 ]; then" \ " echo \"Failure to execute command '${cmd}'.\";" \ @@ -291,6 +292,46 @@ printDataTypeAsHex(virNWFilterHashTableP } +static char * +escapeComment(const char *buf) +{ + char *res; + size_t i, j, add = 0, len = strlen(buf); + + static const char SINGLEQUOTE_REPLACEMENT[12] = "'\\'\\\"\\'\\\"\\''"; + + if (len > IPTABLES_MAX_COMMENT_SIZE) + len = IPTABLES_MAX_COMMENT_SIZE; + + for (i = 0; i < len; i++) { + if (buf[i] == '`') + add++; + else if (buf[i] == '\'') + add += sizeof(SINGLEQUOTE_REPLACEMENT); + } + + if (VIR_ALLOC_VAR(res, char, len+add+1) < 0) { + virReportOOMError(); + return NULL; + } + + j = 0; + for (i = 0; i < len; i++) { + if (buf[i] == '`') + res[j++] = '\\'; + else if (buf[i] == '\'') { + strcpy(&res[j], SINGLEQUOTE_REPLACEMENT); + j += sizeof(SINGLEQUOTE_REPLACEMENT); + continue; + } + res[j++] = buf[i]; + } + res[j] = 0; + + return res; +} + + static void ebiptablesRuleInstFree(ebiptablesRuleInstPtr inst) { @@ -993,6 +1034,16 @@ iptablesHandleIpHdr(virBufferPtr buf, } } + if (HAS_ENTRY_ITEM(&ipHdr->dataComment)) { + char *cmt = escapeComment(ipHdr->dataComment.u.string); + if (!cmt) + goto err_exit; + virBufferVSprintf(buf, + " -m comment --comment '\\''%s'\\''", + cmt); + VIR_FREE(cmt); + } + return 0; err_exit: Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.h =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.h +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.h @@ -45,4 +45,6 @@ extern virNWFilterTechDriver ebiptables_ # define EBIPTABLES_DRIVER_ID "ebiptables" +# define IPTABLES_MAX_COMMENT_SIZE 256 + #endif
On 09/27/2010 12:40 PM, Stefan Berger wrote:
V2 changes: following Eric's comments: - commands in the script are assigned using cmd='...' rather than cmd="..." - invoke commands using eval res=... rather than res=eval ... - rewrote function escaping ` and single quotes (which became more tricky)
In this patch I am extending the rule instantiator to create the comment node where supported, which is the case for iptables and ip6tables.
Since commands are written in the format
cmd='iptables ...-m comment --comment \"\" '
certain characters ('`) in the comment need to be escaped to prevent comments from becoming commands themselves or cause other forms of (bash) substitutions. I have tested this with various input and in my tests the input made it straight into the comment. A test case for TCK will be provided separately that tests this.
At first, I failed to see how ` needs escaping inside '', unless you aren't uniformly using '' like you think you are. Then it hit me - yuck - we aren't uniformly using '' - we really do have to escape ` inside ``, or come up with an alternate solution. That is: ret=`iptables -m comment --comment '`'` is indeed ill-formed. But if you are going to escape `, then you also have to escape \ and $ for the same duration. Yuck again. But fear not - I have a slicker solution: comment='comment with lone '\'', ", `, \, $x, and two spaces' cmd='iptables ...-m comment --comment "$comment"' eval ret=\`"$cmd"\` which at expansion time results in: eval ret=\`"$cmd"\` ret=`iptables ...-m comment --comment "$comment"` iptables ...-m comment --comment \ 'comment with lone '\'', `, ", `, \, $x, and two spaces' That is, rather than trying to pass the comment literally through $cmd (and thus having to carefully escape $cmd for its eventual use inside ``), it might be nicer to stick the comment in an intermediate variable (where you only need to escape ') and make $cmd reference the intermediate variable (where you won't have any problematic uses of ", `, or ' to contend with, and where your only $ is one where you intentionally want variable expansion).
@@ -52,10 +53,10 @@
#define CMD_SEPARATOR "\n" -#define CMD_DEF_PRE "cmd=\"" -#define CMD_DEF_POST "\"" +#define CMD_DEF_PRE "cmd='" +#define CMD_DEF_POST "'" #define CMD_DEF(X) CMD_DEF_PRE X CMD_DEF_POST -#define CMD_EXEC "res=`${cmd}`" CMD_SEPARATOR +#define CMD_EXEC "eval res=\\`\"${cmd}\"\\`" CMD_SEPARATOR
This part seems okay - the ` is quoted to protect it from evaluation until after eval has had a chance to collect its arguments.
+static char * +escapeComment(const char *buf) +{ + char *res; + size_t i, j, add = 0, len = strlen(buf); + + static const char SINGLEQUOTE_REPLACEMENT[12] = "'\\'\\\"\\'\\\"\\''";
That seems rather long to me. Broken into chunks with C-string escaping eliminated: ' \'\"\'\"\' ' end the current single quoting the 5 literal shell chars '"'"' resume single quoting I'm not following why we need 5 literal shell characters, instead of 1.
+ + if (len > IPTABLES_MAX_COMMENT_SIZE) + len = IPTABLES_MAX_COMMENT_SIZE; + + for (i = 0; i < len; i++) { + if (buf[i] == '`') + add++; + else if (buf[i] == '\'') + add += sizeof(SINGLEQUOTE_REPLACEMENT); + }
Back to my observation that this doesn't help for \ or $, the other two characters that need escaping inside ``. It would be so much easier if we could rely on $() instead of ``. Wait a minute - this is only done for Linux hosts, where we are guaranteed a sane shell (it's pretty much just Solaris where /bin/sh is too puny to do $()) - using $() instead of `` would solve a lot of escaping issues.
@@ -993,6 +1034,16 @@ iptablesHandleIpHdr(virBufferPtr buf, } }
+ if (HAS_ENTRY_ITEM(&ipHdr->dataComment)) { + char *cmt = escapeComment(ipHdr->dataComment.u.string); + if (!cmt) + goto err_exit; + virBufferVSprintf(buf, + " -m comment --comment '\\''%s'\\''", + cmt); + VIR_FREE(cmt);
OK, maybe I see why your comment had such a long replacement for ', because you aren't adding any escaping to cmt here. But I still think we can come up with a more elegant solution, by using the intermediate variable. Thanks for forcing me to explain myself - it's an interesting process thinking about this. [Do I even dare mention that at an even higher layer, it might be nicer to avoid /bin/sh in the first place, and instead put effort into my pending virCommand API patches to make it easier to directly invoke all the iptables commands from C?] -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake <eblake@redhat.com> wrote on 09/27/2010 04:52:31 PM:
V2 changes: following Eric's comments: - commands in the script are assigned using cmd='...' rather
On 09/27/2010 12:40 PM, Stefan Berger wrote: than cmd="..."
- invoke commands using eval res=... rather than res=eval ... - rewrote function escaping ` and single quotes (which became more tricky)
In this patch I am extending the rule instantiator to create the
comment
node where supported, which is the case for iptables and ip6tables.
Since commands are written in the format
cmd='iptables ...-m comment --comment \"\" '
certain characters ('`) in the comment need to be escaped to prevent comments from becoming commands themselves or cause other forms of (bash) substitutions. I have tested this with various input and in my tests the input made it straight into the comment. A test case for TCK will be provided separately that tests this.
At first, I failed to see how ` needs escaping inside '', unless you aren't uniformly using '' like you think you are. Then it hit me - yuck
- we aren't uniformly using '' - we really do have to escape ` inside ``, or come up with an alternate solution. That is:
ret=`iptables -m comment --comment '`'`
is indeed ill-formed. But if you are going to escape `, then you also have to escape \ and $ for the same duration. Yuck again.
I had tested these also and it doesn't seem to be the case. I looked like really only ` and ' needed special treatment.
But fear not - I have a slicker solution:
comment='comment with lone '\'', ", `, \, $x, and two spaces' cmd='iptables ...-m comment --comment "$comment"' eval ret=\`"$cmd"\`
I changed this now for v3.
which at expansion time results in:
eval ret=\`"$cmd"\` ret=`iptables ...-m comment --comment "$comment"` iptables ...-m comment --comment \ 'comment with lone '\'', `, ", `, \, $x, and two spaces'
That is, rather than trying to pass the comment literally through $cmd (and thus having to carefully escape $cmd for its eventual use inside ``), it might be nicer to stick the comment in an intermediate variable (where you only need to escape ') and make $cmd reference the intermediate variable (where you won't have any problematic uses of ", `, or ' to contend with, and where your only $ is one where you intentionally want variable expansion).
It's nicer, true, just a little more changes need for building the final command where a 'prefix', here the comment='' is stuck in front of the line with the iptables command.
@@ -52,10 +53,10 @@
#define CMD_SEPARATOR "\n" -#define CMD_DEF_PRE "cmd=\"" -#define CMD_DEF_POST "\"" +#define CMD_DEF_PRE "cmd='" +#define CMD_DEF_POST "'" #define CMD_DEF(X) CMD_DEF_PRE X CMD_DEF_POST -#define CMD_EXEC "res=`${cmd}`" CMD_SEPARATOR +#define CMD_EXEC "eval res=\\`\"${cmd}\"\\`" CMD_SEPARATOR
This part seems okay - the ` is quoted to protect it from evaluation until after eval has had a chance to collect its arguments.
+static char * +escapeComment(const char *buf) +{ + char *res; + size_t i, j, add = 0, len = strlen(buf); + + static const char SINGLEQUOTE_REPLACEMENT[12] =
"'\\'\\\"\\'\\\"\\''";
That seems rather long to me. Broken into chunks with C-string escaping
eliminated:
' \'\"\'\"\' ' end the current single quoting the 5 literal shell chars '"'"' resume single quoting
I'm not following why we need 5 literal shell characters, instead of 1.
That's because I needed to place the ' in between "". Without it would not work. The first ' was to close the string in front and the final ' was to start another string.
+ + if (len > IPTABLES_MAX_COMMENT_SIZE) + len = IPTABLES_MAX_COMMENT_SIZE; + + for (i = 0; i < len; i++) { + if (buf[i] == '`') + add++; + else if (buf[i] == '\'') + add += sizeof(SINGLEQUOTE_REPLACEMENT); + }
Back to my observation that this doesn't help for \ or $, the other two characters that need escaping inside ``. It would be so much easier if we could rely on $() instead of ``. Wait a minute - this is only done for Linux hosts, where we are guaranteed a sane shell (it's pretty much just Solaris where /bin/sh is too puny to do $()) - using $() instead of
`` would solve a lot of escaping issues.
@@ -993,6 +1034,16 @@ iptablesHandleIpHdr(virBufferPtr buf, } }
+ if (HAS_ENTRY_ITEM(&ipHdr->dataComment)) { + char *cmt = escapeComment(ipHdr->dataComment.u.string); + if (!cmt) + goto err_exit; + virBufferVSprintf(buf, + " -m comment --comment '\\''%s'\\''", + cmt); + VIR_FREE(cmt);
OK, maybe I see why your comment had such a long replacement for ', because you aren't adding any escaping to cmt here. But I still think we can come up with a more elegant solution, by using the intermediate variable. Thanks for forcing me to explain myself - it's an interesting
process thinking about this.
[Do I even dare mention that at an even higher layer, it might be nicer to avoid /bin/sh in the first place, and instead put effort into my pending virCommand API patches to make it easier to directly invoke all the iptables commands from C?]
I do also generate some scripts with 'if .. then' statements that won't be able to executed via your virCommand API... Stefan
Extend the nwfilter.rng schema to accept comment attributes for all protocol types. Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- docs/schemas/nwfilter.rng | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) Index: libvirt-acl/docs/schemas/nwfilter.rng =================================================================== --- libvirt-acl.orig/docs/schemas/nwfilter.rng +++ libvirt-acl/docs/schemas/nwfilter.rng @@ -24,6 +24,7 @@ <ref name="match-attribute"/> <ref name="common-l2-attributes"/> <ref name="mac-attributes"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -33,6 +34,7 @@ <ref name="match-attribute"/> <ref name="common-l2-attributes"/> <ref name="arp-attributes"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -42,6 +44,7 @@ <ref name="match-attribute"/> <ref name="common-l2-attributes"/> <ref name="arp-attributes"/> <!-- same as arp --> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -54,6 +57,7 @@ <ref name="common-port-attributes"/> <ref name="ip-attributes"/> <ref name="dscp-attribute"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -65,6 +69,7 @@ <ref name="common-ipv6-attributes-p1"/> <ref name="common-port-attributes"/> <ref name="ip-attributes"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -76,6 +81,7 @@ <ref name="common-port-attributes"/> <ref name="common-ip-attributes-p1"/> <ref name="common-ip-attributes-p2"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -87,6 +93,7 @@ <ref name="common-port-attributes"/> <ref name="common-ip-attributes-p1"/> <ref name="common-ip-attributes-p2"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -98,6 +105,7 @@ <ref name="common-port-attributes"/> <ref name="common-ip-attributes-p1"/> <ref name="common-ip-attributes-p2"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -109,6 +117,7 @@ <ref name="common-ip-attributes-p1"/> <ref name="common-ip-attributes-p2"/> <ref name="icmp-attributes"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -119,6 +128,7 @@ <ref name="srcmac-attribute"/> <ref name="common-ip-attributes-p1"/> <ref name="common-ip-attributes-p2"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -129,6 +139,7 @@ <ref name="srcmac-attribute"/> <ref name="common-ip-attributes-p1"/> <ref name="common-ip-attributes-p2"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -139,6 +150,7 @@ <ref name="srcmac-attribute"/> <ref name="common-ip-attributes-p1"/> <ref name="common-ip-attributes-p2"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -149,6 +161,7 @@ <ref name="srcmac-attribute"/> <ref name="common-ip-attributes-p1"/> <ref name="common-ip-attributes-p2"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -159,6 +172,7 @@ <ref name="srcmac-attribute"/> <ref name="common-ip-attributes-p1"/> <ref name="common-ip-attributes-p2"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -170,6 +184,7 @@ <ref name="common-port-attributes"/> <ref name="common-ipv6-attributes-p1"/> <ref name="common-ipv6-attributes-p2"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -181,6 +196,7 @@ <ref name="common-port-attributes"/> <ref name="common-ipv6-attributes-p1"/> <ref name="common-ipv6-attributes-p2"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -192,6 +208,7 @@ <ref name="common-port-attributes"/> <ref name="common-ipv6-attributes-p1"/> <ref name="common-ipv6-attributes-p2"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -203,6 +220,7 @@ <ref name="common-ipv6-attributes-p1"/> <ref name="common-ipv6-attributes-p2"/> <ref name="icmp-attributes"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -213,6 +231,7 @@ <ref name="srcmac-attribute"/> <ref name="common-ipv6-attributes-p1"/> <ref name="common-ipv6-attributes-p2"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -223,6 +242,7 @@ <ref name="srcmac-attribute"/> <ref name="common-ipv6-attributes-p1"/> <ref name="common-ipv6-attributes-p2"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -233,6 +253,7 @@ <ref name="srcmac-attribute"/> <ref name="common-ipv6-attributes-p1"/> <ref name="common-ipv6-attributes-p2"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -243,6 +264,7 @@ <ref name="srcmac-attribute"/> <ref name="common-ipv6-attributes-p1"/> <ref name="common-ipv6-attributes-p2"/> + <ref name="comment-attribute"/> </element> </zeroOrMore> </optional> @@ -319,6 +341,16 @@ </interleave> </define> + <define name="comment-attribute"> + <interleave> + <optional> + <attribute name="comment"> + <ref name="comment-type"/> + </attribute> + </optional> + </interleave> + </define> + <define name="srcmac-attribute"> <interleave> <optional> @@ -826,4 +858,10 @@ <param name="pattern">([Ff][Aa][Ll][Ss][Ee]|0)</param> </data> </define> + + <define name='comment-type'> + <data type="string"> + <param name="pattern">[a-zA-Z0-9`~\^!@#$%\-_+=|\\:";,./ \(\)\[\]\{\}"&<>']*</param> + </data> + </define> </grammar>
On 09/27/2010 12:40 PM, Stefan Berger wrote:
Extend the nwfilter.rng schema to accept comment attributes for all protocol types.
Signed-off-by: Stefan Berger <stefanb@us.ibm.com>
+ <define name="comment-attribute"> + <interleave> + <optional> + <attribute name="comment"> + <ref name="comment-type"/> + </attribute> + </optional> + </interleave> + </define>
Maybe I'm not understanding rng, but what is being interleaved here? Do things still validate if "comment-attribute" does not contain an <interleave>?
+ + <define name='comment-type'> + <data type="string"> + <param name="pattern">[a-zA-Z0-9`~\^!@#$%\-_+=|\\:";,./ \(\)\[\]\{\}"&<>']*</param>
Since we are enforcing a maximum comment length of 256, would it make sense to use {0,256} rather than * (or is it \{0,256\} for this flavor of regular expression)? This explicitly leaves out tabs; I guess that's okay. It also leaves out 8-bit bytes - could that be a problem for i18n where people want comments with native-language accented characters? That is, are we being too strict here? Maybe a better pattern would be to reject specific non-printing ASCII bytes we want to avoid, assuing you can use escape sequences like [^\001]? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake <eblake@redhat.com> wrote on 09/27/2010 05:07:47 PM:
On 09/27/2010 12:40 PM, Stefan Berger wrote:
Extend the nwfilter.rng schema to accept comment attributes for all
protocol
types.
Signed-off-by: Stefan Berger <stefanb@us.ibm.com>
+ <define name="comment-attribute"> + <interleave> + <optional> + <attribute name="comment"> + <ref name="comment-type"/> + </attribute> + </optional> + </interleave> + </define>
Maybe I'm not understanding rng, but what is being interleaved here? Do
things still validate if "comment-attribute" does not contain an <interleave>?
It's not necessary from what I can see, so I removed it.
+ + <define name='comment-type'> + <data type="string"> + <param name="pattern">[a-zA-Z0-9`~\^!@#$%\-_+=|\\:";,./ \ (\)\[\]\{\}"&<>']*</param>
Since we are enforcing a maximum comment length of 256, would it make sense to use {0,256} rather than * (or is it \{0,256\} for this flavor of regular expression)? This explicitly leaves out tabs; I guess that's
Correct.
okay. It also leaves out 8-bit bytes - could that be a problem for i18n
where people want comments with native-language accented characters? That is, are we being too strict here? Maybe a better pattern would be to reject specific non-printing ASCII bytes we want to avoid, assuing you can use escape sequences like [^\001]?
Looking at http://www.asciitable.com/ I should probably include 0x20-0x7E and 128-175, 224-238 - maybe even more? So the regex then becomes [ -~€-¯à-î]{0,256} Stefan
On 09/28/2010 04:28 AM, Stefan Berger wrote:
okay. It also leaves out 8-bit bytes - could that be a problem for i18n
where people want comments with native-language accented characters? That is, are we being too strict here? Maybe a better pattern would be to reject specific non-printing ASCII bytes we want to avoid, assuing you can use escape sequences like [^\001]?
Looking at
I should probably include 0x20-0x7E and 128-175, 224-238 - maybe even more? So the regex then becomes
[ -~€-¯à-î]{0,256}
True ASCII is strictly 7-bit; any locale where isprint() returns true on 8-bit bytes is a superset single-byte encoding, such as ISO-8859-1, or 'extended ascii' from the URL you posted above. But I'm also thinking about multi-byte encodings, like UTF-8, where we cannot a priori write a regex that will accept all valid Unicode printable characters, in part because you have to look at more than one byte at a time to determine if you have a printable character. Which goes back to my suggestion of an inverse charset - rejecting bytes that are known to be non-printable ASCII, and letting everything else whether or not it is is a printable byte sequence in the current locale. So what about this idea: exclude control characters except for tab, and let space and everything after through (I don't know if it needs to be adjusted to also reject �): [^- - ]{0,256} -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake <eblake@redhat.com> wrote on 09/28/2010 03:26:48 PM:
[image removed]
Re: [libvirt] [PATCH v2 3/5] Extend nwfilter schema to accept comment attributes
Eric Blake
to:
Stefan Berger
09/28/2010 03:27 PM
Cc:
libvir-list
On 09/28/2010 04:28 AM, Stefan Berger wrote:
okay. It also leaves out 8-bit bytes - could that be a problem for i18n
where people want comments with native-language accented characters? That is, are we being too strict here? Maybe a better pattern would be to reject specific non-printing ASCII bytes we want to avoid, assuing you can use escape sequences like [^\001]?
Looking at
I should probably include 0x20-0x7E and 128-175, 224-238 - maybe even more? So the regex then becomes
[ -~€-¯à-î]{0,256}
True ASCII is strictly 7-bit; any locale where isprint() returns true on
8-bit bytes is a superset single-byte encoding, such as ISO-8859-1, or 'extended ascii' from the URL you posted above. But I'm also thinking about multi-byte encodings, like UTF-8, where we cannot a priori write a
regex that will accept all valid Unicode printable characters, in part because you have to look at more than one byte at a time to determine if
you have a printable character. Which goes back to my suggestion of an inverse charset - rejecting bytes that are known to be non-printable ASCII, and letting everything else whether or not it is is a printable byte sequence in the current locale. So what about this idea: exclude control characters except for tab, and let space and everything after through (I don't know if it needs to be adjusted to also reject �):
[^- - ]{0,256}
Fine by me. We may just give the impression of accepting unicode while the code does not handle it. Stefan
On 09/28/2010 04:06 PM, Stefan Berger wrote:
Eric Blake <eblake@redhat.com> wrote on 09/28/2010 03:26:48 PM:
[image removed]
Re: [libvirt] [PATCH v2 3/5] Extend nwfilter schema to accept comment attributes
Eric Blake
to:
Stefan Berger
09/28/2010 03:27 PM
Cc:
libvir-list
On 09/28/2010 04:28 AM, Stefan Berger wrote:
okay. It also leaves out 8-bit bytes - could that be a problem for i18n
where people want comments with native-language accented characters? That is, are we being too strict here? Maybe a better pattern would be to reject specific non-printing ASCII bytes we want to avoid, assuing you can use escape sequences like [^\001]?
Looking at
I should probably include 0x20-0x7E and 128-175, 224-238 - maybe even more? So the regex then becomes
[ -~€-¯à-î]{0,256}
True ASCII is strictly 7-bit; any locale where isprint() returns true on 8-bit bytes is a superset single-byte encoding, such as ISO-8859-1, or 'extended ascii' from the URL you posted above. But I'm also thinking about multi-byte encodings, like UTF-8, where we cannot a priori write a regex that will accept all valid Unicode printable characters, in part because you have to look at more than one byte at a time to determine if you have a printable character. Which goes back to my suggestion of an inverse charset - rejecting bytes that are known to be non-printable ASCII, and letting everything else whether or not it is is a printable byte sequence in the current locale. So what about this idea: exclude control characters except for tab, and let space and everything after through (I don't know if it needs to be adjusted to also reject �):
[^- - ]{0,256}
Fine by me. We may just give the impression of accepting unicode while the code does not handle it.
... except that xmllint does not like with or without preceding ^ (among other things): xmllint --relaxng ./docs/schemas/nwfilter.rng tests/nwfilterxml2xmlout/comment-test.xml ./docs/schemas/nwfilter.rng:862: parser error : xmlParseCharRef: invalid xmlChar value 1 <param name="pattern">[^- - ]{0,256}</param> ^ ./docs/schemas/nwfilter.rng:862: parser error : CharRef: invalid hexadecimal value <param name="pattern">[^- - ]{0,256}</param> ^ ./docs/schemas/nwfilter.rng:862: parser error : xmlParseCharRef: invalid xmlChar value 0 <param name="pattern">[^- - ]{0,256}</param> ^ ./docs/schemas/nwfilter.rng:862: parser error : CharRef: invalid hexadecimal value <param name="pattern">[^- - ]{0,256}</param> ^ ./docs/schemas/nwfilter.rng:862: parser error : xmlParseCharRef: invalid xmlChar value 0 <param name="pattern">[^- - ]{0,256}</param> ^ ./docs/schemas/nwfilter.rng:862: parser error : CharRef: invalid hexadecimal value <param name="pattern">[^- - ]{0,256}</param> ^ ./docs/schemas/nwfilter.rng:862: parser error : xmlParseCharRef: invalid xmlChar value 0 <param name="pattern">[^- - ]{0,256}</param> Stefan
On Thu, Sep 30, 2010 at 07:31:56AM -0400, Stefan Berger wrote:
On 09/28/2010 04:06 PM, Stefan Berger wrote: [...]
you have a printable character. Which goes back to my suggestion of an inverse charset - rejecting bytes that are known to be non-printable ASCII, and letting everything else whether or not it is is a printable byte sequence in the current locale. So what about this idea: exclude control characters except for tab, and let space and everything after through (I don't know if it needs to be adjusted to also reject �):
[^- - ]{0,256}
Fine by me. We may just give the impression of accepting unicode while the code does not handle it.
... except that xmllint does not like with or without preceding ^ (among other things):
xmllint --relaxng ./docs/schemas/nwfilter.rng tests/nwfilterxml2xmlout/comment-test.xml ./docs/schemas/nwfilter.rng:862: parser error : xmlParseCharRef: invalid xmlChar value 1 <param name="pattern">[^- - ]{0,256}</param> ^
The set of characters allowed in XML documents are defined by the XML specification: http://www.w3.org/TR/REC-xml/#NT-Char It's a subset of Unicode, it refuses 0 et most ASCII control chars. If comments are really comments embbeded in XML I don't see why you should try to limit the comment content beside what XML imposes. On the otehr hand if the comments are exposed somewhere else, we need to look at the conversion issues, while limiting to ASCII is safe, it can be really inconvenient in practice. Do you really need to put any extra restriction ? Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
I am adding a row with information about the newly supported comment attribute to each of the tables describing supported attributes of protocols. Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- docs/formatnwfilter.html.in | 51 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) Index: libvirt-acl/docs/formatnwfilter.html.in =================================================================== --- libvirt-acl.orig/docs/formatnwfilter.html.in +++ libvirt-acl/docs/formatnwfilter.html.in @@ -388,6 +388,11 @@ <td>UINT16 (0x600-0xffff), STRING</td> <td>Layer 3 protocol ID</td> </tr> + <tr> + <td>comment</td> + <td>STRING</td> + <td>text with max. 256 characters</td> + </tr> </table> <p> Valid Strings for <code>protocolid</code> are: arp, rarp, ipv4, ipv6 @@ -466,6 +471,11 @@ <td>IP_ADDR</td> <td>Destination IP address in ARP/RARP packet</td> </tr> + <tr> + <td>comment</td> + <td>STRING</td> + <td>text with max. 256 characters</td> + </tr> </table> <p> Valid strings for the <code>Opcode</code> field are: @@ -551,6 +561,11 @@ <td>UINT16</td> <td>End of range of valid destination ports; requires <code>protocol</code></td> </tr> + <tr> + <td>comment</td> + <td>STRING</td> + <td>text with max. 256 characters</td> + </tr> </table> <p> Valid strings for <code>protocol</code> are: @@ -636,6 +651,11 @@ <td>UINT16</td> <td>End of range of valid destination ports; requires <code>protocol</code></td> </tr> + <tr> + <td>comment</td> + <td>STRING</td> + <td>text with max. 256 characters</td> + </tr> </table> <p> Valid strings for <code>protocol</code> are: @@ -723,6 +743,11 @@ <td>UINT16</td> <td>End of range of valid destination ports</td> </tr> + <tr> + <td>comment</td> + <td>STRING</td> + <td>text with max. 256 characters</td> + </tr> </table> <p> <br><br> @@ -813,6 +838,11 @@ <td>UINT16</td> <td>ICMP code</td> </tr> + <tr> + <td>comment</td> + <td>STRING</td> + <td>text with max. 256 characters</td> + </tr> </table> <p> <br><br> @@ -892,6 +922,11 @@ <td>IP_ADDR</td> <td>End of range of destination IP address</td> </tr> + <tr> + <td>comment</td> + <td>STRING</td> + <td>text with max. 256 characters</td> + </tr> </table> <p> <br><br> @@ -978,6 +1013,11 @@ <td>UINT16</td> <td>End of range of valid destination ports</td> </tr> + <tr> + <td>comment</td> + <td>STRING</td> + <td>text with max. 256 characters</td> + </tr> </table> <p> <br><br> @@ -1054,6 +1094,11 @@ <td>UINT16</td> <td>ICMPv6 code</td> </tr> + <tr> + <td>comment</td> + <td>STRING</td> + <td>text with max. 256 characters</td> + </tr> </table> <p> <br><br> @@ -1118,7 +1163,11 @@ <td>IPV6_ADDR</td> <td>End of range of destination IP address</td> </tr> - + <tr> + <td>comment</td> + <td>STRING</td> + <td>text with max. 256 characters</td> + </tr> </table> <p> <br><br>
On 09/27/2010 12:40 PM, Stefan Berger wrote:
I am adding a row with information about the newly supported comment attribute to each of the tables describing supported attributes of protocols.
Signed-off-by: Stefan Berger <stefanb@us.ibm.com>
--- docs/formatnwfilter.html.in | 51 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-)
Index: libvirt-acl/docs/formatnwfilter.html.in =================================================================== --- libvirt-acl.orig/docs/formatnwfilter.html.in +++ libvirt-acl/docs/formatnwfilter.html.in @@ -388,6 +388,11 @@ <td>UINT16 (0x600-0xffff), STRING</td> <td>Layer 3 protocol ID</td> </tr> + <tr> + <td>comment</td> + <td>STRING</td> + <td>text with max. 256 characters</td> + </tr>
We be using <span class="since">Since 0.8.5</span> on all of these, to distinguish when this element was added to the XML. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake <eblake@redhat.com> wrote on 09/27/2010 05:13:47 PM:
libvir-list
On 09/27/2010 12:40 PM, Stefan Berger wrote:
I am adding a row with information about the newly supported comment attribute to each of the tables describing supported attributes ofprotocols.
Signed-off-by: Stefan Berger <stefanb@us.ibm.com>
--- docs/formatnwfilter.html.in | 51 ++++++++++++++++++++++++++++++ +++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-)
Index: libvirt-acl/docs/formatnwfilter.html.in =================================================================== --- libvirt-acl.orig/docs/formatnwfilter.html.in +++ libvirt-acl/docs/formatnwfilter.html.in @@ -388,6 +388,11 @@ <td>UINT16 (0x600-0xffff), STRING</td> <td>Layer 3 protocol ID</td> </tr> + <tr> + <td>comment</td> + <td>STRING</td> + <td>text with max. 256 characters</td> + </tr>
We be using <span class="since">Since 0.8.5</span> on all of these, to distinguish when this element was added to the XML.
I'll add it to 'comment'. Stefan
This patch adds a test case for testing the XML parser's and instantiator's support of the comment attribute. Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- tests/nwfilterxml2xmlin/comment-test.xml | 68 ++++++++++++++++++++++++++++++ tests/nwfilterxml2xmlout/comment-test.xml | 24 ++++++++++ tests/nwfilterxml2xmltest.c | 2 3 files changed, 94 insertions(+) Index: libvirt-acl/tests/nwfilterxml2xmltest.c =================================================================== --- libvirt-acl.orig/tests/nwfilterxml2xmltest.c +++ libvirt-acl/tests/nwfilterxml2xmltest.c @@ -124,6 +124,8 @@ mymain(int argc, char **argv) DO_TEST("hex-data-test"); + DO_TEST("comment-test"); + return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); } Index: libvirt-acl/tests/nwfilterxml2xmlin/comment-test.xml =================================================================== --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlin/comment-test.xml @@ -0,0 +1,68 @@ +<filter name='testcase'> + <uuid>01a992d2-f8c8-7c27-f69b-ab0a9d377379</uuid> + + <rule action='accept' direction='in'> + <mac protocolid='0x1234' comment='mac rule'/> + </rule> + + <rule action='accept' direction='out'> + <ip srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:ff' + dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:ff' + srcipaddr='10.1.2.3' srcipmask='255.255.255.255' + dstipaddr='10.1.2.3' dstipmask='255.255.255.255' + protocol='udp' + srcportstart='0x123' srcportend='0x234' + dstportstart='0x3456' dstportend='0x4567' + dscp='0x32' comment='ip rule'/> + </rule> + + <rule action='accept' direction='out'> + <ipv6 srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:fe' + dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:80' + srcipaddr='::10.1.2.3' srcipmask='22' + dstipaddr='::10.1.2.3' + dstipmask='ffff:ffff:ffff:ffff:ffff:ffff:ffff:8000' + protocol='tcp' + srcportstart='0x111' srcportend='400' + dstportstart='0x3333' dstportend='65535' comment='ipv6 rule'/> + </rule> + + <rule action='accept' direction='out'> + <arp srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:ff' + dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:ff' + hwtype='0x12' + protocoltype='0x56' + opcode='Request' + arpsrcmacaddr='1:2:3:4:5:6' + arpdstmacaddr='a:b:c:d:e:f' + comment='arp rule'/> + </rule> + + <rule action='accept' direction='out'> + <udp srcmacaddr='1:2:3:4:5:6' + dstipaddr='10.1.2.3' dstipmask='255.255.255.255' + dscp='0x22' + srcportstart='0x123' srcportend='400' + dstportstart='0x234' dstportend='0x444' + comment='udp rule'/> + </rule> + + <rule action='accept' direction='in'> + <tcp-ipv6 srcmacaddr='1:2:3:4:5:6' + srcipaddr='a:b:c::' srcipmask='128' + dscp='0x40' + srcportstart='0x20' srcportend='0x21' + dstportstart='0x100' dstportend='0x1111' + comment='tcp/ipv6 rule'/> + </rule> + + <rule action='accept' direction='in'> + <udp-ipv6 srcmacaddr='1:2:3:4:5:6' + srcipaddr='a:b:c::' srcipmask='128' + dscp='0x40' + srcportstart='0x20' srcportend='0x21' + dstportstart='0x100' dstportend='0x1111' + comment='`ls`;${COLUMNS};$(ls);"test";&'3 spaces''/> + </rule> + +</filter> Index: libvirt-acl/tests/nwfilterxml2xmlout/comment-test.xml =================================================================== --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlout/comment-test.xml @@ -0,0 +1,24 @@ +<filter name='testcase' chain='root'> + <uuid>01a992d2-f8c8-7c27-f69b-ab0a9d377379</uuid> + <rule action='accept' direction='in' priority='500'> + <mac protocolid='0x1234' comment='mac rule'/> + </rule> + <rule action='accept' direction='out' priority='500'> + <ip srcmacaddr='01:02:03:04:05:06' srcmacmask='ff:ff:ff:ff:ff:ff' dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:ff' srcipaddr='10.1.2.3' srcipmask='32' dstipaddr='10.1.2.3' dstipmask='32' protocol='udp' srcportstart='0x123' srcportend='0x234' dstportstart='0x3456' dstportend='0x4567' dscp='0x32' comment='ip rule'/> + </rule> + <rule action='accept' direction='out' priority='500'> + <ipv6 srcmacaddr='01:02:03:04:05:06' srcmacmask='ff:ff:ff:ff:ff:fe' dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:80' srcipaddr='::10.1.2.3' srcipmask='22' dstipaddr='::10.1.2.3' dstipmask='113' protocol='tcp' srcportstart='0x111' srcportend='400' dstportstart='0x3333' dstportend='65535' comment='ipv6 rule'/> + </rule> + <rule action='accept' direction='out' priority='500'> + <arp srcmacaddr='01:02:03:04:05:06' srcmacmask='ff:ff:ff:ff:ff:ff' dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:ff' hwtype='0x12' protocoltype='0x56' opcode='Request' arpsrcmacaddr='01:02:03:04:05:06' arpdstmacaddr='0a:0b:0c:0d:0e:0f' comment='arp rule'/> + </rule> + <rule action='accept' direction='out' priority='500'> + <udp srcmacaddr='01:02:03:04:05:06' dstipaddr='10.1.2.3' dstipmask='32' dscp='0x22' srcportstart='0x123' srcportend='400' dstportstart='0x234' dstportend='0x444' comment='udp rule'/> + </rule> + <rule action='accept' direction='in' priority='500'> + <tcp-ipv6 srcmacaddr='01:02:03:04:05:06' srcipaddr='a:b:c::' srcipmask='128' srcportstart='0x20' srcportend='0x21' dstportstart='0x100' dstportend='0x1111' comment='tcp/ipv6 rule'/> + </rule> + <rule action='accept' direction='in' priority='500'> + <udp-ipv6 srcmacaddr='01:02:03:04:05:06' srcipaddr='a:b:c::' srcipmask='128' srcportstart='0x20' srcportend='0x21' dstportstart='0x100' dstportend='0x1111' comment='`ls`;${COLUMNS};$(ls);"test";&'3 spaces''/> + </rule> +</filter>
On 09/27/2010 12:40 PM, Stefan Berger wrote:
+ <rule action='accept' direction='in'> + <udp-ipv6 srcmacaddr='1:2:3:4:5:6' + srcipaddr='a:b:c::' srcipmask='128' + dscp='0x40' + srcportstart='0x20' srcportend='0x21' + dstportstart='0x100' dstportend='0x1111' + comment='`ls`;${COLUMNS};$(ls);"test";&'3 spaces''/>
Should we also be testing \, <, or *? Also, your tests for " and ' are balanced; do unbalanced " or ' expose any problems? But this is indeed a useful patch, given the churn I'm putting you through on the rest of the series :) -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake <eblake@redhat.com> wrote on 09/27/2010 05:17:32 PM:
On 09/27/2010 12:40 PM, Stefan Berger wrote:
+ <rule action='accept' direction='in'> + <udp-ipv6 srcmacaddr='1:2:3:4:5:6' + srcipaddr='a:b:c::' srcipmask='128' + dscp='0x40' + srcportstart='0x20' srcportend='0x21' + dstportstart='0x100' dstportend='0x1111' + comment='`ls`;${COLUMNS};$(ls);"test";&'3 spaces''/>
Should we also be testing \, <, or *? Also, your tests for " and ' are balanced; do unbalanced " or ' expose any problems?
None of them show any problems at the moment but in case the implementation ever got changed I guess it's better to have more automated tests (TCK). I'll add some more test cases.
But this is indeed a useful patch, given the churn I'm putting you through on the rest of the series :)
I hope so :-) Stefan
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (4)
-
Daniel Veillard -
Eric Blake -
Stefan Berger -
Stefan Berger