[PATCH,HURD] Fix grub-probe with userland partition support

[PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

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

Hello,

GNU/Hurd is moving to userland partition support, i.e. /dev/hd0s* slices
are actually served by userland translators which use libparted.  The
grub probe code needs to be fixed for that, here is a proposed patch.

The principle is that it gets the store translator behind the file being
probed for, and compares the storage area there with what grub detects
itself.  That is why it makes use of the map array, which I preferred
to rename to hostdisk_map (thus big but trivial changes in hostdisk.c):
even if it was already an external symbol, it's preferrable to prefix
it.

Samuel

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 18919 bytes --]

=== modified file 'Makefile.util.def'
--- Makefile.util.def	2012-04-01 19:35:18 +0000
+++ Makefile.util.def	2012-04-22 18:26:01 +0000
@@ -170,7 +170,7 @@
   ldadd = libgrubgcry.a;
   ldadd = libgrubkern.a;
   ldadd = grub-core/gnulib/libgnu.a;
-  ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
+  ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM) $(LIBSTORE)';
 };
 
 program = {
@@ -300,7 +300,7 @@
   ldadd = libgrubgcry.a;
   ldadd = libgrubkern.a;
   ldadd = grub-core/gnulib/libgnu.a;
-  ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
+  ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM) $(LIBSTORE)';
 };
 
 program = {
@@ -316,7 +316,7 @@
   ldadd = libgrubkern.a;
   ldadd = libgrubgcry.a;
   ldadd = grub-core/gnulib/libgnu.a;
-  ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
+  ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM) $(LIBSTORE)';
   cppflags = '-DGRUB_SETUP_BIOS=1';
 };
 
@@ -334,7 +334,7 @@
   ldadd = libgrubkern.a;
   ldadd = libgrubgcry.a;
   ldadd = grub-core/gnulib/libgnu.a;
-  ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
+  ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM) $(LIBSTORE)';
   cppflags = '-DGRUB_SETUP_SPARC64=1';
 };
 

=== modified file 'configure.ac'
--- configure.ac	2012-04-19 09:34:43 +0000
+++ configure.ac	2012-04-22 17:50:59 +0000
@@ -341,6 +341,18 @@
 ])
 AC_SUBST([LIBUTIL])
 
+# On Hurd we need libstore for util/getroot.c
+case "$host_kernel" in 
+  hurd)
+    AC_CHECK_HEADER([hurd/store.h], [], [AC_MSG_ERROR([hurd/store.h is needed])])
+    AC_CHECK_LIB([store], [store_create], [], [AC_MSG_ERROR([libstore is needed])])
+    LIBSTORE="-lstore"
+    ;;
+  *)
+    ;;
+esac
+AC_SUBST([LIBSTORE])
+
 #
 # Check for host and build compilers.
 #

=== modified file 'grub-core/kern/emu/hostdisk.c'
--- grub-core/kern/emu/hostdisk.c	2012-04-22 16:44:19 +0000
+++ grub-core/kern/emu/hostdisk.c	2012-04-22 18:46:07 +0000
@@ -115,12 +115,7 @@
 # endif /* ! RAW_FLOPPY_MAJOR */
 #endif /* defined(__NetBSD__) */
 
-struct
-{
-  char *drive;
-  char *device;
-  int device_map;
-} map[256];
+struct hostdisk_map hostdisk_map[256];
 
 struct grub_util_biosdisk_data
 {
@@ -202,8 +197,8 @@
 
   if (name)
     {
-      for (i = 0; i < ARRAY_SIZE (map); i++)
-	if (map[i].drive && unescape_cmp (map[i].drive, name) == 0)
+      for (i = 0; i < ARRAY_SIZE (hostdisk_map); i++)
+	if (hostdisk_map[i].drive && unescape_cmp (hostdisk_map[i].drive, name) == 0)
 	  return i;
     }
 
@@ -215,8 +210,8 @@
 {
   unsigned int i;
 
-  for (i = 0; i < sizeof (map) / sizeof (map[0]); i++)
-    if (! map[i].drive)
+  for (i = 0; i < sizeof (hostdisk_map) / sizeof (hostdisk_map[0]); i++)
+    if (! hostdisk_map[i].drive)
       return i;
 
   return -1;
@@ -231,8 +226,8 @@
   if (pull != GRUB_DISK_PULL_NONE)
     return 0;
 
-  for (i = 0; i < sizeof (map) / sizeof (map[0]); i++)
-    if (map[i].drive && hook (map[i].drive))
+  for (i = 0; i < sizeof (hostdisk_map) / sizeof (hostdisk_map[0]); i++)
+    if (hostdisk_map[i].drive && hook (hostdisk_map[i].drive))
       return 1;
 
   return 0;
@@ -354,14 +349,14 @@
   data->access_mode = 0;
   data->fd = -1;
   data->is_disk = 0;
-  data->device_map = map[drive].device_map;
+  data->device_map = hostdisk_map[drive].device_map;
 
   /* Get the size.  */
 #if defined(__MINGW32__)
   {
     grub_uint64_t size;
 
-    size = grub_util_get_disk_size (map[drive].device);
+    size = grub_util_get_disk_size (hostdisk_map[drive].device);
 
     if (size % 512)
       grub_util_error (_("unaligned device size"));
@@ -376,12 +371,12 @@
   {
     int fd;
 
-    fd = open (map[drive].device, O_RDONLY);
+    fd = open (hostdisk_map[drive].device, O_RDONLY);
     if (fd == -1)
       return grub_error (GRUB_ERR_UNKNOWN_DEVICE, N_("cannot open `%s': %s"),
-			 map[drive].device, strerror (errno));
+			 hostdisk_map[drive].device, strerror (errno));
 
-    disk->total_sectors = grub_util_get_fd_size (fd, map[drive].device,
+    disk->total_sectors = grub_util_get_fd_size (fd, hostdisk_map[drive].device,
 						 &disk->log_sector_size);
     disk->total_sectors >>= disk->log_sector_size;
 
@@ -769,26 +764,26 @@
 {
   unsigned int i;
 
-  for (i = 0; i < ARRAY_SIZE (map); i++)
-    if (! map[i].device)
+  for (i = 0; i < ARRAY_SIZE (hostdisk_map); i++)
+    if (! hostdisk_map[i].device)
       break;
-    else if (strcmp (map[i].device, os_disk) == 0)
-      return map[i].drive;
+    else if (strcmp (hostdisk_map[i].device, os_disk) == 0)
+      return hostdisk_map[i].drive;
 
   if (!add)
     return NULL;
 
-  if (i == ARRAY_SIZE (map))
+  if (i == ARRAY_SIZE (hostdisk_map))
     /* TRANSLATORS: it refers to the lack of free slots.  */
     grub_util_error ("%s", _("device count exceeds limit"));
 
-  map[i].device = xstrdup (os_disk);
-  map[i].drive = xmalloc (sizeof ("hostdisk/") + strlen (os_disk));
-  strcpy (map[i].drive, "hostdisk/");
-  strcpy (map[i].drive + sizeof ("hostdisk/") - 1, os_disk);
-  map[i].device_map = 0;
+  hostdisk_map[i].device = xstrdup (os_disk);
+  hostdisk_map[i].drive = xmalloc (sizeof ("hostdisk/") + strlen (os_disk));
+  strcpy (hostdisk_map[i].drive, "hostdisk/");
+  strcpy (hostdisk_map[i].drive + sizeof ("hostdisk/") - 1, os_disk);
+  hostdisk_map[i].device_map = 0;
 
-  return map[i].drive;
+  return hostdisk_map[i].drive;
 }
 
 static int
@@ -823,9 +818,9 @@
 
     part_start = grub_partition_get_start (disk->partition);
 
-    strcpy (dev, map[disk->id].device);
+    strcpy (dev, hostdisk_map[disk->id].device);
     if (disk->partition
-	&& strncmp (map[disk->id].device, "/dev/", 5) == 0)
+	&& strncmp (hostdisk_map[disk->id].device, "/dev/", 5) == 0)
       {
 	if (sector >= part_start)
 	  is_partition = linux_find_partition (dev, part_start);
@@ -884,7 +879,7 @@
 	    if (*max == 0)
 	      *max = ~0ULL;
 	    is_partition = 0;
-	    strcpy (dev, map[disk->id].device);
+	    strcpy (dev, hostdisk_map[disk->id].device);
 	    goto reopen;
 	  }
 	sector -= part_start;
@@ -910,7 +905,7 @@
     }
 #endif
 
-  if (data->dev && strcmp (data->dev, map[disk->id].device) == 0 &&
+  if (data->dev && strcmp (data->dev, hostdisk_map[disk->id].device) == 0 &&
       data->access_mode == (flags & O_ACCMODE))
     {
       grub_dprintf ("hostdisk", "reusing open device `%s'\n", data->dev);
@@ -933,10 +928,10 @@
 	    data->fd = -1;
 	}
 
-      fd = open (map[disk->id].device, flags);
+      fd = open (hostdisk_map[disk->id].device, flags);
       if (fd >= 0)
 	{
-	  data->dev = xstrdup (map[disk->id].device);
+	  data->dev = xstrdup (hostdisk_map[disk->id].device);
 	  data->access_mode = (flags & O_ACCMODE);
 	  data->fd = fd;
 	}
@@ -954,13 +949,13 @@
 #if defined(__APPLE__)
   /* If we can't have exclusive access, try shared access */
   if (fd < 0)
-    fd = open(map[disk->id].device, flags | O_SHLOCK);
+    fd = open(hostdisk_map[disk->id].device, flags | O_SHLOCK);
 #endif
 
   if (fd < 0)
     {
       grub_error (GRUB_ERR_BAD_DEVICE, N_("cannot open `%s': %s"),
-		  map[disk->id].device, strerror (errno));
+		  hostdisk_map[disk->id].device, strerror (errno));
       return -1;
     }
 #endif /* ! __linux__ */
@@ -969,7 +964,7 @@
   configure_device_driver (fd);
 #endif /* defined(__NetBSD__) */
 
-  if (grub_util_fd_seek (fd, map[disk->id].device,
+  if (grub_util_fd_seek (fd, hostdisk_map[disk->id].device,
 			 sector << disk->log_sector_size))
     {
       close (fd);
@@ -1056,7 +1051,7 @@
 	  if (grub_util_fd_read (fd, buf, (1 << disk->log_sector_size))
 	      != (1 << disk->log_sector_size))
 	    return grub_error (GRUB_ERR_READ_ERROR, N_("cannot read `%s': %s"),
-			       map[disk->id].device, strerror (errno));
+			       hostdisk_map[disk->id].device, strerror (errno));
 	  
 	  buf += (1 << disk->log_sector_size);
 	  size--;
@@ -1067,7 +1062,7 @@
       if (grub_util_fd_read (fd, buf, max << disk->log_sector_size)
 	  != (ssize_t) (max << disk->log_sector_size))
 	return grub_error (GRUB_ERR_READ_ERROR, N_("cannot read `%s': %s"),
-			   map[disk->id].device, strerror (errno));
+			   hostdisk_map[disk->id].device, strerror (errno));
       size -= max;
       buf += (max << disk->log_sector_size);
       sector += max;
@@ -1101,7 +1096,7 @@
 	      != (1 << disk->log_sector_size))
 	    return grub_error (GRUB_ERR_WRITE_ERROR,
 			       N_("cannot write to `%s': %s"),
-			       map[disk->id].device, strerror (errno));
+			       hostdisk_map[disk->id].device, strerror (errno));
 	  
 	  buf += (1 << disk->log_sector_size);
 	  size--;
@@ -1112,7 +1107,7 @@
       if (grub_util_fd_write (fd, buf, max << disk->log_sector_size)
 	  != (ssize_t) (max << disk->log_sector_size))
 	return grub_error (GRUB_ERR_WRITE_ERROR, N_("cannot write to `%s': %s"),
-			   map[disk->id].device, strerror (errno));
+			   hostdisk_map[disk->id].device, strerror (errno));
       size -= max;
       buf += (max << disk->log_sector_size);
     }
