Opened 4 years ago

Last modified 10 months ago

#2762 reopened enhancement

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

Reported by: dylan Owned by: grass-dev@…
Priority: minor Milestone: 7.6.2
Component: Raster Version: svn-trunk
Keywords: error message Cc:
CPU: All Platform: All

Description

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 r66508):

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 on a couple of 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);

Attachments (1)

get_row.c-patch (3.5 KB) - added by dylan 4 years ago.
patch with some suggested error messages, edits welcome

Download all attachments as: .zip

Change History (19)

Changed 4 years ago by dylan

Attachment: get_row.c-patch added

patch with some suggested error messages, edits welcome

comment:1 Changed 4 years ago by neteler

For the improved errors are a good idea. In the patch I would not use \n but simply a white space.

comment:2 Changed 4 years ago by wenzeslaus

As noted in #2750, it might be easier to first apply patch from #2750 and then do these changes. Otherwise this patch makes absolute sense, different messages for different cases.

comment:3 in reply to:  2 Changed 4 years ago by dylan

Replying to wenzeslaus:

As noted in #2750, it might be easier to first apply patch from #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 #2750 is addressed. I was thinking about adding some additional information such as buffer allocation vs. actual number of bytes read.

comment:4 Changed 4 years ago by neteler

Milestone: 7.1.07.2.0

Milestone renamed

comment:5 Changed 3 years ago by neteler

Milestone: 7.2.07.2.1

Ticket retargeted after milestone closed

comment:6 Changed 3 years ago by martinl

Milestone: 7.2.17.2.2

comment:7 Changed 2 years ago by martinl

Milestone: 7.2.27.4.0

All enhancement tickets should be assigned to 7.4 milestone.

comment:8 Changed 2 years ago by dylan

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

comment:9 Changed 2 years ago by dylan

Resolution: fixed
Status: newclosed

comment:10 Changed 2 years ago by neteler

Resolution: fixed
Status: closedreopened

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

comment:11 in reply to:  10 Changed 2 years ago by mmetz

Replying to 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.

comment:12 Changed 2 years ago by neteler

Milestone: 7.4.07.4.1

comment:13 Changed 2 years ago by martinl

What exactly should be backported?

comment:14 in reply to:  13 Changed 2 years ago by neteler

Replying to martinl:

What exactly should be backported?

I think these msg improvements (from #3499):

  • r72030 libraster: more informative messages when reading a row fails
  • r72031 libgis: more informative messages when (de)compression fails
  • r72042 libgis: add warnings when writing compressed data fails

comment:15 Changed 20 months ago by neteler

Milestone: 7.4.17.4.2

comment:16 Changed 16 months ago by martinl

Milestone: 7.4.27.6.0

All enhancement tickets should be assigned to 7.6 milestone.

comment:17 Changed 12 months ago by martinl

Milestone: 7.6.07.6.1

Ticket retargeted after milestone closed

comment:18 Changed 10 months ago by martinl

Milestone: 7.6.17.6.2

Ticket retargeted after milestone closed

Note: See TracTickets for help on using tickets.