Issue #22: HostFiles.NewWriter violating the specification

Issue #22: HostFiles.NewWriter violating the specification

Postby Ivan Denisov » Sun Mar 16, 2014 1:13 pm

Finder & Solver: Ilya Ermakov

According the specification in a case of read-only files NewWriter should return NIL.
System/Docu/Files wrote:PROCEDURE (f: File) NewWriter (old: Writer): Writer
NEW, ABSTRACT
Returns a writer which has the appropriate type (for this file type). If old = NIL, then a new writer is allocated. If old # NIL and old has the appropriate type, old is returned. Otherwise, a new writer is allocated. The returned writer is connected to f, and its position is somewhere on the file. If an old writer is passed as parameter, the old position will be retained if possible.
If an old writer is passed as parameter, it is the application's responsibility to guarantee that it is not in use anymore. Passing an unused old writer is recommended because it avoids unnecessary allocations.
Read-only files allow no writers at all. In such cases, NewWriter returns NIL.

Post
result # NIL
old # NIL & old.Base() = f
result.Pos() = old.Pos()
old = NIL OR old.Base() # f
result.Pos() = f.Length()
result = NIL
read-only file


However the code of HostFiles is next:
Code: Select all
       PROCEDURE (f: File) NewWriter (old: Files.Writer): Files.Writer;
          VAR w: Writer;
       BEGIN   (* portable *)
          ASSERT(f.state # closed, 20); ASSERT(f.state # shared, 21);
          IF (old # NIL) & (old IS Writer) THEN w := old(Writer) ELSE NEW(w) END;
          IF w.base # f THEN
             w.base := f; w.buf := NIL; w.SetPos(f.len)
          END;
          RETURN w
       END NewWriter;


ASSERT(f.state # shared, 21); means TRAP except RETURN NIL.

Ilya had suggested next solution:
Code: Select all
   PROCEDURE (f: File) NewWriter (old: Files.Writer): Files.Writer;
      VAR w: Writer;
   BEGIN   (* portable *)
      ASSERT(f.state # closed, 20);
      IF f.state # shared THEN
         IF (old # NIL) & (old IS Writer) THEN w := old(Writer) ELSE NEW(w) END;
         IF w.base # f THEN
            w.base := f; w.buf := NIL; w.SetPos(f.len)
         END;
         RETURN w
      ELSE
         RETURN NIL
      END
   END NewWriter;


Lets vote for including this fix in the Center version.
Ivan Denisov
 
Posts: 1694
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

Re: B1: HostFiles.NewWriter is violating the specification

Postby Ivan Denisov » Thu Apr 17, 2014 1:03 am

I am really think, that we should vote for each change in the forum like this example.

The Center task is provide stable version and select stable features to unite users around authoritative BlackBox version. It is impossible without deep analysis and voting around each change.
Ivan Denisov
 
Posts: 1694
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

Re: B1: HostFiles.NewWriter is violating the specification

Postby ReneK » Thu Apr 17, 2014 7:48 am

While, yes, each bug needs to be analsed and fixed with scrutiny, I am not sure that voting for each and every bug fix is manageable.
User avatar
ReneK
 
Posts: 214
Joined: Tue Sep 17, 2013 9:16 am
Location: Vienna, Austria, Europe

Re: B1: HostFiles.NewWriter is violating the specification

Postby Josef Templ » Fri Apr 18, 2014 8:39 am

I agree with Rene.

We have a backlog of more than 50 changes and if we are not dealing
with this backlog quickly, the new issues will arrive faster than the backlog shrinks,
which leads to frustration.

In addition, having looked at all the changes done in the CPC 1.7 edition, I know
that it is a lot of work and it will not be possible for most of the center members
to analyze all the changes in depth, both for the skills and for the time required.
You will have to dig deeply into the compiler, for example.
Some of the changes are of course rather simple.

For those center members who have both the skills and the time I would
propose they look at the CPC 1.7 edition and if there are any problems with
the existing fixes, please list them.
I would expect this list to be a rather small.

The main problem with the CPC edition, In my opinion, is that it is
not yet put in a (git) repository with an issue tracker being connected.
Instead, it uses a text file for describing all the changes.
So this would be my recommended first steps.
0. set up a repository and an issue tracker
1. put the 'BB 1.6 final' sources and resources in a repository (not the binary files)
2. add the CPC 1.7 issues one by one to an issue tracker (such as redmine)
and commit the changes related to each issue individually to the repository.
Even this 'editing' is quite some work, but not comparable with the work
already being done.
After this work has been done, it will be much easier for
the center members to look at the list of resolved issues and how they have
been resolved. One by One.

Ideally there should also be a build machine that automates the
creation of a downloadable binary file after changes in the repository.
But this is a separate issue that can be worked on in parallel.
It can be done manually at the beginning. Later it should be automated.
One tool dedicated to exactly this problem is Jenkins.
There may be other tools or simpler ones, I don't know.
Jenkins seems to be one of the standard open source tools used
by many similar projects for what is called 'continuous integration',
which means to continually integrate the changes committed to a
repository into a binary distribution package.
Then you get features like latest stable build, daily build,
latest developer build, statistics, error logs, etc.

- Josef
User avatar
Josef Templ
 
Posts: 2002
Joined: Tue Sep 17, 2013 6:50 am

Re: B1: HostFiles.NewWriter is violating the specification

Postby cfbsoftware » Fri Apr 18, 2014 1:14 pm

There is also the strong possibility that the code is as intended but the documentation was mistakenly not updated when the code was changed. An earlier version of the source code was:

Code: Select all
   PROCEDURE (f: File) NewWriter (old: Files.Writer): Files.Writer;
      VAR w: Writer;
   BEGIN   (* portable *)
      ASSERT(f.state # closed, 20);
      IF f.state IN {shared, shidden} THEN RETURN NIL END;
      IF (old # NIL) & (old IS Writer) THEN w := old(Writer) ELSE NEW(w) END;
      IF w.base # f THEN
         w.base := f; w.buf := NIL; w.SetPos(f.len)
      END;
      RETURN w
   END NewWriter;

The assertion appears to have been intentionally added. This highlights the error as soon as it is detected, rather than letting a NIL reference cause a problem sometime later.
cfbsoftware
 
Posts: 202
Joined: Wed Sep 18, 2013 10:06 pm

Re: B1: HostFiles.NewWriter is violating the specification

Postby Josef Templ » Fri Apr 18, 2014 2:02 pm

cfbsoftware wrote:There is also the strong possibility that the code is as intended but the documentation was mistakenly not updated when the code was changed. An earlier version of the source code was:

Code: Select all
   PROCEDURE (f: File) NewWriter (old: Files.Writer): Files.Writer;
      VAR w: Writer;
   BEGIN   (* portable *)
      ASSERT(f.state # closed, 20);
      IF f.state IN {shared, shidden} THEN RETURN NIL END;
      IF (old # NIL) & (old IS Writer) THEN w := old(Writer) ELSE NEW(w) END;
      IF w.base # f THEN
         w.base := f; w.buf := NIL; w.SetPos(f.len)
      END;
      RETURN w
   END NewWriter;

The assertion appears to have been intentionally added. This highlights the error as soon as it is detected, rather than letting a NIL reference cause a problem sometime later.



This is a very strong point indeed.
BTW, where did you get this previous code version from?
In 1.5 and 1.5 Beta I cannot find this code.
Until 1.4 there was no official release of the source code, was it?

In any case, it really gives sense to trap as soon as possible.
I have changed my vote to 'no', which means that we should change the documentation.
@Helmut: what do you think?

- Josef
User avatar
Josef Templ
 
Posts: 2002
Joined: Tue Sep 17, 2013 6:50 am

Re: B1: HostFiles.NewWriter is violating the specification

Postby Ivan Denisov » Fri Apr 18, 2014 6:00 pm

The argument was, that there is no way to now is there file shared or not with a help of framework. That is why this TRAP becomes unavoidable.

There are one rule in Files documentation.
Opening a file in shared mode is the rule in BlackBox; opening a file in exclusive mode is an infrequent exception.

So, this problem should not appear if you led this rule.

However, how to prevent TRAP in infrequent exceptions? That means that programmer should provide this check with it's own module variables... and make strong encapsulation of files variables.

One File can be open with dir.Old other made with dir.New. You can not distinguish can you make file.NewWriter or not by having only the Files.File variable.
Ivan Denisov
 
Posts: 1694
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

Re: B1: HostFiles.NewWriter is violating the specification

Postby Ivan Denisov » Fri Apr 18, 2014 6:20 pm

The other solution is to make new procedure in Files.

Code: Select all
PROCEDURE (f: File) IsWritable* (): BOLEAN, NEW, ABSTRACT;


And it's simple realisation in HostFiles.

Code: Select all
PROCEDURE (f: File) IsWritable* (): BOLEAN;
BEGIN
RETURN (f.state # shared) & (f.state # closed)
END Shared;
Ivan Denisov
 
Posts: 1694
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

Re: B1: HostFiles.NewWriter is violating the specification

Postby DGDanforth » Fri Apr 18, 2014 11:56 pm

Josef Templ wrote:I agree with Rene.

We have a backlog of more than 50 changes and if we are not dealing
with this backlog quickly, the new issues will arrive faster than the backlog shrinks,
which leads to frustration....

- Josef


That seems like a very good plan to me and I support it. Now how to make it happen?
User avatar
DGDanforth
 
Posts: 1061
Joined: Tue Sep 17, 2013 1:16 am
Location: Palo Alto, California, USA

Re: B1: HostFiles.NewWriter is violating the specification

Postby Ivan Denisov » Sat Apr 19, 2014 2:07 am

DGDanforth wrote:
Josef Templ wrote:I agree with Rene.

We have a backlog of more than 50 changes and if we are not dealing
with this backlog quickly, the new issues will arrive faster than the backlog shrinks,
which leads to frustration....

- Josef


That seems like a very good plan to me and I support it. Now how to make it happen?

This experience with HostFiles shows that each bugfix should be discussed. Doug, for managing this "more than 50 changes" you should add the topic for each change for discussion on the forum happen, after the discussion we will put it on the issues tracker if a format of (issue + solution) and later commit this changes in downloadable repository.
Ivan Denisov
 
Posts: 1694
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

Next

Return to Resolved (Bugs)

Who is online

Users browsing this forum: No registered users and 0 guests

cron