[RFC][icedtea-web:netx]: handle error code returning functions
Omair Majid
omajid at redhat.com
Fri Mar 11 06:49:54 PST 2011
On 03/10/2011 07:21 PM, Denis Lila wrote:
> Hi.
>
>> > I do have a few concerns noted inline below.
> The attached patch addresses those (except I use
> exists() instead of isDirectory() to check whether
> the parent exits, because it's a parent so it must
> be a directory).
>
Not sure if that's entirely correct. A user may accidentally create a
file with that name. exists() will return true even though it's not a
directory. For robustness's sake, I think we should always explicitly
check for a directory.
> I also moved the error handling logic in separate
> utility functions, so now everything is cleaner.
>
Some comments below.
> * netx/net/sourceforge/jnlp/Parser.java
> diff -r d91d02e798ff netx/net/sourceforge/jnlp/Launcher.java
> --- a/netx/net/sourceforge/jnlp/Launcher.java Wed Mar 09 13:51:48 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/Launcher.java Thu Mar 10 19:17:52 2011 -0500
> @@ -709,7 +709,7 @@
> File netxRunningFile = new File(JNLPRuntime.getConfiguration()
> .getProperty(DeploymentConfiguration.KEY_USER_NETX_RUNNING_FILE));
> if (!netxRunningFile.exists()) {
> - netxRunningFile.getParentFile().mkdirs();
> + FileUtils.createParentDir(netxRunningFile, null);
> FileUtils.createRestrictedFile(netxRunningFile, true);
> FileOutputStream fos = new FileOutputStream(netxRunningFile);
> try {
> diff -r d91d02e798ff netx/net/sourceforge/jnlp/cache/CacheDirectory.java
> --- a/netx/net/sourceforge/jnlp/cache/CacheDirectory.java Wed Mar 09 13:51:48 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/cache/CacheDirectory.java Thu Mar 10 19:17:52 2011 -0500
> @@ -39,6 +39,8 @@
> import java.io.File;
> import java.util.ArrayList;
>
> +import net.sourceforge.jnlp.util.FileUtils;
> +
> public class CacheDirectory {
> /**
> * Get the structure of directory for keeping track of the protocol and
> @@ -103,7 +105,7 @@
> if (parent.getParent() == null)
> return; // Don't delete the root.
> if (parent.getChildren().size() == 0) {
> - parent.getFile().delete();
> + FileUtils.deleteWithErrMesg(parent.getFile(), null);
> parent.getParent().removeChild(parent);
> cleanParent(parent);
> }
> diff -r d91d02e798ff netx/net/sourceforge/jnlp/cache/CacheUtil.java
> --- a/netx/net/sourceforge/jnlp/cache/CacheUtil.java Wed Mar 09 13:51:48 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/cache/CacheUtil.java Thu Mar 10 19:17:52 2011 -0500
> @@ -291,7 +291,7 @@
> String cacheDir = JNLPRuntime.getConfiguration()
> .getProperty(DeploymentConfiguration.KEY_USER_CACHE_DIR);
> File localFile = urlToPath(source, cacheDir);
> - localFile.getParentFile().mkdirs();
> + FileUtils.createParentDir(localFile, null);
>
> return localFile;
> } catch (Exception ex) {
> diff -r d91d02e798ff netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java
> --- a/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java Wed Mar 09 13:51:48 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java Thu Mar 10 19:17:52 2011 -0500
> @@ -38,6 +38,7 @@
> import javax.naming.ConfigurationException;
>
> import net.sourceforge.jnlp.runtime.JNLPRuntime;
> +import net.sourceforge.jnlp.util.FileUtils;
>
> /**
> * Manages the various properties and configuration related to deployment.
> @@ -518,7 +519,7 @@
> }
> }
>
> - userPropertiesFile.getParentFile().mkdirs();
> + FileUtils.createParentDir(userPropertiesFile, null);
> OutputStream out = new BufferedOutputStream(new FileOutputStream(userPropertiesFile));
> try {
> toSave.store(out, DEPLOYMENT_COMMENT);
> diff -r d91d02e798ff netx/net/sourceforge/jnlp/controlpanel/CachePane.java
> --- a/netx/net/sourceforge/jnlp/controlpanel/CachePane.java Wed Mar 09 13:51:48 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/controlpanel/CachePane.java Thu Mar 10 19:17:52 2011 -0500
> @@ -43,6 +43,7 @@
> import net.sourceforge.jnlp.cache.DirectoryNode;
> import net.sourceforge.jnlp.config.DeploymentConfiguration;
> import net.sourceforge.jnlp.runtime.Translator;
> +import net.sourceforge.jnlp.util.FileUtils;
>
> public class CachePane extends JPanel {
>
> @@ -132,7 +133,7 @@
> DirectoryNode fileNode = ((DirectoryNode) cacheTable.getModel().getValueAt(modelRow, 0));
> if (fileNode.getFile().delete()) {
> fileNode.getParent().removeChild(fileNode);
> - fileNode.getInfoFile().delete();
> + FileUtils.deleteWithErrMesg(fileNode.getInfoFile(), null);
> ((DefaultTableModel) cacheTable.getModel()).removeRow(modelRow);
> cacheTable.getSelectionModel().setSelectionInterval(row, row);
> CacheDirectory.cleanParent(fileNode);
> diff -r d91d02e798ff netx/net/sourceforge/jnlp/resources/Messages.properties
> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties Wed Mar 09 13:51:48 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties Thu Mar 10 19:17:52 2011 -0500
> @@ -135,6 +135,13 @@
> RExitTaken=Exit class already set and caller is not exit class.
> RCantReplaceSM=Changing the SecurityManager is not allowed.
> RCantCreateFile=Cant create file {0}
> +RCantDeleteFile=Cant delete file {0}
> +RRemoveRPermFailed=Removing read permission on file {0} failed
> +RRemoveWPermFailed=Removing write permissions on file {0} failed
> +RRemoveXPermFailed=Removing execute permissions on file {0} failed
> +RGetRPermFailed=Acquiring read permissions on file {0} failed
> +RGetWPermFailed=Acquiring write permissions on file {0} failed
> +RGetXPermFailed=Acquiring execute permissions on file {0} failed
> RCantCreateDir=Cant create directory {0}
> RCantRename=Cant rename {0} to {0}
> RDenyStopped=Stopped applications have no permissions.
> diff -r d91d02e798ff netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java Wed Mar 09 13:51:48 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java Thu Mar 10 19:17:52 2011 -0500
> @@ -277,10 +277,10 @@
>
> try {
> File errFile = new File(logDir, JNLPRuntime.STDERR_FILE);
> - errFile.getParentFile().mkdirs();
> + FileUtils.createParentDir(errFile, null);
> FileUtils.createRestrictedFile(errFile, true);
> File outFile = new File(logDir, JNLPRuntime.STDOUT_FILE);
> - outFile.getParentFile().mkdirs();
> + FileUtils.createParentDir(outFile, null);
> FileUtils.createRestrictedFile(outFile, true);
>
> if (redirectStreams) {
> diff -r d91d02e798ff netx/net/sourceforge/jnlp/services/XPersistenceService.java
> --- a/netx/net/sourceforge/jnlp/services/XPersistenceService.java Wed Mar 09 13:51:48 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/services/XPersistenceService.java Thu Mar 10 19:17:52 2011 -0500
> @@ -93,7 +93,8 @@
> checkLocation(location);
>
> File file = toCacheFile(location);
> - file.getParentFile().mkdirs();
> + FileUtils.createParentDir(file, "Persistence store for "
> + + location.toString());
>
> if (file.exists())
> throw new IOException("File already exists.");
> @@ -110,7 +111,7 @@
> public void delete(URL location) throws MalformedURLException, IOException {
> checkLocation(location);
>
> - toCacheFile(location).delete();
> + FileUtils.deleteWithErrMesg(toCacheFile(location), " tocache");
> }
>
> /**
> @@ -124,7 +125,8 @@
> if (!file.exists())
> throw new FileNotFoundException("Persistence store for "
> + location.toString() + " is not found.");
> - file.getParentFile().mkdirs();
> + FileUtils.createParentDir(file, "Persistence store for "
> + + location.toString());
>
> return (FileContents) ServiceUtil.createPrivilegedProxy(FileContents.class, new XFileContents(file));
> }
> diff -r d91d02e798ff netx/net/sourceforge/jnlp/util/FileUtils.java
> --- a/netx/net/sourceforge/jnlp/util/FileUtils.java Wed Mar 09 13:51:48 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/util/FileUtils.java Thu Mar 10 19:17:52 2011 -0500
> @@ -95,6 +95,20 @@
> createRestrictedFile(file, false, writableByOwner);
> }
>
> + public static void createParentDir(File f, String eMsg) throws IOException {
> + File parent = f.getParentFile();
> + if (!parent.exists()&& !parent.mkdirs()) {
> + throw new IOException(R("RCantCreateDir",
> + eMsg == null ? parent : eMsg));
> + }
> + }
> +
For ease of reading, may I recommend overloading this? If you create
another method createParentDir(File f) which delegates to this by
calling createParentDir(f, null), it might make other parts of the code
easier to read.
Also, please add JavaDocs.
> + public static void deleteWithErrMesg(File f, String eMsg) {
> + if (!f.delete()) {
> + System.err.println(R("RCantDeleteFile", eMsg == null ? f : eMsg));
> + }
> + }
> +
> /**
> * Creates a new file or directory with minimum permissions. The file is not
> * readable or writable by anyone other than the owner. If writeableByOnwer
> @@ -120,21 +134,29 @@
> }
>
> // remove all permissions
> - tempFile.setExecutable(false, false);
> - tempFile.setReadable(false, false);
> - tempFile.setWritable(false, false);
> + if (!tempFile.setExecutable(false, false)) {
> + throw new IOException(R("RRemoveXPermFailed", tempFile));
> + }
> + if (!tempFile.setReadable(false, false)) {
> + throw new IOException(R("RRemoveRPermFailed", tempFile));
> + }
> + if (!tempFile.setWritable(false, false)) {
> + throw new IOException(R("RRemoveWPermFailed", tempFile));
> + }
>
> // allow owner to read
> - tempFile.setReadable(true, true);
> + if (!tempFile.setReadable(true, true)) {
> + throw new IOException(R("RGetRPermFailed", tempFile));
> + }
>
> // allow owner to write
> - if (writableByOwner) {
> - tempFile.setWritable(true, true);
> + if (writableByOwner&& !tempFile.setWritable(true, true)) {
> + throw new IOException(R("RGetWPermFailed", tempFile));
> }
>
> // allow owner to enter directories
> - if (isDir) {
> - tempFile.setExecutable(true, true);
> + if (isDir&& !tempFile.setExecutable(true, true)) {
> + throw new IOException(R("RGetXPermFailed", tempFile));
> }
>
> // rename this file. Unless the file is moved/renamed, any program that
Everything else looks fine to me.
Cheers,
Omair
More information about the distro-pkg-dev
mailing list