2:38 p.m.
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
2:38 p.m.
New subject: [libvirt] [patch 1/5] Extend XMl parser and generator to support comment attribute
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(a)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;
};
2:38 p.m.
New subject: [libvirt] [patch 2/5] Instantiate comments in ip(6)tables rules
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(a)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
3:01 p.m.
New subject: [libvirt] [patch 2/5] Instantiate comments in ip(6)tables rules
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:22 p.m.
New subject: [libvirt] [patch 2/5] Instantiate comments in ip(6)tables rules
Eric Blake <eblake(a)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
5:16 p.m.
New subject: [libvirt] [patch 2/5] Instantiate comments in ip(6)tables rules
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:11 p.m.
New subject: [libvirt] [patch 2/5] Instantiate comments in ip(6)tables rules
Eric Blake <eblake(a)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
2:38 p.m.
New subject: [libvirt] [patch 3/5] Extend nwfilter schema to accept comment attributes
Extend the nwfilter.rng schema to accept comment attributes for all protocol
types.
Signed-off-by: Stefan Berger <stefanb(a)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>
2:38 p.m.
New subject: [libvirt] [patch 4/5] Extend docs with information about the comment attribute
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(a)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>
2:38 p.m.
New subject: [libvirt] [patch 5/5] Add a test case for testing the comment attribute
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(a)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>
5207
days inactive
5208
days old
9 comments
2 participants
participants (2)
-
Eric Blake -
Stefan Berger