hi,

-----<snip>-----
#version 3.8;

global_settings {assumed_gamma 1}
box {0,1}

#if (0)
#declare dir_ = "/badName/";
#else
#declare dir_ = "/tmp/";
#end

#declare arr_ = array [3] {"foo","bar","baz"};

#fopen fp_ concat(dir_,"wtest.txt") write

#for (i_,0,2)
  #if (2 != i_) #local c_ = ","; #else #local c_ = ""; #end
  #write (fp_,concat("\"",arr_[i_],"\"",c_))
#end

#fclose fp_
-----<snip>-----

when I change the '#if' to use a non-existing directory, the alpha.9945627 and
beta.2, as well as WFP's povr, all silently ignore the fact that no file was
(could be) created, and do not return an error.  both on Linux and Windows.

(thanks for the discovery goes to TdG who ran into this issue while testing some
code of mine)


regards, jr.

Post a reply to this message

On 10/6/21 5:01 AM, jr wrote:
> hi,
> 
> -----<snip>-----
> #version 3.8;
> 
> global_settings {assumed_gamma 1}
> box {0,1}
> 
> #if (0)
> #declare dir_ = "/badName/";
> #else
> #declare dir_ = "/tmp/";
> #end
> 
> #declare arr_ = array [3] {"foo","bar","baz"};
> 
> #fopen fp_ concat(dir_,"wtest.txt") write
> 
> #for (i_,0,2)
>    #if (2 != i_) #local c_ = ","; #else #local c_ = ""; #end
>    #write (fp_,concat("\"",arr_[i_],"\"",c_))
> #end
> 
> #fclose fp_
> -----<snip>-----
> 
> when I change the '#if' to use a non-existing directory, the alpha.9945627 and
> beta.2, as well as WFP's povr, all silently ignore the fact that no file was
> (could be) created, and do not return an error.  both on Linux and Windows.
> 
> (thanks for the discovery goes to TdG who ran into this issue while testing some
> code of mine)
> 

This a bug is basically the same as the radiosity file append bug.

It looks like after v3.7 some file IO class work got done making the 
test for successful opens on write and append(a) 'if (*(wfile))' instead 
of the more common / expected 'if (wfile != nullptr)'.

(a) - Read too depending on the set up and usage.

To complicate things...

1) Though POV-Ray's IStream is using the same base class as OStream, 
because the read pointer is a shared_ptr, you must use
the usual 'if (rfile != nullptr)' test for success on reads. The 
override version in fact segfaults for me when I tried it - and I've not 
gotten my head around the C++ reason(s) why.

2) The read open success/fail, from a users perspective, is working but 
it's due overlapping code in a function called Locate_File() and not due 
the testing in Parse_Fopen(). Meaning, I think, but I'm too lazy to 
chase it at the moment, that the read 'if (rfile != nullptr)' is 
probably also not working in Parse_Fopen(), but we are covered so it 
doesn't matter.

3) Given this and the radiosity bug,I have to wonder if we have other 
file open write / append bugs in v3.8. What other file opens might be 
write or append in POV-Ray?

Anyhow, updates not checked in, but fopen write/append fixes will be in 
the next povr tarball.

Bill P.

Post a reply to this message

hi,

William F Pokorny <ano### [at] anonymousorg> wrote:
> On 10/6/21 5:01 AM, jr wrote:
> > ...
> This a bug is basically the same as the radiosity file append bug.

a known issue, ok.  thanks.


regards, jr.

Post a reply to this message

On 10/9/21 3:49 AM, jr wrote:
> hi,
> 
> William F Pokorny <ano### [at] anonymousorg> wrote:
>> On 10/6/21 5:01 AM, jr wrote:
>>> ...
>> This a bug is basically the same as the radiosity file append bug.
         x
> 
> a known issue, ok.  thanks.
> 

Hi jr,

No. Well, it perhaps now is. I wasn't smart enough to consider the 
radiosity sample file issue might indicate a general problem.

If you see other similar file/IO issues, please report them. Thanks to 
you both for reporting this one. Internally, it led to two issues in 
that the write and append modes were both not working.

