thr3ads.net - Syslinux - [syslinux] [PATCH 4/4] extlinux: simplification by removing one label [Nov 2015]

If this information is useful, please help other people find it:
Share via:

Nicolas Cornu

2015-Nov-13 20:19 UTC

[syslinux] [PATCH 1/4] extlinux: simplification

Merge installation of ldlinux.c32 from ext2_fat_install_file,
btrfs_install_file and xfs_install_file into one function
ext_install_ldlinux_c32

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

diff --git a/extlinux/main.c b/extlinux/main.c
index a2a396a..5bab712 100644
--- a/extlinux/main.c
+++ b/extlinux/main.c
@@ -561,19 +561,51 @@ error:
     return -1;
 }
 
+static int ext_install_ldlinux_c32(const char *path)
+{
+    char *file;
+    int fd, rv;
+
+    rv = asprintf(&file, "%s%sldlinux.c32",
+		  path, path[0] && path[strlen(path) - 1] == '/' ?
"" : "/");
+    if (rv < 0 || !file) {
+	perror(program);
+	return 1;
+    }
+
+    fd = open(file, O_WRONLY | O_TRUNC | O_CREAT | O_SYNC,
+	      S_IRUSR | S_IRGRP | S_IROTH);
+    if (fd < 0) {
+	perror(file);
+	free(file);
+	return 1;
+    }
+
+    rv = xpwrite(fd, (const char _force *)syslinux_ldlinuxc32,
+		 syslinux_ldlinuxc32_len, 0);
+    if (rv != (int)syslinux_ldlinuxc32_len) {
+	fprintf(stderr, "%s: write failure on %s\n", program, file);
+	rv = 1;
+    } else
+	rv = 0;
+
+    close(fd);
+    free(file);
+
+    return rv;
+}
+
 int ext2_fat_install_file(const char *path, int devfd, struct stat *rst)
 {
-    char *file, *oldfile, *c32file;
+    char *file, *oldfile;
     int fd = -1, dirfd = -1;
-    int r1, r2, r3;
+    int r1, r2;
 
     r1 = asprintf(&file, "%s%sldlinux.sys",
 		  path, path[0] && path[strlen(path) - 1] == '/' ?
"" : "/");
     r2 = asprintf(&oldfile, "%s%sextlinux.sys",
 		  path, path[0] && path[strlen(path) - 1] == '/' ?
"" : "/");
-    r3 = asprintf(&c32file, "%s%sldlinux.c32",
-		  path, path[0] && path[strlen(path) - 1] == '/' ?
"" : "/");
-    if (r1 < 0 || !file || r2 < 0 || !oldfile || r3 < 0 || !c32file) {
+    if (r1 < 0 || !file || r2 < 0 || !oldfile) {
 	perror(program);
 	return 1;
     }
@@ -619,23 +651,11 @@ int ext2_fat_install_file(const char *path, int devfd,
struct stat *rst)
 	unlink(oldfile);
     }
 
-    fd = open(c32file, O_WRONLY | O_TRUNC | O_CREAT | O_SYNC,
-	      S_IRUSR | S_IRGRP | S_IROTH);
-    if (fd < 0) {
-	perror(c32file);
-	goto bail;
-    }
-
-    r3 = xpwrite(fd, (const char _force *)syslinux_ldlinuxc32,
-		 syslinux_ldlinuxc32_len, 0);
-    if (r3 != syslinux_ldlinuxc32_len) {
-	fprintf(stderr, "%s: write failure on %s\n", program, c32file);
+    if (ext_install_ldlinux_c32(path))
 	goto bail;
-    }
 
     free(file);
     free(oldfile);
-    free(c32file);
     return 0;
 
 bail:
@@ -646,7 +666,6 @@ bail:
 
     free(file);
     free(oldfile);
-    free(c32file);
     return 1;
 }
 
