Syslinux - Nov 2015 - [PATCH] 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.

Use those new functions in rewrite_boot_image and btrfs_install_file
where it is actually hardcoded.

Signed-off-by: Nicolas Cornu <nicolac76 at yahoo.fr>
---
 extlinux/main.c | 113 +++++++++++++++++++++++++++-----------------------------
 1 file changed, 55 insertions(+), 58 deletions(-)

diff --git a/extlinux/main.c b/extlinux/main.c
index 6871fb1..a2a396a 100644
--- a/extlinux/main.c
+++ b/extlinux/main.c
@@ -338,6 +338,59 @@ static int patch_file_and_bootblock(int fd, const char
*dir, int devfd)
     return rv;
 }
 
+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, offset) != adv_size)
+      return -1;
+
+    return syslinux_validate_adv(syslinux_adv) ? 1 : 0;
+}
+
+static int ext_read_adv(const char *path, int devfd, const char **namep)
+{
+    int err;
+    const char *name;
+
+    if (fs_type == BTRFS) {
+	/* btrfs "ldlinux.sys" is in 64k blank area */
+	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 ext_read_adv_offset(devfd, boot_image_len);
+    } else {
+	err = read_adv(path, name = "ldlinux.sys");
+	if (err == 2)		/* ldlinux.sys does not exist */
+	    err = read_adv(path, name = "extlinux.sys");
+	if (namep)
+	    *namep = name;
+	return err;
+    }
+}
+
+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;
+}
+
+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 */
+	return ext_write_adv_offset(devfd, BTRFS_ADV_OFFSET);
+    } else {
+	return write_adv(path, cfg);
+    }
+}
+
 /*
  * Install the boot block on the specified device.
  * Must be run AFTER install_file()!
@@ -484,8 +537,7 @@ static int rewrite_boot_image(int devfd, const char *path,
const char *filename)
     }
 
     /* Write ADV */
-    ret = xpwrite(fd, syslinux_adv, 2 * ADV_SIZE, boot_image_len);
-    if (ret != 2 * ADV_SIZE) {
+    if (ext_write_adv_offset(fd, boot_image_len)) {
 	fprintf(stderr, "%s: write failure on %s\n", program, filename);
 	goto error;
     }
@@ -614,9 +666,7 @@ int btrfs_install_file(const char *path, int devfd, struct
stat *rst)
 	return 1;
     }
     dprintf("write boot_image to 0x%x\n", BTRFS_EXTLINUX_OFFSET);
