Merge ~pushkarnk/ubuntu/+source/libmina-sshd-java:fix-sftp-ioexception into ubuntu/+source/libmina-sshd-java:ubuntu/devel

Proposed by Pushkar Kulkarni
Status: Merged
Merged at revision: 1bfd79bdabb8586245626109781bde638f369a06
Proposed branch: ~pushkarnk/ubuntu/+source/libmina-sshd-java:fix-sftp-ioexception
Merge into: ubuntu/+source/libmina-sshd-java:ubuntu/devel
Diff against target: 124 lines (+90/-1)
4 files modified
debian/changelog (+7/-0)
debian/control (+2/-1)
debian/patches/fix_sftp_io_exception.patch (+80/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Vladimir Petko (community) Approve
Simon Chopin (community) Needs Information
git-ubuntu import Pending
Review via email: mp+468349@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Pushkar Kulkarni (pushkarnk) wrote :
Revision history for this message
Simon Chopin (schopin) wrote :

Hi Pushkar!

Thanks for looking into this.

Rather than executing the test for all paths, couldn't we just use the first valid one?

review: Needs Information
Revision history for this message
Pushkar Kulkarni (pushkarnk) wrote :

Hi Simon

Thanks for reviewing this in detail.

The java.nio.files.* standard library API lets us access file store information in two ways:

1. Get the file store for a given file path - Files.getFileStore(Path) [3]
2. Get all file stores for a given file system - ileSystem.getFileStores() [4]

The test uses [1] to find the file store for a file. This seems to fail in some chroots like the one used by LP builders. It appears that the bind-mounted /proc on chroots on these builder VMs has no device information for /.

So, I am proposing we rely on [2] instead. We can get the file system of the desired file, and then
enumerate of all its file stores (this method uses /etc/mtab). One of them will be what [1] would have returned!

Now turning to your suggestion: I think we can't stop this enumeration when we get the valid file store, because there is no other way to map a file (path) to a file store other than [1] and hence we will never know which of them corresponds to our file.

Of course, we are either depending on /etc/mtab or on /proc/mounts. If there is a possibility that both are empty, we should disable this test.

Hope that answers your question.

[3] https://docs.oracle.com/en%2Fjava%2Fjavase%2F21%2Fdocs%2Fapi%2F%2F/java.base/java/nio/file/Files.html#getFileStore(java.nio.file.Path)
[4] https://docs.oracle.com/en%2Fjava%2Fjavase%2F21%2Fdocs%2Fapi%2F%2F/java.base/java/nio/file/FileSystem.html#getFileStores()

Revision history for this message
Vladimir Petko (vpa1977) wrote :

This is an existing upstream issue[1] and the only way to avoid it is to skip code branches calling Files.getFileStore(Path).

[1] https://bugs.openjdk.org/browse/JDK-8166162

Revision history for this message
Vladimir Petko (vpa1977) :
review: Approve
Revision history for this message
Vladimir Petko (vpa1977) wrote :

nit

Revision history for this message
Vladimir Petko (vpa1977) wrote :

Uploaded, thank you!!!
Please forward the patch to Debian.
It might be also applicable to the upstream if the build container does not have /proc mounted.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 9487555..4c0ed54 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,10 @@
6+libmina-sshd-java (2.12.1-2ubuntu1) oracular; urgency=medium
7+
8+ * d/patches: Add patch to work-around SFTP test failure
9+ (LP: #2071358)
10+
11+ -- Pushkar Kulkarni <pushkar.kulkarni@canonical.com> Thu, 27 Jun 2024 18:14:54 +0530
12+
13 libmina-sshd-java (2.12.1-2) unstable; urgency=medium
14
15 * Source-only upload to unstable
16diff --git a/debian/control b/debian/control
17index c9de1e0..5e4fafd 100644
18--- a/debian/control
19+++ b/debian/control
20@@ -1,7 +1,8 @@
21 Source: libmina-sshd-java
22 Section: java
23 Priority: optional
24-Maintainer: Debian Java Maintainers <pkg-java-maintainers@lists.alioth.debian.org>
25+Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com>
26+XSBC-Original-Maintainer: Debian Java Maintainers <pkg-java-maintainers@lists.alioth.debian.org>
27 Uploaders: Thomas Koch <thomas@koch.ro>,
28 Pierre Gruet <pgt@debian.org>
29 Build-Depends: debhelper-compat (= 13),
30diff --git a/debian/patches/fix_sftp_io_exception.patch b/debian/patches/fix_sftp_io_exception.patch
31new file mode 100644
32index 0000000..d50d12b
33--- /dev/null
34+++ b/debian/patches/fix_sftp_io_exception.patch
35@@ -0,0 +1,80 @@
36+Description: Modify test SpaceAvailableExtensionImplTest.testFileStoreReport()
37+ that fails only in some chroots. The changed code will run more tests including
38+ the existing test. This is irrelevant to the upstream repository because it
39+ is a work-around for a specific build environment and is better than skipping
40+ the test altogether.
41+Forwarded: not-needed
42+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/cura-engine/+bug/2071358
43+Author: Pushkar Kulkarni <pushkar.kulkarni@canonical.com>
44+--- a/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/extensions/helpers/SpaceAvailableExtensionImplTest.java
45++++ b/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/extensions/helpers/SpaceAvailableExtensionImplTest.java
46+@@ -23,6 +23,7 @@
47+ import java.io.StreamCorruptedException;
48+ import java.nio.file.FileStore;
49+ import java.nio.file.Files;
50++import java.nio.file.FileSystem;
51+ import java.nio.file.Path;
52+ import java.util.Collections;
53+ import java.util.List;
54+@@ -63,34 +64,35 @@
55+ Path lclSftp = CommonTestSupportUtils.resolve(targetPath, SftpConstants.SFTP_SUBSYSTEM_NAME, getClass().getSimpleName(),
56+ getCurrentTestName());
57+ Path parentPath = targetPath.getParent();
58+- FileStore store = Files.getFileStore(lclSftp.getRoot());
59+- final String queryPath = CommonTestSupportUtils.resolveRelativeRemotePath(parentPath, lclSftp);
60+- final SpaceAvailableExtensionInfo expected = new SpaceAvailableExtensionInfo(store);
61+-
62+- List<? extends SubsystemFactory> factories = sshd.getSubsystemFactories();
63+- sshd.setSubsystemFactories(Collections.singletonList(new SftpSubsystemFactory() {
64+- @Override
65+- public Command createSubsystem(ChannelSession channel) throws IOException {
66+- return new SftpSubsystem(channel, this) {
67+- @Override
68+- protected SpaceAvailableExtensionInfo doSpaceAvailable(int id, String path) throws IOException {
69+- if (!queryPath.equals(path)) {
70+- throw new StreamCorruptedException(
71+- "Mismatched query paths: expected=" + queryPath + ", actual=" + path);
72+- }
73++ for(FileStore store: parentPath.getFileSystem().getFileStores()) {
74++ final String queryPath = CommonTestSupportUtils.resolveRelativeRemotePath(parentPath, lclSftp);
75++ final SpaceAvailableExtensionInfo expected = new SpaceAvailableExtensionInfo(store);
76++
77++ List<? extends SubsystemFactory> factories = sshd.getSubsystemFactories();
78++ sshd.setSubsystemFactories(Collections.singletonList(new SftpSubsystemFactory() {
79++ @Override
80++ public Command createSubsystem(ChannelSession channel) throws IOException {
81++ return new SftpSubsystem(channel, this) {
82++ @Override
83++ protected SpaceAvailableExtensionInfo doSpaceAvailable(int id, String path) throws IOException {
84++ if (!queryPath.equals(path)) {
85++ throw new StreamCorruptedException(
86++ "Mismatched query paths: expected=" + queryPath + ", actual=" + path);
87++ }
88+
89+- return expected;
90+- }
91+- };
92++ return expected;
93++ }
94++ };
95++ }
96++ }));
97++
98++ try (SftpClient sftp = createSingleSessionClient()) {
99++ SpaceAvailableExtension ext = assertExtensionCreated(sftp, SpaceAvailableExtension.class);
100++ SpaceAvailableExtensionInfo actual = ext.available(queryPath);
101++ assertEquals("Mismatched information", expected, actual);
102++ } finally {
103++ sshd.setSubsystemFactories(factories);
104+ }
105+- }));
106+-
107+- try (SftpClient sftp = createSingleSessionClient()) {
108+- SpaceAvailableExtension ext = assertExtensionCreated(sftp, SpaceAvailableExtension.class);
109+- SpaceAvailableExtensionInfo actual = ext.available(queryPath);
110+- assertEquals("Mismatched information", expected, actual);
111+- } finally {
112+- sshd.setSubsystemFactories(factories);
113+ }
114+ }
115+ }
116diff --git a/debian/patches/series b/debian/patches/series
117index 0f20cc9..f0ccfdb 100644
118--- a/debian/patches/series
119+++ b/debian/patches/series
120@@ -5,3 +5,4 @@ no_testcontainers.patch
121 overriding_IoHandler.patch
122 skip_tests_requiring_networks.patch
123 missing_test_classes.patch
124+fix_sftp_io_exception.patch

Subscribers

People subscribed via source and target branches