Skip to content

Commit

Permalink
chimera: update FsSqlDriver#inodeOf to throw exception if file not found
Browse files Browse the repository at this point in the history 
Motivation:

Currently, `FsSqlDriver#inodeOf` returns `null` if the target directory
element does not exist.

Returning a `null` value is inherently dangerous operation, so this
operation is undesirable.

Moreover, in many places in the code, the return value is checked for
the `null` value and, if present, the code throws a corresponding
`FileNotFoundChimeraException`.  This is not DRY.

Modification:

Update `FsSqlDriver#inodeOf` so it now never returns a `null` value.
Instead, the method will throw FileNotFoundException if the target does
not exist.

Remove redundant code that checked for a `null` value and throws a
`FileNotFoundChimeraException`.

Update a few places where a special value (either `null` or an empty
collection) is returned so they continue to support returning this
value.

Result:

No user or admin observable changes.

The code base is (hopefully) simpler and easier to maintain.

Target: master
Requires-notes: no
Requires-book: no
Acked-by: Tigran Mkrtchyan
Patch: https://rb.dcache.org/r/14033/
  • Loading branch information
@paulmillar
paulmillar committed Aug 30, 2023
1 parent c425a62 commit 2fb9170
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 61 deletions.
36 changes: 20 additions & 16 deletions modules/chimera/src/main/java/org/dcache/chimera/FsSqlDriver.java
View file
Expand Up @@ -506,21 +506,24 @@ boolean rename(FsInode inode, FsInode srcDir, String source, FsInode destDir, St
*
* @param parent
* @param name
* @return null if path is not found
* @throws FileNotFoundChimeraFsException if name does not exist in parent path
* @return the inode for the named file in the parent directory.
*/
FsInode inodeOf(FsInode parent, String name, StatCacheOption stat) {
FsInode inodeOf(FsInode parent, String name, StatCacheOption stat)
throws FileNotFoundChimeraFsException {
switch (name) {
case ".":
return parent.isDirectory() ? parent : null;
case "..":
if (!parent.isDirectory()) {
return null;
throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, name);
}
FsInode dir = parent.getParent();
return (dir == null) ? parent : dir;
default:
FsInode child;
if (stat == STAT) {
return _jdbc.query(
child = _jdbc.query(
"SELECT c.* FROM t_dirs d JOIN t_inodes c ON d.ichild = c.inumber " +
"WHERE d.iparent = ? AND d.iname = ?",
ps -> {
Expand All @@ -530,14 +533,18 @@ FsInode inodeOf(FsInode parent, String name, StatCacheOption stat) {
rs -> rs.next() ? new FsInode(parent.getFs(), rs.getLong("inumber"),
FsInodeType.INODE, 0, toStat(rs)) : null);
} else {
return _jdbc.query("SELECT ichild FROM t_dirs WHERE iparent=? AND iname=?",
child = _jdbc.query("SELECT ichild FROM t_dirs WHERE iparent=? AND iname=?",
ps -> {
ps.setLong(1, parent.ino());
ps.setString(2, name);
},
rs -> rs.next() ? new FsInode(parent.getFs(), rs.getLong("ichild"))
: null);
}
if (child == null) {
throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, name);
}
return child;
}
}

Expand Down Expand Up @@ -1606,13 +1613,10 @@ FsInode path2inode(FsInode root, String path) throws ChimeraFsException {
*/
for (int i = pathElemts.size(); i > 0; i--) {
String f = pathElemts.get(i - 1);
inode = inodeOf(parentInode, f, STAT);

if (inode == null) {
/*
* element not found stop walking
*/
break;
try {
inode = inodeOf(parentInode, f, STAT);
} catch (FileNotFoundChimeraFsException e) {
return null;
}

/*
Expand All @@ -1639,7 +1643,7 @@ FsInode path2inode(FsInode root, String path) throws ChimeraFsException {
*
* @param root staring point
* @param path
* @return inode or null if path does not exist.
* @return inode or an empty collection if path does not exist.
*/
List<FsInode> path2inodes(FsInode root, String path) throws ChimeraFsException {
File pathFile = new File(path);
Expand All @@ -1664,9 +1668,9 @@ List<FsInode> path2inodes(FsInode root, String path) throws ChimeraFsException {
/* Path elements are in reverse order.
*/
for (String f : Lists.reverse(pathElements)) {
inode = inodeOf(parentInode, f, STAT);

if (inode == null) {
try {
inode = inodeOf(parentInode, f, STAT);
} catch (FileNotFoundChimeraFsException e) {
return Collections.emptyList();
}

Expand Down
67 changes: 22 additions & 45 deletions modules/chimera/src/main/java/org/dcache/chimera/JdbcFs.java
View file
Expand Up @@ -423,9 +423,6 @@ public FsInode createFile(FsInode parent, String name, int owner, int group, int
int level = Integer.parseInt(cmd[1]);
return inTransaction(status -> {
FsInode useInode = _sqlDriver.inodeOf(parent, cmd[2], STAT);
if (useInode == null) {
throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, cmd[2]);
}
try {
Stat stat = useInode.statCache();
return _sqlDriver.createLevel(useInode, stat.getUid(),
Expand Down Expand Up @@ -562,7 +559,7 @@ public void remove(String path) throws ChimeraFsException {
FsInode parent = path2inode(parentPath);
String name = filePath.getName();
FsInode inode = _sqlDriver.inodeOf(parent, name, STAT);
if (inode == null || !_sqlDriver.remove(parent, name, inode)) {
if (!_sqlDriver.remove(parent, name, inode)) {
throw FileNotFoundChimeraFsException.ofPath(path);
}
return null;
Expand Down Expand Up @@ -799,9 +796,6 @@ public FsInode inodeOf(FsInode parent, String name, StatCacheOption cacheOption)
throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, name);
}
FsInode inode = _sqlDriver.inodeOf(parent, cmd[1], NO_STAT);
if (inode == null) {
throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, cmd[1]);
}
return new FsInode_ID(this, inode.ino());
}

Expand All @@ -814,9 +808,6 @@ public FsInode inodeOf(FsInode parent, String name, StatCacheOption cacheOption)
int level = Integer.parseInt(cmd[1]);

FsInode inode = _sqlDriver.inodeOf(parent, cmd[2], NO_STAT);
if (inode == null) {
throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, cmd[2]);
}
if (level <= LEVELS_NUMBER) {
stat(inode, level);
return new FsInode(this, inode.ino(), level);
Expand Down Expand Up @@ -917,9 +908,6 @@ public FsInode inodeOf(FsInode parent, String name, StatCacheOption cacheOption)
throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, name);
}
FsInode inode = _sqlDriver.inodeOf(parent, cmd[1], NO_STAT);
if (inode == null) {
throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, cmd[1]);
}
return new FsInode_SURI(this, inode.ino());
}

Expand Down Expand Up @@ -960,9 +948,6 @@ public FsInode inodeOf(FsInode parent, String name, StatCacheOption cacheOption)
}

FsInode inode = _sqlDriver.inodeOf(parent, cmd[1], NO_STAT);
if (inode == null) {
throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, cmd[1]);
}
switch (cmd[2]) {
case "locality":
return new FsInode_PLOC(this, inode.ino());
Expand All @@ -985,11 +970,7 @@ public FsInode inodeOf(FsInode parent, String name, StatCacheOption cacheOption)
if (cmd.length != 2) {
throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, name);
}
FsInode inode = _sqlDriver.inodeOf(getWormID(), cmd[1], NO_STAT);
if (inode == null) {
throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, cmd[1]);
}
return inode;
return _sqlDriver.inodeOf(getWormID(), cmd[1], NO_STAT);
}

if (name.startsWith(".(fset)(")) {
Expand All @@ -999,10 +980,6 @@ public FsInode inodeOf(FsInode parent, String name, StatCacheOption cacheOption)
}

FsInode inode = _sqlDriver.inodeOf(parent, cmd[1], NO_STAT);
if (inode == null) {
throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, cmd[1]);
}

String[] args = new String[cmd.length - 2];
System.arraycopy(cmd, 2, args, 0, args.length);

Expand All @@ -1011,9 +988,6 @@ public FsInode inodeOf(FsInode parent, String name, StatCacheOption cacheOption)
}

FsInode inode = _sqlDriver.inodeOf(parent, name, cacheOption);
if (inode == null) {
throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, name);
}
fillIdCaches(inode);
inode.setParent(parent);
return inode;
Expand Down Expand Up @@ -1156,29 +1130,32 @@ public boolean rename(FsInode inode, FsInode srcDir, String source, FsInode dest
throw new NotDirChimeraException(destDir);
}

FsInode destInode = _sqlDriver.inodeOf(destDir, dest, STAT);

if (destInode != null) {
if (destInode.equals(inode)) {
// according to POSIX, we are done
return false;
FsInode destInode;
try {
destInode = _sqlDriver.inodeOf(destDir, dest, STAT);
} catch (FileNotFoundChimeraFsException e) {
if (!_sqlDriver.rename(inode, srcDir, source, destDir, dest)) {
throw FileNotFoundChimeraFsException.ofPath(source);
}
return true;
}

/* Renaming into existing is only allowed for the same type of entry.
*/
if (inode.isDirectory() != destInode.isDirectory()) {
throw new FileExistsChimeraFsException(dest);
}
if (destInode.equals(inode)) {
// according to POSIX, we are done
return false;
}

if (!_sqlDriver.remove(destDir, dest, destInode)) {
// Concurrent modification - retry
return rename(inode, srcDir, source, destDir, dest);
}
/* Renaming into existing is only allowed for the same type of entry.
*/
if (inode.isDirectory() != destInode.isDirectory()) {
throw new FileExistsChimeraFsException(dest);
}

if (!_sqlDriver.rename(inode, srcDir, source, destDir, dest)) {
throw FileNotFoundChimeraFsException.ofPath(source);
if (!_sqlDriver.remove(destDir, dest, destInode)) {
// Concurrent modification - retry
return rename(inode, srcDir, source, destDir, dest);
}

return true;
});
}
Expand Down

0 comments on commit 2fb9170

Please sign in to comment.