You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Stefan Bodewig <bo...@apache.org> on 2009/02/05 14:15:34 UTC
[compress] SANDBOX-246 remaining Findbugs issues
Since Sebb's original report the code base has changed quite a bit so
I reran findbugs.
I've fixed most of them, still open are:
* JarArchiveEntry certificates and manifestAttributes is never written
to, so they are useless.
I'm unsure of the class' purpose and simply left things as they are,
assuming setters will be provided one day.
* ZipOutputStream contains some proteted static final byte[]
"constants"
This means any subclass could modify them. Findbugs suggests to
make the package private. Ant couldn't do that because of backwards
incompatibility, but a sandbox component can. Should we?
* ArchiveStreamFactory should do something when it fails to read
enough bytes for the signature, but what?
Given the original TODO comment, I stayed away from a decision.
* CpioArchiveEntry#setMode first performs some work to check the mode
just passed in, creates an IllegalArgumentException if it is unknown
and then forgets to throw it.
If I change the code to actually throw the exception,
testCpioUnarchive fails. Obviously the code supports more modes
than it thinks.
Remove the checks?
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] SANDBOX-246 remaining Findbugs issues
Posted by sebb <se...@gmail.com>.
On 05/02/2009, Christian Grobmeier <gr...@gmail.com> wrote:
> Hi,
> thanks Stefan for running findbugs! I'll try to give answers as far as
> I know till now.
>
>
> > * JarArchiveEntry certificates and manifestAttributes is never written
> > to, so they are useless.
> >
> > I'm unsure of the class' purpose and simply left things as they are,
> > assuming setters will be provided one day.
>
>
> It's planned that the *Entry classes provide additional information
> for the archiver. For example see TarArchiveEntry, which gives you the
> chance to add a username to the file. In the case of JarArchiveEntry
> it's simply not finished and we need to extend this when the rest of
> the work is more cool.
>
>
> > * ZipOutputStream contains some proteted static final byte[]
> > "constants"
> >
> > This means any subclass could modify them. Findbugs suggests to
> > make the package private. Ant couldn't do that because of backwards
> > incompatibility, but a sandbox component can. Should we?
>
>
> I guess we can, but I think we should wait after the first release. At
But then you again have the problem of backwards compatibility.
I'd say make the byte[] arrays private if possible; there is likely a
better way to use the constants.
Leaving the constants exposed to mutation should be a last resort.
> the moment I am copying Ant-codebase to here. If Ant is changing
> forward I may have problems if we change such kind of stuff when
> updating. Otherwise we will have development on the same code in two
> projects and that may be diffcult.
>
>
> > * ArchiveStreamFactory should do something when it fails to read
> > enough bytes for the signature, but what?
> >
> > Given the original TODO comment, I stayed away from a decision.
>
>
> I have not thought about that. At first glance I guess it should throw
> an ArchiverException.
>
>
> > * CpioArchiveEntry#setMode first performs some work to check the mode
> > just passed in, creates an IllegalArgumentException if it is unknown
> > and then forgets to throw it.
> >
> > If I change the code to actually throw the exception,
> > testCpioUnarchive fails. Obviously the code supports more modes
> > than it thinks.
>
>
> The testcase is not the best I ever wrote. I can digg into this and
> will improve all testcases. The CPIO stuff must be reviewed since it
> came in from another project just before a while.
>
>
> Christian
>
>
>
> > Remove the checks?
>
>
>
> >
> > Stefan
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] SANDBOX-246 remaining Findbugs issues
Posted by Niall Pemberton <ni...@gmail.com>.
On Fri, Feb 6, 2009 at 3:18 AM, Stefan Bodewig <bo...@apache.org> wrote:
> On 2009-02-05, Christian Grobmeier <gr...@gmail.com> wrote:
>
>>> * ZipOutputStream contains some proteted static final byte[]
>>> "constants"
>
>>> This means any subclass could modify them. Findbugs suggests to
>>> make the package private. Ant couldn't do that because of backwards
>>> incompatibility, but a sandbox component can. Should we?
>
>> I guess we can, but I think we should wait after the first release.
>
> I'm with Sebb here and would remove any part of the API that I'm
> uncomfortable with before the release.
+1
Niall
>> At the moment I am copying Ant-codebase to here. If Ant is changing
>> forward I may have problems if we change such kind of stuff when
>> updating.
>
> You may have noticed how I merged stuff from Ant a few times over the
> past days and for the time being I intend to keep doing that, don't
> worry.
>
> The svn merges were more or less painless, the only conflict I got was
> in UnknownExtraField because the commons-compress version uses m_foo
> fields where Ant uses foo (likely because of Avalon/Peter Donald
> coding style).
>
>>> * ArchiveStreamFactory should do something when it fails to read
>>> enough bytes for the signature, but what?
>
>>> Given the original TODO comment, I stayed away from a decision.
>
>> I have not thought about that. At first glance I guess it should throw
>> an ArchiverException.
>
> An empty bzip2 compressed file is 14 bytes, maybe there are some
> formats that we want to support in the future that create even shorter
> outputs.
>
> I suggest we add the number of bytes actually read as a second
> argument to the various matches methods and will end up with no
> InputStream matching the file's signature instead.
>
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] SANDBOX-246 remaining Findbugs issues
Posted by Christian Grobmeier <gr...@gmail.com>.
> The svn merges were more or less painless, the only conflict I got was
> in UnknownExtraField because the commons-compress version uses m_foo
> fields where Ant uses foo (likely because of Avalon/Peter Donald
> coding style).
If you could do that - yes, thats cool then.
> I suggest we add the number of bytes actually read as a second
> argument to the various matches methods and will end up with no
> InputStream matching the file's signature instead.
OK
Christian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] SANDBOX-246 remaining Findbugs issues
Posted by Stefan Bodewig <bo...@apache.org>.
On 2009-02-05, Christian Grobmeier <gr...@gmail.com> wrote:
>> * ZipOutputStream contains some proteted static final byte[]
>> "constants"
>> This means any subclass could modify them. Findbugs suggests to
>> make the package private. Ant couldn't do that because of backwards
>> incompatibility, but a sandbox component can. Should we?
> I guess we can, but I think we should wait after the first release.
I'm with Sebb here and would remove any part of the API that I'm
uncomfortable with before the release.
> At the moment I am copying Ant-codebase to here. If Ant is changing
> forward I may have problems if we change such kind of stuff when
> updating.
You may have noticed how I merged stuff from Ant a few times over the
past days and for the time being I intend to keep doing that, don't
worry.
The svn merges were more or less painless, the only conflict I got was
in UnknownExtraField because the commons-compress version uses m_foo
fields where Ant uses foo (likely because of Avalon/Peter Donald
coding style).
>> * ArchiveStreamFactory should do something when it fails to read
>> enough bytes for the signature, but what?
>> Given the original TODO comment, I stayed away from a decision.
> I have not thought about that. At first glance I guess it should throw
> an ArchiverException.
An empty bzip2 compressed file is 14 bytes, maybe there are some
formats that we want to support in the future that create even shorter
outputs.
I suggest we add the number of bytes actually read as a second
argument to the various matches methods and will end up with no
InputStream matching the file's signature instead.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] SANDBOX-246 remaining Findbugs issues
Posted by Christian Grobmeier <gr...@gmail.com>.
Hi,
thanks Stefan for running findbugs! I'll try to give answers as far as
I know till now.
> * JarArchiveEntry certificates and manifestAttributes is never written
> to, so they are useless.
>
> I'm unsure of the class' purpose and simply left things as they are,
> assuming setters will be provided one day.
It's planned that the *Entry classes provide additional information
for the archiver. For example see TarArchiveEntry, which gives you the
chance to add a username to the file. In the case of JarArchiveEntry
it's simply not finished and we need to extend this when the rest of
the work is more cool.
> * ZipOutputStream contains some proteted static final byte[]
> "constants"
>
> This means any subclass could modify them. Findbugs suggests to
> make the package private. Ant couldn't do that because of backwards
> incompatibility, but a sandbox component can. Should we?
I guess we can, but I think we should wait after the first release. At
the moment I am copying Ant-codebase to here. If Ant is changing
forward I may have problems if we change such kind of stuff when
updating. Otherwise we will have development on the same code in two
projects and that may be diffcult.
> * ArchiveStreamFactory should do something when it fails to read
> enough bytes for the signature, but what?
>
> Given the original TODO comment, I stayed away from a decision.
I have not thought about that. At first glance I guess it should throw
an ArchiverException.
> * CpioArchiveEntry#setMode first performs some work to check the mode
> just passed in, creates an IllegalArgumentException if it is unknown
> and then forgets to throw it.
>
> If I change the code to actually throw the exception,
> testCpioUnarchive fails. Obviously the code supports more modes
> than it thinks.
The testcase is not the best I ever wrote. I can digg into this and
will improve all testcases. The CPIO stuff must be reviewed since it
came in from another project just before a while.
Christian
> Remove the checks?
>
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org