Re: [PATCH 1/4] Add envblk open functions to grub file interface

[Daniel Kiper]

On Tue, Feb 25, 2020 at 11:26:35AM -0800, Paul Dagnelie wrote:
> These functions are added to support the remainder of this patch set.
> In order to add these special functions we have to refactor out the
> logic that applies file filters so that it can be applied to both the
> normal and envblk open functions.
>
> commit 45ee383e11ecd15ac2820131d410a434eeea47a1
> Author:     Paul Dagnelie <address@hidden>
> AuthorDate: Mon Feb 24 11:19:40 2020 -0800
> Commit:     Paul Dagnelie <address@hidden>
> CommitDate: Tue Feb 25 10:08:07 2020 -0800
>
>     Add support for special envblk functions in GRUB

I am not sure how you generated this. Could you use "git format-patch"
and "git send-email" to send your patches. And please do not forget
about cover letter and SOB.

> ---
>  grub-core/kern/file.c | 76 
> +++++++++++++++++++++++++++++++++++++--------------
>  include/grub/file.h   | 10 +++++++
>  include/grub/fs.h     | 12 ++++++++
>  3 files changed, 77 insertions(+), 21 deletions(-)
>
> diff --git a/grub-core/kern/file.c b/grub-core/kern/file.c
> index 58454458c..f929f61a0 100644
> --- a/grub-core/kern/file.c
> +++ b/grub-core/kern/file.c
> @@ -57,14 +57,37 @@ grub_file_get_device_name (const char *name)
>    return 0;
>  }
>
> +static grub_file_t
> +grub_apply_file_filters (grub_file_t file, enum grub_file_type type,
> const char *name)
> +{
> +  grub_file_filter_id_t filter;
> +  grub_file_t last_file = NULL;
> +
> +  for (filter = 0; file && filter < ARRAY_SIZE (grub_file_filters);
> +       filter++)
> +    if (grub_file_filters[filter])
> +      {
> +    last_file = file;
> +    file = grub_file_filters[filter] (file, type);
> +    if (file && file != last_file)
> +      {
> +        file->name = grub_strdup (name);
> +        grub_errno = GRUB_ERR_NONE;
> +      }
> +      }
> +  if (!file)
> +    grub_file_close (last_file);
> +
> +  return file;
> +}
> +
>  grub_file_t
>  grub_file_open (const char *name, enum grub_file_type type)
>  {
> -  grub_device_t device = 0;
> -  grub_file_t file = 0, last_file = 0;
> +  grub_device_t device = NULL;
> +  grub_file_t file = NULL;
>    char *device_name;
>    const char *file_name;
> -  grub_file_filter_id_t filter;
>
>    device_name = grub_file_get_device_name (name);
>    if (grub_errno)
> @@ -113,22 +136,7 @@ grub_file_open (const char *name, enum grub_file_type 
> type)
>    file->name = grub_strdup (name);
>    grub_errno = GRUB_ERR_NONE;
>
> -  for (filter = 0; file && filter < ARRAY_SIZE (grub_file_filters);
> -       filter++)
> -    if (grub_file_filters[filter])
> -      {
> -    last_file = file;
> -    file = grub_file_filters[filter] (file, type);
> -    if (file && file != last_file)
> -      {
> -        file->name = grub_strdup (name);
> -        grub_errno = GRUB_ERR_NONE;
> -      }
> -      }
> -  if (!file)
> -    grub_file_close (last_file);
> -
> -  return file;
> +  return grub_apply_file_filters(file, type, name);
>
>   fail:
>    if (device)
> @@ -141,6 +149,27 @@ grub_file_open (const char *name, enum grub_file_type 
> type)
>    return 0;
>  }
>
> +grub_file_t
> +grub_file_envblk_open (grub_fs_t fs, grub_device_t device, enum
> grub_file_type type)
> +{
> +  grub_file_t file = NULL;
> +  file = (grub_file_t) grub_zalloc (sizeof (*file));
> +  if (! file)
> +    return NULL;
> +  file->device = device;
> +  file->envblk = 1;
> +
> +  if ((fs->fs_envblk_open) (file) != GRUB_ERR_NONE) {
> +    grub_free(file);
> +    return NULL;
> +  }
> +
> +  file->fs = fs;
> +  grub_errno = GRUB_ERR_NONE;
> +
> +  return grub_apply_file_filters(file, type, NULL);
> +}
> +

