[PATCH] no compile warnings from cyclictest

[PATCH] no compile warnings from cyclictest

[rolf.freitag]

Hi,

compiling cyclictest gives 4 warnings of type

  warning: ignoring return value of 'write'

and three of type

  warning: ... may be used uninitialized in this function

So i eleminated the first sort with a trash variable, which adds up 
the return values and recycles them (their entropy) at the end via
srand(). This also avoids a write-only variable.
I eleminated the second sort by initializing the variables.

--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -222,6 +222,7 @@ static int use_fifo = 0;
 static pthread_t fifo_threadid;
 static int laptop = 0;
 static int use_histfile = 0;
+static unsigned int trash; // for unused return values, to avoid compiler warnings because of ignored return values
 
 #ifdef ARCH_HAS_SMI_COUNTER
 static int smi = 0;
@@ -491,10 +492,10 @@ static void tracemark(char *fmt, ...)
        va_end(ap);
 
        /* write the tracemark message */
-       write(tracemark_fd, tracebuf, len);
+       trash += write(tracemark_fd, tracebuf, len);
 
        /* now stop any trace */
-       write(trace_fd, "0\n", 2);
+       trash += write(trace_fd, "0\n", 2);
 }
 
 
@@ -510,7 +511,7 @@ static void tracing(int on)
                case KV_26_LT24: prctl(0, 1); break;
                case KV_26_33:
                case KV_30:
-                       write(trace_fd, "1", 1);
+                       trash += write(trace_fd, "1", 1);
                        break;
                default:         break;
                }
@@ -520,7 +521,7 @@ static void tracing(int on)
                case KV_26_LT24: prctl(0, 0); break;
                case KV_26_33:
                case KV_30:
-                       write(trace_fd, "0", 1);
+                       trash += write(trace_fd, "0", 1);
                        break;
                default:        break;
                }
@@ -984,8 +985,10 @@ static void *timerthread(void *param)
        int stopped = 0;
        cpu_set_t mask;
        pthread_t thread;
-       unsigned long smi_now, smi_old;
+       unsigned long smi_now, smi_old=0;
 
+    memset(&stop, 0, sizeof(stop)); /* grrr */
+    
        /* if we're running in numa mode, set our memory node */
        if (par->node != -1)
                rt_numa_set_numa_run_on_node(par->node, par->cpu);
@@ -1066,7 +1069,6 @@ static void *timerthread(void *param)
        tsnorm(&next);
 
        if (duration) {
-               memset(&stop, 0, sizeof(stop)); /* grrr */
                stop = now;
                stop.tv_sec += duration;
        }
@@ -2623,5 +2625,7 @@ int main(int argc, char **argv)
        if (affinity_mask)
                rt_bitmask_free(affinity_mask);
 
+    srand(trash); // recycling of the trash (unused return values)
+    
        exit(ret);
 }

Re: [PATCH] no compile warnings from cyclictest

[Uwe Kleine-König]

Hello,

On Sat, Jun 03, 2017 at 12:01:24PM +0200, rolf.freitag@email.de wrote:
> Hi,
> 
> compiling cyclictest gives 4 warnings of type
> 
>   warning: ignoring return value of 'write'
> 
> and three of type
> 
>   warning: ... may be used uninitialized in this function
> 
> So i eleminated the first sort with a trash variable, which adds up 
> the return values and recycles them (their entropy) at the end via
> srand(). This also avoids a write-only variable.
> I eleminated the second sort by initializing the variables.
> 
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -222,6 +222,7 @@ static int use_fifo = 0;
>  static pthread_t fifo_threadid;
>  static int laptop = 0;
>  static int use_histfile = 0;
> +static unsigned int trash; // for unused return values, to avoid compiler warnings because of ignored return values

What is the good reason to collect these in a global variable?
Ah, later in the patch you use it for srand. Is this a good idea? If so,
document it here.

Oh, and AFAIK cyclictest uses C comments, not C++.

