diff mbox

[13/15] tap-solaris: Convert tap_open() to Error

Message ID 1431432187-10993-14-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster May 12, 2015, 12:03 p.m. UTC 
Fixes inappropriate use of syslog().

Not fixed: leaks on error paths, suspicious non-fatal errors.  FIXMEs
added instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/tap-solaris.c | 59 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 30 insertions(+), 29 deletions(-)

Comments

Eric Blake May 14, 2015, 10 p.m. UTC | #1 
On 05/12/2015 06:03 AM, Markus Armbruster wrote:
> Fixes inappropriate use of syslog().
> 
> Not fixed: leaks on error paths, suspicious non-fatal errors.  FIXMEs
> added instead.

At least you're admitting where the code is still bad.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  net/tap-solaris.c | 59 ++++++++++++++++++++++++++++---------------------------
>  1 file changed, 30 insertions(+), 29 deletions(-)
> 

> @@ -99,20 +100,20 @@ static int tap_alloc(char *dev, size_t dev_size)
>      strioc_ppa.ic_len = sizeof(ppa);
>      strioc_ppa.ic_dp = (char *)&ppa;
>      if ((ppa = ioctl (tap_fd, I_STR, &strioc_ppa)) < 0)
> -       syslog (LOG_ERR, "Can't assign new interface");
> +        error_report("Can't assign new interface");

I see you're fixing spacing while at it, as well.

>  
>      TFR(if_fd = open("/dev/tap", O_RDWR, 0));
>      if (if_fd < 0) {
> -       syslog(LOG_ERR, "Can't open /dev/tap (2)");
> -       return -1;
> +        error_setg(errp, "Can't open /dev/tap (2)");
> +        return -1;
>      }
>      if(ioctl(if_fd, I_PUSH, "ip") < 0){
> -       syslog(LOG_ERR, "Can't push IP module");

Should you add the space after 'if' while touching this?

> -       return -1;
> +        error_setg(errp, "Can't push IP module");
> +        return -1;
>      }
>  
>      if (ioctl(if_fd, SIOCGLIFFLAGS, &ifr) < 0)
> -	syslog(LOG_ERR, "Can't get flags\n");
> +        error_report("Can't get flags");

What about adding missing {} while touching this file?  Hmm - there's
enough cruft that it may involve a separate patch just to clean up
style. For this patch, I'm not going to hold up review on style.

Reviewed-by: Eric Blake <eblake@redhat.com>
Markus Armbruster May 15, 2015, 8:48 a.m. UTC | #2 
Eric Blake <eblake@redhat.com> writes:

> On 05/12/2015 06:03 AM, Markus Armbruster wrote:
>> Fixes inappropriate use of syslog().
>> 
>> Not fixed: leaks on error paths, suspicious non-fatal errors.  FIXMEs
>> added instead.
>
> At least you're admitting where the code is still bad.

Actually, git-rm felt pretty tempting.

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  net/tap-solaris.c | 59 ++++++++++++++++++++++++++++---------------------------
>>  1 file changed, 30 insertions(+), 29 deletions(-)
>> 
>
>> @@ -99,20 +100,20 @@ static int tap_alloc(char *dev, size_t dev_size)
>>      strioc_ppa.ic_len = sizeof(ppa);
>>      strioc_ppa.ic_dp = (char *)&ppa;
>>      if ((ppa = ioctl (tap_fd, I_STR, &strioc_ppa)) < 0)
>> -       syslog (LOG_ERR, "Can't assign new interface");
>> +        error_report("Can't assign new interface");
>
> I see you're fixing spacing while at it, as well.
>
>>  
>>      TFR(if_fd = open("/dev/tap", O_RDWR, 0));
>>      if (if_fd < 0) {
>> -       syslog(LOG_ERR, "Can't open /dev/tap (2)");
>> -       return -1;
>> +        error_setg(errp, "Can't open /dev/tap (2)");
>> +        return -1;
>>      }
>>      if(ioctl(if_fd, I_PUSH, "ip") < 0){
>> -       syslog(LOG_ERR, "Can't push IP module");
>
> Should you add the space after 'if' while touching this?

Fixing up just enough to make checkpatch happy.  Also keeps git-blame
useful even without -w.

>> -       return -1;
>> +        error_setg(errp, "Can't push IP module");
>> +        return -1;
>>      }
>>  
>>      if (ioctl(if_fd, SIOCGLIFFLAGS, &ifr) < 0)
>> -	syslog(LOG_ERR, "Can't get flags\n");
>> +        error_report("Can't get flags");
>
> What about adding missing {} while touching this file?  Hmm - there's
> enough cruft that it may involve a separate patch just to clean up
> style. For this patch, I'm not going to hold up review on style.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!
diff mbox

