Poor reporting of schematic load errors

[Peter TB Brett]

libgeda doesn't signal an error when a schematic or symbol file contains invalid syntax, or when any other load error is encountered.

For example, gnetlist succeeds even when the input schematics specified contain unusable garbage.

Steps to reproduce:

  echo GARBAGE > test.sch;
  if gnetlist -ggeda test.sch > /dev/null 2>/dev/null
  then
    echo Succeeded
  else
    echo Failed
  fi

Expected output:

  Failed

Actual output:

  Succeeded

Thanks to Peter Clifton for pointing out this problem.

[Peter TB Brett (peter-b)]

This turns out to be a problem with libgeda -- the file loading API doesn't signal an error. o_read_buffer() needs to be improved to be able to indicate that parsing input failed.

[Krzysztof Kościuszkiewicz (k-kosciuszkiewicz)]

The input parsing is bad enough it can die with a segfault (as in bug 832682).
Bad reporting is one thing, but segfault is more severe.

[Eivind Kvedalen (eivind-z)]

I have a nearly-finished patch for better error-handling (will post later). Do we have an example where we get a segfault in the loader?

[Marius B. Kotsbak (mariusko)]

Japp, I am afraid of people trying this program on the seemingly right file type from Eagle and concludes it is too unstable to be used. I almost did, but held open the possibility that it still could be used for new layouts.

[Eivind Kvedalen (eivind-z)]

The attached patch tries to improve the error handling when reading schematic files. See patch comment for details. It is intentionally much less tolerant to errors, and will fail and report errors in more cases than the current version (which will only dump messages to stderr, and happily continue parsing).

