Build breaks on ignored return values

[metux]

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

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

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

[metux]

Okay, reworked it. Please review now.

[andrew_b]

What about

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

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

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

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

[metux]

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

Please review

[slavazanko]

I have commit in branch:

• cd561fff3cbe8a7e33fd168772152822f57062da: src/background.c: little fix for respect return value of read() function

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

[metux]

Replying to slavazanko:

I have commit in branch:

• cd561fff3cbe8a7e33fd168772152822f57062da: src/background.c: little fix for respect return value of read() function

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

[metux]

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

[metux]

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

But please keep an eye on this branch :)

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

[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

[metux]

okay, fixed it.
please review again.

[slavazanko]

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

[slavazanko]

• e579dad69b30f2b6473ac9f5c7761e0727f8af5b: Fixed a little bit broken logic after return values fuxup.

Vote, please, for this my latest commit.

[andrew_b]

There are memory leaks in this branch:

• in panel_operate()
• in parent_call_string()

Seems the file descriptor leak is in close_error_pipe().

[metux]

Rebased to current master. Please review.

[slavazanko]

Recreated new branch 1897_libc_respect_return_values

changesets:

• 21cfbe73566aab4cf7733b847e21b0fc70c2f3f1: Changes into lib directory
• af6a4208c31f490c822df55e623643b767e2564b: Changes into src directory
• b190ef6b6c78573b6f42d5fa85eeca1ae21a44ba: Changes into src/editor directory
• ca9a9cf4a34d02d8fdbf22d78f206610c37c976c: Changes into src/consaver directory
• ab36f139328caa190c9a19f8aad49ad05c94f332: Final Indentation of all touched files

May be, this branch looks better rathen than 1897_libc_return_values

[andrew_b]

My vote for 1897_libc_respect_return_values branch.

[slavazanko]

merge changeset:3b375cc2d3b398688c9e136aa38530e0efc5fa7e

[slavazanko]

Changes partially moved into stable: changeset:973cbffa5196ca930ba1e23e9ad02f56e9ffd84e