Patch

diff --git a/net/tap-solaris.c b/net/tap-solaris.c
index 079046b..90b2fd1 100644
--- a/net/tap-solaris.c
+++ b/net/tap-solaris.c
@@ -36,7 +36,6 @@ 
 #include <netinet/udp.h>
 #include <netinet/tcp.h>
 #include <net/if.h>
-#include <syslog.h>
 #include <stropts.h>
 #include "qemu/error-report.h"
 
@@ -56,8 +55,10 @@  ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen)
  * Allocate TAP device, returns opened fd.
  * Stores dev name in the first arg(must be large enough).
  */
-static int tap_alloc(char *dev, size_t dev_size)
+static int tap_alloc(char *dev, size_t dev_size, Error **errp)
 {
+    /* FIXME leaks like a sieve on error paths */
+    /* FIXME suspicious: many errors are reported, then ignored */
     int tap_fd, if_fd, ppa = -1;
     static int ip_fd = 0;
     char *ptr;
@@ -83,14 +84,14 @@  static int tap_alloc(char *dev, size_t dev_size)
 
     TFR(ip_fd = open("/dev/udp", O_RDWR, 0));
     if (ip_fd < 0) {
-       syslog(LOG_ERR, "Can't open /dev/ip (actually /dev/udp)");
-       return -1;
+        error_setg(errp, "Can't open /dev/ip (actually /dev/udp)");
+        return -1;
     }
 
     TFR(tap_fd = open("/dev/tap", O_RDWR, 0));
     if (tap_fd < 0) {
-       syslog(LOG_ERR, "Can't open /dev/tap");
-       return -1;
+        error_setg(errp, "Can't open /dev/tap");
+        return -1;
     }
 
     /* Assign a new PPA and get its unit number. */
@@ -99,20 +100,20 @@  static int tap_alloc(char *dev, size_t dev_size)
     strioc_ppa.ic_len = sizeof(ppa);
     strioc_ppa.ic_dp = (char *)&ppa;
     if ((ppa = ioctl (tap_fd, I_STR, &strioc_ppa)) < 0)
-       syslog (LOG_ERR, "Can't assign new interface");
+        error_report("Can't assign new interface");
 
     TFR(if_fd = open("/dev/tap", O_RDWR, 0));
     if (if_fd < 0) {
-       syslog(LOG_ERR, "Can't open /dev/tap (2)");
-       return -1;
+        error_setg(errp, "Can't open /dev/tap (2)");
+        return -1;
     }
     if(ioctl(if_fd, I_PUSH, "ip") < 0){
-       syslog(LOG_ERR, "Can't push IP module");
-       return -1;
+        error_setg(errp, "Can't push IP module");
+        return -1;
     }
 
     if (ioctl(if_fd, SIOCGLIFFLAGS, &ifr) < 0)
-	syslog(LOG_ERR, "Can't get flags\n");
+        error_report("Can't get flags");
 
     snprintf (actual_name, 32, "tap%d", ppa);
     pstrcpy(ifr.lifr_name, sizeof(ifr.lifr_name), actual_name);
@@ -121,22 +122,22 @@  static int tap_alloc(char *dev, size_t dev_size)
     /* Assign ppa according to the unit number returned by tun device */
 
     if (ioctl (if_fd, SIOCSLIFNAME, &ifr) < 0)
-        syslog (LOG_ERR, "Can't set PPA %d", ppa);
+        error_report("Can't set PPA %d", ppa);
     if (ioctl(if_fd, SIOCGLIFFLAGS, &ifr) <0)
-        syslog (LOG_ERR, "Can't get flags\n");
+        error_report("Can't get flags");
     /* Push arp module to if_fd */
     if (ioctl (if_fd, I_PUSH, "arp") < 0)
-        syslog (LOG_ERR, "Can't push ARP module (2)");
+        error_report("Can't push ARP module (2)");
 
     /* Push arp module to ip_fd */
     if (ioctl (ip_fd, I_POP, NULL) < 0)
-        syslog (LOG_ERR, "I_POP failed\n");
+        error_report("I_POP failed");
     if (ioctl (ip_fd, I_PUSH, "arp") < 0)
-        syslog (LOG_ERR, "Can't push ARP module (3)\n");
+        error_report("Can't push ARP module (3)");
     /* Open arp_fd */
     TFR(arp_fd = open ("/dev/tap", O_RDWR, 0));
     if (arp_fd < 0)
-       syslog (LOG_ERR, "Can't open %s\n", "/dev/tap");
+        error_report("Can't open %s", "/dev/tap");
 
     /* Set ifname to arp */
     strioc_if.ic_cmd = SIOCSLIFNAME;
@@ -144,16 +145,16 @@  static int tap_alloc(char *dev, size_t dev_size)
     strioc_if.ic_len = sizeof(ifr);
     strioc_if.ic_dp = (char *)&ifr;
     if (ioctl(arp_fd, I_STR, &strioc_if) < 0){
-        syslog (LOG_ERR, "Can't set ifname to arp\n");
+        error_report("Can't set ifname to arp");
     }
 
     if((ip_muxid = ioctl(ip_fd, I_LINK, if_fd)) < 0){
-       syslog(LOG_ERR, "Can't link TAP device to IP");
-       return -1;
+        error_setg(errp, "Can't link TAP device to IP");
+        return -1;
     }
 
     if ((arp_muxid = ioctl (ip_fd, link_type, arp_fd)) < 0)
-        syslog (LOG_ERR, "Can't link TAP device to ARP");
+        error_report("Can't link TAP device to ARP");
 
     close (if_fd);
 
@@ -166,7 +167,7 @@  static int tap_alloc(char *dev, size_t dev_size)
     {
       ioctl (ip_fd, I_PUNLINK , arp_muxid);
       ioctl (ip_fd, I_PUNLINK, ip_muxid);
-      syslog (LOG_ERR, "Can't set multiplexor id");
+      error_report("Can't set multiplexor id");
     }
 
     snprintf(dev, dev_size, "tap%d", ppa);
@@ -176,12 +177,12 @@  static int tap_alloc(char *dev, size_t dev_size)
 int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
              int vnet_hdr_required, int mq_required, Error **errp)
 {
-    /* FIXME error_setg(errp, ...) on failure */
     char  dev[10]="";
     int fd;
-    if( (fd = tap_alloc(dev, sizeof(dev))) < 0 ){
-       fprintf(stderr, "Cannot allocate TAP device\n");
-       return -1;
+
+    fd = tap_alloc(dev, sizeof(dev), errp);
+    if (fd < 0) {
+        return -1;
     }
     pstrcpy(ifname, ifname_size, dev);
     if (*vnet_hdr) {
@@ -189,8 +190,8 @@  int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
         *vnet_hdr = 0;
 
         if (vnet_hdr_required && !*vnet_hdr) {
-            error_report("vnet_hdr=1 requested, but no kernel "
-                         "support for IFF_VNET_HDR available");
+            error_setg(errp, "vnet_hdr=1 requested, but no kernel "
+                       "support for IFF_VNET_HDR available");
             close(fd);
             return -1;
         }