Skip to content

Commit f842a8c

Browse files
tjgqtjgq
authored
[8.5.0] Propagate errors correctly from WindowsFileSystem symlink methods. (#27323)
A bare IOException should not be used in place of FileNotFoundException and NotASymlinkException, which are sometimes used for control flow (to minimize the number of syscalls in a fast path). PiperOrigin-RevId: 733735243 Change-Id: I12b95a61da5ab49a4be15c570df70a5870614bb7
1 parent 98c77ea commit f842a8c

File tree

8 files changed

+40
-100
lines changed

8 files changed

+40
-100
lines changed

‎src/main/java/com/google/devtools/build/lib/windows/BUILD‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ java_11_library(
1919
deps = [
2020
":windows_path_operations",
2121
"//src/main/java/com/google/devtools/build/lib/jni",
22+
"//src/main/java/com/google/devtools/build/lib/vfs",
23+
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
2224
],
2325
)
2426

‎src/main/java/com/google/devtools/build/lib/windows/WindowsFileOperations.java‎

Lines changed: 9 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
package com.google.devtools.build.lib.windows;
1616

1717
import com.google.devtools.build.lib.jni.JniLoader;
18+
import com.google.devtools.build.lib.vfs.FileSystem.NotASymlinkException;
19+
import com.google.devtools.build.lib.vfs.PathFragment;
1820
import java.io.FileNotFoundException;
1921
import java.io.IOException;
2022
import java.nio.file.AccessDeniedException;
@@ -49,34 +51,6 @@ private WindowsFileOperations() {
4951
// Prevent construction
5052
}
5153

52-
/** Result of {@link #readSymlinkOrJunction}. */
53-
public static class ReadSymlinkOrJunctionResult {
54-
55-
/** Status code, indicating success or failure. */
56-
public enum Status {
57-
OK,
58-
NOT_A_LINK,
59-
ERROR
60-
}
61-
62-
private String result;
63-
private Status status;
64-
65-
public ReadSymlinkOrJunctionResult(Status s, String r) {
66-
this.status = s;
67-
this.result = r;
68-
}
69-
70-
/** Result string (junction target) or error message (depending on {@link status}). */
71-
public String getResult() {
72-
return result;
73-
}
74-
75-
public Status getStatus() {
76-
return status;
77-
}
78-
}
79-
8054
// Keep IS_SYMLINK_OR_JUNCTION_* values in sync with src/main/native/windows/file.cc.
8155
private static final int IS_SYMLINK_OR_JUNCTION_SUCCESS = 0;
8256
// IS_SYMLINK_OR_JUNCTION_ERROR = 1;
@@ -115,7 +89,6 @@ public Status getStatus() {
11589
private static final int READ_SYMLINK_OR_JUNCTION_ACCESS_DENIED = 2;
11690
private static final int READ_SYMLINK_OR_JUNCTION_DOES_NOT_EXIST = 3;
11791
private static final int READ_SYMLINK_OR_JUNCTION_NOT_A_LINK = 4;
118-
private static final int READ_SYMLINK_OR_JUNCTION_UNKNOWN_LINK_TYPE = 5;
11992

12093
private static native int nativeIsSymlinkOrJunction(
12194
String path, boolean[] result, String[] error);
@@ -140,8 +113,7 @@ public static boolean isSymlinkOrJunction(String path) throws IOException {
140113
case IS_SYMLINK_OR_JUNCTION_SUCCESS:
141114
return result[0];
142115
case IS_SYMLINK_OR_JUNCTION_DOES_NOT_EXIST:
143-
error[0] = "path does not exist";
144-
break;
116+
throw new FileNotFoundException(path);
145117
default:
146118
// This is IS_SYMLINK_OR_JUNCTION_ERROR (1). The JNI code puts a custom message in
147119
// 'error[0]'.
@@ -225,34 +197,23 @@ public static void createSymlink(String name, String target) throws IOException
225197
String.format("Cannot create symlink (name=%s, target=%s): %s", name, target, error[0]));
226198
}
227199

228-
public static ReadSymlinkOrJunctionResult readSymlinkOrJunction(String name) {
200+
public static String readSymlinkOrJunction(String name) throws IOException {
229201
String[] target = new String[] {null};
230202
String[] error = new String[] {null};
231203
switch (nativeReadSymlinkOrJunction(WindowsPathOperations.asLongPath(name), target, error)) {
232204
case READ_SYMLINK_OR_JUNCTION_SUCCESS:
233-
return new ReadSymlinkOrJunctionResult(
234-
ReadSymlinkOrJunctionResult.Status.OK,
235-
WindowsPathOperations.removeUncPrefixAndUseSlashes(target[0]));
205+
return WindowsPathOperations.removeUncPrefixAndUseSlashes(target[0]);
236206
case READ_SYMLINK_OR_JUNCTION_ACCESS_DENIED:
237-
error[0] = "access is denied";
238-
break;
207+
throw new AccessDeniedException(name);
239208
case READ_SYMLINK_OR_JUNCTION_DOES_NOT_EXIST:
240-
error[0] = "path does not exist";
241-
break;
209+
throw new FileNotFoundException(name);
242210
case READ_SYMLINK_OR_JUNCTION_NOT_A_LINK:
243-
return new ReadSymlinkOrJunctionResult(
244-
ReadSymlinkOrJunctionResult.Status.NOT_A_LINK, "path is not a link");
245-
case READ_SYMLINK_OR_JUNCTION_UNKNOWN_LINK_TYPE:
246-
error[0] = "unknown link type";
247-
break;
211+
throw new NotASymlinkException(PathFragment.create(name));
248212
default:
249213
// This is READ_SYMLINK_OR_JUNCTION_ERROR (1). The JNI code puts a custom message in
250214
// 'error[0]'.
251-
break;
215+
throw new IOException(String.format("Cannot read link (name=%s): %s", name, error[0]));
252216
}
253-
return new ReadSymlinkOrJunctionResult(
254-
ReadSymlinkOrJunctionResult.Status.ERROR,
255-
String.format("Cannot read link (name=%s): %s", name, error[0]));
256217
}
257218

258219
public static boolean deletePath(String path) throws IOException {

‎src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -103,15 +103,9 @@ protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFrag
103103
@Override
104104
protected PathFragment readSymbolicLink(PathFragment path) throws IOException {
105105
java.nio.file.Path nioPath = getNioPath(path);
106-
WindowsFileOperations.ReadSymlinkOrJunctionResult result =
107-
WindowsFileOperations.readSymlinkOrJunction(nioPath.toString());
108-
if (result.getStatus() == WindowsFileOperations.ReadSymlinkOrJunctionResult.Status.OK) {
109-
return PathFragment.create(StringEncoding.platformToInternal(result.getResult()));
110-
}
111-
if (result.getStatus() == WindowsFileOperations.ReadSymlinkOrJunctionResult.Status.NOT_A_LINK) {
112-
throw new NotASymlinkException(path);
113-
}
114-
throw new IOException(result.getResult());
106+
return PathFragment.create(
107+
StringEncoding.platformToInternal(
108+
WindowsFileOperations.readSymlinkOrJunction(nioPath.toString())));
115109
}
116110

117111
@Override

‎src/main/native/windows/file.cc‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -607,7 +607,10 @@ int ReadSymlinkOrJunction(const wstring& path, wstring* result,
607607
return ReadSymlinkOrJunctionResult::kNotALink;
608608
}
609609
default:
610-
return ReadSymlinkOrJunctionResult::kUnknownLinkType;
610+
*error =
611+
MakeErrorMessage(WSTR(__FILE__), __LINE__, L"ReadSymlinkOrJunction",
612+
path, L"unsupported link type");
613+
return ReadSymlinkOrJunctionResult::kError;
611614
}
612615
}
613616

‎src/main/native/windows/file.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,6 @@ struct ReadSymlinkOrJunctionResult {
133133
kAccessDenied = 2,
134134
kDoesNotExist = 3,
135135
kNotALink = 4,
136-
kUnknownLinkType = 5,
137136
};
138137
};
139138

