Re: [icedtea-web][rfc] Fix of backup of deployment.properties onsomeOSs
Jacob Wisor
gitne at excite.co.jp
Tue May 14 10:48:13 PDT 2013
"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.
> >>>>
> >>>> 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.
> > 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
No, but seriously. This seems to be getting out of hand. Nevertheless, I have commented on your patch.
> 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.
> 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.
+ }
+
+ /**
+ * 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.
+ 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
+ //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.
+ 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."
+ }
+ }
+ 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.
+ // 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".
+ 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.
+ } 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?
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).
+ }
+ }
+
+ //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.
+ }
+ 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! ;-)
+ 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.
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.
Regards,
Jacob
More information about the distro-pkg-dev
mailing list