Nicolas Cornu
2015-Nov-13 19:35 UTC
[syslinux] [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
Celelibi
2015-Nov-18 06:31 UTC
[syslinux] [PATCH] extlinux: code cleanup and simplification
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
nico cornu
2015-Nov-18 12:35 UTC
[syslinux] [PATCH] extlinux: code cleanup and simplification
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
Paulo Alcantara
2015-Nov-26 00:02 UTC
[syslinux] [PATCH] extlinux: code cleanup and simplification
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.