Re: Eliminating nested functions

[Colin D Bennett]

David Miller wrote on Friday 17 April 2009:
> From: Pavel Roskin 
> Date: Fri, 17 Apr 2009 11:54:57 -0400
>
> > I suggest that we eliminate all nested functions.
>
> I support this completely.

Me too.

While I like the idea of nested functions, since they are like closures and 
make a lot of common operations (such as iterating over a collection) a little 
more concise in the source code, you can certainly implement anything without 
nested functions that you can with them.  Probably passing a pointer to a 
local structure is the easiest way to do it in most cases if the iteration 
function needs to access some state, right?

Regards,
Colin


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

[PATCH] bless command

[Vladimir Serbinenko]

Hello, due to request by ams I wrote this. It's an analog of "bless" command
available under OSX rewritten using grub2 fs functions and according to
apple specification of hfs+ on-disk format. This command only update the
blessed folder on a partition it doesn't change which drive is used for
booting. The later will be a separate command. Also you can choose which
volume to boot from by holding option key. Syntax:
hfspbless 
It works only on HFS+ volumes. Also due to the lack of hardware I wasn't
unable to test this "in vivo"
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] bless command

[Vladimir Serbinenko]

Sorry, forgot to attach the file

On Sat, Apr 18, 2009 at 8:59 PM, Vladimir Serbinenko wrote:

