Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trying to open a nonexisting table doesn't generate an error #542

Open
vsht opened this issue Jun 20, 2024 · 14 comments
Open

Trying to open a nonexisting table doesn't generate an error #542

vsht opened this issue Jun 20, 2024 · 14 comments

Comments

@vsht
Copy link

vsht commented Jun 20, 2024

Hi,

I'm not sure if it's an omission or was originally intended like that, but I noticed that trying to
open a tablebase that physically doesn't exist on disk doesn't generate any errors.

TableBase "blalba.tbl" open;
TableBase "blalba.tbl" enter;
L exp = 42;

.end

Only when trying to insert an element from a nonexisting table will this code crash as it should.

Cheers,
Vlad

@jodavies
Copy link
Collaborator

For me FORM does crash here, though it should certainly give a useful error message.

    Tablebase "test.tbl" open;
    Tablebase "test.tbl" enter;
Program terminating at open.frm Line 8 -->

It's a segfault since "enter" tries to loop through the tables without checking if there are any.

Note that the open does actually create a file (since if it doesn't find the file, it just calls NewDbase as "create" does). The following works just fine:

CTable,sparse f(1);
Fill f(1) = 1;
Fill f(2) = 2;
.sort

Tablebase "test.tbl" open;
Tablebase "test.tbl" addto f;
.end

@vsht
Copy link
Author

vsht commented Jun 20, 2024

For me FORM does crash here, though it should certainly give a useful error message.

Ok, may be my version is too old: FORM 4.1 (May 15 2023, v4.1-20131025-587-g8a37a42).

But I agree that a useful error message would be nice.

Note that the open does actually create a file (since if it doesn't find the file, it just calls NewDbase as "create" does).

Is this expected? According to the manual

This opens an existing file with the indicated name. It is assumed that the file has been created
with the ‘create’ option in a previous FORM program. It gives the user access to the contents of
the tablebase. In addition it allows the user to add to its contents.

This doesn't read like a new file will be created if it doesn't exist so far.

@jodavies
Copy link
Collaborator

The behaviour regarding the tablebases should not have changed here since 8a37a42 . Can you try running the debug binary on your script under gdb?

@vsht
Copy link
Author

vsht commented Jun 20, 2024

Ok, now I see the problem. When I run this code for the first time, it will crash but at the same time create an empty blabla.tbl. Then, running it for the second time will not generate any crashes.

Tbh, I find this behavior quite confusing, since in real life the table might be missing for a reason and substituting it with an empty one is definitely not what the user would like.

@jodavies
Copy link
Collaborator

It would be straightforward to have "open" give an error and terminate FORM, in case the file doesn't exist. This could break people's existing FORM scripts in principle, so let's see if anyone else has opinions on this change.

It is also not so nice that "create" overwrites existing files with the same name. Here one could check for an existing file and terminate if one is found.

@vsht
Copy link
Author

vsht commented Jun 20, 2024

Is there another way to check for the physical existence of the table before "opening" it?

audit seemingly does what I want, but I also don't want to pollute the logs by printing the full table content.

@jodavies
Copy link
Collaborator

I don't think so with the tablebase command. Of course you can use #system or #pipe to make such a check.

@vsht
Copy link
Author

vsht commented Jun 20, 2024

I don't think so with the tablebase command. Of course you can use #system or #pipe to make such a check.

Thanks. I also thought about that, but of course a native FORM command would be nicer.

@tueda
Copy link
Collaborator

tueda commented Jun 20, 2024

but of course a native FORM command would be nicer.

So far, no one has tried to implement it: #240

@vsht
Copy link
Author

vsht commented Jun 21, 2024

Speaking of pipes, I noticed that trying to check for the existance of a table using #pipe and #define also leads to a somewhat strange behavior

#pipe echo "#define TABLEPRESENT \"$((ls blalba.tbl >> /dev/null 2>&1 && echo yes) || echo no)\""

#message `TABLEPRESENT'

#if (`TABLEPRESENT'!="yes")        
  exit "Error, the table is missing.";
#endif

TableBase "blalba.tbl" open;
TableBase "blalba.tbl" enter;
L exp = 42;

.end

My expectation is that if TABLEPRESENT not equals yes, the code should stop immediately due to the exit statement. However, in reality the execution continues after exit and a fake blalba.tbl still gets generated :(

It works if I use something like

#if (`TABLEPRESENT'!="yes")        
  exit "Error, the table is missing.";
.end
#endif

but this really contradicts the main idea of using exit in the first place.

@vsht
Copy link
Author

vsht commented Jun 21, 2024

It would be straightforward to have "open" give an error and terminate FORM, in case the file doesn't exist. This could break people's existing FORM scripts in principle, so let's see if anyone else has opinions on this change.

It is also not so nice that "create" overwrites existing files with the same name. Here one could check for an existing file and terminate if one is found.

A possible compromise would be to change the manual stating that open will also create an empty table if it doesn't exist so far. Then adding something like shortaudit as a way to check for the existence of the table without polluting the logs would be more than sufficient. The current behavior is IMHO not really ok.

@jodavies
Copy link
Collaborator

You need to use a preprocessor terminate:

#if (`TABLEPRESENT'!="yes")
   #message "Error, the table is missing."
   #terminate
#endif

Exit is an executable statement and you are not processing any terms when you make the pre-processor check.

My preference would be to make open fail if the file doesn't exist, rather than add shortaudit or anything.

@vsht
Copy link
Author

vsht commented Jun 21, 2024

Exit is an executable statement and you are not processing any terms when you make the pre-processor check.

Ok, that makes sense. Thanks.

My preference would be to make open fail if the file doesn't exist, rather than add shortaudit or anything.

That would be an ideal solution, but at the cost of breaking someone's scripts who was silently using this bug as a feature.

@jodavies
Copy link
Collaborator

There should also be #531 merged at some point, readonly mode should certainly fail if the file doesn't exist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants