is this compiler-safe?

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

is this compiler-safe?

Markus Metz-2
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
_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev
Reply | Threaded
Open this post in threaded view
|

Re: is this compiler-safe?

Paul Kelly
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
_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev
Reply | Threaded
Open this post in threaded view
|

Re: is this compiler-safe?

Markus Metz-2

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

_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev
Reply | Threaded
Open this post in threaded view
|

Re: is this compiler-safe?

Glynn Clements
In reply to this post by Markus Metz-2

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 <[hidden email]>
_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev
Reply | Threaded
Open this post in threaded view
|

Re: is this compiler-safe?

Markus Metz-2

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.

_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev
Reply | Threaded
Open this post in threaded view
|

Re: is this compiler-safe?

Markus Metz-2
In reply to this post by Glynn Clements

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
_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev
Reply | Threaded
Open this post in threaded view
|

Re: is this compiler-safe?

Glynn Clements

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 <[hidden email]>
_______________________________________________
grass-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/grass-dev