Fix compilation of netmap backend

PATCH Superseded Vincenzo Maffione 3 participants 11 messages Nov 5, 2015 – Nov 6, 2015 Networking

Share & tools

Vincenzo Maffione

author

2015-11-05 09:36 UTC #0 lore raw

cover Fix compilation of netmap backend

This patch series adds some fixes to the netmap net backend. It contains two changes:
(1) Fix compilation issue of netmap.c introduced by the reorganization of struct NetClientOptions
(2) Address the FIXME comment that was asking to use error_setg() in place of error_report()

Vincenzo Maffione (2):
net: netmap: Fix compilation issue
net: netmap: use error_setg_errno() in place of error_report()

 net/netmap.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

Signature · 1 line

2.3.3

Vincenzo Maffione

author

2015-11-05 09:36 UTC #9 lore raw

PATCH 2/2 net: netmap: use error_setg_errno() in place of error_report()

This update was required to align netmap backend to the other net backends in terms of error reporting.

Signed-off-by Vincenzo Maffione <[email protected]>

---
 net/netmap.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

net/netmap.c +6 -8

index 4197a9c..45290d8 100644
@@ -90,7 +90,7 @@ pkt_copy(const void *_src, void *_dst, int l)
  * Open a netmap device. We assume there is only one queue
  * (which is the case for the VALE bridge).
  */