-    if (xpwrite(devfd, syslinux_adv, 2 * ADV_SIZE, BTRFS_ADV_OFFSET)
-	!= 2 * ADV_SIZE) {
-	perror("writing adv");
+    if (ext_write_adv_offset(devfd, BTRFS_ADV_OFFSET)) {
 	return 1;
     }
     dprintf("write adv to 0x%x\n", BTRFS_ADV_OFFSET);
@@ -1415,59 +1465,6 @@ 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)
-{
-    const size_t adv_size = 2 * ADV_SIZE;
-
-    if (xpread(devfd, syslinux_adv, adv_size, boot_image_len) != adv_size)
-	return -1;
-
-    return syslinux_validate_adv(syslinux_adv) ? 1 : 0;
-}
-
-static int ext_read_adv(const char *path, int devfd, const char **namep)
-{
-    int err;
-    const char *name;
-
-    if (fs_type == BTRFS) {
-	/* btrfs "ldlinux.sys" is in 64k blank area */
-	return btrfs_read_adv(devfd);
-    } else if (fs_type == XFS) {
-	/* XFS "ldlinux.sys" is in the first 2048 bytes of the primary AG */
-	return xfs_read_adv(devfd);
-    } else {
-	err = read_adv(path, name = "ldlinux.sys");
-	if (err == 2)		/* ldlinux.sys does not exist */
-	    err = read_adv(path, name = "extlinux.sys");
-	if (namep)
-	    *namep = name;
-	return err;
-    }
-}
-
-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;
-    }
-    return write_adv(path, cfg);
-}
-
 static int install_loader(const char *path, int update_only)
 {
     struct stat st, fst;
-- 
2.6.2

2015-11-13 20:35 UTC+01:00, Nicolas Cornu via Syslinux <syslinux at
zytor.com>:
> 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.
>
> Use those new functions in rewrite_boot_image and btrfs_install_file
> where it is actually hardcoded.
>
> Signed-off-by: Nicolas Cornu <nicolac76 at yahoo.fr>
> ---
>  extlinux/main.c | 113
> +++++++++++++++++++++++++++-----------------------------
>  1 file changed, 55 insertions(+), 58 deletions(-)
>
> diff --git a/extlinux/main.c b/extlinux/main.c
> index 6871fb1..a2a396a 100644
> --- a/extlinux/main.c
> +++ b/extlinux/main.c
> @@ -338,6 +338,59 @@ static int patch_file_and_bootblock(int fd, const char
> *dir, int devfd)
>      return rv;
>  }
>
> +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, offset) != adv_size)
> +      return -1;
> +
> +    return syslinux_validate_adv(syslinux_adv) ? 1 : 0;
> +}
> +
> +static int ext_read_adv(const char *path, int devfd, const char **namep)
> +{
> +    int err;
> +    const char *name;
> +
> +    if (fs_type == BTRFS) {
> +	/* btrfs "ldlinux.sys" is in 64k blank area */
> +	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 ext_read_adv_offset(devfd, boot_image_len);
> +    } else {
> +	err = read_adv(path, name = "ldlinux.sys");
> +	if (err == 2)		/* ldlinux.sys does not exist */
> +	    err = read_adv(path, name = "extlinux.sys");
> +	if (namep)
> +	    *namep = name;
> +	return err;
> +    }
> +}
> +
> +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;
> +}
> +
> +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 */
> +	return ext_write_adv_offset(devfd, BTRFS_ADV_OFFSET);
> +    } else {
> +	return write_adv(path, cfg);
> +    }
> +}
> +
>  /*
>   * Install the boot block on the specified device.
>   * Must be run AFTER install_file()!
> @@ -484,8 +537,7 @@ static int rewrite_boot_image(int devfd, const char
> *path, const char *filename)
>      }
>
>      /* Write ADV */
> -    ret = xpwrite(fd, syslinux_adv, 2 * ADV_SIZE, boot_image_len);
> -    if (ret != 2 * ADV_SIZE) {
> +    if (ext_write_adv_offset(fd, boot_image_len)) {
>  	fprintf(stderr, "%s: write failure on %s\n", program,
filename);
>  	goto error;
>      }
> @@ -614,9 +666,7 @@ int btrfs_install_file(const char *path, int devfd,
> struct stat *rst)
>  	return 1;
>      }
>      dprintf("write boot_image to 0x%x\n",
BTRFS_EXTLINUX_OFFSET);
> -    if (xpwrite(devfd, syslinux_adv, 2 * ADV_SIZE, BTRFS_ADV_OFFSET)
> -	!= 2 * ADV_SIZE) {
> -	perror("writing adv");
> +    if (ext_write_adv_offset(devfd, BTRFS_ADV_OFFSET)) {
>  	return 1;
>      }
>      dprintf("write adv to 0x%x\n", BTRFS_ADV_OFFSET);
> @@ -1415,59 +1465,6 @@ 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)
> -{
> -    const size_t adv_size = 2 * ADV_SIZE;
> -
> -    if (xpread(devfd, syslinux_adv, adv_size, boot_image_len) != adv_size)
> -	return -1;
> -
> -    return syslinux_validate_adv(syslinux_adv) ? 1 : 0;
> -}
> -
> -static int ext_read_adv(const char *path, int devfd, const char **namep)
> -{
> -    int err;
> -    const char *name;
> -
> -    if (fs_type == BTRFS) {
> -	/* btrfs "ldlinux.sys" is in 64k blank area */
> -	return btrfs_read_adv(devfd);
> -    } else if (fs_type == XFS) {
> -	/* XFS "ldlinux.sys" is in the first 2048 bytes of the primary
AG */
> -	return xfs_read_adv(devfd);
> -    } else {
> -	err = read_adv(path, name = "ldlinux.sys");
> -	if (err == 2)		/* ldlinux.sys does not exist */
> -	    err = read_adv(path, name = "extlinux.sys");
> -	if (namep)
> -	    *namep = name;
> -	return err;
> -    }
> -}
> -
> -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;
> -    }
> -    return write_adv(path, cfg);
> -}
> -
>  static int install_loader(const char *path, int update_only)
>  {
>      struct stat st, fst;
> --
> 2.6.2
>
I guess this patch is a replacement of your former submission:
http://www.syslinux.org/archives/2015-November/024554.html
Right?

When this happen, please state so to ease the reviewing process.

Celelibi

Right sorry for that ;)
 


    Le Mercredi 18 novembre 2015 7h31, Celelibi <celelibi at gmail.com> a