This is only a partial fix, and needs comments. Especially:
* libgeda/src/scheme_page.c: Most likely not correct reporting string (I'm not very familiar with guile)
* gschem/src/o_complex.c: May be improved more; see FIXME comment.

Also, this patch does not fix any reported segfaults occurring when reading non-gschem format files such as Eagle files. I think this is a separate issue, and should have its own patch.

[Peter TB Brett (peter-b)]

Thanks for the patch, Eivind. Unfortunately, at first glance your changes to the Scheme API are obviously bogus, and I have to NACK this patch for now.

- You must update the testsuite (probably libgeda/scheme/unit-tests/t0203-page-string-syntax.scm).
- You must update the documentation (docs/scheme-api/geda-scheme.texi).
- You must use an appropriate exception key symbol (i.e. not 'object-state').
- You must use a '~a' or '~s' format specifier (Scheme does not use '%' as the format escape symbol; read the Guile manual entry for 'simple-format').

I also suggest that you split the patch into two: one which updates the libgeda core, and one which updates the Scheme API with the changes I've requested above.

These were the issues that immediately leapt out at me; I haven't had time to look through the rest of the patch yet, but I'll try to find time to do so later this week.

[Eivind Kvedalen (eivind-z)]

This patch probably needs even more work. I have therefore set up a clone at github, where I have created a branch (bug-700448-load-error) for this fix (and others I'm working on, they're just not there yet). You can find it at git://github.com/eivindkv/geda-gaf.git . I've tried to implement your requests above in that branch now.

Regarding splitting this up into two patches, I can agree, but it is technically difficult to do: the o_read_buffer function has an extra parameter now for the error, and the string_to_page function uses that, so the first patch will not compile if the scheme changes are in another separate patch.

[Peter TB Brett (peter-b)]

The branch sounds like an excellent idea.

For separating the patches, why not just make the Scheme API code pass a NULL GError in the main patch, since that's a simple and obviously correct one-line change that keeps the current behaviour? In a subsequent patch, you could then pass a non-NULL pointer, check the error condition, and do the other API updates.

[Peter TB Brett (peter-b)]

Hi Eivind,

I've now had time to do a more in-depth review of your patch (looking at the latest commit on your github branch, http://goo.gl/tJW63), and I have a few comments. I'm afraid I still haven't been able to actually go through it on a line-by-line basis, though, so there might be some remaining issues that I've missed. Sorry. :-(

- Please indent with spaces, not tabs.

- In o_complex_prepare_place(), the correct thing to do if the symbol can't be loaded is to abort the symbol placement operation and show an error dialog box to the user.

- I personally am not a fan of generic_msg_dialog(), because it usually doesn't show the correct message dialog variant (warning/error/info etc) for the type of message to be displayed. Consider using a GtkMessageDialog directly.

- For the preview, would it be possible not to pop up a modal dialog and instead show the error message in the preview pane somehow? Displaying a preview is not a "critical operation" such that failure should interrupt what the user is doing. By contrast, if the user attempts to open the file for viewing/editing, that load *is* a critical operation and a message dialog would be exactly the right thing. What do you think?

- I'm not convinced that EDA_ERROR_READ is the best possible name for this error. What about EDA_ERROR_PARSE or something that similarly describes the source of the error more specifically? To me, EDA_ERROR_READ suggests that an error occurred in actually *reading* the data from storage/network... It's also not clear why you're using EDA_ERROR_READ in some places and G_FILE_ERROR_FAILED in others.

- Similarly, you've chosen "invalid-string" as the key symbol for the corresponding Scheme error. "Invalid" has quite a large number of interpretations. Consider something like "string-format" or a name that otherwise specifically identifies the class of error that you're looking for.

This might be a good opportunity to also standardise the formatting and wording of the errors emitted during file loading.

Thanks for your hard work on this. I appreciate that the existing file loader code is both slightly baroque and also spread across a bunch of different files, and that fixing it up is a non-trivial exercise!

                               Peter

[Peter TB Brett (peter-b)]

> The input parsing is bad enough it can die with a segfault (as in bug 832682).

Hi Krzysztof,

The issue in bug 832682 is *not* a segfault, it is an assertion failure, as the backtrace supplied by the user clearly shows.

Although I agree that we should be detecting and handling these kinds of problems in a more robust fashion, a segfault would be a much more serious concern. All that bug demonstrates is that we are actually doing input validation!

                                     Peter

[Eivind Kvedalen (eivind-z)]

Well, it's actually quite easy to crash gschem: Make a file containing a single '[' char, and gschem will crash immediately when loading it. This is not fixed in my branch either (yet), btw.

Peter: I've pushed my changes based on your feedback to github now. Unfortunately, I rebased my branch by mistake, so I guess you have to remove your local bug-700448-load-error branch first, as pulling probably won't work directly.

[Peter TB Brett (peter-b)]

One of the problems we have is that attempting to load a schematic causes a whole load of other files and directories to be accessed, and if any one of those accesses fails for any reason or if there are any syntax errors in any of the files involved, the whole thing blows up.

I wonder if there is any way we could modify the loader so that library users get two levels of API -- a basic level which just loads the schematic and all the dependent files, and emits an error if any problems occur, and a lower level where a schematic can be loaded *without* other resources in a first pass, and then additional resources can be loaded individually in a second pass... that way e.g. gschem could provide much more fine-grained feedback to the user when things go wrong.

[Peter TB Brett (peter-b)]

BTW Eivind, I made some more comments on your branch using the github review feature.

[Eivind Kvedalen (eivind-z)]

I've pushed my changes based on your comments now. The bug-700448-load-error branch has been rebased to current master (and you comments on github seems to have been lost when I did that; I must have done something wrong...).

Regarding loading a file without loading its dependencies, a starting point might be to force the loader to use "placeholder" instead of "complex", unless it is embedded, in which case it will load as normal. Apart from the complex type, I think it's only the picture object type that has dependencies to other files, and it would need a flag or something to tell the system that it is not completely loaded.

I'm unsure if this will help much, though. The low-level loader will make things much harder to use. It might actually be easier to change the error reporting to be a list of errors instead of the first one encountered. Then the loader can choose to continue trying other objects to report all encountered errors in one pass, or maybe use it as a stack to report the root source of the problem.

[Peter TB Brett (peter-b)]

Hi Eivind! I was going to merge this, but then I found that it makes `make distcheck' fail. Could you please investigate? Thanks!

[Eivind Kvedalen (eivind-z)]

I've pushed a fix for this now to my github branch. It was caused by different ordering of attributes internally. make distcheck works here on my system now.

[gpleda.org commit robot (gpleda-launchpad-robot)]

Bug was fixed by a commit
git master commit 31440e054a54001bf462f7da9d31bfc0f0063d3f
http://git.gpleda.org/?p=gaf.git;a=commit;h=31440e054a54001bf462f7da9d31bfc0f0063d3f

commit 31440e054a54001bf462f7da9d31bfc0f0063d3f
Merge: c6db159... 0884425...
Author: Peter TB Brett <email address hidden>
Commit: Peter TB Brett <email address hidden>

    Merge branch 'bug-700448-load-error'.

    Eivind Kvedalen has provided some changes to libgeda and tools that
    provide better error detection when loading schematic and symbol
    files, and I have made a few improvements and bugfixes based on his
    branch.

    Closes-bug: lp-700448

    Conflicts:
     libgeda/src/a_basic.c

[Peter TB Brett (peter-b)]

At some point we need to go through and clean up all of the error messages. But this will do for a start, I think!