util: fix virNetDevSetCoalesce fallback on Win32/FreeBSD

[libvirt] [PATCH] util: fix virNetDevSetCoalesce fallback on Win32/FreeBSD

Posted by Daniel P. Berrange 6 years, 11 months ago

The current fallback stub for virNetDevSetCoalesce is inside an
earlier conditional block. This deals with the feature being
missing on older Linux platforms. We need a second fallback stub
though, outside the top level conditional, to ensure builds work
on Win32/FreeBSD platforms too.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---

Pushed as a build fix

 src/util/virnetdev.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 6ff1b48..27f1637 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -3207,6 +3207,15 @@ virNetDevGetFeatures(const char *ifname ATTRIBUTE_UNUSED,
               ifname);
     return 0;
 }
+
+int virNetDevSetCoalesce(const char *ifname,
+                         virNetDevCoalescePtr coalesce ATTRIBUTE_UNUSED)
+{
+    virReportSystemError(ENOSYS,
+                         _("Cannot set coalesce info on interface '%s'"),
+                         ifname);
+    return -1;
+}
 #endif
 
 
-- 
2.9.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: fix virNetDevSetCoalesce fallback on Win32/FreeBSD

Posted by Roman Bogorodskiy 6 years, 11 months ago

  Daniel P. Berrange wrote:

> The current fallback stub for virNetDevSetCoalesce is inside an
> earlier conditional block. This deals with the feature being
> missing on older Linux platforms. We need a second fallback stub
> though, outside the top level conditional, to ensure builds work
> on Win32/FreeBSD platforms too.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> 
> Pushed as a build fix
> 
>  src/util/virnetdev.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 6ff1b48..27f1637 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -3207,6 +3207,15 @@ virNetDevGetFeatures(const char *ifname ATTRIBUTE_UNUSED,
>                ifname);
>      return 0;
>  }
> +
> +int virNetDevSetCoalesce(const char *ifname,
> +                         virNetDevCoalescePtr coalesce ATTRIBUTE_UNUSED)
> +{
> +    virReportSystemError(ENOSYS,
> +                         _("Cannot set coalesce info on interface '%s'"),
> +                         ifname);
> +    return -1;
> +}
>  #endif

I'm wondering if this stub could be relaxed to just return 0 instead
of triggering error?

As this function is called by virNetDevTapCreateInBridgePort(), it fails
because virNetDevSetCoalesce() fails, making impossible to use the
bridged networking (which is very unfortunate).

The other option could be to ignore virNetDevSetCoalesce() return value
in virNetDevTapCreateInBridgePort() (the only called of this function so
far).

Roman Bogorodskiy
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: fix virNetDevSetCoalesce fallback on Win32/FreeBSD

Posted by Michal Privoznik 6 years, 11 months ago

On 04/23/2017 02:55 PM, Roman Bogorodskiy wrote:
>   Daniel P. Berrange wrote:
> 
>> The current fallback stub for virNetDevSetCoalesce is inside an
>> earlier conditional block. This deals with the feature being
>> missing on older Linux platforms. We need a second fallback stub
>> though, outside the top level conditional, to ensure builds work
>> on Win32/FreeBSD platforms too.
>>
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> ---
>>
>> Pushed as a build fix
>>
>>  src/util/virnetdev.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index 6ff1b48..27f1637 100644
>> --- a/src/util/virnetdev.c
>> +++ b/src/util/virnetdev.c
>> @@ -3207,6 +3207,15 @@ virNetDevGetFeatures(const char *ifname ATTRIBUTE_UNUSED,
>>                ifname);
>>      return 0;
>>  }
>> +
>> +int virNetDevSetCoalesce(const char *ifname,
>> +                         virNetDevCoalescePtr coalesce ATTRIBUTE_UNUSED)
>> +{
>> +    virReportSystemError(ENOSYS,
>> +                         _("Cannot set coalesce info on interface '%s'"),
>> +                         ifname);
>> +    return -1;
>> +}
>>  #endif
> 
> I'm wondering if this stub could be relaxed to just return 0 instead
> of triggering error?

We can make it return 0 if coalesce is NULL just like real function
does. Otherwise the error will be be reported. Just like it is now.

> 
> As this function is called by virNetDevTapCreateInBridgePort(), it fails
> because virNetDevSetCoalesce() fails, making impossible to use the
> bridged networking (which is very unfortunate).
> 
> The other option could be to ignore virNetDevSetCoalesce() return value
> in virNetDevTapCreateInBridgePort() (the only called of this function so
> far).

That would not fly though. It would ignore any real error too.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: fix virNetDevSetCoalesce fallback on Win32/FreeBSD

Posted by Roman Bogorodskiy 6 years, 11 months ago

  Michal Privoznik wrote:

> On 04/23/2017 02:55 PM, Roman Bogorodskiy wrote:
> >   Daniel P. Berrange wrote:
> > 
> >> The current fallback stub for virNetDevSetCoalesce is inside an
> >> earlier conditional block. This deals with the feature being
> >> missing on older Linux platforms. We need a second fallback stub
> >> though, outside the top level conditional, to ensure builds work
> >> on Win32/FreeBSD platforms too.
> >>
> >> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> >> ---
> >>
> >> Pushed as a build fix
> >>
> >>  src/util/virnetdev.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> >> index 6ff1b48..27f1637 100644
> >> --- a/src/util/virnetdev.c
> >> +++ b/src/util/virnetdev.c
> >> @@ -3207,6 +3207,15 @@ virNetDevGetFeatures(const char *ifname ATTRIBUTE_UNUSED,
> >>                ifname);
> >>      return 0;
> >>  }
> >> +
> >> +int virNetDevSetCoalesce(const char *ifname,
> >> +                         virNetDevCoalescePtr coalesce ATTRIBUTE_UNUSED)
> >> +{
> >> +    virReportSystemError(ENOSYS,
> >> +                         _("Cannot set coalesce info on interface '%s'"),
> >> +                         ifname);
> >> +    return -1;
> >> +}
> >>  #endif
> > 
> > I'm wondering if this stub could be relaxed to just return 0 instead
> > of triggering error?
> 
> We can make it return 0 if coalesce is NULL just like real function
> does. Otherwise the error will be be reported. Just like it is now.

Yeah, this sounds reasonable and works fine. Here's a patch for that:

https://www.redhat.com/archives/libvir-list/2017-April/msg01019.html

> > 
> > As this function is called by virNetDevTapCreateInBridgePort(), it fails
> > because virNetDevSetCoalesce() fails, making impossible to use the
> > bridged networking (which is very unfortunate).
> > 
> > The other option could be to ignore virNetDevSetCoalesce() return value
> > in virNetDevTapCreateInBridgePort() (the only called of this function so
> > far).
> 
> That would not fly though. It would ignore any real error too.
> 
> Michal

Roman Bogorodskiy
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list