[PATCH] buffered file read

[PATCH] buffered file read

[Bean]

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

Hi,

This patch add a new module bufio, which reads block of data at a
time, and return the required range to the upper level. This is
extremely useful in modules like png, where data are normally read one
byte at a time.

To use buffered io service, just include header file <grub/bufio.h>,
and replace grub_file_open with grub_buffile_open.

Changelog:

2008-07-27  Bean  <bean123ch@gmail.com>
	
	* conf/common.rmk (pkglib_MODULES): Add bufio.mod.
	(bufio_mod_SOURCES): New macro.
	(bufio_mod_CFLAGS): Likewise.
	(bufio_mod_LDFLAGS): Likewise.

	* include/grub/bufio.h: New file.

	* io/bufio.c: Likewise.

	* video/png.c (grub_video_reader_png): Use grub_buffile_open to open
	file.

	* video/jpeg.c (grub_video_reader_jpeg): Likewise.

	* video/tga.c (grub_video_reader_tga): Likewise.

-- 
Bean

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bufio.diff --]
[-- Type: text/x-diff; name=bufio.diff, Size: 7601 bytes --]

diff --git a/conf/common.rmk b/conf/common.rmk
index 7db0b2a..3d3cd8a 100644
--- a/conf/common.rmk
+++ b/conf/common.rmk
@@ -382,7 +382,7 @@ crc_mod_CFLAGS = $(COMMON_CFLAGS)
 crc_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
 # Misc.
-pkglib_MODULES += gzio.mod elf.mod
+pkglib_MODULES += gzio.mod bufio.mod elf.mod
 
 # For elf.mod.
 elf_mod_SOURCES = kern/elf.c
@@ -393,3 +393,8 @@ elf_mod_LDFLAGS = $(COMMON_LDFLAGS)
 gzio_mod_SOURCES = io/gzio.c
 gzio_mod_CFLAGS = $(COMMON_CFLAGS)
 gzio_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
+# For bufio.mod.
+bufio_mod_SOURCES = io/bufio.c
+bufio_mod_CFLAGS = $(COMMON_CFLAGS)
+bufio_mod_LDFLAGS = $(COMMON_LDFLAGS)
diff --git a/include/grub/bufio.h b/include/grub/bufio.h
new file mode 100644
index 0000000..5ca54dd
--- /dev/null
+++ b/include/grub/bufio.h
@@ -0,0 +1,28 @@
+/* bufio.h - prototypes for bufio */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 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 <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef GRUB_BUFIO_H
+#define GRUB_BUFIO_H	1
+
+#include <grub/file.h>
+
+grub_file_t grub_bufio_open (grub_file_t io);
+grub_file_t grub_buffile_open (const char *name);
+
+#endif /* ! GRUB_BUFIO_H */
diff --git a/io/bufio.c b/io/bufio.c
new file mode 100644
index 0000000..34034b0
--- /dev/null
+++ b/io/bufio.c
@@ -0,0 +1,172 @@
+/* bufio.c - buffered io access */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 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 <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/err.h>
+#include <grub/types.h>
+#include <grub/mm.h>
+#include <grub/misc.h>
+#include <grub/fs.h>
+#include <grub/bufio.h>
+
+#define GRUB_BUFIO_BUFSIZE	8192
+
+struct grub_bufio
+{
+  grub_file_t file;
+  char buffer[GRUB_BUFIO_BUFSIZE];
+  grub_size_t length;
+};
+typedef struct grub_bufio *grub_bufio_t;
+
+static struct grub_fs grub_bufio_fs;
+
+grub_file_t
+grub_bufio_open (grub_file_t io)
+{
+  grub_file_t file;
+  grub_bufio_t bufio = 0;
+
+  file = (grub_file_t) grub_malloc (sizeof (*file));
+  if (! file)
+    return 0;
+
+  bufio = grub_malloc (sizeof (*bufio));
+  if (! bufio)
+    {
+      grub_free (file);
+      return 0;
+    }
+
+  grub_memset (bufio, 0, sizeof (*bufio));
+  bufio->file = io;
+  bufio->length = 1;
+
+  file->device = io->device;
+  file->offset = 0;
+  file->size = io->size;
+  file->data = bufio;
+  file->read_hook = 0;
+  file->fs = &grub_bufio_fs;
+
+  return file;
+}
+
+grub_file_t
+grub_buffile_open (const char *name)
+{
+  grub_file_t io, file;
+
+  io = grub_file_open (name);
+  if (! io)
+    return 0;
+
+  file = grub_bufio_open (io);
+  if (! file)
+    {
+      grub_file_close (io);
+      return 0;
+    }
+
+  return file;
+}
+
+static grub_ssize_t
+grub_bufio_read (grub_file_t file, char *buf, grub_size_t len)
+{
+  grub_size_t res = len;
+  grub_bufio_t bufio = file->data;
+  grub_size_t pos = file->offset & (GRUB_BUFIO_BUFSIZE - 1);
+
+  if (bufio->file->offset - bufio->length == file->offset - pos)
+    {
+      grub_size_t n;
+
+      n = bufio->length - pos;
+      if (n < len)
+        n = len;
+
+      grub_memcpy (buf, &bufio->buffer[pos], n);
+      len -= n;
+
+      if (! len)
+        return res;
+
+      buf += n;
+      pos = 0;
+    }
+  else
+    bufio->file->offset = file->offset - pos;
+
+  while (pos + len >= GRUB_BUFIO_BUFSIZE)
+    {
+      grub_size_t n;
+
+      n = GRUB_BUFIO_BUFSIZE - pos;
+      bufio->file->offset += pos;
+      bufio->file->fs->read (bufio->file, buf, n);
+      if (grub_errno)
+        return -1;
+
+      buf += n;
+      len -= n;
+      pos = 0;
+    }
+
+  if (! len)
+    {
+      bufio->length = 1;
+      return res;
+    }
+
+  bufio->length = bufio->file->size - bufio->file->offset;
+  if (bufio->length > GRUB_BUFIO_BUFSIZE)
+    bufio->length = GRUB_BUFIO_BUFSIZE;
+
+  bufio->file->fs->read (bufio->file, bufio->buffer, bufio->length);
+  if (grub_errno)
+    return -1;
+
+  grub_memcpy (buf, &bufio->buffer[pos], len);
+
+  return res;
+}
+
+static grub_err_t
+grub_bufio_close (grub_file_t file)
+{
+  grub_bufio_t bufio = file->data;
+
+  grub_file_close (bufio->file);
+  grub_free (bufio);
+
+  file->device = 0;
+
+  return grub_errno;
+}
+
+static struct grub_fs grub_bufio_fs =
+  {
+    .name = "bufio",
+    .dir = 0,
+    .open = 0,
+    .read = grub_bufio_read,
+    .close = grub_bufio_close,
+    .label = 0,
+    .next = 0
+  };
diff --git a/video/readers/jpeg.c b/video/readers/jpeg.c
index 8a4f803..bec084e 100644
--- a/video/readers/jpeg.c
+++ b/video/readers/jpeg.c
@@ -23,7 +23,7 @@
 #include <grub/mm.h>
 #include <grub/misc.h>
 #include <grub/arg.h>
