grub-install --root-directory=/mnt /dev/sda1 fails

grub-install --root-directory=/mnt /dev/sda1 fails

[Felix Zielcke]

Hello list,

grub-install with some --root-directory option and a partition given as
setup device and not a whole disk doestn't work, i.e. blocklists are
used.
grub-setup fails that it can't find the core.img.

The problem are these 2 lines in setup():
core_path_dev = grub_util_get_path (dir, core_file);
...
file = grub_file_open (core_path_dev);

core_path_dev contains the full path given with the `-d' or
`--directory' option for example /mnt/boot/grub.

This can't work because /mnt is a different filesystem and so GRUB only
sees /boot/grub.
Removing the /mnt part from it works fine:
(gdb) set core_path_dev="/boot/grub/core.img"
(gdb) print *file->device->disk 
$13 = {name = 0x640b90 "hd1,1", dev = 0x62f520, total_sectors = 16777216, has_partitions = 1, id = 1, partition = 0x640c10, read_hook = 0, data = 0x0}

The only solution that comes to my mind for this problem is, to
introduce a `--root-directory' option and making the `--directory'
option relative to it.

Does anyone have a better idea?
-- 
Felix Zielcke

Re: grub-install --root-directory=/mnt /dev/sda1 fails

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1459 bytes --]

On Wed, May 6, 2009 at 4:31 PM, Felix Zielcke <fzielcke@z-51.de> wrote:

> Hello list,
>
> grub-install with some --root-directory option and a partition given as
> setup device and not a whole disk doestn't work, i.e. blocklists are
> used.
> grub-setup fails that it can't find the core.img.
>
> The problem are these 2 lines in setup():
> core_path_dev = grub_util_get_path (dir, core_file);
> ...
> file = grub_file_open (core_path_dev);
>
> core_path_dev contains the full path given with the `-d' or
> `--directory' option for example /mnt/boot/grub.
>
> This can't work because /mnt is a different filesystem and so GRUB only
> sees /boot/grub.
> Removing the /mnt part from it works fine:
> (gdb) set core_path_dev="/boot/grub/core.img"
> (gdb) print *file->device->disk
> $13 = {name = 0x640b90 "hd1,1", dev = 0x62f520, total_sectors = 16777216,
> has_partitions = 1, id = 1, partition = 0x640c10, read_hook = 0, data = 0x0}
>
> The only solution that comes to my mind for this problem is, to
> introduce a `--root-directory' option and making the `--directory'
> option relative to it.
>
> Does anyone have a better idea?

Don't we already have a function which transforms host directory into grub
directory? AFAIR we have.

>
> --
> Felix Zielcke
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: Type: text/html, Size: 2197 bytes --]

Re: grub-install --root-directory=/mnt /dev/sda1 fails

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

Am Mittwoch, den 06.05.2009, 17:12 +0200 schrieb Vladimir 'phcoder'
Serbinenko:
> Don't we already have a function which transforms host directory into
> grub
> directory? AFAIR we have.

There's just the shell function in grub-mkconfig_lib.in
Here's now a patch wich implements it in util/hostdisk.c and gets used
for core_path_dev in setup ().
But it doestn't work with symlinks.
readlink () can only be used if the file pointed to is a symlink, not if
a symlink is somewhere in between.
coreutils where the readlink binary is from is GPL 3+ but the function
for it uses hash tables and it seems like it would be too much code to
copy just for this.
-- 
Felix Zielcke

[-- Attachment #2: make_sys_path_relative.patch --]
[-- Type: text/x-patch, Size: 2244 bytes --]

2009-06-01  Felix Zielcke  <fzielcke@z-51.de>

	    * include/grub/util/hostdisk.c
	    (grub_make_system_path_relative_to_its_root): New function
	    prototype.
	    * util/hostdisk.c (grub_make_system_path_relative_to_its_root): 
	    New function.
	    * util/i386/pc/grub-setup.c (setup): Use 
	    grub_make_system_path_relative_to_its_root to make core_path_dev
	    relative to the partition.

diff --git a/include/grub/util/hostdisk.h b/include/grub/util/hostdisk.h
index 21efb0d..0fb0219 100644
--- a/include/grub/util/hostdisk.h
+++ b/include/grub/util/hostdisk.h
@@ -23,5 +23,7 @@
 void grub_util_biosdisk_init (const char *dev_map);
 void grub_util_biosdisk_fini (void);
 char *grub_util_biosdisk_get_grub_dev (const char *os_dev);
+char *grub_make_system_path_relative_to_its_root (char *path);
+
 
 #endif /* ! GRUB_BIOSDISK_MACHINE_UTIL_HEADER */
diff --git a/util/hostdisk.c b/util/hostdisk.c
index eaccb73..2b80b48 100644
--- a/util/hostdisk.c
+++ b/util/hostdisk.c
@@ -1072,3 +1072,39 @@ grub_util_biosdisk_get_grub_dev (const char *os_dev)
   return make_device_name (drive, -1, -1);
 #endif
 }
+
+char *grub_make_system_path_relative_to_its_root (char *path)
+{
+
+  struct stat st;
+  char buf[500], buf2[500];
+  dev_t num;
+  char *p;
+
+  if (stat (path, &st) < 0)
+    return path;
+
+  num = st.st_dev;
+  memset (buf, 0 , sizeof (buf));
+  strncpy (buf, path, 500);
+  strcpy (buf2, buf);
+  while (1)
+    {
+      p = strrchr (buf, '/');
+      if (p != buf)
+	*p = 0;
+      else *++p = 0;
+
+      if (stat (buf, &st) < 0)
+	return path;
+
+      if (st.st_dev != num)
+	break;
+      strcpy(buf2,buf);
+      if (p - 1 == buf)
+	return path;
+    }
+  for (p = buf2; *p != 0; p++)
+    path++;
+  return path;
+}
diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
index 997811b..57f49d4 100644
--- a/util/i386/pc/grub-setup.c
+++ b/util/i386/pc/grub-setup.c
@@ -405,6 +405,7 @@ unable_to_embed:
   /* Make sure that GRUB reads the identical image as the OS.  */
   tmp_img = xmalloc (core_size);
   core_path_dev = grub_util_get_path (dir, core_file);
