Pull request updated. I think there were a lot of little bugs in the writing
of the extensions area in the file.
I believe I've got it all fixed now.
On Oct 30, 2012, at 4:17 PM, Larry Gritz wrote:
> Yeah, there are a few more things that look very sketchy. Let me take
> another look...
>
>
>
> On Oct 30, 2012, at 3:04 PM, Nicolas Burtnyk wrote:
>
>> Hi Larry,
>>
>> I took a peek at your pull request and although it looks like this
>> change will cleanly close tga files on write errors, it still seems
>> like we're bailing too early in the case of empty author or document
>> name fields.
>>
>> For example, in the block of code below, if the image spec doesn't
>> have an "Artist" attribute (and hence tmpstr is empty), we early
>> return false form write_tga20_data_fields skipping all other fields.
>>
>> if (!fwrite (tmpstr.c_str(), std::min (tmpstr.length (), size_t(40))))
>> return false;
>>
>> Is that the intended behavior?
>>
>> I guess the core of the problem is that the fwrite helper function is
>> returning false not only on write errors, but also when it is asked to
>> write zero bytes and no attempt is made to distinguish bewteen those 2
>> cases.
>>
>> Perhaps the solution is to have the helper fwrite return true when
>> itemsize or nitems is zero?
>>
>> Also, I noticed the block of code that fills with zeros after writing
>> the "job ID" field (taken from the "DocumentName" attribute) looks
>> kind of sketchy:
>>
>> // job ID
>> tmpstr = m_spec.get_string_attribute ("DocumentName", "");
>> if (!fwrite (tmpstr.c_str(), std::min (tmpstr.length (), size_t(40))))
>> return false;
>>
>> // fill the rest with zeros
>> for (int i = 41 - std::min (tmpstr.length (), size_t(40)); i > 0; i--) {
>> if (!fwrite (&tmpint) ||
>> !fwrite (tmpstr.c_str(), std::min (tmpstr.length (), size_t(40))))
>> return false;
>> }
>>
>> Why is it trying to write the DocumentName string again every time it
>> successfully writes 1 zero?
>>
>>
>> -Nicolas
>>
>>
>> On Tue, Oct 30, 2012 at 12:44 PM, Larry Gritz <[email protected]> wrote:
>>> https://github.com/OpenImageIO/oiio/pull/448
>>>
>>>
>>> On Oct 30, 2012, at 10:53 AM, Larry Gritz wrote:
>>>
>>>> Thanks for pointing that out.
>>>>
>>>> Yes, in fact there are also several places in TGAOutput::open() where the
>>>> file handle is not properly closed.
>>>>
>>>> Sounds easy to fix. Stay tuned for a pull request from me.
>>>>
>>>>
>>>>
>>>> On Oct 30, 2012, at 10:39 AM, Nicolas Burtnyk wrote:
>>>>
>>>>> While testing OIIO, I noticed that TGAs that were being written to
>>>>> disk were not being closed despite calling ImageOutput::close().
>>>>> Indeed fclose is not called in TGAOutput::close when the "Artist"
>>>>> attribute is empty.
>>>>> The problematic block of code is:
>>>>>
>>>>> if (!fwrite (tmpstr.c_str(), std::min (tmpstr.length (), size_t(40)))) {
>>>>> return false;
>>>>> }
>>>>>
>>>>> Since tmpstr is empty, the itemsize parameter of fwrite is 0 and
>>>>> fwrite returns false, which in turn makes TGAOutput::close early
>>>>> return false (skipping the call to fclose).
>>>>>
>>>>> Writing of other string attributes are similarly problematic when the
>>>>> attribute is empty.
>>>>>
>>>>> -Nicolas
>>>
>>> --
>>> Larry Gritz
>>> [email protected]
>>>
>>>
>>> _______________________________________________
>>> Oiio-dev mailing list
>>> [email protected]
>>> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
>> _______________________________________________
>> Oiio-dev mailing list
>> [email protected]
>> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
>
> --
> Larry Gritz
> [email protected]
>
>
> _______________________________________________
> Oiio-dev mailing list
> [email protected]
> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
--
Larry Gritz
[email protected]
_______________________________________________
Oiio-dev mailing list
[email protected]
http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