@@ -1234,10 +1229,10 @@
 	  show_error (tmp);
 	}
 
-      map[drive].drive = xmalloc (p - e + sizeof ('\0'));
-      strncpy (map[drive].drive, e, p - e + sizeof ('\0'));
-      map[drive].drive[p - e] = '\0';
-      map[drive].device_map = 1;
+      hostdisk_map[drive].drive = xmalloc (p - e + sizeof ('\0'));
+      strncpy (hostdisk_map[drive].drive, e, p - e + sizeof ('\0'));
+      hostdisk_map[drive].drive[p - e] = '\0';
+      hostdisk_map[drive].device_map = 1;
 
       p++;
       /* Skip leading spaces.  */
@@ -1260,8 +1255,8 @@
       if (stat (p, &st) == -1)
 #endif
 	{
-	  free (map[drive].drive);
-	  map[drive].drive = NULL;
+	  free (hostdisk_map[drive].drive);
+	  hostdisk_map[drive].drive = NULL;
 	  grub_util_info ("Cannot stat `%s', skipping", p);
 	  continue;
 	}
@@ -1272,13 +1267,13 @@
 	 symbolic links.  Leave /dev/mapper/ alone, though.  */
       if (strncmp (p, "/dev/mapper/", 12) != 0)
 	{
-	  map[drive].device = xmalloc (PATH_MAX);
-	  if (! realpath (p, map[drive].device))
+	  hostdisk_map[drive].device = xmalloc (PATH_MAX);
+	  if (! realpath (p, hostdisk_map[drive].device))
 	    grub_util_error (_("failed to get canonical path of %s"), p);
 	}
       else
 #endif
-      map[drive].device = xstrdup (p);
+      hostdisk_map[drive].device = xstrdup (p);
     }
 
   fclose (fp);
@@ -1296,13 +1291,13 @@
 {
   unsigned i;
 
-  for (i = 0; i < sizeof (map) / sizeof (map[0]); i++)
+  for (i = 0; i < sizeof (hostdisk_map) / sizeof (hostdisk_map[0]); i++)
     {
-      if (map[i].drive)
-	free (map[i].drive);
-      if (map[i].device)
-	free (map[i].device);
-      map[i].drive = map[i].device = NULL;
+      if (hostdisk_map[i].drive)
+	free (hostdisk_map[i].drive);
+      if (hostdisk_map[i].device)
+	free (hostdisk_map[i].device);
+      hostdisk_map[i].drive = hostdisk_map[i].device = NULL;
     }
 
   grub_disk_dev_unregister (&grub_util_biosdisk_dev);
@@ -1311,7 +1306,7 @@
 const char *
 grub_util_biosdisk_get_compatibility_hint (grub_disk_t disk)
 {
-  if (disk->dev != &grub_util_biosdisk_dev || map[disk->id].device_map)
+  if (disk->dev != &grub_util_biosdisk_dev || hostdisk_map[disk->id].device_map)
     return disk->name;
   return 0;
 }
@@ -1322,5 +1317,5 @@
   if (disk->dev != &grub_util_biosdisk_dev)
     return 0;
 
-  return map[disk->id].device;
+  return hostdisk_map[disk->id].device;
 }

=== modified file 'include/grub/emu/hostdisk.h'
--- include/grub/emu/hostdisk.h	2012-02-29 14:23:31 +0000
+++ include/grub/emu/hostdisk.h	2012-04-22 18:21:40 +0000
@@ -24,6 +24,15 @@
 #include <grub/partition.h>
 #include <sys/types.h>
 
+struct hostdisk_map
+{
+  char *drive;
+  char *device;
+  int device_map;
+};
+
+extern struct hostdisk_map hostdisk_map[256];
+
 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);

=== modified file 'util/getroot.c'
--- util/getroot.c	2012-04-22 19:02:55 +0000
+++ util/getroot.c	2012-04-22 20:12:16 +0000
@@ -700,6 +700,153 @@
 
 #elif defined (__GNU__)
 
+#include <mach.h>
+#include <hurd/hurd_types.h>
+#include <hurd/store.h>
+#include <grub/disk.h>
+#include <grub/partition.h>
+
+/* Open a store for storage information only. */
+static struct store *
+storeinfo_store (const char *path)
+{
+  file_t file;
+  struct store *store;
+  int err;
+
+  file = file_name_lookup (path, 0, 0);
+  if (file == MACH_PORT_NULL)
+    return NULL;
+
+  err = store_create (file, STORE_INACTIVE | STORE_NO_FILEIO, NULL, &store);
+  return err ? NULL : store;
+}
+
+/* Determine whether two stores share the same underlying storage. */
+static int
+same_storage (struct store *s1, struct store *s2)
+{
+  if (s1->class->id != s2->class->id)
+    return 0;
+
+  switch (s1->class->id)
+    {
+      case STORAGE_DEVICE:
+      case STORAGE_HURD_FILE:
+      case STORAGE_TASK:
+      case STORAGE_MEMORY:
+	if (s1->port != MACH_PORT_NULL && s1->port == s2->port)
+	  return 1;
+	else if (s1->name && s2->name && strcmp (s1->name, s2->name) == 0)
+	  return 1;
+	else
+	  return 0;
+
+      default:
+	return 0;
+    }
+}
+
+/* Retrieve the storage information for PATH, and search the device map until
+ * we find the matching disk, and maybe partition. */
+static char *
+find_hurd_root_device (const char *path)
+{
+  auto int check_part (grub_disk_t disk __attribute__((unused)),
+		  const grub_partition_t partition);
+  auto int check_disk (const char *name);
+
+  int winner_disk = -1, winner_part = -1;
+  struct store *path_store;
+  char *devpath = NULL;
+
+  int check_part (grub_disk_t disk __attribute__((unused)),
+		  const grub_partition_t partition)
+    {
+      store_offset_t start = partition->start * GRUB_DISK_SECTOR_SIZE,
+		     length = partition->len * GRUB_DISK_SECTOR_SIZE,
+		     fstart = path_store->runs->start * path_store->block_size;
+
+      grub_util_info ("Trying partition #%d (%ld+%ld)", partition->number,
+		      (long) start, (long) length);
+
+      if (fstart >= start && fstart < start + length)
+	winner_part = partition->number;
+      else
+	grub_util_info ("The target is outside (%ld)", (long) fstart);
+
+      return winner_part >= 0;
+    }
+
+  int check_disk (const char *name)
+    {
+      grub_disk_t disk;
+      struct store *disk_store;
+
+      grub_util_info ("Trying disk `%s'", name);
+
+      disk = grub_disk_open (name);
+      if (! disk)
+	goto out;
+
+      if (disk->dev->id != GRUB_DISK_DEVICE_BIOSDISK_ID && disk->dev->id != GRUB_DISK_DEVICE_HOSTDISK_ID)
+	{
+	  grub_util_info ("`%s' is not a biosdisk or hostdisk, it is %d", name, disk->dev->id);
+	  goto out_disk;
+	}
+
+      disk_store = storeinfo_store (hostdisk_map[disk->id].device);
+      if (! disk_store)
+	{
+	  grub_util_info ("Could not retrieve storage info for `%s'", hostdisk_map[disk->id].device);
+	  goto out_disk;
+	}
+
+      if (! same_storage (disk_store, path_store))
+	{
+	  grub_util_info ("`%s' does not use the same storage as `%s'", name, path);
+	  goto out_store;
+	}
+
+      winner_disk = disk->id;
+      grub_partition_iterate (disk, check_part);
+
+    out_store:
+      store_free (disk_store);
+    out_disk:
+      grub_disk_close (disk);
+    out:
+      return winner_disk >= 0;
+    }
+
+  path_store = storeinfo_store (path);
+  if (! path_store)
+    grub_util_error ("Could not retrieve storage information for `%s'", path);
+
+  if (path_store->num_runs >= 1)
+    grub_disk_dev_iterate (check_disk);
+
+  if (winner_disk >= 0)
+    {
+      const char *diskpath = hostdisk_map[winner_disk].device;
+      devpath = xmalloc (strlen (diskpath) + 3 * sizeof (int) + 2);
+
+      strcpy (devpath, diskpath);
+      if (winner_part >= 0)
+	sprintf (devpath + strlen (devpath), "s%d", winner_part + 1);
+    }
+  else if (path_store->class->id == STORAGE_DEVICE && path_store->name)
+    {
+      /* use whatever device PATH_STORE is based on */
+      devpath = xmalloc (strlen (path_store->name) + sizeof "/dev/");
+      sprintf (devpath, "/dev/%s", path_store->name);
+    }
+
+  store_free (path_store);
+
+  return devpath;
+}
+
 #elif ! defined(__CYGWIN__)
 
 char *
@@ -930,65 +1077,7 @@
 grub_guess_root_devices (const char *dir)
 {
   char **os_dev = NULL;
-#ifdef __GNU__
-  file_t file;
-  mach_port_t *ports;
-  int *ints;
-  loff_t *offsets;
-  char *data;
-  error_t err;
-  mach_msg_type_number_t num_ports = 0, num_ints = 0, num_offsets = 0, data_len = 0;
-  size_t name_len;
-
-  file = file_name_lookup (dir, 0, 0);
-  if (file == MACH_PORT_NULL)
-    return 0;
-
-  err = file_get_storage_info (file,
-			       &ports, &num_ports,
-			       &ints, &num_ints,
-			       &offsets, &num_offsets,
-			       &data, &data_len);
-
-  if (num_ints < 1)
-    grub_util_error (_("Storage info for `%s' does not include type"), dir);
-  if (ints[0] != STORAGE_DEVICE)
-    grub_util_error (_("Filesystem of `%s' is not stored on local disk"), dir);
-
-  if (num_ints < 5)
-    grub_util_error (_("Storage info for `%s' does not include name"), dir);
-  name_len = ints[4];
-  if (name_len < data_len)
-    grub_util_error (_("Bogus name length for storage info for `%s'"), dir);
-  if (data[name_len - 1] != '\0')
-    grub_util_error (_("Storage name for `%s' not NUL-terminated"), dir);
-
-  os_dev = xmalloc (2 * sizeof (os_dev[0]));
-  os_dev[0] = xmalloc (sizeof ("/dev/") - 1 + data_len);
-  memcpy (os_dev[0], "/dev/", sizeof ("/dev/") - 1);
-  memcpy (os_dev[0] + sizeof ("/dev/") - 1, data, data_len);
-  os_dev[1] = 0;
-
-  if (ports && num_ports > 0)
-    {
-      mach_msg_type_number_t i;
-      for (i = 0; i < num_ports; i++)
-        {
-	  mach_port_t port = ports[i];
-	  if (port != MACH_PORT_NULL)
-	    mach_port_deallocate (mach_task_self(), port);
-        }
-      munmap ((caddr_t) ports, num_ports * sizeof (*ports));
-    }
-
-  if (ints && num_ints > 0)
-    munmap ((caddr_t) ints, num_ints * sizeof (*ints));
-  if (offsets && num_offsets > 0)
-    munmap ((caddr_t) offsets, num_offsets * sizeof (*offsets));
-  if (data && data_len > 0)
-    munmap (data, data_len);
-  mach_port_deallocate (mach_task_self (), file);
-#else /* !__GNU__ */
+#ifndef __GNU__
   struct stat st;
   dev_t dev;
 
@@ -1035,6 +1124,7 @@
     grub_util_error (_("cannot stat `%s': %s"), dir, strerror (errno));
 
   dev = st.st_dev;
+#endif /* !__GNU__ */
 
   os_dev = xmalloc (2 * sizeof (os_dev[0]));
   
@@ -1042,6 +1132,10 @@
   /* Cygwin specific function.  */
   os_dev[0] = grub_find_device (dir, dev);
 
+#elif defined __GNU__
+  /* GNU/Hurd specific function.  */
+  os_dev[0] = find_hurd_root_device (dir);
+
 #else
 
   /* This might be truly slow, but is there any better way?  */
@@ -1054,7 +1148,6 @@
     }
 
   os_dev[1] = 0;
