Opened 8 years ago

Closed 5 weeks ago

#331 closed defect (fixed)

SimFactory does not abort if it has been given a nonexistent optionlist

Reported by: Ian Hinder Owned by: mthomas
Priority: major Milestone:
Component: SimFactory Version:
Keywords: Cc:


If I specify a nonexistent optionlist in a machine's .ini file, I get the warning

Info: optionlist is: None
Warning: no option list specified, using blank option list

This is incorrect. I have specified an optionlist, but it has not been found, which indicates an error. In this case I would expect a fatal error.

Attachments (0)

Change History (5)

comment:1 Changed 8 years ago by Ian Hinder

I just ran into this again with the latest version of simfactory. I thought it would be easy to fix, but it seems like the offending code in is more opaque than I thought. The "optionlist" variable, by the time we have got to the error message, contains "None", rather than the specified optionlist name (the file which does not exist). So I can't check for a nonexistent file and abort with a fatal error.

comment:2 Changed 8 years ago by Erik Schnetter

The problem seems to be in, function GetCactusFile, in the last if statement

if not(os.path.exists(ff)):

This routine obtains a configuration file (e.g. an option list), and takes an argument that specifies whether this file is required to be present or not. If it is required, it outputs an error message if the file is not found, if not, it fails gracefully and returns None. In our case, an option list is not required, hence None is returned.

I believe the logic in this if statement is wrong. If a file name is specified, this file name should be returned no matter what. An additional test may be performed (to see whether the file exists), or an error message may be output, but the specified option should never be ignored and None returned.

Notice that the routine contains some duplication; e.g. the search logic to find the file is repeated twice, first for a user option that can override the MDB entry, and then for an MDB entry.

I believe the correction should be to remove the "else: ff=None" in the last if statement, and possibly also the "if required:". The latter would make both failure paths (for user options and MDB entries) the same.

A cleanup of this routine may also be helpful.

comment:4 Changed 5 weeks ago by Roland Haas

Status: reviewreviewed_ok

comment:5 Changed 5 weeks ago by Roland Haas

Resolution: fixed
Status: reviewed_okclosed

Applied as git hash e6b9c1d5054f "simlib: replace os.path.exists with os.path.isfile or os.path.isdir" of simfactory2.

Modify Ticket

Change Properties
Set your email in Preferences
as closed The owner will remain mthomas.
The resolution will be deleted.

Add Comment

E-mail address and name can be saved in the Preferences.

Note: See TracTickets for help on using tickets.