[PATCH] AFS fixes and improvements

[PATCH] AFS fixes and improvements

[Vladimir 'phcoder' Serbinenko]

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

Hello. Currently I'm coding BFS (filesystem of BeOS and Haiku) which
is similar to AFS. So I took the later as codebase. I found some bugs
and incompletenesses in it. Here is the fix. Tested using grub-fstest
on virtual machine image downloaded from Syllable website and then
successfully booted from it using grub-mkrescue image

-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

[-- Attachment #2: afs.diff --]
[-- Type: text/plain, Size: 6314 bytes --]

diff --git a/ChangeLog b/ChangeLog
index e38ebc5..7eb823f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,23 @@
+2009-07-17  Vladimir Serbinenko  <phcoder@gmail.com>
+
+	Fix and improve AtheFS support.
+
+	* fs/afs.c: Fix comments style.
+	(grub_afs_blockrun): Declare as packed.
+	(grub_afs_datastream): Likewise.
+	(grub_afs_bnode): Likewise.
+	(grub_afs_btree): Likewise.
+	(grub_afs_sblock): Likewise.
+	Declare `name' as char.
+	(grub_afs_inode): Declare as packed.
+	Change void *vnode to grub_uint32_t unused.
+	(grub_afs_iterate_dir): Check that key_size is positive.
+	(grub_afs_mount): Don't read superblock twice.
+	(grub_afs_dir): Remove double semicolon.
+	Pass mtime.
+	(grub_afs_label): New function.
+	(grub_afs_fs): Add grub_afs_label.
+
 2009-07-16  Pavel Roskin  <proski@gnu.org>
 
 	* configure.ac: Never add "-c" to CFLAGS.
diff --git a/fs/afs.c b/fs/afs.c
index 1f6e163..10ae931 100644
--- a/fs/afs.c
+++ b/fs/afs.c
@@ -29,7 +29,8 @@
 #define	GRUB_AFS_DIRECT_BLOCK_COUNT	12
 #define	GRUB_AFS_BLOCKS_PER_DI_RUN	4
 
-#define	GRUB_AFS_SBLOCK_MAGIC1	0x41465331	/* AFS1 */
+/* AFS1 in hexadecimal.  */
+#define	GRUB_AFS_SBLOCK_MAGIC1	0x41465331
 #define	GRUB_AFS_SBLOCK_MAGIC2	0xdd121031
 #define	GRUB_AFS_SBLOCK_MAGIC3	0x15b6830e
 
@@ -81,7 +82,7 @@ struct grub_afs_blockrun
   grub_uint32_t group;
   grub_uint16_t start;
   grub_uint16_t len;
-};
+} __attribute__ ((packed));
 
 struct grub_afs_datastream
 {
@@ -92,7 +93,7 @@ struct grub_afs_datastream
   struct grub_afs_blockrun double_indirect;
   grub_afs_off_t max_double_indirect_range;
   grub_afs_off_t size;
-};
+} __attribute__ ((packed));
 
 struct grub_afs_bnode
 {
@@ -102,7 +103,7 @@ struct grub_afs_bnode
   grub_uint32_t key_count;
   grub_uint32_t key_size;
   char key_data[0];
-};
+} __attribute__ ((packed));
 
 struct grub_afs_btree
 {
@@ -111,11 +112,11 @@ struct grub_afs_btree
   grub_uint32_t tree_depth;
   grub_afs_bvalue_t last_node;
   grub_afs_bvalue_t first_free;
-} ;
+} __attribute__ ((packed));
 
 struct grub_afs_sblock
 {
-  grub_uint8_t name[32];
+  char name[32];
   grub_uint32_t magic1;
   grub_uint32_t byte_order;
   grub_uint32_t	block_size;
@@ -124,8 +125,10 @@ struct grub_afs_sblock
   grub_afs_off_t used_blocks;
   grub_uint32_t	inode_size;
   grub_uint32_t	magic2;
-  grub_uint32_t	block_per_group;	// Number of blocks per allocation group (Max 65536)
-  grub_uint32_t	alloc_group_shift;	// Number of bits to shift a group number to get a byte address.
+  /* Number of blocks per allocation group. (Max 65536)  */
+  grub_uint32_t	block_per_group;
+  /* Number of bits to shift a group number to get a byte address.  */
+  grub_uint32_t	alloc_group_shift;
   grub_uint32_t	alloc_group_count;
   grub_uint32_t	flags;
   struct grub_afs_blockrun log_block;
@@ -133,12 +136,15 @@ struct grub_afs_sblock
   grub_uint32_t valid_log_blocks;
   grub_uint32_t log_size;
   grub_uint32_t	magic3;
-  struct grub_afs_blockrun root_dir;	// Root dir inode.
-  struct grub_afs_blockrun deleted_files; // Directory containing files scheduled for deletion.
-  struct grub_afs_blockrun index_dir;	// Directory of index files.
+  /* Root dir inode.  */
+  struct grub_afs_blockrun root_dir;
+  /* Directory containing files scheduled for deletion.  */
+  struct grub_afs_blockrun deleted_files;
+  /* Directory of index files.  */
+  struct grub_afs_blockrun index_dir;
   grub_uint32_t boot_loader_size;
   grub_uint32_t	pad[7];
-};
+}  __attribute__ ((packed));
 
 struct grub_afs_inode
 {
@@ -153,13 +159,14 @@ struct grub_afs_inode
   grub_afs_bigtime modified_time;
   struct grub_afs_blockrun parent;
   struct grub_afs_blockrun attrib_dir;
-  grub_uint32_t index_type;		/* Key data-key only used for index files */
+  /* Key data-key only used for index files. */
+  grub_uint32_t index_type;
   grub_uint32_t inode_size;
-  void* vnode;
+  grub_uint32_t unused;
   struct grub_afs_datastream stream;
   grub_uint32_t	pad[4];
   grub_uint32_t small_data[1];
-};
+} __attribute__ ((packed));
 
 struct grub_fshelp_node
 {
@@ -343,7 +350,7 @@ grub_afs_iterate_dir (grub_fshelp_node_t dir,
 
           key_start = U16 (sb, (cur_key > 0) ? index[cur_key - 1] : 0);
           key_size = U16 (sb, index[cur_key]) - key_start;
-          if (key_size)
+          if (key_size > 0)
             {
               char filename [key_size + 1];
               struct grub_fshelp_node *fdiro;
@@ -470,14 +477,7 @@ grub_afs_mount (grub_disk_t disk)
     goto fail;
 
   if (! grub_afs_validate_sblock (&data->sblock))
-    {
-      if (grub_disk_read (disk, 1 * 2, 0, sizeof (struct grub_afs_sblock),
-                          &data->sblock))
-        goto fail;
-
-      if (! grub_afs_validate_sblock (&data->sblock))
-        goto fail;
-    }
+    goto fail;
 
   data->diropen.data = data;
   data->inode = &data->diropen.inode;
@@ -557,7 +557,7 @@ grub_afs_dir (grub_device_t device, const char *path,
               int (*hook) (const char *filename,
 			   const struct grub_dirhook_info *info))
 {
-  struct grub_afs_data *data = 0;;
+  struct grub_afs_data *data = 0;
   struct grub_fshelp_node *fdiro = 0;
 
   auto int NESTED_FUNC_ATTR iterate (const char *filename,
@@ -571,6 +571,8 @@ grub_afs_dir (grub_device_t device, const char *path,
       struct grub_dirhook_info info;
       grub_memset (&info, 0, sizeof (info));
       info.dir = ((filetype & GRUB_FSHELP_TYPE_MASK) == GRUB_FSHELP_DIR);
+      info.mtimeset = 1;
+      info.mtime = grub_divmod64 (node->inode.modified_time, 1000000, 0);
       grub_free (node);
       return hook (filename, &info);
     }
@@ -598,13 +600,35 @@ grub_afs_dir (grub_device_t device, const char *path,
   return grub_errno;
 }
 
+static grub_err_t
+grub_afs_label (grub_device_t device, char **label)
+{
+  struct grub_afs_data *data;
+  grub_disk_t disk = device->disk;
+
+  grub_dl_ref (my_mod);
+
+  data = grub_afs_mount (disk);
+  if (data)
+    *label = grub_strndup (data->sblock.name, sizeof (data->sblock.name));
+  else
+    *label = NULL;
+
+  grub_dl_unref (my_mod);
+
+  grub_free (data);
+
+  return grub_errno;
+}
+
+
 static struct grub_fs grub_afs_fs = {
   .name = "afs",
   .dir = grub_afs_dir,
   .open = grub_afs_open,
   .read = grub_afs_read,
   .close = grub_afs_close,
-  .label = 0,
+  .label = grub_afs_label,
   .next = 0
 };
 

Re: [PATCH] AFS fixes and improvements

[Vladimir 'phcoder' Serbinenko]

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

Update: added symlink support

On Fri, Jul 17, 2009 at 9:30 PM, Vladimir 'phcoder'
Serbinenko<phcoder@gmail.com> wrote:
> Hello. Currently I'm coding BFS (filesystem of BeOS and Haiku) which
> is similar to AFS. So I took the later as codebase. I found some bugs
> and incompletenesses in it. Here is the fix. Tested using grub-fstest
> on virtual machine image downloaded from Syllable website and then
> successfully booted from it using grub-mkrescue image
>
> --
> Regards
> Vladimir 'phcoder' Serbinenko
>
> Personal git repository: http://repo.or.cz/w/grub2/phcoder.git
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

[-- Attachment #2: afs.diff --]
[-- Type: text/plain, Size: 9294 bytes --]

diff --git a/ChangeLog b/ChangeLog
index e38ebc5..e7e5d4a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,29 @@
+2009-07-17  Vladimir Serbinenko  <phcoder@gmail.com>
+
+	Fix and improve AtheFS support.
+
+	* fs/afs.c: Fix comments style.
+	(grub_afs_blockrun): Declare as packed.
+	(grub_afs_datastream): Likewise.
+	(grub_afs_bnode): Likewise.
+	(grub_afs_btree): Likewise.
+	(grub_afs_sblock): Likewise.
+	Declare `name' as char.
+	(grub_afs_inode): Declare as packed.
+	Change void *vnode to grub_uint32_t unused.
+	(grub_afs_iterate_dir): Check that key_size is positive.
+	(grub_afs_mount): Don't read superblock twice.
+	(grub_afs_dir): Remove double semicolon.
+	Pass mtime.
+	Don't free node in case of error grub_fshelp_find_file already 
+	handles this.
+	(grub_afs_open): Don't free node in case of error 
+	grub_fshelp_find_file already handles this.
+	(grub_afs_label): New function.
+	(grub_afs_fs): Add grub_afs_label.
+	(grub_afs_read_symlink): New function.
+
+
 2009-07-16  Pavel Roskin  <proski@gnu.org>
 
 	* configure.ac: Never add "-c" to CFLAGS.
diff --git a/fs/afs.c b/fs/afs.c
index 1f6e163..a35f41d 100644
--- a/fs/afs.c
+++ b/fs/afs.c
@@ -1,7 +1,7 @@
 /* afs.c - The native AtheOS file-system.  */
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2008  Free Software Foundation, Inc.
+ *  Copyright (C) 2008,2009  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
@@ -29,7 +29,8 @@
 #define	GRUB_AFS_DIRECT_BLOCK_COUNT	12
 #define	GRUB_AFS_BLOCKS_PER_DI_RUN	4
 
-#define	GRUB_AFS_SBLOCK_MAGIC1	0x41465331	/* AFS1 */
+/* AFS1 in hexadecimal.  */
+#define	GRUB_AFS_SBLOCK_MAGIC1	0x41465331
 #define	GRUB_AFS_SBLOCK_MAGIC2	0xdd121031
 #define	GRUB_AFS_SBLOCK_MAGIC3	0x15b6830e
 
@@ -81,7 +82,7 @@ struct grub_afs_blockrun
   grub_uint32_t group;
   grub_uint16_t start;
   grub_uint16_t len;
-};
+} __attribute__ ((packed));
 
 struct grub_afs_datastream
 {
@@ -92,7 +93,7 @@ struct grub_afs_datastream
   struct grub_afs_blockrun double_indirect;
   grub_afs_off_t max_double_indirect_range;
   grub_afs_off_t size;
-};
+} __attribute__ ((packed));
 
 struct grub_afs_bnode
 {
@@ -102,7 +103,7 @@ struct grub_afs_bnode
   grub_uint32_t key_count;
   grub_uint32_t key_size;
   char key_data[0];
-};
+} __attribute__ ((packed));
 
 struct grub_afs_btree
 {
@@ -111,11 +112,11 @@ struct grub_afs_btree
   grub_uint32_t tree_depth;
   grub_afs_bvalue_t last_node;
   grub_afs_bvalue_t first_free;
-} ;
+} __attribute__ ((packed));
 
 struct grub_afs_sblock
 {
-  grub_uint8_t name[32];
+  char name[32];
   grub_uint32_t magic1;
   grub_uint32_t byte_order;
   grub_uint32_t	block_size;
@@ -124,8 +125,10 @@ struct grub_afs_sblock
   grub_afs_off_t used_blocks;
   grub_uint32_t	inode_size;
   grub_uint32_t	magic2;
-  grub_uint32_t	block_per_group;	// Number of blocks per allocation group (Max 65536)
-  grub_uint32_t	alloc_group_shift;	// Number of bits to shift a group number to get a byte address.
+  /* Number of blocks per allocation group. (Max 65536)  */
+  grub_uint32_t	block_per_group;
+  /* Number of bits to shift a group number to get a byte address.  */
+  grub_uint32_t	alloc_group_shift;
   grub_uint32_t	alloc_group_count;
   grub_uint32_t	flags;
   struct grub_afs_blockrun log_block;
@@ -133,12 +136,15 @@ struct grub_afs_sblock
   grub_uint32_t valid_log_blocks;
   grub_uint32_t log_size;
   grub_uint32_t	magic3;
-  struct grub_afs_blockrun root_dir;	// Root dir inode.
-  struct grub_afs_blockrun deleted_files; // Directory containing files scheduled for deletion.
-  struct grub_afs_blockrun index_dir;	// Directory of index files.
+  /* Root dir inode.  */
+  struct grub_afs_blockrun root_dir;
+  /* Directory containing files scheduled for deletion.  */
+  struct grub_afs_blockrun deleted_files;
+  /* Directory of index files.  */
+  struct grub_afs_blockrun index_dir;
   grub_uint32_t boot_loader_size;
   grub_uint32_t	pad[7];
-};
+}  __attribute__ ((packed));
 
 struct grub_afs_inode
 {
@@ -153,13 +159,14 @@ struct grub_afs_inode
   grub_afs_bigtime modified_time;
   struct grub_afs_blockrun parent;
   struct grub_afs_blockrun attrib_dir;
-  grub_uint32_t index_type;		/* Key data-key only used for index files */
+  /* Key data-key only used for index files. */
+  grub_uint32_t index_type;
   grub_uint32_t inode_size;
-  void* vnode;
+  grub_uint32_t unused;
   struct grub_afs_datastream stream;
   grub_uint32_t	pad[4];
   grub_uint32_t small_data[1];
-};
+} __attribute__ ((packed));
 
 struct grub_fshelp_node
 {
@@ -294,6 +301,30 @@ grub_afs_read_file (grub_fshelp_node_t node,
                                 - GRUB_DISK_SECTOR_BITS);
 }
 
