[libvirt] [PATCH V4 04/10] Use scripting for cleaning and renaming of chains
Use scripts for the renaming and cleaning up of chains. This allows us to get rid of some of the code that is only capable of renaming and removing chains whose names are hardcoded. A shell function 'collect_chains' is introduced that is given the name of an ebtables chain and then recursively determines the names of all chanins that are accessed from this chain and its sub-chains using 'jumps'. The resulting list of chain names is then used to delete all the found chains by first flushing and then deleting them. The same function is also used for renaming temporary filters to their final names. I tested this with the bash and dash as script interpreters. v2: - setting IFS to intended value; works with bash and dash (phew!) Signed-off-by: Stefan Berger --- src/nwfilter/nwfilter_ebiptables_driver.c | 204 +- 1 file changed, 117 insertions(+), 87 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 @@ -91,6 +91,49 @@ static char *gawk_cmd_path; #define PRINT_CHAIN(buf, prefix, ifname, suffix) \ snprintf(buf, sizeof(buf), "%c-%s-%s", prefix, ifname, suffix) +static const char ebtables_script_func_collect_chains[] = +"collect_chains()\n" +"{\n" +" local sc\n" +" sc=$(%s -t %s -L $1 | \\\n" +" sed -n \"/Bridge chain*/,$ s/.*\\-j \\([%s]-.*\\)/\\1/p\")\n" +" for tmp in `echo \"$sc\"`; do\n" +"[ -n \"$tmp\" ] && sc=\"$sc $(collect_chains $tmp)\"\n" +" done\n" +" echo \"$sc\"\n" +"}\n"; + +static const char ebiptables_script_func_rm_chains[] = +"rm_chains()\n" +"{\n" +" for tmp in `echo \"$1\"`; do [ -n \"$tmp\" ] && %s -t %s -F $tmp; done\n" +" for tmp in `echo \"$1\"`; do [ -n \"$tmp\" ] && %s -t %s -X $tmp; done\n" +"}\n"; + +static const char ebiptables_script_func_rename_chains[] = +"rename_chains()\n" +"{\n" +" for tmp in `echo \"$1\"`; do\n" +"if [ -n \"$tmp\" ]; then\n" +" tmp2=`expr substr \"$tmp\" 1 1`\n" +" if [ $tmp2 = \"%c\" ]; then\n" +" %s -t %s -E \"$tmp\" \"%c\"`expr substr \"$tmp\" 2 33`\n" +" elif [ $tmp2 = \"%c\" ]; then\n" +" %s -t %s -E \"$tmp\" \"%c\"`expr substr \"$tmp\" 2 33`\n" +" fi\n" +"fi\n" +" done\n" +"}\n"; + +static const char ebiptables_script_set_ifs[] = +"IFS=$' \\t\\n'\n" +"[ `expr length \"$IFS\"` != 3 ] && " +"IFS=$(echo | %s '{ print \"\\t\\n \"}')\n"; + +#define NWFILTER_FUNC_COLLECT_CHAINS ebtables_script_func_collect_chains +#define NWFILTER_FUNC_DELETE_CHAINS ebiptables_script_func_rm_chains +#define NWFILTER_FUNC_RENAME_CHAINS ebiptables_script_func_rename_chains +#define NWFILTER_FUNC_SET_IFS ebiptables_script_set_ifs #define VIRT_IN_CHAIN "libvirt-in" #define VIRT_OUT_CHAIN "libvirt-out" @@ -2805,95 +2848,65 @@ ebtablesCreateTmpSubChain(virBufferPtr b return 0; } - -static int -_ebtablesRemoveSubChain(virBufferPtr buf, -int incoming, -const char *ifname, -enum l3_proto_idx protoidx, -int isTempChain) +static int _ebtablesRemoveSubChains(virBufferPtr buf, +const char *ifname, +const char *chains) { -char rootchain[MAX_CHAINNAME_LENGTH], chain[MAX_CHAINNAME_LENGTH]; -char chainPrefix; - -if (isTempChain) { -chainPrefix =(incoming) ? CHAINPREFIX_HOST_IN_TEMP -: CHAINPREFIX_HOST_OUT_TEMP; -} else { -chainPrefix =(incoming) ? CHAINPREFIX_HOST_IN -: CHAINPREFIX_HOST_OUT; -} +char rootchain[MAX_CHAINNAME_LENGTH]; +unsigned i; -PRINT_ROOT_CHAIN(rootchain, chainPrefix, ifname); -PRINT_CHAIN(chain, chainPrefix, ifname, l3_protocols[protoidx].val); - -virBufferAsprintf(buf, - "%s -t %s -D %s -p 0x%x -j %s" CMD_SEPARATOR - "%s -t %s -F %s" CMD_SEPARATOR - "%s -t %s -X %s" CMD_SEPARATOR, +virBufferAsprintf(buf, NWFILTER_FUNC_COLLECT_CHAINS, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chains); +virBufferAsprintf(buf, NWFILTER_FUNC_DELETE_CHAINS, ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, - rootchain, l3_protocols[protoidx].attr, chain, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE); - ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chain, +virBufferAsprintf(buf, NWFILTER_FUNC_SET_IFS, gawk_cmd_path); +virBufferAddLit(buf, "a=\""); +for (i = 0; chains[i] != 0; i++) { +PRINT_ROOT_CHAIN(rootchain, chains[i], ifname); +virBufferAsprintf(buf,
Re: [libvirt] [PATCH V4 04/10] Use scripting for cleaning and renaming of chains
On 10/26/2011 05:36 AM, Stefan Berger wrote: > Use scripts for the renaming and cleaning up of chains. This allows us to get > rid of some of the code that is only capable of renaming and removing chains > whose names are hardcoded. Resuming where I left off, in the hopes that this helps before you post v5... > > A shell function 'collect_chains' is introduced that is given the name > of an ebtables chain and then recursively determines the names of all > chanins that are accessed from this chain and its sub-chains using 'jumps'. s/chanins/chains/ > > The resulting list of chain names is then used to delete all the found > chains by first flushing and then deleting them. > > The same function is also used for renaming temporary filters to their final > names. > > I tested this with the bash and dash as script interpreters. Well, there's still the overriding design nag that we want to avoid shell interpretation if at all possible, but that's a _much_ bigger rewrite for another day, so I can live with this current patch (it's not making our use of sh any worse than it already was). > > +static const char ebtables_script_func_collect_chains[] = Reading shell code that has been quoted into a C string literal to be passed through printf is an interesting exercise :) > +"collect_chains()\n" Making sure I understand the rest of this patch, this function is reached in two places by the rest of your patch, with code like: +virBufferAsprintf(buf, "$(collect_chains %s) ", rootchain); thus, you are using one command substitution per rootchain, where this function then outputs words to be parsed as part of a resulting command. That's a lot of processes; would it be worth using a shell for loop instead of a command-substitution per rootchain > +"{\n" > +" local sc\n" 'local' is not portable; POSIX doesn't guarantee it. I suppose we can get away with it since both bash and dash support it, and this code is specific to Linux where we know /bin/sh will be either bash or dash, but it would be even nicer if we could stick to POSIX syntax (which means avoiding 'local' and treating all variables are global, and you have to be careful that variables used inside a function are not likely to cause conflicts with any other variables used globally). > +" sc=$(%s -t %s -L $1 | \\\n" Just so I'm making sure I understand things, this line is paired with ebtables_cmd_path and EBTABLES_DEFAULT_TABLE, which results in something like this in shell ($1 being a rootchain name, fitting the pattern libvirt-%c-%s, so I think you're okay that $1 wasn't quoted): sc=$(/path/to/iptables -t nat -L $1 | ... Can you please include a comment mentioning the typical output from such a command that you intend to be parsing? Technically, you don't need backslash-newline after pipe; a trailing pipe is sufficient to continue a statement to the next line in shell. But it doesn't hurt to leave it in either. > +" sed -n \"/Bridge chain*/,$ s/.*\\-j \\([%s]-.*\\)/\\1/p\")\n" 1 2 3 4 5 1. Did you really mean to match lines with "Bridge chai" or "Bridge chainnn"? That * is probably superfluous. 2. "$ " is not portable shell. To be portable, it should be "\$ " or '$ '. 3. \- is not a portable sed escape. But you don't need escaping for '-', since searching for a literal - in a sed expression works just fine. 4. This %s matches up to the 'chains' argument in this code: +virBufferAsprintf(buf, NWFILTER_FUNC_COLLECT_CHAINS, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chains); which I in turn traced back to: +char chains[3] = { +CHAINPREFIX_HOST_IN_TEMP, // 'J' +CHAINPREFIX_HOST_OUT_TEMP, // 'P' +0}; so it looks like you are taking a line that looks like: Bridge chain ... -j J-... and turning it into: J-... while ignoring all other lines. 5. Use of "\(\)/\1" in shell is unusual (it's well-defined by POSIX that the backslash is preserved literally if the next character is not backquote, dollar, backslash, or newline), but for safety reason, I prefer to write it as "\\(\\)/\\1" to make it obvious that in the shell code I meant a literal backslash rather than relying on POSIX rules. Put all of these together, and this line should probably be: " sed -n \"/Bridge chain/,\\$ s/.*-j ([%s]-.*)/1/p\")\n" > +" for tmp in `echo \"$sc\"`; do\n" I prefer $() over ``. But this echo is a wasted process - no need to echo when you can just use $sc directly: " for tmp in $sc; do\n" > +"[ -n \"$tmp\" ] && sc=\"$sc $(collect_chains $tmp)\"\n" Odd to be assigning to $sc in the middle of a loop based on the contents of $sc, and problematic if we change $sc to be global instead of local. Furthermore, [ -n $tmp ] will always be true (whether with your 'for tmp in `echo "$sc"`' or my 'for tmp in $sc', the point remains that word splitting is done, so each value of $tmp will be non-e
Re: [libvirt] [PATCH V4 04/10] Use scripting for cleaning and renaming of chains
On 11/17/2011 12:33 PM, Eric Blake wrote: On 10/26/2011 05:36 AM, Stefan Berger wrote: The resulting list of chain names is then used to delete all the found chains by first flushing and then deleting them. The same function is also used for renaming temporary filters to their final names. I tested this with the bash and dash as script interpreters. Well, there's still the overriding design nag that we want to avoid shell interpretation if at all possible, but that's a _much_ bigger rewrite for another day, so I can live with this current patch (it's not making our use of sh any worse than it already was). Agree. +static const char ebtables_script_func_collect_chains[] = Reading shell code that has been quoted into a C string literal to be passed through printf is an interesting exercise :) +"collect_chains()\n" Making sure I understand the rest of this patch, this function is reached in two places by the rest of your patch, with code like: +virBufferAsprintf(buf, "$(collect_chains %s) ", rootchain); thus, you are using one command substitution per rootchain, where this function then outputs words to be parsed as part of a resulting command. Yes, so this function starts out with the name of an ebtables chain and tries to determine all dependent chains of it, i.e., 'subchains'. #> ebtables -t nat -L libvirt-I-tck-test205002 Bridge table: nat Bridge chain: libvirt-I-tck-test205002, entries: 5, policy: ACCEPT -p IPv4 -j I-tck-test205002-ipv4 -p ARP -j I-tck-test205002-arp -p 0x8035 -j I-tck-test205002-rarp -p 0x835 -j ACCEPT -j DROP Assuming I have the interface 'tck-test205002', I then run this function to find the chains I-tck-test205002-ipv4, I-tck-test205002-arp and I-tck-test-205002-rarp and then visit each one of those and try to find the chains they are 'jumping into'. That's a lot of processes; would it be worth using a shell for loop instead of a command-substitution per rootchain I followed your suggested code below. [...] sc=$(/path/to/iptables -t nat -L $1 | ... Can you please include a comment mentioning the typical output from such a command that you intend to be parsing? See above (uses ebtables rather than iptables). Technically, you don't need backslash-newline after pipe; a trailing pipe is sufficient to continue a statement to the next line in shell. But it doesn't hurt to leave it in either. +" sed -n \"/Bridge chain*/,$ s/.*\\-j \\([%s]-.*\\)/\\1/p\")\n" 1 2 3 4 5 1. Did you really mean to match lines with "Bridge chai" or "Bridge chainnn"? That * is probably superfluous. No, that was wrong. 2. "$ " is not portable shell. To be portable, it should be "\$ " or '$ '. Didn't know... 3. \- is not a portable sed escape. But you don't need escaping for '-', since searching for a literal - in a sed expression works just fine. 4. This %s matches up to the 'chains' argument in this code: +virBufferAsprintf(buf, NWFILTER_FUNC_COLLECT_CHAINS, + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chains); which I in turn traced back to: +char chains[3] = { +CHAINPREFIX_HOST_IN_TEMP, // 'J' +CHAINPREFIX_HOST_OUT_TEMP, // 'P' +0}; so it looks like you are taking a line that looks like: Bridge chain ... -j J-... and turning it into: J-... while ignoring all other lines. 5. Use of "\(\)/\1" in shell is unusual (it's well-defined by POSIX that the backslash is preserved literally if the next character is not backquote, dollar, backslash, or newline), but for safety reason, I prefer to write it as "\\(\\)/\\1" to make it obvious that in the shell code I meant a literal backslash rather than relying on POSIX rules. Put all of these together, and this line should probably be: " sed -n \"/Bridge chain/,\\$ s/.*-j ([%s]-.*)/1/p\")\n" Thanks. Ah, so the end result is that you echo each name found, as well as recurse on each name found to echo any dependent names. Is order important (all names from the first chain must be output before any names from the recursive calls)? If not, then I can rewrite this function to avoid local sc in the first place. Here, in shell form (I'll leave you to convert it back into C string literal form): collect_chains() { for tmp in $(iptables -t nat -L $1 | \ sed -n "/Bridge chain/,\$ s/.*-j \\([JP]-.*\\)/\\1/p"); do echo $tmp collect_chains $tmp done } I took this 'as-is'. Thanks. + +static const char ebiptables_script_func_rm_chains[] = +"rm_chains()\n" It looks like you are calling rm_chains() with a single argument containing a whitespace separated list of arguments. +virBufferAddLit(buf, "rm_chains \"$a\"\n"); Why not just call rm_chains with multiple arguments? virBufferAddLit(buf, "rm_chains $a\n"); +"{\n" +" for tmp in `echo \"$1\"`; do [ -n \"$tmp\" ]&& %s -t %s -F $tmp; done\n" +" for tmp in `echo \"$1\"`; do [ -n \"$