---
I'd taken the radiosity samples file bug to be a one off problem. On 
seeing the same root cause for this fopen keyword issue, it's looking 
more like a systemic problem with user file IO in v3.8. :-(

While away from my computer the past few days, I found myself thinking 
about this issue. And prior to leaving, I'd looked back through the 
radiosity samples file fix to radiosity.cpp; This time looking at other 
IStream/OStream class usage. I saw additional concerning code.

I've not spent much time with POV-Ray's IO code. We have an IO class 
which doesn't end up as nullptr on a failed open, but rather an 
allocated class object with extra information - like the file name. Our 
our always allocated 'file handle, handler' can be in a fail state or 
not. That state could be due an attempt to open a file - or I guess any 
other cause thereafter.

I'm far from understanding all the particulars - including not 
understanding what changed in the v3.7 to v3.8 work that we have these 
issues popping up. There was obviously some IO class work - and quite a 
lot of code re-factoring around IO. I found a commit from 2018 making 
changes so the #read directive 'again worked.' Did #read not work in 
v3.7 and/or some other post v3.7 versions?

Further, I'm unsure why the FOPEN, Parse_Fopen() code is using a 
shared_ptr<IStream> when I see no reason for a shared pointer(1). The 
v3.7 code doesn't use a shared_ptr.

Plus, for the read code, there is extra code appending common suffixes 
(I guess potentially .csv here?) before failing. On unix/linux I'd like 
all the 'guess a file suffix' code gone! I believe, even on windows 
where suffixes are hidden by default, this 'guess a suffix' code likely 
causes user confusion as often as not(2).

Oh, I think I might now understand why using the 'if (*readPtr)' was 
causing  segfaults. It's that the particular 'open for read' code - 
looking for files while guessing suffixes - is 'sometimes' returning 
simply 'nullptr'.

So, sometimes, the 'if (fid != nullptr)' test is what you have to use 
while elsewhere you need 'if (*fid)'. This suggests, to be robust, we 
should be using 'if ((fid != nullptr) && (*fid))' in the latter case. 
Hmm... Or maybe in all cases as it should be safe and it's consistent!

I guess, I should spend time digging into POV-Ray's IO code though I'd 
rather be playing elsewhere... :-(

Bill P.

(1) - I had the thought, maybe with shared_ptr() what the coder was 
after was safe clean up when the last referencing object went out of 
scope / was deleted. But, if so, why wasn't the same thing done with the 
write and append OStream pointers... And if cleanup the reason, I think, 
these days, a unique_ptr is likely better.

(2) - "Cannot find file 'fopen_HAPPY_r.inc', even after trying to append 
file type extension." <-- Message used where it doesn't makes sense 
given the passed file name already has a valid extension and POV-Ray 
should not be second guessing the user.

Post a reply to this message

hi,

William F Pokorny <ano### [at] anonymousorg> wrote:
> On 10/9/21 3:49 AM, jr wrote:
> > William F Pokorny <ano### [at] anonymousorg> wrote:
> >> On 10/6/21 5:01 AM, jr wrote:
> >>> ...
> >> This a bug is basically the same as the radiosity file append bug.
>          x
> > a known issue, ok.  thanks.
>
> No. Well, it perhaps now is.

good, I think.  :-)


> ...
> I've not spent much time with POV-Ray's IO code.

sounds like opening a can of worms, only to find a multi-pack of can of worms
inside..  think it .. bad not to have access to (tech) notes dealing with design
etc of the "new" IO class.  bazaar, huh?!

coincidentally, have just posted a new macro in p.text.scene-files, 'Filed()', a
CSV file handler, hope you'll find that useful when "stirring the worms" ;-).


regards, jr.

Post a reply to this message

On 10/13/21 10:44 AM, jr wrote:
> hi,
> 
> William F Pokorny <ano### [at] anonymousorg> wrote:
>> On 10/9/21 3:49 AM, jr wrote:
>>> William F Pokorny <ano### [at] anonymousorg> wrote:
>>>> On 10/6/21 5:01 AM, jr wrote:
>>>>> ...
>>>> This a bug is basically the same as the radiosity file append bug.
>>           x
>>> a known issue, ok.  thanks.
>>
>> No. Well, it perhaps now is.
> 
> good, I think.  :-)
> 
> 
>> ...
>> I've not spent much time with POV-Ray's IO code.
> 
> sounds like opening a can of worms, only to find a multi-pack of can of worms
> inside..  think it .. bad not to have access to (tech) notes dealing with design
> etc of the "new" IO class.  bazaar, huh?!
> 
> coincidentally, have just posted a new macro in p.text.scene-files, 'Filed()', a
> CSV file handler, hope you'll find that useful when "stirring the worms" ;-).
> 

