Re: [PATCH] virNetDevMacVLanTapSetup: Work around older systems

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

 



On 12/12/2015 02:20 AM, Michal Privoznik wrote:
Some older systems, e.g. RHEL-6 do not have IFF_MULTI_QUEUE flag
which we use to enable multiqueue feature. Therefore one gets the
following compile error there:

   CC     util/libvirt_util_la-virnetdevmacvlan.lo
util/virnetdevmacvlan.c: In function 'virNetDevMacVLanTapSetup':
util/virnetdevmacvlan.c:338: error: 'IFF_MULTI_QUEUE' undeclared (first use in this function)
util/virnetdevmacvlan.c:338: error: (Each undeclared identifier is reported only once
util/virnetdevmacvlan.c:338: error: for each function it appears in.)
make[3]: *** [util/libvirt_util_la-virnetdevmacvlan.lo] Error 1

So, whenever user wants us to enable the feature on such systems,
we will just throw a runtime error instead.

Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
  src/util/virnetdevmacvlan.c | 8 ++++++++
  1 file changed, 8 insertions(+)

diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index d8d1d90..28c9f22 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -307,49 +307,57 @@ static int
  virNetDevMacVLanTapSetup(int *tapfd, size_t tapfdSize, bool vnet_hdr, bool multiqueue)
  {
      unsigned int features;
      struct ifreq ifreq;
      short new_flags = 0;
      size_t i;
for (i = 0; i < tapfdSize; i++) {
          memset(&ifreq, 0, sizeof(ifreq));
if (ioctl(tapfd[i], TUNGETIFF, &ifreq) < 0) {
              virReportSystemError(errno, "%s",
                                   _("cannot get interface flags on macvtap tap"));
              return -1;
          }
new_flags = ifreq.ifr_flags; if (vnet_hdr) {
              if (ioctl(tapfd[i], TUNGETFEATURES, &features) < 0) {
                  virReportSystemError(errno, "%s",
                                       _("cannot get feature flags on macvtap tap"));
                  return -1;
              }
              if (features & IFF_VNET_HDR)
                  new_flags |= IFF_VNET_HDR;
          } else {
              new_flags &= ~IFF_VNET_HDR;
          }
+#ifdef IFF_MULTI_QUEUE
          if (multiqueue)
              new_flags |= IFF_MULTI_QUEUE;
          else
              new_flags &= ~IFF_MULTI_QUEUE;
+#else
+        if (multiqueue) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("Multiqueue devices are not supported on this system"));
+            return -1;
+        }
+#endif

Yep, missed that difference with the tap version in review.

BTW, this caused me to look back at patch 5 and 6 of your 7 patch series, and I noticed a couple things:


1) you pass down a separate bool, multiqueue, to this function in the macvtap version, but in the tap version you just check "tapfdSize" right at the spot where you're setting it, eliminating the need for an extra arg. That seems cleaner.

2) When creating the bool to pass into this function, you use:

+        if (virNetDevMacVLanTapSetup(tapfd, tapfdSize, vnet_hdr, tapfdSize > 0) < 0) {

That should be "tapfdSize > 1" (and as I implied in (1), I think it would be simpler
to just do that directly in virNetDevMacVLanTapSetup rather than passing it in a separate
bool.

ACK to this change.

It looks like you haven't pushed the other patches yet, so how about
you just squash this change into the other patches so they are all
correct from the start?


if (new_flags != ifreq.ifr_flags) {
              ifreq.ifr_flags = new_flags;
              if (ioctl(tapfd[i], TUNSETIFF, &ifreq) < 0) {
                  virReportSystemError(errno, "%s",
                                       _("unable to set vnet or multiqueue flags on macvtap"));
                  return -1;
              }
          }
      }
return 0;
  }

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]