Merge lp:~mandel/ubuntuone-client/fix-os-helper into lp:ubuntuone-client

[Manuel de la Peña]

Ensure that the os_helper layer handles encodings correctly and that does not leak platform details to the rest of the client (that includes not using literal paths).

Ensure that the os_helper layer handles encodings correctly and that does not leak platform details to the rest of the client (that includes not using literal paths).

[Natalia Bidart (nataliabidart)]

* remove_utf8_chars method name is misleading. As per what we talked in IRC, this method needs to be renamed to restore_illegal_chars and remove_windows_illegal_chars will be renamed to remove_illegal_chars.

* in windows/.../filesystem_manager.py there are several inconsistencies, some methods use the windowspath decorator, and some others don't: for example, rm_watch and add_watch; freeze_begin vs freeze_commit and freeze_rollback, etc.
In IRC we agreed:
(09:26:12 AM) nessita: mandel: ok, so I think this class needs a careful review of decorators, I'll add that to the MP, yes?
(09:26:26 AM) mandel: nessita: completely agree

* Remove _get_syncdaemon_valid_path as per:
nessita: mandel: so, I'm starting again with the next question to avoid confusions: in os_helper.py: _get_syncdaemon_valid_path, I would expect this class, that assert over the param to be a unicode, to return bytes
nessita: mandel: ex-remove_utf8_chars only replaces unicode chars, but does not transform to bytes
nessita: mandel: that method (_get_syncdaemon_valid_path) should do the .encode('utf8')
nessita: mandel: the thing is the name of the method. A method with that name, *should* return bytes since is meant to be used by syncdaemon
mandel: nessita: I'd remove that method and use reinstantiate_illegla_chars instead
nessita: mandel: restore_illegal_chars? :-)

* Rename _validate_windows_path to _assert_path_is_bytes, and then you can use that in the implementation of _valiate_output_path, reaplacing the following code:

assert isinstance(path, str), 'All paths returned to sd should be bytes'

review: Needs Fixing

[lp:~mandel/ubuntuone-client/fix-os-helper]

1067. By Manuel de la Peña on 2011-07-20

Added decorators to ensure that the correct encoding is used.

[Natalia Bidart (nataliabidart)]

* If you plan to use _get_windows_valid_path from other modules, do not name it with an initial underscore. Things named _something are not meant to be used outside the module that defines them.

* If I understand this test correctly, it makes no sense:

    def test_restore_illegal_chars(self):
        """Test that the path does not have utf chars."""
        path = self.testfile + ''.join(LINUX_ILLEGAL_CHARS_MAP)
        path = restore_illegal_chars(path)
        self.assertFalse(any(c in path for c in LINUX_ILLEGAL_CHARS_MAP))

it should be something like (please note the docstring was corrected):

    def test_restore_illegal_chars(self):
        """The illegal unicode chars are replaced by characters, valid in linux."""
        original_path = self.testfile + ''.join(LINUX_ILLEGAL_CHARS_MAP)
        path = remove_illegal_chars(original_path)
        assert not any(c in path for c in LINUX_ILLEGAL_CHARS_MAP)

        path = restore_illegal_chars(path)
        self.assertEqual(original_path, path)

and you need to add an analogous test for remove_illegal_chars:

    def test_remove_illegal_chars(self):
        """The illegal chars are replaced by unicode objects, valid on windows."""
        original_path = self.testfile + ''.join(WINDOWS_ILLEGAL_CHARS_MAP)
        path = remove_illegal_chars(original_path)
        assert not any(c in path for c in WINDOWS_ILLEGAL_CHARS_MAP)

        path = restore_illegal_chars(path)
        self.assertEqual(original_path, path)

* as per out chat this morning, get_syncdaemon_valid_path was supposed to disappear. Why is still being used in filesystem_notifications.py?