Thanks jr, I'll use it do some testing.


--- An update

+ Using 'if ((fid != nullptr) && (*fid))' is frequently proving useful.

+ My additional concern in radiosity.cpp's Save was justified. We are 
sometimes running the Save() code per render block(1) - without 
complaint from POV-Ray - when we in fact don't have a valid file handle. 
This particular issue is fixed in povr.

+ In many places finding the IO checking to be correct, which is good 
news. The less good news is, in a number of those situations, the code 
to generate messages for the end user is commented out or too generic to 
be of much help(2).

+ Unfortunately, we are often failing in complete silence. Stuff 
sometimes runs cleanly to all appearances, when there are in fact user 
file specification issues.

+ With povr, I've checked in a set of updates to add and/or improve 
warning/error messages both where the file IO checking was OK and where 
it was not. For example, I made changes to the previous randiosity.cpp 
samples file fix so that we now generate messages related to the 
radiosity samples file usage.

+ I think I now understand some of what changed v3.7 to v3.8 to 
introduce issues with the fopen keyword and invalid files.

+ I've got doubts about getting everything scrubbed anytime soon. Given 
the code (the IO class usage) is different file to file, use to use over 
a large code base. I don't currently see any easy way to flag potential 
problems aside from trying things and reviewing code.

Bill P.

(1) - Yep, and with this, we picked up a related puzzle with radiosity. 
Parts of the samples related code is running all the time! Meaning 
whether we have radiosity in the scene or not, whether the quality level 
supports radiosity or not - this samples code runs for each render 
block. The 'additional radiosity concern,' fixed now in the povr branch, 
extended what code was always run due testing for only != nullptr 
(testing true for a valid file handle). A puzzle / optimization for 
another day.

(2) - I learned yesterday the state file location is derived from the 
output image location! I wrongly thought the state file (aka continue 
trace log) was always local to where the render command was issued, but 
it ends up wherever you write the output image file. Further, if your 
output image location is bogus, the error message seen relates to the 
state file and not the output image file - which is confusing. I changed 
the error messages in a way which I hope will help.

Post a reply to this message

William F Pokorny <ano### [at] anonymousorg> wrote:
>
> (1) - Yep, and with this, we picked up a related puzzle with radiosity.
> Parts of the samples related code is running all the time! Meaning
> whether we have radiosity in the scene or not, whether the quality level
> supports radiosity or not - this samples code runs for each render
> block.

Wow! Good detective work.

>
> (2) - I learned yesterday the state file location is derived from the
> output image location! I wrongly thought the state file (aka continue
> trace log) was always local to where the render command was issued, but
> it ends up wherever you write the output image file.

Yes, that's the way it has been (in Windows versions) since the state-file
feature was added. I thought this was by design(!) It's usually not a problem--
I just delete the file when not needed-- unless it happens to get made while
rendering (and stopping) animations. Then it sometimes shows up within the
sequence of images in my output-image folder (because I usually do not use the
state-file to 'continue the trace' during such renders.) My animation apps then
stop processing any further images at the point where that file is detected--
but that's just a temporary annoyance, until I delete it.

Post a reply to this message

On 10/13/2021 5:32 PM, Kenneth wrote:

> Yes, that's the way it has been (in Windows versions) since the state-file
> feature was added. I thought this was by design(!) It's usually not a problem--
> I just delete the file when not needed-- unless it happens to get made while
> rendering (and stopping) animations. Then it sometimes shows up within the
> sequence of images in my output-image folder (because I usually do not use the
> state-file to 'continue the trace' during such renders.) My animation apps then
> stop processing any further images at the point where that file is detected--
> but that's just a temporary annoyance, until I delete it.
> 

Add to your .ini file
Create_Continue_Trace_Log=Off

I, too, do only animations and hate it when those random state files, 
that I'll never use, don't get deleted.

dik

Post a reply to this message

Dick Balaska <dic### [at] buckosoftcom> wrote:
> On 10/13/2021 5:32 PM, Kenneth wrote:
>
> > Yes, that's the way it has been (in Windows versions) since the state-file
> > feature was added. I thought this was by design(!) It's usually not a
> > problem...

>
> Add to your .ini file
> Create_Continue_Trace_Log=Off
>
> I, too, do only animations and hate it when those random state files,
> that I'll never use, don't get deleted.
>

Thanks! I had forgotten about that feature.

Post a reply to this message