@@ -655,9 +674,6 @@ bail:
    since the cow feature of btrfs will move the ldlinux.sys every where */
 int btrfs_install_file(const char *path, int devfd, struct stat *rst)
 {
-    char *file;
-    int fd, rv;
-
     patch_file_and_bootblock(-1, path, devfd);
     if (xpwrite(devfd, (const char _force *)boot_image,
 		boot_image_len, BTRFS_EXTLINUX_OFFSET)
@@ -680,32 +696,7 @@ int btrfs_install_file(const char *path, int devfd, struct
stat *rst)
      * it doesn't need to be within the first 64K. The Syslinux core
      * has enough smarts to search the btrfs dirs and find this file.
      */
-    rv = asprintf(&file, "%s%sldlinux.c32",
-		  path, path[0] && path[strlen(path) - 1] == '/' ?
"" : "/");
-    if (rv < 0 || !file) {
-	perror(program);
-	return 1;
-    }
-
-    fd = open(file, O_WRONLY | O_TRUNC | O_CREAT | O_SYNC,
-	      S_IRUSR | S_IRGRP | S_IROTH);
-    if (fd < 0) {
-	perror(file);
-	free(file);
-	return 1;
-    }
-
-    rv = xpwrite(fd, (const char _force *)syslinux_ldlinuxc32,
-		 syslinux_ldlinuxc32_len, 0);
-    if (rv != (int)syslinux_ldlinuxc32_len) {
-	fprintf(stderr, "%s: write failure on %s\n", program, file);
-	rv = 1;
-    } else
-	rv = 0;
-
-    close(fd);
-    free(file);
-    return rv;
+    return ext_install_ldlinux_c32(path);
 }
 
 /*
@@ -720,15 +711,11 @@ int btrfs_install_file(const char *path, int devfd, struct
stat *rst)
 static int xfs_install_file(const char *path, int devfd, struct stat *rst)
 {
     static char file[PATH_MAX + 1];
-    static char c32file[PATH_MAX + 1];
     int dirfd = -1;
     int fd = -1;
-    int retval;
 
     snprintf(file, PATH_MAX + 1, "%s%sldlinux.sys", path,
 	     path[0] && path[strlen(path) - 1] == '/' ? "" :
"/");
-    snprintf(c32file, PATH_MAX + 1, "%s%sldlinux.c32", path,
-	     path[0] && path[strlen(path) - 1] == '/' ? "" :
"/");
 
     dirfd = open(path, O_RDONLY | O_DIRECTORY);
     if (dirfd < 0) {
@@ -767,21 +754,8 @@ static int xfs_install_file(const char *path, int devfd,
struct stat *rst)
     dirfd = -1;
     fd = -1;
 
-    fd = open(c32file, O_WRONLY | O_TRUNC | O_CREAT | O_SYNC,
-	      S_IRUSR | S_IRGRP | S_IROTH);
-    if (fd < 0) {
-	perror(c32file);
-	goto bail;
-    }
-
-    retval = xpwrite(fd, (const char _force *)syslinux_ldlinuxc32,
-		     syslinux_ldlinuxc32_len, 0);
-    if (retval != (int)syslinux_ldlinuxc32_len) {
-	fprintf(stderr, "%s: write failure on %s\n", program, file);
+    if (ext_install_ldlinux_c32(path))
 	goto bail;
-    }
-
-    close(fd);
 
     sync();
 
-- 
2.6.2

Nicolas Cornu

2015-Nov-13 20:19 UTC

head link

[syslinux] [PATCH 2/4] extlinux: rename ext2_fat_install_file

Rename from ext2_fat_install_file to ext_common_install_file as it is
use for ext, vfat, ntfs and ufs.

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

diff --git a/extlinux/main.c b/extlinux/main.c
index 5bab712..c446c75 100644
--- a/extlinux/main.c
+++ b/extlinux/main.c
@@ -595,7 +595,7 @@ static int ext_install_ldlinux_c32(const char *path)
     return rv;
 }
 
-int ext2_fat_install_file(const char *path, int devfd, struct stat *rst)
+int ext_common_install_file(const char *path, int devfd, struct stat *rst)
 {
     char *file, *oldfile;
     int fd = -1, dirfd = -1;
@@ -1007,7 +1007,7 @@ static int install_file(const char *path, int devfd,
struct stat *rst)
 {
     if (fs_type == EXT2 || fs_type == VFAT || fs_type == NTFS
 	|| fs_type == UFS1 || fs_type == UFS2)
-	return ext2_fat_install_file(path, devfd, rst);
+	return ext_common_install_file(path, devfd, rst);
     else if (fs_type == BTRFS)
 	return btrfs_install_file(path, devfd, rst);
     else if (fs_type == XFS)
-- 
2.6.2

Nicolas Cornu

2015-Nov-13 20:19 UTC

head link

[syslinux] [PATCH 3/4] extlinux: simplification

Merge installation of ldlinux.sys from ext_common_install_file and
xfs_install_file into one function ext_install_ldlinux_sys.

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

diff --git a/extlinux/main.c b/extlinux/main.c
index c446c75..43c1a31 100644
--- a/extlinux/main.c
+++ b/extlinux/main.c
@@ -595,32 +595,24 @@ static int ext_install_ldlinux_c32(const char *path)
     return rv;
 }
 
-int ext_common_install_file(const char *path, int devfd, struct stat *rst)
+static int ext_install_ldlinux_sys(int devfd, const char *path, struct stat
*rst)
 {
-    char *file, *oldfile;
-    int fd = -1, dirfd = -1;
-    int r1, r2;
+    char *file;
+    int fd, rv;
 
-    r1 = asprintf(&file, "%s%sldlinux.sys",
-		  path, path[0] && path[strlen(path) - 1] == '/' ?
"" : "/");
-    r2 = asprintf(&oldfile, "%s%sextlinux.sys",
+    rv = asprintf(&file, "%s%sldlinux.sys",
 		  path, path[0] && path[strlen(path) - 1] == '/' ?
"" : "/");
-    if (r1 < 0 || !file || r2 < 0 || !oldfile) {
+    if (rv < 0 || !file) {
 	perror(program);
 	return 1;
     }
 
-    dirfd = open(path, O_RDONLY | O_DIRECTORY);
-    if (dirfd < 0) {
-	perror(path);
-	goto bail;
-    }
-
     fd = open(file, O_RDONLY);
     if (fd < 0) {
 	if (errno != ENOENT) {
 	    perror(file);
-	    goto bail;
+	    free(file);
+	    return 1;
 	}
     } else {
 	clear_attributes(fd);
@@ -628,8 +620,10 @@ int ext_common_install_file(const char *path, int devfd,
struct stat *rst)
     close(fd);
 
     fd = rewrite_boot_image(devfd, path, file);
-    if (fd < 0)
-	goto bail;
+    if (fd < 0) {
+	free(file);
+	return 1;
+    }
 
     /* Attempt to set immutable flag and remove all write access */
     /* Only set immutable flag if file is owned by root */
@@ -637,11 +631,39 @@ int ext_common_install_file(const char *path, int devfd,
struct stat *rst)
 
     if (fstat(fd, rst)) {
 	perror(file);
+	free(file);
+	close(fd);
+	return 1;
+    }
+
+    close(fd);
+
+    return 0;
+}
+
+int ext_common_install_file(const char *path, int devfd, struct stat *rst)
+{
+    char *oldfile;
+    int fd = -1, dirfd = -1;
+    int r1;
+
+    r1 = asprintf(&oldfile, "%s%sextlinux.sys",
+		  path, path[0] && path[strlen(path) - 1] == '/' ?
"" : "/");
+    if (r1 < 0 || !oldfile) {
+	perror(program);
+	return 1;
+    }
+
+    dirfd = open(path, O_RDONLY | O_DIRECTORY);
+    if (dirfd < 0) {
+	perror(path);
 	goto bail;
     }
 
+    if (ext_install_ldlinux_sys(devfd, path, rst))
+	goto bail;
+
     close(dirfd);
-    close(fd);
 
     /* Look if we have the old filename */
     fd = open(oldfile, O_RDONLY);
@@ -654,7 +676,6 @@ int ext_common_install_file(const char *path, int devfd,
struct stat *rst)
     if (ext_install_ldlinux_c32(path))
 	goto bail;
 
-    free(file);
     free(oldfile);
     return 0;
 
@@ -664,7 +685,6 @@ bail:
     if (fd >= 0)
 	close(fd);
 
-    free(file);
     free(oldfile);
     return 1;
 }
