[libvirt] [PATCH] nwfilter: Use -m conntrack rather than -m state

[John Ferlan]

On 08/06/2013 12:43 PM, Stefan Berger wrote:
> On 08/06/2013 11:20 AM, John Ferlan wrote:
>> On 08/06/2013 09:52 AM, Stefan Berger wrote:
>>> Since iptables version 1.4.16 '-m state --state NEW' is converted to
>>> '-m conntrack --ctstate NEW'. Therefore, when encountering this or later
>>> versions of iptables use '-m conntrack --ctstate'.
>>>
>>> Signed-off-by: Stefan Berger <stefanb at linux.vnet.ibm.com>
>>>
>>> ---
>>>   src/nwfilter/nwfilter_ebiptables_driver.c |   50
>>> +++++++++++++++++++++++++++++-
>>>   1 file changed, 49 insertions(+), 1 deletion(-)
>>>
>>> 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
>>> @@ -188,6 +188,9 @@ static const char ebiptables_script_set_
>>>     static const char *m_state_out_str   = "-m state --state
>>> NEW,ESTABLISHED";
>>>   static const char *m_state_in_str    = "-m state --state ESTABLISHED";
>>> +static const char *m_state_out_str_new = "-m conntrack --ctstate
>>> NEW,ESTABLISHED";
>>> +static const char *m_state_in_str_new  = "-m conntrack --ctstate
>>> ESTABLISHED";
>>> +
>>>   static const char *m_physdev_in_str  = "-m physdev " PHYSDEV_IN;
>>>   static const char *m_physdev_out_str = "-m physdev " PHYSDEV_OUT;
>>>   static const char *m_physdev_out_old_str = "-m physdev "
>>> PHYSDEV_OUT_OLD;
>>> @@ -4353,6 +4356,49 @@ ebiptablesDriverProbeCtdir(void)
>>>           iptables_ctdir_corrected = CTDIR_STATUS_OLD;
>>>   }
>>>   +static void
>>> +ebiptablesDriverProbeStateMatch(void)
>>> +{
>>> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
>>> +    char *cmdout = NULL, *version;
>>> +    unsigned long thisversion;
>>> +
>>> +    NWFILTER_SET_IPTABLES_SHELLVAR(&buf);
>>> +
>>> +    virBufferAsprintf(&buf,
>>> +                      "$IPT --version");
>>> +
>>> +    if (ebiptablesExecCLI(&buf, NULL, &cmdout) < 0) {
>>> +        VIR_ERROR(_("Testing of iptables command failed: %s"),
>>> +                  cmdout);
>>> +        return;
>> Probably should just goto cleanup since we'll need to free buf
> 
> ebiptablesExecCLI already takes care of freeing the buffer.
> 
>>
>>> +    }
>>> +
>>> +    /*
>>> +     * we expect output in the format
>>> +     * iptables v1.4.16
>>> +     */
>>> +    if (!(version = strchr(cmdout, 'v')) &&
>>> +        virParseVersionString(version + 1, &thisversion, true) < 0) {
>>> +        VIR_ERROR(_("Could not determine iptables version from
>>> string %s"),
>>> +                  cmdout);
>>> +        goto cleanup;
>>> +    }
>>> +
>>> +    /*
>>> +     * since version 1.4.16 '-m state --state ...' will be converted to
>>> +     * '-m conntrack --ctstate ...'
>>> +     */
>>> +    if (thisversion > 1 * 1000000 + 4 * 1000 + 16) {
>>> +        m_state_out_str = m_state_out_str_new;
>>> +        m_state_in_str = m_state_in_str_new;
>>> +    }
>>> +
>>> +cleanup:
>> Need to free 'buf' too right?
> 
> Should not be needed due to the reason above.
> 
Ahh, I see... I must dig deeper :-) virCommandAddArgBuffer() does the
magic free...

Does the thisversion check need to be >= or is > proper? Reading the
comment makes me believe it was added as of 1.4.16, thus a >= rather
than >.

Again - the changes seem reasonable to me.

ACK

John