Version: 1.2.0_devel (using KDE KDE 3.4.0) OS: Linux When there is a problem with a dirfile, currently kst silently ignores most problems, causing confusion for the user. It would be better if at the very least kst popped up a dialog warning the user that certain vectors could not be loaded etc. because of an error(s) in the Format file or missing raw files etc. In the case of a malformed format file, it would be nice if kst (getdata) would be able to read data from entries that are formed correctly.
There are also a fair number of exit(0)s in getdata.c connected to error conditions which might be inconvenient while using getdata in our data analysis libraries.
Can you attach some testcases? That will make it much easier to pick off the bugs. Basically we need to just pass them to KstDebug though. If you guys want to do that your self, feel free to do it. If they are sent with log level "Error" then the indicator will appear in the Kst statusbar too.
SVN commit 469654 by staikos: remove the exit(0) and replace with abort() or a runtime check as appropriate BUG: 111571 M +11 -6 getdata.c --- trunk/extragear/graphics/kst/kst/datasources/dirfile/getdata.c #469653:469654 @@ -956,12 +956,11 @@ break; default: printf("Unexpected bad type error in AllocTmpbuff (%c)\n",type); - exit(0); + abort(); break; } if ((type != 'n') && (buff==NULL)) { printf("Memory Allocation error in AllocTmpbuff\n"); - exit(0); } return(buff); } @@ -1029,7 +1028,7 @@ break; default: printf("Another impossible error\n"); - exit(0); + abort(); break; } } @@ -1082,7 +1081,7 @@ break; default: printf("Unexpected bad type error in AddData\n"); - exit(0); + abort(); break; } } @@ -1136,7 +1135,7 @@ break; default: printf("Unexpected bad type error in MultiplyData\n"); - exit(0); + abort(); break; } } @@ -1192,6 +1191,9 @@ if (*error_code != GD_E_OK) return(1); tmpbuf = AllocTmpbuff(return_type, *n_read * (int)ceil((double)spf2 / spf1)); + if (!tmpbuf && return_type != 'n') { + return(0); + } num_samp = (*n_read * spf2 / spf1) % spf2; @@ -1261,6 +1263,9 @@ if (*error_code != GD_E_OK) return(1); tmpbuf = AllocTmpbuff(return_type, *n_read * (int)ceil((double)spf2 / spf1)); + if (!tmpbuf && return_type != 'n') { + return(0); + } n_read2 = DoField(F, M->in_fields[1], first_frame, first_samp, @@ -1469,7 +1474,7 @@ break; default: printf("Another impossible error\n"); - exit(0); + abort(); break; } }
I'm not sure that changing exit to abort really addresses this bug. As I read it the point was to ensure that the data source doesn't simply die as the result of some format error, which could be very annoying for the user. At worst the data source should simply return an error code or refuse to read any data. At best the user would receive some meaningful error message that would allow them to correct the problem. -----Original Message----- From: George Staikos [mailto:staikos@kde.org] Sent: Tuesday, October 11, 2005 2:17 PM To: kst@kde.org Subject: [Kst] [Bug 111571] dirfile data source poorly handles errorconditions. ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee. http://bugs.kde.org/show_bug.cgi?id=111571 staikos kde org changed: What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Resolution| |FIXED ------- Additional Comments From staikos kde org 2005-10-11 23:17 ------- SVN commit 469654 by staikos: remove the exit(0) and replace with abort() or a runtime check as appropriate BUG: 111571 M +11 -6 getdata.c --- trunk/extragear/graphics/kst/kst/datasources/dirfile/getdata.c #469653:469654 @ -956,12 +956,11 @ break; default: printf("Unexpected bad type error in AllocTmpbuff (%c)\n",type); - exit(0); + abort(); break; } if ((type != 'n') && (buff==NULL)) { printf("Memory Allocation error in AllocTmpbuff\n"); - exit(0); } return(buff); } @ -1029,7 +1028,7 @ break; default: printf("Another impossible error\n"); - exit(0); + abort(); break; } } @ -1082,7 +1081,7 @ break; default: printf("Unexpected bad type error in AddData\n"); - exit(0); + abort(); break; } } @ -1136,7 +1135,7 @ break; default: printf("Unexpected bad type error in MultiplyData\n"); - exit(0); + abort(); break; } } @ -1192,6 +1191,9 @ if (*error_code != GD_E_OK) return(1); tmpbuf = AllocTmpbuff(return_type, *n_read * (int)ceil((double)spf2 / spf1)); + if (!tmpbuf && return_type != 'n') { + return(0); + } num_samp = (*n_read * spf2 / spf1) % spf2; @ -1261,6 +1263,9 @ if (*error_code != GD_E_OK) return(1); tmpbuf = AllocTmpbuff(return_type, *n_read * (int)ceil((double)spf2 / spf1)); + if (!tmpbuf && return_type != 'n') { + return(0); + } n_read2 = DoField(F, M->in_fields[1], first_frame, first_samp, @ -1469,7 +1474,7 @ break; default: printf("Another impossible error\n"); - exit(0); + abort(); break; } } _______________________________________________ Kst mailing list Kst@kde.org https://mail.kde.org/mailman/listinfo/kst
On Tue, Oct 11, 2005 at 06:33:45PM -0400, Barth Netterfield wrote: > All of these, (except for the famous memory allocation error... we've been > down that road several times...), were errors which can only happen due to an > internal bug in GetData. Neither faulty data, nor faulty function input/use > can ever trigger any of them. I had the 'exit(0)' and the error message > there for identifying bugs when I wrote the code several years ago. None of > them have ever been triggered. So the bug is an 'invalid' as far as I am > concerned. When I wrote the bug report, I wasn't thinking of bugs internal to getdata; I was thinking of when people write their own format files which contain typos/invalid entries. We've been seeing alot of these since we started data analysis on BLAST and people are creating their own raw data and adding (by hand) entries in the format file. If these entries are mistyped or invalid or missing (both the binary data missing or the format entry missing) kst silently fails.
On Tuesday 11 October 2005 22:17, Matthew Truch wrote: > When I wrote the bug report, I wasn't thinking of bugs internal to > getdata; I was thinking of when people write their own format files > which contain typos/invalid entries. We've been seeing alot of these > since we started data analysis on BLAST and people are creating their > own raw data and adding (by hand) entries in the format file. If these > entries are mistyped or invalid or missing (both the binary data missing > or the format entry missing) kst silently fails. That's different than the exit(0) problem though. The ones I changed, as far as I can tell, are all related to coding specific errors except one. That one should return an error code, and is only triggered in OOM conditions anyway.
I started to submit this bug, but I had before, although it was marked as closed when a different issue was resolved, so I'm re-opening. To restate the problem: If the user has a dirfile which is invalid for whatever reason (missing field, malformed format file syntax, missing/malformed include etc), kst should report the error, including the type of error (missing file, syntax etc) as well as the line number or field name etc.
I think this bug is about datasources reporting errors, in the general sense. It is true that apart from kstdDebug() calls in the source, there is currently no way (at least that I know of) to get feedback from the datasource, which once again may be a file format problem and not a coding problem. I'd suggest giving the datasource a way to write in the kst log output, and the ability to raise an error flag. One good example of this is what happens when loading a datasource with unresolved symbols: the sign in the lower rright corner, which you can click on to get to the log, is very convenient.
You can trigger that indicator with KstDebug.
Nice to know :-) It would be nice if the required headers were installed so that datasources built outside of the kst tree could use it as well... Matt, I don't know if that answers your needs, so I'll leave it up to you to decide whether to close this report or not.
Please file a feature request for that. The thing is, that won't be used inside getdata.
Right, The crux of this bug is that getdata (the library that the dirfile data source uses) provides virtually no info when it fails. It (getdata) needs to provide some info that then can be passed along to the user via KstDebug. I could impliment the KstDebug part in the dirfile reader; I don't know how to properly impliment the actual error reporting in getdata.
1.2.1
SVN commit 667431 by arwalker: BUG:111571 Produce a debug log message when we fail to read in data from a dirfile. A record is kept of past errors so that we don't produce a continuous stream of errors when the dirfile has a format error. M +53 -8 dirfile.cpp M +1 -0 dirfile.h --- branches/work/kst/1.5/kst/src/datasources/dirfile/dirfile.cpp #667430:667431 @@ -18,8 +18,8 @@ #include "dirfile.h" #include "getdata.h" #include "getdata_struct.h" +#include "kstdebug.h" - DirFileSource::DirFileSource(KConfig *cfg, const QString& filename, const QString& type) : KstDataSource(cfg, filename, type) { if (init()) { @@ -72,7 +72,13 @@ } _writable = true; + } else { + char error[200]; + + GetDataErrorString(error, 200); + KstDebug::self()->log(error, KstDebug::Error); } + return update() == KstObject::UPDATE; } @@ -94,21 +100,35 @@ int DirFileSource::readField(double *v, const QString& field, int s, int n) { - int err = 0; + int err = GD_E_OK; + int read; if (n < 0) { - return GetData(_filename.latin1(), field.left(FIELD_LENGTH).latin1(), + read = GetData(_filename.latin1(), field.left(FIELD_LENGTH).latin1(), s, 0, /* 1st sframe, 1st samp */ 0, 1, /* num sframes, num samps */ 'd', (void*)v, &err); } else { - return GetData(_filename.latin1(), field.left(FIELD_LENGTH).latin1(), + read = GetData(_filename.latin1(), field.left(FIELD_LENGTH).latin1(), s, 0, /* 1st sframe, 1st samp */ n, 0, /* num sframes, num samps */ 'd', (void*)v, &err); } + + if (err != GD_E_OK) { + if (_errors.find(field) == 0L) { + char error[200]; + + _errors.insert(field, (int*)1L); + + GetDataErrorString(error, 200); + KstDebug::self()->log(error, KstDebug::Error); + } + } + + return read; } @@ -124,15 +144,34 @@ bool DirFileSource::isValidField(const QString& field) const { - int err = 0; + int err = GD_E_OK; GetSamplesPerFrame(_filename.latin1(), field.left(FIELD_LENGTH).latin1(), &err); - return err == 0; + + if (err != GD_E_OK) { + char error[200]; + + GetDataErrorString(error, 200); + KstDebug::self()->log(error, KstDebug::Error); + } + + return err == GD_E_OK; } int DirFileSource::samplesPerFrame(const QString &field) { - int err = 0; - return GetSamplesPerFrame(_filename.latin1(), field.left(FIELD_LENGTH).latin1(), &err); + int samples = 0; + int err = GD_E_OK; + + samples = GetSamplesPerFrame(_filename.latin1(), field.left(FIELD_LENGTH).latin1(), &err); + + if (err != GD_E_OK) { + char error[200]; + + GetDataErrorString(error, 200); + KstDebug::self()->log(error, KstDebug::Error); + } + + return samples; } @@ -222,7 +261,13 @@ for (int i = 0; i < ft->n_raw; i++) { fieldList.append(ft->rawEntries[i].field); } + } else { + char error[200]; + + GetDataErrorString(error, 200); + KstDebug::self()->log(error, KstDebug::Error); } + return fieldList; } --- branches/work/kst/1.5/kst/src/datasources/dirfile/dirfile.h #667430:667431 @@ -50,6 +50,7 @@ bool reset(); private: + QDict<int> _errors; int _frameCount; };