[libvirt] [patch 0/5] nwfilter: Support comment attribute in filter rule descriptions
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
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 | 101 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/nwfilter_conf.h | 10 ++++ 2 files changed, 110 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 @@ -38,6 +38,7 @@ #include "internal.h" #include "uuid.h" +#include "c-ctype.h" #include "memory.h" #include "virterror_internal.h" #include "datatypes.h" @@ -181,6 +182,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 +249,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 +357,28 @@ virNWFilterRuleDefAddVar(virNWFilterRule } +static char * +virNWFilterRuleDefAddString(virNWFilterRuleDefPtr nwf, + const char *string) +{ + if (VIR_REALLOC_N(nwf->strings, nwf->nstrings+1) < 0) { + virReportOOMError(); + return NULL; + } + + nwf->strings[nwf->nstrings] = strdup(string); + + if (!nwf->strings[nwf->nstrings]) { + virReportOOMError(); + return NULL; + } + + nwf->nstrings++; + + return nwf->strings[nwf->nstrings-1]; +} + + void virNWFilterPoolObjRemove(virNWFilterPoolObjListPtr pools, virNWFilterPoolObjPtr pool) @@ -632,6 +661,25 @@ dscpValidator(enum attrDatatype datatype return 1; } + +static bool +commentValidator(enum attrDatatype datatype ATTRIBUTE_UNUSED, union data *val, + virNWFilterRuleDefPtr nwf ATTRIBUTE_UNUSED) +{ + int i = 0; + char c; + + while ((c = val->c[i++]) != 0) { + if (!c_isgraph(c) && c != ' ') + return 0; + } + + if (i > MAX_COMMENT_LENGTH + 1) + val->c[MAX_COMMENT_LENGTH] = 0; + + return 1; +} + #define COMMON_MAC_PROPS(STRUCT) \ {\ .name = SRCMACADDR,\ @@ -655,6 +703,23 @@ dscpValidator(enum attrDatatype datatype } +#define COMMENT_PROP(STRUCT) \ + {\ + .name = "comment",\ + .datatype = DATATYPE_STRINGCOPY,\ + .dataIdx = offsetof(virNWFilterRuleDef, p.STRUCT.dataComment),\ + .validator = commentValidator,\ + } + +#define COMMENT_PROP_IPHDR(STRUCT) \ + {\ + .name = "comment",\ + .datatype = DATATYPE_STRINGCOPY,\ + .dataIdx = offsetof(virNWFilterRuleDef, p.STRUCT.ipHdr.dataComment),\ + .validator = commentValidator,\ + } + + static const virXMLAttr2Struct macAttributes[] = { COMMON_MAC_PROPS(ethHdrFilter), { @@ -664,6 +729,7 @@ static const virXMLAttr2Struct macAttrib .validator= checkMacProtocolID, .formatter= macProtocolIDFormatter, }, + COMMENT_PROP(ethHdrFilter), { .name = NULL, } @@ -702,6 +768,7 @@ static const virXMLAttr2Struct arpAttrib .datatype = DATATYPE_IPADDR, .dataIdx = offsetof(virNWFilterRuleDef, p.arpHdrFilter.dataARPDstIPAddr), }, + COMMENT_PROP(arpHdrFilter), { .name = NULL, } @@ -762,6 +829,7 @@ static const virXMLAttr2Struct ipAttribu .dataIdx = offsetof(virNWFilterRuleDef, p.ipHdrFilter.ipHdr.dataDSCP), .validator = dscpValidator, }, + COMMENT_PROP_IPHDR(ipHdrFilter), { .name = NULL, } @@ -817,6 +885,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 +983,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 +992,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 +1000,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 +1008,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 +1016,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 +1025,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 +1044,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 +1053,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 +1062,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 +1077,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 +1086,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 +1095,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 +1104,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 +1113,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 +1123,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 +1142,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 +1151,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 +1418,17 @@ virNWFilterRuleDetailsParse(xmlNodePtr n found = 1; break; + case DATATYPE_STRINGCOPY: + if (!validator || + !(item->u.string = + virNWFilterRuleDefAddString(nwf, prop))) { + rc = -1; + break; + } + data.c = item->u.string; + found = 1; + break; + case DATATYPE_LAST: default: break; @@ -2510,6 +2607,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; };
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 (twice) 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. To prevent consecutive spaces in comments from becoming a single space (by bash), the IFS variable is now set to an empty string. Also, commands are now executed using bash's 'eval' command. Since the regular strchr() function causes a compiler warning when neither one of the parameters is a constant, I reimplemented this function and called it _strchr(). Here is the reference to this bug http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36513 Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- src/nwfilter/nwfilter_ebiptables_driver.c | 68 +++++++++++++++++++++++++++++- src/nwfilter/nwfilter_ebiptables_driver.h | 3 + 2 files changed, 69 insertions(+), 2 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 @@ -55,7 +55,7 @@ #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 "res=`eval ${cmd}`" CMD_SEPARATOR #define CMD_STOPONERR(X) \ X ? "if [ $? -ne 0 ]; then" \ " echo \"Failure to execute command '${cmd}'.\";" \ @@ -291,6 +291,58 @@ printDataTypeAsHex(virNWFilterHashTableP } +/* avoiding a compiler warning trough own implementation */ +static const char * +_strchr(const char *s, int c) +{ + while (*s && *s != (char)c) + s++; + if (*s) + return s; + return NULL; +} + + +static char * +shellEscapeString(const char *data, size_t maxlen, + const char *s, bool doubleEscape) +{ + char *res; + size_t i, j, add = 0, len = strlen(data); + + if (len > maxlen) + len = maxlen; + + for (i = 0; i < len; i++) { + if (_strchr(s, data[i])) { + add += 1; + if (doubleEscape) + add += 2; + } + } + + if (VIR_ALLOC_VAR(res, char, len+add+1) < 0) { + virReportOOMError(); + return NULL; + } + + j = 0; + for (i = 0; i < len; i++) { + if (_strchr(s, data[i])) { + res[j++] = '\\'; + if (doubleEscape) { + res[j++] = '\\'; + res[j++] = '\\'; + } + } + res[j++] = data[i]; + } + res[j] = 0; + + return res; +} + + static void ebiptablesRuleInstFree(ebiptablesRuleInstPtr inst) { @@ -993,6 +1045,18 @@ iptablesHandleIpHdr(virBufferPtr buf, } } + if (HAS_ENTRY_ITEM(&ipHdr->dataComment)) { + char *cmt = shellEscapeString(ipHdr->dataComment.u.string, + IPTABLES_MAX_COMMENT_SIZE, + "\"`\\$", true); + if (!cmt) + goto err_exit; + virBufferVSprintf(buf, + " -m comment --comment \\\"%s\\\"", + cmt); + VIR_FREE(cmt); + } + return 0; err_exit: @@ -2211,7 +2275,7 @@ ebiptablesWriteToTempFile(const char *st char *header; size_t written; - virBufferVSprintf(&buf, "#!%s\n", bash_cmd_path); + virBufferVSprintf(&buf, "#!%s\nIFS=""\n", bash_cmd_path); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); 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,7 @@ extern virNWFilterTechDriver ebiptables_ # define EBIPTABLES_DRIVER_ID "ebiptables" + +# define IPTABLES_MAX_COMMENT_SIZE 256 + #endif
On 09/24/2010 01:38 PM, Stefan Berger wrote:
To prevent consecutive spaces in comments from becoming a single space (by bash), the IFS variable is now set to an empty string. Also, commands are now executed using bash's 'eval' command.
-#define CMD_EXEC "res=`${cmd}`" CMD_SEPARATOR +#define CMD_EXEC "res=`eval ${cmd}`" CMD_SEPARATOR Underquoted. To be robust, this needs to be: "res=`eval \"$cmd\"" CMD_SEPARATOR which will then avoid your need for the empty IFS hack and double escaping (you'll still need single escaping, but that's a bit more manageable).
+/* avoiding a compiler warning trough own implementation */ +static const char * +_strchr(const char *s, int c) +{ + while (*s && *s != (char)c) + s++; + if (*s) + return s; + return NULL; +}
Ouch. That's probably 4x slower than the glibc version. I'd much rather see: #undef strchr or (strchr)(a, b) which then guarantees that you get the function call, rather than the macro expansion; after all, the macro expansion just defers to the function call if both arguments are non-constants, not to mention the fact that the -Wlogical-op warning is only triggered by the macro and not by the function. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake <eblake@redhat.com> wrote on 09/24/2010 04:01:55 PM:
libvir-list
On 09/24/2010 01:38 PM, Stefan Berger wrote:
To prevent consecutive spaces in comments from becoming a single space (by bash), the IFS variable is now set to an empty string. Also, commands are now executed using bash's 'eval' command.
-#define CMD_EXEC "res=`${cmd}`" CMD_SEPARATOR +#define CMD_EXEC "res=`eval ${cmd}`" CMD_SEPARATOR
Underquoted. To be robust, this needs to be:
"res=`eval \"$cmd\"" CMD_SEPARATOR
Ok, I made this change.
which will then avoid your need for the empty IFS hack and double escaping (you'll still need single escaping, but that's a bit more manageable).
I just tried the TCK test without and with double-escaping in libvirtd and double-escaping does seem to be necessary otherwise `ls` and $(ls) do get executed and their results end up in the comment. The spaces are preserved, though, so I can revert the change to IFS.
+/* avoiding a compiler warning trough own implementation */ +static const char * +_strchr(const char *s, int c) +{ + while (*s && *s != (char)c) + s++; + if (*s) + return s; + return NULL; +}
Ouch. That's probably 4x slower than the glibc version. I'd much rather see:
#undef strchr
Yes, that does the trick. Thanks.
or
(strchr)(a, b)
which then guarantees that you get the function call, rather than the macro expansion; after all, the macro expansion just defers to the function call if both arguments are non-constants, not to mention the fact that the -Wlogical-op warning is only triggered by the macro and not by the function.
Stefan
On 09/24/2010 02:22 PM, Stefan Berger wrote:
I just tried the TCK test without and with double-escaping in libvirtd and double-escaping does seem to be necessary otherwise `ls` and $(ls) do get executed and their results end up in the comment. The spaces are preserved, though, so I can revert the change to IFS.
Hmm.
"res=`eval \"$cmd\"" CMD_SEPARATOR
+ virBufferVSprintf(buf, + " -m comment --comment \\\"%s\\\"", + cmt);
Thinking about it more: Let's see why double-escaping helped you, by setting up a user comment with two spaces and a $. You ultimately want to call: iptables -m comment --comment 'user $comment' but you used a "" style, resulting in: iptables -m comment --comment "user \$comment" Then, you are capturing that command in a shell variable $cmd (to avoid repetition when displaying a failed command) and the output in $res. So you need one level of escaping for assigning to cmd within "" context: cmd="iptables -m comment --comment \"user \\\$comment\"" res=`$cmd` Oops, that failed, because it splits $cmd at IFS boundaries, which breaks up the user comment because it looks roughly like res=`'iptables' '-m' 'comment' '--comment' '"user' '\$comment"'` But quoting $cmd isn't right either, because then you try to execute a single command with no arguments: res=`"$cmd"` res=`'iptables -m comment --comment "user \$comment"'` So you _do_ need eval to control where the word boundaries are, which means you will be removing a level of quoting from the contents of $cmd, and you also need quoting when assigning to cmd, so I now see why double escaping is needed. Continuing on with your original example, your underquoted version with IFS cleared to avoid field splitting of $cmd looks like: IFS= cmd="iptables -m comment --comment \"user \\\$comment\"" res=`eval $cmd` res=`eval 'iptables -m comment --comment "user \$comment"'` res=`iptables -m comment --comment "user $comment"` oops - here, `` turned \$ into $ before the eval got run, which means eval will expand the (empty variable) $comment. Even with two levels of quoting, you risk problems when mixing "" and ``. My suggestion is to assign cmd using '' rather than "" (fewer things to quote), as well as moving the eval outside of the `` (so it becomes obvious which \ are interpreted by eval rather than by ``: cmd='iptables -m comment --comment '\''user $comment'\'' eval res=\`"$cmd"\` res=`iptables -m comment --comment 'user $comment'` And the nice part of that is the implementation: virBufferVSprintf(buf, " -m comment --comment '%s'", escapeSingleQuotes(user_comment)); virBufferVSprintf(cmd, "cmd='%s'\nres=\\`\"$cmd\"\\`", escapeSingleQuotes(buf)); still results in the needed double quoting (one for assignment to cmd, and one to be stripped by eval), but via two passes of a single function that only has to escape all instances of ', rather than a function that has to add escaping for four different bytes and pay attention to how many escape levels must be added.
Ouch. That's probably 4x slower than the glibc version. I'd much rather see:
#undef strchr
Yes, that does the trick. Thanks.
or
(strchr)(a, b)
On further thought, gnulib might be doing: #define strchr rpl_strchr on platforms where strchr is broken, so using #undef strchr is too risky. So I'd recommend sticking with (strchr)(a, b), which still works if gnulib has to replace a broken strchr. (It's surprising how easy it is to have bugs in such a low-level function - until just this month, glibc 2.12 on Alpha hardware had a bug in memchr that gnulib has to work around). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake <eblake@redhat.com> wrote on 09/24/2010 06:16:35 PM:
On 09/24/2010 02:22 PM, Stefan Berger wrote:
I just tried the TCK test without and with double-escaping in libvirtd and double-escaping does seem to be necessary otherwise `ls` and $(ls) do get executed and their results end up in the comment. The spaces are preserved, though, so I can revert the change to IFS.
Hmm.
"res=`eval \"$cmd\"" CMD_SEPARATOR
+ virBufferVSprintf(buf, + " -m comment --comment \\\"%s\\\"", + cmt);
Thinking about it more:
[...]
My suggestion is to assign cmd using '' rather than "" (fewer things to quote), as well as moving the eval outside of the `` (so it becomes obvious which \ are interpreted by eval rather than by ``:
cmd='iptables -m comment --comment '\''user $comment'\'' eval res=\`"$cmd"\` res=`iptables -m comment --comment 'user $comment'`
And the nice part of that is the implementation:
virBufferVSprintf(buf, " -m comment --comment '%s'", escapeSingleQuotes(user_comment));
virBufferVSprintf(cmd, "cmd='%s'\nres=\\`\"$cmd\"\\`", escapeSingleQuotes(buf));
Also I followed this. I had to write it like this here to reflect what you wrote further above: virBufferVSprintf(buf, " -m comment --comment '\''%s'\''", shellEscapeString(user_comment)); and shellEscapeString() needs to escape ' as well as `, otherwise I can execute commands. Thanks for the help.
On further thought, gnulib might be doing:
#define strchr rpl_strchr
on platforms where strchr is broken, so using #undef strchr is too risky. So I'd recommend sticking with (strchr)(a, b), which still works
if gnulib has to replace a broken strchr.
Ok. This also works. Wished they left a note in the man pages about this... 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>
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>
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";&'/> + </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";&'/> + </rule> +</filter>
participants (2)
-
Eric Blake -
Stefan Berger