+  core_path_dev = grub_make_system_path_relative_to_its_root (core_path_dev);
   
   /* It is a Good Thing to sync two times.  */
   sync ();

[PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 826 bytes --]

Am Montag, den 01.06.2009, 21:39 +0200 schrieb Felix Zielcke:
> Am Mittwoch, den 06.05.2009, 17:12 +0200 schrieb Vladimir 'phcoder'
> Serbinenko:
> > Don't we already have a function which transforms host directory into
> > grub
> > directory? AFAIR we have.
> 
> There's just the shell function in grub-mkconfig_lib.in
> Here's now a patch wich implements it in util/hostdisk.c and gets used
> for core_path_dev in setup ().
> But it doestn't work with symlinks.
> readlink () can only be used if the file pointed to is a symlink, not if
> a symlink is somewhere in between.
> coreutils where the readlink binary is from is GPL 3+ but the function
> for it uses hash tables and it seems like it would be too much code to
> copy just for this.

Here's a new patch which prints out an error if stat () fails.
-- 
Felix Zielcke

[-- Attachment #2: make_sys_path_relative.patch.2 --]
[-- Type: text/plain, Size: 1890 bytes --]

2009-06-08  Felix Zielcke  <fzielcke@z-51.de>

	    * include/grub/util/hostdisk.c
	    (grub_make_system_path_relative_to_its_root): New function
	    prototype.
	    * util/hostdisk.c (grub_make_system_path_relative_to_its_root):
	    New function.
	    * util/i386/pc/grub-setup.c (setup): Use
	    grub_make_system_path_relative_to_its_root to make core_path_dev
	    relative to the partition.

diff --git a/util/hostdisk.c b/util/hostdisk.c
index a7262dd..0a786ba 100644
--- a/util/hostdisk.c
+++ b/util/hostdisk.c
@@ -1076,3 +1076,39 @@ grub_util_biosdisk_get_grub_dev (const char *os_dev)
   return make_device_name (drive, -1, -1);
 #endif
 }
+
+char *grub_make_system_path_relative_to_its_root (char *path)
+{
+
+  struct stat st;
+  char buf[500], buf2[500];
+  dev_t num;
+  char *p;
+
+  if (stat (path, &st) < 0)
+    return NULL;
+
+  num = st.st_dev;
+  memset (buf, 0 , sizeof (buf));
+  strncpy (buf, path, 500);
+  strcpy (buf2, buf);
+  while (1)
+    {
+      p = strrchr (buf, '/');
+      if (p != buf)
+	*p = 0;
+      else *++p = 0;
+
+      if (stat (buf, &st) < 0)
+	return NULL;
+
+      if (st.st_dev != num)
+	break;
+      strcpy(buf2,buf);
+      if (p - 1 == buf)
+	return path;
+    }
+  for (p = buf2; *p != 0; p++)
+    path++;
+  return path;
+}
diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
index 997811b..9446fd5 100644
--- a/util/i386/pc/grub-setup.c
+++ b/util/i386/pc/grub-setup.c
@@ -405,6 +405,9 @@ unable_to_embed:
   /* Make sure that GRUB reads the identical image as the OS.  */
   tmp_img = xmalloc (core_size);
   core_path_dev = grub_util_get_path (dir, core_file);
+  core_path_dev = grub_make_system_path_relative_to_its_root (core_path_dev);
+  if (core_path_dev == NULL)
+    grub_util_error ("failed to make path of core.img relative to its root");
   
   /* It is a Good Thing to sync two times.  */
   sync ();

Re: [PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails

[Vladimir 'phcoder' Serbinenko]

2009-06-08  Felix Zielcke  <fzielcke@z-51.de>

	    * include/grub/util/hostdisk.c
	    (grub_make_system_path_relative_to_its_root): New function
	    prototype.
	    * util/hostdisk.c (grub_make_system_path_relative_to_its_root):
	    New function.
	    * util/i386/pc/grub-setup.c (setup): Use
	    grub_make_system_path_relative_to_its_root to make core_path_dev
	    relative to the partition.

diff --git a/util/hostdisk.c b/util/hostdisk.c
index a7262dd..0a786ba 100644
--- a/util/hostdisk.c
+++ b/util/hostdisk.c
@@ -1076,3 +1076,39 @@ grub_util_biosdisk_get_grub_dev (const char *os_dev)
   return make_device_name (drive, -1, -1);
 #endif
 }
