more informative error messages in lib/rast/get_row.c

[landam]

Reported by dylan on 15 Oct 2015 19:32 UTC In the file lib/rast/get_row.c there are several places in which the "Error reading raster data..." error message can be generated (grass_trunk https://trac.osgeo.org/grass/changeset/66508):

95:     G_fatal_error(_("Error reading raster data for row %d of <%s>"),
101:    G_fatal_error(_("Error reading raster data for row %d of <%s>"),
135:    G_fatal_error(_("Error reading raster data for row %d of <%s>"),
142:    G_fatal_error(_("Error reading raster data for row %d of <%s>"),
177:    G_fatal_error(_("Error reading raster data for row %d of <%s>"),
181:    G_fatal_error(_("Error reading raster data for row %d of <%s>"),
217:    G_fatal_error(_("Error reading raster data via GDAL for row %d of <%s>"),

It might be nice to know what exactly triggered these kind of messages, especially afer encountering these errors http://osgeo-org.1560.x6.nabble.com/Error-reading-raster-data-for-row-xxx-only-when-using-r-series-and-t-rast-series-td5229569.html on a couple] of [http://lists.osgeo.org/pipermail/grass-dev/2015-July/075627.html occasions.

I would offer some text, but I don't really understand what is going on in code like this:

if (lseek(fcb->data_fd, t1, SEEK_SET) < 0)
    G_fatal_error(_("Error reading raster data for row %d of <%s>"),
              row, fcb->name);

Something as simple as "Error reading raster data for row %d of <%s>\n file offset less than 0, ... blah blah blah" would IMHO help with debugging those modules that may have created such a file.

I would be happy to make the changes if someone is willing to comments on the meaning and implications of each case, outlined below:

if (lseek(fcb->data_fd, t1, SEEK_SET) < 0)
    G_fatal_error(_("Error reading raster data for row %d of <%s>"),
              row, fcb->name);

 if ((size_t) G_zlib_read(fcb->data_fd, readamount, data_buf, bufsize) != bufsize)
    G_fatal_error(_("Error reading raster data for row %d of <%s>"),
              row, fcb->name);

 if (read(fcb->data_fd, cmp, readamount) != readamount) {
    G_freea(cmp);
    G_fatal_error(_("Error reading raster data for row %d of <%s>"),
              row, fcb->name);
    }

if (lseek(fcb->data_fd, (off_t) row * bufsize, SEEK_SET) == -1)
    G_fatal_error(_("Error reading raster data for row %d of <%s>"),
              row, fcb->name);

if (read(fcb->data_fd, data_buf, bufsize) != bufsize)
    G_fatal_error(_("Error reading raster data for row %d of <%s>"),
              row, fcb->name);

 if (err != CE_None)
    G_fatal_error(_("Error reading raster data via GDAL for row %d of <%s>"),
              row, fcb->name);

GRASS GIS version and provenance

svn-trunk

Migrated-From: https://trac.osgeo.org/grass/ticket/2762

[landam]

Attachment from dylan on 16 Oct 2015 16:00 UTC patch with some suggested error messages, edits welcome https://trac.osgeo.org/grass/attachment/ticket/2762/get_row.c-patch

[landam]

Comment by neteler on 18 Oct 2015 18:15 UTC For the improved errors are a good idea. In the patch I would not use \n but simply a white space.

[landam]

Comment by wenzeslaus on 18 Oct 2015 18:51 UTC As noted in https://trac.osgeo.org/grass/ticket/2750, it might be easier to first apply patch from https://trac.osgeo.org/grass/ticket/2750 and then do these changes. Otherwise this patch makes absolute sense, different messages for different cases.

[landam]

Comment by dylan on 19 Oct 2015 20:14 UTC Replying to [comment:2 wenzeslaus]:

As noted in https://trac.osgeo.org/grass/ticket/2750, it might be easier to first apply patch from https://trac.osgeo.org/grass/ticket/2750 and then do these changes. Otherwise this patch makes absolute sense, different messages for different cases.

I agree, please feel free it ignore my suggested patch for now. I'll re-write it using Markus' suggestions above '''after''' https://trac.osgeo.org/grass/ticket/2750 is addressed. I was thinking about adding some additional information such as buffer allocation vs. actual number of bytes read.

[landam]

Comment by neteler on 5 May 2016 14:08 UTC Milestone renamed

[landam]

Comment by neteler on 28 Dec 2016 15:04 UTC Ticket retargeted after milestone closed

[landam]

Modified by @landam on 5 May 2017 20:41 UTC

[landam]

Comment by @landam on 1 Sep 2017 20:28 UTC All enhancement tickets should be assigned to 7.4 milestone.

[landam]

Comment by dylan on 31 Dec 2017 05:33 UTC We can probably close this ticket since Markus just added a more informative patch as of https://trac.osgeo.org/grass/ticket/2764#comment:8

[landam]

Modified by dylan on 7 Jan 2018 05:49 UTC

[landam]

Comment by neteler on 7 Jan 2018 07:57 UTC Reopening since the ticket is tagged "7.4.0" but the messages are so far only in trunk. Is a backport desired?

[landam]

Comment by mmetz on 7 Jan 2018 09:50 UTC Replying to [comment:10 neteler]:

Reopening since the ticket is tagged "7.4.0" but the messages are so far only in trunk. Is a backport desired?

This is not a bug-fix and we are at 7.4.0RC1, therefore I would recommend backporting to 7.4.1, together with the warnings added to lib/gis/compress.c and lib/gis/cmpr*.c.

[landam]

Modified by neteler on 7 Jan 2018 10:21 UTC

[landam]

Comment by @landam on 28 Feb 2018 20:46 UTC What exactly should be backported?

[landam]

Comment by neteler on 28 Feb 2018 21:39 UTC Replying to [comment:13 martinl]:

What exactly should be backported?

I think these msg improvements (from https://trac.osgeo.org/grass/ticket/3499):

• https://trac.osgeo.org/grass/changeset/72030 libraster: more informative messages when reading a row fails
• https://trac.osgeo.org/grass/changeset/72031 libgis: more informative messages when (de)compression fails
• https://trac.osgeo.org/grass/changeset/72042 libgis: add warnings when writing compressed data fails

[landam]

Modified by neteler on 12 Jun 2018 20:48 UTC

[landam]

Comment by @landam on 25 Sep 2018 16:53 UTC All enhancement tickets should be assigned to 7.6 milestone.

[landam]

Comment by @landam on 25 Jan 2019 21:08 UTC Ticket retargeted after milestone closed