You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@jmeter.apache.org by pm...@apache.org on 2013/09/08 22:39:35 UTC
svn commit: r1520921 -
/jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java
Author: pmouawad
Date: Sun Sep 8 20:39:35 2013
New Revision: 1520921
URL: http://svn.apache.org/r1520921
Log:
Test for existence before trying to delete (thanks sebb)
Modified:
jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java
Modified: jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java
URL: http://svn.apache.org/viewvc/jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java?rev=1520921&r1=1520920&r2=1520921&view=diff
==============================================================================
--- jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java (original)
+++ jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java Sun Sep 8 20:39:35 2013
@@ -118,7 +118,7 @@ public class KeyToolUtils {
public static void generateProxyCA(File keystore, String password, int validity) throws IOException {
keystore.delete(); // any existing entries will be invalidated anyway
// not strictly needed
- if(!new File(CACERT).delete()) {
+ if(new File(CACERT).exists() && !new File(CACERT).delete()) {
// Noop as we accept not to be able to delete it
log.warn("Could not delete file:"+new File(CACERT).getAbsolutePath()+", will continue ignoring this");
}
Re: svn commit: r1520921 - /jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java
Posted by sebb <se...@gmail.com>.
On 8 September 2013 21:48, Philippe Mouawad <ph...@gmail.com> wrote:
> Hello,
> In this case why leave this code:
> //not strictly needed
> new File(CACERT).delete();
>
> As reading it I understand it is not an issue if delete fails? while in
> fact it is ?
The task here is to update the key store and generate a new certificate.
If the gencert cannot create the file, it will fail at that point.
That's why I added the comment that the CACERT delete was not necessary.
I put the deletes in originally partly for tidiness.
Delete the output files at the start, and then recreate them.
Also the CACERT file is useless without the key store.
Question is: do we need to know if the delete failed?
If not, there's no point checking and reporting it.
If we do need to know, then we should probably report failure to
delete the key store as well.
But if we do go down that route, then it's probably safer to use code like:
if (!file.delete() && file.exists()) {
// report error
}
>
> On Sun, Sep 8, 2013 at 10:46 PM, sebb <se...@gmail.com> wrote:
>
>> On 8 September 2013 21:39, <pm...@apache.org> wrote:
>> > Author: pmouawad
>> > Date: Sun Sep 8 20:39:35 2013
>> > New Revision: 1520921
>> >
>> > URL: http://svn.apache.org/r1520921
>> > Log:
>> > Test for existence before trying to delete (thanks sebb)
>> >
>> > Modified:
>> > jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java
>> >
>> > Modified:
>> jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java
>> > URL:
>> http://svn.apache.org/viewvc/jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java?rev=1520921&r1=1520920&r2=1520921&view=diff
>> >
>> ==============================================================================
>> > --- jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java
>> (original)
>> > +++ jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java
>> Sun Sep 8 20:39:35 2013
>> > @@ -118,7 +118,7 @@ public class KeyToolUtils {
>> > public static void generateProxyCA(File keystore, String password,
>> int validity) throws IOException {
>> > keystore.delete(); // any existing entries will be invalidated
>> anyway
>> > // not strictly needed
>> > - if(!new File(CACERT).delete()) {
>> > + if(new File(CACERT).exists() && !new File(CACERT).delete()) {
>>
>> I still think it's better to leave the failure reporting to the gencert
>> command.
>>
>> e.g. if the CACERT file is read-only, we will now get a warning and an
>> error.
>>
>> > // Noop as we accept not to be able to delete it
>> > log.warn("Could not delete file:"+new
>> File(CACERT).getAbsolutePath()+", will continue ignoring this");
>> > }
>> >
>> >
>>
>
>
>
> --
> Cordialement.
> Philippe Mouawad.
Re: svn commit: r1520921 - /jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java
Posted by Philippe Mouawad <ph...@gmail.com>.
Hello,
In this case why leave this code:
//not strictly needed
new File(CACERT).delete();
As reading it I understand it is not an issue if delete fails? while in
fact it is ?
On Sun, Sep 8, 2013 at 10:46 PM, sebb <se...@gmail.com> wrote:
> On 8 September 2013 21:39, <pm...@apache.org> wrote:
> > Author: pmouawad
> > Date: Sun Sep 8 20:39:35 2013
> > New Revision: 1520921
> >
> > URL: http://svn.apache.org/r1520921
> > Log:
> > Test for existence before trying to delete (thanks sebb)
> >
> > Modified:
> > jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java
> >
> > Modified:
> jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java?rev=1520921&r1=1520920&r2=1520921&view=diff
> >
> ==============================================================================
> > --- jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java
> (original)
> > +++ jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java
> Sun Sep 8 20:39:35 2013
> > @@ -118,7 +118,7 @@ public class KeyToolUtils {
> > public static void generateProxyCA(File keystore, String password,
> int validity) throws IOException {
> > keystore.delete(); // any existing entries will be invalidated
> anyway
> > // not strictly needed
> > - if(!new File(CACERT).delete()) {
> > + if(new File(CACERT).exists() && !new File(CACERT).delete()) {
>
> I still think it's better to leave the failure reporting to the gencert
> command.
>
> e.g. if the CACERT file is read-only, we will now get a warning and an
> error.
>
> > // Noop as we accept not to be able to delete it
> > log.warn("Could not delete file:"+new
> File(CACERT).getAbsolutePath()+", will continue ignoring this");
> > }
> >
> >
>
--
Cordialement.
Philippe Mouawad.
Re: svn commit: r1520921 - /jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java
Posted by sebb <se...@gmail.com>.
On 8 September 2013 21:39, <pm...@apache.org> wrote:
> Author: pmouawad
> Date: Sun Sep 8 20:39:35 2013
> New Revision: 1520921
>
> URL: http://svn.apache.org/r1520921
> Log:
> Test for existence before trying to delete (thanks sebb)
>
> Modified:
> jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java
>
> Modified: jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java
> URL: http://svn.apache.org/viewvc/jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java?rev=1520921&r1=1520920&r2=1520921&view=diff
> ==============================================================================
> --- jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java (original)
> +++ jmeter/trunk/src/jorphan/org/apache/jorphan/exec/KeyToolUtils.java Sun Sep 8 20:39:35 2013
> @@ -118,7 +118,7 @@ public class KeyToolUtils {
> public static void generateProxyCA(File keystore, String password, int validity) throws IOException {
> keystore.delete(); // any existing entries will be invalidated anyway
> // not strictly needed
> - if(!new File(CACERT).delete()) {
> + if(new File(CACERT).exists() && !new File(CACERT).delete()) {
I still think it's better to leave the failure reporting to the gencert command.
e.g. if the CACERT file is read-only, we will now get a warning and an error.
> // Noop as we accept not to be able to delete it
> log.warn("Could not delete file:"+new File(CACERT).getAbsolutePath()+", will continue ignoring this");
> }
>
>