-#include <grub/file.h>
+#include <grub/bufio.h>
 
 /* Uncomment following define to enable JPEG debug.  */
 //#define JPEG_DEBUG
@@ -664,7 +664,7 @@ grub_video_reader_jpeg (struct grub_video_bitmap **bitmap,
   grub_file_t file;
   struct grub_jpeg_data *data;
 
-  file = grub_file_open (filename);
+  file = grub_buffile_open (filename);
   if (!file)
     return grub_errno;
 
diff --git a/video/readers/png.c b/video/readers/png.c
index 9dac4b6..ce38f9f 100644
--- a/video/readers/png.c
+++ b/video/readers/png.c
@@ -23,7 +23,7 @@
 #include <grub/mm.h>
 #include <grub/misc.h>
 #include <grub/arg.h>
-#include <grub/file.h>
+#include <grub/bufio.h>
 
 /* Uncomment following define to enable PNG debug.  */
 //#define PNG_DEBUG
@@ -840,7 +840,7 @@ grub_video_reader_png (struct grub_video_bitmap **bitmap,
   grub_file_t file;
   struct grub_png_data *data;
 
-  file = grub_file_open (filename);
+  file = grub_buffile_open (filename);
   if (!file)
     return grub_errno;
 
diff --git a/video/readers/tga.c b/video/readers/tga.c
index 7b944b0..0fe785f 100644
--- a/video/readers/tga.c
+++ b/video/readers/tga.c
@@ -23,7 +23,7 @@
 #include <grub/mm.h>
 #include <grub/misc.h>
 #include <grub/arg.h>
-#include <grub/file.h>
+#include <grub/bufio.h>
 
 /* Uncomment following define to enable TGA debug.  */
 //#define TGA_DEBUG
@@ -319,7 +319,7 @@ grub_video_reader_tga (struct grub_video_bitmap **bitmap,
   struct grub_tga_header header;
   int has_alpha;
 
-  file = grub_file_open (filename);
+  file = grub_buffile_open (filename);
   if (! file)
     return grub_errno;
 

Re: [PATCH] buffered file read

[Colin D Bennett]

On Mon, 28 Jul 2008 01:52:40 +0800
Bean <bean123ch@gmail.com> wrote:

> Hi,
> 
> This patch add a new module bufio, which reads block of data at a
> time, and return the required range to the upper level. This is
> extremely useful in modules like png, where data are normally read one
> byte at a time.
> 
> To use buffered io service, just include header file <grub/bufio.h>,
> and replace grub_file_open with grub_buffile_open.
> 
> Changelog:
> 
> 2008-07-27  Bean  <bean123ch@gmail.com>
> 	
> 	* conf/common.rmk (pkglib_MODULES): Add bufio.mod.
> 	(bufio_mod_SOURCES): New macro.
> 	(bufio_mod_CFLAGS): Likewise.
> 	(bufio_mod_LDFLAGS): Likewise.
> 
> 	* include/grub/bufio.h: New file.
> 
> 	* io/bufio.c: Likewise.
> 
> 	* video/png.c (grub_video_reader_png): Use grub_buffile_open
> to open file.
> 
> 	* video/jpeg.c (grub_video_reader_jpeg): Likewise.
> 
> 	* video/tga.c (grub_video_reader_tga): Likewise.

Hi Bean,

Beautiful code!  Thanks so much for doing this work -- using buffered
file reads is absolutely necessary from a performance perspective, and
I am glad to see it implemented so cleanly.

My poor hack of grub_file_read() bows to your far superior patch!  I
really appreciate the simplicity and clarity of both its design and its
use.  I like how you implemented it using the Decorator pattern (from
the GoF design patterns) to wrap the basic file with the buffered file
layer in a way transparent to users. This is very intuitive to me,
especially since Java does exactly the same thing with InputStream and
BufferedInputStream.

The best part is that users of file I/O only have to change one
function call to get all the benefits of the buffering.

I will begin testing with this patch instead of mine shortly.

Thanks again!
Regards,
Colin

Re: [PATCH] buffered file read

[Bean]

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

On Mon, Jul 28, 2008 at 3:51 AM, Colin D Bennett <colin@gibibit.com> wrote:
> On Mon, 28 Jul 2008 01:52:40 +0800
> Bean <bean123ch@gmail.com> wrote:
>
>> Hi,
>>
>> This patch add a new module bufio, which reads block of data at a
>> time, and return the required range to the upper level. This is
>> extremely useful in modules like png, where data are normally read one
>> byte at a time.
>>
>> To use buffered io service, just include header file <grub/bufio.h>,
>> and replace grub_file_open with grub_buffile_open.
>>
>> Changelog:
>>
>> 2008-07-27  Bean  <bean123ch@gmail.com>
>>
>>       * conf/common.rmk (pkglib_MODULES): Add bufio.mod.
>>       (bufio_mod_SOURCES): New macro.
>>       (bufio_mod_CFLAGS): Likewise.
>>       (bufio_mod_LDFLAGS): Likewise.
>>
>>       * include/grub/bufio.h: New file.
>>
>>       * io/bufio.c: Likewise.
>>
>>       * video/png.c (grub_video_reader_png): Use grub_buffile_open
>> to open file.
>>
>>       * video/jpeg.c (grub_video_reader_jpeg): Likewise.
>>
>>       * video/tga.c (grub_video_reader_tga): Likewise.
>
> Hi Bean,
>
> Beautiful code!  Thanks so much for doing this work -- using buffered
> file reads is absolutely necessary from a performance perspective, and
> I am glad to see it implemented so cleanly.
>
> My poor hack of grub_file_read() bows to your far superior patch!  I
> really appreciate the simplicity and clarity of both its design and its
> use.  I like how you implemented it using the Decorator pattern (from
> the GoF design patterns) to wrap the basic file with the buffered file
> layer in a way transparent to users. This is very intuitive to me,
> especially since Java does exactly the same thing with InputStream and
> BufferedInputStream.
>
> The best part is that users of file I/O only have to change one
> function call to get all the benefits of the buffering.
>
> I will begin testing with this patch instead of mine shortly.

Hi,

Oh, I just spot a few bugs, please try the new patch.

-- 
Bean

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bufio_2.diff --]
[-- Type: text/x-diff; name=bufio_2.diff, Size: 7679 bytes --]

diff --git a/conf/common.rmk b/conf/common.rmk
index 7db0b2a..3d3cd8a 100644
--- a/conf/common.rmk
+++ b/conf/common.rmk
@@ -382,7 +382,7 @@ crc_mod_CFLAGS = $(COMMON_CFLAGS)
 crc_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
 # Misc.
-pkglib_MODULES += gzio.mod elf.mod
+pkglib_MODULES += gzio.mod bufio.mod elf.mod
 
 # For elf.mod.
 elf_mod_SOURCES = kern/elf.c
@@ -393,3 +393,8 @@ elf_mod_LDFLAGS = $(COMMON_LDFLAGS)
 gzio_mod_SOURCES = io/gzio.c
 gzio_mod_CFLAGS = $(COMMON_CFLAGS)
 gzio_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
+# For bufio.mod.
+bufio_mod_SOURCES = io/bufio.c
+bufio_mod_CFLAGS = $(COMMON_CFLAGS)
+bufio_mod_LDFLAGS = $(COMMON_LDFLAGS)
diff --git a/include/grub/bufio.h b/include/grub/bufio.h
new file mode 100644
index 0000000..5ca54dd
--- /dev/null
+++ b/include/grub/bufio.h
@@ -0,0 +1,28 @@
+/* bufio.h - prototypes for bufio */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 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 <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef GRUB_BUFIO_H
+#define GRUB_BUFIO_H	1
+
+#include <grub/file.h>
+
+grub_file_t grub_bufio_open (grub_file_t io);
+grub_file_t grub_buffile_open (const char *name);
+
+#endif /* ! GRUB_BUFIO_H */
diff --git a/io/bufio.c b/io/bufio.c
new file mode 100644
index 0000000..b1b2a2f
--- /dev/null
+++ b/io/bufio.c
@@ -0,0 +1,174 @@
+/* bufio.c - buffered io access */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 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 <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/err.h>
+#include <grub/types.h>
+#include <grub/mm.h>
+#include <grub/misc.h>
+#include <grub/fs.h>
+#include <grub/bufio.h>
+
+#define GRUB_BUFIO_BUFSIZE	8192
+
+struct grub_bufio
+{
+  grub_file_t file;
+  grub_size_t length;
+  char buffer[GRUB_BUFIO_BUFSIZE];
+};
+typedef struct grub_bufio *grub_bufio_t;
+
+static struct grub_fs grub_bufio_fs;
+
+grub_file_t
+grub_bufio_open (grub_file_t io)
+{
+  grub_file_t file;
+  grub_bufio_t bufio = 0;
+
+  file = (grub_file_t) grub_malloc (sizeof (*file));
+  if (! file)
+    return 0;
+
+  bufio = grub_malloc (sizeof (*bufio));
+  if (! bufio)
+    {
+      grub_free (file);
+      return 0;
+    }
+
+  grub_memset (bufio, 0, sizeof (*bufio));
+  bufio->file = io;
+  bufio->file->offset = -1;
+
+  file->device = io->device;
+  file->offset = 0;
+  file->size = io->size;
+  file->data = bufio;
+  file->read_hook = 0;
+  file->fs = &grub_bufio_fs;
+
+  return file;
+}
+
+grub_file_t
+grub_buffile_open (const char *name)
+{
+  grub_file_t io, file;
+
+  io = grub_file_open (name);
+  if (! io)
+    return 0;
+
+  file = grub_bufio_open (io);
+  if (! file)
+    {
+      grub_file_close (io);
+      return 0;
+    }
+
+  return file;
+}
+
+static grub_ssize_t
+grub_bufio_read (grub_file_t file, char *buf, grub_size_t len)
+{
+  grub_size_t res = len;
+  grub_bufio_t bufio = file->data;
+  grub_size_t pos = file->offset & (GRUB_BUFIO_BUFSIZE - 1);
+
+  if (bufio->file->offset == file->offset - pos)
+    {
+      grub_size_t n;
+
+      n = bufio->length - pos;
+      if (n > len)
+        n = len;
+
+      grub_memcpy (buf, &bufio->buffer[pos], n);
+      len -= n;
+
+      if (! len)
+        return res;
+
+      buf += n;
+      pos = 0;
+      bufio->file->offset += bufio->length;
+    }
+  else
+    bufio->file->offset = file->offset - pos;
+
+  while (pos + len >= GRUB_BUFIO_BUFSIZE)
+    {
+      grub_size_t n;
+
+      n = GRUB_BUFIO_BUFSIZE - pos;
+      bufio->file->offset += pos;
+      bufio->file->fs->read (bufio->file, buf, n);
+      if (grub_errno)
+        return -1;
+
+      buf += n;
+      len -= n;
+      bufio->file->offset += n;
+      pos = 0;
+    }
+
+  if (! len)
+    {
+      bufio->file->offset = -1;
+      return res;
+    }
+
+  bufio->length = bufio->file->size - bufio->file->offset;
+  if (bufio->length > GRUB_BUFIO_BUFSIZE)
+    bufio->length = GRUB_BUFIO_BUFSIZE;
+
+  bufio->file->fs->read (bufio->file, bufio->buffer, bufio->length);
+  if (grub_errno)
+    return -1;
+
+  grub_memcpy (buf, &bufio->buffer[pos], len);
+
+  return res;
+}
+
+static grub_err_t
+grub_bufio_close (grub_file_t file)
+{
+  grub_bufio_t bufio = file->data;
+
+  grub_file_close (bufio->file);
+  grub_free (bufio);
+
+  file->device = 0;
+
+  return grub_errno;
+}
+
+static struct grub_fs grub_bufio_fs =
+  {
+    .name = "bufio",
+    .dir = 0,
+    .open = 0,
+    .read = grub_bufio_read,
+    .close = grub_bufio_close,
+    .label = 0,
+    .next = 0
+  };
diff --git a/video/readers/jpeg.c b/video/readers/jpeg.c
index 8a4f803..bec084e 100644
--- a/video/readers/jpeg.c
+++ b/video/readers/jpeg.c
@@ -23,7 +23,7 @@
 #include <grub/mm.h>
 #include <grub/misc.h>
 #include <grub/arg.h>
-#include <grub/file.h>
+#include <grub/bufio.h>
 
 /* Uncomment following define to enable JPEG debug.  */
 //#define JPEG_DEBUG
@@ -664,7 +664,7 @@ grub_video_reader_jpeg (struct grub_video_bitmap **bitmap,
   grub_file_t file;
   struct grub_jpeg_data *data;
 
-  file = grub_file_open (filename);
+  file = grub_buffile_open (filename);
   if (!file)
     return grub_errno;
 
diff --git a/video/readers/png.c b/video/readers/png.c
index 9dac4b6..ce38f9f 100644
--- a/video/readers/png.c
+++ b/video/readers/png.c
@@ -23,7 +23,7 @@
 #include <grub/mm.h>
 #include <grub/misc.h>
 #include <grub/arg.h>
-#include <grub/file.h>
+#include <grub/bufio.h>
 
 /* Uncomment following define to enable PNG debug.  */
 //#define PNG_DEBUG
@@ -840,7 +840,7 @@ grub_video_reader_png (struct grub_video_bitmap **bitmap,
   grub_file_t file;
   struct grub_png_data *data;
 
-  file = grub_file_open (filename);
+  file = grub_buffile_open (filename);
   if (!file)
     return grub_errno;
 
diff --git a/video/readers/tga.c b/video/readers/tga.c
index 7b944b0..0fe785f 100644
--- a/video/readers/tga.c
+++ b/video/readers/tga.c
@@ -23,7 +23,7 @@
 #include <grub/mm.h>
 #include <grub/misc.h>
 #include <grub/arg.h>
-#include <grub/file.h>
+#include <grub/bufio.h>
 
 /* Uncomment following define to enable TGA debug.  */
 //#define TGA_DEBUG
@@ -319,7 +319,7 @@ grub_video_reader_tga (struct grub_video_bitmap **bitmap,
   struct grub_tga_header header;
   int has_alpha;
 
-  file = grub_file_open (filename);
+  file = grub_buffile_open (filename);
   if (! file)
     return grub_errno;
 

Re: [PATCH] buffered file read

[Robert Millan]

On Mon, Jul 28, 2008 at 01:52:40AM +0800, Bean wrote:
> 
> 	* video/png.c (grub_video_reader_png): Use grub_buffile_open to open
> 	file.
> 
> 	* video/jpeg.c (grub_video_reader_jpeg): Likewise.
> 
> 	* video/tga.c (grub_video_reader_tga): Likewise.

Maybe it's a good idea to use it in the font loader too?  In font/manager.c's
add_font().

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] buffered file read

[Bean]

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

Hi,

I have added a buffer length parameter to grub_buffile_open:

grub_file_t grub_buffile_open (const char *name, int size);

size < 0: Load the whole file.
size = 0: Use default buffer size (8192 bytes)
size > 0: Custom buffer size = size

The upper limit is 1m, if size > 1m, we set it to 1m.

2008-07-28  Bean  <bean123ch@gmail.com>

	* conf/common.rmk (pkglib_MODULES): Add bufio.mod.
	(bufio_mod_SOURCES): New macro.
	(bufio_mod_CFLAGS): Likewise.
	(bufio_mod_LDFLAGS): Likewise.

	* include/grub/bufio.h: New file.

	* io/bufio.c: Likewise.

	* video/png.c (grub_video_reader_png): Use grub_buffile_open to open
	file.

	* video/jpeg.c (grub_video_reader_jpeg): Likewise.

	* video/tga.c (grub_video_reader_tga): Likewise.

	* font/manager.c (add_font): Likewise.

-- 
Bean

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bufio_3.diff --]
[-- Type: text/x-diff; name=bufio_3.diff, Size: 9289 bytes --]

diff --git a/conf/common.rmk b/conf/common.rmk
index 7db0b2a..3d3cd8a 100644
--- a/conf/common.rmk
+++ b/conf/common.rmk
@@ -382,7 +382,7 @@ crc_mod_CFLAGS = $(COMMON_CFLAGS)
 crc_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
 # Misc.
-pkglib_MODULES += gzio.mod elf.mod
+pkglib_MODULES += gzio.mod bufio.mod elf.mod
 
 # For elf.mod.
 elf_mod_SOURCES = kern/elf.c
@@ -393,3 +393,8 @@ elf_mod_LDFLAGS = $(COMMON_LDFLAGS)
 gzio_mod_SOURCES = io/gzio.c
 gzio_mod_CFLAGS = $(COMMON_CFLAGS)
 gzio_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
+# For bufio.mod.
+bufio_mod_SOURCES = io/bufio.c
+bufio_mod_CFLAGS = $(COMMON_CFLAGS)
+bufio_mod_LDFLAGS = $(COMMON_LDFLAGS)
diff --git a/font/manager.c b/font/manager.c
index 80d8d07..887a4a3 100644
--- a/font/manager.c
+++ b/font/manager.c
@@ -23,6 +23,7 @@
 #include <grub/types.h>
 #include <grub/mm.h>
 #include <grub/font.h>
+#include <grub/bufio.h>
 
 struct entry
 {
@@ -69,7 +70,7 @@ add_font (const char *filename)
   grub_uint32_t num, i;
   struct font *font = 0;
 
-  file = grub_file_open (filename);
+  file = grub_buffile_open (filename, 0);
   if (! file)
     goto fail;
 
diff --git a/include/grub/bufio.h b/include/grub/bufio.h
new file mode 100644
index 0000000..9a2294c
--- /dev/null
+++ b/include/grub/bufio.h
@@ -0,0 +1,28 @@
+/* bufio.h - prototypes for bufio */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 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 <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef GRUB_BUFIO_H
+#define GRUB_BUFIO_H	1
+
+#include <grub/file.h>
+
+grub_file_t grub_bufio_open (grub_file_t io, int size);
+grub_file_t grub_buffile_open (const char *name, int size);
+
+#endif /* ! GRUB_BUFIO_H */
diff --git a/io/bufio.c b/io/bufio.c
new file mode 100644
index 0000000..40ed89b
--- /dev/null
+++ b/io/bufio.c
@@ -0,0 +1,205 @@
+/* bufio.c - buffered io access */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 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 <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/err.h>
+#include <grub/types.h>
+#include <grub/mm.h>
+#include <grub/misc.h>
+#include <grub/fs.h>
+#include <grub/bufio.h>
+
+#define GRUB_BUFIO_DEF_SIZE	8192
+#define GRUB_BUFIO_MAX_SIZE	1048576
+
+struct grub_bufio
+{
+  grub_file_t file;
+  grub_size_t block_size;
+  grub_size_t buffer_len;
+  char buffer[0];
+};
+typedef struct grub_bufio *grub_bufio_t;
+
+static struct grub_fs grub_bufio_fs;
+
+grub_file_t
+grub_bufio_open (grub_file_t io, int size)
+{
+  grub_file_t file;
+  grub_bufio_t bufio = 0;
+
+  file = (grub_file_t) grub_malloc (sizeof (*file));
+  if (! file)
+    return 0;
+
+  if (size == 0)
+    size = GRUB_BUFIO_DEF_SIZE;
+  else if (size > GRUB_BUFIO_MAX_SIZE)
+    size = GRUB_BUFIO_MAX_SIZE;
+
+  if ((size < 0) || ((unsigned) size > io->size))
+    size = ((io->size > GRUB_BUFIO_MAX_SIZE) ? GRUB_BUFIO_MAX_SIZE :
+            io->size);
+
+  bufio = grub_malloc (sizeof (struct grub_bufio) + size);
+  if (! bufio)
+    {
+      grub_free (file);
+      return 0;
+    }
+
+  bufio->file = io;
+  bufio->block_size = size;
+  bufio->buffer_len = 0;
+
+  file->device = io->device;
+  file->offset = 0;
+  file->size = io->size;
+  file->data = bufio;
+  file->read_hook = 0;
+  file->fs = &grub_bufio_fs;
+
+  return file;
+}
+
+grub_file_t
+grub_buffile_open (const char *name, int size)
+{
+  grub_file_t io, file;
+
+  io = grub_file_open (name);
+  if (! io)
+    return 0;
+
+  file = grub_bufio_open (io, size);
+  if (! file)
+    {
+      grub_file_close (io);
+      return 0;
+    }
+
+  return file;
+}
+
+static grub_ssize_t
+grub_bufio_read (grub_file_t file, char *buf, grub_size_t len)
+{
+  grub_size_t res = len;
+  grub_bufio_t bufio = file->data;
+  grub_uint32_t pos;
+
+  if ((file->offset >= bufio->file->offset) &&
+      (file->offset < bufio->file->offset + bufio->buffer_len))
+    {
+      grub_size_t n;
+
+      pos = file->offset - bufio->file->offset;
+      n = bufio->buffer_len - pos;
+      if (n > len)
+        n = len;
+
+      grub_memcpy (buf, &bufio->buffer[pos], n);
+      len -= n;
+      if (! len)
+        return res;
+
+      buf += n;
+      bufio->file->offset += bufio->buffer_len;
+      pos = 0;
+    }
+  else
+    {
+      bufio->file->offset = grub_divmod64 (file->offset, bufio->block_size,
+                                           &pos);
+      bufio->file->offset *= bufio->block_size;
+    }
+
+  if (pos + len >= bufio->block_size)
+    {
+      if (pos)
+        {
+          grub_size_t n;
+
+          bufio->file->fs->read (bufio->file, bufio->buffer,
+                                 bufio->block_size);
+          if (grub_errno)
+            return -1;
+
+          n = bufio->block_size - pos;
+          grub_memcpy (buf, &bufio->buffer[pos], n);
+          len -= n;
+          buf += n;
+          bufio->file->offset += bufio->block_size;
+          pos = 0;
+        }
+
+      while (len >= bufio->block_size)
+        {
+          bufio->file->fs->read (bufio->file, buf, bufio->block_size);
+          if (grub_errno)
+            return -1;
+
+          len -= bufio->block_size;
+          buf += bufio->block_size;
+          bufio->file->offset += bufio->block_size;
+        }
+
+      if (! len)
+        {
+          bufio->buffer_len = 0;
+          return res;
+        }
+    }
+
+  bufio->buffer_len = bufio->file->size - bufio->file->offset;
+  if (bufio->buffer_len > bufio->block_size)
+    bufio->buffer_len = bufio->block_size;
+
+  bufio->file->fs->read (bufio->file, bufio->buffer, bufio->buffer_len);
+  if (grub_errno)
+    return -1;
+
+  grub_memcpy (buf, &bufio->buffer[pos], len);
+
+  return res;
+}
+
+static grub_err_t
+grub_bufio_close (grub_file_t file)
+{
+  grub_bufio_t bufio = file->data;
+
+  grub_file_close (bufio->file);
+  grub_free (bufio);
+
+  file->device = 0;
+
+  return grub_errno;
+}
+
+static struct grub_fs grub_bufio_fs =
+  {
+    .name = "bufio",
+    .dir = 0,
+    .open = 0,
+    .read = grub_bufio_read,
+    .close = grub_bufio_close,
+    .label = 0,
+    .next = 0
+  };
diff --git a/video/readers/jpeg.c b/video/readers/jpeg.c
index 8a4f803..03f3a94 100644
--- a/video/readers/jpeg.c
+++ b/video/readers/jpeg.c
@@ -23,7 +23,7 @@
 #include <grub/mm.h>
 #include <grub/misc.h>
 #include <grub/arg.h>
-#include <grub/file.h>
+#include <grub/bufio.h>
 
 /* Uncomment following define to enable JPEG debug.  */
 //#define JPEG_DEBUG
@@ -664,7 +664,7 @@ grub_video_reader_jpeg (struct grub_video_bitmap **bitmap,
   grub_file_t file;
   struct grub_jpeg_data *data;
 
-  file = grub_file_open (filename);
+  file = grub_buffile_open (filename, 0);
   if (!file)
     return grub_errno;
 
diff --git a/video/readers/png.c b/video/readers/png.c
index 9dac4b6..ceb77e7 100644
--- a/video/readers/png.c
+++ b/video/readers/png.c
@@ -23,7 +23,7 @@
 #include <grub/mm.h>
 #include <grub/misc.h>
 #include <grub/arg.h>
-#include <grub/file.h>
+#include <grub/bufio.h>
 
 /* Uncomment following define to enable PNG debug.  */
 //#define PNG_DEBUG
@@ -840,7 +840,7 @@ grub_video_reader_png (struct grub_video_bitmap **bitmap,
   grub_file_t file;
   struct grub_png_data *data;
 
-  file = grub_file_open (filename);
+  file = grub_buffile_open (filename, 0);
   if (!file)
     return grub_errno;
 
diff --git a/video/readers/tga.c b/video/readers/tga.c
index 7b944b0..e5f3834 100644
--- a/video/readers/tga.c
+++ b/video/readers/tga.c
@@ -23,7 +23,7 @@
 #include <grub/mm.h>
 #include <grub/misc.h>
 #include <grub/arg.h>
-#include <grub/file.h>
+#include <grub/bufio.h>
 
 /* Uncomment following define to enable TGA debug.  */
 //#define TGA_DEBUG
@@ -319,7 +319,7 @@ grub_video_reader_tga (struct grub_video_bitmap **bitmap,
   struct grub_tga_header header;
   int has_alpha;
 
-  file = grub_file_open (filename);
+  file = grub_buffile_open (filename, 0);
   if (! file)
     return grub_errno;
 

Re: [PATCH] buffered file read

[Bean]

On Mon, Jul 28, 2008 at 1:08 PM, Bean <bean123ch@gmail.com> wrote:
> Hi,
>
> I have added a buffer length parameter to grub_buffile_open:
>
> grub_file_t grub_buffile_open (const char *name, int size);
>
> size < 0: Load the whole file.
> size = 0: Use default buffer size (8192 bytes)
> size > 0: Custom buffer size = size
>
> The upper limit is 1m, if size > 1m, we set it to 1m.
>
> 2008-07-28  Bean  <bean123ch@gmail.com>
>
>        * conf/common.rmk (pkglib_MODULES): Add bufio.mod.
>        (bufio_mod_SOURCES): New macro.
>        (bufio_mod_CFLAGS): Likewise.
>        (bufio_mod_LDFLAGS): Likewise.
>
>        * include/grub/bufio.h: New file.
>
>        * io/bufio.c: Likewise.
>
>        * video/png.c (grub_video_reader_png): Use grub_buffile_open to open
>        file.
>
>        * video/jpeg.c (grub_video_reader_jpeg): Likewise.
>
>        * video/tga.c (grub_video_reader_tga): Likewise.
>
>        * font/manager.c (add_font): Likewise.

Hi,

If no one objects, I'd commit this patch soon.

-- 
Bean

Re: [PATCH] buffered file read

[Colin D Bennett]

On Wed, 30 Jul 2008 11:48:19 +0800
Bean <bean123ch@gmail.com> wrote:

> On Mon, Jul 28, 2008 at 1:08 PM, Bean <bean123ch@gmail.com> wrote:
> > Hi,
> >
> > I have added a buffer length parameter to grub_buffile_open:
> >
> 
...
> 
> If no one objects, I'd commit this patch soon.
> 

Sounds good to me!

Regards,
Colin

Re: [PATCH] buffered file read

[Marco Gerards]

Hi Bean,

Bean <bean123ch@gmail.com> writes:

> I have added a buffer length parameter to grub_buffile_open:
>
> grub_file_t grub_buffile_open (const char *name, int size);
>
> size < 0: Load the whole file.
> size = 0: Use default buffer size (8192 bytes)
> size > 0: Custom buffer size = size
>
> The upper limit is 1m, if size > 1m, we set it to 1m.
>
> 2008-07-28  Bean  <bean123ch@gmail.com>
>
> 	* conf/common.rmk (pkglib_MODULES): Add bufio.mod.
> 	(bufio_mod_SOURCES): New macro.
> 	(bufio_mod_CFLAGS): Likewise.
> 	(bufio_mod_LDFLAGS): Likewise.
>
> 	* include/grub/bufio.h: New file.
>
> 	* io/bufio.c: Likewise.
>
> 	* video/png.c (grub_video_reader_png): Use grub_buffile_open to open
> 	file.
>
> 	* video/jpeg.c (grub_video_reader_jpeg): Likewise.
>
> 	* video/tga.c (grub_video_reader_tga): Likewise.
>
> 	* font/manager.c (add_font): Likewise.

Please mention it when you include <include/grub/bufio.h>.  The same
for when you remove <include/grub/file.h>.

Otherwise it seems good for inclusion :-)

--
Marco

Re: [PATCH] buffered file read

[Bean]

On Fri, Aug 1, 2008 at 3:18 AM, Marco Gerards <mgerards@xs4all.nl> wrote:
> Hi Bean,
>
> Bean <bean123ch@gmail.com> writes:
>
>> I have added a buffer length parameter to grub_buffile_open:
>>
>> grub_file_t grub_buffile_open (const char *name, int size);
>>
>> size < 0: Load the whole file.
>> size = 0: Use default buffer size (8192 bytes)
>> size > 0: Custom buffer size = size
>>
>> The upper limit is 1m, if size > 1m, we set it to 1m.
>>
>> 2008-07-28  Bean  <bean123ch@gmail.com>
>>
>>       * conf/common.rmk (pkglib_MODULES): Add bufio.mod.
>>       (bufio_mod_SOURCES): New macro.
>>       (bufio_mod_CFLAGS): Likewise.
>>       (bufio_mod_LDFLAGS): Likewise.
>>
>>       * include/grub/bufio.h: New file.
>>
>>       * io/bufio.c: Likewise.
>>
>>       * video/png.c (grub_video_reader_png): Use grub_buffile_open to open
>>       file.
>>
>>       * video/jpeg.c (grub_video_reader_jpeg): Likewise.
>>
>>       * video/tga.c (grub_video_reader_tga): Likewise.
>>
>>       * font/manager.c (add_font): Likewise.
>
> Please mention it when you include <include/grub/bufio.h>.  The same
> for when you remove <include/grub/file.h>.
>
> Otherwise it seems good for inclusion :-)

Hi,

Oh, how the changelog would be like ?

-- 
Bean

Re: [PATCH] buffered file read

[Marco Gerards]

Bean <bean123ch@gmail.com> writes:

> On Fri, Aug 1, 2008 at 3:18 AM, Marco Gerards <mgerards@xs4all.nl> wrote:
>> Hi Bean,
>>
>> Bean <bean123ch@gmail.com> writes:
>>
>>> I have added a buffer length parameter to grub_buffile_open:
>>>
>>> grub_file_t grub_buffile_open (const char *name, int size);
>>>
>>> size < 0: Load the whole file.
>>> size = 0: Use default buffer size (8192 bytes)
>>> size > 0: Custom buffer size = size
>>>
>>> The upper limit is 1m, if size > 1m, we set it to 1m.
>>>
>>> 2008-07-28  Bean  <bean123ch@gmail.com>
>>>
>>>       * conf/common.rmk (pkglib_MODULES): Add bufio.mod.
>>>       (bufio_mod_SOURCES): New macro.
>>>       (bufio_mod_CFLAGS): Likewise.
>>>       (bufio_mod_LDFLAGS): Likewise.
>>>
>>>       * include/grub/bufio.h: New file.
>>>
>>>       * io/bufio.c: Likewise.
>>>
>>>       * video/png.c (grub_video_reader_png): Use grub_buffile_open to open
>>>       file.
>>>
>>>       * video/jpeg.c (grub_video_reader_jpeg): Likewise.
>>>
>>>       * video/tga.c (grub_video_reader_tga): Likewise.
>>>
>>>       * font/manager.c (add_font): Likewise.
>>
>> Please mention it when you include <include/grub/bufio.h>.  The same
>> for when you remove <include/grub/file.h>.
>>
>> Otherwise it seems good for inclusion :-)
>
> Hi,
>
> Oh, how the changelog would be like ?

Specifically or generally?  Generally something like:

         * fs/foo.c: Include <grub/bar.h>.
         (baz): New function.
....

--
Marco

Re: [PATCH] buffered file read

[Bean]

On Fri, Aug 1, 2008 at 4:06 AM, Marco Gerards <mgerards@xs4all.nl> wrote:
> Bean <bean123ch@gmail.com> writes:
>
>> On Fri, Aug 1, 2008 at 3:18 AM, Marco Gerards <mgerards@xs4all.nl> wrote:
>>> Hi Bean,
>>>
>>> Bean <bean123ch@gmail.com> writes:
>>>
>>>> I have added a buffer length parameter to grub_buffile_open:
>>>>
>>>> grub_file_t grub_buffile_open (const char *name, int size);
>>>>
>>>> size < 0: Load the whole file.
>>>> size = 0: Use default buffer size (8192 bytes)
>>>> size > 0: Custom buffer size = size
>>>>
>>>> The upper limit is 1m, if size > 1m, we set it to 1m.
>>>>
>>>> 2008-07-28  Bean  <bean123ch@gmail.com>
>>>>
>>>>       * conf/common.rmk (pkglib_MODULES): Add bufio.mod.
>>>>       (bufio_mod_SOURCES): New macro.
>>>>       (bufio_mod_CFLAGS): Likewise.
>>>>       (bufio_mod_LDFLAGS): Likewise.
>>>>
>>>>       * include/grub/bufio.h: New file.
>>>>
>>>>       * io/bufio.c: Likewise.
>>>>
>>>>       * video/png.c (grub_video_reader_png): Use grub_buffile_open to open
>>>>       file.
>>>>
>>>>       * video/jpeg.c (grub_video_reader_jpeg): Likewise.
>>>>
>>>>       * video/tga.c (grub_video_reader_tga): Likewise.
>>>>
>>>>       * font/manager.c (add_font): Likewise.
>>>
>>> Please mention it when you include <include/grub/bufio.h>.  The same
>>> for when you remove <include/grub/file.h>.
>>>
>>> Otherwise it seems good for inclusion :-)
>>
>> Hi,
>>
>> Oh, how the changelog would be like ?
>
> Specifically or generally?  Generally something like:
>
>         * fs/foo.c: Include <grub/bar.h>.
>         (baz): New function.

Committed.

-- 
Bean