[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/6] Introduce possiblity to have an iterator per variable



On 01/10/2012 12:40 PM, Eric Blake wrote:
s/possiblity/possibility/ in the subject

Fixed.

On 12/09/2011 08:07 AM, Stefan Berger wrote:
This patch introduces the capability to use a different iterator per
variable.

The currently supported notation of variables in a filtering rule like

   <rule action='accept' direction='out'>
      <tcp  srcipaddr='$A' srcportstart='$B'/>
   </rule>

processes the two lists 'A' and 'B' in parallel. This means that A and B
must have the same number of 'N' elements and that 'N' rules will be
instantiated (assuming all tuples from A and B are unique).

In this patch we now introduce the assignment of variables to different
iterators. Therefore a rule like

   <rule action='accept' direction='out'>
      <tcp  srcipaddr='$A[ 1]' srcportstart='$B[ 2]'/>
   </rule>

will now create every combination of elements in A with elements in B since
A has been assigned to an iterator with Id '1' and B has been assigned to an
iterator with Id '2', thus processing their value independently.

The first rule has an equivalent notation of

   <rule action='accept' direction='out'>
      <tcp  srcipaddr='$A[ 0]' srcportstart='$B[ 0]'/>
   </rule>

---
  docs/schemas/nwfilter.rng                 |   71 ++------
This needs something in docs/formatnwfilter.html.in as well.


In 6/6.

  src/conf/nwfilter_conf.c                  |   35 ++--
  src/conf/nwfilter_conf.h                  |    6
  src/conf/nwfilter_params.c                |  239 +++++++++++++++++++++++++++---
  src/conf/nwfilter_params.h                |   38 ++++
  src/libvirt_private.syms                  |    1
  src/nwfilter/nwfilter_ebiptables_driver.c |   13 +
  src/nwfilter/nwfilter_gentech_driver.c    |    8 -
  8 files changed, 307 insertions(+), 104 deletions(-)


-    if (VIR_REALLOC_N(nwf->vars, nwf->nvars+1)<  0) {
-        virReportOOMError();
-        return -1;
-    }
-
-    nwf->vars[nwf->nvars] = strdup(var);
-
-    if (!nwf->vars[nwf->nvars]) {
+    if (VIR_REALLOC_N(nwf->varAccess, nwf->nVarAccess + 1)<  0) {
I'm okay with this as-is, but you might want to switch to VIR_EXPAND_N
for slightly easier usage.

Fixed.

@@ -3069,7 +3069,8 @@ virNWFilterRuleDefDetailsFormat(virBuffe
                     goto err_exit;
                 }
              } else if ((flags&  NWFILTER_ENTRY_ITEM_FLAG_HAS_VAR)) {
-                virBufferAsprintf(buf, "$%s", item->var);
+                virBufferAddLit(buf, "$");
I prefer virBufferAddChar(buf, '$'); but under the hood, it's
practically the same amount of work.

Changed.


+        if (parseError) {
+            const char *what = "iterator id";
+            if (dest->accessType == VIR_NWFILTER_VAR_ACCESS_ELEMENT)
+                what = "array index";
"iterator id" and "array index" should probably be marked for
translation, but see my next comment...

+            virNWFilterReportError(VIR_ERR_INVALID_ARG,
+                                   _("Malformatted %s"), what);
That doesn't make life easy for translators (in languages where nouns
can be masculine or feminine, the adjective translation for
"malformatted" may have a different spelling for the two possilibities
for what).  Rather, I'd write:

if (dest->accessType == VIR_NWFILTER_VAR_ACCESS_ELEMENT)
   virNWFilterReportError(VIR_ERR_INVALID_ARG,
      _("Malformatted array index"));
else
   virNWFilterReportError(VIR_ERR_INVALID_ARG,
      _("Malformatted iterator id"));

Fixed. Thanks.


Index: libvirt-iterator/src/conf/nwfilter_params.h
===================================================================
--- libvirt-iterator.orig/src/conf/nwfilter_params.h
+++ libvirt-iterator/src/conf/nwfilter_params.h
@@ -91,6 +91,38 @@ int virNWFilterHashTablePutAll(virNWFilt
  # define VALID_VARVALUE \
    "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_.:"

+enum virNWFilterVarAccessType {
+    VIR_NWFILTER_VAR_ACCESS_ELEMENT = 0,
+    VIR_NWFILTER_VAR_ACCESS_ITERATOR = 1,
+
+    VIR_NWFILTER_VAR_ACCESS_LAST,
+};
+
+typedef struct _virNWFilterVarAccess  virNWFilterVarAccess;
Is the double-space intentional?


No, fixed.

Index: libvirt-iterator/docs/schemas/nwfilter.rng
===================================================================
--- libvirt-iterator.orig/docs/schemas/nwfilter.rng
+++ libvirt-iterator/docs/schemas/nwfilter.rng
@@ -811,12 +811,15 @@
      </choice>
    </define>

+<define name="variable-name-type">
+<data type="string">
+<param name="pattern">$[a-zA-Z0-9_]+(\[[ ]*[ ]?[0-9]+[ ]*\])?</param>
Does this need to be \$ instead of $, to avoid the meta-meaning of $ as EOL?


It has to be just $. With \$ the test cases don't pass anymore.


+</data>
+</define>
+
    <define name="addrMAC">
      <choice>
-<!-- variable -->
-<data type="string">
-<param name="pattern">$[a-zA-Z0-9_]+</param>
Then again, you were previously using it unquoted, so maybe this is a
latent bug.  My guess is that we need to add some sample files somewhere
under tests/*data/*.xml, and ensure that they get schema-tested as part
of 'make check', to prove that our schema makes sense.  A quick
  git grep 'srcmacaddr=.\$' tests
didn't turn up any existing xml files that would exercise this part of
the rng file.


    Stefan



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]