@@ -710,12 +730,7 @@ int btrfs_install_file(const char *path, int devfd, struct
stat *rst)
  */
 static int xfs_install_file(const char *path, int devfd, struct stat *rst)
 {
-    static char file[PATH_MAX + 1];
     int dirfd = -1;
-    int fd = -1;
-
-    snprintf(file, PATH_MAX + 1, "%s%sldlinux.sys", path,
-	     path[0] && path[strlen(path) - 1] == '/' ? "" :
"/");
 
     dirfd = open(path, O_RDONLY | O_DIRECTORY);
     if (dirfd < 0) {
@@ -723,36 +738,12 @@ static int xfs_install_file(const char *path, int devfd,
struct stat *rst)
 	goto bail;
     }
 
-    fd = open(file, O_RDONLY);
-    if (fd < 0) {
-	if (errno != ENOENT) {
-	    perror(file);
-	    goto bail;
-	}
-    } else {
-	clear_attributes(fd);
-    }
-
-    close(fd);
-
-    fd = rewrite_boot_image(devfd, path, file);
-    if (fd < 0)
+    if (ext_install_ldlinux_sys(devfd, path, rst))
 	goto bail;
 
-    /* Attempt to set immutable flag and remove all write access */
-    /* Only set immutable flag if file is owned by root */
-    set_attributes(fd);
-
-    if (fstat(fd, rst)) {
-	perror(file);
-	goto bail;
-    }
-
     close(dirfd);
-    close(fd);
 
     dirfd = -1;
-    fd = -1;
 
     if (ext_install_ldlinux_c32(path))
 	goto bail;
@@ -765,9 +756,6 @@ bail:
     if (dirfd >= 0)
 	close(dirfd);
 
-    if (fd >= 0)
-	close(fd);
-
     return 1;
 }
 
