[syslinux] [PATCH] Unification of ext_(write/read)_adv

[Celelibi]

2015-11-12 11:02 UTC+01:00, Nicolas Cornu via Syslinux <syslinux at zytor.com>:
> Signed-off-by: Nicolas Cornu <nicolac76 at yahoo.fr>
> ---

It would be nice if you included a commit message giving a longer
description of your patch. IMO the first line of the commit message
should say what this patch does (in less than 50 characters if
possible) and the body of the commit message should say how you did it
without being too detailed, and possibly saying briefly why you did it
if it's not obvious. For instance, it could be:
----------------------------
extlinux: code cleanup and simplification

Merge btrfs_read_adv and xfs_read_adv into a single generic function
ext_read_adv and split ext_write_adv_offset out of ext_write_adv.
----------------------------

Unfortunately I don't know why this patch is needed, so I can't add it
to the commit message example. Is it just general code improvement? Or
is there a technical reason, like the binary is too large for some
usage?


>  extlinux/main.c | 44 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/extlinux/main.c b/extlinux/main.c
> index 6871fb1..385b364 100644
> --- a/extlinux/main.c
> +++ b/extlinux/main.c
> @@ -1415,21 +1415,12 @@ static int open_device(const char *path, struct stat
> *st, char **_devname)
>      return devfd;
>  }
>
> -static int btrfs_read_adv(int devfd)
> -{
> -    if (xpread(devfd, syslinux_adv, 2 * ADV_SIZE, BTRFS_ADV_OFFSET)
> -	!= 2 * ADV_SIZE)
> -	return -1;
> -
> -    return syslinux_validate_adv(syslinux_adv) ? 1 : 0;
> -}
> -
> -static inline int xfs_read_adv(int devfd)
> +static int ext_read_adv_offset(int devfd, off_t offset)
>  {
>      const size_t adv_size = 2 * ADV_SIZE;
>
> -    if (xpread(devfd, syslinux_adv, adv_size, boot_image_len) != adv_size)
> -	return -1;
> +    if (xpread(devfd, syslinux_adv, adv_size, offset) != adv_size)
> +      return -1;
>
>      return syslinux_validate_adv(syslinux_adv) ? 1 : 0;
>  }
> @@ -1441,10 +1432,10 @@ static int ext_read_adv(const char *path, int devfd,
> const char **namep)
>
>      if (fs_type == BTRFS) {
>  	/* btrfs "ldlinux.sys" is in 64k blank area */
> -	return btrfs_read_adv(devfd);
> +	return ext_read_adv_offset(devfd, BTRFS_ADV_OFFSET);
>      } else if (fs_type == XFS) {
>  	/* XFS "ldlinux.sys" is in the first 2048 bytes of the primary AG */
> -	return xfs_read_adv(devfd);
> +	return ext_read_adv_offset(devfd, boot_image_len);
>      } else {
>  	err = read_adv(path, name = "ldlinux.sys");
>  	if (err == 2)		/* ldlinux.sys does not exist */
> @@ -1455,17 +1446,26 @@ static int ext_read_adv(const char *path, int devfd,
> const char **namep)
>      }
>  }
>
> +static int ext_write_adv_offset(int devfd, off_t offset)
> +{
> +    const size_t adv_size = 2 * ADV_SIZE;
> +
> +    if (xpwrite(devfd, syslinux_adv, adv_size, offset) != adv_size) {
> +	perror("writing adv");
> +	return 1;
> +    }
> +
> +    return 0;
> +}
> +

Is there a reason to split this function out of ext_write_adv beside
code clarity? Is it reused in an upcomming patch? I'm not saying it is
bad to do that, it just looks unnecessary as ext_write_adv and
ext_write_adv_offset are both "static" and ext_write_adv_offset is
only used in ext_write_adv.

>  static int ext_write_adv(const char *path, const char *cfg, int devfd)
>  {
> -    if (fs_type == BTRFS) { /* btrfs "ldlinux.sys" is in 64k blank area */
> -	if (xpwrite(devfd, syslinux_adv, 2 * ADV_SIZE,
> -		BTRFS_ADV_OFFSET) != 2 * ADV_SIZE) {
> -		perror("writing adv");
> -		return 1;
> -	}
> -	return 0;
> +    if (fs_type == BTRFS) {
> +	/* btrfs "ldlinux.sys" is in 64k blank area */
> +	return ext_write_adv_offset(devfd, BTRFS_ADV_OFFSET);
> +    } else {
> +	return write_adv(path, cfg);
>      }
> -    return write_adv(path, cfg);
>  }
>
>  static int install_loader(const char *path, int update_only)
> --
> 2.6.2
>
> _______________________________________________
> Syslinux mailing list
> Submissions to Syslinux at zytor.com
> Unsubscribe or set options at:
> http://www.zytor.com/mailman/listinfo/syslinux
>


Celelibi.