Re: status grub2 port of grub-legasy map command

[Vladimir 'phcoder' Serbinenko]

+/* Realmode far ptr (2 * 16b) to the previous INT13h handler.  */
+extern grub_uint32_t grub_drivemap_int13_oldhandler;
I prefer it to be 2 variables because of the way they interract so nobody would do something like (char *) grub_drivemap_int13_oldhandler;
+/* The type "void" is used for imported assembly labels, takes no storage and
+   serves just to take the address with &label.  Do NOT put void* here.  */
+/* Start of the handler bundle.  */
+extern void grub_drivemap_int13_handler_base;
The common practice is to use declarations like
extern char grub_drivemap_int13_handler_base[];
it makes pointer arithmetics easy
+typedef struct drivemap_node
+{
+  grub_uint8_t newdrive;
+  grub_uint8_t redirto;
+  struct drivemap_node *next;
+} drivemap_node_t;
Here you could reuse Bean's list functions
+      if (!mapping)
Put a space after !. I know GCS can be PITA but grub has chosen to follow it. Not my decision
+      else            /* Entry was head of list.  */ 
+    map_head = mapping->next; 
put the comment on a separate line 
+  if (!name || *name == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name empty");
This check is unnecessary since your function is static and calling function already ensured that name isn't empty
+  /* Skip the first ( in (hd0) - disk_open wants just the name.  */
+  if (*name == '(')
+    name++;
AFAIK you need to remove ')' as well
It's not necessary to try to open the disk actually because for some reason biosdisk module may be not loaded (e.g. grub may use ata) and loading it may cause nasty effects (e.g. conflict between ata and biosdisk)
Same applies for revparse_biosdisk. And if user wants to map to unexistant disk it's ok. So just use tryparse_diskstring
+  if ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd')
change this to something like
if (! ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd'))
   return ...
It makes code nicer by avoiding unnecessary long else
+      grub_errno = GRUB_ERR_NONE;
No need to set grub_errno explicitely unless you want to ignore an error
+      unsigned long drivenum = grub_strtoul (str + 2, 0, 0);
+      if (grub_errno != GRUB_ERR_NONE || drivenum > 127)
I think it's enough to just check the range
+    { 
+      /* N not a number or out of range.  */ 
+      goto fail; 
Since at fail you have just a return, just put a return here with informative error message
+    } 
+      else
else is unnecessary, just continue with the code
+  if (cmd->state[OPTIDX_LIST].set || 0 == argc)
argc == 0 sounds better. Also moving list function to a separate function is a good idea
+        return grub_error (err, "invalid mapping: non-existent disk"
+                   "or not managed by the BIOS");
If you change error message change error number too. Use either
return err;
or 
return grub_error (GRUB_ERR_..., message);
+      map_head = 0;
+    }
+  else
Use return rather then else
+      return GRUB_ERR_NONE;
+    }
+  else
no need for "else"
+      for (i = 0; i < entries && curentry; ++i, curentry = curentry->next)
You don't need to test for both conditions since theirequivalencyis ensured by preceding code
+  if (0 != grub_drivemap_int13_oldhandler)
Better swap the parts
+                    MODNAME
+                    " -l | -r | [-s] grubdev biosdisk",
Here MODNAME doesn't save any space and makes code less readable. Just write the command name explicitely 
+       push    %bp
+       mov     %sp, %bp
You don't need to change bp. Just use sp. In this case it makes code easier
+       lcall   *%cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler)
+       pop     %bp /* The pushed flags were removed by iret.  */
Use rather:
pushf

lcall   *%cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler)
Also remember to restore original %dl after the call
I'm looking forward for your improved patch
Thank you for your effort
Could you also prepare a changelog entry for it?
2009/5/6 Javier Martín <address@hidden>

Here is a new version of the patch. As you suggested, "grub_symbol_t"
was replaced with "void". Also, drivemap.h no longer exists, its little
content integrated into drivemap.c. Last but not least, I've mostly
adopted your version of the assembly file (indenting, labels, etc.)

--
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

_______________________________________________
Grub-devel mailing list
address@hidden
http://lists.gnu.org/mailman/listinfo/grub-devel

-- 
Regards
Vladimir 'phcoder' Serbinenko