Opened 7 years ago

Last modified 3 years 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 7 years ago.
patch with some suggested error messages, edits welcome

Download all attachments as: .zip

Change History (19)

by dylan, 7 years ago

Attachment: get_row.c-patch added

patch with some suggested error messages, edits welcome

comment:1 by neteler, 7 years ago

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

comment:2 by wenzeslaus, 7 years ago

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.

in reply to:  2 comment:3 by dylan, 7 years ago

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 by neteler, 6 years ago

Milestone: 7.1.07.2.0

Milestone renamed

comment:5 by neteler, 5 years ago

Milestone: 7.2.07.2.1

Ticket retargeted after milestone closed

comment:6 by martinl, 5 years ago

Milestone: 7.2.17.2.2

comment:7 by martinl, 5 years ago

Milestone: 7.2.27.4.0

All enhancement tickets should be assigned to 7.4 milestone.

comment:8 by dylan, 4 years ago

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 by dylan, 4 years ago

Resolution: fixed
Status: newclosed

comment:10 by neteler, 4 years ago

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?

in reply to:  10 comment:11 by mmetz, 4 years ago

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 by neteler, 4 years ago

Milestone: 7.4.07.4.1

comment:13 by martinl, 4 years ago

What exactly should be backported?

in reply to:  13 comment:14 by neteler, 4 years ago

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 by neteler, 4 years ago

Milestone: 7.4.17.4.2

comment:16 by martinl, 4 years ago

Milestone: 7.4.27.6.0

All enhancement tickets should be assigned to 7.6 milestone.

comment:17 by martinl, 3 years ago

Milestone: 7.6.07.6.1

Ticket retargeted after milestone closed

comment:18 by martinl, 3 years ago

Milestone: 7.6.17.6.2

Ticket retargeted after milestone closed

Note: See TracTickets for help on using tickets.