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(a)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