-static int netmap_open(NetmapPriv *me)
+static int netmap_open(NetmapPriv *me, Error **errp)
 {
     int fd;
     int err;

@@ -99,8 +99,8 @@ static int netmap_open(NetmapPriv *me)
 
     me->fd = fd = open(me->fdname, O_RDWR);
     if (fd < 0) {
-        error_report("Unable to open netmap device '%s' (%s)",
-                        me->fdname, strerror(errno));
+        error_setg_errno(errp, errno, "Unable to open netmap device '%s'",
+                         me->fdname);
         return -1;
     }
     memset(&req, 0, sizeof(req));

@@ -109,15 +109,14 @@ static int netmap_open(NetmapPriv *me)
     req.nr_version = NETMAP_API;
     err = ioctl(fd, NIOCREGIF, &req);
     if (err) {
-        error_report("Unable to register %s: %s", me->ifname, strerror(errno));
+        error_setg_errno(errp, errno, "Unable to register %s", me->ifname);
         goto error;
     }
     l = me->memsize = req.nr_memsize;
 
     me->mem = mmap(0, l, PROT_WRITE | PROT_READ, MAP_SHARED, fd, 0);
     if (me->mem == MAP_FAILED) {
-        error_report("Unable to mmap netmap shared memory: %s",
-                        strerror(errno));
+        error_setg_errno(errp, errno, "Unable to mmap netmap shared memory");
         me->mem = NULL;
         goto error;
     }

@@ -438,7 +437,6 @@ static NetClientInfo net_netmap_info = {
 int net_init_netmap(const NetClientOptions *opts,
                     const char *name, NetClientState *peer, Error **errp)
 {
-    /* FIXME error_setg(errp, ...) on failure */
     const NetdevNetmapOptions *netmap_opts = opts->u.netmap;
     NetClientState *nc;
     NetmapPriv me;

@@ -448,7 +446,7 @@ int net_init_netmap(const NetClientOptions *opts,
         netmap_opts->has_devname ? netmap_opts->devname : "/dev/netmap");
     /* Set default name for the port if not supplied. */
     pstrcpy(me.ifname, sizeof(me.ifname), netmap_opts->ifname);
-    if (netmap_open(&me)) {
+    if (netmap_open(&me, errp)) {
         return -1;
     }
     /* Create the object. */

Signature · 1 line

2.3.3

Vincenzo Maffione

author

2015-11-05 09:36 UTC #1 lore raw

PATCH 1/2 net: netmap: Fix compilation issue

In reply to Vincenzo Maffione

Reorganization of struct NetClientOptions caused a compilation failure of the netmap backend. This patch fixes the issue by properly accessing the union field.

Signed-off-by Vincenzo Maffione <[email protected]>

---
 net/netmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/netmap.c +1 -1

index 508b829..4197a9c 100644
@@ -439,7 +439,7 @@ int net_init_netmap(const NetClientOptions *opts,
                     const char *name, NetClientState *peer, Error **errp)
 {
     /* FIXME error_setg(errp, ...) on failure */
-    const NetdevNetmapOptions *netmap_opts = opts->netmap;
+    const NetdevNetmapOptions *netmap_opts = opts->u.netmap;
     NetClientState *nc;
     NetmapPriv me;
     NetmapState *s;

Signature · 1 line

2.3.3

Markus Armbruster

Reviewed-by on v1 1/2

2015-11-05 13:01 UTC #2 lore raw

Re: [Qemu-devel] [PATCH 1/2] net: netmap: Fix compilation issue

Quoted 14 lines · Vincenzo Maffione

Reorganization of struct NetClientOptions caused a compilation failure of the netmap backend. This patch fixes the issue by properly accessing the union field.
Signed-off-by Vincenzo Maffione <[email protected]>
---
 net/netmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/netmap.c +0 -0
index 508b829..4197a9c 100644

net/netmap.c int net_init_netmap(const NetClientOptions *opts, · line 445

Quoted 8 lines

net/netmap.c +1 -1

@@ -439,7 +439,7 @@ int net_init_netmap(const NetClientOptions *opts,
                     const char *name, NetClientState *peer, Error **errp)
 {
     /* FIXME error_setg(errp, ...) on failure */
-    const NetdevNetmapOptions *netmap_opts = opts->netmap;
+    const NetdevNetmapOptions *netmap_opts = opts->u.netmap;
     NetClientState *nc;
     NetmapPriv me;
     NetmapState *s;

Broken in commit e4ba22b. Recommend to point to that in the commit message. Thanks for cleaning up the mess we made there!

Reviewed-by Markus Armbruster <[email protected]>

Markus Armbruster

2015-11-05 13:13 UTC #10 lore raw

Re: [Qemu-devel] [PATCH 2/2] net: netmap: use error_setg_errno() in place of error_report()

In reply to Vincenzo Maffione

Quoted 2 lines · Vincenzo Maffione

This update was required to align netmap backend to the other net backends in terms of error reporting.

Thank you very much for helping to complete this job!

Recommend to point to commit a30ecde in your commit message.

Quoted 56 lines

Signed-off-by Vincenzo Maffione <[email protected]>

---
 net/netmap.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

net/netmap.c +5 -7

index 4197a9c..45290d8 100644
@@ -90,7 +90,7 @@ pkt_copy(const void *_src, void *_dst, int l)
  * Open a netmap device. We assume there is only one queue
  * (which is the case for the VALE bridge).
  */
-static int netmap_open(NetmapPriv *me)
+static int netmap_open(NetmapPriv *me, Error **errp)
 {
     int fd;
     int err;

@@ -99,8 +99,8 @@ static int netmap_open(NetmapPriv *me)
 
     me->fd = fd = open(me->fdname, O_RDWR);
     if (fd < 0) {
-        error_report("Unable to open netmap device '%s' (%s)",
-                        me->fdname, strerror(errno));
+        error_setg_errno(errp, errno, "Unable to open netmap device '%s'",
+                         me->fdname);
         return -1;
     }
     memset(&req, 0, sizeof(req));

@@ -109,15 +109,14 @@ static int netmap_open(NetmapPriv *me)
     req.nr_version = NETMAP_API;
     err = ioctl(fd, NIOCREGIF, &req);
     if (err) {
-        error_report("Unable to register %s: %s", me->ifname, strerror(errno));
+        error_setg_errno(errp, errno, "Unable to register %s", me->ifname);
         goto error;
     }
     l = me->memsize = req.nr_memsize;
 
     me->mem = mmap(0, l, PROT_WRITE | PROT_READ, MAP_SHARED, fd, 0);
     if (me->mem == MAP_FAILED) {
-        error_report("Unable to mmap netmap shared memory: %s",
-                        strerror(errno));
+        error_setg_errno(errp, errno, "Unable to mmap netmap shared memory");
         me->mem = NULL;
         goto error;
     }

@@ -438,7 +437,6 @@ static NetClientInfo net_netmap_info = {
 int net_init_netmap(const NetClientOptions *opts,
                     const char *name, NetClientState *peer, Error **errp)
 {
-    /* FIXME error_setg(errp, ...) on failure */
     const NetdevNetmapOptions *netmap_opts = opts->u.netmap;
     NetClientState *nc;
     NetmapPriv me;

net/netmap.c int net_init_netmap(const NetClientOptions *opts, · line 452

Quoted 8 lines

net/netmap.c +1 -1

@@ -448,7 +446,7 @@ int net_init_netmap(const NetClientOptions *opts,
         netmap_opts->has_devname ? netmap_opts->devname : "/dev/netmap");
     /* Set default name for the port if not supplied. */
     pstrcpy(me.ifname, sizeof(me.ifname), netmap_opts->ifname);
-    if (netmap_open(&me)) {
+    if (netmap_open(&me, errp)) {
         return -1;
     }
     /* Create the object. */

netmap_open() returns 0 on success, -1 on failure. For better or worse, we avoid such returns for functions that reports errors via an Error ** parameter. For an example, see commit 445f116.

Since errp can be null, you can't test *err, but have to use a local variable, like this:

    netmap_open(&me, &err);
    if (err) {
        error_propagate(errp, err);
        return;
    }

See the big comment in include/qapi/error.h.

I actually like the way you did it, but consistency is more important than what I like here.

Vincenzo Maffione

author

2015-11-05 13:26 UTC #3 lore raw

Re: [Qemu-devel] [PATCH 1/2] net: netmap: Fix compilation issue

In reply to Markus Armbruster

Sure, no problems. I was looking for that commit, but using git blame didn't work, since there the data structure seems to be part of autogenerated code.

2015-11-05 14:01 GMT+01:00 Markus Armbruster <[email protected]>:

Quoted 29 lines

Quoted 14 lines · Vincenzo Maffione
Reorganization of struct NetClientOptions caused a compilation failure of the netmap backend. This patch fixes the issue by properly accessing the union field.
Signed-off-by Vincenzo Maffione <[email protected]>
---
 net/netmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/netmap.c +0 -0
index 508b829..4197a9c 100644
net/netmap.c int net_init_netmap(const NetClientOptions *opts, · line 445
Quoted 8 lines
net/netmap.c +1 -1
@@ -439,7 +439,7 @@ int net_init_netmap(const NetClientOptions *opts,
                     const char *name, NetClientState *peer, Error **errp)
 {
     /* FIXME error_setg(errp, ...) on failure */
-    const NetdevNetmapOptions *netmap_opts = opts->netmap;
+    const NetdevNetmapOptions *netmap_opts = opts->u.netmap;
     NetClientState *nc;
     NetmapPriv me;
     NetmapState *s;
Broken in commit e4ba22b. Recommend to point to that in the commit message. Thanks for cleaning up the mess we made there!
Reviewed-by Markus Armbruster <[email protected]>

Signature · 1 line

Vincenzo Maffione

Eric Blake

Reviewed-by

2015-11-05 14:46 UTC #4 lore raw

Re: [Qemu-devel] [PATCH 1/2] net: netmap: Fix compilation issue

In reply to Markus Armbruster

Quoted 11 lines · Markus Armbruster

Quoted 9 lines · Vincenzo Maffione
Reorganization of struct NetClientOptions caused a compilation failure of the netmap backend. This patch fixes the issue by properly accessing the union field.
Signed-off-by Vincenzo Maffione <[email protected]>
---
 net/netmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 16 lines

net/netmap.c int net_init_netmap(const NetClientOptions *opts, · line 445

Quoted 13 lines

net/netmap.c +1 -1

index 508b829..4197a9c 100644
@@ -439,7 +439,7 @@ int net_init_netmap(const NetClientOptions *opts,
                     const char *name, NetClientState *peer, Error **errp)
 {
     /* FIXME error_setg(errp, ...) on failure */
-    const NetdevNetmapOptions *netmap_opts = opts->netmap;
+    const NetdevNetmapOptions *netmap_opts = opts->u.netmap;
     NetClientState *nc;
     NetmapPriv me;
     NetmapState *s;

Broken in commit e4ba22b. Recommend to point to that in the commit message. Thanks for cleaning up the mess we made there!

My fault for not compiling netmap code. Does it need specific configure options?

Quoted 3 lines

Reviewed-by Markus Armbruster <[email protected]>

Reviewed-by Eric Blake <[email protected]>

Signature · 2 lines

Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Vincenzo Maffione

author

2015-11-05 14:52 UTC #5 lore raw

Re: [Qemu-devel] [PATCH 1/2] net: netmap: Fix compilation issue

No worries.

It needs --enable-netmap (default is --disable-netmap on linux), and --extra-cflags=-I/path/to/netmap/sys for netmap API headers.

(netmap code is avaliable here https://github.com/luigirizzo/netmap)

2015-11-05 15:46 GMT+01:00 Eric Blake <[email protected]>:

Quoted 42 lines · Markus Armbruster

Quoted 11 lines
Quoted 9 lines · Vincenzo Maffione
Reorganization of struct NetClientOptions caused a compilation failure of the netmap backend. This patch fixes the issue by properly accessing the union field.
Signed-off-by Vincenzo Maffione <[email protected]>
---
 net/netmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 16 lines
net/netmap.c int net_init_netmap(const NetClientOptions *opts, · line 445
Quoted 13 lines
net/netmap.c +1 -1
index 508b829..4197a9c 100644
@@ -439,7 +439,7 @@ int net_init_netmap(const NetClientOptions *opts,
                     const char *name, NetClientState *peer, Error **errp)
 {
     /* FIXME error_setg(errp, ...) on failure */
-    const NetdevNetmapOptions *netmap_opts = opts->netmap;
+    const NetdevNetmapOptions *netmap_opts = opts->u.netmap;
     NetClientState *nc;
     NetmapPriv me;
     NetmapState *s;
Broken in commit e4ba22b. Recommend to point to that in the commit message. Thanks for cleaning up the mess we made there!
My fault for not compiling netmap code. Does it need specific configure options?
Quoted 3 lines
Reviewed-by Markus Armbruster <[email protected]>
Reviewed-by Eric Blake <[email protected]>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Signature · 1 line

Vincenzo Maffione

Eric Blake

2015-11-05 15:01 UTC #6 lore raw

Re: [Qemu-devel] [PATCH 1/2] net: netmap: Fix compilation issue

Quoted 6 lines · Vincenzo Maffione

No worries.
It needs --enable-netmap (default is --disable-netmap on linux), and --extra-cflags=-I/path/to/netmap/sys for netmap API headers.
(netmap code is avaliable here https://github.com/luigirizzo/netmap)

Default-off tends to bit-rot; default-probe is slightly better (compile if /path/to/netmap/sys is found during configure, otherwise not). But not the end of the world. Thanks for the pointer to the headers. Any idea if someone is trying to package netmap development headers for Fedora?

Signature · 2 lines

Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Vincenzo Maffione

author

2015-11-05 20:27 UTC #7 lore raw

Re: [Qemu-devel] [PATCH 1/2] net: netmap: Fix compilation issue

2015-11-05 16:01 GMT+01:00 Eric Blake <[email protected]>:

Quoted 11 lines · Vincenzo Maffione

Quoted 6 lines
No worries.
It needs --enable-netmap (default is --disable-netmap on linux), and --extra-cflags=-I/path/to/netmap/sys for netmap API headers.
(netmap code is avaliable here https://github.com/luigirizzo/netmap)
Default-off tends to bit-rot; default-probe is slightly better (compile if /path/to/netmap/sys is found during configure, otherwise not). But not the end of the world.

The default-off decision was taken at the time when netmap was merged in (see the
"[PATCH v3] net: Adding netmap network backend" thread). The qemu ./configure already has support for probing netmap headers, so we could actually switch to default-on also on linux (like it happens for freebsd qemu build).

Thanks for the pointer to the headers. Any

Quoted 1 line

idea if someone is trying to package netmap development headers for Fedora?

No idea, unfortunately. I personally maintain an AUR package for Archlinux (https://aur.archlinux.org/packages/netmap/), that's all I know. Having netmap headers packaged in some distributions is something we were willing to do, but still nothing has been done, afaik.

Quoted 5 lines

--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Signature · 1 line

Vincenzo Maffione

Markus Armbruster

2015-11-06 10:06 UTC #8 lore raw

Re: [Qemu-devel] [PATCH 1/2] net: netmap: Fix compilation issue

Quoted 20 lines · Vincenzo Maffione

2015-11-05 16:01 GMT+01:00 Eric Blake <[email protected]>:
Quoted 11 lines · Vincenzo Maffione
Quoted 6 lines
No worries.
It needs --enable-netmap (default is --disable-netmap on linux), and --extra-cflags=-I/path/to/netmap/sys for netmap API headers.
(netmap code is avaliable here https://github.com/luigirizzo/netmap)
Default-off tends to bit-rot; default-probe is slightly better (compile if /path/to/netmap/sys is found during configure, otherwise not). But not the end of the world.
The default-off decision was taken at the time when netmap was merged in (see the
"[PATCH v3] net: Adding netmap network backend" thread). The qemu ./configure already has support for probing netmap headers, so we could actually switch to default-on also on linux (like it happens for freebsd qemu build).

Quoted 8 lines

Thanks for the pointer to the headers. Any
Quoted 1 line
idea if someone is trying to package netmap development headers for Fedora?
No idea, unfortunately. I personally maintain an AUR package for Archlinux (https://aur.archlinux.org/packages/netmap/), that's all I know. Having netmap headers packaged in some distributions is something we were willing to do, but still nothing has been done, afaik.

As long as netmap isn't packaged in at least one distro we actually compile-test before merge into master, enabling it automatically won't do squat for avoiding bitrot.