[PATCH 0/3] Handle NTFS 3g case sensitive Windows paths in the daemon
In the current Perl library we have a function called resolve_windows_path which handles the useful [for Windows guests] case where we need to resolve the true, case-sensitive path behind a case-insensitive Windows path. For example, a Windows path like "C:\Windows\System32" can be presented to Linux by NTFS 3g in a variety of ways, eg: /WINDOWS/system32 /WINDOWS/SYSTEM32 depending entirely on the inconsequential details of how it was originally created. Windows doesn't care about case sensitivity, but NTFS 3g does. Since resolve_windows_path is a very useful function, the first patch moves it into the daemon. The new API is called "guestfs_case_sensitive_path". The second patch adds some useful functionality to guestfish, so you can use Windows-style paths, like this: touch win:C:\windows\foo (Note that all ordinary paths must begin with a '/' char, so this can't conflict with any existing, working scripts). The third patch deprecates resolve_windows_path in the Perl code and replaces it with calls to $g->case_sensitive_path. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
Richard W.M. Jones wrote: ...
diff --git a/daemon/realpath.c b/daemon/realpath.c ...
Hi Rich,
@@ -42,3 +46,111 @@ do_realpath (const char *path)
return ret; /* caller frees */ } + +char * +do_case_sensitive_path (const char *path) +{ + char ret[PATH_MAX+1] = "/"; + size_t next = 1; + + /* MUST chdir ("/") before leaving this function. */ + if (chdir (sysroot) == -1) {
If this function might ever be used from a multi-threaded application, then you'll want to change it not to use chdir, since chdir changes the process-wide current directory. That can cause rare but particularly hard to debug problems. Another (albeit small) advantage of not performing the chdir/readdir is that the function will be able to handle those rare names with a directory component that grants "x" but not "r" access.
+ reply_with_perror ("%s", sysroot); + return NULL; + } + + /* First character is a '/'. Take each subsequent path element + * and follow it. + */ + path++; + while (*path) { + size_t i = strcspn (path, "/"); + if (i == 0) { /* "//" in path */ + path++; + continue; + } + if ((i == 1 && path[0] == '.') || + (i == 2 && path[0] == '.' && path[1] == '.')) { + reply_with_error ("case_sensitive_path: path contained . or .. elements"); + goto error; + } + if (i > NAME_MAX) { + reply_with_error ("case_sensitive_path: path element too long"); + goto error; + } + + char name[NAME_MAX+1]; + memcpy (name, path, i); + name[i] = '\0'; + + /* Skip to next element in path (for the next loop iteration). */ + path += i; + if (*path == '/') path++; + + /* Read the current directory looking (case insensitively) for + * this element of the path. + */ + DIR *dir = opendir ("."); + if (dir == NULL) { + reply_with_perror ("opendir"); + goto error; + } + + struct dirent *d = NULL; + + while ((d = readdir (dir)) != NULL) {
If you want to distinguish readdir "EOF" from failure (e.g., EIO), you'll have to set errno=0 before each call and test whether it's still 0 upon NULL return.
+ if (strcasecmp (d->d_name, name) == 0) + break; + } + + if (closedir (dir) == -1) { + reply_with_perror ("closedir"); + goto error; + } + + if (d == NULL) { + reply_with_error ("%s: no file or directory found with this name", name); + goto error; + } + + /* Add the real name of this path element to the return value. */ + if (next > 1) + ret[next++] = '/'; + + i = strlen (d->d_name); + if (next + i >= PATH_MAX) { + reply_with_error ("final path too long"); + goto error; + } + + strcpy (&ret[next], d->d_name); + next += i; + + /* Is it a directory? Try going into it. */ + if (chdir (d->d_name) == 0) + continue; + + /* This is OK provided we've reached the end of the path. */ + if (errno == ENOTDIR) { + if (*path) { + reply_with_error ("non-directory element in path"); + goto error; + } + break; + }
What about other errno values?
+ } + + ignore_value (chdir ("/")); + + ret[next] = '\0'; + char *retp = strdup (ret); + if (retp == NULL) { + reply_with_perror ("strdup"); + return NULL; + } + return retp; /* caller frees */ + + error: + ignore_value (chdir ("/")); + return NULL;
On Mon, Oct 26, 2009 at 11:41:22AM +0100, Jim Meyering wrote:
Richard W.M. Jones wrote:
+char * +do_case_sensitive_path (const char *path) +{ + char ret[PATH_MAX+1] = "/"; + size_t next = 1; + + /* MUST chdir ("/") before leaving this function. */ + if (chdir (sysroot) == -1) {
If this function might ever be used from a multi-threaded application, then you'll want to change it not to use chdir, since chdir changes the process-wide current directory. That can cause rare but particularly hard to debug problems.
This code would be much harder to write if we don't use chdir. Any suggestions on that? If there was a "readdirat" variant then that might be promising. Note that the daemon isn't likely to be multithreaded any time soon (more likely to be pre-forking multiprocess if anything).
Another (albeit small) advantage of not performing the chdir/readdir is that the function will be able to handle those rare names with a directory component that grants "x" but not "r" access.
This shouldn't be an issue because the daemon always runs as root.
+ while ((d = readdir (dir)) != NULL) {
If you want to distinguish readdir "EOF" from failure (e.g., EIO), you'll have to set errno=0 before each call and test whether it's still 0 upon NULL return.
OK, I'll fix that.
+ /* This is OK provided we've reached the end of the path. */ + if (errno == ENOTDIR) { + if (*path) { + reply_with_error ("non-directory element in path"); + goto error; + } + break; + }
What about other errno values?
.. and I'll fix that. Thanks for looking at this. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
On 26/10/09 10:52, Richard W.M. Jones wrote:
On Mon, Oct 26, 2009 at 11:41:22AM +0100, Jim Meyering wrote:
Richard W.M. Jones wrote:
+char * +do_case_sensitive_path (const char *path) +{ + char ret[PATH_MAX+1] = "/"; + size_t next = 1; + + /* MUST chdir ("/") before leaving this function. */ + if (chdir (sysroot) == -1) {
If this function might ever be used from a multi-threaded application, then you'll want to change it not to use chdir, since chdir changes the process-wide current directory. That can cause rare but particularly hard to debug problems.
This code would be much harder to write if we don't use chdir. Any suggestions on that? If there was a "readdirat" variant then that might be promising.
There's an fdopendir(), which returns the DIR* required by readdir(). Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
Richard W.M. Jones wrote:
On Mon, Oct 26, 2009 at 11:41:22AM +0100, Jim Meyering wrote:
Richard W.M. Jones wrote:
+char * +do_case_sensitive_path (const char *path) +{ + char ret[PATH_MAX+1] = "/"; + size_t next = 1; + + /* MUST chdir ("/") before leaving this function. */ + if (chdir (sysroot) == -1) {
If this function might ever be used from a multi-threaded application, then you'll want to change it not to use chdir, since chdir changes the process-wide current directory. That can cause rare but particularly hard to debug problems.
This code would be much harder to write if we don't use chdir. Any suggestions on that? If there was a "readdirat" variant then that might be promising.
To traverse without chdir, you can use the combination of openat and fdopendir, and then readdir as usual. With those gnulib modules, you use both openat and fdopendir portably.
On 26/10/09 09:20, Richard W.M. Jones wrote:
+char * +do_case_sensitive_path (const char *path) +{ + char ret[PATH_MAX+1] = "/";
Is PATH_MAX on Windows <= POSIX PATH_MAX? Does ntfs_3g honour this? Seems like a grey area. It would be safer to realloc this buffer as necessary. You also wouldn't need the strdup() at the end.
+ size_t next = 1; + + /* MUST chdir ("/") before leaving this function. */ + if (chdir (sysroot) == -1) { + reply_with_perror ("%s", sysroot); + return NULL; + }
I'm not convinced chdir is necessary in this function if you use openat() throughout.
+ /* First character is a '/'. Take each subsequent path element + * and follow it. + */
A check that *path == '/' wouldn't go amiss here. I'd stick an assert in, but then DV might shoot me ;)
+ path++; + while (*path) { + size_t i = strcspn (path, "/"); + if (i == 0) { /* "//" in path */ + path++; + continue; + } + if ((i == 1&& path[0] == '.') || + (i == 2&& path[0] == '.'&& path[1] == '.')) { + reply_with_error ("case_sensitive_path: path contained . or .. elements");
This string isn't internationalised. I won't mention all of these.
+ goto error; + } + if (i> NAME_MAX) { + reply_with_error ("case_sensitive_path: path element too long"); + goto error; + } + + char name[NAME_MAX+1]; + memcpy (name, path, i); + name[i] = '\0'; + + /* Skip to next element in path (for the next loop iteration). */ + path += i; + if (*path == '/') path++; + + /* Read the current directory looking (case insensitively) for + * this element of the path. + */ + DIR *dir = opendir ("."); + if (dir == NULL) { + reply_with_perror ("opendir"); + goto error; + } + + struct dirent *d = NULL; + + while ((d = readdir (dir)) != NULL) { + if (strcasecmp (d->d_name, name) == 0) + break; + } + + if (closedir (dir) == -1) { + reply_with_perror ("closedir"); + goto error; + } + + if (d == NULL) { + reply_with_error ("%s: no file or directory found with this name", name);
Non-internationalised string. This message is definitely for the user.
+ goto error; + } + + /* Add the real name of this path element to the return value. */ + if (next> 1) + ret[next++] = '/'; + + i = strlen (d->d_name); + if (next + i>= PATH_MAX) { + reply_with_error ("final path too long"); + goto error; + } + + strcpy (&ret[next], d->d_name); + next += i; + + /* Is it a directory? Try going into it. */ + if (chdir (d->d_name) == 0) + continue;
openat() and fstat() would be much nicer here.
+ + /* This is OK provided we've reached the end of the path. */ + if (errno == ENOTDIR) { + if (*path) { + reply_with_error ("non-directory element in path"); + goto error; + } + break; + } + } + + ignore_value (chdir ("/")); + + ret[next] = '\0'; + char *retp = strdup (ret); + if (retp == NULL) { + reply_with_perror ("strdup"); + return NULL; + } + return retp; /* caller frees */ + + error: + ignore_value (chdir ("/")); + return NULL; +} diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR index 0f11735..5381652 100644 --- a/src/MAX_PROC_NR +++ b/src/MAX_PROC_NR @@ -1 +1 @@ -196 +197 diff --git a/src/generator.ml b/src/generator.ml index cea5178..668002e 100755 --- a/src/generator.ml +++ b/src/generator.ml @@ -3645,6 +3645,45 @@ The result list is not sorted.
=back");
+ ("case_sensitive_path", (RString "rpath", [Pathname "path"]), 197, [], + [InitISOFS, Always, TestOutput ( + [["case_sensitive_path"; "/DIRECTORY"]], "/directory"); + InitISOFS, Always, TestOutput ( + [["case_sensitive_path"; "/Known-1"]], "/known-1"); + InitBasicFS, Always, TestOutput ( + [["mkdir"; "/a"]; + ["mkdir"; "/a/bbb"]; + ["touch"; "/a/bbb/c"]; + ["case_sensitive_path"; "/A/bbB/C"]], "/a/bbb/c")],
These don't exercise the following cases you've coded for: / /foo//bar /foo/../bar /foo///bar/
+ "return true path on case-insensitive filesystem", + "\ +This command handles a peculiarity of the Linux ntfs-3g +filesystem driver (and probably others), which is that although +the underlying filesystem is case-insensitive, the driver +exports the filesystem to Linux as case-sensitive. + +One consequence of this is that special directories such +as C<c:\\windows> may appear as C</WINDOWS> or C</windows> +(or other things) depending on the precise details of how +they were created. In Windows itself this would not be +a problem. + +Bug or feature? You decide: +L<http://www.tuxera.com/community/ntfs-3g-faq/#posixfilenames1>
It's definitely a bug ;)
+ +This function resolves the true case of each element in the +path and returns the case-sensitive path. + +Thus C<guestfs_case_sensitive_path> (\"/Windows/System32\") +might return C<\"/WINDOWS/system32\"> (the exact return value +would depend on details of how the directories were originally +created under Windows). + +I<Note>: +This function does not handle drive names, backslashes etc. + +See also C<guestfs_realpath>."); + ]
let all_functions = non_daemon_functions @ daemon_functions -- 1.6.5.rc2
This API seems like an unfortunate pimple. Would it not be better to *not* export this API, and instead call this automatically everywhere the daemon opens a file on ntfs? Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
On Mon, Oct 26, 2009 at 11:03:04AM +0000, Matthew Booth wrote:
On 26/10/09 09:20, Richard W.M. Jones wrote:
+char * +do_case_sensitive_path (const char *path) +{ + char ret[PATH_MAX+1] = "/";
Is PATH_MAX on Windows <= POSIX PATH_MAX? Does ntfs_3g honour this? Seems like a grey area. It would be safer to realloc this buffer as necessary. You also wouldn't need the strdup() at the end.
PATH_MAX is a Linux thing. NTFS 3g couldn't export something through the Linux VFS unless it honoured this.
+ size_t next = 1; + + /* MUST chdir ("/") before leaving this function. */ + if (chdir (sysroot) == -1) { + reply_with_perror ("%s", sysroot); + return NULL; + }
I'm not convinced chdir is necessary in this function if you use openat() throughout.
I'm pretty sure I need opendirat to make this work, and that function doesn't seem to exist (checked on Fedora 11).
+ /* First character is a '/'. Take each subsequent path element + * and follow it. + */
A check that *path == '/' wouldn't go amiss here. I'd stick an assert in, but then DV might shoot me ;)
This is already checked in the generated code.
+ reply_with_error ("case_sensitive_path: path contained . or .. elements");
This string isn't internationalised. I won't mention all of these.
We don't internationalize any string in the daemon (yet).
+ ("case_sensitive_path", (RString "rpath", [Pathname "path"]), 197, [], + [InitISOFS, Always, TestOutput ( + [["case_sensitive_path"; "/DIRECTORY"]], "/directory"); + InitISOFS, Always, TestOutput ( + [["case_sensitive_path"; "/Known-1"]], "/known-1"); + InitBasicFS, Always, TestOutput ( + [["mkdir"; "/a"]; + ["mkdir"; "/a/bbb"]; + ["touch"; "/a/bbb/c"]; + ["case_sensitive_path"; "/A/bbB/C"]], "/a/bbb/c")],
These don't exercise the following cases you've coded for:
/ /foo//bar /foo/../bar /foo///bar/
Agreed that more tests are needed here. I will add them in the next version.
This API seems like an unfortunate pimple. Would it not be better to *not* export this API, and instead call this automatically everywhere the daemon opens a file on ntfs?
But how does it know what the underlying filesystem is up to? I agree it's a bug in NTFS 3g, but changing all the code to do very expensive magic doesn't seem like the right way, and you can't detect which filesystems are broken like this. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
On 26/10/09 11:08, Richard W.M. Jones wrote:
+ size_t next = 1; + + /* MUST chdir ("/") before leaving this function. */ + if (chdir (sysroot) == -1) { + reply_with_perror ("%s", sysroot); + return NULL; + }
I'm not convinced chdir is necessary in this function if you use openat() throughout.
I'm pretty sure I need opendirat to make this work, and that function doesn't seem to exist (checked on Fedora 11).
You only need openat() followed by fdopendir().
This API seems like an unfortunate pimple. Would it not be better to *not* export this API, and instead call this automatically everywhere the daemon opens a file on ntfs?
But how does it know what the underlying filesystem is up to? I agree it's a bug in NTFS 3g, but changing all the code to do very expensive magic doesn't seem like the right way, and you can't detect which filesystems are broken like this.
Problem is, if you don't put the magic in you're pushing the problem on to the library user. The code's going to look pretty much the same every time, so it might as well be in the library. You could take 2 approaches here: 1. Always use case-sensitive-path: slow, but should be safe. 2. Detect fs type and call case-sensitive-path as required: probably tricky to get absolutely right (think: ntfs mounted as an additional volume under /mnt/windows). I'd go for 1. It should be safe, and it's not as if the slowness is likely to be noticeable here. Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
On Mon, Oct 26, 2009 at 11:18:26AM +0000, Matthew Booth wrote:
Problem is, if you don't put the magic in you're pushing the problem on to the library user.
This is not true. It's only used for the case where you are resolving paths given to you by Windows -- eg. coming from a Windows configuration file or from the Windows Registry. The case we use it is precisely this: we read the Windows %systemroot% from NTLDR and then have to resolve that against the filesystem. If Windows allowed its root filesystem to be on ext3 it would work the same way. In any other case, using case_sensitive_path is almost certainly wrong, in fact not just slow but quite likely to be insecure too. You don't want a Linux config file that specifies /etc/something to randomly pick /etc/SOMETHING. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
On 26/10/09 12:13, Richard W.M. Jones wrote:
On Mon, Oct 26, 2009 at 11:18:26AM +0000, Matthew Booth wrote:
Problem is, if you don't put the magic in you're pushing the problem on to the library user.
This is not true. It's only used for the case where you are resolving paths given to you by Windows -- eg. coming from a Windows configuration file or from the Windows Registry. The case we use it is precisely this: we read the Windows %systemroot% from NTLDR and then have to resolve that against the filesystem. If Windows allowed its root filesystem to be on ext3 it would work the same way.
.. or the guestfish user who types 'cat win:c:\windows\system32\blah.txt' and doesn't get the result they intended. Essentially you need to do this in 2 cases: you read a path from a config file, or you're accessing a file whose location you already know. In either case, the additional call is just untidiness.
In any other case, using case_sensitive_path is almost certainly wrong, in fact not just slow but quite likely to be insecure too. You don't want a Linux config file that specifies /etc/something to randomly pick /etc/SOMETHING.
It's a very good point. You can fix this, though. When iterating down the directory structure, you'll open a file to check if it's a directory. At the same time, to fstatfs() to check what filesystem it's on. By doing this you can choose directory-by-directory whether or not to be case-sensitive. This should always be safe. Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
On Mon, Oct 26, 2009 at 01:16:15PM +0000, Matthew Booth wrote:
.. or the guestfish user who types 'cat win:c:\windows\system32\blah.txt' and doesn't get the result they intended. Essentially you need to do this in 2 cases: you read a path from a config file, or you're accessing a file whose location you already know. In either case, the additional call is just untidiness.
I don't understand this point. Who told them to look at "c:\windows\system32\blah.txt"? It must have come from a Windows config file, a Windows document or a book, and that path is not case-sensitive.
In any other case, using case_sensitive_path is almost certainly wrong, in fact not just slow but quite likely to be insecure too. You don't want a Linux config file that specifies /etc/something to randomly pick /etc/SOMETHING.
It's a very good point. You can fix this, though. When iterating down the directory structure, you'll open a file to check if it's a directory. At the same time, to fstatfs() to check what filesystem it's on. By doing this you can choose directory-by-directory whether or not to be case-sensitive. This should always be safe.
fstatfs doesn't tell you if a filesystem is case sensitive, and in any case that is irrelevant. Think of the imaginary Windows-on-ext3 case where you'd still have Windows containing case-insensitive configuration paths, even if the filesystem is case-sensitive. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming blog: http://rwmj.wordpress.com Fedora now supports 80 OCaml packages (the OPEN alternative to F#) http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
On 26/10/09 13:25, Richard W.M. Jones wrote:
On Mon, Oct 26, 2009 at 01:16:15PM +0000, Matthew Booth wrote:
.. or the guestfish user who types 'cat win:c:\windows\system32\blah.txt' and doesn't get the result they intended. Essentially you need to do this in 2 cases: you read a path from a config file, or you're accessing a file whose location you already know. In either case, the additional call is just untidiness.
I don't understand this point.
Who told them to look at "c:\windows\system32\blah.txt"? It must have come from a Windows config file, a Windows document or a book, and that path is not case-sensitive.
In any other case, using case_sensitive_path is almost certainly wrong, in fact not just slow but quite likely to be insecure too. You don't want a Linux config file that specifies /etc/something to randomly pick /etc/SOMETHING.
It's a very good point. You can fix this, though. When iterating down the directory structure, you'll open a file to check if it's a directory. At the same time, to fstatfs() to check what filesystem it's on. By doing this you can choose directory-by-directory whether or not to be case-sensitive. This should always be safe.
fstatfs doesn't tell you if a filesystem is case sensitive, and in any case that is irrelevant. Think of the imaginary Windows-on-ext3 case where you'd still have Windows containing case-insensitive configuration paths, even if the filesystem is case-sensitive.
Bleargh. Let me start again on this one. So I have 2 real objections here: 1. It's *really* ugly 2. It can be done automatically So, let's say that some file on Windows has a well-defined location. Maybe it's some driver in system32 or something. The library user can't just use this location, they have to manually 'resolve' it first. It's the same case as getting a path from a config file, only less explicit. Either way, there's an ugly additional step required. It's also an entirely guest OS specific api addition. I know we have others (SELinux, for eg), but that's no excuse to add more without trying very hard not to. The alternative here, is to do 2 things: 1. Change the implementation of every existing API call which takes a Guest path to call the new path resolution function first. 2. Fix up the path resolution function so this is safe to do. The first shouldn't be too hard because it's all auto-generated. The second should be hard to do either, because it's a relatively small change to your existing patch to make it check the filesystem type first, before deciding whether to be case-sensitive or case-insensitive. The result is that reading a file out of a windows config file and then accessing it will Just Work, as will accessing a file with a well-defined path on Windows. The library user doesn't need to do anything extra, and there is no addition to the API. Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
On Mon, Oct 26, 2009 at 01:54:23PM +0000, Matthew Booth wrote:
The alternative here, is to do 2 things:
1. Change the implementation of every existing API call which takes a Guest path to call the new path resolution function first. 2. Fix up the path resolution function so this is safe to do.
The first shouldn't be too hard because it's all auto-generated. The second should[n't] be hard to do either, because it's a relatively small change to your existing patch to make it check the filesystem type first, before deciding whether to be case-sensitive or case-insensitive.
The result is that reading a file out of a windows config file and then accessing it will Just Work, as will accessing a file with a well-defined path on Windows. The library user doesn't need to do anything extra, and there is no addition to the API.
OK I now understand what you mean. I think that such a large change to the API is very risky, and I think a lot harder than you imagine (not all the path parsing code is autogenerated, unfortunately[1], and also it's not clear what filesystems are case-{in,}sensitive). And in order to support something which is only used in one very narrow case: getting a string from a Windows configuration file. That's why it's better to confine this change to a single API function that people who do that can use, and everyone else can ignore. Rich. [1] Look at recursive_mkdir for example, or mkdtemp, or the functions that can take either a path or a device. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
Richard W.M. Jones wrote:
On Mon, Oct 26, 2009 at 11:03:04AM +0000, Matthew Booth wrote:
On 26/10/09 09:20, Richard W.M. Jones wrote:
+char * +do_case_sensitive_path (const char *path) +{ + char ret[PATH_MAX+1] = "/";
Is PATH_MAX on Windows <= POSIX PATH_MAX? Does ntfs_3g honour this? Seems like a grey area. It would be safer to realloc this buffer as necessary. You also wouldn't need the strdup() at the end.
PATH_MAX is a Linux thing. NTFS 3g couldn't export something through the Linux VFS unless it honoured this.
+ size_t next = 1; + + /* MUST chdir ("/") before leaving this function. */ + if (chdir (sysroot) == -1) { + reply_with_perror ("%s", sysroot); + return NULL; + }
I'm not convinced chdir is necessary in this function if you use openat() throughout.
I'm pretty sure I need opendirat to make this work, and that function doesn't seem to exist (checked on Fedora 11).
It's in lib/fts.c: /* file-descriptor-relative opendir. */ /* FIXME: if others need this function, move it into lib/openat.c */ static inline DIR * internal_function opendirat (int fd, char const *dir) { int new_fd = openat (fd, dir, O_RDONLY | O_DIRECTORY | O_NOCTTY | O_NONBLOCK); DIR *dirp; if (new_fd < 0) return NULL; set_cloexec_flag (new_fd, true); dirp = fdopendir (new_fd); if (dirp == NULL) { int saved_errno = errno; close (new_fd); errno = saved_errno; } return dirp; }
Matthew Booth wrote:
On 26/10/09 09:20, Richard W.M. Jones wrote:
+char * +do_case_sensitive_path (const char *path) +{ + char ret[PATH_MAX+1] = "/";
Is PATH_MAX on Windows <= POSIX PATH_MAX? Does ntfs_3g honour this? Seems like a grey area. It would be safer to realloc this buffer as necessary. You also wouldn't need the strdup() at the end.
Good point. There's another reason to avoid using PATH_MAX: some systems don't define it at all, and others define it to an outrageously (for stack use) large value.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
Richard W.M. Jones wrote:
Subject: [PATCH 2/3] guestfish: Add win: prefix to use Windows paths.
Add a win: prefix for path arguments in guestfish:
A prefix of "lose:" would be more apt ;-) Did you consider simply letting a "drive_letter:" prefix serve this purpose? Given the "win:" choice, the patch looks fine.
On Mon, Oct 26, 2009 at 11:16:57AM +0100, Jim Meyering wrote:
Did you consider simply letting a "drive_letter:" prefix serve this purpose?
There's a subtle point here which means we need a real prefix. Ordinary paths should all be prefixed by "/". It seems we could say that paths prefixed by "\" or "C:" would be treated as Windows paths, although I don't think I like the implicitness going on. But in any case this doesn't work. We might have some mixed path like: "/windows\system". "win:/windows\system" works (and "/windows\system" also works under Windows). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://et.redhat.com/~rjones/virt-df/
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming blog: http://rwmj.wordpress.com Fedora now supports 80 OCaml packages (the OPEN alternative to F#) http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
Umm ... they're not quite the same because the error case is different. Try this updated patch. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
Richard W.M. Jones wrote: ...
diff --git a/perl/lib/Sys/Guestfs/Lib.pm b/perl/lib/Sys/Guestfs/Lib.pm index 8ea2c1b..f7aaf07 100644 --- a/perl/lib/Sys/Guestfs/Lib.pm +++ b/perl/lib/Sys/Guestfs/Lib.pm @@ -247,6 +247,9 @@ sub _is_pv {
=head2 resolve_windows_path
+I<This function is deprecated>. +Use C<$g-E<gt>case_sensitive_path> as a direct substitute. + $path = resolve_windows_path ($g, $path);
$path = resolve_windows_path ($g, "/windows/system"); @@ -267,38 +270,10 @@ by C</> characters. Do not use C<\>, drive names, etc.
Hi Rich, This looks fine.
sub resolve_windows_path { - local $_; my $g = shift; my $path = shift;
If you're interested in stylistic suggestions, you might prefer the one-line style to repeated uses of "shift". Saving N-1 lines per function is a Good Thing: my ($g, $path) = @_; Also, a general guideline I've found useful is to say "file" or "filename" in place of "path" (as long as we're not talking about shell search paths, a la $PATH) with the implication that "filename" is general enough to encompass names of things like directories and non-regular files. POSIX does this, and it makes it a lot easier to write about "file system objects" as "files" without having to say "regular files or directories or special devices", etc. i.e., my ($g, $filename) = @_;
- if (substr ($path, 0, 1) ne "/") { - warn __"resolve_windows_path: path must start with a / character"; - return undef; - } ...
participants (3)
-
Jim Meyering -
Matthew Booth -
Richard W.M. Jones