Re: [PATCH 3/3] grub EFI disk device enumberating

[bibo,mao]

Johan Rydberg wrote:
> Johan Rydberg <address@hidden> writes:
>
>  > This code can be shorter; you only have to compare the lengths. If
>  > they match, you can do a memcmp on the whole device path. 
>
> It could look something like this;
>
> /* Returns zero if device path SUBPATH is a subpath of device path
>    PATH.  */
> static int
> compare_subpath (const grub_efi_device_path_t *subpath,
>                  const grub_efi_device_path_t *path)
> {
>   if (! subpath || ! path)
>     return 1;
>
>   while (1)
>     {
>       int len, ret;
>
>       if (GRUB_EFI_END_ENTIRE_DEVICE_PATH (subpath))
>         return 0;
>       else if (GRUB_EFI_END_ENTIRE_DEVICE_PATH (path))
>         return 1;
>
>       if (GRUB_EFI_DEVICE_PATH_LENGTH (subpath)
>           != GRUB_EFI_DEVICE_PATH_LENGTH (path))
>         return ((int) GRUB_EFI_DEVICE_PATH_LENGTH (subpath)
>                 - (int)  GRUB_EFI_DEVICE_PATH_LENGTH (path));
>
>       len = GRUB_EFI_DEVICE_PATH_LENGTH (path);
>       ret = grub_memcmp (subpath, path, len);
>       if (ret)
>         return ret;
>
>       path = (grub_efi_device_path_t *) ((char *) path + len);
>       subpath = (grub_efi_device_path_t *) ((char *) subpath + len);
>     }
> }
>
> I guess that should work at least, it is not tested.
I tested on my EFI IA32 bios, it works for me, your method is better
than me,  I incorporated your function in my second patch.

thanks
bibo,mao
> In gnufi I have a device_path_iterate function that could be used for
> these kind of things.  Maybe we should bring it in to GRUB2.
>
> /* Iterate nodes of the device path.  *PATHP should be set to point to
>    the path that is to be iterated.  NULL will be returned when the
>    end of the path has been reached.  */
> efi_device_path_t *
> efi_device_path_iterate (efi_device_path_t **pathp)
> {
>   efi_device_path_t *p = *pathp;
>
>   if (EFI_END_ENTIRE_DEVICE_PATH (p))
>     return NULL;
>   else
>     {
>       efi_uint_t len = EFI_DEVICE_PATH_LENGTH (p);
>       *pathp = (efi_device_path_t *) (((char *) p) + len);
>       return p;
>     }
> }
>
> ~j

--- grub2.org/disk/efi/efidisk.c        2006-10-25 12:47:28.000000000 +0800
+++ grub2/disk/efi/efidisk.c    2006-10-25 12:50:19.000000000 +0800
@@ -87,6 +87,39 @@ find_last_device_path (const grub_efi_de
  return p;
}

+/* Returns zero if device path SUBPATH is a subpath of device path
+   PATH.  */
+static int
+compare_subpath (const grub_efi_device_path_t *subpath,
+                 const grub_efi_device_path_t *path)
+{
+  if (! subpath || ! path)
+    return 1;
+
+  while (1)
+    {
+      int len, ret;
+
+      if (GRUB_EFI_END_ENTIRE_DEVICE_PATH (subpath))
+        return 0;
+      else if (GRUB_EFI_END_ENTIRE_DEVICE_PATH (path))
+        return 1;
+
+      if (GRUB_EFI_DEVICE_PATH_LENGTH (subpath)
+          != GRUB_EFI_DEVICE_PATH_LENGTH (path))
+        return ((int) GRUB_EFI_DEVICE_PATH_LENGTH (subpath)
+                - (int)  GRUB_EFI_DEVICE_PATH_LENGTH (path));
+
+      len = GRUB_EFI_DEVICE_PATH_LENGTH (path);
+      ret = grub_memcmp (subpath, path, len);
+      if (ret)
+        return ret;
+
+      path = (grub_efi_device_path_t *) ((char *) path + len);
+      subpath = (grub_efi_device_path_t *) ((char *) subpath + len);
+    }
+}
+
/* Compare device paths.  */
static int
compare_device_paths (const grub_efi_device_path_t *dp1,
@@ -197,44 +230,6 @@ make_devices (void)
  return devices;
}