This change should land in separate patch...

>  grub_disk_read_hook_t grub_file_progress_hook;
>
>  grub_ssize_t
> @@ -177,7 +206,10 @@ grub_file_read (grub_file_t file, void *buf,
> grub_size_t len)
>        file->read_hook_data = file;
>        file->progress_offset = file->offset;
>      }
> -  res = (file->fs->fs_read) (file, buf, len);
> +  if (grub_file_envblk (file))
> +    res = (file->fs->fs_envblk_read) (file, buf, len);
> +  else
> +    res = (file->fs->fs_read) (file, buf, len);

??? Probably ditto...

>    file->read_hook = read_hook;
>    file->read_hook_data = read_hook_data;
>    if (res > 0)
> @@ -189,7 +221,9 @@ grub_file_read (grub_file_t file, void *buf,
> grub_size_t len)
>  grub_err_t
>  grub_file_close (grub_file_t file)
>  {
> -  if (file->fs->fs_close)
> +  if (grub_file_envblk (file) && file->fs->fs_envblk_close)
> +    (file->fs->fs_envblk_close) (file);
> +  else if (file->fs->fs_close)

Same here and below...

>      (file->fs->fs_close) (file);
>
>    if (file->device)
> diff --git a/include/grub/file.h b/include/grub/file.h
> index 31567483c..1cc688f55 100644
> --- a/include/grub/file.h
> +++ b/include/grub/file.h
> @@ -170,6 +170,9 @@ struct grub_file
>
>    /* Caller-specific data passed to the read hook.  */
>    void *read_hook_data;
> +
> +  /* If the file is an FS's envblk, which uses separate functions for
> interaction. */
> +  int envblk;
>  };
>  typedef struct grub_file *grub_file_t;
>
> @@ -211,6 +214,7 @@ grub_ssize_t EXPORT_FUNC(grub_file_read)
> (grub_file_t file, void *buf,
>                        grub_size_t len);
>  grub_off_t EXPORT_FUNC(grub_file_seek) (grub_file_t file, grub_off_t offset);
>  grub_err_t EXPORT_FUNC(grub_file_close) (grub_file_t file);
> +grub_file_t EXPORT_FUNC(grub_file_envblk_open) (grub_fs_t fs,
> grub_device_t device, enum grub_file_type type);
>
>  /* Return value of grub_file_size() in case file size is unknown. */
>  #define GRUB_FILE_SIZE_UNKNOWN     0xffffffffffffffffULL
> @@ -233,6 +237,12 @@ grub_file_seekable (const grub_file_t file)
>    return !file->not_easily_seekable;
>  }
>
> +static inline int
> +grub_file_envblk (const grub_file_t file)
> +{
> +  return file->envblk;
> +}
> +
>  grub_file_t
>  grub_file_offset_open (grub_file_t parent, enum grub_file_type type,
>                 grub_off_t start, grub_off_t size);
> diff --git a/include/grub/fs.h b/include/grub/fs.h
> index 302e48d4b..ec1a7a50e 100644
> --- a/include/grub/fs.h
> +++ b/include/grub/fs.h
> @@ -96,6 +96,18 @@ struct grub_fs
>    /* Whether blocklist installs have a chance to work.  */
>    int blocklist_install;
>  #endif
> +
> +  /*
> +   * The envblk functions are defined on filesystems that need to handle
> +   * grub-writable files in a special way. This is most commonly the case for
> +   * CoW filesystems like btrfs and ZFS.  The normal read and close functions
> +   * should detect that they are being called on the envblk file and act
> +   * appropriately.
> +   */
> +  grub_err_t (*fs_envblk_open) (struct grub_file *file);
> +  grub_ssize_t (*fs_envblk_read) (struct grub_file *file, char *buf,
> grub_size_t len);
> +  grub_err_t (*fs_envblk_write) (struct grub_file *file, char *buf,
> grub_size_t len);
> +  grub_err_t (*fs_envblk_close) (struct grub_file *file);
>  };
>  typedef struct grub_fs *grub_fs_t;

Daniel