Fix warnings #95
Fix warnings #95
Conversation
| @@ -221,17 +221,25 @@ int send_dirlists_request(int fd, char **local_result, char **exece_result, char | |||
| client_recv_msg_dynamic(fd, &message, LDCS_READ_BLOCK); | |||
| COMM_UNLOCK; | |||
|
|
|||
| debug_printf("QQQ After client_recv_msg_dynamic, message.header.len=%zu.\n", message.header.len); | |||
There was a problem hiding this comment.
Clean up debug printfs through here. If these are generally useful, then format as normal and make them debug_printf3. If not generally useful, then delete.
| #include "sheep.h" | ||
| #include "spindle_launch.h" | ||
|
|
||
| typedef struct { | ||
| volatile unsigned long *lock; | ||
| volatile unsigned long *held_by; | ||
| volatile pid_t *lock; |
There was a problem hiding this comment.
Leave 'lock' an unsigned long (or a uint64_t). Same with the other 'lock' variables below.
held_by can remain a pid_t
| @@ -347,7 +347,7 @@ realpath_stk (const char *name, char *resolved, struct realpath_bufs *bufs) | |||
|
|
|||
| /* Length of this file name component; it can be zero if a file | |||
| name ends in '/'. */ | |||
| long startlen = end - start; | |||
| size_t startlen = end - start; | |||
There was a problem hiding this comment.
This file is copied from the libc implementation of realpath (we're both LGPL, so that's okay). Would rather leave the body of libc's realpath as unmodified as possible.
| @@ -365,7 +365,7 @@ realpath_stk (const char *name, char *resolved, struct realpath_bufs *bufs) | |||
| if (!ISSLASH (dest[-1])) | |||
| *dest++ = '/'; | |||
|
|
|||
| while (rname + bufs->rname.length - dest | |||
| while ( (size_t)(rname + bufs->rname.length - dest) | |||
There was a problem hiding this comment.
Same as above for not modifying realpath if not necessary
| static int reliable_read(int fd, void *buf, size_t size); | ||
| static int read_key(char *key_filepath, int key_length_bytes); | ||
| static size_t reliable_write(int fd, const void *buf, size_t size, int *err); | ||
| static size_t reliable_read(int fd, void *buf, size_t size, int *err); |
There was a problem hiding this comment.
Not sure that the 'int *err' is the way to go here. The INTERNAL error that this returns is the error code for something going wrong in the spindle code, implying a spindle bug. But we don't really know the level of error in this kind of low level code. There's non-spindle-bug situations where reliable_read and reliable_write fail.
We could do better error returns by going to a ssize_t return and returning -1 from these. Then let the higher level code decide what kind of error it is.
| int _ldcs_write_pipe(int fd, const void *data, int bytes ); | ||
| int _ldcs_read_pipe(int fd, void *data, int bytes, ldcs_read_block_t block ); | ||
| size_t _ldcs_write_pipe(int fd, const void *data, size_t bytes, int *err ); | ||
| size_t _ldcs_read_pipe(int fd, void *data, size_t bytes, ldcs_read_block_t block, int *err ); |
There was a problem hiding this comment.
Similar concerns here, though there's larger issues with adding err to ldcs_read_pipe and ldcs_write_pipe. The implementations below set err even when there's soft errors that the functions recover from. But the calling code treats all contents of *err as a hard error.
Again, suggest making the return ssize_t and using a -1 return on error.
| if(bread<0) { | ||
| errno=0; | ||
| rc = read(fd, dataptr, btoread); | ||
| *err = errno; |
There was a problem hiding this comment.
As an example from the above review comment, here we set *err even if it's not a hard error.
| @@ -1440,6 +1442,7 @@ static int handle_request_file(ldcs_process_data_t *procdata, node_peer_t from, | |||
| case REQ_DIRECTORY: | |||
| dir_result = handle_send_query(procdata, dirname, 1, 0); | |||
| add_requestor(procdata->file_requests, dirname, from); | |||
| [[ fallthrough ]]; /* Requires gcc >= 10.0 or -std=c23 or ... */ | |||
There was a problem hiding this comment.
Can we ifdef this to the right gcc versions?
| } while (result == -1 && errno == EINTR); | ||
| if (result == -1) | ||
| return -1; | ||
| } while (errno == EINTR); |
There was a problem hiding this comment.
Needs the (result == -1) part of the check. errno only has meaning if the function returns -1
Fixes warnings generated by gcc 15.1.0 and
-Wall -Wextra -Werror.The vast majority are either unused parameters or comparing incompatible types (signed vs unsigned). I handled the former by adding
foo=foo;(or, in C++ code, removing the parameter name from the function signature). The latter could be a bit more involved, but generally if a variable contains a count of things in memory, it's nowsize_t, with a few workarounds to handle glibc calls that were defined in less suspicious times.A single case of intentional fallthrough is now marked with
[[ fallthrough ]]. This feature has been available in gcc since v10.0 and is now part of the C23 standard. If we have users with older compilers, there are other possible workarounds.The single (fixed) bug uncovered was an unintentional fallthrough.
I can chop this up into smaller (per file?) chunks if that's useful.
Fixes #94