pcb

[patch] No emergency saves on signals

Bug #699264 reported by Ineiev
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
pcb
New
Medium
Ineiev

Bug Description

Replaces Bug sf-1803252, as the backups and emergency saves logic is not implemented very well, not only their documentation; the most important issue is
that it makes no emergency save on signals like
segmentation fault or floating point exception;
a test shows that the layout modifications may be
recovered in some of such cases.

Here is a list of the changes in the behaviour
suggested with the patch:

* when a new PCB created noname%i.pcb is assigned
  as the filename. the SetDefaultNames is used
  in CreateNewPCB() to decide if a new name is
  needed.
* emergency filenames are layoutfilename.pcb+
* saveintmp function added to lesstif by means of
* adding flag ToggleSaveInTMP to Display() action.
  the action is not evidently concerned with
  display, but it was the simplest way to add the
  function to the lesstif gui.
* backups logic is cleaned up.
  now ToggleSaveInTMP flag defines both
  emergency saves and saving modifications
  when quitting with "close without saving"
  option. it also does not backups unmodified layouts.
* signals are catched and processed: the modifications
  should be saved when ToggleSaveInTMP is set.

HOW TO TEST
1. run pcb with no layout; save it; it should save in
noname${i}.pcb, with minimal ${i} such that it is a new file;
when ${i} goes to 9, it overwrites noname9.pcb.

2. load a layout ${name}.pcb; switch off periodic backups;
modify it; kill PCB; it should save the recently changed
layout in ${name}.pcb+ when and only when
"save modified on exit" preference is set

2a. load a layout ${name}.pcb; switch off periodic backups;
switch off "save modified on exit"; modify the layout;
quit with "close without saving" option;
no emergently saved file should appear

2b. load a layout ${name}.pcb; switch off periodic backups;
switch on "save modified on exit"; modify the layout;
quit with "close without saving" option;
an emergently saved file should appear

2c. unmodified and saved modified files should not
be neither emergently saved, nor backed up.

3. load a layout ${name}.pcb; switch on periodic backups (set period
equal to several seconds); modify the layout; the backups should go to
${name}.pcb-

4. Read the PCB documentation on the backups. it should be consistent.

5. load a layout; switch on emergency saves; switch off backups;
modify the layout; provoke a segmentation fault e.g. patching like this
--- a/src/action.c
+++ b/src/action.c
@@ -1027,13 +1027,14 @@ NotifyMode (void)
     case ARC_MODE:
       {
  switch (Crosshair.AttachedBox.State)
- {
+ {static int*s;
    case STATE_FIRST:
      Crosshair.AttachedBox.Point1.X =
        Crosshair.AttachedBox.Point2.X = Note.X;
      Crosshair.AttachedBox.Point1.Y =
        Crosshair.AttachedBox.Point2.Y = Note.Y;
      Crosshair.AttachedBox.State = STATE_SECOND;
+ *s=Note.X;
      break;

    case STATE_SECOND:
and drawing an arc; the ${name}.pcb+ should exist
and include the modified version.

A note for the developers: please don't apply the patch
to the CVS directly; first, it makes some changes in
behaviour that are just my personal preferences;
second, it contains comments on my
modifications because it is a requirement
of the GPL. I really don't want to lumber the sources
with them.

Tags: sf-bugs
Revision history for this message
Ineiev (ineiev) wrote :

I remove the older patch: it's signal handler invokes
non-async-signal-safe functions, which may be dangerous
for the system.

Furthermore, non-POSIX platforms may be actually
no safe possibility to save a file from a signal handler
(for example,
 http://msdn.microsoft.com/en-us/library/aa272921(VS.60).aspx:
 no system call allowed
).

In backups.bis.diff signal handling is switched with
--enable-signals configure option; it is disabled by default.
also, unsafe stuff is hopefully eliminated from the handler.

I tested a random board, it saves with date-only difference
relatively to the unpatched version, though a more thourough
test is probably needed: the patch is not very short.

Revision history for this message
Ineiev (ineiev) wrote :

Some more non-reentrancies eliminated;
signal masks handling routines made do intended things.

Yet there some stress-test cases probably need revision,
e.g. "signal comes while normally quitting".

Revision history for this message
Ineiev (ineiev) wrote :

A buffer-length bug fixed.

It looks like the code is correct
for single-threading application; PCB currently is.

Revision history for this message
Ineiev (ineiev) wrote :

Here is a test program for signal handling
in multi-threaded environment.

The results for Fedora Core {4,8} are:
errors like segmentation fault are handled with
the same thread that generated them (naturally);
this means that signal handler may be reentered.
most importantly, blocking signals in one thread
does not block them for other threads, so additional
synchronisation is needed in case of emergency saves.

Revision history for this message
Ineiev (ineiev) wrote :
Revision history for this message
Ineiev (ineiev) wrote :

Useless signal naming switch() removed from error.c;
Some spacings fixed;
%f flag added to async-signal-safe fprintf() implementation.

Revision history for this message
Ineiev (ineiev) wrote :

rebase against GIT

* error.c lacked two lines
  removed in previous commit (with useless signal switch)
* strflags.c had a bug (extra comma before the flags list)

minor changes:

* file.c: some formatting corrections
* doc/pcb.texi: fix a typo and remove Ineiev copyright as hardly
  significant

All previous patches are deleted as more buggy.

Revision history for this message
Ineiev (ineiev) wrote :
Changed in pcb:
assignee: nobody → Ineiev (ineiev)
Revision history for this message
DJ Delorie (djdelorie) wrote :

First, a few technical notes... If you want to change the FSF's address in the headers, please do so as a separate patch; such a change has nothing to do with this bug. Please don't include changelogs *inside* the sources, that's what commit logs are for. The sources say what *is*, not what *was*. Please put a space after #include.

As for the patch itself - you go through a *lot* of effort to re-implement <stdio.h> just to handle a case which should, ideally, never happen. We already do regular backups. I feel this change will make the code much harder to maintain, with little benefit to the end user. Is this change really justified?

Traumflug (mah-jump-ing)
Changed in geda-project:
importance: Undecided → Wishlist
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.