Ticket #1897 (closed defect: fixed)

Opened 9 years ago

Last modified 9 years ago

Build breaks on ignored return values

Reported by: metux Owned by: slavazanko
Priority: blocker Milestone: 4.7.2
Component: mc-core Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: Votes for changeset: committed-master

Description (last modified by metux) (diff)

glibc has a lot of functions whose return values should not be ignored. this produces a lot of warnings, which cause the build to break when using -Werror (which *really* should be standard for any production build ;-p):

background.c:446: error: ignoring return value of 'write', declared with attribute warn_unused_result

(setting the priority to blocker, since it breaks any serious build)

branch:1897_libc_return_values
changeset:b396e0e959179f85e2a10c6d2cffb18879025e0b

Change History

comment:1 follow-up: ↓ 2 Changed 9 years ago by andrew_b

Branch: 1897_libc_return_values.

-       fscanf (f, ";\n");
+       if (fscanf (f, ";\n"));

Where you've seen such way? The widely used way to suppress warings of unsed return values is

       (void) fscanf (f, ";\n");

I think we should use a common coding styles in MC codebase.

comment:2 in reply to: ↑ 1 Changed 9 years ago by slyfox

Replying to andrew_b:

Branch: 1897_libc_return_values.

-       fscanf (f, ";\n");
+       if (fscanf (f, ";\n"));

Where you've seen such way?

Agreed, looks ambiguous, odd and (not yet) generates another warning:

warning: suggest braces around empty body in an 'if' statement [-Wempty-body]

The widely used way to suppress warings of unsed return values is

       (void) fscanf (f, ";\n");

I think we should use a common coding styles in MC codebase.

Agreed.

comment:3 Changed 9 years ago by metux

  • Status changed from new to accepted
  • Version changed from version not selected to master
  • Description modified (diff)
  • severity changed from no branch to on review

Okay, reworked it. Please review now.

comment:4 follow-up: ↓ 5 Changed 9 years ago by andrew_b

What about

(void) fscanf (f, ";\n");

comment:5 in reply to: ↑ 4 ; follow-up: ↓ 6 Changed 9 years ago by metux

Replying to andrew_b:

What about

(void) fscanf (f, ";\n");

Doesnt work, still get the warnings with that.
The best way seems to be:

if (ffscanf (f, ";\n")) {}

Changed it this way in the branch.

