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

["bibo,mao" <bibo.mao@intel.com>]

Johan Rydberg wrote:
> Johan Rydberg <jrydberg@night.trouble.net> 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;