-#endif /* !__GNU__ */
 
   return os_dev;
 }

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 22.04.2012 20:24, Samuel Thibault wrote:
> Hello,
>
> GNU/Hurd is moving to userland partition support, i.e. /dev/hd0s* slices
> are actually served by userland translators which use libparted.  The
> grub probe code needs to be fixed for that, here is a proposed patch.
>
> The principle is that it gets the store translator behind the file being
> probed for, and compares the storage area there with what grub detects
> itself.  That is why it makes use of the map array, which I preferred
> to rename to hostdisk_map (thus big but trivial changes in hostdisk.c):
> even if it was already an external symbol, it's preferrable to prefix
> it.
map was never intended to be globally visible. At most perhaps
"semi-local" due to difficulties of split between getroot.c and hostdisk.c
Moreover the behaviour of find_root_device must be independent of what
is already in map since we don't rely anymore at map being populated at
all and at entries only on as-needed basis.
Additional problems include:

+      if (disk->dev->id != GRUB_DISK_DEVICE_BIOSDISK_ID && disk->dev->id != GRUB_DISK_DEVICE_HOSTDISK_ID)
+	{
No need to check for BIOSDISK. It's not used for hostdisk.
+      store_offset_t start = partition->start * GRUB_DISK_SECTOR_SIZE,
+		     length = partition->len * GRUB_DISK_SECTOR_SIZE,
This is both stylistically incorrect and wrong.
Stylistically at declaration we repeat type on every line.
Additionally partition->start is only relative to container, not whole disk. You have to use grub_disk_partition_get_start for this.


>
> Samuel
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

Vladimir 'φ-coder/phcoder' Serbinenko, le Mon 23 Apr 2012 10:52:09 +0200, a écrit :
> map was never intended to be globally visible. At most perhaps
> "semi-local" due to difficulties of split between getroot.c and hostdisk.c
> Moreover the behaviour of find_root_device must be independent of what
> is already in map since we don't rely anymore at map being populated at
> all and at entries only on as-needed basis.

How can I get the same information, then? (the unix path corresponding
to a disk opened with grub_disk_open())

Samuel

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 23.04.2012 11:40, Samuel Thibault wrote:
> Vladimir 'φ-coder/phcoder' Serbinenko, le Mon 23 Apr 2012 10:52:09 +0200, a écrit :
>> map was never intended to be globally visible. At most perhaps
>> "semi-local" due to difficulties of split between getroot.c and hostdisk.c
>> Moreover the behaviour of find_root_device must be independent of what
>> is already in map since we don't rely anymore at map being populated at
>> all and at entries only on as-needed basis.
> How can I get the same information, then? (the unix path corresponding
> to a disk opened with grub_disk_open())
The problem is bigger than just getting the device from a disk. The
problem is that you assume that grub_disk_dev_iterate will go through
the disk in question which isn't necessarily true.
> Samuel
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

Vladimir 'φ-coder/phcoder' Serbinenko, le Mon 23 Apr 2012 12:45:09 +0200, a écrit :
> On 23.04.2012 11:40, Samuel Thibault wrote:
> > Vladimir 'φ-coder/phcoder' Serbinenko, le Mon 23 Apr 2012 10:52:09 +0200, a écrit :
> >> map was never intended to be globally visible. At most perhaps
> >> "semi-local" due to difficulties of split between getroot.c and hostdisk.c
> >> Moreover the behaviour of find_root_device must be independent of what
> >> is already in map since we don't rely anymore at map being populated at
> >> all and at entries only on as-needed basis.
> > How can I get the same information, then? (the unix path corresponding
> > to a disk opened with grub_disk_open())
> The problem is bigger than just getting the device from a disk. The
> problem is that you assume that grub_disk_dev_iterate will go through
> the disk in question which isn't necessarily true.

And so?

Our need is to iterate over all disks that GRUB sees, to match that with
where the file resides.

Samuel

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 23.04.2012 13:06, Samuel Thibault wrote:
> Vladimir 'φ-coder/phcoder' Serbinenko, le Mon 23 Apr 2012 12:45:09 +0200, a écrit :
>> On 23.04.2012 11:40, Samuel Thibault wrote:
>>> Vladimir 'φ-coder/phcoder' Serbinenko, le Mon 23 Apr 2012 10:52:09 +0200, a écrit :
>>>> map was never intended to be globally visible. At most perhaps
>>>> "semi-local" due to difficulties of split between getroot.c and hostdisk.c
>>>> Moreover the behaviour of find_root_device must be independent of what
>>>> is already in map since we don't rely anymore at map being populated at
>>>> all and at entries only on as-needed basis.
>>> How can I get the same information, then? (the unix path corresponding
>>> to a disk opened with grub_disk_open())
>> The problem is bigger than just getting the device from a disk. The
>> problem is that you assume that grub_disk_dev_iterate will go through
>> the disk in question which isn't necessarily true.
> And so?
>
> Our need is to iterate over all disks that GRUB sees, to match that with
> where the file resides.
It looks like you have missed some of 2.00 developement. We auto-add the
device to map if necessary right after find_root_devices so at
find_root_devices the disk won't be seen yet but find_root_devices has
to work nevertheless.
This function has nothing to do with the rest of GRUB. It could be a
standalone program with syntax like
find_root_devices DIR
Output:
/dev/sda1 /dev/sdb2
Any of GRUB context is irrelevant. (and actually so is the GRUB
partition view, but you can use partition functions as they are
stateless but beware that their view is likely to be different from
GRUB's one).
> Samuel
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

Vladimir 'φ-coder/phcoder' Serbinenko, le Mon 23 Apr 2012 13:17:15 +0200, a écrit :
> On 23.04.2012 13:06, Samuel Thibault wrote:
> > Our need is to iterate over all disks that GRUB sees, to match that with
> > where the file resides.
> It looks like you have missed some of 2.00 developement.

I don't really follow GRUB development.

> This function has nothing to do with the rest of GRUB.

Right.  That's not how we were considering things due to more
possibilites on the Hurd, but I won't try to fight against that, I have
other things to spend time on :)

Samuel

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

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

Here is a revamped patch.

Samuel

[-- Attachment #2: patch-grub --]
[-- Type: text/plain, Size: 7840 bytes --]

=== modified file 'Makefile.util.def'
--- Makefile.util.def	2012-04-01 19:35:18 +0000
+++ Makefile.util.def	2012-04-22 18:26:01 +0000
@@ -170,7 +170,7 @@
   ldadd = libgrubgcry.a;
   ldadd = libgrubkern.a;
   ldadd = grub-core/gnulib/libgnu.a;
-  ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
+  ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM) $(LIBSTORE)';
 };
 
 program = {
@@ -300,7 +300,7 @@
   ldadd = libgrubgcry.a;
   ldadd = libgrubkern.a;
   ldadd = grub-core/gnulib/libgnu.a;
-  ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
+  ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM) $(LIBSTORE)';
 };
 
 program = {
@@ -316,7 +316,7 @@
   ldadd = libgrubkern.a;
   ldadd = libgrubgcry.a;
   ldadd = grub-core/gnulib/libgnu.a;
-  ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
+  ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM) $(LIBSTORE)';
   cppflags = '-DGRUB_SETUP_BIOS=1';
 };
 
@@ -334,7 +334,7 @@
   ldadd = libgrubkern.a;
   ldadd = libgrubgcry.a;
   ldadd = grub-core/gnulib/libgnu.a;
-  ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
+  ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM) $(LIBSTORE)';
   cppflags = '-DGRUB_SETUP_SPARC64=1';
 };
 

=== modified file 'configure.ac'
--- configure.ac	2012-04-19 09:34:43 +0000
+++ configure.ac	2012-04-22 17:50:59 +0000
@@ -341,6 +341,18 @@
 ])
 AC_SUBST([LIBUTIL])
 
+# On Hurd we need libstore for kern/emu/getroot.c
+case "$host_kernel" in 
+  hurd)
+    AC_CHECK_HEADER([hurd/store.h], [], [AC_MSG_ERROR([hurd/store.h is needed])])
+    AC_CHECK_LIB([store], [store_create], [], [AC_MSG_ERROR([libstore is needed])])
+    LIBSTORE="-lstore"
+    ;;
+  *)
+    ;;
+esac
+AC_SUBST([LIBSTORE])
+
 #
 # Check for host and build compilers.
 #

=== modified file 'util/getroot.c'
--- util/getroot.c	2012-04-22 19:02:55 +0000
+++ util/getroot.c	2012-04-24 01:27:50 +0000
@@ -45,8 +45,11 @@
 #endif
 
 #ifdef __GNU__
+#include <mach.h>
 #include <hurd.h>
 #include <hurd/lookup.h>
+#include <hurd/hurd_types.h>
+#include <hurd/store.h>
 #include <hurd/fs.h>
 #include <sys/mman.h>
 #endif
@@ -700,6 +703,115 @@
 
 #elif defined (__GNU__)
 
+/* Open a store for storage information only. */
+static struct store *
+storeinfo_store (const char *path)
+{
+  file_t file;
+  struct store *store;
+  int err;
+
+  file = file_name_lookup (path, 0, 0);
+  if (file == MACH_PORT_NULL)
+    return NULL;
+
+  err = store_create (file, STORE_INACTIVE | STORE_NO_FILEIO, NULL, &store);
+  if (err)
+    {
+      errno = err;
+      return NULL;
+    }
+
+  return store;
+}
+
+/* Retrieve the storage information for PATH, and search which partitions
+ * contains its start.  */
+static char *
+find_hurd_root_device (const char *path)
+{
+  struct store *path_store, *part_store;
+  char *devpath = NULL;
+  const char *prefix;
+  size_t len, prefix_len;
+  store_offset_t path_start;
+  int part;
+
+  path_store = storeinfo_store (path);
+  if (! path_store)
+    grub_util_error ("Could not retrieve storage information for `%s'", path);
+  if (! path_store->name)
+    grub_util_error ("No storage name associated with `%s'", path);
+
+  grub_util_info ("Storage is %s, type %d",
+      path_store->name, path_store->class->id);
+
+  if (path_store->class->id == STORAGE_DEVICE)
+    prefix = "/dev/";
+  else
+    prefix = "";
+
+  prefix_len = strlen (prefix);
+  len = strlen (path_store->name);
+  devpath = xmalloc (prefix_len + len + 4 + 1);
+
+  path_start = path_store->runs[0].start * path_store->block_size;
+
+  grub_util_info ("Path runs from %llu", path_start);
+
+  for (part = 1; part < 1000; part++)
+    {
+      struct store_run *first_run, *last_run;
+
+      snprintf (devpath, prefix_len + len + 4 + 1, "%s%ss%u",
+	  prefix, path_store->name, part);
+
+      grub_util_info ("Trying partition %s", devpath);
+      part_store = storeinfo_store (devpath);
+      if (! part_store)
+	{
+	  if (errno == ENOENT && part >= 5)
+	    {
+	      free (devpath);
+	      devpath = NULL;
+	      break;
+	    }
+	  continue;
+	}
+
+      first_run = &part_store->runs[0];
+      last_run = &part_store->runs[part_store->num_runs - 1];
+
+      grub_util_info ("Partition %u runs from %llu to %llu", part,
+	first_run->start * part_store->block_size,
+	(last_run->start + last_run->length) * part_store->block_size);
+
+      if (first_run->start * part_store->block_size <= path_start &&
+	  (last_run->start + last_run->length) * part_store->block_size > path_start) {
+	store_free (part_store);
+	break;
+      }
+      store_free (part_store);
+    }
+
+  if (part >= 1000)
+    {
+      free (devpath);
+      devpath = NULL;
+    }
+
+  if (!devpath)
+    {
+      devpath = xmalloc (prefix_len + len + 1);
+      snprintf (devpath, prefix_len + len + 1, "%s%s", prefix, path_store->name);
+      grub_util_info ("End of partitions, assuming %s", devpath);
+    }
+
+  store_free (path_store);
+
+  return devpath;
+}
+
 #elif ! defined(__CYGWIN__)
 
 char *
