[icedtea-web][rfc] Fix of backup of deployment.properties onsomeOSs
Jiri Vanek
jvanek at redhat.com
Wed May 15 07:20:26 PDT 2013
On 05/14/2013 07:48 PM, Jacob Wisor wrote:
> "Jiri Vanek"<jvanek at redhat.com> wrote:
>> 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.
this sentence ^ is important ;)
>>>>>>
>>>>>> 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?
>
> No, that is what I actually meant by "keep the application running and not to bother the user": warn the user, but do not ask to quit or keep running. An info message box should warn the user about either being unable to save modifications to the config or backup the config.
Well if not to bother uzer, then both IOexception you thrown were incorrect (Am I missing
something??). I think That user *should* be warned in gui mode, (and to continue in headless mode).
No throwing - this throwing caused to not to save the new properties.
>
>>> close the application at any time or keep on running anyway.
>>
>> Well I do not like the "to much dependence on backup"
>
> There is no dependency on backing up the config. As long as the the current config can be saved everything is fine, even in the event of the little flaw that the old config can not be backed up. ;-)
>
>> 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.
>
> You call this patch less complex? :-D
ehm ..no O:) Sorry As told, I got trapped into "writtable directory" hell.
> No, but seriously. This seems to be getting out of hand. Nevertheless, I have commented on your patch.
Ouch. My patch was not mentioned to be "patch", or ever to be pushed. It was moreover what I have in
mind. I hooped you will simplify your patch to some subset of yopur original good one, and my second
"simple and evil" one.
>
>> 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.
>
> Agreed, this is definitly correct (or was a bug). I missed it the first time too. This suffices to call for an update/patch.
goood.
>
>> 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. "
>
> /**
> + * Backups current
> + * <code>deployment.properties</code> file. This elaborate scheme is
> + * necessary to handle OSs that do not allow to overwrite while renaming or
> + * moving onto an existing file.
> + */
> + private boolean backupPropertiesFile() throws IOException {
> + return backupFile(userPropertiesFile);
>
> This is rather redundant. A supposed replacement by a static method for backing up files should be placed in FileUtils.
Don't think so. Depends on personal feelings l.Feel free to keep or remove.
>
> + }
> +
> + /**
> + * Backups current given file. This elaborate scheme is necessary to handle
> + * OSs that do not allow to overwrite while renaming or moving onto an
> + * existing file.
> + */
> + //package private for testing
> + static boolean backupFile(File f) throws IOException {
>
> This method should definitely be moved to FileUtils, since it provides general-purpose functionality.
> There is no use for declaring this method throwing an IOException.
>
> + //do all this madnes only if there is something to backup
>
> madnes -> madness
>
> + if (f.isFile()) {
>
> Please do check the method's parameters for validity before first use and throw an IllegalArgumentException if invalid, or catch a possible NullPointerException on the first use and chain it to an IllegalArgumentException. A simple null check should be enough in this case.
No - f.isFile() was checking also for existence of this. If there is no *file* to backup, then there
is nothing to do. No exception needed.
Catching of NPE can have two sharp edges. In this case I'm 100% for et it be thrown.
>
> + File backupPropertiesFile = new File(f.getAbsolutePath() + ".old");
> + //if old backup exists, remove it - some OSs do not suport later renaming to
>
> suport -> support
> Btw, does a backup have to be /old/? :-D
I'm afraid we have to keep the .old due to backward comaptibility :(
But the utility method can have parameter holding the suffix.
>
> + //already existing file
> + if (backupPropertiesFile.isFile()) {
> + boolean success = backupPropertiesFile.delete();
>
> The local variable "success" has no real meaning and any further use. Drop it, please. Besides, "success" is a quite meaningless term.
Depend on taste. I like as thin code ass possible == as few commands on line as possible
>
> + if (!success) {
> + System.err.println("There was an error deleting " + backupPropertiesFile + ". Backup may not be possible and future operations may fail");
>
> Please load these messages from resources and use simple English: "Can not backup <f>, because <backupFile> can not be deleted."
100% agree. I would do if I ment this as patch and not as idea. And sorry for my english :(.
Also I won't to highlight places which maybe deserves attention on gui/headless mode.
>
> + }
> + }
> + if (backupPropertiesFile.isDirectory()) {
> + System.err.println("The file used for backuping is directory - " + backupPropertiesFile + ". Buckup will not be possible and future operations will fail");
> + }
>
> This if-block is unnecessary and can be dropped, because by issuing File.isFile() we have already checked whether the backup file exists and is indeed a normal file.
True. I added the isFile later.
>
> + // Try to rename current file to file.old (which is not supposed to exists now)
> + boolean success = f.renameTo(backupPropertiesFile);
>
> Please get rid of the meaningless local variable "success".
I really like them :)
>
> + if (!success) {
> + System.err.println("Error to backup " + f + " to " + backupPropertiesFile);
>
> Please load these messages from resources.
>
> + }
> + return success;
>
> Use "return true" instead for better readability and performance, see above.
there should be else more then anything else.
>
> + } else {
> + File parent = f.getAbsoluteFile().getParentFile();
> + if (parent != null) {
> + return isDirectoryWriteable(parent);
>
> isDirectoryWriteable -> isDirectoryWritable
>
> + } else {
> + return false;
> + }
>
> I am sorry, but I do not understand the reasoning behind this. Why should a check be performed on the backed up file's parent directory - whether it is writable - when the file to be backed up does not exist or is a directory? Besides, what does the return value "true" mean when the non-existent file's parent directory or directory being backed up is writable but still has not been backed up?
Definitely is unnecessary burden here.
I got into this when I was thinking what this function should return if there is nothing to backup.
Probably true is enough :)
I wanted to return something mor relvant.. well to much ideas in time probably :)
>
> If "f" does not exist or is a directory simply return "false", because there is nothing that can be done anyway. If you want to handle backup of files and directories in one method, then please provide additional code to backup directories too, otherwise it should be split into e.g. backupFile(File) and backupDirectory(File).
I would like any possible simplification :)
>
> + }
> + }
> +
> + //package private for testing, probably candindate to FileUtils class
> + static boolean isDirectoryWriteable(final File f) throws IOException {
> + if (!f.isDirectory()) {
>
> Check "f" for "null" before first dereferencing.
>
> + throw new IOException(f + " have to be directory");
>
> Throw rather IllegalArgumentException than IOException since no I/O operation has been actually done. Please note also, that File.isDirectory() tests for existence as well, hence isDirectoryWritable() is going to have a misleading or incorrect semantics when "f" denotes a non-existent directory.
depends.. Perhaps. I do not insists on IoException. But as there can be alter catch for IOexception
from this topic later, I decided for it. nvm
>
> + }
> + File ff = null;
> + try {
> + File.createTempFile("itw", "isDirWriteable", f);
> + if (ff != null && ff.exists()) {
>
> At what point should "ff" become non-null? Besides, "ff" is a meaningless and cryptic name for a variable. Please use longer expressive and descriptive variable names. It ain't K&R C anymore! ;-)
:DDD /me ashamed/ Best would be to abandon whole this terrible check.
>
> + return true;
> + } else {
> + return false;
> + }
> + } catch (IOException ex) {
> + if (JNLPRuntime.isDebug()) {
> + ex.printStackTrace();
> + }
> + return false;
> + } finally {
> + if (ff != null && ff.exists()) {
> + boolean deleted = ff.delete(); ///die silently?
> + if (JNLPRuntime.isDebug() && !deleted) {
> + System.err.println("can not delete just created tmp file " + ff.getAbsolutePath());
> + }
> + }
> + }
> + }
>
> After all, I am not convinced that this is a suitable way to test a directory for being writable. I also do not fully understand why it is actually needed right now.
What can be best then create and delte file in it?
>
> This should be enough nitpicking for now. Thank you for taking a look into this problem. I was supposed to offer a revised patch based on my first proposal, but I am unsure how to proceed next. Should I still propose my revise version.
Yes! As I told, those were just coded-ideas. Whatever you will manage now will be better then my
nasty approach.
My points form last time are still valid
- make it more simple,
- make it unittestable
- and add those tests
+ few nits we foud later. Especially the
>> FileUtils.createParentDir(userPropertiesFile);
>> line. Imho very necessary.
Thanx for checking this!
J.
Btw will you update the manifest patch? Imho worthy O:)
More information about the distro-pkg-dev
mailing list