-/* Find the parent device.  */
-static struct grub_efidisk_data *
-find_parent_device (struct grub_efidisk_data *devices,
-                   struct grub_efidisk_data *d)
-{
-  grub_efi_device_path_t *dp, *ldp;
-  struct grub_efidisk_data *parent;
-  
-  dp = duplicate_device_path (d->device_path);
-  if (! dp)
-    return 0;
-
-  ldp = find_last_device_path (dp);
-  ldp->type = GRUB_EFI_END_DEVICE_PATH_TYPE;
-  ldp->subtype = GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE;
-  ldp->length[0] = sizeof (*ldp);
-  ldp->length[1] = 0;
-
-  for (parent = devices; parent; parent = parent->next)
-    {
-      /* Ignore itself.  */
-      if (parent == d)
-       continue;
-      
-      if (compare_device_paths (parent->device_path, dp) == 0)
-       {
-         /* Found.  */
-         if (! parent->last_device_path)
-           parent = 0;
-	  
-	  break;
-       }
-    }
-
-  grub_free (dp);
-  return parent;
-}
-
static int
iterate_child_devices (struct grub_efidisk_data *devices,
                       struct grub_efidisk_data *d,
@@ -305,63 +300,6 @@ static void
name_devices (struct grub_efidisk_data *devices)
{
  struct grub_efidisk_data *d;
-  
-  /* First, identify devices by media device paths.  */
-  for (d = devices; d; d = d->next)
-    {
-      grub_efi_device_path_t *dp;
-
-      dp = d->last_device_path;
-      if (! dp)
-       continue;
-      
-      if (GRUB_EFI_DEVICE_PATH_TYPE (dp) == GRUB_EFI_MEDIA_DEVICE_PATH_TYPE)
-       {
-         int is_hard_drive = 0;
-	  
-	  switch (GRUB_EFI_DEVICE_PATH_SUBTYPE (dp))
-           {
-           case GRUB_EFI_HARD_DRIVE_DEVICE_PATH_SUBTYPE:
-             is_hard_drive = 1;
-             /* Fall through by intention.  */
-           case GRUB_EFI_CDROM_DEVICE_PATH_SUBTYPE:
-             {
-               struct grub_efidisk_data *parent;
-
-               parent = find_parent_device (devices, d);
-               if (parent)
-                 {
-                   if (is_hard_drive)
-                     {
-#if 0
-                       grub_printf ("adding a hard drive by a partition: ");
-                       grub_print_device_path (parent->device_path);
-#endif
-                       add_device (&hd_devices, parent);
-                     }
-                   else
-                     {
-#if 0
-                       grub_printf ("adding a cdrom by a partition: ");
-                       grub_print_device_path (parent->device_path);
-#endif
-                       add_device (&cd_devices, parent);
-                     }
-		    
-		    /* Mark the parent as used.  */
-                   parent->last_device_path = 0;
-                 }
-             }
-             /* Mark itself as used.  */
-             d->last_device_path = 0;
-             break;
-
-           default:
-             /* For now, ignore the others.  */
-             break;
-           }
-       }
-    }

  /* Let's see what can be added more.  */
  for (d = devices; d; d = d->next)
@@ -374,34 +312,22 @@ name_devices (struct grub_efidisk_data *
        continue;

      m = d->block_io->media;
-      if (m->logical_partition)
-       {
-         /* Only one partition in a non-media device. Assume that this
-            is a floppy drive.  */
-#if 0
-         grub_printf ("adding a floppy by guessing: ");
-         grub_print_device_path (d->device_path);
-#endif
-         add_device (&fd_devices, d);
-       }
-      else if (m->read_only && m->block_size > GRUB_DISK_SECTOR_SIZE)
+      if (GRUB_EFI_DEVICE_PATH_TYPE(dp) == GRUB_EFI_MESSAGING_DEVICE_PATH_TYPE)
        {
-         /* This check is too heuristic, but assume that this is a
-            CDROM drive.  */
-#if 0
-         grub_printf ("adding a cdrom by guessing: ");
-         grub_print_device_path (d->device_path);
-#endif
-         add_device (&cd_devices, d);
+         if (m->read_only && m->block_size > GRUB_DISK_SECTOR_SIZE)
+           {
+             grub_printf("adding a cd by guessing\n");
+             add_device (&cd_devices, d);
+           } else
+           {
+             grub_printf("adding a hd by guessing\n");
+             add_device (&hd_devices, d);
+           }
        }
-      else
+      if (GRUB_EFI_DEVICE_PATH_TYPE(dp) == GRUB_EFI_ACPI_DEVICE_PATH_TYPE)
        {
-         /* The default is a hard drive.  */
-#if 0
-         grub_printf ("adding a hard drive by guessing: ");
-         grub_print_device_path (d->device_path);
-#endif
-         add_device (&hd_devices, d);
+         grub_printf ("adding a floppy by guessing\n");
+         add_device (&fd_devices, d);
        }
    }
}
@@ -734,7 +660,6 @@ grub_efidisk_get_device_name (grub_efi_h
      grub_disk_t parent = 0;
      char *partition_name = 0;
      char *device_name;
-      grub_efi_device_path_t *dup_dp, *dup_ldp;
      grub_efi_hard_drive_device_path_t hd;
      auto int find_parent_disk (const char *name);
      auto int find_partition (grub_disk_t disk, const grub_partition_t part);
@@ -753,7 +678,7 @@ grub_efidisk_get_device_name (grub_efi_h
              struct grub_efidisk_data *d;
	      
	      d = disk->data;
-             if (compare_device_paths (d->device_path, dup_dp) == 0)
+             if (compare_subpath (d->device_path, dp) == 0)
                {
                  parent = disk;
                  return 1;
@@ -778,20 +703,7 @@ grub_efidisk_get_device_name (grub_efi_h
          return 0;
        }

-      /* It is necessary to duplicate the device path so that GRUB
-        can overwrite it.  */
-      dup_dp = duplicate_device_path (dp);
-      if (! dup_dp)
-       return 0;
-
-      dup_ldp = find_last_device_path (dup_dp);
-      dup_ldp->type = GRUB_EFI_END_DEVICE_PATH_TYPE;
-      dup_ldp->subtype = GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE;
-      dup_ldp->length[0] = sizeof (*dup_ldp);
-      dup_ldp->length[1] = 0;
-
      grub_efidisk_iterate (find_parent_disk);
-      grub_free (dup_dp);

      if (! parent)
        return 0;