CA-387770 Improve error message for readonly shares

From: Robin Newton <robin.newton@cloud.com>

This deals with the situation where an NFSSR or SMBSR is being attached,
but that share it uses cannot be written to.
* Make an explicit check that the share can be written to after it is
  mounted, raising a suitable error message if it's not. The check comes
  before the hard-link check, as this also writes to the share, and so
  would fail in this situation.
* If the attach fails (for this, or any other, reason) after the share
  has been mounted, ensure that it's unmounted. This is because if it is
  left mounted then a subsequent attach may succeed, by virtue of the
  checks being skipped.

Signed-off-by: Robin Newton <robin.newton@cloud.com>
---
 drivers/FileSR.py            |   14 ++++++++++++++
 drivers/NFSSR.py             |   14 ++++++++++----
 drivers/SMBSR.py             |   10 +++++++++-
 drivers/XE_SR_ERRORCODES.xml |    6 ++++++
 tests/test_FileSR.py         |    1 +
 tests/test_NFSSR.py          |    4 +++-
 tests/test_SMBSR.py          |   10 +++++++---
 7 files changed, 50 insertions(+), 9 deletions(-)

diff --git a/drivers/FileSR.py b/drivers/FileSR.py
index 5da02af..068c896 100755
--- a/drivers/FileSR.py
+++ b/drivers/FileSR.py
@@ -1104,6 +1104,20 @@ class SharedFileSR(FileSR):
     """
     NO_HARDLINK_SUPPORT = "no_hardlinks"
 
+    def _check_writable(self):
+        """
+        Checks that the filesystem being used by the SR can be written to,
+        raising an exception if it can't.
+        """
+        test_name = os.path.join(self.path, str(uuid4()))
+        try:
+            open(test_name, 'ab').close()
+        except OSError as e:
+            util.SMlog("Cannot write to SR file system: %s" % e)
+            raise xs_errors.XenError('SharedFileSystemNoWrite')
+        finally:
+            util.force_unlink(test_name)
+
     def _raise_hardlink_error(self):
         raise OSError(542, "Unknown error 524")
 
diff --git a/drivers/NFSSR.py b/drivers/NFSSR.py
index 8e8595e..6a2e532 100755
--- a/drivers/NFSSR.py
+++ b/drivers/NFSSR.py
@@ -133,10 +133,16 @@ class NFSSR(FileSR.SharedFileSR):
 
     def attach(self, sr_uuid):
         if not self._checkmount():
-            self.validate_remotepath(False)
-            util._testHost(self.dconf['server'], NFSPORT, 'NFSTarget')
-            self.mount_remotepath(sr_uuid)
-            self._check_hardlinks()
+            try:
+                self.validate_remotepath(False)
+                util._testHost(self.dconf['server'], NFSPORT, 'NFSTarget')
+                self.mount_remotepath(sr_uuid)
+                self._check_writable()
+                self._check_hardlinks()
+            except:
+                if self._checkmount():
+                    nfs.unmount(self.path, True)
+                raise
         self.attached = True
 
     def mount_remotepath(self, sr_uuid):
diff --git a/drivers/SMBSR.py b/drivers/SMBSR.py
index 496fabc..141a009 100755
--- a/drivers/SMBSR.py
+++ b/drivers/SMBSR.py
@@ -191,9 +191,17 @@ class SMBSR(FileSR.SharedFileSR):
             try:
                 self.mount()
                 os.symlink(self.linkpath, self.path)
+                self._check_writable()
+                self._check_hardlinks()
             except SMBException, exc:
                 raise xs_errors.XenError('SMBMount', opterr=exc.errstr)
-            self._check_hardlinks()
+            except:
+                if util.pathexists(self.path):
+                    os.unlink(self.path)
+                if self.checkmount():
+                    self.unmount(self.mountpoint, True)
+                raise
+
         self.attached = True
 
     def probe(self):
diff --git a/drivers/XE_SR_ERRORCODES.xml b/drivers/XE_SR_ERRORCODES.xml
index de074ce..7ab7bc1 100755
--- a/drivers/XE_SR_ERRORCODES.xml
+++ b/drivers/XE_SR_ERRORCODES.xml
@@ -888,6 +888,12 @@
             <value>460</value>
         </code>
 
+        <code>
+            <name>SharedFileSystemNoWrite</name>
+            <description>The file system for SR cannot be written to.</description>
+            <value>461</value>
+        </code>
+
         <code>
             <name>GenericException</name>
             <description>SM has thrown a generic python exception</description>
diff --git a/tests/test_FileSR.py b/tests/test_FileSR.py
index dd70c8c..f34cafa 100644
--- a/tests/test_FileSR.py
+++ b/tests/test_FileSR.py
@@ -119,6 +119,7 @@ class FakeSharedFileSR(FileSR.SharedFileSR):
         self.path = os.path.join(SR.MOUNT_BASE, sr_uuid)
 
     def attach(self, sr_uuid):
+        self._check_writable()
         self._check_hardlinks()
 
 
diff --git a/tests/test_NFSSR.py b/tests/test_NFSSR.py
index c683634..e5c7458 100644
--- a/tests/test_NFSSR.py
+++ b/tests/test_NFSSR.py
@@ -106,6 +106,7 @@ class TestNFSSR(unittest.TestCase):
         with self.assertRaises(SR.SROSError):
             nfssr.create(sr_uuid, size)
 
+    @mock.patch('FileSR.SharedFileSR._check_writable', autospec=True)
     @mock.patch('FileSR.SharedFileSR._check_hardlinks', autospec=True)
     @mock.patch('util.makedirs', autospec=True)
     @mock.patch('NFSSR.Lock', autospec=True)
@@ -114,7 +115,8 @@ class TestNFSSR(unittest.TestCase):
     @mock.patch('nfs.check_server_tcp', autospec=True)
     @mock.patch('nfs.validate_nfsversion', autospec=True)
     def test_attach(self, validate_nfsversion, check_server_tcp, _testhost,
-                    soft_mount, Lock, makedirs, mock_checklinks):
+                    soft_mount, Lock, makedirs, mock_checklinks,
+                    mock_checkwritable):
         validate_nfsversion.return_value = "aNfsversionChanged"
         nfssr = self.create_nfssr(server='aServer', serverpath='/aServerpath',
                                   sr_uuid='UUID', useroptions='options')
diff --git a/tests/test_SMBSR.py b/tests/test_SMBSR.py
index 2513d23..abd4976 100644
--- a/tests/test_SMBSR.py
+++ b/tests/test_SMBSR.py
@@ -71,6 +71,7 @@ class Test_SMBSR(unittest.TestCase):
         smbsr.attach('asr_uuid')
         self.assertTrue(smbsr.attached)
 
+    @mock.patch('FileSR.SharedFileSR._check_writable', autospec=True)
     @mock.patch('FileSR.SharedFileSR._check_hardlinks', autospec=True)
     @mock.patch('SMBSR.SMBSR.checkmount', autospec=True)
     @mock.patch('SMBSR.SMBSR.makeMountPoint', autospec=True)
@@ -79,7 +80,8 @@ class Test_SMBSR(unittest.TestCase):
     @mock.patch('os.symlink', autospec=True)
     @mock.patch('util.listdir', autospec=True)
     def test_attach_vanilla(self, listdir, symlink, pread, mock_lock,
-                            makeMountPoint, mock_checkmount, mock_checklinks):
+                            makeMountPoint, mock_checkmount, mock_checklinks,
+                            mock_checkwritable):
         mock_checkmount.return_value = False
         smbsr = self.create_smbsr()
         makeMountPoint.return_value = "/var/mount"
@@ -88,6 +90,7 @@ class Test_SMBSR(unittest.TestCase):
         pread.assert_called_with(['mount.cifs', '\\aServer', "/var/mount", '-o', 'cache=loose,vers=3.0,actimeo=0'],
                                  new_env={'PASSWD': 'aPassword', 'USER': 'aUsername'})
 
+    @mock.patch('FileSR.SharedFileSR._check_writable', autospec=True)
     @mock.patch('FileSR.SharedFileSR._check_hardlinks', autospec=True)
     @mock.patch('SMBSR.SMBSR.checkmount', autospec=True)
     @mock.patch('SMBSR.SMBSR.makeMountPoint', autospec=True)
@@ -97,7 +100,7 @@ class Test_SMBSR(unittest.TestCase):
     @mock.patch('util.listdir', autospec=True)
     def test_attach_with_cifs_password(
             self, listdir, symlink, pread, mock_lock, makeMountPoint,
-            mock_checkmount, mock_checklinks):
+            mock_checkmount, mock_checklinks, mock_checkwritable):
         smbsr = self.create_smbsr(dconf_update={"password": "winter2019"})
         mock_checkmount.return_value = False
         makeMountPoint.return_value = "/var/mount"
@@ -105,6 +108,7 @@ class Test_SMBSR(unittest.TestCase):
         self.assertTrue(smbsr.attached)
         pread.assert_called_with(['mount.cifs', '\\aServer', "/var/mount", '-o', 'cache=loose,vers=3.0,actimeo=0'], new_env={'PASSWD': 'winter2019', 'USER': 'aUsername'})
 
+    @mock.patch('FileSR.SharedFileSR._check_writable', autospec=True)
     @mock.patch('FileSR.SharedFileSR._check_hardlinks', autospec=True)
     @mock.patch('SMBSR.SMBSR.checkmount', autospec=True)
     @mock.patch('SMBSR.SMBSR.makeMountPoint', autospec=True)
@@ -114,7 +118,7 @@ class Test_SMBSR(unittest.TestCase):
     @mock.patch('util.listdir', autospec=True)
     def test_attach_with_cifs_password_and_domain(
             self, listdir, symlink, pread, mock_lock, makeMountPoint,
-            mock_checkmount, mock_checklinks):
+            mock_checkmount, mock_checklinks, mock_checkwritable):
         smbsr = self.create_smbsr(username="citrix\jsmith", dconf_update={"password": "winter2019"})
         mock_checkmount.return_value = False
         makeMountPoint.return_value = "/var/mount"
