[GRASS-dev] is this compiler-safe?

GRASS GRASS developer
1

I found two code snippets in the segment library (all branches) that look fishy to me:

the first is in relbr6 here
https://trac.osgeo.org/grass/browser/grass/branches/releasebranch_6_4/lib/segment/format.c#L135

should
    if (lseek(fd, 0L, SEEK_SET) == (off_t) - 1) {

not be
    if (lseek(fd, 0L, SEEK_SET) == (off_t) -1) {

because I can't see where 1 is subtracted from? Introduced with automated indentation in r32527

the second is in relbr6 here:
https://trac.osgeo.org/grass/browser/grass/branches/releasebranch_6_4/lib/segment/format.c#L178

    if ((bytes_wrote = write(fd, &x, sizeof(int)) == sizeof(int)) < 0)

I am missing parentheses somewhere and would rather use

    if ((bytes_wrote = write(fd, &x, sizeof(int))) != sizeof(int))
    G_warning("%s", strerror(errno));

    return (bytes_wrote == sizeof(int));

just to be on the safe side

2

On Wed, 10 Feb 2010, Markus Metz wrote:

I found two code snippets in the segment library (all branches) that look fishy to me:

the first is in relbr6 here
https://trac.osgeo.org/grass/browser/grass/branches/releasebranch_6_4/lib/segment/format.c#L135

should
  if (lseek(fd, 0L, SEEK_SET) == (off_t) - 1) {

not be
  if (lseek(fd, 0L, SEEK_SET) == (off_t) -1) {

because I can't see where 1 is subtracted from? Introduced with automated indentation in r32527

Hello Markus,
Well, looks like a bug in the indent program that got confused by the cast and thought the - was being used as a binary rather than unary operator. But is it not only an aesthetic problem? As far as I can see the code should do exactly the same thing?

the second is in relbr6 here:
https://trac.osgeo.org/grass/browser/grass/branches/releasebranch_6_4/lib/segment/format.c#L178

  if ((bytes_wrote = write(fd, &x, sizeof(int)) == sizeof(int)) < 0)

I am missing parentheses somewhere and would rather use

  if ((bytes_wrote = write(fd, &x, sizeof(int))) != sizeof(int))
  G_warning("%s", strerror(errno));

  return (bytes_wrote == sizeof(int));

just to be on the safe side

Yes I suppose you saw too that the code that calls this function assumes if the return value is non-zero that it succeeded, which doesn't seem right. I would suggest to make it even clearer by removing the redundant variable x, correcting the grammar (written instead of wrote) and rearranging:

Index: format.c

--- format.c (revision 40905)
+++ format.c (working copy)
@@ -170,15 +170,12 @@

  static int write_int(int fd, int n)
  {
- int x;
- int bytes_wrote;
+ int bytes_written = write(fd, &n, sizeof(int));

- x = n;
-
- if ((bytes_wrote = write(fd, &x, sizeof(int)) == sizeof(int)) < 0)
+ if (bytes_written != sizeof(int))
         G_warning("%s", strerror(errno));

- return bytes_wrote;
+ return (bytes_written == sizeof(int));
  }

I think that is yet easier to read - do you agree?

Paul

3

Paul Kelly wrote:

Markus Metz wrote:

should
  if (lseek(fd, 0L, SEEK_SET) == (off_t) - 1) {

not be
  if (lseek(fd, 0L, SEEK_SET) == (off_t) -1) {

Hello Markus,
Well, looks like a bug in the indent program that got confused by the cast and thought the - was being used as a binary rather than unary operator. But is it not only an aesthetic problem? As far as I can see the code should do exactly the same thing?

I am not sure if the code would do exactly the same thing with other compilers than gcc, but that's beyond me.

  if ((bytes_wrote = write(fd, &x, sizeof(int)) == sizeof(int)) < 0)

I am missing parentheses somewhere and would rather use

  if ((bytes_wrote = write(fd, &x, sizeof(int))) != sizeof(int))
  G_warning("%s", strerror(errno));

  return (bytes_wrote == sizeof(int));

just to be on the safe side

Yes I suppose you saw too that the code that calls this function assumes if the return value is non-zero that it succeeded, which doesn't seem right. I would suggest to make it even clearer by removing the redundant variable x, correcting the grammar (written instead of wrote) and rearranging:

Index: format.c

--- format.c (revision 40905)
+++ format.c (working copy)
@@ -170,15 +170,12 @@

static int write_int(int fd, int n)
{
- int x;
- int bytes_wrote;
+ int bytes_written = write(fd, &n, sizeof(int));

- x = n;
-
- if ((bytes_wrote = write(fd, &x, sizeof(int)) == sizeof(int)) < 0)
+ if (bytes_written != sizeof(int))
        G_warning("%s", strerror(errno));

- return bytes_wrote;
+ return (bytes_written == sizeof(int));
}

I think that is yet easier to read - do you agree?

Yes, that looks much better! AFAICT, ready to commit.

Markus M

4

Markus Metz wrote:

I found two code snippets in the segment library (all branches) that
look fishy to me:

the first is in relbr6 here
https://trac.osgeo.org/grass/browser/grass/branches/releasebranch_6_4/lib/segment/format.c#L135

should
    if (lseek(fd, 0L, SEEK_SET) == (off_t) - 1) {

not be
    if (lseek(fd, 0L, SEEK_SET) == (off_t) -1) {

The two are equivalent; the whitespace isn't significant here.

because I can't see where 1 is subtracted from? Introduced with
automated indentation in r32527

There is no subtraction; the "-" is treated as a unary minus
(negation) operator in this context. If you were to change it to
something which cannot be interpreted as a unary operator
(e.g. / or %), you would get an error.

the second is in relbr6 here:
https://trac.osgeo.org/grass/browser/grass/branches/releasebranch_6_4/lib/segment/format.c#L178

    if ((bytes_wrote = write(fd, &x, sizeof(int)) == sizeof(int)) < 0)

Ouch. There's no way that that can make sense. As it stands, it's
parsed as:

      if ((bytes_wrote = (write(fd, &x, sizeof(int)) == sizeof(int))) < 0)

i.e. bytes_wrote will be either 0 or 1, so the "... < 0" will always
be false.

I am missing parentheses somewhere and would rather use

    if ((bytes_wrote = write(fd, &x, sizeof(int))) != sizeof(int))
    G_warning("%s", strerror(errno));

    return (bytes_wrote == sizeof(int));

I would use:

  static int write_int(int fd, int n)
  {
      if (write(fd, &n, sizeof(int)) != sizeof(int)) {
    G_warning("%s", strerror(errno));
    return 0;
      }
  
      return 1;
  }

There's no need to make a copy of "n"; it's already a copy of the
actual argument.

Actually, looking at the code in context, I'd write:

  static void write_int(int fd, int n)
  {
      if (write(fd, &n, sizeof(int)) != sizeof(int))
    G_fatal_error("%s", strerror(errno));
  }

Lets face it: you aren't going to be recovering from this situation,
are you? If _segment_format fails, the module fails.

While you're there, can you fix this nonsense:

  if (file_size > INT_MAX) {
?

OTOH, the segment library is long overdue for being replaced with
something more efficient.

--
Glynn Clements <glynn@gclements.plus.com>

5

Glynn Clements wrote:

Actually, looking at the code in context, I'd write:

  static void write_int(int fd, int n)
  {
      if (write(fd, &n, sizeof(int)) != sizeof(int))
    G_fatal_error("%s", strerror(errno));
  }

Lets face it: you aren't going to be recovering from this situation,
are you? If _segment_format fails, the module fails.
  

Yup. Or worse, the module finishes and produces corrupt output. OTOH, _segment_format() is supposed to return -1 on seek or write failure and it's up to modules calling segment_format() or segment_format_nofill() to check the return value.

While you're there, can you fix this nonsense:

  if (file_size > INT_MAX) {
?
  

Sure, that should be handled by write() I guess.

OTOH, the segment library is long overdue for being replaced with
something more efficient.
  

Try grass7. Not a replacement but optimization.

6

Glynn Clements wrote:

I would use:

  static int write_int(int fd, int n)
  {
      if (write(fd, &n, sizeof(int)) != sizeof(int)) {
    G_warning("%s", strerror(errno));
    return 0;
      }
  
      return 1;
  }

fixed in trunk r40910, devbr r40911 and relbr r40912

While you're there, can you fix this nonsense:

  if (file_size > INT_MAX) {
?
  

also fixed, removed

Any suggestions on how to compute required file size on 32bit OS with sizeof(off_t) = 4 ?
e.g.
(double) nrows * ncols * sizeof(DCELL)
or
(long long int) nrows * ncols * sizeof(DCELL)
more generic
number_of_items * sizeof(item)

and safely compare that to 2^31 (2GB)?

the segment library and the vector libs in grass7 would need such a test.

Thanks,

Markus M

7

Markus Metz wrote:

Any suggestions on how to compute required file size on 32bit OS with
sizeof(off_t) = 4 ?
e.g.
(double) nrows * ncols * sizeof(DCELL)
or
(long long int) nrows * ncols * sizeof(DCELL)
more generic
number_of_items * sizeof(item)

and safely compare that to 2^31 (2GB)?

Use double, cast to off_t, check that the two are equal, e.g.

  double d_size = (double) nrows * ncols * sizeof(DCELL);
  off_t size = (off_t) d_size;

  if ((double) size != d_size)
      G_fatal_error(_("File too large"));

--
Glynn Clements <glynn@gclements.plus.com>