Fix Cygwin path handling
PATCH 1/1 Fix Cygwin path handling
The Cywin path handling is broken since make_system_path_relative_to_its_root() functionality was moved from the lib script to misc.c.
This patch should fix this. It reuses the Cygwin specific code from getroot.c:grub_get_prefix() which apparently is a different implementation of the same function.
I would suggest to remove grub_get_prefix(), it is now only used in grub-emu.c and sparc64/ieee1275/grub-setup.c. Not included in the patch, should be done in a separate commit.
2010-04-14 Christian Franke<[email protected]>
* util/grub-mkconfig_lib.in (make_system_path_relative_to_its_root):
Remove broken Cygwin path conversion.
* util/misc.c: [__CYGWIN__] Add include and define. [__CYGWIN__] (get_win32_path): Copy function from getroot.c, modify for Cygwin 1.7.
(make_system_path_relative_to_its_root): Simplify loop, replace early return by break.
[__CYGWIN__] Add conversion to win32 path.
Signature · 2 lines
Regards, Christian Franke
=== modified file 'util/grub-mkconfig_lib.in'
grub-mkconfig_lib.in +2 -16
@@ -1,5 +1,5 @@ # Helper library for grub-mkconfig -# Copyright (C) 2007,2008,2009 Free Software Foundation, Inc. +# Copyright (C) 2007,2008,2009,2010 Free Software Foundation, Inc. # # GRUB is free software: you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by
@@ -38,21 +38,7 @@ make_system_path_relative_to_its_root () { - path="`${grub_mkrelpath} $1`" - - case "`uname 2>/dev/null`" in - CYGWIN*) - # Cygwin: Check if regular or emulated mount. - if [ -z "$dir" ] || [ "`stat -c %D "$dir/.."`" != 620000 ] ; then - # Reached some mount point not below /cygdrive. - # GRUB does not know Cygwin's emulated mounts, - # convert to Win32 path and remove drive letter. - path=`cygpath -m "$path" | sed -n 's,^[A-Za-z]:,,p'` - test ! -z "$path" || return 1 - fi ;; - esac - - echo "$path" + ${grub_mkrelpath} $1 } is_path_readable_by_grub ()
=== modified file 'util/misc.c'
misc.c +46 -16
86 lines not on this page · raw message
Quoted 13 lines · Christian Franke
The Cywin path handling is broken since
make_system_path_relative_to_its_root()functionality was moved from the lib script tomisc.c.This patch should fix this. It reuses the Cygwin specific code from getroot.c:grub_get_prefix() which apparently is a different implementation of the same function.
I would suggest to remove
grub_get_prefix(), it is now only used ingrub-emu.candsparc64/ieee1275/grub-setup.c. Not included in the patch, should be done in a separate commit.
Quoted 7 lines
2010-04-14 Christian Franke<[email protected]>
* util/grub-mkconfig_lib.in (
make_system_path_relative_to_its_root):
Remove broken Cygwin path conversion.
*util/misc.c: [__CYGWIN__] Add include and define. [__CYGWIN__] (get_win32_path): Copy function fromgetroot.c, modify for Cygwin 1.7.
Please avoid duplicating code. Rather than that rename get_win32_path to grub_get_win32_path and remove static attribute
Quoted 12 lines
(
make_system_path_relative_to_its_root): Simplify loop, replace early return by break.
[__CYGWIN__] Add conversion to win32 path.------------------------------------------------------------------------
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Signature · 2 lines
Regards Vladimir 'φ-coder/phcoder' Serbinenko
Quoted 17 lines · Vladimir 'φ-coder/phcoder' Serbinenko
Quoted 15 lines · Christian Franke
The Cywin path handling is broken since
make_system_path_relative_to_its_root()functionality was moved from the lib script tomisc.c.This patch should fix this. It reuses the Cygwin specific code from getroot.c:grub_get_prefix() which apparently is a different implementation of the same function.
I would suggest to remove
grub_get_prefix(), it is now only used ingrub-emu.candsparc64/ieee1275/grub-setup.c. Not included in the patch, should be done in a separate commit.2010-04-14 Christian Franke<[email protected]>
Quoted 9 lines
Quoted 6 lines
* util/grub-mkconfig_lib.in (
make_system_path_relative_to_its_root):
Remove broken Cygwin path conversion.
*util/misc.c: [__CYGWIN__] Add include and define. [__CYGWIN__] (get_win32_path): Copy function fromgetroot.c, modify for Cygwin 1.7.Please avoid duplicating code. Rather than that rename
get_win32_pathtogrub_get_win32_pathand remove static attribute
Normally I would have done that but duplication was intentional in this case:
The getroot.c:get_win32_path() can later be removed together with grub_get_prefix(), see my suggestion above. The patch takes this into account and adds new private misc.c:get_win32_path() and so avoids unnecessary temporary changes to misc.h and getroot.c.
The actual code duplication happened when misc.c:make_system_path_relative_to_its_root() was added instead of moving and reusing getroot.c:grub_get_prefix() :-)
BTW: My last commits to grub codebase were before the move to bzr.
As far as I understand "Bazaar workflow for GRUB" (http://lists.gnu.org/archive/html/grub-devel/2010-01/msg00175.html) such changes should be 'bzr push'ed to e.g. '.../branches/feature-foo' (e.g. '.../branches/cygwin-path' in this case) after review has finished.
Is this workflow still valid or is there a more current document?
Signature · 2 lines
Regards, Christian Franke
Quoted 37 lines · Christian Franke
Quoted 25 lines · Vladimir 'φ-coder/phcoder' Serbinenko
Quoted 15 lines · Christian Franke
The Cywin path handling is broken since
make_system_path_relative_to_its_root()functionality was moved from the lib script tomisc.c.This patch should fix this. It reuses the Cygwin specific code from getroot.c:grub_get_prefix() which apparently is a different implementation of the same function.
I would suggest to remove
grub_get_prefix(), it is now only used ingrub-emu.candsparc64/ieee1275/grub-setup.c. Not included in the patch, should be done in a separate commit.2010-04-14 Christian Franke<[email protected]>
Quoted 8 lines
* util/grub-mkconfig_lib.in
(make_system_path_relative_to_its_root):
Remove broken Cygwin path conversion.
*util/misc.c: [__CYGWIN__] Add include and define. [__CYGWIN__] (get_win32_path): Copy function fromgetroot.c,
modifyfor Cygwin 1.7.Quoted 3 lines
Please avoid duplicating code. Rather than that rename
get_win32_pathtogrub_get_win32_pathand remove static attributeNormally I would have done that but duplication was intentional in this case:
The getroot.c:get_win32_path() can later be removed together withgrub_get_prefix(), see my suggestion above. The patch takes this into account and adds new private misc.c:get_win32_path() and so avoids unnecessary temporary changes tomisc.handgetroot.c.
Quoted 4 lines
The actual code duplication happened when misc.c:make_system_path_relative_to_its_root() was added instead of moving and reusing getroot.c:grub_get_prefix() :-)
Ok. Can you supply the dedup patch? (perhaps it should come before the fix).
Quoted 8 lines
BTW: My last commits to grub codebase were before the move to bzr.
As far as I understand "Bazaar workflow for GRUB"
(http://lists.gnu.org/archive/html/grub-devel/2010-01/msg00175.html)
such changes should be 'bzr push'ed to e.g. '.../branches/feature-foo' (e.g. '.../branches/cygwin-path' in this case) after review has finished.
Creating new branches doesn't need any approval at all. If the changes are approved for trunk they are applied or merged into trunk. experimental branch is a merge of sufficiently functional branches but which need more testing for testers convenience. Merging into it follows similar rules as comitting to trunk.
Quoted 2 lines
Is this workflow still valid or is there a more current document?
Signature · 2 lines
Regards Vladimir 'φ-coder/phcoder' Serbinenko
Quoted 8 lines · Christian Franke
As far as I understand "Bazaar workflow for GRUB"
(http://lists.gnu.org/archive/html/grub-devel/2010-01/msg00175.html) such changes should be 'bzr push'ed to e.g. '.../branches/feature-foo' (e.g. '.../branches/cygwin-path' in this case) after review has finished.Is this workflow still valid or is there a more current document?
That is the only workflow document we have. I had to add couple of more sections to it, but I haven't done that yet :-(
We are not strictly following that document :-) We are deciding -- informally over IRC -- whether a patch can go to trunk (mainline) or to experimental case-by-case basis, based on the fix/feature severity.
Signature · 1 line
bvk.chaitanya
Quoted 15 lines · Christian Franke
The Cywin path handling is broken since
make_system_path_relative_to_its_root()functionality was moved from the lib script tomisc.c.This patch should fix this. It reuses the Cygwin specific code from getroot.c:grub_get_prefix() which apparently is a different implementation of the same function.
I would suggest to remove
grub_get_prefix(), it is now only used ingrub-emu.candsparc64/ieee1275/grub-setup.c. Not included in the patch, should be done in a separate commit.2010-04-14 Christian Franke<[email protected]>
Quoted 9 lines
* util/grub-mkconfig_lib.in (
make_system_path_relative_to_its_root):
Remove broken Cygwin path conversion.
*util/misc.c: [__CYGWIN__] Add include and define. [__CYGWIN__] (get_win32_path): Copy function fromgetroot.c, modify for Cygwin 1.7.
(make_system_path_relative_to_its_root): Simplify loop, replace early return by break.
[__CYGWIN__] Add conversion to win32 path.
+ /* Include leading slash. */ + offset = 0; + break;
Why do you make this change? It seems to make the behaviour of make_path_relative_to_its root inconsistent since E.g./boot/grub -> grub
/etc -> /etc
Can we decide to either always include this slash or never to? I prefer never to include it since it's the way it is now and I prefer to keep it to avoid possible breakage.
Quoted 8 lines
------------------------------------------------------------------------
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Signature · 2 lines
Regards Vladimir 'φ-coder/phcoder' Serbinenko
Quoted 10 lines · Vladimir 'φ-coder/phcoder' Serbinenko
Quoted 1 line · Christian Franke
misc.c +3
-0+/* Include leading slash. */ + offset =0 ; +break ;Why do you make this change? It seems to make the behaviour of
make_path_relative_to_itsroot inconsistent since E.g./boot/grub-> grub
/etc -> /etc
Could not reproduce. Old and new code always include the leading slash (except "/" -> ''").
Signature · 2 lines
Regards, Christian Franke
Quoted 13 lines · Christian Franke
Quoted 11 lines · Vladimir 'φ-coder/phcoder' Serbinenko
Quoted 1 line · Christian Franke
misc.c +3
-0+/* Include leading slash. */ + offset =0 ; +break ;Why do you make this change? It seems to make the behaviour of
make_path_relative_to_itsroot inconsistent since E.g./boot/grub-> grub
/etc -> /etc
Quoted 3 lines
Could not reproduce. Old and new code always include the leading slash (except "/" -> ''").
I actually seen the '/' case and have incorrectly assumed it's always so. However there is still one case when you patch changes the behaviour:
$ grub-mkrelpath /
$ ./grub-mkrelpath / / Although: $ ./grub-mkrelpath /mnt/boot/
$ grub-mkrelpath /mnt/boot/
A simple filter at the end of make_path_relative_to_its_root is enough. Just it's better to avoid both inconsistency between mountpoint and root and behaviour change
Signature · 2 lines
Regards Vladimir 'φ-coder/phcoder' Serbinenko
Quoted 14 lines · Vladimir 'φ-coder/phcoder' Serbinenko
I actually seen the '/' case and have incorrectly assumed it's always so. However there is still one case when you patch changes the behaviour:
$ grub-mkrelpath /$ ./grub-mkrelpath / / Although: $ ./grub-mkrelpath /mnt/boot/$ grub-mkrelpath /mnt/boot/
I actually did not test the "/" corner case, sorry. New patch attached.
Signature · 2 lines
Regards, Christian Franke
=== modified file 'ChangeLog'
ChangeLog +12 -0
@@ -1,5 +1,17 @@ 2010-04-26 Christian Franke <[email protected]> + * util/grub-mkconfig_lib.in (make_system_path_relative_to_its_root): + Remove broken Cygwin path conversion. + * util/misc.c: [__CYGWIN__] Add include and define. + [__CYGWIN__] (get_win32_path): Copy function from getroot.c, modify + for Cygwin 1.7. + (make_system_path_relative_to_its_root): Simplify loop, replace early + return by break. + [__CYGWIN__] Add conversion to win32 path. + Include "/" case in trailing slash removal. + +2010-04-26 Christian Franke <[email protected]> + * include/grub/util/getroot.h (grub_get_prefix): Remove prototype. * util/getroot.c [__CYGWIN__] (get_win32_path): Remove function. (grub_get_prefix): Remove function.
=== modified file 'util/grub-mkconfig_lib.in'
grub-mkconfig_lib.in +2 -16
@@ -1,5 +1,5 @@ # Helper library for grub-mkconfig -# Copyright (C) 2007,2008,2009 Free Software Foundation, Inc. +# Copyright (C) 2007,2008,2009,2010 Free Software Foundation, Inc. # # GRUB is free software: you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by
@@ -44,21 +44,7 @@ make_system_path_relative_to_its_root () { - path="`${grub_mkrelpath} $1`" - - case "`uname 2>/dev/null`" in - CYGWIN*) - # Cygwin: Check if regular or emulated mount. - if [ -z "$dir" ] || [ "`stat -c %D "$dir/.."`" != 620000 ] ; then - # Reached some mount point not below /cygdrive. - # GRUB does not know Cygwin's emulated mounts, - # convert to Win32 path and remove drive letter. - path=`cygpath -m "$path" | sed -n 's,^[A-Za-z]:,,p'` - test ! -z "$path" || return 1 - fi ;; - esac - - echo "$path" + ${grub_mkrelpath} $1 } is_path_readable_by_grub ()
=== modified file 'util/misc.c'
misc.c +44 -17
86 lines not on this page · raw message
Committed
Quoted 25 lines · Christian Franke
Quoted 4 lines · Vladimir 'φ-coder/phcoder' Serbinenko
I actually seen the '/' case and have incorrectly assumed it's always so. However there is still one case when you patch changes the behaviour:
Quoted 11 lines
$ grub-mkrelpath /$ ./grub-mkrelpath / / Although: $ ./grub-mkrelpath /mnt/boot/$ grub-mkrelpath /mnt/boot/I actually did not test the "/" corner case, sorry. New patch attached.
------------------------------------------------------------------------
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Signature · 2 lines
Regards Vladimir 'φ-coder/phcoder' Serbinenko