+static char *
+grub_afs_read_symlink (grub_fshelp_node_t node)
+{
+  char *ret;
+  struct grub_afs_sblock *sb = &node->data->sblock;
+  grub_afs_off_t size = U64 (sb, node->inode.stream.size);
+
+  if (size == 0)
+    {
+      size = sizeof (node->inode.stream);
+      ret = grub_zalloc (size + 1);
+      if (! ret)
+	return 0;
+      grub_memcpy (ret, (char *) &(node->inode.stream), 
+		   sizeof (node->inode.stream));
+      return ret;
+    }
+  ret = grub_zalloc (size + 1);
+  if (! ret)
+    return 0;
+  grub_afs_read_file (node, 0, 0, size, ret);
+  return ret;
+}
+
 static int
 grub_afs_iterate_dir (grub_fshelp_node_t dir,
                       int NESTED_FUNC_ATTR
@@ -307,7 +338,7 @@ grub_afs_iterate_dir (grub_fshelp_node_t dir,
   struct grub_afs_sblock *sb = &dir->data->sblock;
   int i;
 
-  if ((! dir->inode.stream.size) ||
+  if ((dir->inode.stream.size == 0) ||
       ((U32 (sb, dir->inode.mode) & GRUB_AFS_S_IFMT) != GRUB_AFS_S_IFDIR))
     return 0;
 
@@ -343,7 +374,7 @@ grub_afs_iterate_dir (grub_fshelp_node_t dir,
 
           key_start = U16 (sb, (cur_key > 0) ? index[cur_key - 1] : 0);
           key_size = U16 (sb, index[cur_key]) - key_start;
-          if (key_size)
+          if (key_size > 0)
             {
               char filename [key_size + 1];
               struct grub_fshelp_node *fdiro;
@@ -367,6 +398,8 @@ grub_afs_iterate_dir (grub_fshelp_node_t dir,
                 type = GRUB_FSHELP_DIR;
               else if (mode == GRUB_AFS_S_IFREG)
                 type = GRUB_FSHELP_REG;
+	      else if (mode == GRUB_AFS_S_IFLNK)
+		type = GRUB_FSHELP_SYMLINK;
               else
                 type = GRUB_FSHELP_UNKNOWN;
 
@@ -470,14 +503,7 @@ grub_afs_mount (grub_disk_t disk)
     goto fail;
 
   if (! grub_afs_validate_sblock (&data->sblock))
-    {
-      if (grub_disk_read (disk, 1 * 2, 0, sizeof (struct grub_afs_sblock),
-                          &data->sblock))
-        goto fail;
-
-      if (! grub_afs_validate_sblock (&data->sblock))
-        goto fail;
-    }
+    goto fail;
 
   data->diropen.data = data;
   data->inode = &data->diropen.inode;
@@ -510,7 +536,7 @@ grub_afs_open (struct grub_file *file, const char *name)
     goto fail;
 
   grub_fshelp_find_file (name, &data->diropen, &fdiro, grub_afs_iterate_dir,
-			 0, GRUB_FSHELP_REG);
+			 grub_afs_read_symlink, GRUB_FSHELP_REG);
   if (grub_errno)
     goto fail;
 
@@ -524,8 +550,6 @@ grub_afs_open (struct grub_file *file, const char *name)
   return 0;
 
 fail:
-  if (fdiro != &data->diropen)
-    grub_free (fdiro);
   grub_free (data);
 
   grub_dl_unref (my_mod);
@@ -557,7 +581,7 @@ grub_afs_dir (grub_device_t device, const char *path,
               int (*hook) (const char *filename,
 			   const struct grub_dirhook_info *info))
 {
-  struct grub_afs_data *data = 0;;
+  struct grub_afs_data *data = 0;
   struct grub_fshelp_node *fdiro = 0;
 
   auto int NESTED_FUNC_ATTR iterate (const char *filename,
@@ -571,6 +595,8 @@ grub_afs_dir (grub_device_t device, const char *path,
       struct grub_dirhook_info info;
       grub_memset (&info, 0, sizeof (info));
       info.dir = ((filetype & GRUB_FSHELP_TYPE_MASK) == GRUB_FSHELP_DIR);
+      info.mtimeset = 1;
+      info.mtime = grub_divmod64 (node->inode.modified_time, 1000000, 0);
       grub_free (node);
       return hook (filename, &info);
     }
@@ -582,15 +608,16 @@ grub_afs_dir (grub_device_t device, const char *path,
     goto fail;
 
   grub_fshelp_find_file (path, &data->diropen, &fdiro, grub_afs_iterate_dir,
-			 0, GRUB_FSHELP_DIR);
+			 grub_afs_read_symlink, GRUB_FSHELP_DIR);
   if (grub_errno)
     goto fail;
 
   grub_afs_iterate_dir (fdiro, iterate);
 
- fail:
   if (fdiro != &data->diropen)
     grub_free (fdiro);
+
+ fail:
   grub_free (data);
 
   grub_dl_unref (my_mod);
@@ -598,13 +625,35 @@ grub_afs_dir (grub_device_t device, const char *path,
   return grub_errno;
 }
 
+static grub_err_t
+grub_afs_label (grub_device_t device, char **label)
+{
+  struct grub_afs_data *data;
+  grub_disk_t disk = device->disk;
+
+  grub_dl_ref (my_mod);
+
+  data = grub_afs_mount (disk);
+  if (data)
+    *label = grub_strndup (data->sblock.name, sizeof (data->sblock.name));
+  else
+    *label = NULL;
+
+  grub_dl_unref (my_mod);
+
+  grub_free (data);
+
+  return grub_errno;
+}
+
+
 static struct grub_fs grub_afs_fs = {
   .name = "afs",
   .dir = grub_afs_dir,
   .open = grub_afs_open,
   .read = grub_afs_read,
   .close = grub_afs_close,
-  .label = 0,
+  .label = grub_afs_label,
   .next = 0
 };
 

Re: [PATCH] AFS fixes and improvements

[Pavel Roskin]

Quoting Vladimir 'phcoder' Serbinenko <phcoder@gmail.com>:

> Update: added symlink support
>
> On Fri, Jul 17, 2009 at 9:30 PM, Vladimir 'phcoder'
> Serbinenko<phcoder@gmail.com> wrote:
>> Hello. Currently I'm coding BFS (filesystem of BeOS and Haiku) which
>> is similar to AFS. So I took the later as codebase. I found some bugs
>> and incompletenesses in it. Here is the fix. Tested using grub-fstest
>> on virtual machine image downloaded from Syllable website and then
>> successfully booted from it using grub-mkrescue image

Many changes have no value at all, e.g.:

-#define        GRUB_AFS_SBLOCK_MAGIC1  0x41465331      /* AFS1 */
+/* AFS1 in hexadecimal.  */
+#define        GRUB_AFS_SBLOCK_MAGIC1  0x41465331
  #define        GRUB_AFS_SBLOCK_MAGIC2  0xdd121031
  #define        GRUB_AFS_SBLOCK_MAGIC3  0x15b6830e

The original comment suggested that only the first value made it clear  
that only the first value is AFS1.  Now it's unclear.  There is no  
need to point out that AFS1 is in hexadecimal notation.  It's obvious  
to anyone competent to read C code.  Besides, there is no need to  
single out AFS1.

-  grub_uint32_t index_type;            /* Key data-key only used for  
index files */
+  /* Key data-key only used for index files. */
+  grub_uint32_t index_type;
    grub_uint32_t inode_size;

The same comment applies.

-  if ((! dir->inode.stream.size) ||
+  if ((dir->inode.stream.size == 0) ||

The later is marginally better, but it would be easier to review your  
patches if you don't include such changes.

Double semicolons can be removed in all files, and it doesn't need a review.

It's better to split fixes from the new features.

I don't have afs images around.  It would be great if you test all new  
functionality with valgrind.  It's very good at finding mistakes in  
the code.

-- 
Regards,
Pavel Roskin

Re: [PATCH] AFS fixes and improvements

[Vladimir 'phcoder' Serbinenko]

On Sun, Jul 19, 2009 at 7:11 AM, Pavel Roskin<proski@gnu.org> wrote:
> Quoting Vladimir 'phcoder' Serbinenko <phcoder@gmail.com>:
>
>> Update: added symlink support
>>
>> On Fri, Jul 17, 2009 at 9:30 PM, Vladimir 'phcoder'
>> Serbinenko<phcoder@gmail.com> wrote:
>>>
>>> Hello. Currently I'm coding BFS (filesystem of BeOS and Haiku) which
>>> is similar to AFS. So I took the later as codebase. I found some bugs
>>> and incompletenesses in it. Here is the fix. Tested using grub-fstest
>>> on virtual machine image downloaded from Syllable website and then
>>> successfully booted from it using grub-mkrescue image
>
> Many changes have no value at all, e.g.:
>
> -#define        GRUB_AFS_SBLOCK_MAGIC1  0x41465331      /* AFS1 */
> +/* AFS1 in hexadecimal.  */
> +#define        GRUB_AFS_SBLOCK_MAGIC1  0x41465331
>  #define        GRUB_AFS_SBLOCK_MAGIC2  0xdd121031
>  #define        GRUB_AFS_SBLOCK_MAGIC3  0x15b6830e
>
> The original comment suggested that only the first value made it clear that
> only the first value is AFS1.  Now it's unclear.  There is no need to point
> out that AFS1 is in hexadecimal notation.  It's obvious to anyone competent
> to read C code.  Besides, there is no need to single out AFS1.
>
I thought GCS mandated against putting comments together with field
but I was wrong
> -  if ((! dir->inode.stream.size) ||
> +  if ((dir->inode.stream.size == 0) ||
>
> The later is marginally better, but it would be easier to review your
> patches if you don't include such changes.
It's not a stylistic improvement. dir->inode.stream.size is 64-bit
quantity which will be truncated on 32-bit platform
>
> Double semicolons can be removed in all files, and it doesn't need a review.
I'll remove all wrong double semicolons now
>
> It's better to split fixes from the new features.
On it
>
> I don't have afs images around.  It would be great if you test all new
> functionality with valgrind.  It's very good at finding mistakes in the
> code.
>
I will do. I use the publically-available image from
http://web.syllable.org/pages/get-Syllable.html#emulate
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] AFS fixes and improvements

[Vladimir 'phcoder' Serbinenko]

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

>> It's better to split fixes from the new features.
Attached patches
>>
>> I don't have afs images around.  It would be great if you test all new
>> functionality with valgrind.  It's very good at finding mistakes in the
>> code.
>>
> I will do. I use the publically-available image from
> http://web.syllable.org/pages/get-Syllable.html#emulate
>> --
>> Regards,
>> Pavel Roskin
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>
>
>
>
> --
> Regards
> Vladimir 'phcoder' Serbinenko
>
> Personal git repository: http://repo.or.cz/w/grub2/phcoder.git
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

[-- Attachment #2: afsfix.diff --]
[-- Type: text/plain, Size: 5978 bytes --]

diff --git a/ChangeLog b/ChangeLog
index bfceb23..443e67e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,22 @@
+2009-07-17  Vladimir Serbinenko  <phcoder@gmail.com>
+
+	Fix AtheFS support.
+
+	* fs/afs.c: Fix comments style.
+	(grub_afs_blockrun): Declare as packed.
+	(grub_afs_datastream): Likewise.
+	(grub_afs_bnode): Likewise.
+	(grub_afs_btree): Likewise.
+	(grub_afs_sblock): Likewise.
+	Declare `name' as char.
+	(grub_afs_inode): Declare as packed.
+	Change void *vnode to grub_uint32_t unused.
+	(grub_afs_iterate_dir): Check that key_size is positive.
+	(grub_afs_mount): Don't read superblock twice.
+	(grub_afs_dir): Don't free node in case of errorx
+	grub_fshelp_find_file already handles this.
+	(grub_afs_open): Likewise.
+
 2009-07-19  Vladimir Serbinenko  <phcoder@gmail.com>
 
 	* disk/usbms.c (grub_usbms_transfer): Fix double semicolon.
diff --git a/fs/afs.c b/fs/afs.c
index 832a95c..41aebb4 100644
--- a/fs/afs.c
+++ b/fs/afs.c
@@ -1,7 +1,7 @@
 /* afs.c - The native AtheOS file-system.  */
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2008  Free Software Foundation, Inc.
+ *  Copyright (C) 2008,2009  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
@@ -29,7 +29,7 @@
 #define	GRUB_AFS_DIRECT_BLOCK_COUNT	12
 #define	GRUB_AFS_BLOCKS_PER_DI_RUN	4
 
-#define	GRUB_AFS_SBLOCK_MAGIC1	0x41465331	/* AFS1 */
+#define	GRUB_AFS_SBLOCK_MAGIC1	0x41465331 /* AFS1.  */
 #define	GRUB_AFS_SBLOCK_MAGIC2	0xdd121031
 #define	GRUB_AFS_SBLOCK_MAGIC3	0x15b6830e
 
@@ -81,7 +81,7 @@ struct grub_afs_blockrun
   grub_uint32_t group;
   grub_uint16_t start;
   grub_uint16_t len;
-};
+} __attribute__ ((packed));
 
 struct grub_afs_datastream
 {
@@ -92,7 +92,7 @@ struct grub_afs_datastream
   struct grub_afs_blockrun double_indirect;
   grub_afs_off_t max_double_indirect_range;
   grub_afs_off_t size;
-};
+} __attribute__ ((packed));
 
 struct grub_afs_bnode
 {
@@ -102,7 +102,7 @@ struct grub_afs_bnode
   grub_uint32_t key_count;
   grub_uint32_t key_size;
   char key_data[0];
-};
+} __attribute__ ((packed));
 
 struct grub_afs_btree
 {
@@ -111,7 +111,7 @@ struct grub_afs_btree
   grub_uint32_t tree_depth;
   grub_afs_bvalue_t last_node;
   grub_afs_bvalue_t first_free;
-} ;
+} __attribute__ ((packed));
 
 struct grub_afs_sblock
 {
@@ -124,8 +124,10 @@ struct grub_afs_sblock
   grub_afs_off_t used_blocks;
   grub_uint32_t	inode_size;
   grub_uint32_t	magic2;
-  grub_uint32_t	block_per_group;	// Number of blocks per allocation group (Max 65536)
-  grub_uint32_t	alloc_group_shift;	// Number of bits to shift a group number to get a byte address.
+  grub_uint32_t	block_per_group; /* Number of blocks per allocation
+				    group. (Max 65536)  */
+  grub_uint32_t	alloc_group_shift; /* Number of bits to shift a group
+				      number to get a byte address.  */
   grub_uint32_t	alloc_group_count;
   grub_uint32_t	flags;
   struct grub_afs_blockrun log_block;
@@ -133,12 +135,13 @@ struct grub_afs_sblock
   grub_uint32_t valid_log_blocks;
   grub_uint32_t log_size;
   grub_uint32_t	magic3;
-  struct grub_afs_blockrun root_dir;	// Root dir inode.
-  struct grub_afs_blockrun deleted_files; // Directory containing files scheduled for deletion.
-  struct grub_afs_blockrun index_dir;	// Directory of index files.
+  struct grub_afs_blockrun root_dir; /* Root dir inode.  */
+  struct grub_afs_blockrun deleted_files; /* Directory containing files
+					     scheduled for deletion.  */
+  struct grub_afs_blockrun index_dir; /* Directory of index files.  */
   grub_uint32_t boot_loader_size;
   grub_uint32_t	pad[7];
-};
+}  __attribute__ ((packed));
 
 struct grub_afs_inode
 {
@@ -153,13 +156,13 @@ struct grub_afs_inode
   grub_afs_bigtime modified_time;
   struct grub_afs_blockrun parent;
   struct grub_afs_blockrun attrib_dir;
-  grub_uint32_t index_type;		/* Key data-key only used for index files */
+  grub_uint32_t index_type; /* Key data-key only used for index files. */
   grub_uint32_t inode_size;
-  void* vnode;
+  grub_uint32_t unused;
   struct grub_afs_datastream stream;
   grub_uint32_t	pad[4];
   grub_uint32_t small_data[1];
-};
+} __attribute__ ((packed));
 
 struct grub_fshelp_node
 {
@@ -307,8 +310,8 @@ grub_afs_iterate_dir (grub_fshelp_node_t dir,
   struct grub_afs_sblock *sb = &dir->data->sblock;
   int i;
 
-  if ((! dir->inode.stream.size) ||
-      ((U32 (sb, dir->inode.mode) & GRUB_AFS_S_IFMT) != GRUB_AFS_S_IFDIR))
+  if ((dir->inode.stream.size == 0)
+      || ((U32 (sb, dir->inode.mode) & GRUB_AFS_S_IFMT) != GRUB_AFS_S_IFDIR))
     return 0;
 
   grub_afs_read_file (dir, 0, 0, sizeof (head), (char *) &head);
@@ -343,7 +346,7 @@ grub_afs_iterate_dir (grub_fshelp_node_t dir,
 
           key_start = U16 (sb, (cur_key > 0) ? index[cur_key - 1] : 0);
           key_size = U16 (sb, index[cur_key]) - key_start;
-          if (key_size)
+          if (key_size > 0)
             {
               char filename [key_size + 1];
               struct grub_fshelp_node *fdiro;
@@ -470,14 +473,7 @@ grub_afs_mount (grub_disk_t disk)
     goto fail;
 
   if (! grub_afs_validate_sblock (&data->sblock))
-    {
-      if (grub_disk_read (disk, 1 * 2, 0, sizeof (struct grub_afs_sblock),
-                          &data->sblock))
-        goto fail;
-
-      if (! grub_afs_validate_sblock (&data->sblock))
-        goto fail;
-    }
+    goto fail;
 
   data->diropen.data = data;
   data->inode = &data->diropen.inode;
@@ -524,8 +520,6 @@ grub_afs_open (struct grub_file *file, const char *name)
   return 0;
 
 fail:
-  if (fdiro != &data->diropen)
-    grub_free (fdiro);
   grub_free (data);
 
   grub_dl_unref (my_mod);
@@ -588,9 +582,10 @@ grub_afs_dir (grub_device_t device, const char *path,
 
   grub_afs_iterate_dir (fdiro, iterate);
 
- fail:
   if (fdiro != &data->diropen)
     grub_free (fdiro);
+
+ fail:
   grub_free (data);
 
   grub_dl_unref (my_mod);

[-- Attachment #3: afs.diff --]
[-- Type: text/plain, Size: 3807 bytes --]

diff --git a/ChangeLog b/ChangeLog
index 443e67e..f69e332 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,18 @@
 2009-07-17  Vladimir Serbinenko  <phcoder@gmail.com>
 
+	Add symlink, mtime and label support to AtheFS.
+
+	* fs/afs.c (grub_afs_sblock): Declare `name' as char.
+	(grub_afs_iterate_dir): Handle symlinks.
+	(grub_afs_open): Use grub_afs_read_symlink.
+	(grub_afs_dir): Likewise.
+	Pass mtime.
+	(grub_afs_label): New function.
+	(grub_afs_fs): Add grub_afs_label.
+	(grub_afs_read_symlink): New function.
+
+2009-07-17  Vladimir Serbinenko  <phcoder@gmail.com>
+
 	Fix AtheFS support.
 
 	* fs/afs.c: Fix comments style.
diff --git a/fs/afs.c b/fs/afs.c
index 41aebb4..a036cd3 100644
--- a/fs/afs.c
+++ b/fs/afs.c
@@ -115,7 +115,7 @@ struct grub_afs_btree
 
 struct grub_afs_sblock
 {
-  grub_uint8_t name[32];
+  char name[32];
   grub_uint32_t magic1;
   grub_uint32_t byte_order;
   grub_uint32_t	block_size;
@@ -297,6 +297,30 @@ grub_afs_read_file (grub_fshelp_node_t node,
                                 - GRUB_DISK_SECTOR_BITS);
 }
 
+static char *
+grub_afs_read_symlink (grub_fshelp_node_t node)
+{
+  char *ret;
+  struct grub_afs_sblock *sb = &node->data->sblock;
+  grub_afs_off_t size = U64 (sb, node->inode.stream.size);
+
+  if (size == 0)
+    {
+      size = sizeof (node->inode.stream);
+      ret = grub_zalloc (size + 1);
+      if (! ret)
+	return 0;
+      grub_memcpy (ret, (char *) &(node->inode.stream),
+		   sizeof (node->inode.stream));
+      return ret;
+    }
+  ret = grub_zalloc (size + 1);
+  if (! ret)
+    return 0;
+  grub_afs_read_file (node, 0, 0, size, ret);
+  return ret;
+}
+
 static int
 grub_afs_iterate_dir (grub_fshelp_node_t dir,
                       int NESTED_FUNC_ATTR
@@ -370,6 +394,8 @@ grub_afs_iterate_dir (grub_fshelp_node_t dir,
                 type = GRUB_FSHELP_DIR;
               else if (mode == GRUB_AFS_S_IFREG)
                 type = GRUB_FSHELP_REG;
+	      else if (mode == GRUB_AFS_S_IFLNK)
+		type = GRUB_FSHELP_SYMLINK;
               else
                 type = GRUB_FSHELP_UNKNOWN;
 
@@ -506,7 +532,7 @@ grub_afs_open (struct grub_file *file, const char *name)
     goto fail;
 
   grub_fshelp_find_file (name, &data->diropen, &fdiro, grub_afs_iterate_dir,
-			 0, GRUB_FSHELP_REG);
+			 grub_afs_read_symlink, GRUB_FSHELP_REG);
   if (grub_errno)
     goto fail;
 
@@ -565,6 +591,9 @@ grub_afs_dir (grub_device_t device, const char *path,
       struct grub_dirhook_info info;
       grub_memset (&info, 0, sizeof (info));
       info.dir = ((filetype & GRUB_FSHELP_TYPE_MASK) == GRUB_FSHELP_DIR);
+      info.mtimeset = 1;
+      info.mtime = grub_divmod64 (U64 (&data->sblock,
+				       node->inode.modified_time), 1000000, 0);
       grub_free (node);
       return hook (filename, &info);
     }
@@ -576,7 +605,7 @@ grub_afs_dir (grub_device_t device, const char *path,
     goto fail;
 
   grub_fshelp_find_file (path, &data->diropen, &fdiro, grub_afs_iterate_dir,
-			 0, GRUB_FSHELP_DIR);
+			 grub_afs_read_symlink, GRUB_FSHELP_DIR);
   if (grub_errno)
     goto fail;
 
@@ -593,13 +622,35 @@ grub_afs_dir (grub_device_t device, const char *path,
   return grub_errno;
 }
 
+static grub_err_t
+grub_afs_label (grub_device_t device, char **label)
+{
+  struct grub_afs_data *data;
+  grub_disk_t disk = device->disk;
+
+  grub_dl_ref (my_mod);
+
+  data = grub_afs_mount (disk);
+  if (data)
+    *label = grub_strndup (data->sblock.name, sizeof (data->sblock.name));
+  else
+    *label = NULL;
+
+  grub_dl_unref (my_mod);
+
+  grub_free (data);
+
+  return grub_errno;
+}
+
+
 static struct grub_fs grub_afs_fs = {
   .name = "afs",
   .dir = grub_afs_dir,
   .open = grub_afs_open,
   .read = grub_afs_read,
   .close = grub_afs_close,
-  .label = 0,
+  .label = grub_afs_label,
   .next = 0
 };
 

Re: [PATCH] AFS fixes and improvements

[Pavel Roskin]

On Sun, 2009-07-19 at 17:34 +0200, Vladimir 'phcoder' Serbinenko wrote:
> >> It's better to split fixes from the new features.
> Attached patches
> >>
> >> I don't have afs images around.  It would be great if you test all new
> >> functionality with valgrind.  It's very good at finding mistakes in the
> >> code.
> >>
> > I will do. I use the publically-available image from
> > http://web.syllable.org/pages/get-Syllable.html#emulate

I could not use those images directly, but I installed Syllable on a
virtual hard drive under qemu.  I could successfully read the image with
the patches.  I tried grub-fstest under valgrind, and it was fine.

Thanks for the patches!

-- 
Regards,
Pavel Roskin

Re: [PATCH] AFS fixes and improvements

[Vladimir 'phcoder' Serbinenko]

On Sun, Jul 19, 2009 at 9:56 PM, Pavel Roskin<proski@gnu.org> wrote:
> On Sun, 2009-07-19 at 17:34 +0200, Vladimir 'phcoder' Serbinenko wrote:
>> >> It's better to split fixes from the new features.
>> Attached patches
>> >>
>> >> I don't have afs images around.  It would be great if you test all new
>> >> functionality with valgrind.  It's very good at finding mistakes in the
>> >> code.
>> >>
>> > I will do. I use the publically-available image from
>> > http://web.syllable.org/pages/get-Syllable.html#emulate
>
> I could not use those images directly, but I installed Syllable on a
> virtual hard drive under qemu.
Sorry, I'm so accustomed to convert vmware image to raw images before
mounting them or using grub-fstest. To do so use:
> qemu-img convert image.vmdk image.img
> I could successfully read the image with
> the patches.  I tried grub-fstest under valgrind, and it was fine.
Yes, I did it too and valgrind only issued some warnings about my -O3
libraries and found a memory leak caused by pc_partition_map_parse.
This function anyway will be removed with my nested partition patch
(BTW I rediffed and resent it and it works fine here (plain and bsd
configurations). Tomorrow I'll test it with opensolaris).
>
> Thanks for the patches!
>
You're welcome
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] AFS fixes and improvements

[Pavel Roskin]

On Sun, 2009-07-19 at 22:23 +0200, Vladimir 'phcoder' Serbinenko wrote:

> Yes, I did it too and valgrind only issued some warnings about my -O3
> libraries and found a memory leak caused by pc_partition_map_parse.

I know.

> This function anyway will be removed with my nested partition patch
> (BTW I rediffed and resent it and it works fine here (plain and bsd
> configurations). Tomorrow I'll test it with opensolaris).

I prefer not to fix minor memory leaks by huge patches.  I have a patch,
I just wanted to look at the possibility to use grub_kzalloc() and
allocating data as part of the partition.

Anyway, here is the preliminary version.

Fix memory leak in grub_disk_close()

	* kern/disk.c (grub_disk_close): Free disk->partition->data.
	* partmap/acorn.c (acorn_partition_map_probe): Set data to NULL.
	* partmap/amiga.c (amiga_partition_map_iterate): Likewise.
	* partmap/apple.c (apple_partition_map_iterate): Likewise.
	* partmap/gpt.c (gpt_partition_map_iterate): Likewise.  We never
	use it.
---
 kern/disk.c     |    3 +++
 partmap/acorn.c |    1 +
 partmap/amiga.c |    1 +
 partmap/apple.c |    1 +
 partmap/gpt.c   |    2 +-
 5 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/kern/disk.c b/kern/disk.c
index e463626..e6d6ab6 100644
--- a/kern/disk.c
+++ b/kern/disk.c
@@ -338,6 +338,9 @@ grub_disk_close (grub_disk_t disk)
   /* Reset the timer.  */
   grub_last_time = grub_get_time_ms ();
 
+  if (disk->partition)
+    grub_free (disk->partition->data);
+
   grub_free (disk->partition);
   grub_free ((void *) disk->name);
   grub_free (disk);
diff --git a/partmap/acorn.c b/partmap/acorn.c
index 42fd61f..ef12043 100644
--- a/partmap/acorn.c
+++ b/partmap/acorn.c
@@ -164,6 +164,7 @@ acorn_partition_map_probe (grub_disk_t disk, const char *str)
   p->len = map[partnum].size;
   p->offset = 6;
   p->index = partnum;
+  p->data = NULL;
   return p;
 
 fail:
diff --git a/partmap/amiga.c b/partmap/amiga.c
index ffb807f..91289d4 100644
--- a/partmap/amiga.c
+++ b/partmap/amiga.c
@@ -124,6 +124,7 @@ amiga_partition_map_iterate (grub_disk_t disk,
 
       part.offset = (grub_off_t) next * 512;
       part.index = partno;
+      part.data = NULL;
       part.partmap = &grub_amiga_partition_map;
 
       if (hook (disk, &part))
diff --git a/partmap/apple.c b/partmap/apple.c
index fce2f2c..6d673ad 100644
--- a/partmap/apple.c
+++ b/partmap/apple.c
@@ -146,6 +146,7 @@ apple_partition_map_iterate (grub_disk_t disk,
       part.len = grub_be_to_cpu32 (apart.blockcnt);
       part.offset = pos;
       part.index = partno;
+      part.data = NULL;
 
       grub_dprintf ("partition",
 		    "partition %d: name %s, type %s, start 0x%x, len 0x%x\n",
diff --git a/partmap/gpt.c b/partmap/gpt.c
index d646d41..35a979d 100644
--- a/partmap/gpt.c
+++ b/partmap/gpt.c
@@ -92,7 +92,7 @@ gpt_partition_map_iterate (grub_disk_t disk,
 	  part.offset = entries;
 	  part.index = i;
 	  part.partmap = &grub_gpt_partition_map;
-	  part.data = &entry;
+	  part.data = NULL;
 
 	  grub_dprintf ("gpt", "GPT entry %d: start=%lld, length=%lld\n", i,
 			(unsigned long long) part.start,


-- 
Regards,
Pavel Roskin

Re: [PATCH] AFS fixes and improvements

[Vladimir 'phcoder' Serbinenko]

> I prefer not to fix minor memory leaks by huge patches.  I have a patch,
> I just wanted to look at the possibility to use grub_kzalloc() and
> allocating data as part of the partition.
Well nested partition patch changes the way how partitions are handled
and fix for memory leak probably wouldn't stay long and will just
cause an unnecessary rediffing. I prefer to commit a big patch and
then look if memory leak remains and if it's the case fix the memory
leak
>
> Anyway, here is the preliminary version.
>
> Fix memory leak in grub_disk_close()
>
>        * kern/disk.c (grub_disk_close): Free disk->partition->data.
>        * partmap/acorn.c (acorn_partition_map_probe): Set data to NULL.
>        * partmap/amiga.c (amiga_partition_map_iterate): Likewise.
>        * partmap/apple.c (apple_partition_map_iterate): Likewise.
>        * partmap/gpt.c (gpt_partition_map_iterate): Likewise.  We never
>        use it.
Wrong. It's used in grub-setup.c. I already stepped into this pitfall
myself. I'll check if  I actually sent the last version (same as in my
repo) of nested partition patch
> ---
>  kern/disk.c     |    3 +++
>  partmap/acorn.c |    1 +
>  partmap/amiga.c |    1 +
>  partmap/apple.c |    1 +
>  partmap/gpt.c   |    2 +-
>  5 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/kern/disk.c b/kern/disk.c
> index e463626..e6d6ab6 100644
> --- a/kern/disk.c
> +++ b/kern/disk.c
> @@ -338,6 +338,9 @@ grub_disk_close (grub_disk_t disk)
>   /* Reset the timer.  */
>   grub_last_time = grub_get_time_ms ();
>
> +  if (disk->partition)
> +    grub_free (disk->partition->data);
> +
>   grub_free (disk->partition);
>   grub_free ((void *) disk->name);
>   grub_free (disk);
> diff --git a/partmap/acorn.c b/partmap/acorn.c
> index 42fd61f..ef12043 100644
> --- a/partmap/acorn.c
> +++ b/partmap/acorn.c
> @@ -164,6 +164,7 @@ acorn_partition_map_probe (grub_disk_t disk, const char *str)
>   p->len = map[partnum].size;
>   p->offset = 6;
>   p->index = partnum;
> +  p->data = NULL;
>   return p;
>
>  fail:
> diff --git a/partmap/amiga.c b/partmap/amiga.c
> index ffb807f..91289d4 100644
> --- a/partmap/amiga.c
> +++ b/partmap/amiga.c
> @@ -124,6 +124,7 @@ amiga_partition_map_iterate (grub_disk_t disk,
>
>       part.offset = (grub_off_t) next * 512;
>       part.index = partno;
> +      part.data = NULL;
>       part.partmap = &grub_amiga_partition_map;
>
>       if (hook (disk, &part))
> diff --git a/partmap/apple.c b/partmap/apple.c
> index fce2f2c..6d673ad 100644
> --- a/partmap/apple.c
> +++ b/partmap/apple.c
> @@ -146,6 +146,7 @@ apple_partition_map_iterate (grub_disk_t disk,
>       part.len = grub_be_to_cpu32 (apart.blockcnt);
>       part.offset = pos;
>       part.index = partno;
> +      part.data = NULL;
>
>       grub_dprintf ("partition",
>                    "partition %d: name %s, type %s, start 0x%x, len 0x%x\n",
> diff --git a/partmap/gpt.c b/partmap/gpt.c
> index d646d41..35a979d 100644
> --- a/partmap/gpt.c
> +++ b/partmap/gpt.c
> @@ -92,7 +92,7 @@ gpt_partition_map_iterate (grub_disk_t disk,
>          part.offset = entries;
>          part.index = i;
>          part.partmap = &grub_gpt_partition_map;
> -         part.data = &entry;
> +         part.data = NULL;
>
>          grub_dprintf ("gpt", "GPT entry %d: start=%lld, length=%lld\n", i,
>                        (unsigned long long) part.start,
>
>
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] AFS fixes and improvements

[Pavel Roskin]

On Sun, 2009-07-19 at 22:40 +0200, Vladimir 'phcoder' Serbinenko wrote:
> > I prefer not to fix minor memory leaks by huge patches.  I have a patch,
> > I just wanted to look at the possibility to use grub_kzalloc() and
> > allocating data as part of the partition.
> Well nested partition patch changes the way how partitions are handled
> and fix for memory leak probably wouldn't stay long and will just
> cause an unnecessary rediffing. I prefer to commit a big patch and
> then look if memory leak remains and if it's the case fix the memory
> leak

I think we should release 1.97 first.  Sorry, I know that it's bad to
sit on a pile of code, but in my opinion, the nested partition patch is
too intrusive at this point.

I was planning a detailed review of that patch, but I didn't have a
chance.

-- 
Regards,
Pavel Roskin

Re: [PATCH] AFS fixes and improvements

[Vladimir 'phcoder' Serbinenko]

On Sun, Jul 19, 2009 at 10:47 PM, Pavel Roskin<proski@gnu.org> wrote:
> On Sun, 2009-07-19 at 22:40 +0200, Vladimir 'phcoder' Serbinenko wrote:
>> > I prefer not to fix minor memory leaks by huge patches.  I have a patch,
>> > I just wanted to look at the possibility to use grub_kzalloc() and
>> > allocating data as part of the partition.
>> Well nested partition patch changes the way how partitions are handled
>> and fix for memory leak probably wouldn't stay long and will just
>> cause an unnecessary rediffing. I prefer to commit a big patch and
>> then look if memory leak remains and if it's the case fix the memory
>> leak
>
> I think we should release 1.97 first.  Sorry, I know that it's bad to
> sit on a pile of code, but in my opinion, the nested partition patch is
> too intrusive at this point.
>
For the release we prepared a lot of improvements for FreeBSD support.
Hopefully it will attract new FreeBSD users. Supporting nested
partitions required to abandon the syntax like (hd0,a) even if the
syntax (hd0,1,a) could be preserved. If we release 1.97 now without
nested partition patch it would be much more difficult in the future
to break the compatibility with (hd0,a) format. So I would prefer to
get nested partition patch in first before releasing 1.97
> I was planning a detailed review of that patch, but I didn't have a
> chance.
>
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] AFS fixes and improvements

[Pavel Roskin]

On Mon, 2009-07-20 at 11:36 +0200, Vladimir 'phcoder' Serbinenko wrote:
> On Sun, Jul 19, 2009 at 10:47 PM, Pavel Roskin<proski@gnu.org> wrote:
> > I think we should release 1.97 first.  Sorry, I know that it's bad to
> > sit on a pile of code, but in my opinion, the nested partition patch is
> > too intrusive at this point.
> >
> For the release we prepared a lot of improvements for FreeBSD support.
> Hopefully it will attract new FreeBSD users. Supporting nested
> partitions required to abandon the syntax like (hd0,a) even if the
> syntax (hd0,1,a) could be preserved. If we release 1.97 now without
> nested partition patch it would be much more difficult in the future
> to break the compatibility with (hd0,a) format. So I would prefer to
> get nested partition patch in first before releasing 1.97

I don't think making another release would change the situation in any
way.  Version 1.96 supported the (hd0,a) format, and that wasn't a
problem.

If you want, we could just disable the (hd0,a) format now without
actually changing the internals.

-- 
Regards,
Pavel Roskin

Re: [PATCH] AFS fixes and improvements

[Vladimir 'phcoder' Serbinenko]

On Mon, Jul 20, 2009 at 8:34 PM, Pavel Roskin<proski@gnu.org> wrote:
> On Mon, 2009-07-20 at 11:36 +0200, Vladimir 'phcoder' Serbinenko wrote:
>> On Sun, Jul 19, 2009 at 10:47 PM, Pavel Roskin<proski@gnu.org> wrote:
>> > I think we should release 1.97 first.  Sorry, I know that it's bad to
>> > sit on a pile of code, but in my opinion, the nested partition patch is
>> > too intrusive at this point.
>> >
>> For the release we prepared a lot of improvements for FreeBSD support.
>> Hopefully it will attract new FreeBSD users. Supporting nested
>> partitions required to abandon the syntax like (hd0,a) even if the
>> syntax (hd0,1,a) could be preserved. If we release 1.97 now without
>> nested partition patch it would be much more difficult in the future
>> to break the compatibility with (hd0,a) format. So I would prefer to
>> get nested partition patch in first before releasing 1.97
>
> I don't think making another release would change the situation in any
> way.  Version 1.96 supported the (hd0,a) format, and that wasn't a
> problem.
>
Version 1.96 isn't very usable for freebsd. You basically need
chainload FreeBSD's bootloader to get anything more than module-less
32-bit boot. Now situation has changed
> If you want, we could just disable the (hd0,a) format now without
> actually changing the internals.
I'm ok with it in condition that pre-release freeze period won't last
long. Now Marco Gerards is just back from vacation and I propose to
wait for his statements rather than speaking about virtual release
>
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] AFS fixes and improvements

[Pavel Roskin]

On Mon, 2009-07-20 at 20:56 +0200, Vladimir 'phcoder' Serbinenko wrote:

> I'm ok with it in condition that pre-release freeze period won't last
> long. Now Marco Gerards is just back from vacation and I propose to
> wait for his statements rather than speaking about virtual release

OK

-- 
Regards,
Pavel Roskin

Re: [PATCH] AFS fixes and improvements

[Pavel Roskin]

On Sun, 2009-07-19 at 15:20 +0200, Vladimir 'phcoder' Serbinenko wrote:

> > -  if ((! dir->inode.stream.size) ||
> > +  if ((dir->inode.stream.size == 0) ||
> >
> > The later is marginally better, but it would be easier to review your
> > patches if you don't include such changes.
> It's not a stylistic improvement. dir->inode.stream.size is 64-bit
> quantity which will be truncated on 32-bit platform

No, values are not truncated like that.  No implicit conversions shorten
the value.  That worst thing you can get is conversion from signed to
unsigned or vice versa, but you will get a warning about it with -Wall.

Just try compiling this for 32-bit:

#include <stdio.h>
unsigned long long size = 0x100000000ULL;
int main()
{
        if (! size)
                printf("test1: size is 0\n");
        else
                printf("test1: size is not 0\n");
        if (size == 0)
                printf("test2: size is 0\n");
        else
                printf("test2: size is not 0\n");
}

-- 
Regards,
Pavel Roskin

Re: [PATCH] AFS fixes and improvements

[Vladimir 'phcoder' Serbinenko]

On Sun, Jul 19, 2009 at 10:10 PM, Pavel Roskin<proski@gnu.org> wrote:
> On Sun, 2009-07-19 at 15:20 +0200, Vladimir 'phcoder' Serbinenko wrote:
>
>> > -  if ((! dir->inode.stream.size) ||
>> > +  if ((dir->inode.stream.size == 0) ||
>> >
>> > The later is marginally better, but it would be easier to review your
>> > patches if you don't include such changes.
>> It's not a stylistic improvement. dir->inode.stream.size is 64-bit
>> quantity which will be truncated on 32-bit platform
>
> No, values are not truncated like that.  No implicit conversions shorten
> the value.  That worst thing you can get is conversion from signed to
> unsigned or vice versa, but you will get a warning about it with -Wall.
>
> Just try compiling this for 32-bit:
>
You're right. Such a problem was pointed to me once by Yoshinori K
Okuji and I accepted it without really checking. Anyway with == 0 it's
sure to work and we both agree that it is a bit more readable even if
technically equivalent
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git