-- 
2.6.2

Nicolas Cornu

2015-Nov-13 20:19 UTC

head link

[syslinux] [PATCH 4/4] extlinux: simplification by removing one label

Now that xfs_install_file is simplified by using new helpers, we can
remove use of label/goto for clarity.

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

diff --git a/extlinux/main.c b/extlinux/main.c
index 43c1a31..8b97a71 100644
--- a/extlinux/main.c
+++ b/extlinux/main.c
@@ -735,28 +735,22 @@ static int xfs_install_file(const char *path, int devfd,
struct stat *rst)
     dirfd = open(path, O_RDONLY | O_DIRECTORY);
     if (dirfd < 0) {
 	perror(path);
-	goto bail;
+	return 1;
     }
 
-    if (ext_install_ldlinux_sys(devfd, path, rst))
-	goto bail;
+    if (ext_install_ldlinux_sys(devfd, path, rst)) {
+	close(dirfd);
+	return 1;
+    }
 
     close(dirfd);
 
-    dirfd = -1;
-
     if (ext_install_ldlinux_c32(path))
-	goto bail;
+	return 1;
 
     sync();
 
     return 0;
-
-bail:
-    if (dirfd >= 0)
-	close(dirfd);
-
-    return 1;
 }
 
 /*
-- 
2.6.2

Celelibi

2015-Nov-18 06:33 UTC

head link

[syslinux] [PATCH 4/4] extlinux: simplification by removing one label

2015-11-13 21:19 UTC+01:00, Nicolas Cornu via Syslinux <syslinux at
zytor.com>:> Now that xfs_install_file is simplified by using new helpers, we can
> remove use of label/goto for clarity.
>
> Signed-off-by: Nicolas Cornu <nicolac76 at yahoo.fr>
> ---
>  extlinux/main.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/extlinux/main.c b/extlinux/main.c
> index 43c1a31..8b97a71 100644
> --- a/extlinux/main.c
> +++ b/extlinux/main.c
> @@ -735,28 +735,22 @@ static int xfs_install_file(const char *path, int
> devfd, struct stat *rst)
>      dirfd = open(path, O_RDONLY | O_DIRECTORY);
>      if (dirfd < 0) {
>  	perror(path);
> -	goto bail;
> +	return 1;
>      }
>
> -    if (ext_install_ldlinux_sys(devfd, path, rst))
> -	goto bail;
> +    if (ext_install_ldlinux_sys(devfd, path, rst)) {
> +	close(dirfd);
> +	return 1;
> +    }
>
>      close(dirfd);
>
> -    dirfd = -1;
> -
>      if (ext_install_ldlinux_c32(path))
> -	goto bail;
> +	return 1;
>
>      sync();
>
>      return 0;
> -
> -bail:
> -    if (dirfd >= 0)
> -	close(dirfd);
> -
> -    return 1;
>  }
>
>  /*
> --
> 2.6.2
I would just note that gotos are often used in C (at least in Linux)
as a a simulation of exceptions. The label is at the end of the
function doing some clean up and returning an error code. That's a
common pattern.

That being said, as your patch shorten the code length, I guess we can
say it's better.

Celelibi

Seemingly Similar Threads

Search for more possibly parallel threads

Syslinux - Nov 2015 - [PATCH 4/4] extlinux: simplification by removing one label

[syslinux] [PATCH 1/4] extlinux: simplification

[syslinux] [PATCH 2/4] extlinux: rename ext2_fat_install_file

[syslinux] [PATCH 3/4] extlinux: simplification

[syslinux] [PATCH 4/4] extlinux: simplification by removing one label

[syslinux] [PATCH 4/4] extlinux: simplification by removing one label

Seemingly Similar Threads