Please review now. We should get in these fixes (also the #1872 branch) ASAP, since master is still broken.

comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 9 years ago by andrew_b

Replying to metux:

The best way seems to be:

if (ffscanf (f, ";\n")) {}

The best way is not ignore return values. :p

Please review now. We should get in these fixes (also the #1872 branch) ASAP, since master is still broken.

Actually, master is not broken.

comment:7 in reply to: ↑ 6 Changed 9 years ago by metux

Replying to andrew_b:

Replying to metux:

The best way seems to be:

if (ffscanf (f, ";\n")) {}

The best way is not ignore return values. :p

Right. So everybody please review and think carefully
think about whether/how to handle the results, case
by case.

Please review now. We should get in these fixes (also the #1872 branch) ASAP, since master is still broken.

Actually, master is not broken.

It doesnt build in several configs (not just due -Werror!), eg.
--without-edit or --without-vfs totally break.

comment:8 Changed 9 years ago by metux

Okay boys, I've heavily reworked it.
Now the errors are actually handled.

Please review

comment:9 follow-up: ↓ 11 Changed 9 years ago by slavazanko

  • Votes for changeset set to slavazanko

I have commit in branch:

Except this, all looks fine. Enrico, you made big work. Thanks.

comment:10 Changed 9 years ago by slavazanko

  • Milestone changed from 4.7 to 4.7.1

comment:11 in reply to: ↑ 9 Changed 9 years ago by metux

Replying to slavazanko:

I have commit in branch:

Except this, all looks fine. Enrico, you made big work. Thanks.

Sorry, overlooked it when rewriting from just-ignore to actually-handling ;-o

All others folks: please review now, we need to get that in asap ;-p

comment:12 Changed 9 years ago by metux

  • Description modified (diff)

Master had several changes, so rebased it and fixed a few more of these issues.

comment:13 Changed 9 years ago by slavazanko

  • Votes for changeset slavazanko deleted

comment:14 Changed 9 years ago by metux

  • Blocking 1818 added

(In #1818) It's not ready for merge yet, since it's based on #1897.

But please keep an eye on this branch :)

comment:15 Changed 9 years ago by angel_il

please fix code like this

598  	    if (mc_write (file, (char *) text, strlen (text)) == -1) 
599 	        return FALSE; 
600 	 
601	    mc_close (file)

need close file before return.
sorry but i can't vote for this...

comment:16 Changed 9 years ago by angel_il

2274  	        system (tmp);   	2274  	        if (system (tmp) == -1) { 
  	  	2275 	            edit_error_dialog (_("Process block"),_("Error calling program")); 
  	  	2276 	            goto edit_block_process_cmd__EXIT; 
  	  	2277 	        } 
2275	        g_free(tmp);  	2278	        g_free(tmp); 

need free tmp

comment:17 Changed 9 years ago by angel_il

  • severity changed from on review to on rework

comment:18 Changed 9 years ago by metux

  • severity changed from on rework to on review

okay, fixed it.
please review again.

comment:19 Changed 9 years ago by slavazanko

  • Blocking 1902 added

(In #1902) Branch 1897_libc_return_values have commit:

Author: Enrico Weigelt, metux IT service <weigelt@metux.de>
Date:   Mon Dec 28 23:56:16 2009 +0100

    VFS: mcserv: denying access when chroot required but failed

diff --git a/vfs/mcserv.c b/vfs/mcserv.c
index 23f7ffe..68a4548 100644
--- a/vfs/mcserv.c
+++ b/vfs/mcserv.c
@@ -1016,7 +1016,8 @@ do_auth (const char *username, const char *password)
        return 0;

     if (strcmp (username, "ftp") == 0)
-       chroot (this->pw_dir);
+       if (chroot (this->pw_dir) == -1)
+           return 0;

     endpwent ();
     return auth;

Looks like this problem.

comment:19 Changed 9 years ago by slavazanko

  • Blocking 1902 removed

Vote, please, for this my latest commit.

comment:20 Changed 9 years ago by metux

  • Votes for changeset set to metux

comment:21 Changed 9 years ago by andrew_b

  • Votes for changeset metux deleted
  • severity changed from on review to on rework

There are memory leaks in this branch:

  • in panel_operate()
  • in parent_call_string()

Seems the file descriptor leak is in close_error_pipe().

comment:22 Changed 9 years ago by metux

  • severity changed from on rework to on review

Rebased to current master. Please review.

comment:23 Changed 9 years ago by slavazanko

Recreated new branch 1897_libc_respect_return_values

changesets:

May be, this branch looks better rathen than 1897_libc_return_values

comment:24 Changed 9 years ago by andrew_b

  • Owner changed from metux to slavazanko
  • Status changed from accepted to assigned
  • Votes for changeset set to andrew_b
  • Milestone changed from 4.7.1 to 4.7.2

My vote for 1897_libc_respect_return_values branch.

comment:25 Changed 9 years ago by angel_il

  • Votes for changeset changed from andrew_b to andrew_b angel_il
  • severity changed from on review to approved

comment:26 Changed 9 years ago by slavazanko

  • Status changed from assigned to testing
  • Votes for changeset changed from andrew_b angel_il to committed-master
  • Resolution set to fixed
  • severity changed from approved to merged

comment:27 Changed 9 years ago by slavazanko

  • Status changed from testing to closed

Changes partially moved into stable: changeset:973cbffa5196ca930ba1e23e9ad02f56e9ffd84e

comment:28 Changed 8 years ago by andrew_b

  • Blocking 1818 removed
Note: See TracTickets for help on using tickets.