[icedtea-web][rfc] Fix of backup of deployment.properties onsome OSs

[Jiri Vanek]

On 05/13/2013 11:34 PM, Jacob Wisor wrote:
> "Jiri Vanek"<jvanek at xxxxxxxxxx> wrote:
>> On 05/07/2013 05:50 PM, Jacob Wisor wrote:
>>> "Jacob Wisor"<gitne at xxxxxxxxxxxx> wrote:
>>>> Hello!
>>>>
>>>> Some OSs (e.g. Windows) or file systems do not allow to overwrite while renaming or moving files. It could also depend on the implementation of java.io.File.renameTo(java.io.File), as its specification leaves open the exact semantics.

It is not just for Windows, this is clearly badly done in ITW:(

>>>>
>>>> Although the proposed fix is not really elegant, it should work on the majority of systems. Maybe the whole idea of renaming deployment.properties to always have a safe backup is just
>>
>> agree
>>
>>>> redundant on modern operating and file systems, since most of them feature journaled or transactional file systems, so there should always be a safe copy having integrity available.
>>>>
>>>> Happy reviewing!
>>>> Jacob
>>>
>>> Sorry, the webmailer keeps dropping attachements at random...
>>>
>> Hi! The patch looks ok, however I'm in doubts whether this quite complex solution is necessary.
>> What I have in mind is:
>> 1) Does .old file exists? If yes, delete it. (if it is directory then... ??? :))  I think we can
>> ignore this case. If Deletion failed, warn user and log exception(if none, then jsut some stderr
>> stuff)  (there would be nice some better error reporting - in comamndline mode[1] just print
>> exception, whether in gui[2] mode also show dialogue, but do not throw exception out - but it is
>> probably work for different changeset)
> 
> Right, deployment.properties.old can be deleted right away. I wanted to make sure that the backup file is also safe until the "renaming operation" is complete. This kind of mimiks the renaming semantics as originally intended.
> 
>> 2)Rename current properties to .old  - if it failed - again warn, but do not fail - maybe ask the
>> user to continue - not continue (in gui mode).
>> 3) save  new properties - if this failed - then this is the only reason for fatal exception imho.
> 

I should wrote "fatal" - classical IO exception which will become some WarningDialogue popup later.

> I am not sure whether I understand you correctly. The proposed solution only 

Yah and I'm afraid it should not. Some warning is enough. The exception which bubble out is to
strong (as the save can still work).

> fails when the properties file can not be backed up and though the application keeps on running,
the settings do not get saved.
> Yeah, there probably should be just a notice on stderr or the UI. In any case, the application should keep on running as this seems to me to be the expected intuitive behavior.
> So, I would rather keep the user unbothered, since he/she can

And stay unwarned that his/her saving failed?

> close the application at any time or keep on running anyway.

Well I do not like the "to much dependence on backup"

You approach is very nice (and by concept maybe best) with its effort to keep old properties in all
costs. Even for cost of complexity.

However I'm for less complexity in cost that the backup can fail.

See my attached patch, but I got trapped by file.canWrite() being less multiplatform then I hoped.
(But even without the nasty isDirectoryWriteable it shows more precisely what I had in mind)
Also see my splitting to static function with parameter - it is necessary for testing, and unittests
will be the necessary part of this commit [1]. Also see the moved  :

 FileUtils.createParentDir(userPropertiesFile);

line. Imho very necessary.


What is most discussable are all the new System.err. Probably we can live with them now, but for gui
mode there would be nice (in most cases) to have an "blah blah... Do you want to continue?" dialogue.

Feel free to disagree. I'm moreover just on doubts.

>   
>> What do you think?
>>
>> If you agree, then for now only the parts without the "user interactions" are worthy. But of course
>> if you are wiling to fix also the interaction then we will be more then happy (but please as
>> different changeset)
>>
>> Thank you for checking this!
>>
>> J.
>>
>>
>> [1]
>> http://icedtea.classpath.org/hg/icedtea-web/file/9f2d8381f5f1/netx/net/sourceforge/jnlp/controlpanel/CommandLine.java
>> [2]
>> http://icedtea.classpath.org/hg/icedtea-web/file/9f2d8381f5f1/netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java
> 
> Thank you for the guiding references.
> 

Best regards,
  J.

[1] http://icedtea.classpath.org/wiki/CommitPolicy : "IcedTea-Web code changes/new feature should be
accompanied with appropriate tests (JUnit class and/or reproducer). If no tests are added/modified,
changes should be accompanied with an explanation as to why. "
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Fix of backup of deployment.properties on some OSs-2.patch
Type: text/x-patch
Size: 5515 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130514/bacfdce3/Fixofbackupofdeployment.propertiesonsomeOSs-2.patch