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

[Stefan Berger]

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