?crit :
 

 2015-11-13 20:35 UTC+01:00, Nicolas Cornu via Syslinux <syslinux at
zytor.com>:
> 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.
>
> Use those new functions in rewrite_boot_image and btrfs_install_file
> where it is actually hardcoded.
>
> Signed-off-by: Nicolas Cornu <nicolac76 at yahoo.fr>
> ---
>? extlinux/main.c | 113
> +++++++++++++++++++++++++++-----------------------------
>? 1 file changed, 55 insertions(+), 58 deletions(-)
>
> diff --git a/extlinux/main.c b/extlinux/main.c
> index 6871fb1..a2a396a 100644
> --- a/extlinux/main.c
> +++ b/extlinux/main.c
> @@ -338,6 +338,59 @@ static int patch_file_and_bootblock(int fd, const char
> *dir, int devfd)
>? ? ? return rv;
>? }
>
> +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, offset) != adv_size)
> +? ? ? return -1;
> +
> +? ? return syslinux_validate_adv(syslinux_adv) ? 1 : 0;
> +}
> +
> +static int ext_read_adv(const char *path, int devfd, const char **namep)
> +{
> +? ? int err;
> +? ? const char *name;
> +
> +? ? if (fs_type == BTRFS) {
> +??? /* btrfs "ldlinux.sys" is in 64k blank area */
> +??? 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 ext_read_adv_offset(devfd, boot_image_len);
> +? ? } else {
> +??? err = read_adv(path, name = "ldlinux.sys");
> +??? if (err == 2)??? ??? /* ldlinux.sys does not exist */
> +??? ? ? err = read_adv(path, name = "extlinux.sys");
> +??? if (namep)
> +??? ? ? *namep = name;
> +??? return err;
> +? ? }
> +}
> +
> +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;
> +}
> +
> +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 */
> +??? return ext_write_adv_offset(devfd, BTRFS_ADV_OFFSET);
> +? ? } else {
> +??? return write_adv(path, cfg);
> +? ? }
> +}
> +
>? /*
>? * Install the boot block on the specified device.
>? * Must be run AFTER install_file()!
> @@ -484,8 +537,7 @@ static int rewrite_boot_image(int devfd, const char
> *path, const char *filename)
>? ? ? }
>
>? ? ? /* Write ADV */
> -? ? ret = xpwrite(fd, syslinux_adv, 2 * ADV_SIZE, boot_image_len);
> -? ? if (ret != 2 * ADV_SIZE) {
> +? ? if (ext_write_adv_offset(fd, boot_image_len)) {
>? ??? fprintf(stderr, "%s: write failure on %s\n", program,
filename);
>? ??? goto error;
>? ? ? }
> @@ -614,9 +666,7 @@ int btrfs_install_file(const char *path, int devfd,
> struct stat *rst)
>? ??? return 1;
>? ? ? }
>? ? ? dprintf("write boot_image to 0x%x\n",
BTRFS_EXTLINUX_OFFSET);
> -? ? if (xpwrite(devfd, syslinux_adv, 2 * ADV_SIZE, BTRFS_ADV_OFFSET)
> -??? != 2 * ADV_SIZE) {
> -??? perror("writing adv");
> +? ? if (ext_write_adv_offset(devfd, BTRFS_ADV_OFFSET)) {
>? ??? return 1;
>? ? ? }
>? ? ? dprintf("write adv to 0x%x\n", BTRFS_ADV_OFFSET);
> @@ -1415,59 +1465,6 @@ 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)
> -{
> -? ? const size_t adv_size = 2 * ADV_SIZE;
> -
> -? ? if (xpread(devfd, syslinux_adv, adv_size, boot_image_len) != adv_size)
> -??? return -1;
> -
> -? ? return syslinux_validate_adv(syslinux_adv) ? 1 : 0;
> -}
> -
> -static int ext_read_adv(const char *path, int devfd, const char **namep)
> -{
> -? ? int err;
> -? ? const char *name;
> -
> -? ? if (fs_type == BTRFS) {
> -??? /* btrfs "ldlinux.sys" is in 64k blank area */
> -??? return btrfs_read_adv(devfd);
> -? ? } else if (fs_type == XFS) {
> -??? /* XFS "ldlinux.sys" is in the first 2048 bytes of the
primary AG */
> -??? return xfs_read_adv(devfd);
> -? ? } else {
> -??? err = read_adv(path, name = "ldlinux.sys");
> -??? if (err == 2)??? ??? /* ldlinux.sys does not exist */
> -??? ? ? err = read_adv(path, name = "extlinux.sys");
> -??? if (namep)
> -??? ? ? *namep = name;
> -??? return err;
> -? ? }
> -}
> -
> -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;
> -? ? }
> -? ? return write_adv(path, cfg);
> -}
> -
>? static int install_loader(const char *path, int update_only)
>? {
>? ? ? struct stat st, fst;
> --
> 2.6.2
>
I guess this patch is a replacement of your former submission:
http://www.syslinux.org/archives/2015-November/024554.html
Right?

When this happen, please state so to ease the reviewing process.

Celelibi

On Fri, 13 Nov 2015 20:35:29 +0100
Nicolas Cornu via Syslinux <syslinux at zytor.com> wrote:
> 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.
> 
> Use those new functions in rewrite_boot_image and btrfs_install_file
> where it is actually hardcoded.
> 
> Signed-off-by: Nicolas Cornu <nicolac76 at yahoo.fr>
> ---
>  extlinux/main.c | 113
> +++++++++++++++++++++++++++----------------------------- 1 file
> changed, 55 insertions(+), 58 deletions(-)
Applied, thanks.

Paulo

-- 
Paulo Alcantara, HP Inc.
Speaking for myself only.

> On Fri, 13 Nov 2015 20:35:29 +0100
> Nicolas Cornu via Syslinux <syslinux at zytor.com> wrote:
> 
> > 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.
> > 
> > Use those new functions in rewrite_boot_image and btrfs_install_file
> > where it is actually hardcoded.
> > 
> > Signed-off-by: Nicolas Cornu <nicolac76 at yahoo.fr>
> > ---
> >  extlinux/main.c | 113
> > +++++++++++++++++++++++++++----------------------------- 1 file
> > changed, 55 insertions(+), 58 deletions(-)
> 
> Applied, thanks.
> 
> Paulo
> 
> -- 
 
I could be wrong but, wasn't there a later set of patches, _replacing_ 
this one? Am I mixing unrelated / independent set of patches?

Also, simplification / unification sometimes might imply that a later 
check / test / review / patch (e.g. for one particular supported 
filesystem, or adding support for a new fs) might be more complicated, 
or might (unwillingly) affect other filesystem support. Am I wrong?

Regards,
Ady.