275 + 'path': head.replace(get_syncdaemon_valid_path(self.path),
297 + full_filename = get_syncdaemon_valid_path(file_name)

* you added this comment:

284 + # assert that all the paths are byte and are not literals

but nothing is actually asserting anything.

* are you sure we need the decorator "@windowspath(paths_indexes=[1])" for filesystem_notifications.py:get_wd?

* typos in:

# literal path, that is not \\?\ prefix in the pat. This is ubber important,
# in case of a doubt ask mandel or nessita

"pat", "ubber"

* please replace:

568 + assert isinstance(path, str), 'All paths returned to sd should be bytes'

by a call to _assert_path_is_bytes, since you already have that function defined. Also, I think all the _assert_something methods should not return anything, since they do not perfom any modification. So, this code:

578 +def _valiate_output_collection(col):
579 +"""Ensure that the collection returned from os helper is valid."""
580 + for path in col:
581 + _assert_path_is_not_literal(_assert_output_path_is_bytes(path))
582 + return col

will mutate to:

578 +def _valiate_output_collection(col):
579 +"""Ensure that the collection returned from os helper is valid."""
580 + for path in col:
                 _assert_output_path_is_bytes(path)
581 + _assert_path_is_not_literal(path)

review: Needs Fixing

[Manuel de la Peña (mandel)]

> * If you plan to use _get_windows_valid_path from other modules, do not name
> it with an initial underscore. Things named _something are not meant to be
> used outside the module that defines them.
>

I was hoping to be allowed to use _ so that it was explicit that you should not be using it unless you really really know what you are doing, and I seriously hope I know what I'm doing in this code ;)

I nevertheless can remove it.

> * If I understand this test correctly, it makes no sense:
>
> def test_restore_illegal_chars(self):
> """Test that the path does not have utf chars."""
> path = self.testfile + ''.join(LINUX_ILLEGAL_CHARS_MAP)
> path = restore_illegal_chars(path)
> self.assertFalse(any(c in path for c in LINUX_ILLEGAL_CHARS_MAP))

What is wrong with the test? it is making sure that the method gets a path with all the utf8 chars in it and returns a path with only illegal chars. The assert ensures that once there are no utf8 chars in the returned path.

>
> it should be something like (please note the docstring was corrected):
>
> def test_restore_illegal_chars(self):
> """The illegal unicode chars are replaced by characters, valid in
> linux."""
> original_path = self.testfile + ''.join(LINUX_ILLEGAL_CHARS_MAP)
> path = remove_illegal_chars(original_path)
> assert not any(c in path for c in LINUX_ILLEGAL_CHARS_MAP)
>
> path = restore_illegal_chars(path)
> self.assertEqual(original_path, path)
>

I do see the confusion is possible because the name of the test is wrong, it should be test_remove_illegla_chars, looks like an error when doing the search/replace when renaming methods. On the other hand I do not like your test because it is testing more than one thing, here you are taking a path with illegal chars, asserting that they are not present and getting back the original. If this test fails we would need to look at two different methods, either remove_illegal_chars failed, and we got a AssertError, or the restore_illegal_chars failed and we got a fail in the assertEqual.

From my point of view tests should have a single concern, in this case would be to remove the illegal chars and we would later add a test to restore them. I do prefer your docstring.

> and you need to add an analogous test for remove_illegal_chars:
>
> def test_remove_illegal_chars(self):
> """The illegal chars are replaced by unicode objects, valid on
> windows."""
> original_path = self.testfile + ''.join(WINDOWS_ILLEGAL_CHARS_MAP)
> path = remove_illegal_chars(original_path)
> assert not any(c in path for c in WINDOWS_ILLEGAL_CHARS_MAP)
>
> path = restore_illegal_chars(path)
> self.assertEqual(original_path, path)
>

Same as the previous comment, I would add a test for restore_illegal_chars that simply asserts that no illegal chars are present.

> * as per out chat this morning, get_syncdaemon_valid_path was supposed to
> disappear. Why is still being used in filesystem_notifications.py?
>
> 275 + 'path': head.replace(get_syncdaemon_valid_path(self.path),
> 297 + full_filename = get_syncdaemon_valid...

[Manuel de la Peña (mandel)]

> > * If you plan to use _get_windows_valid_path from other modules, do not name
> > it with an initial underscore. Things named _something are not meant to be
> > used outside the module that defines them.
> >
>
> I was hoping to be allowed to use _ so that it was explicit that you should
> not be using it unless you really really know what you are doing, and I
> seriously hope I know what I'm doing in this code ;)
>
> I nevertheless can remove it.
>
> > * If I understand this test correctly, it makes no sense:
> >
> > def test_restore_illegal_chars(self):
> > """Test that the path does not have utf chars."""
> > path = self.testfile + ''.join(LINUX_ILLEGAL_CHARS_MAP)
> > path = restore_illegal_chars(path)
> > self.assertFalse(any(c in path for c in LINUX_ILLEGAL_CHARS_MAP))
>
> What is wrong with the test? it is making sure that the method gets a path
> with all the utf8 chars in it and returns a path with only illegal chars. The
> assert ensures that once there are no utf8 chars in the returned path.
>
> >
> > it should be something like (please note the docstring was corrected):
> >
> > def test_restore_illegal_chars(self):
> > """The illegal unicode chars are replaced by characters, valid in
> > linux."""
> > original_path = self.testfile + ''.join(LINUX_ILLEGAL_CHARS_MAP)
> > path = remove_illegal_chars(original_path)
> > assert not any(c in path for c in LINUX_ILLEGAL_CHARS_MAP)
> >
> > path = restore_illegal_chars(path)
> > self.assertEqual(original_path, path)
> >
>
> I do see the confusion is possible because the name of the test is wrong, it
> should be test_remove_illegla_chars, looks like an error when doing the
> search/replace when renaming methods. On the other hand I do not like your

I correct myself, the names is perfectly OK.

> test because it is testing more than one thing, here you are taking a path
> with illegal chars, asserting that they are not present and getting back the
> original. If this test fails we would need to look at two different methods,
> either remove_illegal_chars failed, and we got a AssertError, or the
> restore_illegal_chars failed and we got a fail in the assertEqual.
>
> From my point of view tests should have a single concern, in this case would
> be to remove the illegal chars and we would later add a test to restore them.
> I do prefer your docstring.
>
> > and you need to add an analogous test for remove_illegal_chars:
> >
> > def test_remove_illegal_chars(self):
> > """The illegal chars are replaced by unicode objects, valid on
> > windows."""
> > original_path = self.testfile + ''.join(WINDOWS_ILLEGAL_CHARS_MAP)
> > path = remove_illegal_chars(original_path)
> > assert not any(c in path for c in WINDOWS_ILLEGAL_CHARS_MAP)
> >
> > path = restore_illegal_chars(path)
> > self.assertEqual(original_path, path)
> >
>
> Same as the previous comment, I would add a test for restore_illegal_chars
> that simply asserts that no illegal chars are present.
>
> > * as per out chat this morning, get_syncdaemon_valid_path was supposed to
> > disappear. Why is...

[lp:~mandel/ubuntuone-client/fix-os-helper]

1068. By Manuel de la Peña on 2011-07-21

Renamed _get_windows_valid_path to get_windows_valid_path.

1069. By Manuel de la Peña on 2011-07-21

Added test for remove_illegal_chars. Updated docstrings.

1070. By Manuel de la Peña on 2011-07-21

Improved the implementation of the decorators and assertions.

[Natalia Bidart (nataliabidart)]

> > * If you plan to use _get_windows_valid_path from other modules, do not name
> > it with an initial underscore. Things named _something are not meant to be
> > used outside the module that defines them.
>
> I was hoping to be allowed to use _ so that it was explicit that you should
> not be using it unless you really really know what you are doing, and I
> seriously hope I know what I'm doing in this code ;)

We are not allowed to use methods starting with _ outside a module, only exception is when writing tests about that module.
If you need people to be careful using a method, add a huge docstring to it stating why is dangerous and what precautions we need to have when using it.

> > * If I understand this test correctly, it makes no sense:
> >
> > def test_restore_illegal_chars(self):
> > """Test that the path does not have utf chars."""
> > path = self.testfile + ''.join(LINUX_ILLEGAL_CHARS_MAP)
> > path = restore_illegal_chars(path)
> > self.assertFalse(any(c in path for c in LINUX_ILLEGAL_CHARS_MAP))
>
> What is wrong with the test? it is making sure that the method gets a path
> with all the utf8 chars in it and returns a path with only illegal chars. The
> assert ensures that once there are no utf8 chars in the returned path.

What is wrong is that you're not testing the key part of restore_illegal_chars which is the following: not only some chars are removed, but also the proper replacements is being made, which is needed to be able to ensure that we can obtain back the original path. See below for a trivial implementation of restore_illegal_chars that will make this test pass and break all the code horribly.

> > it should be something like (please note the docstring was corrected):
> >
> > def test_restore_illegal_chars(self):
> > """The illegal unicode chars are replaced by characters, valid in
> > linux."""
> > original_path = self.testfile + ''.join(LINUX_ILLEGAL_CHARS_MAP)
> > path = remove_illegal_chars(original_path)
> > assert not any(c in path for c in LINUX_ILLEGAL_CHARS_MAP)
> >
> > path = restore_illegal_chars(path)
> > self.assertEqual(original_path, path)
> >
>
> I do see the confusion is possible because the name of the test is wrong, it
> should be test_remove_illegla_chars, looks like an error when doing the
> search/replace when renaming methods. On the other hand I do not like your
> test because it is testing more than one thing, here you are taking a path

Is not testing more that one thing. Please note that using the builtin "assert" is not testing something, but being explicit about an assumption that is being made while preparing the variables to perform the actual test, which is the call to assertEqual.

> original. If this test fails we would need to look at two different methods,

We will have either an Error or Failure, depending if the assert failed or the assertEqual, respectively.
If we get an Error, we know some of our assumptions were wrong, and we need to either reconsider our test, or see what thing we trusted that is not behaving as expected.

This is a common strategy when writing tests, you need to as...

[Manuel de la Peña (mandel)]

> > > * If you plan to use _get_windows_valid_path from other modules, do not
> name
> > > it with an initial underscore. Things named _something are not meant to be
> > > used outside the module that defines them.
> >
> > I was hoping to be allowed to use _ so that it was explicit that you should
> > not be using it unless you really really know what you are doing, and I
> > seriously hope I know what I'm doing in this code ;)
>
> We are not allowed to use methods starting with _ outside a module, only
> exception is when writing tests about that module.
> If you need people to be careful using a method, add a huge docstring to it
> stating why is dangerous and what precautions we need to have when using it.

No worries, in this new version the name has changed.

>
> > > * If I understand this test correctly, it makes no sense:
> > >
> > > def test_restore_illegal_chars(self):
> > > """Test that the path does not have utf chars."""
> > > path = self.testfile + ''.join(LINUX_ILLEGAL_CHARS_MAP)
> > > path = restore_illegal_chars(path)
> > > self.assertFalse(any(c in path for c in LINUX_ILLEGAL_CHARS_MAP))
> >
> > What is wrong with the test? it is making sure that the method gets a path
> > with all the utf8 chars in it and returns a path with only illegal chars.
> The
> > assert ensures that once there are no utf8 chars in the returned path.
>
> What is wrong is that you're not testing the key part of restore_illegal_chars
> which is the following: not only some chars are removed, but also the proper
> replacements is being made, which is needed to be able to ensure that we can
> obtain back the original path. See below for a trivial implementation of
> restore_illegal_chars that will make this test pass and break all the code
> horribly.
>
> > > it should be something like (please note the docstring was corrected):
> > >
> > > def test_restore_illegal_chars(self):
> > > """The illegal unicode chars are replaced by characters, valid in
> > > linux."""
> > > original_path = self.testfile + ''.join(LINUX_ILLEGAL_CHARS_MAP)
> > > path = remove_illegal_chars(original_path)
> > > assert not any(c in path for c in LINUX_ILLEGAL_CHARS_MAP)
> > >
> > > path = restore_illegal_chars(path)
> > > self.assertEqual(original_path, path)
> > >
> >
> > I do see the confusion is possible because the name of the test is wrong, it
> > should be test_remove_illegla_chars, looks like an error when doing the
> > search/replace when renaming methods. On the other hand I do not like your
> > test because it is testing more than one thing, here you are taking a path
>
> Is not testing more that one thing. Please note that using the builtin
> "assert" is not testing something, but being explicit about an assumption that
> is being made while preparing the variables to perform the actual test, which
> is the call to assertEqual.
>
> > original. If this test fails we would need to look at two different methods,
>
> We will have either an Error or Failure, depending if the assert failed or the
> assertEqual, respectively.
> If we get an Error, we know some of our assumptions wer...

[lp:~mandel/ubuntuone-client/fix-os-helper]

1071. By Manuel de la Peña on 2011-07-21

Merged with trunk.

1072. By Manuel de la Peña on 2011-07-21

Fixed issues created during the merge with trunk.

1073. By Manuel de la Peña on 2011-07-21

Added tests implementation as per requested in the reviews.

1075. By Manuel de la Peña on 2011-07-21

Fixed screwed up test.

[Roberto Alsina (ralsina)]

In line 84, I would use u'test' to be explicit original_path is unicode

In line 86, we are comparing a bytes string (path) against unicode strings (the keys in LINUX_ILLEGAL_CHARS_MAP).

In line 99, we are comparing a unicode string (path) against strings (the keys in WINDOWS_ILLEGAL_CHARS_MAP)

Those are not well-defined operations, AFAIK, and will tend to give no matches, which means the tests will tend to pass.

I would also add asserts to check that in both those tests path is the right type. Automatic promotion to unicode is a real risk.

review: Needs Fixing

[Alejandro J. Cura (alecu)]

There's no member self.path in class Watch, so the following line probably means self._path:

191 + 'path': head.replace(get_syncdaemon_valid_path(self.path),

--

Why did we separate assert_path_is_bytes and assert_path_is_not_literal? They seem to be called all the time together, and with the same arguments. Perhaps we should make "one" decorator that joins both.

--

The following bit makes no sense in get_windows_valid_path() and it may mask some problem.

    if path.startswith(LONG_PATH_PREFIX):
        path = path.replace(LONG_PATH_PREFIX, '')

perhaps it should be an assertion like "assert not path.startswith(LONG_PATH_PREFIX)

--

We should never check for: "LONG_PATH_PREFIX in path", we should use "path.startswith(LONG_PATH_PREFIX)" instead.
(for instance, it's used in _contains_chars, _modify_chars)

--

typos: litdir, unfortunatly

--

All the times we are using windowspath, we use assert_path_is_bytes above it.
we should only be using windowspath, and have it call assert_path_is_bytes
The same applies to the proliferation of decorator pairs that are present throughout the code.

--

I understand that vm_helper.get_udf_path should not be making any / to \ transformations, and we should be doing those in the os_helper layer.

review: Needs Fixing

Unmerged revisions

=== modified file 'tests/platform/windows/test_filesystem_notifications.py'
--- tests/platform/windows/test_filesystem_notifications.py	2011-06-23 22:17:13 +0000
+++ tests/platform/windows/test_filesystem_notifications.py	2011-07-21 16:16:14 +0000
@@ -23,8 +23,8 @@
 from mocker import MockerTestCase, ANY
 
 from contrib.testing.testcase import BaseTwistedTestCase
+from ubuntuone.platform.windows.os_helper import get_windows_valid_path
 from ubuntuone.platform.windows.pyinotify import ProcessEvent
-from ubuntuone.platform.windows.os_helper import LONG_PATH_PREFIX
 from ubuntuone.platform.windows.filesystem_notifications import (
     WatchManager,
     Notifier,
@@ -60,8 +60,7 @@
     def setUp(self):
         """Set infor for the tests."""
         super(TestWatch, self).setUp()
-        self.basedir = LONG_PATH_PREFIX + os.path.abspath(
-            self.mktemp('test_root'))
+        self.basedir = os.path.abspath(self.mktemp('test_root'))
         self.mask = FILE_NOTIFY_CHANGE_FILE_NAME | \
             FILE_NOTIFY_CHANGE_DIR_NAME | \
             FILE_NOTIFY_CHANGE_ATTRIBUTES | \
@@ -443,7 +442,7 @@
         exclude_filter = lambda x: False
         proc_fun = lambda x: None
         # expect the factory to be called with the passed values
-        self.watch_factory(wd, path, mask, auto_add,
+        self.watch_factory(wd, get_windows_valid_path(os.path.abspath(path)), mask, auto_add,
             events_queue=events_queue, exclude_filter=exclude_filter,
             proc_fun=proc_fun)
         self.mocker.result(self.watch)

=== modified file 'tests/platform/windows/test_os_helper.py'
--- tests/platform/windows/test_os_helper.py	2011-07-21 13:40:32 +0000
+++ tests/platform/windows/test_os_helper.py	2011-07-21 16:16:14 +0000
@@ -30,7 +30,6 @@
     _set_file_attributes,
     access,
     can_write,
-    is_illegal_path,
     listdir,
     make_dir,
     normpath,
@@ -47,6 +46,7 @@
     EVERYONE_GROUP,
     LONG_PATH_PREFIX,
     WINDOWS_ILLEGAL_CHARS_MAP,
+    LINUX_ILLEGAL_CHARS_MAP,
 )
 from contrib.testing.testcase import BaseTwistedTestCase
 
@@ -179,11 +179,15 @@
         """Return a list of the files in a dir."""
         open(os.path.join(self.basedir, 'foo'), 'w').close()
         open(os.path.join(self.basedir, 'bar'), 'w').close()
+        # create a path with illegal chars that is written in the fs with
+        # the legal ones
+        test_file = 'test_file' + ''.join(WINDOWS_ILLEGAL_CHARS_MAP)
+        full_test_file = os.path.join(self.basedir, test_file)
+        open(replace_illegal_chars_with_unicode(full_test_file), 'w').close()
         l = listdir(self.basedir)
-        test_file = u'test_file\u200b\u2033\u200b\u200b\u203d\u200b\u200b' +\
-                    u'\u26b9\u200b\u200b\u2223\u200b\u200b\u2236\u200b\u200b' +\
-                    u'\u2039\u200b\u200b\u2044\u200b\u200b\u203a\u200b'
         self.assertEqual(sorted(l), ['bar', 'foo', test_file])
+        for path in l:
+            self.assertIsInstance(path, type(self.basedir))
 
     def test_access_rw(self):
         """Test access on a file with full permission."""
@@ -234,9 +238,30 @@
         self.assertFalse(path_exists(os.path.join(self.basedir, 'subdir')))
 
     def test_replace_unicode_with_illegal_chars(self):
-        """Test that the path does not have utf chars."""
-        path = replace_unicode_with_illegal_chars(replace_illegal_chars_with_unicode(self.testfile))
-        self.assertFalse(is_illegal_path(path))
+        """Test the mangling of the paths.
+
+        We ensure that the illegal unicode chars are replaced by
+        characters valid in linux.
+        """
+        original_path = 'test'.join(LINUX_ILLEGAL_CHARS_MAP)
+        path = replace_unicode_with_illegal_chars(original_path)
+        assert not any(c in path for c in LINUX_ILLEGAL_CHARS_MAP)
+        
+        path = replace_illegal_chars_with_unicode(path)
+        self.assertEqual(original_path, path)
+
+    def test_replace_illegal_chars_with_unicode(self):
+        """Test the mangling of the paths.
+
+        We ensure that the illegal chars are replaced by unicode 
+        characters valid in windows.
+        """
+        original_path = 'test'.join(WINDOWS_ILLEGAL_CHARS_MAP)
+        path = replace_illegal_chars_with_unicode(original_path)
+        assert not any(c in path for c in WINDOWS_ILLEGAL_CHARS_MAP)
+        
+        path = replace_unicode_with_illegal_chars(path)
+        self.assertEqual(original_path, path)
 
 
 class TestSpecialOSCalls(BaseTwistedTestCase):
@@ -244,14 +269,14 @@
 
     def test_normpath_with_longprefix(self):
         """Ensure that the normpath is correct when it uses the long prefix"""
-        paths = [os.path.join('A','B'),
-                 os.path.join('A', 'B') + os.path.sep,
-                 os.path.join('A', '.', 'B'),
-                 os.path.join('A', 'foo', '..', 'B')]
+        paths = [os.path.join('A','B?'),
+                 os.path.join('A', 'B?') + os.path.sep,
+                 os.path.join('A:C', '.', 'B?'),
+                 os.path.join('A', 'foo', '..', 'B?')]
         for current_path in paths:
-            long_path = LONG_PATH_PREFIX + current_path
-            self.assertEqual(LONG_PATH_PREFIX + os.path.normpath(current_path),
-                             normpath(long_path))
+            self.assertEqual(os.path.normpath(current_path),
+                             normpath(current_path))
+            self.assertFalse(LONG_PATH_PREFIX in current_path)
 
 
 class TestAccess(BaseTwistedTestCase):
@@ -400,4 +425,4 @@
         groups = {}
         groups[EVERYONE_GROUP] = FILE_ALL_ACCESS
         _set_file_attributes(self.testfile, groups)
-        self.assertTrue(can_write(self.testfile))
\ No newline at end of file
+        self.assertTrue(can_write(self.testfile))

=== modified file 'ubuntuone/platform/windows/filesystem_notifications.py'
--- ubuntuone/platform/windows/filesystem_notifications.py	2011-07-21 13:40:32 +0000
+++ ubuntuone/platform/windows/filesystem_notifications.py	2011-07-21 16:16:14 +0000
@@ -66,12 +66,15 @@
 from ubuntuone.syncdaemon.filesystem_notifications import (
     GeneralINotifyProcessor)
 from ubuntuone.platform.windows.os_helper import (
-    LONG_PATH_PREFIX,
-    listdir,
+    assert_path_is_bytes,
+    assert_path_is_not_literal,
+    get_windows_valid_path,
+    get_syncdaemon_valid_path,
     replace_unicode_with_illegal_chars,
+    windowspath,
 )
 from ubuntuone import logger
-from ubuntuone.platform.windows.os_helper import longpath
+
 # our logging level
 TRACE = logger.TRACE
 
@@ -138,14 +141,14 @@
         self._proc_fun = proc_fun
         self._cookie = None
         self._source_pathname = None
+        self._watch_thread = None
+        self._process_thread = None
         # remember the subdirs we have so that when we have a delete we can
         # check if it was a remove
         self._subdirs = []
         # ensure that we work with an abspath and that we can deal with
         # long paths over 260 chars.
         self._path = os.path.abspath(path)
-        if not self._path.startswith(LONG_PATH_PREFIX):
-            self._path = LONG_PATH_PREFIX + self._path
         self._mask = mask
         # lets make the q as big as possible
         self._raw_events_queue = Queue()
@@ -153,9 +156,9 @@
             events_queue = Queue()
         self.events_queue = events_queue
 
-    @longpath(paths_indexes=[1])
     def _path_is_dir(self, path):
         """Check if the path is a dir and update the local subdir list."""
+        path = replace_unicode_with_illegal_chars(path)
         self.log.debug('Testing if path "%s" is a dir', path)
         is_dir = False
         if os.path.exists(path):
@@ -209,7 +212,8 @@
                 'dir': is_dir,
                 'mask': mask,
                 'name': tail,
-                'path': head.replace(self.path, '.')}
+                'path': head.replace(get_syncdaemon_valid_path(self.path),
+                                     '.')}
             # by the way in which the win api fires the events we know for
             # sure that no move events will be added in the wrong order, this
             # is kind of hacky, I dont like it too much
@@ -281,17 +285,18 @@
             self.log.debug('Events from ReadDirectoryChangesW are %s', results)
             # add the diff events to the q so that the can be processed no
             # matter the speed.
-            for action, file in results:
-                full_filename = replace_unicode_with_illegal_chars(os.path.join(self._path,
-                                                  file))
+            for action, file_name in results:
+                file_name = os.path.join(self._path, file_name)
+                full_filename = get_syncdaemon_valid_path(file_name)
+                # the COM api returns unicode files which although later in
+                # the future is something we want atm we are not interested.
                 self._raw_events_queue.put((full_filename, action))
                 self.log.debug('Added %s to raw events queue.',
                     (full_filename, action))
 
     def start_watching(self):
         """Tell the watch to start processing events."""
-        # get the diff dirs in the path
-        for current_child in listdir(self._path):
+        for current_child in os.listdir(self._path):
             full_child_path = os.path.join(self._path, current_child)
             if os.path.isdir(full_child_path):
                 self._subdirs.append(full_child_path)
@@ -313,14 +318,16 @@
         CloseHandle(self._handle)
         self._watching = False
         # ensure that we have no dangling threads
-        self._watch_thread.join(5)
-        if self._watch_thread.isAlive():
-            self.log.critical('Failed to stop the watch thread for %s!',
-                              self._path)
-        self._process_thread.join(5)
-        if self._process_thread.isAlive():
-            self.log.critical('Failed to stop the process thread for %s!',
-                              self._path)
+        if self._watch_thread:
+            self._watch_thread.join(5)
+            if self._watch_thread.isAlive():
+                self.log.critical('Failed to stop the watch thread for %s!',
+                                  self._path)
+        if self._process_thread:
+            self._process_thread.join(5)
+            if self._process_thread.isAlive():
+                self.log.critical('Failed to stop the process thread for %s!',
+                                  self._path)
         self._subdirs = []
 
     def update(self, mask, proc_fun=None, auto_add=False):
@@ -381,9 +388,11 @@
         except KeyError, e:
             logging.error(str(e))
 
-    @longpath(paths_indexes=[1])
+    @assert_path_is_bytes(paths_indexes=[1])
+    @assert_path_is_not_literal(paths_indexes=[1])
     def _add_single_watch(self, path, mask, proc_fun=None, auto_add=False,
         quiet=True, exclude_filter=None):
+        path = get_windows_valid_path(os.path.abspath(path))
         if path in self._ignored_paths:
             # simply removed it from the filter
             self._ignored_paths.remove(path)
@@ -432,7 +441,9 @@
             if not quiet:
                 raise WatchManagerError('Watch %s was not found' % wd, {})
 
-    @longpath(paths_indexes=[1])
+    @assert_path_is_bytes(paths_indexes=[1])
+    @assert_path_is_not_literal(paths_indexes=[1])
+    @windowspath(paths_indexes=[1])
     def get_wd(self, path):
         """Return the watcher that is used to watch the given path."""
         for current_wd in self._wdm:
@@ -459,7 +470,8 @@
             if not quiet:
                 raise WatchManagerError('Watch %s was not found' % wd, {})
 
-    @longpath(paths_indexes=[1])
+    @assert_path_is_bytes(paths_indexes=[1])
+    @assert_path_is_not_literal(paths_indexes=[1])
     def rm_path(self, path):
         """Remove a watch to the given path."""
         wd = self.get_wd(path)
@@ -563,6 +575,10 @@
         """Stop processing events and the watch manager."""
         self._watch_manager.stop()
 
+# Interfaces that will be exposed to syncdaemon, this means that all passed
+# and returned paths from this point on will have to be in BYTES and not a
+# literal path, that is not \\?\ prefix in the path. This is uber important,
+# in case of a doubt ask mandel or nessita
 
 class NotifyProcessor(ProcessEvent):
     """Processor that takes care of dealing with the events."""
@@ -581,7 +597,8 @@
         """Add an event and path(s) to the mute filter."""
         self.general_processor.add_to_mute_filter(event, paths)
 
-    @longpath(paths_indexes=[1])
+    @assert_path_is_bytes(paths_indexes=[1])
+    @assert_path_is_not_literal(paths_indexes=[1])
     def platform_is_ignored(self, path):
         """Should we ignore this path in the current platform.?"""
         # don't support links yet
@@ -589,7 +606,8 @@
             return True
         return False
 
-    @longpath(paths_indexes=[1])
+    @assert_path_is_bytes(paths_indexes=[1])
+    @assert_path_is_not_literal(paths_indexes=[1])
     def is_ignored(self, path):
         """Should we ignore this path?"""
         return self.general_processor.is_ignored(path)
@@ -699,7 +717,8 @@
             self.release_held_event()
         self.general_processor.push_event(event)
 
-    @longpath(paths_indexes=[1])
+    @assert_path_is_bytes(paths_indexes=[1])
+    @assert_path_is_not_literal(paths_indexes=[1])
     def handle_dir_delete(self, fullpath):
         """Some special work when a directory is deleted."""
         # remove the watch on that dir from our structures
@@ -719,7 +738,8 @@
             else:
                 self.general_processor.eq_push('FS_FILE_DELETE', path=path)
 
-    @longpath(paths_indexes=[1])
+    @assert_path_is_bytes(paths_indexes=[1])
+    @assert_path_is_not_literal(paths_indexes=[1])
     def freeze_begin(self, path):
         """Puts in hold all the events for this path."""
         self.general_processor.freeze_begin(path)
@@ -771,26 +791,12 @@
         self._notifier = Notifier(self._watch_manager, self._processor)
         self._process_task = self._hook_inotify_to_twisted()
 
-    def _fix_mute_filter_info(self, info):
-        # we always make sure that the long path prefix is present so
-        # that we can deal with the paths
-        if 'path' in info and not info['path'].startswith(LONG_PATH_PREFIX):
-            info['path'] = LONG_PATH_PREFIX + info['path']
-        if 'path_to' in info and \
-            not info['path_to'].startswith(LONG_PATH_PREFIX):
-            info['path_to'] = LONG_PATH_PREFIX + info['path_to']
-        if 'path_from' in info and \
-            not info['path_from'].startswith(LONG_PATH_PREFIX):
-            info['path_from'] = LONG_PATH_PREFIX + info['path_from']
-
     def add_to_mute_filter(self, event, **info):
         """Add info to mute filter in the processor."""
-        self._fix_mute_filter_info(info)
         self._processor.add_to_mute_filter(event, info)
 
     def rm_from_mute_filter(self, event, **info):
         """Remove info to mute filter in the processor."""
-        self._fix_mute_filter_info(info)
         self._processor.rm_from_mute_filter(event, info)
 
     def _hook_inotify_to_twisted(self):
@@ -813,13 +819,15 @@
         self._notifier.stop()
         self._process_task.stop()
 
-    @longpath(paths_indexes=[1])
+    @assert_path_is_bytes(paths_indexes=[1])
+    @assert_path_is_not_literal(paths_indexes=[1])
     def rm_watch(self, dirpath):
         """Remove watch from a dir."""
         # trust the implementation of the manager
         self._watch_manager.rm_path(dirpath)
 
-    @longpath(paths_indexes=[1])
+    @assert_path_is_bytes(paths_indexes=[1])
+    @assert_path_is_not_literal(paths_indexes=[1])
     def add_watch(self, dirpath):
         """Add watch to a dir."""
         # we try to get a watch descriptor, it it exists it means that
@@ -830,7 +838,9 @@
             self._watch_manager.add_watch(dirpath, FILESYSTEM_MONITOR_MASK,
                 auto_add=True)
 
-    @longpath(paths_indexes=[1])
+    @assert_path_is_bytes(paths_indexes=[1])
+    @assert_path_is_not_literal(paths_indexes=[1])
+    @windowspath(paths_indexes=[1])
     def has_watch(self, dirpath):
         """Check if a dirpath is watched."""
         wd = self._watch_manager.get_wd(dirpath)
@@ -840,7 +850,8 @@
         """Checks if there's something frozen."""
         return self._processor.frozen_path is not None
 
-    @longpath(paths_indexes=[1])
+    @assert_path_is_bytes(paths_indexes=[1])
+    @assert_path_is_not_literal(paths_indexes=[1])
     def freeze_begin(self, path):
         """Puts in hold all the events for this path."""
         if self._processor.frozen_path is not None:

=== modified file 'ubuntuone/platform/windows/os_helper.py'
--- ubuntuone/platform/windows/os_helper.py	2011-07-21 13:40:32 +0000
+++ ubuntuone/platform/windows/os_helper.py	2011-07-21 16:16:14 +0000
@@ -19,6 +19,7 @@
 """Windows tools to interact with the os."""
 
 import logging
+import errno
 import os
 import sys
 import stat
@@ -76,17 +77,10 @@
     '?':u'\N{ZERO WIDTH SPACE}\N{INTERROBANG}\N{ZERO WIDTH SPACE}',
     '*':u'\N{ZERO WIDTH SPACE}\N{SEXTILE}\N{ZERO WIDTH SPACE}'
 }
-LINUX_ILLEGAL_CHARS_MAP = {
-    u'\N{ZERO WIDTH SPACE}\N{SINGLE LEFT-POINTING ANGLE QUOTATION MARK}\N{ZERO WIDTH SPACE}':'<',
-    u'\N{ZERO WIDTH SPACE}\N{SINGLE RIGHT-POINTING ANGLE QUOTATION MARK}\N{ZERO WIDTH SPACE}':'>',
-    u'\N{ZERO WIDTH SPACE}\N{RATIO}\N{ZERO WIDTH SPACE}':':',
-    u'\N{ZERO WIDTH SPACE}\N{DOUBLE PRIME}\N{ZERO WIDTH SPACE}':'"',
-    u'\N{ZERO WIDTH SPACE}\N{FRACTION SLASH}\N{ZERO WIDTH SPACE}':'/',
-    u'\N{ZERO WIDTH SPACE}\N{SET MINUS}\N{ZERO WIDTH SPACE}':'\\',
-    u'\N{ZERO WIDTH SPACE}\N{DIVIDES}\N{ZERO WIDTH SPACE}':'|',
-    u'\N{ZERO WIDTH SPACE}\N{INTERROBANG}\N{ZERO WIDTH SPACE}':'?',
-    u'\N{ZERO WIDTH SPACE}\N{SEXTILE}\N{ZERO WIDTH SPACE}':'*'
-}
+# inverse map
+LINUX_ILLEGAL_CHARS_MAP = {}
+for key, value in WINDOWS_ILLEGAL_CHARS_MAP.iteritems():
+    LINUX_ILLEGAL_CHARS_MAP[value] = key
 
 
 def _int_to_bin(n):
@@ -94,12 +88,69 @@
     return "".join([str((n >> y) & 1) for y in range(32 - 1, -1, -1)])
 
 
-def _set_as_long_path(path):
-    """Allows to access files longer than 260 chars."""
-    if path and not path.startswith(LONG_PATH_PREFIX):
-        # return the path with the required prefix added
-        return LONG_PATH_PREFIX + path
-    return path
+def get_windows_valid_path(path):
+    """Get a path and modify it so that it can be used.
+
+    On Windows there are a number of things we have to deal with to
+    ensure that we can sync files with Linux. This method takes care of the
+    following changes:
+
+    1. Remove illegal chars: There are a number of illegal chars on windows
+       that are allowed on Linux. The method removes those illegal chars and
+       replaces them with unicode chars that look alike.
+    2. Long paths: There is a limit of 255 chars per path on windows. This is
+       solved by using literal paths (prepending \\?\ to the absolute path).
+    """
+    # we get the path in bytes encoded in utf-8 lets build the unicode
+    path = path.decode('utf8', 'error')
+    # lets remove the long path prefix if present since it has an illegal char in it
+    if path.startswith(LONG_PATH_PREFIX):
+        path = path.replace(LONG_PATH_PREFIX, '')
+    # remove the illegal windows chars
+    path = replace_illegal_chars_with_unicode(path)
+    return  LONG_PATH_PREFIX + path
+
+
+def _assert_path_is_bytes(path):
+    """Ensure that the path is a byte sequence encoded with utf8."""
+    assert isinstance(path, str), 'All paths passed to os_helper should be bytes'
+    try:
+        path = path.decode('utf8', 'error')
+    except UnicodeDecodeError:
+        raise AssertionError(
+                'All paths passed to os_helper should be encoded using utf8')
+
+
+def _assert_path_is_not_literal(path):
+    """Ensure that the path is not a literal one."""
+    assert not path.startswith(LONG_PATH_PREFIX)
+
+
+def _validate_output_collection(col):
+    """Ensure that the collection returned from os helper is valid."""
+    for path in col:
+        _assert_path_is_bytes(path)
+        _assert_path_is_not_literal(path)
+
+
+def get_syncdaemon_valid_path(path):
+    """Get a path used on windows and modify it for syncdaemon.
+
+    On Windows there are a number of changes we make to allow the use of
+    illegal chars, those generate a unicode path which sd does not handle. This
+    method does the following:
+
+    1. Remove utf-8 chars: There are some utf-8 chars used instead of the illegal
+       paths, those are moved back to be windows illegal chars.
+    2. Remove long path prefix: that's an implementation detail isolated in this
+       module.
+    """
+    assert isinstance(path,
+                      unicode), 'All paths from os_helper should be unicode'
+    if path.startswith(LONG_PATH_PREFIX):
+        path = path.replace(LONG_PATH_PREFIX, '')
+    path = replace_unicode_with_illegal_chars(path)
+    return path.encode('utf8', 'error')
 
 
 def _get_group_sid(group_name):
@@ -135,11 +186,16 @@
     """Returns if one of the chars is in the path."""
     if LONG_PATH_PREFIX in path:
         path = path.replace(LONG_PATH_PREFIX, '')
-    return any(c in chars for c in os.path.splitdrive(path)[1])
+    path = os.path.splitdrive(path)[1]
+    return any(c in path for c in chars)
 
 
 def _modify_chars(path, should_apply, chars):
-    """Remove illegal chars and return a valid windows path."""
+    """Remove illegal chars defined by 'chars'.
+
+    Only apply of 'should_apply(path)' holds.
+
+    """
     if not should_apply(path):
         return path
     prefix = False
@@ -162,7 +218,7 @@
         result = volume + '\\' + result
     if prefix:
         result = LONG_PATH_PREFIX + result
-    return unicode(result)
+    return result
 
 
 def is_illegal_path(path):
@@ -187,6 +243,8 @@
 
 def _set_file_attributes(path, groups):
     """Set file attributes using the wind32api."""
+    if not os.path.exists(path):
+        raise WindowsError('Path %s could not be found.' % path)
     security_descriptor = None
     try:
         security_descriptor = GetFileSecurity(path, DACL_SECURITY_INFORMATION)
@@ -213,31 +271,71 @@
     SetFileSecurity(path, DACL_SECURITY_INFORMATION, security_descriptor)
 
 
-def absparam(paths_indexes=[], paths_names=[]):
-    """Ensure that the paths are absolute."""
-    def decorator(function):
-        """Decorate the function to make sure the paths are absolute."""
-        @wraps(function)
-        def abspath_wrapper(*args, **kwargs):
-            """Set the paths to be absolute."""
-            fixed_args = list(args)
-            for current_path in paths_indexes:
-                fixed_args[current_path] = \
-                    os.path.abspath(args[current_path])
-            fixed_args = tuple(fixed_args)
-            for current_key in paths_names:
-                try:
-                    kwargs[current_key] = \
-                        os.path.abspath(kwargs[current_key])
-                except KeyError:
-                    logger.warn('Parameter %s could not be made a long path',
-                        current_key)
-            return function(*fixed_args, **kwargs)
-        return abspath_wrapper
-    return decorator
-
-
-def longpath(paths_indexes=[], paths_names=[]):
+def assert_path_is_bytes(paths_indexes=[], paths_names=[]):
+    """Ensure that passed paths are correct."""
+    def decorator(function):
+        """Decorate function so that the given params are long paths."""
+        @wraps(function)
+        def correct_path_wrapper(*args, **kwargs):
+            """Assert paths are correct."""
+            for current_path in paths_indexes:
+                _assert_path_is_bytes(args[current_path])
+            for current_key in paths_names:
+                _assert_path_is_bytes(kwargs[current_key])
+            return function(*args, **kwargs)
+        return correct_path_wrapper
+    return decorator
+
+
+def assert_path_is_not_literal(paths_indexes=[], paths_names=[]):
+    """Ensure that passed paths are correct."""
+    def decorator(function):
+        """Decorate function so that the given params are long paths."""
+        @wraps(function)
+        def correct_path_wrapper(*args, **kwargs):
+            """Assert paths are correct."""
+            for current_path in paths_indexes:
+                _assert_path_is_not_literal(args[current_path])
+            for current_key in paths_names:
+                _assert_path_is_not_literal(kwargs[current_key])
+            return function(*args, **kwargs)
+        return correct_path_wrapper
+    return decorator
+
+
+def assert_output_path_is_bytes(function):
+    """Ensure that the returned path is correct."""
+    @wraps(function)
+    def correct_path_wrapper(*args, **kwargs):
+        """Assert resulting path."""
+        result = function(*args, **kwargs)
+        _assert_path_is_bytes(result)
+        return result
+    return correct_path_wrapper
+
+
+def assert_output_path_is_not_literal(function):
+    """Ensure that the returned path is not a literal one."""
+    @wraps(function)
+    def correct_path_wrapper(*args, **kwargs):
+        """Assert resulting path."""
+        result = function(*args, **kwargs)
+        _assert_path_is_not_literal(result)
+        return result
+    return correct_path_wrapper
+
+def validate_output_collection(function):
+    """Ensure that the returned collection of paths is correct."""
+    @wraps(function)
+    def correct_path_wrapper(*args, **kwargs):
+        """Assert resulting path."""
+        result = function(*args, **kwargs)
+        _validate_output_collection(result)
+        return result
+    return correct_path_wrapper
+
+
+def windowspath(paths_indexes=[], paths_names=[]):
     """Ensure that the parameters are long paths."""
     def decorator(function):
         """Decorate function so that the given params are long paths.."""
@@ -247,15 +345,10 @@
             fixed_args = list(args)
             for current_path in paths_indexes:
                 fixed_args[current_path] = \
-                    _set_as_long_path(args[current_path])
+                    get_windows_valid_path(args[current_path])
             fixed_args = tuple(fixed_args)
             for current_key in paths_names:
-                try:
-                    kwargs[current_key] = \
-                        _set_as_long_path(kwargs[current_key])
-                except KeyError:
-                    logger.warn('Parameter %s could not be made a long path',
-                        current_key)
+                kwargs[current_key] = get_windows_valid_path(kwargs[current_key])
             return function(*fixed_args, **kwargs)
         return long_path_wrapper
     return decorator
@@ -268,7 +361,8 @@
     _set_file_attributes(path, groups)
 
 
-@longpath(paths_indexes=[0])
+@assert_path_is_bytes(paths_indexes=[0])
+@windowspath(paths_indexes=[0])
 def set_no_rights(path):
     # set the groups to be empty which will remove all the rights of the file
     try:
@@ -279,7 +373,8 @@
         return _check_winerror_123(e, _set_no_rights_helper, path)
 
 
-@longpath(paths_indexes=[0])
+@assert_path_is_bytes(paths_indexes=[0])
+@windowspath(paths_indexes=[0])
 def set_file_readonly(path):
     """Change path permissions to readonly in a file."""
     # we use the win32 api because chmod just sets the readonly flag and
@@ -291,7 +386,8 @@
     _set_file_attributes(path, groups)
 
 
-@longpath(paths_indexes=[0])
+@assert_path_is_bytes(paths_indexes=[0])
+@windowspath(paths_indexes=[0])
 def set_file_readwrite(path):
     """Change path permissions to readwrite in a file."""
     groups = {}
@@ -308,7 +404,8 @@
         return _check_winerror_123(e, os.chmod, path, stat.S_IWRITE)
 
 
-@longpath(paths_indexes=[0])
+@assert_path_is_bytes(paths_indexes=[0])
+@windowspath(paths_indexes=[0])
 def set_dir_readonly(path):
     """Change path permissions to readonly in a dir."""
     groups = {}
@@ -318,7 +415,8 @@
     _set_file_attributes(path, groups)
 
 
-@longpath(paths_indexes=[0])
+@assert_path_is_bytes(paths_indexes=[0])
+@windowspath(paths_indexes=[0])
 def set_dir_readwrite(path):
     """Change path permissions to readwrite in a dir."""
     groups = {}
@@ -329,32 +427,25 @@
     _set_file_attributes(path, groups)
     # remove the read only flag
     os.chmod(path, stat.S_IWRITE)
-    for dirpath, dirs, files in os.walk(path):
-        for dir in dirs:
-            nested_path = os.path.join(dirpath, dir)
-            if not can_write(nested_path):
-                set_dir_readwrite(nested_path)
-        for file in files:
-            nested_path = os.path.join(dirpath, file)
-            if not can_write(nested_path):
-                set_file_readwrite(nested_path)
-
-
+
+
+@assert_path_is_bytes(paths_indexes=[0])
 @contextmanager
-@longpath(paths_indexes=[0])
 def allow_writes(path):
     """A very simple context manager to allow witting in RO dirs."""
+    valid_path = get_windows_valid_path(path)
     # get the old dacl of the file so that we can reset it back when done
-    security_descriptor = GetFileSecurity(path, DACL_SECURITY_INFORMATION)
+    security_descriptor = GetFileSecurity(valid_path, DACL_SECURITY_INFORMATION)
     old_dacl = security_descriptor.GetSecurityDescriptorDacl()
     set_dir_readwrite(path)
     yield
     # set the old dacl back
     security_descriptor.SetSecurityDescriptorDacl(1, old_dacl, 0)
-    SetFileSecurity(path, DACL_SECURITY_INFORMATION, security_descriptor)
-
-
-@longpath(paths_indexes=[0])
+    SetFileSecurity(valid_path, DACL_SECURITY_INFORMATION, security_descriptor)
+
+
+@assert_path_is_bytes(paths_indexes=[0])
+@windowspath(paths_indexes=[0])
 def remove_file(path):
     """Remove a file."""
     try:
@@ -364,7 +455,8 @@
         return _check_winerror_123(e, os.remove, path)
 
 
-@longpath(paths_indexes=[0])
+@assert_path_is_bytes(paths_indexes=[0])
+@windowspath(paths_indexes=[0])
 def remove_dir(path):
     """Remove a dir."""
     try:
@@ -374,7 +466,8 @@
         return _check_winerror_123(e, os.rmdir, path)
 
 
-@longpath(paths_indexes=[0])
+@assert_path_is_bytes(paths_indexes=[0])
+@windowspath(paths_indexes=[0])
 def path_exists(path):
     """Return if the path exists."""
     if is_illegal_path(path):
@@ -382,7 +475,8 @@
     return os.path.exists(path)
 
 
-@longpath(paths_indexes=[0])
+@assert_path_is_bytes(paths_indexes=[0])
+@windowspath(paths_indexes=[0])
 def make_dir(path, recursive=False):
     """Make a dir, optionally creating all the middle ones."""
     if recursive:
@@ -399,7 +493,8 @@
             return _check_winerror_123(e, os.mkdir, path)
 
 
-@longpath(paths_indexes=[0])
+@assert_path_is_bytes(paths_indexes=[0])
+@windowspath(paths_indexes=[0])
 def open_file(path, mode='r'):
     """Open a file."""
     try:
@@ -409,8 +504,8 @@
         return _check_winerror_123(e, open, path, mode)
 
 
-@absparam(paths_indexes=[0, 1])
-@longpath(paths_indexes=[0, 1])
+@assert_path_is_bytes(paths_indexes=[0, 1])
+@windowspath(paths_indexes=[0, 1])
 def rename(path_from, path_to):
     """Rename a file or directory."""
     # to ensure the same behaviors as on linux, use the MoveEx function from
@@ -444,7 +539,8 @@
             raise OSError(str(e))
 
 
-@absparam(paths_indexes=[0, 1])
+@assert_path_is_bytes(paths_indexes=[0, 1])
+@windowspath(paths_indexes=[0, 1])
 def make_link(target, destination):
     """Create a link from the target in the given destination."""
     # append the correct file type
@@ -455,12 +551,15 @@
     if dirname != '' and not os.path.exists(dirname):
         make_dir(dirname, recursive=True)
     shell = Dispatch('WScript.Shell')
-    shortcut = shell.CreateShortCut(destination)
-    shortcut.Targetpath = target
+    shortcut = shell.CreateShortCut(os.path.abspath(destination))
+    shortcut.Targetpath = os.path.abspath(target)
     shortcut.save()
 
 
-@absparam(paths_indexes=[0])
+@assert_output_path_is_not_literal
+@assert_output_path_is_bytes
+@assert_path_is_bytes(paths_indexes=[0])
+@windowspath(paths_indexes=[0])
 def read_link(path):
     """Read the destination of a link."""
     if not path.endswith('.lnk'):
@@ -470,7 +569,8 @@
     return os.path.abspath(shortcut.Targetpath)
 
 
-@absparam(paths_indexes=[0])
+@assert_path_is_bytes(paths_indexes=[0])
+@windowspath(paths_indexes=[0])
 def is_link(path):
     """Returns if a path is a link or not."""
     if not path.endswith('.lnk'):
@@ -478,7 +578,8 @@
     return os.path.exists(path)
 
 
-@absparam(paths_indexes=[0])
+@assert_path_is_bytes(paths_indexes=[0])
+@windowspath(paths_indexes=[0])
 def remove_link(path):
     """Removes a link."""
     if not path.endswith('.lnk'):
@@ -486,17 +587,43 @@
     if os.path.exists(path):
         os.unlink(path)
     
-
-@longpath(paths_indexes=[0])
+@validate_output_collection
+@assert_path_is_bytes(paths_indexes=[0])
 def listdir(directory):
     """List a directory."""
-    # os.listdir combines / and \ in its search which breaks things on
-    # windows, we use the win32 api instead.
-    if not directory.endswith(os.path.sep):
-        directory += os.path.sep
-    return os.listdir(directory)
-
-@longpath(paths_indexes=[0])
+    
+    def set_correct_output(path):
+        """Return the output in the correct obj type."""
+        path = replace_unicode_with_illegal_chars(path)
+        if isinstance(directory, str):
+            return path.encode('utf8', 'error')
+        return path
+
+    valid_path = get_windows_valid_path(directory)
+    # The main reason why we have to append os.path.sep is the following:
+    #
+    # os.listdir implementation will append a unix path separator to the
+    # path used for listdir, for example if we do:
+    # os.litdir('C:\\Python27')
+    # the path would be:
+    # 'C:\\Python27/*.*'
+    # the above does not generate any problems, unfortunatly the same cannot
+    # be said when using literal paths, that is, paths starting with '\\?\'.
+    # So, while the above works (and returns a consistent error if the path
+    # does not exist), the following fails:
+    # os.listdir('\\\\?\\C:\\Python27')
+    # with the following exception:
+    # WindowsError: [Error 123] The filename, directory name, or volume
+    # label syntax is incorrect: '\\\\?\\C:\\Python27/*.*'
+    # which gets fixed if os.path.sep is added, that is, if we use:
+    # os.listdir('\\\\?\\C:\\Python27\\')
+    if not valid_path.endswith(os.path.sep):
+        valid_path += os.path.sep
+    return [set_correct_output(path) for path in os.listdir(valid_path)]
+
+
+@assert_path_is_bytes(paths_indexes=[0])
+@windowspath(paths_indexes=[0])
 def access(path):
     """Return if the path is at least readable."""
     # lets consider the access on an illegal path to be a special case
@@ -530,7 +657,8 @@
             os.access(path, os.R_OK)
 
 
-@longpath(paths_indexes=[0])
+@assert_path_is_bytes(paths_indexes=[0])
+@windowspath(paths_indexes=[0])
 def can_write(path):
     """Return if the path is at least readable."""
     # lets consider the access on an illegal path to be a special case
@@ -563,7 +691,9 @@
     return (GetUserName() in accounts or EVERYONE_GROUP in accounts) and\
             os.access(path, os.R_OK)
 
-@longpath(paths_indexes=[0])
+
+@assert_path_is_bytes(paths_indexes=[0])
+@windowspath(paths_indexes=[0])
 def stat_path(path, look_for_link=True):
     """Return stat info about a path."""
     # if the path end with .lnk, that means we are dealing with a link
@@ -577,10 +707,13 @@
     try:
         return os.lstat(path)
     except OSError, e:
+        if e.errno == errno.ENOENT:
+            raise e
         logger.exception('Got exception when trying to stat path %s', path)
         return _check_winerror_123(e, stat_path, path, look_for_link)
 
 
+@assert_path_is_bytes(paths_indexes=[0])
 def move_to_trash(path):
     """Move the file or dir to trash.
 
@@ -602,24 +735,29 @@
 
 def validate_path_from_unix(path):
     """Get a unix path and return it so that it can be used."""
-    path = path.replace('/', '\\')
-    return path
-
-
-@longpath(paths_indexes=[0])
+    return path.replace('/', '\\')
+
+
+@validate_output_collection
+@assert_path_is_bytes(paths_indexes=[0])
+@windowspath(paths_indexes=[0])
 def get_path_list(path):
     """Return a list with the diff components of the path."""
     path = path.replace(LONG_PATH_PREFIX, '')
     path = path.replace(os.path.expandvars('%HOMEDRIVE%') + os.path.sep, '')
-    list = [LONG_PATH_PREFIX + os.path.expandvars('%HOMEDRIVE%'),]
-    list.extend(path.split(os.path.sep))
-    return list
-
-
+    result = [LONG_PATH_PREFIX + os.path.expandvars('%HOMEDRIVE%'),]
+    result.extend(path.split(os.path.sep))
+    # ensure that we do return the original chars
+    return [replace_unicode_with_illegal_chars(path) for path in result]
+
+
+@assert_output_path_is_not_literal
+@assert_output_path_is_bytes
+@assert_path_is_bytes(paths_indexes=[0])
 def normpath(path):
     """Normalize path, eliminating double slashes, etc."""
     # the basic normpath does not work correctly with \\?\ which we use
     # as a trick for long paths, lets check it is is there
     if path.startswith(LONG_PATH_PREFIX):
         path = path.replace(LONG_PATH_PREFIX, '')
-    return LONG_PATH_PREFIX + os.path.normpath(path)
\ No newline at end of file
+    return os.path.normpath(path)

=== modified file 'ubuntuone/platform/windows/vm_helper.py'
--- ubuntuone/platform/windows/vm_helper.py	2011-07-08 12:21:59 +0000
+++ ubuntuone/platform/windows/vm_helper.py	2011-07-21 16:16:14 +0000
@@ -19,7 +19,6 @@
 
 import os
 from ubuntuone.platform.windows.os_helper import (
-    LONG_PATH_PREFIX,
     make_link,
 )
 
@@ -68,4 +67,4 @@
 def get_udf_path(suggested_path):
     """Build the udf path using the suggested_path."""
     path = suggested_path.replace('/', '\\')
-    return LONG_PATH_PREFIX + os.path.expanduser(path).encode("utf8")
+    return os.path.expanduser(path)