> Hello, due to request by ams I wrote this. It's an analog of "bless"
> command available under OSX rewritten using grub2 fs functions and according
> to apple specification of hfs+ on-disk format. This command only update the
> blessed folder on a partition it doesn't change which drive is used for
> booting. The later will be a separate command. Also you can choose which
> volume to boot from by holding option key. Syntax:
> hfspbless 
> It works only on HFS+ volumes. Also due to the lack of hardware I wasn't
> unable to test this "in vivo"
>
>
>
diff --git a/commands/hfspbless.c b/commands/hfspbless.c
new file mode 100644
index 000..318a77c
--- /dev/null
+++ b/commands/hfspbless.c
@@ -0,0 +1,199 @@
+/* hfspbless.c - set the hfs+ boot directory.  */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2003,2005,2007,2008  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static grub_uint64_t inode;
+static char *dirname;
+
+static int find_inode (const char *filename, 
+		   const struct grub_dirhook_info *info)
+{
+  if ((grub_strcmp (dirname, filename) == 0
+   || (info->case_insensitive && grub_strcasecmp (dirname, filename) == 0))
+  && info->dir && info->inodeset)
+inode = info->inode;
+  return 0;
+}
+
+static grub_err_t
+grub_cmd_hfspbless (grub_command_t cmd __attribute__ ((unused)),
+		int argc, char **args)
+{
+  char *device_name;
+  char *path = 0, *tail;
+  int embedded_offset;
+  grub_device_t dev;
+  grub_fs_t fs;
+
+  union {
+struct grub_hfs_sblock hfs;
+struct grub_hfsplus_volheader hfsplus;
+  } volheader;
+
+  if (argc != 1)
+return grub_error (GRUB_ERR_BAD_ARGUMENT, "directory required");
+  device_name = grub_file_get_device_name (args[0]);
+  dev = grub_device_open (device_name);
+
+  path = grub_strchr (args[0], ')');
+  if (! path)
+path = grub_strdup (dirname);
+  else
+path = grub_strdup (path + 1);
+  
+  if (! path || *path == 0 || ! device_name)
+{
+  if (dev)
+	grub_device_close (dev);
+  
+  grub_free (device_name);
+  grub_free (path);
+
+  return grub_error (GRUB_ERR_BAD_ARGUMENT, "invalid argument");
+}
+
+  fs = grub_fs_probe (dev);
+  if (! fs || grub_strcmp (fs->name, "hfsplus") != 0)
+{
+  grub_device_close (dev);
+  grub_free (device_name);
+  grub_free (path);
+
+  return grub_error (GRUB_ERR_BAD_FS, "no suitable FS found");
+}
+
+  tail = path + grub_strlen (path) - 1;
+
+  /* Remove trailing '/'. */
+  while (tail != path && *tail == '/')
+*(tail--) = 0;
+  
+  tail = grub_strrchr (path, '/');
+  inode = 0;
+
+  if (tail)
+{
+  *tail = 0;
+  dirname = tail + 1;
+  
+  (fs->dir) (dev, *path == 0 ?"/":path, find_inode);
+}
+  else
+{
+  dirname = path + 1;
+  (fs->dir) (dev, "/", find_inode);
+}
+  grub_free (path);
+
+  if (inode == 0)
+{
+  grub_device_close (dev);
+  grub_free (device_name);
+  return grub_error (GRUB_ERR_FILE_NOT_FOUND, 
+			 "%s not found or isn't a directory\n",
+			 args[0]);
+}
+
+  /* Read the bootblock.  */
+  grub_disk_read (dev->disk, GRUB_HFSPLUS_SBLOCK, 0, sizeof (volheader),
+		  (char *) &volheader);
+  if (grub_errno)
+{
+  grub_device_close (dev);
+  grub_free (device_name);
+  return grub_errno;
+}
+
+  embedded_offset = 0;
+  if (grub_be_to_cpu16 (volheader.hfs.magic) == GRUB_HFS_MAGIC)
+{
+  int extent_start;
+  int ablk_size;
+  int ablk_start;
+
+  /* See if there's an embedded HFS+ filesystem.  */
+  if (grub_be_to_cpu16 (volheader.hfs.embed_sig) != GRUB_HFSPLUS_MAGIC)
+	{
+	  grub_device_close (dev);
+	  grub_free (device_name);
+	  return grub_errno;
+	}
+
+  /* Calculate the offset needed to translate HFS+ sector numbers.  */
+  extent_start = grub_be_to_cpu16 (volheader.hfs.embed_extent.first_block);
+  ablk_size = grub_be_to_cpu32 (volheader.hfs.blksz);
+  ablk_start = grub_be_to_cpu16 (volheader.hfs.first_block);
+  embedded_offset = (ablk_start
+			 + extent_start
+			 * (ablk_size >> GRUB_DISK_SECTOR_BITS));
+
+  grub_disk_read (dev

Re: [PATCH] bless command

[Isaac Dupree]

Vladimir Serbinenko wrote:
> Syntax:
> hfspbless 
> It works only on HFS+ volumes.

Could it be named 'hfsplusbless' (or possibly 'hfs+bless') (I guess our 
current naming style involves no underscores or other word-separation)?  For 
example, Linux `mount` calls the filesystem `hfsplus`, and I like naming-
consistency.  But feel free to mention counterexamples too!

-Isaac
 


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

Re: Eliminating nested functions

[David Miller]

From: Colin D Bennett 
Date: Sat, 18 Apr 2009 08:57:33 -0700

> Probably passing a pointer to a local structure is the easiest way
> to do it in most cases if the iteration function needs to access
> some state, right?

Right.

The biggest surprise for me, easily, when reading the grub2 sources
for the first time was seeing all of this masterbation with nested
functions.


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

Re: [PATCH] bless command

[Peter Cros]

Hi,

Tested and works on Apple imac81 with Mac OSX 10.5, patch applied to r 2074

grub> hfspbless (hd0,3)/efi

Last login: Sun Apr 19 14:30:23 on console
im81:~ pxw$ bless --info /Volumes/hfsp
finderinfo[0]: 52 => Blessed System Folder is /Volumes/hfsp/efi
finderinfo[1]:  0 => No Blessed System File
finderinfo[2]:  0 => Open-folder linked list empty
finderinfo[3]:  0 => No OS 9 + X blessed 9 folder
finderinfo[4]:  0 => Unused field unset
finderinfo[5]:  0 => No OS 9 + X blessed X folder
64-bit VSDB volume id:  0x0F87F7680B9C5211
im81:~ pxw$

Now it just needs to bless the file in order to boot from grub.efi on Apple
EFI

hfspbless  

This would be VERY useful, making grub.efi boot possible on Apple Mac
without needing Mac OSX or refit.

hfspbless is fine for a name


2009/4/19 Vladimir Serbinenko 

> Hello, due to request by ams I wrote this. It's an analog of "bless"
> command available under OSX rewritten using grub2 fs functions and according
> to apple specification of hfs+ on-disk format. This command only update the
> blessed folder on a partition it doesn't change which drive is used for
> booting. The later will be a separate command. Also you can choose which
> volume to boot from by holding option key. Syntax:
> hfspbless 
> It works only on HFS+ volumes. Also due to the lack of hardware I wasn't
> unable to test this "in vivo"
>
>
>
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>


-- 
Cros (pxw)
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: Eliminating nested functions

[Bean]

Hi,

Yeah, I agree with you. The conversion will take some effort, but it
could payoff in the long run. Perhaps we can achieve this in two
steps:

1, Change nested function definition to accept only one parameter. For
function with multiple parameters, place them in a structure and pass
the pointer. This would eliminate NESTED_FUNC_ATTR, as the regparm
issue won't occur in function with only one parameter.

2. Eliminate nested function. This would be easier after step 1. As we
now pass parameters in a structure, we can append extra variables at
the end, and cast it to the required type. Inside the callback
function, we cast it back to use the extra fields.

On Sat, Apr 18, 2009 at 5:38 AM, Pavel Roskin  wrote:
> On Sat, 2009-04-18 at 03:33 +0800, Bean wrote:
>> Hi,
>>
>> One of the advantage of nested function is to use local variables.
>> Without it, we would need to pass them as global variable, or add
>> custom data pointer in many of the iterate function, which would make
>> the code a lot uglier IMO.
>
> Indeed, I tried to get rid on nested functions in fs/ext2.c, and it
> requires more changes than I expected.
>
> However, my impression is that everything can be written nicely with
> more effort.  Some arguments could be put into the structures we need to
> pass, so the number of arguments doesn't increase.
>
> We should probably try to avoid adding new nested functions in the
> meantime.
>
> --
> Regards,
> Pavel Roskin
>
>
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Bean


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