Re: 5 bug/misc. fixes for ppf_PakFile.cc

[Christian Reiniger <warewolf@mayn.de>]

Bjarke Hammersholt Roune wrote:

>There is what I would regard as a cosmetic bug in ppf_PakFile::Mount().
>At a point, it checks to see wheter write access is specified in its
>argument, and if it is, it checks to see that the pak file in question is
>of format 1. If this is not the case, this line gets executed:
>
>ppDebug1 ("PakFile format doesn't support write access");
>
>This isn't strictly correct. The PakFile format does support write access
>(or it will), however, it is correct that pakfiles of format 0 does not, so
>the line should rather be something like this:
>
>ppDebug1 ("PakFiles of format 0 doesn't support write access");

Or better: "This PakFile's format doesn't support write access" as only
format #1 is writeable. Better to have a more generic message here.

>There is another thing in ppf_PakFile::Mount() that doesn't really have to
>be a bug, but it puzzels me somewhat.
>
>The RDAttribs member of ppf_PakFile has its ppfFA_executable bit set,
>nomatter the cursumstances (outside of a failure). Why?
>
>btw, what does the RD in RDAttribs stand for?

For "Root Directory".
RDAttribs contains the attributes of the PakFile's root directory.
ppfFA_executable means "searchable" if set for a directory, i.e. it is
allowed to change to that directory. That's a convention from most Unix
filesystems.

>I noticed this little piece of code in ppf_PakFile::Mount():
>
>try {
>     root_dir = new ppf_PakFileDir (tmpfilename,
>        this,
>        93,
>        0,
>        RDAttribs,
>        creation_time,
>        creation_time);
>} catch (ppException &XCept) {
>     delete[] tmpfilename;
>    root_dir = 0;            // memory leak ??
>
>     ppWarning ("ppf_PakFileDir construction failed");
>    return false;
>}
>
>I checked what happends if an exception is thrown in a constructor by
>creating a little test program (attached). Setting root_dir to 0 is not only
>not a memory leak, it is also redundant, as the value of root_dir is never
>altered in this piece of code if an exception is thrown from
>ppf_PakFileDir::ppf_PakFileDir().

Ok, removed the comment. I'll leave the assignment in place nevertheless to
be on the safer side (some compiler might actually change root_dir
nevertheless).

>There is also another thing with this code, though: If you are not going to
>use a variable, would you please not name it? (here I'm talking about XCept)

That's a purely automatic thing ;) But you're right. Corrected it.

>The first line of ppf_PakFile::Mount() is:
>
>char *tmpfilename = ppf_RemoveExtension (filename);
>
>The caller of ppf_RemoveExtension() must itself delete[] the returned
>string.

>Now, what happends in the called constructor of ppf_Directory is that a copy
>is made of the first parameter. It is *not*, however, deleted: We have a
>memory leak. tmpfile should be delete[]d in ppf_PakFile::Mount(), regardless
>of wheter or not the called constructor of ppf_PakFileDir throws an
>exception.

Thanks. Corrected.

>Also, tmpfilename should not be initialised before its needed, as there is
>several places that can cause ppf_PakFile::Mount() to return before the
>point it is used. It currently is initialised at the very top of
>ppf_PakFile::Mount().

Corrected.

>The member variable handle (the name of the variable is handle) of
>ppf_PakFile is in the constructor ppf_PakFile::ppf_PakFile() initialised to
>0. This constructor then proceeds to call ReadHeader() without doing
>anything more to the member variable handle.
>
>The problem is that ReadHeader() expects the member variable handle to be a
>handle to an open file which header it can read...

Ooops, right. I "corrected" that in the wrong way ;)
Now it should be ok.

>ppf_PakFile::ppf_PakFile() should open a file and put the handle in the
>member variable handle before calling ReadHeader().

Now ReadHeader opens/closes the Pak itself.


	Christian
--

Drive A: not responding...Formatting C: instead