1:40 p.m.
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
1:40 p.m.
New subject: [libvirt] [PATCH v2 1/5] Extend XML parser and generator to support comment attribute
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(a)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;
};
3:02 p.m.
New subject: [libvirt] [PATCH v2 1/5] Extend XML parser and generator to support comment attribute
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:26 p.m.
New subject: [libvirt] [PATCH v2 1/5] Extend XML parser and generator to support comment attribute
Eric Blake <eblake(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
2:19 p.m.
New subject: [libvirt] [PATCH v2 1/5] Extend XML parser and generator to support comment attribute
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
2:59 p.m.
New subject: [libvirt] [PATCH v2 1/5] Extend XML parser and generator to support comment attribute
Eric Blake <eblake(a)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
1:40 p.m.
New subject: [libvirt] [PATCH v2 2/5] Instantiate comments in ip(6)tables rules
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(a)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
3:52 p.m.
New subject: [libvirt] [PATCH v2 2/5] Instantiate comments in ip(6)tables rules
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
7:32 p.m.
New subject: [libvirt] [PATCH v2 2/5] Instantiate comments in ip(6)tables rules
Eric Blake <eblake(a)redhat.com> wrote on 09/27/2010 04:52:31 PM:
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.
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
1:40 p.m.
New subject: [libvirt] [PATCH v2 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>
4:07 p.m.
New subject: [libvirt] [PATCH v2 3/5] Extend nwfilter schema to accept comment attributes
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(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5:28 a.m.
New subject: [libvirt] [PATCH v2 3/5] Extend nwfilter schema to accept comment attributes
Eric Blake <eblake(a)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(a)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
2:26 p.m.
New subject: [libvirt] [PATCH v2 3/5] Extend nwfilter schema to accept comment attributes
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
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}
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:06 p.m.
New subject: [libvirt] [PATCH v2 3/5] Extend nwfilter schema to accept comment attributes
Eric Blake <eblake(a)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
>
> 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}
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
6:31 a.m.
New subject: [libvirt] [PATCH v2 3/5] Extend nwfilter schema to accept comment attributes
On 09/28/2010 04:06 PM, Stefan Berger wrote:
Eric Blake <eblake(a)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
> >
> > 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}
>
> 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
9:30 a.m.
New subject: [libvirt] [PATCH v2 3/5] Extend nwfilter schema to accept comment attributes
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
1:40 p.m.
New subject: [libvirt] [PATCH v2 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>
4:13 p.m.
New subject: [libvirt] [PATCH v2 4/5] Extend docs with information about the comment attribute
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(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>
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
7:45 p.m.
New subject: [libvirt] [PATCH v2 4/5] Extend docs with information about the comment attribute
Eric Blake <eblake(a)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(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>
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
1:40 p.m.
New subject: [libvirt] [PATCH v2 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";&'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>
4:17 p.m.
New subject: [libvirt] [PATCH v2 5/5] Add a test case for testing the comment attribute
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
7:48 p.m.
New subject: [libvirt] [PATCH v2 5/5] Add a test case for testing the comment attribute
Eric Blake <eblake(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5162
days inactive
5172
days old
21 comments
4 participants
participants (4)
-
Daniel Veillard -
Eric Blake -
Stefan Berger -
Stefan Berger