>  #ifdef ARCH_HAS_SMI_COUNTER
>  static int smi = 0;
> @@ -491,10 +492,10 @@ static void tracemark(char *fmt, ...)
>         va_end(ap);
>  
>         /* write the tracemark message */
> -       write(tracemark_fd, tracebuf, len);
> +       trash += write(tracemark_fd, tracebuf, len);

IMHO that's not very sensible. The compiler tells you, that you ignore
the return value of write and that might be a bad thing. So the (more)
sensible options are:

 - add proper error checking and let (here) tracemark fail if the write
   fails (and retry if the return value is positive but less than len).

 - you don't care about the return value, so use:

 	(void)write(tracemark_fd, tracebuf, len)

 - keep the warnings until someone else fixes them properly.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] no compile warnings from cyclictest

[John Kacur]

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



On Sat, 3 Jun 2017, rolf.freitag@email.de wrote:

> Hi,
> 
> compiling cyclictest gives 4 warnings of type
> 
>   warning: ignoring return value of 'write'
> 
> and three of type
> 
>   warning: ... may be used uninitialized in this function
> 
> So i eleminated the first sort with a trash variable, which adds up 
> the return values and recycles them (their entropy) at the end via
> srand(). This also avoids a write-only variable.
> I eleminated the second sort by initializing the variables.
> 
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -222,6 +222,7 @@ static int use_fifo = 0;
>  static pthread_t fifo_threadid;
>  static int laptop = 0;
>  static int use_histfile = 0;
> +static unsigned int trash; // for unused return values, to avoid compiler warnings because of ignored return values
>  
>  #ifdef ARCH_HAS_SMI_COUNTER
>  static int smi = 0;
> @@ -491,10 +492,10 @@ static void tracemark(char *fmt, ...)
>         va_end(ap);
>  
>         /* write the tracemark message */
> -       write(tracemark_fd, tracebuf, len);
> +       trash += write(tracemark_fd, tracebuf, len);
>  
>         /* now stop any trace */
> -       write(trace_fd, "0\n", 2);
> +       trash += write(trace_fd, "0\n", 2);
>  }
>  
>  
> @@ -510,7 +511,7 @@ static void tracing(int on)
>                 case KV_26_LT24: prctl(0, 1); break;
>                 case KV_26_33:
>                 case KV_30:
> -                       write(trace_fd, "1", 1);
> +                       trash += write(trace_fd, "1", 1);
>                         break;
>                 default:         break;
>                 }
> @@ -520,7 +521,7 @@ static void tracing(int on)
>                 case KV_26_LT24: prctl(0, 0); break;
>                 case KV_26_33:
>                 case KV_30:
> -                       write(trace_fd, "0", 1);
> +                       trash += write(trace_fd, "0", 1);
>                         break;
>                 default:        break;
>                 }
> @@ -984,8 +985,10 @@ static void *timerthread(void *param)
>         int stopped = 0;
>         cpu_set_t mask;
>         pthread_t thread;
> -       unsigned long smi_now, smi_old;
> +       unsigned long smi_now, smi_old=0;
>  
> +    memset(&stop, 0, sizeof(stop)); /* grrr */
> +    
>         /* if we're running in numa mode, set our memory node */
>         if (par->node != -1)
>                 rt_numa_set_numa_run_on_node(par->node, par->cpu);
> @@ -1066,7 +1069,6 @@ static void *timerthread(void *param)
>         tsnorm(&next);
>  
>         if (duration) {
> -               memset(&stop, 0, sizeof(stop)); /* grrr */
>                 stop = now;
>                 stop.tv_sec += duration;
>         }
> @@ -2623,5 +2625,7 @@ int main(int argc, char **argv)
>         if (affinity_mask)
>                 rt_bitmask_free(affinity_mask);
>  
> +    srand(trash); // recycling of the trash (unused return values)
> +    
>         exit(ret);
>  }
> --

I seen to recall some old conversations on lkml where these kind of
patches to supress compiler warnings were considered worse than the 
warnings. It is possible to supress warnings that we need to deal with.
In addition I see that some of your patch touches on tracing, which
I'm planning on ripping out and replacing with the ability to do the same
kind of tracing with external programs, specifically trace-cmd.

Thanks

John