‎src/main/tools/build-runfiles-windows.cc‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -113,9 +113,6 @@ bool ReadSymlink(const wstring& abs_path, wstring* target, wstring* error) {
113113
case bazel::windows::ReadSymlinkOrJunctionResult::kNotALink:
114114
*error = L"path is not a link";
115115
break;
116-
case bazel::windows::ReadSymlinkOrJunctionResult::kUnknownLinkType:
117-
*error = L"unknown link type";
118-
break;
119116
default:
120117
// This is bazel::windows::ReadSymlinkOrJunctionResult::kError (1).
121118
// The JNI code puts a custom message in 'error'.

‎src/test/java/com/google/devtools/build/lib/windows/WindowsFileOperationsTest.java‎

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,15 @@
1515
package com.google.devtools.build.lib.windows;
1616

1717
import static com.google.common.truth.Truth.assertThat;
18+
import static org.junit.Assert.assertThrows;
1819
import static org.junit.Assert.fail;
1920

2021
import com.google.common.collect.ImmutableMap;
2122
import com.google.devtools.build.lib.testutil.TestSpec;
2223
import com.google.devtools.build.lib.util.OS;
2324
import com.google.devtools.build.lib.windows.util.WindowsTestUtil;
2425
import java.io.File;
26+
import java.io.FileNotFoundException;
2527
import java.io.IOException;
2628
import java.nio.file.Files;
2729
import java.util.Arrays;
@@ -76,12 +78,9 @@ public void testSymlinkCreation() throws Exception {
7678
// Assert deleting the symlink does not remove the target file.
7779
assertThat(WindowsFileOperations.deletePath(symlinkFile.toString())).isTrue();
7880
assertThat(helloFile.exists()).isTrue();
79-
try {
80-
WindowsFileOperations.isSymlinkOrJunction(symlinkFile.toString());
81-
fail("Expected to throw: Symlink should no longer exist.");
82-
} catch (IOException e) {
83-
assertThat(e).hasMessageThat().contains("path does not exist");
84-
}
81+
assertThrows(
82+
FileNotFoundException.class,
83+
() -> WindowsFileOperations.isSymlinkOrJunction(symlinkFile.toString()));
8584
}
8685

8786
@Test
@@ -143,12 +142,9 @@ public void testIsJunction() throws Exception {
143142
assertThat(WindowsFileOperations.isSymlinkOrJunction(root + "\\longtargetpath\\file2.txt"))
144143
.isFalse();
145144
assertThat(WindowsFileOperations.isSymlinkOrJunction(root + "\\longta~1\\file2.txt")).isFalse();
146-
try {
147-
WindowsFileOperations.isSymlinkOrJunction(root + "\\non-existent");
148-
fail("expected to throw");
149-
} catch (IOException e) {
150-
assertThat(e.getMessage()).contains("path does not exist");
151-
}
145+
assertThrows(
146+
FileNotFoundException.class,
147+
() -> WindowsFileOperations.isSymlinkOrJunction(root + "\\non-existent"));
152148
assertThat(Arrays.asList(new File(root + "/shrtpath/a").list())).containsExactly("file1.txt");
153149
assertThat(Arrays.asList(new File(root + "/shrtpath/b").list())).containsExactly("file2.txt");
154150
assertThat(Arrays.asList(new File(root + "/shrtpath/c").list())).containsExactly("file2.txt");

‎src/test/java/com/google/devtools/build/lib/windows/WindowsFileSystemTest.java‎

Lines changed: 14 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
package com.google.devtools.build.lib.windows;
1616

1717
import static com.google.common.truth.Truth.assertThat;
18-
import static org.junit.Assert.fail;
18+
import static org.junit.Assert.assertThrows;
1919

2020
import com.google.common.base.Function;
2121
import com.google.common.collect.ImmutableList;
@@ -24,12 +24,13 @@
2424
import com.google.devtools.build.lib.testutil.TestSpec;
2525
import com.google.devtools.build.lib.util.OS;
2626
import com.google.devtools.build.lib.vfs.DigestHashFunction;
27+
import com.google.devtools.build.lib.vfs.FileSystem.NotASymlinkException;
2728
import com.google.devtools.build.lib.vfs.Path;
2829
import com.google.devtools.build.lib.vfs.PathFragment;
2930
import com.google.devtools.build.lib.vfs.Symlinks;
3031
import com.google.devtools.build.lib.windows.util.WindowsTestUtil;
3132
import java.io.File;
32-
import java.io.IOException;
33+
import java.io.FileNotFoundException;
3334
import java.nio.file.Files;
3435
import java.util.Arrays;
3536
import java.util.HashMap;
@@ -166,12 +167,10 @@ public void testIsJunction() throws Exception {
166167
.isFalse();
167168
assertThat(WindowsFileSystem.isSymlinkOrJunction(new File(root, "longta~1/file2.txt")))
168169
.isFalse();
169-
try {
170-
WindowsFileSystem.isSymlinkOrJunction(new File(root, "non-existent"));
171-
fail("expected failure");
172-
} catch (IOException e) {
173-
assertThat(e.getMessage()).contains("path does not exist");
174-
}
170+
171+
assertThrows(
172+
FileNotFoundException.class,
173+
() -> WindowsFileSystem.isSymlinkOrJunction(new File(root, "non-existent")));
175174

176175
assertThat(Arrays.asList(new File(root + "/shrtpath/a").list())).containsExactly("file1.txt");
177176
assertThat(Arrays.asList(new File(root + "/shrtpath/b").list())).containsExactly("file2.txt");
@@ -364,26 +363,15 @@ public void testReadJunction() throws Exception {
364363
assertThat(dirPath.isSymbolicLink()).isFalse();
365364
assertThat(juncPath.isSymbolicLink()).isTrue();
366365

367-
try {
368-
testUtil.createVfsPath(fs, "does-not-exist").readSymbolicLink();
369-
fail("expected exception");
370-
} catch (IOException expected) {
371-
assertThat(expected).hasMessageThat().matches(".*path does not exist");
372-
}
366+
assertThrows(
367+
FileNotFoundException.class,
368+
() -> testUtil.createVfsPath(fs, "does-not-exist").readSymbolicLink());
373369

374-
try {
375-
testUtil.createVfsPath(fs, "dir\\hello.txt").readSymbolicLink();
376-
fail("expected exception");
377-
} catch (IOException expected) {
378-
assertThat(expected).hasMessageThat().matches(".*is not a symlink");
379-
}
370+
assertThrows(
371+
NotASymlinkException.class,
372+
() -> testUtil.createVfsPath(fs, "dir\\hello.txt").readSymbolicLink());
380373

381-
try {
382-
dirPath.readSymbolicLink();
383-
fail("expected exception");
384-
} catch (IOException expected) {
385-
assertThat(expected).hasMessageThat().matches(".*is not a symlink");
386-
}
374+
assertThrows(NotASymlinkException.class, () -> dirPath.readSymbolicLink());
387375

388376
assertThat(juncPath.readSymbolicLink()).isEqualTo(dirPath.asFragment());
389377
}

0 commit comments

Comments
 (0)