@@ -930,65 +1042,7 @@
 grub_guess_root_devices (const char *dir)
 {
   char **os_dev = NULL;
-#ifdef __GNU__
-  file_t file;
-  mach_port_t *ports;
-  int *ints;
-  loff_t *offsets;
-  char *data;
-  error_t err;
-  mach_msg_type_number_t num_ports = 0, num_ints = 0, num_offsets = 0, data_len = 0;
-  size_t name_len;
-
-  file = file_name_lookup (dir, 0, 0);
-  if (file == MACH_PORT_NULL)
-    return 0;
-
-  err = file_get_storage_info (file,
-			       &ports, &num_ports,
-			       &ints, &num_ints,
-			       &offsets, &num_offsets,
-			       &data, &data_len);
-
-  if (num_ints < 1)
-    grub_util_error (_("Storage info for `%s' does not include type"), dir);
-  if (ints[0] != STORAGE_DEVICE)
-    grub_util_error (_("Filesystem of `%s' is not stored on local disk"), dir);
-
-  if (num_ints < 5)
-    grub_util_error (_("Storage info for `%s' does not include name"), dir);
-  name_len = ints[4];
-  if (name_len < data_len)
-    grub_util_error (_("Bogus name length for storage info for `%s'"), dir);
-  if (data[name_len - 1] != '\0')
-    grub_util_error (_("Storage name for `%s' not NUL-terminated"), dir);
-
-  os_dev = xmalloc (2 * sizeof (os_dev[0]));
-  os_dev[0] = xmalloc (sizeof ("/dev/") - 1 + data_len);
-  memcpy (os_dev[0], "/dev/", sizeof ("/dev/") - 1);
-  memcpy (os_dev[0] + sizeof ("/dev/") - 1, data, data_len);
-  os_dev[1] = 0;
-
-  if (ports && num_ports > 0)
-    {
-      mach_msg_type_number_t i;
-      for (i = 0; i < num_ports; i++)
-        {
-	  mach_port_t port = ports[i];
-	  if (port != MACH_PORT_NULL)
-	    mach_port_deallocate (mach_task_self(), port);
-        }
-      munmap ((caddr_t) ports, num_ports * sizeof (*ports));
-    }
-
-  if (ints && num_ints > 0)
-    munmap ((caddr_t) ints, num_ints * sizeof (*ints));
-  if (offsets && num_offsets > 0)
-    munmap ((caddr_t) offsets, num_offsets * sizeof (*offsets));
-  if (data && data_len > 0)
-    munmap (data, data_len);
-  mach_port_deallocate (mach_task_self (), file);
-#else /* !__GNU__ */
+#ifndef __GNU__
   struct stat st;
   dev_t dev;
 
@@ -1035,6 +1089,7 @@
     grub_util_error (_("cannot stat `%s': %s"), dir, strerror (errno));
 
   dev = st.st_dev;
+#endif /* !__GNU__ */
 
   os_dev = xmalloc (2 * sizeof (os_dev[0]));
   
@@ -1042,6 +1097,10 @@
   /* Cygwin specific function.  */
   os_dev[0] = grub_find_device (dir, dev);
 
+#elif defined __GNU__
+  /* GNU/Hurd specific function.  */
+  os_dev[0] = find_hurd_root_device (dir);
+
 #else
 
   /* This might be truly slow, but is there any better way?  */
@@ -1054,7 +1113,6 @@
     }
 
   os_dev[1] = 0;
