Bug 71631 - [patch] cleanup of the usr.sbin/pppctl code
Summary: [patch] cleanup of the usr.sbin/pppctl code
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 5.3-BETA3
Hardware: Any Any
: Normal Affects Some People
Assignee: freebsd-bugs mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-09-12 03:40 UTC by Dan Lukes
Modified: 2017-11-05 21:04 UTC (History)
1 user (show)

See Also:


Attachments
file.diff (717 bytes, patch)
2004-09-12 03:40 UTC, Dan Lukes
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Lukes 2004-09-12 03:40:30 UTC
	There are more than 5000 warnings issued during "make buildworld".
Some of them are false positives, but some of them are sign of true errors.

	Nobody is upset by warnings due it's amount, so some errors remain
uncorrected.

	I want to cleanup the code-base from warnings, so warnings will
become "attention mark" again.

usr.sbin/pppctl/pppctl.c:368: warning: 'fd' might be used uninitialized in this function
'fd' is initialized with no exception, so turn-off warning only

How-To-Repeat: 	N/A
Comment 1 Giorgos Keramidas freebsd_committer 2004-09-12 20:50:50 UTC
On 2004-09-12 04:38, Dan Lukes <dan@obluda.cz> wrote:
> usr.sbin/pppctl/pppctl.c:368: warning: 'fd' might be used uninitialized in this function
> 'fd' is initialized with no exception, so turn-off warning only

It can't really.  But this is not a good fix for the warning:

> -     int n, arg, fd, len, verbose, save_errno, hide1, hide1off, hide2;
> +     int n, arg, len, verbose, save_errno, hide1, hide1off, hide2;
> +     int fd = fd;	/* init to avoid "might be used unitialized" warning 8/

fd = -1; would be a better initialization, since no valid descriptor can
ever be negative and this will expose any bugs that using fd before a
proper initialization can trigger.

The following patch fixes a few other warnings too, and allows me to build
pppctl with WARNS?=6 in today's 6.0-CURRENT:

%%%
Index: pppctl.c
===================================================================
RCS file: /home/ncvs/src/usr.sbin/pppctl/pppctl.c,v
retrieving revision 1.32
diff -u -r1.32 pppctl.c
--- pppctl.c	7 Dec 2003 08:39:29 -0000	1.32
+++ pppctl.c	12 Sep 2004 19:47:32 -0000
@@ -77,7 +77,7 @@
  * How to use pppctl...
  */
 static int
-usage()
+usage(void)
 {
     fprintf(stderr, "usage: pppctl [-v] [-t n] [-p passwd] "
             "Port|LocalSock [command[;command]...]\n");
@@ -93,7 +93,7 @@
  * Handle the SIGALRM received due to a connect() timeout.
  */
 static void
-Timeout(int Sig)
+Timeout(int Sig __unused)
 {
     TimedOut = 1;
 }
@@ -103,10 +103,12 @@
  * All the work is done in Receive() below.
  */
 static char *
-GetPrompt(EditLine *e)
+GetPrompt(EditLine *e __unused)
 {
+    static char empty_prompt[] = "";
+
     if (prompt == NULL)
-        prompt = "";
+        prompt = empty_prompt;
     return prompt;
 }
 
@@ -119,6 +121,7 @@
 Receive(int fd, int display)
 {
     static char Buffer[LINELEN];
+    static char empty_prompt[] = "";
     struct timeval t;
     int Result;
     char *last;
@@ -173,7 +176,7 @@
                 Result = 0;
             break;
         } else
-            prompt = "";
+            prompt = empty_prompt;
         if (len == sizeof Buffer - 1) {
             int flush;
             if ((last = strrchr(Buffer, '\n')) == NULL)
@@ -202,7 +205,7 @@
  * Note, this is a signal handler - be careful of what we do !
  */
 static void
-InputHandler(int sig)
+InputHandler(int sig __unused)
 {
     static char buf[LINELEN];
     struct timeval t;
@@ -365,9 +368,11 @@
 main(int argc, char **argv)
 {
     struct sockaddr_un ifsun;
-    int n, arg, fd, len, verbose, save_errno, hide1, hide1off, hide2;
+    int fd = -1;
+    int n, arg, len, verbose, save_errno, hide1, hide1off, hide2;
     unsigned TimeoutVal;
-    char *DoneWord = "x", *next, *start;
+    char DoneWord[] = "x";
+    char *next, *start;
     struct sigaction act, oact;
     void *thread_ret;
     pthread_t mon;
@@ -431,7 +436,7 @@
                             hide1off, argv[harg]);
           else
             n = 0;
-          if (n < 0 || n >= sizeof title - pos)
+          if (n < 0 || (size_t)n >= sizeof title - pos)
             break;
           pos += n;
         }
@@ -553,7 +558,7 @@
     len = 0;
     Command[sizeof(Command)-1] = '\0';
     for (arg++; arg < argc; arg++) {
-        if (len && len < sizeof(Command)-1)
+        if (len && (size_t) len < sizeof(Command)-1)
             strcpy(Command+len++, " ");
         strncpy(Command+len, argv[arg], sizeof(Command)-len-1);
         len += strlen(Command+len);
%%%
Comment 2 Dan Lukes 2004-09-13 02:11:47 UTC
On Sun, 12 Sep 2004, Giorgos Keramidas wrote:

>> -     int n, arg, fd, len, verbose, save_errno, hide1, hide1off, hide2;
>> +     int n, arg, len, verbose, save_errno, hide1, hide1off, hide2;
>> +     int fd = fd;	/* init to avoid "might be used unitialized" warning 8/
>
> fd = -1; would be a better initialization, since no valid descriptor can
> ever be negative and this will expose any bugs that using fd before a
> proper initialization can trigger.

 	But unnecesarry over-initialisation is waste of resources.

 	It's about decision ...

 	I have no opinion which is better (generally).
Comment 3 Giorgos Keramidas freebsd_committer 2004-09-13 06:56:42 UTC
On 2004-09-13 03:11, Dan Lukes <dan@obluda.cz> wrote:
> On Sun, 12 Sep 2004, Giorgos Keramidas wrote:
> >>-     int n, arg, fd, len, verbose, save_errno, hide1, hide1off, hide2;
> >>+     int n, arg, len, verbose, save_errno, hide1, hide1off, hide2;
> >>+     int fd = fd;	/* init to avoid "might be used unitialized" warning 8/
> >
> >fd = -1; would be a better initialization, since no valid descriptor can
> >ever be negative and this will expose any bugs that using fd before a
> >proper initialization can trigger.
>
> 	But unnecesarry over-initialisation is waste of resources.
> 	It's about decision ...

Setting `int fd = fd' is also an initialiation that is a waste of
resources.  I see no reason why it's better than an initialization
that also buys us some safety.
Comment 4 Dan Lukes 2004-09-13 07:37:55 UTC
> Setting `int fd = fd' is also an initialiation that is a waste of
> resources.  

	Not true as it's optimized-out despite of O level. It's
hack/information for compiler, not real assignment statement.

> I see no reason why it's better than an initialization
> that also buys us some safety.

	Agree. Lets initialise fd to -1

Dan
Comment 5 Enji Cooper freebsd_committer 2015-11-10 11:13:54 UTC
This patch isn't correct. I'll take this PR and fix the warnings.
Comment 6 Enji Cooper freebsd_committer 2017-11-05 21:04:29 UTC
I have no way to test this. Untaking.