bugrdiff-backup - Bugs: bug #24100, Can't specify the root directory...

 
 

bug #24100: Can't specify the root directory ('c:\') as src in native win32 binary.

Submitter:  Anders Hellstrand <ahd71>
Submitted:  Tue 19 Aug 2008 01:01:16 AM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  None Status:  In Progress
Privacy:  Public Assigned to:  owsla
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 10 Oct 2008 09:20:08 PM UTC, comment #23: 

Hi Andrew,

Now I know more and it seems to work good with / instead of \.

In your first reply you asked me to try with / instead of \ and I did, however I did one serious mistake when doing that test. I only had access to one drive on that computer and did something simlar to this: "rdiff-backup -v5 c:/ c:/tmp24" which didn't work as I replied but the reason is that it is on the same drive and not because of the regular slash.

If I do the same thing but between different drives it works ("rdiff-backup -v5 d:/ c:/tmp25") which solves the problem if the user are aware of inverting slashes from standard windows syntax, espacially when it's a trailing slash from a root folder. 

But, that is not a problem when it's documented which I know you already have done (six days after that this bug report was written according to CVS).

I'm sorry to have bothered you with this discussion but I guess we have learned something and other users might find the thread in the future if they encounter the same problem as I did (and they didn't read the Windows-README.txt)

You may close the bug report if you like. Have a nice weekend!

Thanks / Anders

Anders Hellstrand <ahd71>
Thu 09 Oct 2008 10:12:38 PM UTC, comment #22: 

I'm not going to edit any code yet, because this works:

mkdir c:\foo
cd "c:\Documents and Settings"
rdiff-backup.exe --include c:\\/foo --exclude c:\\/** c:\/ c:\bar


Here's the thing, Anders. If/after we get this invalid escape sequence code fixed in SetConnections.py, we would still have many more problems in selection.py. You see, the selection code truly expects that the directory separator is /. We can't change that easily because \ is used for the regular-expressions escape sequence (as is very common practice -- I don't know any regex package that doesn't use \ for escape).

So, why does this work?

The code which handles --include and --exclude expect true escaping -- in other words, \\ -> \.  However, the code which handles the paths does a really lazy job at unescaping. It does not map \\ -> \.

Can you confirm this behavior?

Andrew Ferguson <owsla>
Group administrator
Thu 09 Oct 2008 09:18:47 PM UTC, comment #21: 

Hi,

No I don't have a build environment for Windows yet, so yes please post a link to a updated version when it's availible.

Thankyou once again for all effort you are putting into this lovely piece of software!

BR / Anders

Anders Hellstrand <ahd71>
Thu 09 Oct 2008 09:01:55 PM UTC, comment #20: 

Valid filenames on Unix include any character except the directory separator '/' and the null character (ASCII zero).

The real problem here is that the code is not, in fact, handling the escape character, \ properly. If it did, then you would be able to type,

rdiff-backup c\:\\ \\\\server\\share

BUT! All these new Windows users are now able to type:

rdiff-backup "c:\my docouments" \\server\share

and are happy ... Introducing proper escaping for \ character at this point would be counter-productive. (Basically, the documentation is a bit wrong.)

So, yeah, while it is traditionally a good idea to complain about an invalid (dangling) escape sequence, I think we may be in a mess where it does more harm than good.

Do you have a Windows build environment? Or should I post a test build for you?

Andrew Ferguson <owsla>
Group administrator
Thu 09 Oct 2008 07:57:51 PM UTC, comment #19: 

Andrew,

Thanks for the explanation of "Windows-long-path-names-with-spaces-in". Great - both that it works and that it is already supported!

Regarding my testcase with "two::colon" it was just the worst possible names choosen by me, it should have been written as "server::path" of course and that passes the test (both with and without the patch).

I didn't know that :: was allowed in linux file names, but of course then the .split method is a bad way to do it.

Take your time and give the proposed patch a second thought because I still think that it may solve the problem. The only thing that the patch does is that if a trailing slash exist in the string, rdiff-backup will not go thru the following lines where the out of range error occur.
The only drawback i can think of, and you may know if there are a use case there, is if there is a good reason to raise an error if a trailing slash exists when used from Linux. (I suspect that the already existing error handling are there for a reason?).
It may be neccessary to have a linux case and a windows case with an OS check as in some other places of the code to allow/disallow a trailing slash?

BR / Anders

Anders Hellstrand <ahd71>
Wed 08 Oct 2008 01:56:39 AM UTC, comment #18: 

Anders,

The reason why we can't blindly split on :: is because :: is valid as part of a path (filename), but rdiff-backup uses it to separate the hostname from the path.

Therefore, we have to have some way to escape characters. This doesn't come from Windows or from Unix, but from the basic need to escape characters which are being used for something special by rdiff-backup, but which are also valid in filenames. So, yes, escape character is relevant on Windows.

If we have an escape character, which is also valid as part of a filename, then we need to be able to escape it as well.

Your two::colons test case will fail, even with your patch. Rdiff-backup will interpret it as 'ssh to host "two"' then run 'rdiff-backup --server' with the target directory "colons".


Does this all make sense to you?


Andrew

Andrew Ferguson <owsla>
Group administrator
Wed 08 Oct 2008 12:35:19 AM UTC, comment #17: 

Anders,

I have not yet finished reading and thinking about all of your comments, but I did want to add: Rdiff-backup has no trouble with spaces in filenames (or long files) on Windows. Simply use "" to surround the filename and you're good to go.

eg:

rdiff-backup "C:/Documents and Settings/My Username/My Test" "C:/Documents and Settings/My Username/Test Output"

works fine for me without any changes. The "" marks are just so that the two paths are not split up by the Windows shell. On Unix, the unix shell would also split paths improperly on the spaces, and using "" around them is one solution.

The fact that you can also solve this space problem on Unix shells by escaping the spaces is just a second solution.


Andrew

Andrew Ferguson <owsla>
Group administrator
Tue 07 Oct 2008 10:29:45 AM UTC, comment #16: 

Hi,

I think that I have a very small adjustment of the code that might solve the problem.

If we modify the the procedure 'parse_file_desc(file_desc)' in SetConnections.py and alter...

>> if file_desc[i] == '\\':


...to also have a second criteria that exclude the corner case when the backslash is in the last position of an input string like this...

>> if (file_desc[i] == '\\' and len(file_desc)<>i+1):


...it makes all my previous listed test cases to pass without the out of range error, and hopefully solves the problem (don't have the build environment for Windows myself yet to prove that it works the but I guess it will)

As asked just before, I don't realy understand why we can't just search for :: and split the string which would make the code much more readable.

What do you think about the patch? Is it a practical way of solving the problem or does we only work around it or even worse introduce any new potential problem?

Is it better to crash with the out of index problem then at a later stage in some case as the "Unexpected end to file" error message explicitly just triggers on that behaviour?

Another, but not so beautiful solution is to just cover Windows OS for this workaround as backslash as a "escape characters" isn't relevant in Windows.

BR / Anders

Anders Hellstrand <ahd71>
Tue 07 Oct 2008 09:12:08 AM UTC, comment #15: 

In the procedure 'parse_file_desc' there are a source code comment which I don't fully understand.

>> "The complication is to allow for quoting of : by a \.


Can somebody explain why/when this is needed? Otherwise I suppose the use of file_desc.split("::") could be helpful to simplify the procedure?

Regarding the handling of long filenames in windows containing spaces i guess that the easiest thing is that the user call rdiff-backup using pathnames in the DOS convention naming style (8.3 syntax) where "c:\long folder name" is equal to "C:\LONGFO~1". (Spaces removed and strings longer then 8 characters are normally truncated to 6 characters and a tilde and digit is added to differentiate potential naming conflicts)

If i make a 'unit test' of the procudure which testcases should I add more then the following? (some are similiar I know but rather too many then too few! I wanted to highligh some variants like windows using front OR backslash and also that it should work using an optional traling backslash after a folder name; unfortunantly not using a trailing backslash of a root folder does not give it the same meaning and thats why this
ticket is still open, the posibility to use "c:\")

testcases:

singleword
/linux/path/
c:/program1
c:\program2
one:colon   << no real case, i don't know why i added it...
two::colons
server.domain::/destdir1
user@server.domain::/destdir2
c:\dir1
c:\dir2\    << currently failing because of trailing backslash
c:\         << currently failing because of trailing backslash

BR / ahd71

Anders Hellstrand <ahd71>
Mon 25 Aug 2008 06:00:14 PM UTC, comment #14: 

Hi,

Sorry for beeing absent a few days.

Yes, and no, working from the root folder works with a syntax like
c:\program\rdiff-backup\rdiff-backup-1.2.0-win32.exe c:/ c:\level1\level2
(where the first c:/ is a valid replacement for c:\ which currently doesn't work).

That leads me to think that \ isn't a problem and "unescaped" in this situation as the \ between level1 and level2 in this example works.

I think, but you know the code much better, that the only thing that needs to be fixed is to modify the procedure so it's allows a trailing \ without generating an "out of range error".

However another problem that will come - sooner or later - is the fact that Windows doesn't have escapecharacters the way that linux has. Because of that it makes it hard to use folder names with spaces in if this should be allowed on the command line. One way of solving this is to optionally allow ' surrounding directory names, but maybe this will lead to unwanted effects in linux where this is a possible character in a filename. Am I right? Another way of solving it is to use the globbing file list which don't need to have this problem.

BR /ahd71

Anders Hellstrand <ahd71>
Sun 24 Aug 2008 11:43:51 PM UTC, comment #13: 

False alarm. That only worked because I was in c:\ as my working directory. :-(

Andrew Ferguson <owsla>
Group administrator
Thu 21 Aug 2008 12:08:21 AM UTC, comment #12: 

Anders,

--include c:/Python25 --exclude c:/** c:/ owsla@172.16.213.2::/tmp/test

worked fine for me. So, I think you should be able to use 1.2.0 just fine if you simply reverse all of the slashes... This simply leaves the question of whether we should accommodate Windows slashes in the first place.

In the windows shell, what is the common escape character? Really, rdiff-backup only supports the escape character because :: is used as the separator between the host and the path. For rdiff-backup's purposes, you only need to escape the first : if it is not being used as the separator.

See the man page:
http://www.nongnu.org/rdiff-backup/rdiff-backup.1.html
the paragraph which starts, "Concerning  quoting, if for some reason..." --- I would appreciate your thoughts after reading that paragraph.

Andrew Ferguson <owsla>
Group administrator
Wed 20 Aug 2008 11:21:58 PM UTC, comment #11: 

Ok, that second problem is actually different and due to the code in selection.py If you look at select.py, you will see quite a few places where it assumes the directory separator is "/" ...

Andrew Ferguson <owsla>
Group administrator
Wed 20 Aug 2008 10:21:14 PM UTC, comment #10: 

All these three combination works...
rdiff-backup-1.2.0-win32.exe c:\\. c:\tmp1
rdiff-backup-1.2.0-win32.exe c:\\ c:\tmp2
rdiff-backup-1.2.0-win32.exe c:\. c:\tmp3

...But not the following one...
rdiff-backup-1.2.0-win32.exe c:\ c:\tmp4

At the same time, none of the three first variants works if an option like
--include c:\data
is added. This will then result in an error like:

Fatal Error: Fatal Error: The file specification
    'c:\data'
cannot match any files in the base directory
    'c:\\'

BR /ahd71

Anders Hellstrand <ahd71>
Wed 20 Aug 2008 10:10:43 PM UTC, comment #9: 

the quoting around directory names would be nice to have but not nescesary, but some way to use directories with spaces in the name must of course exist. One solution is to use the globbing options and a file list in a file maybe?

the folder in windows called:
c:\documents and settings
could be written at command line as either
'c:\documents and settings'
or as
c:\\documents\ and\ settings
but the last option requires windows users to learn new habits if they are unfamiliar with linux escape syntax.

BR / ahd71

Anders Hellstrand <ahd71>
Wed 20 Aug 2008 09:54:57 PM UTC, comment #8: 

Hi,

I don't have a compile/build environment to make the code changes myself and test. I was thinking of a command line similar like this example:

rdiff-backup --include 'c:\documents' --include 'c:\images' --exclude '**' c:\ d:\backup

Can't the function use file_desc.split("::") in some way?

BR /ahd71

Anders Hellstrand <ahd71>
Wed 20 Aug 2008 09:40:06 PM UTC, comment #7: 

I actually just tried making that code change and it worked for me. Anders, do you see the same thing? How about if you try your include/exclude rules?

If you can't get them to work, can you please submit the command line you are using to call rdiff-backup, plus the list of include/exclude rules you wish to use? That way, I can adjust the code until it behaves correctly.

Thanks.

Andrew Ferguson <owsla>
Group administrator
Wed 20 Aug 2008 09:30:22 PM UTC, comment #6: 

c:\\. works for me (escape the backslash, then put a period)

If you change the line:
if file_desc[i] == '\\':
to
if file_desc[i] == '\\' and not last_was_quoted:

does c:\\ work for you?


Andrew

Andrew Ferguson <owsla>
Group administrator
Wed 20 Aug 2008 11:06:23 AM UTC, comment #5: 

Hi,

It's the function parse_file_desc() which is the problem (which never were a problem as long as it was used in linux or cygwin as a single trailing backslash won't occur in real life).

Just call the function parse_file_desc("c:\") and you will see the problem which results in a "index out of range" because of the logic which increases the index and assumes that a single backslash can't occur in a string.

BR / Ahd71

Anders Hellstrand <ahd71>
Wed 20 Aug 2008 09:49:18 AM UTC, comment #4: 


c\:\\ and c\:/ did not work (Fatal Error: Source directory ... does not exist)

However the combination c:\/ did work.

Thanks for your time and effort in resolving this issue!

BR/ahd71



Anders Hellstrand <ahd71>
Wed 20 Aug 2008 03:04:02 AM UTC, comment #3: 

After studying the code, I have two more tests for you,

c\:\\

and

c\:/

The point is that the : should also be escaped because rdiff-backup uses :: to separate the hostname from the path.

If escaping the colon works, then either \\ (giving us standard windows paths, but escaped) or the simple / should work.

Andrew Ferguson <owsla>
Group administrator
Tue 19 Aug 2008 01:15:08 AM UTC, comment #2: 

Sorry, it didn't work to use a regular slash instead of a backslash. I got the following stack trace then:

Data: {'type': None}' raised of class '<type 'exceptions.AssertionError'>':
  File "rdiff_backup\Main.pyc", line 302, in error_check_Main
  File "rdiff_backup\Main.pyc", line 322, in Main
  File "rdiff_backup\Main.pyc", line 278, in take_action
  File "rdiff_backup\Main.pyc", line 332, in Backup
  File "rdiff_backup\fs_abilities.pyc", line 779, in backup_set_globals
  File "rdiff_backup\fs_abilities.pyc", line 522, in get_readonly_fsa
  File "rdiff_backup\fs_abilities.pyc", line 128, in init_readonly
  File "rdiff_backup\fs_abilities.pyc", line 342, in set_case_sensitive_readonly

  File "rdiff_backup\fs_abilities.pyc", line 325, in test_triple

Anders Hellstrand <ahd71>
Tue 19 Aug 2008 01:09:46 AM UTC, comment #1: 

Out of curiosity, what happens if you try switching all of the '\' characters to '/' ?

Python on Windows is smart enough to translate automatically, for example:

f = open("C:/Documents and Settings/Administrator/Desktop/test.txt", "w")
f.write("test file")
f.close()

works perfectly to create a test file on my desktop.

I admit it's unintuitive, however, it may be the most simple solution.

Andrew Ferguson <owsla>
Group administrator
Tue 19 Aug 2008 01:01:16 AM UTC, original submission:  

The native win32 binary, release 1.2, can't use the root directory of a windows file system as source directory without a exception on execution.

Exception 'Unexpected end to file description c:\' raised of class '<class 'rdiff_backup.SetConnections.SetConnectionsException'>':
  File "rdiff_backup\Main.pyc", line 302, in error_check_Main
  File "rdiff_backup\Main.pyc", line 317, in Main
  File "rdiff_backup\SetConnections.pyc", line 59, in get_cmd_pairs
  File "rdiff_backup\SetConnections.pyc", line 107, in parse_file_desc
  File "rdiff_backup\SetConnections.pyc", line 98, in check_len

The function 'parse_file_desc()' in 'SetConnection.py' works with 'c:\dir' but not with 'c:\' or 'c:\dir1\'

The problem is strings which ends with a backslash (as the logic thinks that it is a escape character which it isn't in this case and that will generate an index out of bound error)

Anders Hellstrand <ahd71>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by davekempe (Updated the item)
  • -email is unavailable- added by owsla (Posted a comment)
  • -email is unavailable- added by ahd71 (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 15 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-02-12 davekempe Open/ClosedOpen Closed
        SummaryCan\'t specify the root directory (\'c:\\\') as src in native win32 binary. Can't specify the root directory ('c:\') as src in native win32 binary.
    2008-10-09 owsla SummaryCan\'t specify the root directory (\'c:\\\') as src in native win32 binary. Can't specify the root directory ('c:\') as src in native win32 binary.
    2008-10-09 owsla SummaryCan\'t specify the root directory (\'c:\\\') as src in native win32 binary. Can't specify the root directory ('c:\') as src in native win32 binary.
    2008-10-08 owsla SummaryCan\'t specify the root directory (\'c:\\\') as src in native win32 binary. Can't specify the root directory ('c:\') as src in native win32 binary.
    2008-10-08 owsla SummaryCan\'t specify the root directory (\'c:\\\') as src in native win32 binary. Can't specify the root directory ('c:\') as src in native win32 binary.
    2008-08-24 owsla SummaryCan\'t specify the root directory (\'c:\\\') as src in native win32 binary. Can't specify the root directory ('c:\') as src in native win32 binary.
    2008-08-21 owsla SummaryCan\'t specify the root directory (\'c:\\\') as src in native win32 binary. Can't specify the root directory ('c:\') as src in native win32 binary.
    2008-08-20 owsla SummaryCan\'t specify the root directory (\'c:\\\') as src in native win32 binary. Can't specify the root directory ('c:\') as src in native win32 binary.
    2008-08-20 owsla SummaryCan\'t specify the root directory (\'c:\\\') as src in native win32 binary. Can't specify the root directory ('c:\') as src in native win32 binary.
    2008-08-20 owsla SummaryCan\'t specify the root directory (\'c:\\\') as src in native win32 binary. Can't specify the root directory ('c:\') as src in native win32 binary.
    2008-08-20 owsla SummaryCan\'t specify the root directory (\'c:\\\') as src in native win32 binary. Can't specify the root directory ('c:\') as src in native win32 binary.
    2008-08-19 owsla StatusNone In Progress
        Assigned toNone owsla
        SummaryCan\'t specify the root directory (\'c:\\\') as src in native win32 binary. Can't specify the root directory ('c:\') as src in native win32 binary.

    Back to the top

    Powered by Savane 3.13-bb6a.
    Corresponding source code