-#endif /* !__GNU__ */
 
   return os_dev;
 }

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 24.04.2012 01:34, Samuel Thibault wrote:
> +	  if (errno == ENOENT && part >= 5)
> +	    {
This introduces an assumption that if part >=5 is missing then there are
no additional partitions. While this is true for msdos, it's false for
GPT, where such config is common after deleting a partition. Moreover,
APM (apple) partitions often start at 8.

-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 10:55:21 +0200, a écrit :
> On 24.04.2012 01:34, Samuel Thibault wrote:
> > +	  if (errno == ENOENT && part >= 5)
> > +	    {
> This introduces an assumption that if part >=5 is missing then there are
> no additional partitions. While this is true for msdos, it's false for
> GPT, where such config is common after deleting a partition. Moreover,
> APM (apple) partitions often start at 8.

Ok, but what do you suggest?  By the very nature of parted-based
translators, there is no way to know how many partitions there are.

Samuel

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 24.04.2012 11:00, Samuel Thibault wrote:
> Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 10:55:21 +0200, a écrit :
>> On 24.04.2012 01:34, Samuel Thibault wrote:
>>> +	  if (errno == ENOENT && part >= 5)
>>> +	    {
>> This introduces an assumption that if part >=5 is missing then there are
>> no additional partitions. While this is true for msdos, it's false for
>> GPT, where such config is common after deleting a partition. Moreover,
>> APM (apple) partitions often start at 8.
> Ok, but what do you suggest?  By the very nature of parted-based
> translators, there is no way to know how many partitions there are.
Can we know whether given device is partition? If we know that it is we
can iterate as long as we can, adding some restriction like your 1000
and issuing error if it's hit since it should never happen.

-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 11:12:11 +0200, a écrit :
> On 24.04.2012 11:00, Samuel Thibault wrote:
> > Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 10:55:21 +0200, a écrit :
> >> On 24.04.2012 01:34, Samuel Thibault wrote:
> >>> +	  if (errno == ENOENT && part >= 5)
> >>> +	    {
> >> This introduces an assumption that if part >=5 is missing then there are
> >> no additional partitions. While this is true for msdos, it's false for
> >> GPT, where such config is common after deleting a partition. Moreover,
> >> APM (apple) partitions often start at 8.
> > Ok, but what do you suggest?  By the very nature of parted-based
> > translators, there is no way to know how many partitions there are.
> Can we know whether given device is partition? If we know that it is we
> can iterate as long as we can, adding some restriction like your 1000
> and issuing error if it's hit since it should never happen.

I don't see how knowing whether the device is partition will help to
know where to stop. A user could very well have all partition nodes in
/dev up to 100, they'll all be partitions. Whether they actually exist
is another story, but as you say, you can't assume anything with the
absence of a partition.

Samuel

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 24.04.2012 11:16, Samuel Thibault wrote:
> Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 11:12:11 +0200, a écrit :
>> On 24.04.2012 11:00, Samuel Thibault wrote:
>>> Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 10:55:21 +0200, a écrit :
>>>> On 24.04.2012 01:34, Samuel Thibault wrote:
>>>>> +	  if (errno == ENOENT && part >= 5)
>>>>> +	    {
>>>> This introduces an assumption that if part >=5 is missing then there are
>>>> no additional partitions. While this is true for msdos, it's false for
>>>> GPT, where such config is common after deleting a partition. Moreover,
>>>> APM (apple) partitions often start at 8.
>>> Ok, but what do you suggest?  By the very nature of parted-based
>>> translators, there is no way to know how many partitions there are.
>> Can we know whether given device is partition? If we know that it is we
>> can iterate as long as we can, adding some restriction like your 1000
>> and issuing error if it's hit since it should never happen.
> I don't see how knowing whether the device is partition will help to
> know where to stop. A user could very well have all partition nodes in
> /dev up to 100, they'll all be partitions. Whether they actually exist
> is another story, but as you say, you can't assume anything with the
> absence of a partition.
Theoretically you shouldn't stop until you find the partition if you
know that your device is partition since it means that Hurd knows the
partition in question.
>
> Samuel
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 11:26:46 +0200, a écrit :
> Theoretically you shouldn't stop until you find the partition if you
> know that your device is partition

What do you precisely mean by "device"?

What we don't know is whether the root storage for the file is
partitioned or not.

Samuel

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

Samuel Thibault, le Tue 24 Apr 2012 11:36:27 +0200, a écrit :
> Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 11:26:46 +0200, a écrit :
> > Theoretically you shouldn't stop until you find the partition if you
> > know that your device is partition
> 
> What do you precisely mean by "device"?

I mean "your device", not device in general: what variable are you
talking about in the source.

Samuel

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 24.04.2012 11:38, Samuel Thibault wrote:
> Samuel Thibault, le Tue 24 Apr 2012 11:36:27 +0200, a écrit :
>> Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 11:26:46 +0200, a écrit :
>>> Theoretically you shouldn't stop until you find the partition if you
>>> know that your device is partition
>> What do you precisely mean by "device"?
> I mean "your device", not device in general: what variable are you
> talking about in the source.
I meant: if we know that /boot/grub/grub.cfg resides on a partition then
we can iterate as long as we need to.
> Samuel
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 11:44:35 +0200, a écrit :
> On 24.04.2012 11:38, Samuel Thibault wrote:
> > Samuel Thibault, le Tue 24 Apr 2012 11:36:27 +0200, a écrit :
> >> Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 11:26:46 +0200, a écrit :
> >>> Theoretically you shouldn't stop until you find the partition if you
> >>> know that your device is partition
> >> What do you precisely mean by "device"?
> > I mean "your device", not device in general: what variable are you
> > talking about in the source.
> I meant: if we know that /boot/grub/grub.cfg resides on a partition then
> we can iterate as long as we need to.

We don't know whether grub.cfg resides on a partition.

Samuel

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 24.04.2012 01:34, Samuel Thibault wrote:
> +/* Retrieve the storage information for PATH, and search which partitions
> + * contains its start.  */
> +static char *
> +find_hurd_root_device (const char *path)
> +{
> +  struct store *path_store, *part_store;
> +  char *devpath = NULL;
> +  const char *prefix;
> +  size_t len, prefix_len;
> +  store_offset_t path_start;
> +  int part;
> +
> +  path_store = storeinfo_store (path);
> +  if (! path_store)
> +    grub_util_error ("Could not retrieve storage information for `%s'", path);
> +  if (! path_store->name)
> +    grub_util_error ("No storage name associated with `%s'", path);
grub_util_error messages should be marked for translation. And probably
have a TRANSLATORS comment about the context.
> +
> +  grub_util_info ("Storage is %s, type %d",
> +      path_store->name, path_store->class->id);
> +
> +  if (path_store->class->id == STORAGE_DEVICE)
> +    prefix = "/dev/";
> +  else
> +    prefix = "";
> +
> +  prefix_len = strlen (prefix);
> +  len = strlen (path_store->name);
> +  devpath = xmalloc (prefix_len + len + 4 + 1);
It's better to have an explicit sizeof ("s999") rather than 4 + 1
> +	  if (errno == ENOENT && part >= 5)
> +	    {
Said in previous mail
> +      if (first_run->start * part_store->block_size <= path_start &&
> +	  (last_run->start + last_run->length) * part_store->block_size > path_start) {
Why this weird check rather than just check that start is the same?
Some partitions may contain more partitions, like logical container
contains logical partitions.

-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 11:04:46 +0200, a écrit :
> On 24.04.2012 01:34, Samuel Thibault wrote:
> > +/* Retrieve the storage information for PATH, and search which partitions
> > + * contains its start.  */
> > +static char *
> > +find_hurd_root_device (const char *path)
> > +{
> > +  struct store *path_store, *part_store;
> > +  char *devpath = NULL;
> > +  const char *prefix;
> > +  size_t len, prefix_len;
> > +  store_offset_t path_start;
> > +  int part;
> > +
> > +  path_store = storeinfo_store (path);
> > +  if (! path_store)
> > +    grub_util_error ("Could not retrieve storage information for `%s'", path);
> > +  if (! path_store->name)
> > +    grub_util_error ("No storage name associated with `%s'", path);
> grub_util_error messages should be marked for translation. And probably
> have a TRANSLATORS comment about the context.

Ah, I had only had a look at the existing grub_util_info calls, which
are not marked for translation.

> > +      if (first_run->start * part_store->block_size <= path_start &&
> > +	  (last_run->start + last_run->length) * part_store->block_size > path_start) {
> Why this weird check rather than just check that start is the same?

Because path_start is the start of the file, not of the partition
containing the file.

> Some partitions may contain more partitions, like logical container
> contains logical partitions.

Yes, but we do not have defined /dev namings for these yet, so we don't
know how to look for them grom GRUB yet.

Samuel

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 24.04.2012 11:19, Samuel Thibault wrote:
> Because path_start is the start of the file, not of the partition
> containing the file.
>
>> > Some partitions may contain more partitions, like logical container
>> > contains logical partitions.
> Yes, but we do not have defined /dev namings for these yet, so we don't
> know how to look for them grom GRUB yet.
That 2 comments together open the can of worms. Second one would imply
we should take the smallest partition containing the given file. E.g.
One could have
hd0s1 containing hd0s5 and hd0s6. In this case we want hd0s5 and not
hd0s1. On the other hand a file may reside in partition but be part of a
filesystem spanning through the whole disk. This is common for all kinds
of hybrid CDROMs, including the ones created by grub-mkrescue. They have
iso9660 spanning through the whole disk but all of the disk other than
the first sector is in some kind of partition table to avoid it being
accidentally overwritten. So even though the file itself is inside a
partition, we want the whole disk.
Also if file is 0-sized, sparse, compressed or encrypted it won't have
blocklist and this approach would fail, possibly even with out-of-range
array access.
I'm surprised that Hurd doesn't offer a way to just ask "What does this
filesystem translator consume?"

-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 11:55:33 +0200, a écrit :
> One could have hd0s1 containing hd0s5 and hd0s6.

Argl, I forgot that case indeed. It's funny how that contracts the very
word "partition"...

> In this case we want hd0s5 and not hd0s1. On the other hand a file may
> reside in partition but be part of a filesystem spanning through the
> whole disk. This is common for all kinds of hybrid CDROMs, including
> the ones created by grub-mkrescue. They have iso9660 spanning through
> the whole disk but all of the disk other than the first sector is
> in some kind of partition table to avoid it being accidentally
> overwritten.  So even though the file itself is inside a partition, we
> want the whole disk.

The partition itself can not be mounted?

> Also if file is 0-sized, sparse, compressed or encrypted it won't have
> blocklist and this approach would fail, possibly even with out-of-range
> array access.

Other than 0-sized would work, the eventual storage would still be on
the partition.

0-sized indeed poses problems.

> I'm surprised that Hurd doesn't offer a way to just ask "What does this
> filesystem translator consume?"

Because the whole point of the Hurd is to let the user have access
to more powerful ways.  A file can reside inside an iso file, which
is stored in an ext2fs, which is stored in a file, using the user's
own translator with no constraint on the naming convention, etc. etc.
That's why it generally does not make sense to know where a file comes
from, because you would not know how to parse the result.  The proper
way would be asking the FS, which may tell you

/hurd/ext2fs --writable --no-inherit-dir-group /dev/hd0s1

or

ext2fs --writable --no-atime --no-inherit-dir-group --store-type=typed device:hd1

But we generally don't want to impose any syntax here, it could actually
be

/opt/my/own/translator xyz

I guess we'll have to impose some syntax anyway for whatever contains
/boot, so that grub can open it itself.

Samuel

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 24.04.2012 14:42, Samuel Thibault wrote:
> Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 11:55:33 +0200, a écrit :
>> One could have hd0s1 containing hd0s5 and hd0s6.
> Argl, I forgot that case indeed. It's funny how that contracts the very
> word "partition"...
>
>> In this case we want hd0s5 and not hd0s1. On the other hand a file may
>> reside in partition but be part of a filesystem spanning through the
>> whole disk. This is common for all kinds of hybrid CDROMs, including
>> the ones created by grub-mkrescue. They have iso9660 spanning through
>> the whole disk but all of the disk other than the first sector is
>> in some kind of partition table to avoid it being accidentally
>> overwritten.  So even though the file itself is inside a partition, we
>> want the whole disk.
> The partition itself can not be mounted?
No, since it contains FS structures at wrong offset.
>> Also if file is 0-sized, sparse, compressed or encrypted it won't have
>> blocklist and this approach would fail, possibly even with out-of-range
>> array access.
> Other than 0-sized would work, the eventual storage would still be on
> the partition.
completely sparse just has a size and not backed by any storage.
> 0-sized indeed poses problems.
>
>> I'm surprised that Hurd doesn't offer a way to just ask "What does this
>> filesystem translator consume?"
> Because the whole point of the Hurd is to let the user have access
> to more powerful ways.  A file can reside inside an iso file, which
> is stored in an ext2fs, which is stored in a file,
So much GRUB can handle. We don't handle loopback automatically right
now since it's not clear whether it's a loopback for VM or loopback used
on host.
>  using the user's
> own translator with no constraint on the naming convention, etc. etc.
For these ones we're limited at what GRUB can. grub-probe is used for 2
purposes:
1) Determining the device for root, its FS and UUID
2) Determining the GRUB environment on boot.
The first is used in grub-mkconfig and if Hurd is so much more flexible
10_hurd.in may need to be adjusted accordingly.
As for the second, we're limited to what GRUB can do and so it won't be
possible to have /boot on translator from hyperspace. grub-probe should
error-out if it reaches a translator which can't be handled by GRUB.
> That's why it generally does not make sense to know where a file comes
> from, because you would not know how to parse the result.  The proper
> way would be asking the FS, which may tell you
>
> /hurd/ext2fs --writable --no-inherit-dir-group /dev/hd0s1
>
> or
>
> ext2fs --writable --no-atime --no-inherit-dir-group --store-type=typed device:hd1
>
> But we generally don't want to impose any syntax here, it could actually
> be
>
> /opt/my/own/translator xyz
>
> I guess we'll have to impose some syntax anyway for whatever contains
> /boot, so that grub can open it itself.
There should be a standartised way to get this information for any
conventional FS, otherwise it makes porting programs which use this
information much more difficult and in most cases results in dirty
workarounds.


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 15:19:25 +0200, a écrit :
> On 24.04.2012 14:42, Samuel Thibault wrote:
> > Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 11:55:33 +0200, a écrit :
> >> They have iso9660 spanning through
> >> the whole disk but all of the disk other than the first sector is
> >> in some kind of partition table to avoid it being accidentally
> >> overwritten.  So even though the file itself is inside a partition, we
> >> want the whole disk.
> > The partition itself can not be mounted?
> No, since it contains FS structures at wrong offset.

Ok.

> >> Also if file is 0-sized, sparse, compressed or encrypted it won't have
> >> blocklist and this approach would fail, possibly even with out-of-range
> >> array access.
> > Other than 0-sized would work, the eventual storage would still be on
> > the partition.
> completely sparse just has a size and not backed by any storage.

That's included in "0-size" in my head :)

> > 0-sized indeed poses problems.
> >
> >> I'm surprised that Hurd doesn't offer a way to just ask "What does this
> >> filesystem translator consume?"
> > Because the whole point of the Hurd is to let the user have access
> > to more powerful ways.  A file can reside inside an iso file, which
> > is stored in an ext2fs, which is stored in a file,
> So much GRUB can handle.

But how to express that to GRUB? grub_guess_root_devices only returns
a series of alternative paths.  See below.

> We don't handle loopback automatically right now since it's not clear
> whether it's a loopback for VM or loopback used on host.

You'd need to know which files to open anyway. You don't want to browse
all filesystems for that :) So the OS-specific function has to tell you.

> >  using the user's
> > own translator with no constraint on the naming convention, etc. etc.
> For these ones we're limited at what GRUB can. grub-probe is used for 2
> purposes:
> 1) Determining the device for root, its FS and UUID
> 2) Determining the GRUB environment on boot.
> The first is used in grub-mkconfig and if Hurd is so much more flexible
> 10_hurd.in may need to be adjusted accordingly.

Sure.

> As for the second, we're limited to what GRUB can do and so it won't be
> possible to have /boot on translator from hyperspace.

Sure.  But it can be something expressed in a complex way by the user,
which can actually be reached by GRUB.  That said, as I said earlier, we
can ask the user to refrain himself when it's about /boot.

> > But we generally don't want to impose any syntax here, it could actually
> > be
> >
> > /opt/my/own/translator xyz
> >
> > I guess we'll have to impose some syntax anyway for whatever contains
> > /boot, so that grub can open it itself.
> There should be a standartised way to get this information for any
> conventional FS, otherwise it makes porting programs which use this
> information much more difficult and in most cases results in dirty
> workarounds.

So far, I've mostly seen GRUB really needing that information.

Samuel

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 24.04.2012 19:13, Samuel Thibault wrote:
> Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 15:19:25 +0200, a écrit :
>> On 24.04.2012 14:42, Samuel Thibault wrote:
>>> Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 11:55:33 +0200, a écrit :
>>>> They have iso9660 spanning through
>>>> the whole disk but all of the disk other than the first sector is
>>>> in some kind of partition table to avoid it being accidentally
>>>> overwritten.  So even though the file itself is inside a partition, we
>>>> want the whole disk.
>>> The partition itself can not be mounted?
>> No, since it contains FS structures at wrong offset.
> Ok.
BTW sometimes partition can be mounted but contains another FS which
references the same file by another name or not at all.
>>> 0-sized indeed poses problems.
>>>
>>>> I'm surprised that Hurd doesn't offer a way to just ask "What does this
>>>> filesystem translator consume?"
>>> Because the whole point of the Hurd is to let the user have access
>>> to more powerful ways.  A file can reside inside an iso file, which
>>> is stored in an ext2fs, which is stored in a file,
>> So much GRUB can handle.
> But how to express that to GRUB? grub_guess_root_devices only returns
> a series of alternative paths.  See below.
By just giving the file in question.
But it currently always suppose that the file will be for VM and not
host use.
>> We don't handle loopback automatically right now since it's not clear
>> whether it's a loopback for VM or loopback used on host.
> You'd need to know which files to open anyway. You don't want to browse
> all filesystems for that :) So the OS-specific function has to tell you.
Yes.
>> As for the second, we're limited to what GRUB can do and so it won't be
>> possible to have /boot on translator from hyperspace.
> Sure.  But it can be something expressed in a complex way by the user,
> which can actually be reached by GRUB.  That said, as I said earlier, we
> can ask the user to refrain himself when it's about /boot.
We can add more translators handling as-needed. But this problem isn't
Hurd-specific. A simple GNU/Linux example is unionfs: currently we can't
traverse it even if it refers to something easy. Trouble is that
unionfs/translator reference "foo" can point to different files
depending on circumstances and we need to follow this changes if they
happen behind the grub-mkconfig back (e.g. if unionfs had kernel just in
branch1 and then it was moved to branch2 we would need to handle this
change without rerunning grub-mkconfig).
>>> But we generally don't want to impose any syntax here, it could actually
>>> be
>>>
>>> /opt/my/own/translator xyz
>>>
>>> I guess we'll have to impose some syntax anyway for whatever contains
>>> /boot, so that grub can open it itself.
>> There should be a standartised way to get this information for any
>> conventional FS, otherwise it makes porting programs which use this
>> information much more difficult and in most cases results in dirty
>> workarounds.
> So far, I've mostly seen GRUB really needing that information.
I suppose that databases would want to know this for optimisations.

-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 22:14:33 +0200, a écrit :
> On 24.04.2012 19:13, Samuel Thibault wrote:
> > Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 15:19:25 +0200, a écrit :
> >> On 24.04.2012 14:42, Samuel Thibault wrote:
> >>> 0-sized indeed poses problems.
> >>>
> >>>> I'm surprised that Hurd doesn't offer a way to just ask "What does this
> >>>> filesystem translator consume?"
> >>> Because the whole point of the Hurd is to let the user have access
> >>> to more powerful ways.  A file can reside inside an iso file, which
> >>> is stored in an ext2fs, which is stored in a file,
> >> So much GRUB can handle.
> > But how to express that to GRUB? grub_guess_root_devices only returns
> > a series of alternative paths.  See below.
> By just giving the file in question.

That will not tell you which image has to be mounted to find it.

> >> As for the second, we're limited to what GRUB can do and so it won't be
> >> possible to have /boot on translator from hyperspace.
> > Sure.  But it can be something expressed in a complex way by the user,
> > which can actually be reached by GRUB.  That said, as I said earlier, we
> > can ask the user to refrain himself when it's about /boot.
> We can add more translators handling as-needed.

So would it be acceptable for now as a horrible-but-works solution to
use the output of sysopts:

ext2fs --writable --no-atime --no-inherit-dir-group --store-type=typed device:hd1

and

/hurd/ext2fs --writable --no-inherit-dir-group /dev/hd0s1

Drop the first parameter (translator path), -* options, and that leaves
only the device, with additional s_device:_/dev/_ mangling.

> >>> But we generally don't want to impose any syntax here, it could actually
> >>> be
> >>>
> >>> /opt/my/own/translator xyz
> >>>
> >>> I guess we'll have to impose some syntax anyway for whatever contains
> >>> /boot, so that grub can open it itself.
> >> There should be a standartised way to get this information for any
> >> conventional FS, otherwise it makes porting programs which use this
> >> information much more difficult and in most cases results in dirty
> >> workarounds.
> > So far, I've mostly seen GRUB really needing that information.
> I suppose that databases would want to know this for optimisations.

libstore does provide the best thing for that: the actual area on the
disk, which is what was used in the very first patch already, and which
is used for swap, etc.  But that reveals to be actually too precise for
GRUB.

Samuel

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 25.04.2012 00:21, Samuel Thibault wrote:
> Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 22:14:33 +0200, a écrit :
>> On 24.04.2012 19:13, Samuel Thibault wrote:
>>> Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 15:19:25 +0200, a écrit :
>>>> On 24.04.2012 14:42, Samuel Thibault wrote:
>>>>> 0-sized indeed poses problems.
>>>>>
>>>>>> I'm surprised that Hurd doesn't offer a way to just ask "What does this
>>>>>> filesystem translator consume?"
>>>>> Because the whole point of the Hurd is to let the user have access
>>>>> to more powerful ways.  A file can reside inside an iso file, which
>>>>> is stored in an ext2fs, which is stored in a file,
>>>> So much GRUB can handle.
>>> But how to express that to GRUB? grub_guess_root_devices only returns
>>> a series of alternative paths.  See below.
>> By just giving the file in question.
> That will not tell you which image has to be mounted to find it.
As I said: currently we assume that image is for VM use, so nothing
needs to be specifically mounted. Just return image filename. When the
adssumption is lifted mounting would be handled in
prepare_grub_to_access_device
>>>> As for the second, we're limited to what GRUB can do and so it won't be
>>>> possible to have /boot on translator from hyperspace.
>>> Sure.  But it can be something expressed in a complex way by the user,
>>> which can actually be reached by GRUB.  That said, as I said earlier, we
>>> can ask the user to refrain himself when it's about /boot.
>> We can add more translators handling as-needed.
> So would it be acceptable for now as a horrible-but-works solution to
> use the output of sysopts:
>
> ext2fs --writable --no-atime --no-inherit-dir-group --store-type=typed device:hd1
>
> and
>
> /hurd/ext2fs --writable --no-inherit-dir-group /dev/hd0s1
>
> Drop the first parameter (translator path), -* options, and that leaves
> only the device, with additional s_device:_/dev/_ mangling.
Yes it will, just we need comprehensive error messages on failure and a
comment in code that it's not complete solution.
>>>>> But we generally don't want to impose any syntax here, it could actually
>>>>> be
>>>>>
>>>>> /opt/my/own/translator xyz
>>>>>
>>>>> I guess we'll have to impose some syntax anyway for whatever contains
>>>>> /boot, so that grub can open it itself.
>>>> There should be a standartised way to get this information for any
>>>> conventional FS, otherwise it makes porting programs which use this
>>>> information much more difficult and in most cases results in dirty
>>>> workarounds.
>>> So far, I've mostly seen GRUB really needing that information.
>> I suppose that databases would want to know this for optimisations.
> libstore does provide the best thing for that: the actual area on the
> disk, which is what was used in the very first patch already, and which
> is used for swap, etc.  But that reveals to be actually too precise for
> GRUB.
It's more of it not being stable. You could access all the files by
blocklists but it would fail very quickly
> Samuel
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

Vladimir 'φ-coder/phcoder' Serbinenko, le Wed 25 Apr 2012 00:31:26 +0200, a écrit :
> On 25.04.2012 00:21, Samuel Thibault wrote:
> > Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 22:14:33 +0200, a écrit :
> >> On 24.04.2012 19:13, Samuel Thibault wrote:
> >>> Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 15:19:25 +0200, a écrit :
> >>>> On 24.04.2012 14:42, Samuel Thibault wrote:
> >>>>>> I'm surprised that Hurd doesn't offer a way to just ask "What does this
> >>>>>> filesystem translator consume?"
> >>>>> A file can reside inside an iso file, which
> >>>>> is stored in an ext2fs, which is stored in a file,
> >>>> So much GRUB can handle.
> >>> But how to express that to GRUB? grub_guess_root_devices only returns
> >>> a series of alternative paths.  See below.
> >> By just giving the file in question.
> > That will not tell you which image has to be mounted to find it.
> As I said: currently we assume that image is for VM use, so nothing
> needs to be specifically mounted. Just return image filename.

That would work with only one layer.  See my example above, it has
several layers, and you'd have to know the image filename at each layer.

I'm not saying that GRUB should implement all these layers.  I'm just
explaining why the "what does this translator consume?" is not a trivial
question at all in the Hurd, and thus does not have any interface for
it, and we have to define one that would only work for the trivial
cases.

> > So would it be acceptable for now as a horrible-but-works solution to
> > use the output of sysopts:
> >
> > ext2fs --writable --no-atime --no-inherit-dir-group --store-type=typed device:hd1
> >
> > and
> >
> > /hurd/ext2fs --writable --no-inherit-dir-group /dev/hd0s1
> >
> > Drop the first parameter (translator path), -* options, and that leaves
> > only the device, with additional s_device:_/dev/_ mangling.
> Yes it will, just we need comprehensive error messages on failure and a
> comment in code that it's not complete solution.

Sure.

> >>>>> But we generally don't want to impose any syntax here, it could actually
> >>>>> be
> >>>>>
> >>>>> /opt/my/own/translator xyz
> >>>>>
> >>>>> I guess we'll have to impose some syntax anyway for whatever contains
> >>>>> /boot, so that grub can open it itself.
> >>>> There should be a standartised way to get this information for any
> >>>> conventional FS, otherwise it makes porting programs which use this
> >>>> information much more difficult and in most cases results in dirty
> >>>> workarounds.
> >>> So far, I've mostly seen GRUB really needing that information.
> >> I suppose that databases would want to know this for optimisations.
> > libstore does provide the best thing for that: the actual area on the
> > disk, which is what was used in the very first patch already, and which
> > is used for swap, etc.  But that reveals to be actually too precise for
> > GRUB.
> It's more of it not being stable. You could access all the files by
> blocklists but it would fail very quickly

Yes, because it's too precise, and thus not stable.

Samuel

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 25.04.2012 00:39, Samuel Thibault wrote:
> Vladimir 'φ-coder/phcoder' Serbinenko, le Wed 25 Apr 2012 00:31:26 +0200, a écrit :
>> On 25.04.2012 00:21, Samuel Thibault wrote:
>>> Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 22:14:33 +0200, a écrit :
>>>> On 24.04.2012 19:13, Samuel Thibault wrote:
>>>>> Vladimir 'φ-coder/phcoder' Serbinenko, le Tue 24 Apr 2012 15:19:25 +0200, a écrit :
>>>>>> On 24.04.2012 14:42, Samuel Thibault wrote:
>>>>>>>> I'm surprised that Hurd doesn't offer a way to just ask "What does this
>>>>>>>> filesystem translator consume?"
>>>>>>> A file can reside inside an iso file, which
>>>>>>> is stored in an ext2fs, which is stored in a file,
>>>>>> So much GRUB can handle.
>>>>> But how to express that to GRUB? grub_guess_root_devices only returns
>>>>> a series of alternative paths.  See below.
>>>> By just giving the file in question.
>>> That will not tell you which image has to be mounted to find it.
>> As I said: currently we assume that image is for VM use, so nothing
>> needs to be specifically mounted. Just return image filename.
> That would work with only one layer.  See my example above, it has
> several layers, and you'd have to know the image filename at each layer.
Right now we assume only one layer (VM use), more layers is for the future.
> I'm not saying that GRUB should implement all these layers.  I'm just
> explaining why the "what does this translator consume?" is not a trivial
> question at all in the Hurd, and thus does not have any interface for
> it, and we have to define one that would only work for the trivial
> cases.
We just need to traverse one layer: the FS, and only the case when it's
an FS.

-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

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

Here is an updated patch.

Samuel

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 4978 bytes --]

=== modified file 'util/getroot.c'
--- util/getroot.c	2012-04-22 19:02:55 +0000
+++ util/getroot.c	2012-04-25 01:41:04 +0000
@@ -700,6 +700,69 @@
 
 #elif defined (__GNU__)
 
+static char *
+find_hurd_root_device (const char *path)
+{
+  file_t file;
+  error_t err;
+  char *argz = NULL, *name = NULL, *ret;
+  size_t argz_len = 0;
+  int i;
+
+  file = file_name_lookup (path, 0, 0);
+  if (file == MACH_PORT_NULL)
+    grub_util_error (_("Could not open path `%s'"), path);
+
+  /* This returns catenated 0-terminated strings.  */
+  err = file_get_fs_options (file, &argz, &argz_len);
+  if (err)
+    grub_util_error (_("Could not get filesystem options "
+                       "for path `%s': %s"), path, strerror(err));
+
+  /* Make sure the string is terminated.  */
+  argz[argz_len-1] = 0;
+
+  /* Skip first word (translator path) and options.  */
+  for (i = strlen (&argz[0]) + 1; i < argz_len; i += strlen (&argz[i]) + 1)
+    {
+      if (argz[i] != '-')
+        {
+          /* Non-option.  Only accept one, assumed to be the FS path.  */
+          /* XXX: this should be replaced by an RPC to the translator.  */
+          if (name)
+            /* TRANSLATORS: we expect to get something like
+               /hurd/foobar --option1 --option2=baz /dev/something
+             */
+            grub_util_error (_("Translator `%s' for path `%s' has several "
+                               "non-option words, at least `%s' and `%s'"),
+                               &argz[0], path, name, &argz[i]);
+          name = &argz[i];
+        }
+    }
+
+  if (!name)
+    /* TRANSLATORS: we expect to get something like
+       /hurd/foobar --option1 --option2=baz /dev/something
+     */
+    grub_util_error (_("Translator `%s' for path `%s' is given only options, "
+                       "could not find device part"), &argz[0], path);
+
+  if (!strncmp (name, "device:", strlen ("device:")))
+    {
+      char *dev_name = name + strlen ("device:");
+      size_t size = strlen ("/dev/") + strlen (dev_name) + 1;
+      ret = malloc (size);
+      snprintf (ret, size, "/dev/%s", dev_name);
+    }
+  else if (!strncmp (name, "file:", strlen ("file:")))
+    ret = strdup (name + strlen ("file:"));
+  else
+    ret = strdup (name);
+
+  munmap (argz, argz_len);
+  return ret;
+}
+
 #elif ! defined(__CYGWIN__)
 
 char *
@@ -930,65 +993,7 @@
 grub_guess_root_devices (const char *dir)
 {
   char **os_dev = NULL;
-#ifdef __GNU__
-  file_t file;
-  mach_port_t *ports;
-  int *ints;
-  loff_t *offsets;
-  char *data;
-  error_t err;
-  mach_msg_type_number_t num_ports = 0, num_ints = 0, num_offsets = 0, data_len = 0;
-  size_t name_len;
-
-  file = file_name_lookup (dir, 0, 0);
-  if (file == MACH_PORT_NULL)
-    return 0;
-
-  err = file_get_storage_info (file,
-			       &ports, &num_ports,
-			       &ints, &num_ints,
-			       &offsets, &num_offsets,
-			       &data, &data_len);
-
-  if (num_ints < 1)
-    grub_util_error (_("Storage info for `%s' does not include type"), dir);
-  if (ints[0] != STORAGE_DEVICE)
-    grub_util_error (_("Filesystem of `%s' is not stored on local disk"), dir);
-
-  if (num_ints < 5)
-    grub_util_error (_("Storage info for `%s' does not include name"), dir);
-  name_len = ints[4];
-  if (name_len < data_len)
-    grub_util_error (_("Bogus name length for storage info for `%s'"), dir);
-  if (data[name_len - 1] != '\0')
-    grub_util_error (_("Storage name for `%s' not NUL-terminated"), dir);
-
-  os_dev = xmalloc (2 * sizeof (os_dev[0]));
-  os_dev[0] = xmalloc (sizeof ("/dev/") - 1 + data_len);
-  memcpy (os_dev[0], "/dev/", sizeof ("/dev/") - 1);
-  memcpy (os_dev[0] + sizeof ("/dev/") - 1, data, data_len);
-  os_dev[1] = 0;
-
-  if (ports && num_ports > 0)
-    {
-      mach_msg_type_number_t i;
-      for (i = 0; i < num_ports; i++)
-        {
-	  mach_port_t port = ports[i];
-	  if (port != MACH_PORT_NULL)
-	    mach_port_deallocate (mach_task_self(), port);
-        }
-      munmap ((caddr_t) ports, num_ports * sizeof (*ports));
-    }
-
-  if (ints && num_ints > 0)
-    munmap ((caddr_t) ints, num_ints * sizeof (*ints));
-  if (offsets && num_offsets > 0)
-    munmap ((caddr_t) offsets, num_offsets * sizeof (*offsets));
-  if (data && data_len > 0)
-    munmap (data, data_len);
-  mach_port_deallocate (mach_task_self (), file);
-#else /* !__GNU__ */
+#ifndef __GNU__
   struct stat st;
   dev_t dev;
 
@@ -1035,6 +1040,7 @@
     grub_util_error (_("cannot stat `%s': %s"), dir, strerror (errno));
 
   dev = st.st_dev;
+#endif /* !__GNU__ */
 
   os_dev = xmalloc (2 * sizeof (os_dev[0]));
   
@@ -1042,6 +1048,10 @@
   /* Cygwin specific function.  */
   os_dev[0] = grub_find_device (dir, dev);
 
+#elif defined __GNU__
+  /* GNU/Hurd specific function.  */
+  os_dev[0] = find_hurd_root_device (dir);
+
 #else
 
   /* This might be truly slow, but is there any better way?  */
@@ -1054,7 +1064,6 @@
     }
 
   os_dev[1] = 0;
-#endif /* !__GNU__ */
 
   return os_dev;
 }

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 25.04.2012 01:52, Samuel Thibault wrote:
> +    grub_util_error (_("Could not open path `%s'"), path);
This one should be unified with other similar messages. It would also
have advantage of giving strerror. strerror should be given on all lib
errors.

-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

Vladimir 'φ-coder/phcoder' Serbinenko, le Wed 25 Apr 2012 01:57:54 +0200, a écrit :
> On 25.04.2012 01:52, Samuel Thibault wrote:
> > +    grub_util_error (_("Could not open path `%s'"), path);
> This one should be unified with other similar messages.

Err, in the file I can see various ways: "Unable to", "cannot", "failed
to", "couldn't", "can't".  Which one should be used.

> It would also have advantage of giving strerror.

Oops, indeed.

Samuel

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 25.04.2012 02:27, Samuel Thibault wrote:
> Vladimir 'φ-coder/phcoder' Serbinenko, le Wed 25 Apr 2012 01:57:54 +0200, a écrit :
>> On 25.04.2012 01:52, Samuel Thibault wrote:
>>> +    grub_util_error (_("Could not open path `%s'"), path);
>> This one should be unified with other similar messages.
> Err, in the file I can see various ways: "Unable to", "cannot", "failed
> to", "couldn't", "can't".  Which one should be used.
Where? For open we always use:
msgid "cannot open `%s': %s"

>
>> It would also have advantage of giving strerror.
> Oops, indeed.
>
> Samuel
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

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

Vladimir 'φ-coder/phcoder' Serbinenko, le Wed 25 Apr 2012 10:36:59 +0200, a écrit :
> On 25.04.2012 02:27, Samuel Thibault wrote:
> > Vladimir 'φ-coder/phcoder' Serbinenko, le Wed 25 Apr 2012 01:57:54 +0200, a écrit :
> >> On 25.04.2012 01:52, Samuel Thibault wrote:
> >>> +    grub_util_error (_("Could not open path `%s'"), path);
> >> This one should be unified with other similar messages.
> > Err, in the file I can see various ways: "Unable to", "cannot", "failed
> > to", "couldn't", "can't".  Which one should be used.
> Where?

The util/getroot.c file, the various error conditions.

> For open we always use: msgid "cannot open `%s': %s"

So be it, here is an updated patch.

Samuel

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 4986 bytes --]

=== modified file 'util/getroot.c'
--- util/getroot.c	2012-04-22 19:02:55 +0000
+++ util/getroot.c	2012-04-25 11:36:04 +0000
@@ -700,6 +700,69 @@
 
 #elif defined (__GNU__)
 
+static char *
+find_hurd_root_device (const char *path)
+{
+  file_t file;
+  error_t err;
+  char *argz = NULL, *name = NULL, *ret;
+  size_t argz_len = 0;
+  int i;
+
+  file = file_name_lookup (path, 0, 0);
+  if (file == MACH_PORT_NULL)
+    grub_util_error (_("cannot open `%s': %s"), path, strerror (errno));
+
+  /* This returns catenated 0-terminated strings.  */
+  err = file_get_fs_options (file, &argz, &argz_len);
+  if (err)
+    grub_util_error (_("cannot get filesystem options "
+                       "for path `%s': %s"), path, strerror(err));
+
+  /* Make sure the string is terminated.  */
+  argz[argz_len-1] = 0;
+
+  /* Skip first word (translator path) and options.  */
+  for (i = strlen (&argz[0]) + 1; i < argz_len; i += strlen (&argz[i]) + 1)
+    {
+      if (argz[i] != '-')
+        {
+          /* Non-option.  Only accept one, assumed to be the FS path.  */
+          /* XXX: this should be replaced by an RPC to the translator.  */
+          if (name)
+            /* TRANSLATORS: we expect to get something like
+               /hurd/foobar --option1 --option2=baz /dev/something
+             */
+            grub_util_error (_("Translator `%s' for path `%s' has several "
+                               "non-option words, at least `%s' and `%s'"),
+                               &argz[0], path, name, &argz[i]);
+          name = &argz[i];
+        }
+    }
+
+  if (!name)
+    /* TRANSLATORS: we expect to get something like
+       /hurd/foobar --option1 --option2=baz /dev/something
+     */
+    grub_util_error (_("Translator `%s' for path `%s' is given only options, "
+                       "cannot find device part"), &argz[0], path);
+
+  if (!strncmp (name, "device:", strlen ("device:")))
+    {
+      char *dev_name = name + strlen ("device:");
+      size_t size = strlen ("/dev/") + strlen (dev_name) + 1;
+      ret = malloc (size);
+      snprintf (ret, size, "/dev/%s", dev_name);
+    }
+  else if (!strncmp (name, "file:", strlen ("file:")))
+    ret = strdup (name + strlen ("file:"));
+  else
+    ret = strdup (name);
+
+  munmap (argz, argz_len);
+  return ret;
+}
+
 #elif ! defined(__CYGWIN__)
 
 char *
@@ -930,65 +993,7 @@
 grub_guess_root_devices (const char *dir)
 {
   char **os_dev = NULL;
-#ifdef __GNU__
-  file_t file;
-  mach_port_t *ports;
-  int *ints;
-  loff_t *offsets;
-  char *data;
-  error_t err;
-  mach_msg_type_number_t num_ports = 0, num_ints = 0, num_offsets = 0, data_len = 0;
-  size_t name_len;
-
-  file = file_name_lookup (dir, 0, 0);
-  if (file == MACH_PORT_NULL)
-    return 0;
-
-  err = file_get_storage_info (file,
-			       &ports, &num_ports,
-			       &ints, &num_ints,
-			       &offsets, &num_offsets,
-			       &data, &data_len);
-
-  if (num_ints < 1)
-    grub_util_error (_("Storage info for `%s' does not include type"), dir);
-  if (ints[0] != STORAGE_DEVICE)
-    grub_util_error (_("Filesystem of `%s' is not stored on local disk"), dir);
-
-  if (num_ints < 5)
-    grub_util_error (_("Storage info for `%s' does not include name"), dir);
-  name_len = ints[4];
-  if (name_len < data_len)
-    grub_util_error (_("Bogus name length for storage info for `%s'"), dir);
-  if (data[name_len - 1] != '\0')
-    grub_util_error (_("Storage name for `%s' not NUL-terminated"), dir);
-
-  os_dev = xmalloc (2 * sizeof (os_dev[0]));
-  os_dev[0] = xmalloc (sizeof ("/dev/") - 1 + data_len);
-  memcpy (os_dev[0], "/dev/", sizeof ("/dev/") - 1);
-  memcpy (os_dev[0] + sizeof ("/dev/") - 1, data, data_len);
-  os_dev[1] = 0;
-
-  if (ports && num_ports > 0)
-    {
-      mach_msg_type_number_t i;
-      for (i = 0; i < num_ports; i++)
-        {
-	  mach_port_t port = ports[i];
-	  if (port != MACH_PORT_NULL)
-	    mach_port_deallocate (mach_task_self(), port);
-        }
-      munmap ((caddr_t) ports, num_ports * sizeof (*ports));
-    }
-
-  if (ints && num_ints > 0)
-    munmap ((caddr_t) ints, num_ints * sizeof (*ints));
-  if (offsets && num_offsets > 0)
-    munmap ((caddr_t) offsets, num_offsets * sizeof (*offsets));
-  if (data && data_len > 0)
-    munmap (data, data_len);
-  mach_port_deallocate (mach_task_self (), file);
-#else /* !__GNU__ */
+#ifndef __GNU__
   struct stat st;
   dev_t dev;
 
@@ -1035,6 +1040,7 @@
     grub_util_error (_("cannot stat `%s': %s"), dir, strerror (errno));
 
   dev = st.st_dev;
+#endif /* !__GNU__ */
 
   os_dev = xmalloc (2 * sizeof (os_dev[0]));
   
@@ -1042,6 +1048,10 @@
   /* Cygwin specific function.  */
   os_dev[0] = grub_find_device (dir, dev);
 
+#elif defined __GNU__
+  /* GNU/Hurd specific function.  */
+  os_dev[0] = find_hurd_root_device (dir);
+
 #else
 
   /* This might be truly slow, but is there any better way?  */
@@ -1054,7 +1064,6 @@
     }
 
   os_dev[1] = 0;
-#endif /* !__GNU__ */
 
   return os_dev;
 }

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 25.04.2012 11:37, Samuel Thibault wrote:
> +  /* Make sure the string is terminated.  */
> +  argz[argz_len-1] = 0;
This would fail if argz_len == 0. You have to check this case.
> +
> +  /* Skip first word (translator path) and options.  */
> +  for (i = strlen (&argz[0]) + 1; i < argz_len; i += strlen (&argz[i]) + 1)
> +    {
please use argz and argz + i
> +      if (argz[i] != '-')
> +        {
> +          /* Non-option.  Only accept one, assumed to be the FS path.  */
> +          /* XXX: this should be replaced by an RPC to the translator.  */
> +          if (name)
> +            /* TRANSLATORS: we expect to get something like
> +               /hurd/foobar --option1 --option2=baz /dev/something
> +             */
> +            grub_util_error (_("Translator `%s' for path `%s' has several "
> +                               "non-option words, at least `%s' and `%s'"),
> +                               &argz[0], path, name, &argz[i]);
> +          name = &argz[i];
ditto
> +        }
> +    }
> +
> +  if (!name)
> +    /* TRANSLATORS: we expect to get something like
> +       /hurd/foobar --option1 --option2=baz /dev/something
> +     */
> +    grub_util_error (_("Translator `%s' for path `%s' is given only options, "
> +                       "cannot find device part"), &argz[0], path);
> +
> +  if (!strncmp (name, "device:", strlen ("device:")))
Please use explicit == 0 and use sizeof ("...")-1 instead of strlen on
const string.
> +    {
> +      char *dev_name = name + strlen ("device:");
> +      size_t size = strlen ("/dev/") + strlen (dev_name) + 1;
> +      ret = malloc (size);
> +      snprintf (ret, size, "/dev/%s", dev_name);
stpcpy is more appropriate than snprintf here.

> +    }
> +  else if (!strncmp (name, "file:", strlen ("file:")))
> +    ret = strdup (name + strlen ("file:"));
> +  else
> +    ret = strdup (name);
> +
> +  munmap (argz, argz_len);
> +  return ret;


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Samuel Thibault]

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