+
+char *grub_make_system_path_relative_to_its_root (char *path)
+{
+
+  struct stat st;
+  char buf[500], buf2[500];
Use malloc instead of static allocation
+      p = strrchr (buf, '/');
+      if (p != buf)
+	*p = 0;
+      else *++p = 0;
You assume path starts with /. You have to check this. Otherwise you
may get sigsegv
+      strcpy(buf2,buf);
Just save (p - buf) instead of copying buf to buf2

On Mon, Jun 8, 2009 at 11:20 PM, Felix Zielcke<fzielcke@z-51.de> wrote:
> Am Montag, den 01.06.2009, 21:39 +0200 schrieb Felix Zielcke:
>> Am Mittwoch, den 06.05.2009, 17:12 +0200 schrieb Vladimir 'phcoder'
>> Serbinenko:
>> > Don't we already have a function which transforms host directory into
>> > grub
>> > directory? AFAIR we have.
>>
>> There's just the shell function in grub-mkconfig_lib.in
>> Here's now a patch wich implements it in util/hostdisk.c and gets used
>> for core_path_dev in setup ().
>> But it doestn't work with symlinks.
>> readlink () can only be used if the file pointed to is a symlink, not if
>> a symlink is somewhere in between.
>> coreutils where the readlink binary is from is GPL 3+ but the function
>> for it uses hash tables and it seems like it would be too much code to
>> copy just for this.
>
> Here's a new patch which prints out an error if stat () fails.
> --
> Felix Zielcke
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: [PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 1157 bytes --]

Am Dienstag, den 09.06.2009, 23:51 +0200 schrieb Vladimir 'phcoder'
Serbinenko:

> +
> +char *grub_make_system_path_relative_to_its_root (char *path)
> +{
> +
> +  struct stat st;
> +  char buf[500], buf2[500];
> Use malloc instead of static allocation

Changed.

> +      p = strrchr (buf, '/');
> +      if (p != buf)
> +	*p = 0;
> +      else *++p = 0;
> You assume path starts with /. You have to check this. Otherwise you
> may get sigsegv

Changed.
> +      strcpy(buf2,buf);
> Just save (p - buf) instead of copying buf to buf2

I did this now if realpath () isn't avaible.
Now realpath is used in case it's avaible. POSIX specifies it and
readlink is actually using it if it's avaible. I didn't read the source
correctly.
The problem is just Cygwin. It has it but it returns the cygwin path.
So C:\\Windows would get /cygdrive/c/windows, which is easy to handle.
But realpath ("/boot/grub") would return /boot/grub which isn't true
from Windows/GRUB point of view.
Maybe Christian and Bean can say something about it.
MingW doestn't have it and I don't know what Windows would have, so the
MingW users would still use my own way.

-- 
Felix Zielcke

[-- Attachment #2: make_sys_path_relative.patch.3 --]
[-- Type: text/plain, Size: 4504 bytes --]

2009-06-11  Felix Zielcke  <fzielcke@z-51.de>

	* configure.ac (AC_CHECK_FUNCS): Add realpath.
	* include/grub/util/hostdisk.c
	(grub_make_system_path_relative_to_its_root): New function
	prototype.
	* util/hostdisk.c: Include <stdint.h>.
	(grub_make_system_path_relative_to_its_root): New function.
	* util/i386/pc/grub-setup.c (setup): Use
	grub_make_system_path_relative_to_its_root to make core_path_dev
	relative to the partition.

diff --git a/configure.ac b/configure.ac
index e448c2f..9e923a6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -205,7 +205,7 @@ if test "$target_cpu"-"$platform" = i386-pc; then
 fi
 
 # Check for functions.
-AC_CHECK_FUNCS(posix_memalign memalign asprintf)
+AC_CHECK_FUNCS(posix_memalign memalign asprintf realpath)
 
 #
 # Check for target programs.
diff --git a/include/grub/util/hostdisk.h b/include/grub/util/hostdisk.h
index 21efb0d..a1ecf59 100644
--- a/include/grub/util/hostdisk.h
+++ b/include/grub/util/hostdisk.h
@@ -23,5 +23,6 @@
 void grub_util_biosdisk_init (const char *dev_map);
 void grub_util_biosdisk_fini (void);
 char *grub_util_biosdisk_get_grub_dev (const char *os_dev);
+char *grub_make_system_path_relative_to_its_root (char *path)
 
 #endif /* ! GRUB_BIOSDISK_MACHINE_UTIL_HEADER */
diff --git a/util/hostdisk.c b/util/hostdisk.c
index a7262dd..bce1b95 100644
--- a/util/hostdisk.c
+++ b/util/hostdisk.c
@@ -27,6 +27,7 @@
 #include <grub/misc.h>
 
 #include <stdio.h>
+#include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
 #include <ctype.h>
@@ -1076,3 +1077,94 @@ grub_util_biosdisk_get_grub_dev (const char *os_dev)
   return make_device_name (drive, -1, -1);
 #endif
 }
+
+char *
+grub_make_system_path_relative_to_its_root (char *path)
+{
+  struct stat st;
+  char *buf, *buf2;
+  uintptr_t offset = 0;
+  dev_t num;
+  long path_max;
+#ifdef HAVE_REALPATH
+  char *p;
+#endif
+
+
+#ifdef HAVE_REALPATH
+# ifdef PATH_MAX
+  path_max = PATH_MAX
+# else
+  path_max = pathconf (path, _PC_PATH_MAX);
+  if (path_max <= 0)
+    /* 1024 is taken from glibc-2.10.  */
+    path_max = 1024;
+# endif
+  p = xmalloc (path_max);
+  p = realpath (path, p);
+  if (p == NULL)
+    grub_util_error ("failed to get realpath of %s", path);
+
+#ifdef __CYGWIN__
+  if (strncmp (p,"/cygdrive/") == 0)
+    {
+      p += sizeof "/cygdrive/c" - 1;
+    }
+  else
+    grub_util_error "path not under /cygdrive/ aborting.";
+#endif
+  buf = xmalloc (path_max);
+  buf2 = xmalloc (path_max);
+  strcpy (buf, p);
+  free (p);
+#else /* ! HAVE_REALPATH */
+# warning "The function `grub_make_system_path_relative_to_its_root' might not work on your OS correctly."
+  path_max = 1024;
+  buf = xmalloc (path_max);
+  buf2 = xmalloc (path_max);
+  memset (buf, 0, sizeof (buf));
+  if (*path != '/')
+    {
+      size_t len = strlen (path);
+
+      if (getcwd (buf, 1024 - len) == NULL)
+	grub_util_error ("can not get current working directory");
+
+      strcat (buf, "/");
+      strcat (buf, path);
+    }
+  else
+    {
+      if (strlen (path) > 1024)
+	grub_util_error ("path too long");
+      strncpy (buf, path, 1024);
+    }
+#endif /* ! HAVE_REALPATH */
+
+  strcpy (buf2, buf);
+  if (stat (buf, &st) < 0)
+    grub_util_error ("can not stat %s", p);
+
+  num = st.st_dev;
+  while (1)
+    {
+      p = strrchr (buf, '/');
+      if (p == NULL)
+	grub_util_error ("FIXME no / in p");
+      if (p != buf)
+	*p = 0;
+      else
+	*++p = 0;
+
+      if (stat (buf, &st) < 0)
+	grub_util_error ("can not stat %s", buf);
+
+      if (st.st_dev != num)
+	break;
+
+      offset = p - buf;
+      if (offset == 1)
+	return buf2;
+    }
+  return buf2 + offset;
+}
diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
index 997811b..7fb0273 100644
--- a/util/i386/pc/grub-setup.c
+++ b/util/i386/pc/grub-setup.c
@@ -89,7 +89,7 @@ setup (const char *dir,
        const char *root, const char *dest, int must_embed, int force)
 {
   char *boot_path, *core_path, *core_path_dev;
-  char *boot_img, *core_img;
+  char *boot_img, *core_img, *p;
   size_t boot_size, core_size;
   grub_uint16_t core_sectors;
   grub_device_t root_dev, dest_dev;
@@ -404,7 +404,9 @@ unable_to_embed:
   
   /* Make sure that GRUB reads the identical image as the OS.  */
   tmp_img = xmalloc (core_size);
-  core_path_dev = grub_util_get_path (dir, core_file);
+  p = grub_util_get_path (dir, core_file);
+  core_path_dev = grub_make_system_path_relative_to_its_root (p);
+  free (p);
   
   /* It is a Good Thing to sync two times.  */
   sync ();

Re: [PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 1495 bytes --]

Am Donnerstag, den 11.06.2009, 01:00 +0200 schrieb Felix Zielcke:
> Am Dienstag, den 09.06.2009, 23:51 +0200 schrieb Vladimir 'phcoder'
> Serbinenko:
> 
> > +
> > +char *grub_make_system_path_relative_to_its_root (char *path)
> > +{
> > +
> > +  struct stat st;
> > +  char buf[500], buf2[500];
> > Use malloc instead of static allocation
> 
> Changed.
> 
> > +      p = strrchr (buf, '/');
> > +      if (p != buf)
> > +	*p = 0;
> > +      else *++p = 0;
> > You assume path starts with /. You have to check this. Otherwise you
> > may get sigsegv
> 
> Changed.
> > +      strcpy(buf2,buf);
> > Just save (p - buf) instead of copying buf to buf2
> 
> I did this now if realpath () isn't avaible.
> Now realpath is used in case it's avaible. POSIX specifies it and
> readlink is actually using it if it's avaible. I didn't read the source
> correctly.
> The problem is just Cygwin. It has it but it returns the cygwin path.
> So C:\\Windows would get /cygdrive/c/windows, which is easy to handle.
> But realpath ("/boot/grub") would return /boot/grub which isn't true
> from Windows/GRUB point of view.
> Maybe Christian and Bean can say something about it.
> MingW doestn't have it and I don't know what Windows would have, so the
> MingW users would still use my own way.

Here's now a new one which aborts if realpath is avaible but doestn't
support (path, NULL)
and the fallback function is changed to dynamically allocate the memory.
And now all memory is properly free'd
-- 
Felix Zielcke

[-- Attachment #2: make_sys_path_relative.patch.5 --]
[-- Type: text/plain, Size: 4560 bytes --]

2009-06-11  Felix Zielcke  <fzielcke@z-51.de>

	* configure.ac (AC_CHECK_FUNCS): Add realpath.
	* include/grub/util/hostdisk.c
	(grub_make_system_path_relative_to_its_root): New function
	prototype.
	* util/hostdisk.c: Include <stdint.h>.
	(grub_make_system_path_relative_to_its_root): New function.
	* util/i386/pc/grub-setup.c (setup): Use
	grub_make_system_path_relative_to_its_root to make core_path_dev
	relative to the partition.

diff --git a/configure.ac b/configure.ac
index b0f7bb7..9d594db 100644
--- a/configure.ac
+++ b/configure.ac
@@ -196,7 +196,7 @@ if test "$target_cpu"-"$platform" = i386-pc; then
 fi
 
 # Check for functions.
-AC_CHECK_FUNCS(posix_memalign memalign asprintf)
+AC_CHECK_FUNCS(posix_memalign memalign asprintf realpath)
 
 #
 # Check for target programs.
diff --git a/include/grub/util/hostdisk.h b/include/grub/util/hostdisk.h
index 21efb0d..85df1c6 100644
--- a/include/grub/util/hostdisk.h
+++ b/include/grub/util/hostdisk.h
@@ -23,5 +23,6 @@
 void grub_util_biosdisk_init (const char *dev_map);
 void grub_util_biosdisk_fini (void);
 char *grub_util_biosdisk_get_grub_dev (const char *os_dev);
+char *grub_make_system_path_relative_to_its_root (char *path,void **free_ptr);
 
 #endif /* ! GRUB_BIOSDISK_MACHINE_UTIL_HEADER */
diff --git a/util/hostdisk.c b/util/hostdisk.c
index 1844a7e..3224e07 100644
--- a/util/hostdisk.c
+++ b/util/hostdisk.c
@@ -27,6 +27,7 @@
 #include <grub/misc.h>
 
 #include <stdio.h>
+#include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
 #include <ctype.h>
@@ -1076,3 +1077,95 @@ grub_util_biosdisk_get_grub_dev (const char *os_dev)
   return make_device_name (drive, -1, -1);
 #endif
 }
+
+char *
+grub_make_system_path_relative_to_its_root (char *path, void **free_ptr)
+{
+  struct stat st;
+  char *buf, *buf2;
+  uintptr_t offset = 0;
+  dev_t num;
+  long path_max;
+  size_t len, len2;
+  char *p;
+
+
+#ifdef HAVE_REALPATH
+  p = realpath (path, NULL);
+
+  if (p == NULL) && (ernno != )
+    grub_util_error ("failed to get realpath of %s", path);
+  else
+    grub_util_error ("realpath not supporting (path, NULL)");
+  len = strlen (p) + 1;
+  buf = xmalloc (len);
+  buf2 = xmalloc (len);
+# ifdef __CYGWIN__
+  if (strncmp (p, 10, "/cygdrive/") == 0)
+    strcpy (buf, p + sizeof ("/cygdrive/c") - 1);
+  else
+    grub_util_error "path not under /cygdrive/. Aborting.";
+# else
+  strcpy (buf, p);
+#endif
+  free (p);
+#else /* ! HAVE_REALPATH */
+# warning "The function `grub_make_system_path_relative_to_its_root' might not work on your OS correctly."
+  if (*path != '/')
+    {
+      len2 = 4096;
+      do
+	{
+	  p = getcwd (buf, len)
+	    if (p == NULL)
+	      {
+		if (errno != ERANGE)
+		  grub_util_error ("can not get current working directory");
+		else
+		  len2 *= 2;
+	      }
+	} while (p == NULL)
+
+      buf = xmalloc (strlen (path) + len2 + 1);
+      strcat (buf, "/");
+      strcat (buf, path);
+    }
+  else
+    {
+      buf = xmalloc (strlen (path) + 1);
+      strcpy (buf, path)
+    }
+#endif /* ! HAVE_REALPATH */
+  buf2 = xmalloc (strlen (buf) + 1);
+  strcpy (buf2, buf);
+  *free_ptr = buf2;
+  if (stat (buf, &st) < 0)
+    grub_util_error ("can not stat %s", p);
+
+  num = st.st_dev;
+  while (1)
+    {
+      p = strrchr (buf, '/');
+      if (p == NULL)
+	grub_util_error ("FIXME no / in buf");
+      if (p != buf)
+	*p = 0;
+      else
+	*++p = 0;
+
+      if (stat (buf, &st) < 0)
+	grub_util_error ("can not stat %s", buf);
+
+      if (st.st_dev != num)
+	break;
+
+      offset = p - buf;
+      if (offset == 1)
+	{
+	  free (buf);
+	  return buf2;
+	}
+    }
+  free (buf);
+  return buf2 + offset;
+}
diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
index bdf234c..c9243bd 100644
--- a/util/i386/pc/grub-setup.c
+++ b/util/i386/pc/grub-setup.c
@@ -89,7 +89,7 @@ setup (const char *dir,
        const char *root, const char *dest, int must_embed, int force)
 {
   char *boot_path, *core_path, *core_path_dev;
-  char *boot_img, *core_img;
+  char *boot_img, *core_img, *p, *free_ptr;
   size_t boot_size, core_size;
   grub_uint16_t core_sectors;
   grub_device_t root_dev, dest_dev;
@@ -404,7 +404,10 @@ unable_to_embed:
 
   /* Make sure that GRUB reads the identical image as the OS.  */
   tmp_img = xmalloc (core_size);
-  core_path_dev = grub_util_get_path (dir, core_file);
+  p = grub_util_get_path (dir, core_file);
+  core_path_dev = grub_make_system_path_relative_to_its_root (p,&free_ptr);
+  free (p);
+  free (free_ptr);
 
   /* It is a Good Thing to sync two times.  */
   sync ();

Re: [PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails

[Felix Zielcke]

Am Freitag, den 12.06.2009, 01:21 +0200 schrieb Felix Zielcke:
> Am Donnerstag, den 11.06.2009, 01:00 +0200 schrieb Felix Zielcke:
> > Am Dienstag, den 09.06.2009, 23:51 +0200 schrieb Vladimir 'phcoder'
> > Serbinenko:
> > 
> > > +
> > > +char *grub_make_system_path_relative_to_its_root (char *path)
> > > +{
> > > +
> > > +  struct stat st;
> > > +  char buf[500], buf2[500];
> > > Use malloc instead of static allocation
> > 
> > Changed.
> > 
> > > +      p = strrchr (buf, '/');
> > > +      if (p != buf)
> > > +	*p = 0;
> > > +      else *++p = 0;
> > > You assume path starts with /. You have to check this. Otherwise you
> > > may get sigsegv
> > 
> > Changed.
> > > +      strcpy(buf2,buf);
> > > Just save (p - buf) instead of copying buf to buf2
> > 
> > I did this now if realpath () isn't avaible.
> > Now realpath is used in case it's avaible. POSIX specifies it and
> > readlink is actually using it if it's avaible. I didn't read the source
> > correctly.
> > The problem is just Cygwin. It has it but it returns the cygwin path.
> > So C:\\Windows would get /cygdrive/c/windows, which is easy to handle.
> > But realpath ("/boot/grub") would return /boot/grub which isn't true
> > from Windows/GRUB point of view.
> > Maybe Christian and Bean can say something about it.
> > MingW doestn't have it and I don't know what Windows would have, so the
> > MingW users would still use my own way.
> 
> Here's now a new one which aborts if realpath is avaible but doestn't
> support (path, NULL)
> and the fallback function is changed to dynamically allocate the memory.
> And now all memory is properly free'd

args just noticed I forgot the *free_ptr = buf2;
I'm too lazy now to send an extra patch just for this.
-- 
Felix Zielcke

Re: [PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 1952 bytes --]

Am Freitag, den 12.06.2009, 01:25 +0200 schrieb Felix Zielcke:
> Am Freitag, den 12.06.2009, 01:21 +0200 schrieb Felix Zielcke:
> > Am Donnerstag, den 11.06.2009, 01:00 +0200 schrieb Felix Zielcke:
> > > Am Dienstag, den 09.06.2009, 23:51 +0200 schrieb Vladimir 'phcoder'
> > > Serbinenko:
> > > 
> > > > +
> > > > +char *grub_make_system_path_relative_to_its_root (char *path)
> > > > +{
> > > > +
> > > > +  struct stat st;
> > > > +  char buf[500], buf2[500];
> > > > Use malloc instead of static allocation
> > > 
> > > Changed.
> > > 
> > > > +      p = strrchr (buf, '/');
> > > > +      if (p != buf)
> > > > +	*p = 0;
> > > > +      else *++p = 0;
> > > > You assume path starts with /. You have to check this. Otherwise you
> > > > may get sigsegv
> > > 
> > > Changed.
> > > > +      strcpy(buf2,buf);
> > > > Just save (p - buf) instead of copying buf to buf2
> > > 
> > > I did this now if realpath () isn't avaible.
> > > Now realpath is used in case it's avaible. POSIX specifies it and
> > > readlink is actually using it if it's avaible. I didn't read the source
> > > correctly.
> > > The problem is just Cygwin. It has it but it returns the cygwin path.
> > > So C:\\Windows would get /cygdrive/c/windows, which is easy to handle.
> > > But realpath ("/boot/grub") would return /boot/grub which isn't true
> > > from Windows/GRUB point of view.
> > > Maybe Christian and Bean can say something about it.
> > > MingW doestn't have it and I don't know what Windows would have, so the
> > > MingW users would still use my own way.
> > 
> > Here's now a new one which aborts if realpath is avaible but doestn't
> > support (path, NULL)
> > and the fallback function is changed to dynamically allocate the memory.
> > And now all memory is properly free'd
> 
> args just noticed I forgot the *free_ptr = buf2;
> I'm too lazy now to send an extra patch just for this.

Ok here's a new one which compiles without warnings.
-- 
Felix Zielcke

[-- Attachment #2: make_sys_path_relative.patch.6 --]
[-- Type: text/plain, Size: 4578 bytes --]

2009-06-12  Felix Zielcke  <fzielcke@z-51.de>

	* configure.ac (AC_CHECK_FUNCS): Add realpath.
	* include/grub/util/hostdisk.c
	(grub_make_system_path_relative_to_its_root): New function
	prototype.
	* util/hostdisk.c: Include <stdint.h>.
	(grub_make_system_path_relative_to_its_root): New function.
	* util/i386/pc/grub-setup.c (setup): Use
	grub_make_system_path_relative_to_its_root to make core_path_dev
	relative to the partition.

diff --git a/configure.ac b/configure.ac
index b0f7bb7..9d594db 100644
--- a/configure.ac
+++ b/configure.ac
@@ -196,7 +196,7 @@ if test "$target_cpu"-"$platform" = i386-pc; then
 fi
 
 # Check for functions.
-AC_CHECK_FUNCS(posix_memalign memalign asprintf)
+AC_CHECK_FUNCS(posix_memalign memalign asprintf realpath)
 
 #
 # Check for target programs.
diff --git a/include/grub/util/hostdisk.h b/include/grub/util/hostdisk.h
index 21efb0d..5c648a1 100644
--- a/include/grub/util/hostdisk.h
+++ b/include/grub/util/hostdisk.h
@@ -23,5 +23,6 @@
 void grub_util_biosdisk_init (const char *dev_map);
 void grub_util_biosdisk_fini (void);
 char *grub_util_biosdisk_get_grub_dev (const char *os_dev);
+char *grub_make_system_path_relative_to_its_root (char *path, char **free_ptr);
 
 #endif /* ! GRUB_BIOSDISK_MACHINE_UTIL_HEADER */
diff --git a/util/hostdisk.c b/util/hostdisk.c
index 1844a7e..25d01b2 100644
--- a/util/hostdisk.c
+++ b/util/hostdisk.c
@@ -27,6 +27,7 @@
 #include <grub/misc.h>
 
 #include <stdio.h>
+#include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
 #include <ctype.h>
@@ -1076,3 +1077,97 @@ grub_util_biosdisk_get_grub_dev (const char *os_dev)
   return make_device_name (drive, -1, -1);
 #endif
 }
+
+char *
+grub_make_system_path_relative_to_its_root (char *path, char **free_ptr)
+{
+  struct stat st;
+  char *buf, *buf2;
+  uintptr_t offset = 0;
+  dev_t num;
+  size_t len;
+  char *p;
+#ifndef HAVE_REALPATH
+  size_t len2;
+#endif
+
+
+#ifdef HAVE_REALPATH
+  p = realpath (path, NULL);
+
+  if (p == NULL && errno != EINVAL)
+    grub_util_error ("failed to get realpath of %s", path);
+  else
+    grub_util_error ("realpath not supporting (path, NULL)");
+  len = strlen (p) + 1;
+  buf = xmalloc (len);
+  buf2 = xmalloc (len);
+# ifdef __CYGWIN__
+  if (strncmp (p, 10, "/cygdrive/") == 0)
+    strcpy (buf, p + sizeof ("/cygdrive/c") - 1);
+  else
+    grub_util_error "path not under /cygdrive/";
+# else
+  strcpy (buf, p);
+#endif
+  free (p);
+#else /* ! HAVE_REALPATH */
+# warning "The function `grub_make_system_path_relative_to_its_root' might not work on your OS correctly."
+  if (*path != '/')
+    {
+      len2 = 4096;
+      do
+	{
+	  p = getcwd (buf, len)
+	    if (p == NULL)
+	      {
+		if (errno != ERANGE)
+		  grub_util_error ("can not get current working directory");
+		else
+		  len2 *= 2;
+	      }
+	} while (p == NULL)
+
+      buf = xmalloc (strlen (path) + len2 + 1);
+      strcat (buf, "/");
+      strcat (buf, path);
+    }
+  else
+    {
+      buf = xmalloc (strlen (path) + 1);
+      strcpy (buf, path)
+    }
+#endif /* ! HAVE_REALPATH */
+  buf2 = xmalloc (strlen (buf) + 1);
+  strcpy (buf2, buf);
+  *free_ptr = buf2;
+  if (stat (buf, &st) < 0)
+    grub_util_error ("can not stat %s", p);
+
+  num = st.st_dev;
+  while (1)
+    {
+      p = strrchr (buf, '/');
+      if (p == NULL)
+	grub_util_error ("FIXME no / in buf");
+      if (p != buf)
+	*p = 0;
+      else
+	*++p = 0;
+
+      if (stat (buf, &st) < 0)
+	grub_util_error ("can not stat %s", buf);
+
+      if (st.st_dev != num)
+	break;
+
+      offset = p - buf;
+      if (offset == 1)
+	{
+	  free (buf);
+	  return buf2;
+	}
+    }
+  free (buf);
+  return buf2 + offset;
+}
diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
index bdf234c..395db9d 100644
--- a/util/i386/pc/grub-setup.c
+++ b/util/i386/pc/grub-setup.c
@@ -89,7 +89,7 @@ setup (const char *dir,
        const char *root, const char *dest, int must_embed, int force)
 {
   char *boot_path, *core_path, *core_path_dev;
-  char *boot_img, *core_img;
+  char *boot_img, *core_img, *p, *free_ptr;
   size_t boot_size, core_size;
   grub_uint16_t core_sectors;
   grub_device_t root_dev, dest_dev;
@@ -404,7 +404,10 @@ unable_to_embed:
 
   /* Make sure that GRUB reads the identical image as the OS.  */
   tmp_img = xmalloc (core_size);
-  core_path_dev = grub_util_get_path (dir, core_file);
+  p = grub_util_get_path (dir, core_file);
+  core_path_dev = grub_make_system_path_relative_to_its_root (p, &free_ptr);
+  free (p);
+  free (free_ptr);
 
   /* It is a Good Thing to sync two times.  */
   sync ();

Re: [PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails

[Pavel Roskin]

On Fri, 2009-06-12 at 12:28 +0200, Felix Zielcke wrote:

> Ok here's a new one which compiles without warnings.

I suggest that whenever a patch is published, it comes with a detailed
description.  Surely, it could be gathered by rereading the thread, but
I think it's not sufficient for two reasons.

The first reason is that we want the patches reviewed by more people.
Not everybody has time to read the whole discussion.

The second reason is that the description the shows how the author sees
the changes, what the potential benefits are, what code is affected.
The reviewers would be able to compare the goals to the implementation.

Even if the description was already published, it should not be hard to
publish it again, perhaps with some improvements.

Regarding this patch, I don't think we need to add a function to
hostdisk.c is it's only used in grub-setup.c.  It would be better to
have it in grub-setup.c unless it's a generic function or there is a
good chance that it will be used elsewhere.

grub_make_system_path_relative_to_its_root is a very long name.  There
is nothing wrong with a long name per se, but it may be in sign that the
function is too specialized and should not be in hostdisk.c.

Following is not good for several reasons:

#warning "The function `grub_make_system_path_relative_to_its_root'
might not work on your OS correctly."

The warning will only be seen by those who compile GRUB, but not by the
end users who installed a binary.  The warning doesn't explain what
exactly may not work correctly.  Since we are talking about new
functionality here, I'd rather omit an unsafe implementation if
realpath() is not available.

What is free_ptr?  It looks like it's a pointer that the caller should
free.  I think functions should be self-contained and should not ask the
caller to do the cleanup (unless they actually return something useful
in the allocated memory).

-- 
Regards,
Pavel Roskin

Re: [PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails

[Felix Zielcke]

Am Freitag, den 12.06.2009, 12:21 -0400 schrieb Pavel Roskin:
> On Fri, 2009-06-12 at 12:28 +0200, Felix Zielcke wrote:
> 
> > Ok here's a new one which compiles without warnings.
> 
> I suggest that whenever a patch is published, it comes with a detailed
> description.  Surely, it could be gathered by rereading the thread, but
> I think it's not sufficient for two reasons.
> 
> The first reason is that we want the patches reviewed by more people.
> Not everybody has time to read the whole discussion.
> 
> The second reason is that the description the shows how the author sees
> the changes, what the potential benefits are, what code is affected.
> The reviewers would be able to compare the goals to the implementation.
> 
> Even if the description was already published, it should not be hard to
> publish it again, perhaps with some improvements.

Well it does (well should) exactly do the same as the
make_system_path_relative_to_its_root in util/grub-mkconfig_lib except
that it's written in C instead of a bash function.

> Regarding this patch, I don't think we need to add a function to
> hostdisk.c is it's only used in grub-setup.c.  It would be better to
> have it in grub-setup.c unless it's a generic function or there is a
> good chance that it will be used elsewhere.
> 
> grub_make_system_path_relative_to_its_root is a very long name.  There
> is nothing wrong with a long name per se, but it may be in sign that the
> function is too specialized and should not be in hostdisk.c.
> 
> Following is not good for several reasons:
> 
> #warning "The function `grub_make_system_path_relative_to_its_root'
> might not work on your OS correctly."
> 
> The warning will only be seen by those who compile GRUB, but not by the
> end users who installed a binary.  The warning doesn't explain what
> exactly may not work correctly.  Since we are talking about new
> functionality here, I'd rather omit an unsafe implementation if
> realpath() is not available.
> 
> What is free_ptr?  It looks like it's a pointer that the caller should
> free.  I think functions should be self-contained and should not ask the
> caller to do the cleanup (unless they actually return something useful
> in the allocated memory).

Robert anyway already said that this needs discussing.
The function is currently only useful in grub-setup in the case
blocklists are used for core.img
I don't think it will be useful for anything else.

About free_ptr,
yeah I could just copy buf2 + offset to a buf3 and then just free buf2
inside the function so that the caller can just free the return value of
it.
But I got this idea only after I sent the patch and I already told
Vladimir that he doestn't need to look anymore because Robert wants to
have the whole design discussed first.

-- 
Felix Zielcke

Re: grub-install --root-directory=/mnt /dev/sda1 fails

[Felix Zielcke]

Am Montag, den 01.06.2009, 21:39 +0200 schrieb Felix Zielcke:
> Am Mittwoch, den 06.05.2009, 17:12 +0200 schrieb Vladimir 'phcoder'
> Serbinenko:
> > Don't we already have a function which transforms host directory into
> > grub
> > directory? AFAIR we have.
> 
> There's just the shell function in grub-mkconfig_lib.in
> Here's now a patch wich implements it in util/hostdisk.c and gets used
> for core_path_dev in setup ().
> But it doestn't work with symlinks.
> readlink () can only be used if the file pointed to is a symlink, not if
> a symlink is somewhere in between.
> coreutils where the readlink binary is from is GPL 3+ but the function
> for it uses hash tables and it seems like it would be too much code to
> copy just for this.

So what do we do know about this problem?
We could just assume that the directory given with grub-setup
--directory is already the real absolute path and just use the stat
magic to make it relative.
Or we use realpath() if avaible to get the real one.
And on systems not having it, like mingw we could just assume that it's
already relative and just return the path given.
Or is there some other way this bug can be fixed?
-- 
Felix Zielcke

Re: grub-install --root-directory=/mnt /dev/sda1 fails

[Robert Millan]

On Wed, Jul 01, 2009 at 04:33:25PM +0200, Felix Zielcke wrote:
> Am Montag, den 01.06.2009, 21:39 +0200 schrieb Felix Zielcke:
> > Am Mittwoch, den 06.05.2009, 17:12 +0200 schrieb Vladimir 'phcoder'
> > Serbinenko:
> > > Don't we already have a function which transforms host directory into
> > > grub
> > > directory? AFAIR we have.
> > 
> > There's just the shell function in grub-mkconfig_lib.in
> > Here's now a patch wich implements it in util/hostdisk.c and gets used
> > for core_path_dev in setup ().
> > But it doestn't work with symlinks.
> > readlink () can only be used if the file pointed to is a symlink, not if
> > a symlink is somewhere in between.
> > coreutils where the readlink binary is from is GPL 3+ but the function
> > for it uses hash tables and it seems like it would be too much code to
> > copy just for this.
> 
> So what do we do know about this problem?
> We could just assume that the directory given with grub-setup
> --directory is already the real absolute path and just use the stat
> magic to make it relative.
> Or we use realpath() if avaible to get the real one.
> And on systems not having it, like mingw we could just assume that it's
> already relative and just return the path given.
> Or is there some other way this bug can be fixed?

I just looked into this, and I don't think there's any real problem that
needs fixing.  The "workaround" is just a red herring, it works for him by
pure chance.

See:

  http://savannah.gnu.org/bugs/index.php?26924

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: grub-install --root-directory=/mnt /dev/sda1 fails

[Felix Zielcke]

Am Samstag, den 04.07.2009, 22:09 +0200 schrieb Robert Millan:
> On Wed, Jul 01, 2009 at 04:33:25PM +0200, Felix Zielcke wrote:
> > Am Montag, den 01.06.2009, 21:39 +0200 schrieb Felix Zielcke:
> > > Am Mittwoch, den 06.05.2009, 17:12 +0200 schrieb Vladimir 'phcoder'
> > > Serbinenko:
> > > > Don't we already have a function which transforms host directory into
> > > > grub
> > > > directory? AFAIR we have.
> > > 
> > > There's just the shell function in grub-mkconfig_lib.in
> > > Here's now a patch wich implements it in util/hostdisk.c and gets used
> > > for core_path_dev in setup ().
> > > But it doestn't work with symlinks.
> > > readlink () can only be used if the file pointed to is a symlink, not if
> > > a symlink is somewhere in between.
> > > coreutils where the readlink binary is from is GPL 3+ but the function
> > > for it uses hash tables and it seems like it would be too much code to
> > > copy just for this.
> > 
> > So what do we do know about this problem?
> > We could just assume that the directory given with grub-setup
> > --directory is already the real absolute path and just use the stat
> > magic to make it relative.
> > Or we use realpath() if avaible to get the real one.
> > And on systems not having it, like mingw we could just assume that it's
> > already relative and just return the path given.
> > Or is there some other way this bug can be fixed?
> 
> I just looked into this, and I don't think there's any real problem that
> needs fixing.  The "workaround" is just a red herring, it works for him by
> pure chance.
> 
> See:
> 
>   http://savannah.gnu.org/bugs/index.php?26924

I think I tested this and the root device was set correctly, so it
looked at the right device for the core.img
-- 
Felix Zielcke