[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



s/possiblity/possibility/ in the subject

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.

>  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.

> @@ -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.

> +        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"));

> 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?

> 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?

> +    </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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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