Here is an updated patch.

Samuel

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 5266 bytes --]

=== modified file 'util/getroot.c'
--- util/getroot.c	2012-04-22 19:02:55 +0000
+++ util/getroot.c	2012-04-30 20:58:17 +0000
@@ -700,6 +700,75 @@
 
 #elif defined (__GNU__)
 
+static char *
+find_hurd_root_device (const char *path)
+{
+  file_t file;
+  error_t err;
+  char *argz = NULL, *name = NULL, *ret;
+  size_t argz_len = 0;
+  int i;
+
+  file = file_name_lookup (path, 0, 0);
+  if (file == MACH_PORT_NULL)
+    grub_util_error (_("cannot open `%s': %s"), path, strerror (errno));
+
+  /* This returns catenated 0-terminated strings.  */
+  err = file_get_fs_options (file, &argz, &argz_len);
+  if (err)
+    grub_util_error (_("cannot get filesystem options "
+                       "for path `%s': %s"), path, strerror(err));
+  if (argz_len == 0)
+    /* TRANSLATORS: a "translator" is similar to a filesystem, but handled by a
+     * userland daemon.  */
+    grub_util_error (_("translator is empty for path `%s'"), path);
+
+  /* Make sure the string is terminated.  */
+  argz[argz_len-1] = 0;
+
+  /* Skip first word (translator path) and options.  */
+  for (i = strlen (argz) + 1; i < argz_len; i += strlen (argz + i) + 1)
+    {
+      if (argz[i] != '-')
+        {
+          /* Non-option.  Only accept one, assumed to be the FS path.  */
+          /* XXX: this should be replaced by an RPC to the translator.  */
+          if (name)
+            /* TRANSLATORS: we expect to get something like
+               /hurd/foobar --option1 --option2=baz /dev/something
+             */
+            grub_util_error (_("translator `%s' for path `%s' has several "
+                               "non-option words, at least `%s' and `%s'"),
+                               argz, path, name, argz + i);
+          name = argz + i;
+        }
+    }
+
+  if (!name)
+    /* TRANSLATORS: we expect to get something like
+       /hurd/foobar --option1 --option2=baz /dev/something
+     */
+    grub_util_error (_("translator `%s' for path `%s' is given only options, "
+                       "cannot find device part"), argz, path);
+
+  if (strncmp (name, "device:", sizeof ("device:") - 1) == 0)
+    {
+      char *dev_name = name + sizeof ("device:") - 1;
+      size_t size = sizeof ("/dev/") - 1 + strlen (dev_name) + 1;
+      char *next;
+      ret = malloc (size);
+      next = stpncpy (ret, "/dev/", size);
+      stpncpy (next, dev_name, size - (next - ret));
+    }
+  else if (!strncmp (name, "file:", sizeof ("file:") - 1))
+    ret = strdup (name + sizeof ("file:") - 1);
+  else
+    ret = strdup (name);
+
+  munmap (argz, argz_len);
+  return ret;
+}
+
 #elif ! defined(__CYGWIN__)
 
 char *
