[Qemu-devel] [PATCH 0/2] Fix compilation of netmap backend

[Qemu-devel] [PATCH 0/2] Fix compilation of netmap backend

[Vincenzo Maffione]

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

-- 
2.3.3

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

[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 <v.maffione@gmail.com>
---
 net/netmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netmap.c b/net/netmap.c
index 508b829..4197a9c 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -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;
-- 
2.3.3

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

[Markus Armbruster]

Vincenzo Maffione <v.maffione@gmail.com> writes:

> 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 <v.maffione@gmail.com>
> ---
>  net/netmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/netmap.c b/net/netmap.c
> index 508b829..4197a9c 100644
> --- a/net/netmap.c
> +++ b/net/netmap.c
> @@ -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 <armbru@redhat.com>

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

[Vincenzo Maffione]

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 <armbru@redhat.com>:
> Vincenzo Maffione <v.maffione@gmail.com> writes:
>
>> 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 <v.maffione@gmail.com>
>> ---
>>  net/netmap.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/netmap.c b/net/netmap.c
>> index 508b829..4197a9c 100644
>> --- a/net/netmap.c
>> +++ b/net/netmap.c
>> @@ -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 <armbru@redhat.com>



-- 
Vincenzo Maffione

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1420 bytes --]

On 11/05/2015 06:01 AM, Markus Armbruster wrote:
> Vincenzo Maffione <v.maffione@gmail.com> writes:
> 
>> 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 <v.maffione@gmail.com>
>> ---
>>  net/netmap.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/netmap.c b/net/netmap.c
>> index 508b829..4197a9c 100644
>> --- a/net/netmap.c
>> +++ b/net/netmap.c
>> @@ -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?

> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[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)

2015-11-05 15:46 GMT+01:00 Eric Blake <eblake@redhat.com>:
> On 11/05/2015 06:01 AM, Markus Armbruster wrote:
>> Vincenzo Maffione <v.maffione@gmail.com> writes:
>>
>>> 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 <v.maffione@gmail.com>
>>> ---
>>>  net/netmap.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/netmap.c b/net/netmap.c
>>> index 508b829..4197a9c 100644
>>> --- a/net/netmap.c
>>> +++ b/net/netmap.c
>>> @@ -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?
>
>>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>



-- 
Vincenzo Maffione

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 687 bytes --]

On 11/05/2015 07:52 AM, Vincenzo Maffione wrote:
> 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?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Vincenzo Maffione]

2015-11-05 16:01 GMT+01:00 Eric Blake <eblake@redhat.com>:
> On 11/05/2015 07:52 AM, Vincenzo Maffione wrote:
>> 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
> 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.


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



-- 
Vincenzo Maffione

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

[Markus Armbruster]

Vincenzo Maffione <v.maffione@gmail.com> writes:

> 2015-11-05 16:01 GMT+01:00 Eric Blake <eblake@redhat.com>:
>> On 11/05/2015 07:52 AM, Vincenzo Maffione wrote:
>>> 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
>> 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.

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

[Vincenzo Maffione]

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

Signed-off-by: Vincenzo Maffione <v.maffione@gmail.com>
---
 net/netmap.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/netmap.c b/net/netmap.c
index 4197a9c..45290d8 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -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. */
-- 
2.3.3

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

[Markus Armbruster]

Vincenzo Maffione <v.maffione@gmail.com> writes:

> 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.

> Signed-off-by: Vincenzo Maffione <v.maffione@gmail.com>
> ---
>  net/netmap.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/net/netmap.c b/net/netmap.c
> index 4197a9c..45290d8 100644
> --- a/net/netmap.c
> +++ b/net/netmap.c
> @@ -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. */

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.