Skip to content

Conversation

@rountree
Copy link

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 now size_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

typedef struct {
volatile unsigned long *lock;
volatile unsigned long *held_by;
volatile pid_t *lock;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave 'lock' an unsigned long (or a uint64_t). Same with the other 'lock' variables below.

held_by can remain a pid_t

*dest++ = '/';

while (rname + bufs->rname.length - dest
while ( (size_t)(rname + bufs->rname.length - dest)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above for not modifying realpath if not necessary

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 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to take a look at this one. Here's the code in the new PR (I haven't changed the logic):

size_t _ldcs_write_pipe(int fd, const void *data, int bytes ) { 
  size_t      bsumwrote;
  ssize_t     bwrite, bwrote, left;
  char       *dataptr;
  
  left      = bytes;
  bsumwrote = 0;
  dataptr   = (char*) data;

  while (left > 0) {
    bwrite     = left;
    bwrote     = write(fd, dataptr, bwrite);
    left      -= bwrote;
    dataptr   += bwrote;
    bsumwrote += bwrote;
  }
  return (bsumwrote);
}

If write() returns -1, the left variable increases and we won't exit until left rolls over.

The _read has a similar issue, in that errors will return 0 rather that percolate the error value up.

size_t _ldcs_read_pipe(int fd, void *data, int bytes, ldcs_read_block_t block ) {
...
    bread      = read(fd, dataptr, btoread);
    if(bread<0) {
       if( (errno==EAGAIN) || (errno==EWOULDBLOCK) ) {
          if (print_count-- > 0)
             debug_printf3("read from fifo: got EAGAIN or EWOULDBLOCK\n");
          if(block==LDCS_READ_NO_BLOCK)
             return 0;
          else
             continue;
       } else { 
          debug_printf3("read from fifo: %ld bytes ... errno=%d (%s)\n",bread,errno,strerror(errno));
       }
    } else {
       debug_printf3("read from fifo: %ld bytes ...\n",bread);
    }
...

The -Wsign-compare warnings are taken care of, but we might want to revisit the underlying code.

if(bread<0) {
errno=0;
rc = read(fd, dataptr, btoread);
*err = errno;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an example from the above review comment, here we set *err even if it's not a hard error.

} while (result == -1 && errno == EINTR);
if (result == -1)
return -1;
} while (errno == EINTR);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs the (result == -1) part of the check. errno only has meaning if the function returns -1

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weirdly enough, fread() doesn't return -1 on error, and doesn't set errno. That might not be what you want.

@rountree
Copy link
Author

Will close this PR as soon as the rest of the discussions are resolved. The new PR is gcc-15.1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clean up warnings

4 participants