@@ -930,65 +999,7 @@
 grub_guess_root_devices (const char *dir)
 {
   char **os_dev = NULL;
-#ifdef __GNU__
-  file_t file;
-  mach_port_t *ports;
-  int *ints;
-  loff_t *offsets;
-  char *data;
-  error_t err;
-  mach_msg_type_number_t num_ports = 0, num_ints = 0, num_offsets = 0, data_len = 0;
-  size_t name_len;
-
-  file = file_name_lookup (dir, 0, 0);
-  if (file == MACH_PORT_NULL)
-    return 0;
-
-  err = file_get_storage_info (file,
-			       &ports, &num_ports,
-			       &ints, &num_ints,
-			       &offsets, &num_offsets,
-			       &data, &data_len);
-
-  if (num_ints < 1)
-    grub_util_error (_("Storage info for `%s' does not include type"), dir);
-  if (ints[0] != STORAGE_DEVICE)
-    grub_util_error (_("Filesystem of `%s' is not stored on local disk"), dir);
-
-  if (num_ints < 5)
-    grub_util_error (_("Storage info for `%s' does not include name"), dir);
-  name_len = ints[4];
-  if (name_len < data_len)
-    grub_util_error (_("Bogus name length for storage info for `%s'"), dir);
-  if (data[name_len - 1] != '\0')
-    grub_util_error (_("Storage name for `%s' not NUL-terminated"), dir);
-
-  os_dev = xmalloc (2 * sizeof (os_dev[0]));
-  os_dev[0] = xmalloc (sizeof ("/dev/") - 1 + data_len);
-  memcpy (os_dev[0], "/dev/", sizeof ("/dev/") - 1);
-  memcpy (os_dev[0] + sizeof ("/dev/") - 1, data, data_len);
-  os_dev[1] = 0;
-
-  if (ports && num_ports > 0)
-    {
-      mach_msg_type_number_t i;
-      for (i = 0; i < num_ports; i++)
-        {
-	  mach_port_t port = ports[i];
-	  if (port != MACH_PORT_NULL)
-	    mach_port_deallocate (mach_task_self(), port);
-        }
-      munmap ((caddr_t) ports, num_ports * sizeof (*ports));
-    }
-
-  if (ints && num_ints > 0)
-    munmap ((caddr_t) ints, num_ints * sizeof (*ints));
-  if (offsets && num_offsets > 0)
-    munmap ((caddr_t) offsets, num_offsets * sizeof (*offsets));
-  if (data && data_len > 0)
-    munmap (data, data_len);
-  mach_port_deallocate (mach_task_self (), file);
-#else /* !__GNU__ */
+#ifndef __GNU__
   struct stat st;
   dev_t dev;
 
@@ -1035,6 +1046,7 @@
     grub_util_error (_("cannot stat `%s': %s"), dir, strerror (errno));
 
   dev = st.st_dev;
+#endif /* !__GNU__ */
 
   os_dev = xmalloc (2 * sizeof (os_dev[0]));
   
@@ -1042,6 +1054,10 @@
   /* Cygwin specific function.  */
   os_dev[0] = grub_find_device (dir, dev);
 
+#elif defined __GNU__
+  /* GNU/Hurd specific function.  */
+  os_dev[0] = find_hurd_root_device (dir);
+
 #else
 
   /* This might be truly slow, but is there any better way?  */
@@ -1054,7 +1070,6 @@
     }
 
   os_dev[1] = 0;
-#endif /* !__GNU__ */
 
   return os_dev;
 }

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 30.04.2012 20:58, Samuel Thibault wrote:
> Here is an updated patch.
Looks good go ahead.
> Samuel
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 30.04.2012 20:58, Samuel Thibault wrote:
> Here is an updated patch.
>
> Samuel
Committed.
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH,HURD] Fix grub-probe with userland partition support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 22.04.2012 20:24, Samuel Thibault wrote:
> +	sprintf (devpath + strlen (devpath), "s%d", winner_part + 1);
> +    }
Also:
- It's probably wrong to assume that Hurd partition numbering is the
same as GRUB's, especially if several partmaps are present.
- Don't use x + strlen (x), keep pointer instead, you can easily do it
if you replace strcpy with stpcpy
- sprintf is banned in GRUB code. Use snprintf

-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]