[Buildroot] [PATCH v2 0/2] package/makedevs: allow recursive on directory with symlinks

[Buildroot] [PATCH v2 0/2] package/makedevs: allow recursive on directory with symlinks

[Joachim Wiberg]

Hi,

while fixing the chmod() issue with symlinks in makedevs, I realized
the program was also in need for some coding style cleanup as well.
So I'm including a patch just to quell my inner daemons, you may of
course skip that part entirely if you don't agree with my changes :)

The first version was reviewed by Yann on IRC, suggesting we instead
use lchmod() everywhere instead of skipping symlinks.  I deceided to
only change the two places that made sense.

Best regards
 /Joachim

Joachim Wiberg (2):
  package/makedevs: allow recursive on directory with symlinks
  package/makedevs: coding style and whitespace cleanup

 package/makedevs/makedevs.c | 78 +++++++++++++++++++++++--------------
 1 file changed, 49 insertions(+), 29 deletions(-)

-- 
2.25.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH v2 1/2] package/makedevs: allow recursive on directory with symlinks

[Joachim Wiberg]

When using BR2_ROOTFS_DEVICE_TABLE to change ownership of /etc, like so:

  /etc        r  -1 root     wheel     - - - - -

makdevs fails due to it trying to chown() a symlink:

  makedevs: chown failed for /src/myLinux/output/build/buildroot-fs/ext2/target/etc/mtab: No such file or directory
  makedevs: line 25: recursive failed for /src/myLinux/output/build/buildroot-fs/ext2/target/etc: No such file or directory
  make[2]: *** [fs/ext2/ext2.mk:63: /src/myLinux/output/images/rootfs.ext2] Error 1
  make[1]: *** [Makefile:84: _all] Error 2
  make[1]: Leaving directory '/src/myLinux/buildroot'

This patch changes chown() to lchown() in two cases in makedevs.c when
the argument can be a symlink.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 package/makedevs/makedevs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/package/makedevs/makedevs.c b/package/makedevs/makedevs.c
index c57b964f5c..634ae7c3d8 100644
--- a/package/makedevs/makedevs.c
+++ b/package/makedevs/makedevs.c
@@ -440,7 +440,7 @@ void bb_show_usage(void)
 int bb_recursive(const char *fpath, const struct stat *sb,
 		int tflag, struct FTW *ftwbuf){
 
-	if (chown(fpath, recursive_uid, recursive_gid) == -1) {
+	if (lchown(fpath, recursive_uid, recursive_gid) == -1) {
 		bb_perror_msg("chown failed for %s", fpath);
 		return -1;
 	}
@@ -628,7 +628,7 @@ int main(int argc, char **argv)
 				if (mknod(full_name_inc, mode, rdev) < 0) {
 					bb_perror_msg("line %d: can't create node %s", linenum, full_name_inc);
 					ret = EXIT_FAILURE;
-				} else if (chown(full_name_inc, uid, gid) < 0) {
+				} else if (lchown(full_name_inc, uid, gid) < 0) {
 					bb_perror_msg("line %d: can't chown %s", linenum, full_name_inc);
 					ret = EXIT_FAILURE;
 				} else if (chmod(full_name_inc, mode) < 0) {
-- 
2.25.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH v2 2/2] package/makedevs: coding style and whitespace cleanup

[Joachim Wiberg]

This program is cobbled up with parts from all over the place, mostly
BusyBox.  This patch is a modest attempt to clean up the coding style
and fix some whitespace issues.  Some changes, e.g., comments are for
consistency with the rest fo the program.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 package/makedevs/makedevs.c | 74 +++++++++++++++++++++++--------------
 1 file changed, 47 insertions(+), 27 deletions(-)

diff --git a/package/makedevs/makedevs.c b/package/makedevs/makedevs.c
index 634ae7c3d8..d321b9651a 100644
--- a/package/makedevs/makedevs.c
+++ b/package/makedevs/makedevs.c
@@ -44,7 +44,7 @@ uid_t recursive_uid;
 gid_t recursive_gid;
 unsigned int recursive_mode;
 #define PASSWD_PATH "etc/passwd"  /* MUST be relative */
-#define GROUP_PATH "etc/group"  /* MUST be relative */
+#define GROUP_PATH  "etc/group"   /* MUST be relative */
 
 void bb_verror_msg(const char *s, va_list p)
 {
@@ -76,10 +76,14 @@ void bb_error_msg_and_die(const char *s, ...)
 
 void bb_vperror_msg(const char *s, va_list p)
 {
-	int err=errno;
-	if(s == 0) s = "";
+	int err = errno;
+
+	if (s == 0)
+	  s = "";
 	bb_verror_msg(s, p);
-	if (*s) s = ": ";
+
+	if (*s)
+	  s = ": ";
 	fprintf(stderr, "%s%s\n", s, strerror(err));
 }
 
@@ -99,14 +103,17 @@ void bb_perror_msg_and_die(const char *s, ...)
 	va_start(p, s);
 	bb_vperror_msg(s, p);
 	va_end(p);
+
 	exit(1);
 }
 
 FILE *bb_xfopen(const char *path, const char *mode)
 {
 	FILE *fp;
+
 	if ((fp = fopen(path, mode)) == NULL)
 		bb_perror_msg_and_die("%s", path);
+
 	return fp;
 }
 
@@ -154,8 +161,10 @@ int bb_make_directory (char *path, long mode, int flags)
 		}
 
 		if (mkdir(path, 0777) < 0) {
-			/* If we failed for any other reason than the directory
-			 * already exists, output a diagnostic and return -1.*/
+			/*
+			 * If we failed for any other reason than the directory
+			 * already exists, output a diagnostic and return -1.
+			 */
 			if ((errno != EEXIST && errno != EISDIR)
 					|| !(flags & FILEUTILS_RECUR)
 					|| (stat(path, &st) < 0 || !S_ISDIR(st.st_mode))) {
@@ -163,9 +172,12 @@ int bb_make_directory (char *path, long mode, int flags)
 				umask(mask);
 				break;
 			}
-			/* Since the directory exists, don't attempt to change
+
+			/*
+			 * Since the directory exists, don't attempt to change
 			 * permissions if it was the full target.  Note that
-			 * this is not an error conditon. */
+			 * this is not an error conditon.
+			 */
 			if (!c) {
 				umask(mask);
 				return 0;
@@ -173,11 +185,13 @@ int bb_make_directory (char *path, long mode, int flags)
 		}
 
 		if (!c) {
-			/* Done.  If necessary, updated perms on the newly
+			/*
+			 * Done.  If necessary, updated perms on the newly
 			 * created directory.  Failure to update here _is_
-			 * an error.*/
+			 * an error.
+			 */
 			umask(mask);
-			if ((mode != -1) && (chmod(path, mode) < 0)){
+			if ((mode != -1) && (chmod(path, mode) < 0)) {
 				fail_msg = "set permissions of";
 				break;
 			}
@@ -198,16 +212,20 @@ const char * const bb_msg_memory_exhausted = "memory exhausted";
 void *xmalloc(size_t size)
 {
 	void *ptr = malloc(size);
+
 	if (ptr == NULL && size != 0)
 		bb_error_msg_and_die(bb_msg_memory_exhausted);
+
 	return ptr;
 }
 
 void *xcalloc(size_t nmemb, size_t size)
 {
 	void *ptr = calloc(nmemb, size);
+
 	if (ptr == NULL && nmemb != 0 && size != 0)
 		bb_error_msg_and_die(bb_msg_memory_exhausted);
+
 	return ptr;
 }
 
@@ -216,6 +234,7 @@ void *xrealloc(void *ptr, size_t size)
 	ptr = realloc(ptr, size);
 	if (ptr == NULL && size != 0)
 		bb_error_msg_and_die(bb_msg_memory_exhausted);
+
 	return ptr;
 }
 
@@ -249,6 +268,7 @@ char *private_get_line_from_file(FILE *file, int c)
 		}
 		linebuf[idx] = 0;
 	}
+
 	return linebuf;
 }
 
@@ -263,7 +283,7 @@ long my_getpwnam(const char *name)
 	FILE *stream;
 
 	stream = bb_xfopen(PASSWD_PATH, "r");
-	while(1) {
+	while (1) {
 		errno = 0;
 		myuser = fgetpwent(stream);
 		if (myuser == NULL)
@@ -284,7 +304,7 @@ long my_getgrnam(const char *name)
 	FILE *stream;
 
 	stream = bb_xfopen(GROUP_PATH, "r");
-	while(1) {
+	while (1) {
 		errno = 0;
 		mygroup = fgetgrent(stream);
 		if (mygroup == NULL)
@@ -312,14 +332,16 @@ unsigned long get_ug_id(const char *s, long (*my_getxxnam)(const char *))
 	return r;
 }
 
-char * last_char_is(const char *s, int c)
+char *last_char_is(const char *s, int c)
 {
 	char *sret = (char *)s;
+
 	if (sret) {
 		sret = strrchr(sret, c);
-		if(sret != NULL && *(sret+1) != 0)
+		if (sret != NULL && *(sret+1) != 0)
 			sret = NULL;
 	}
+
 	return sret;
 }
 
@@ -437,9 +459,8 @@ void bb_show_usage(void)
 	exit(1);
 }
 
-int bb_recursive(const char *fpath, const struct stat *sb,
-		int tflag, struct FTW *ftwbuf){
-
+int bb_recursive(const char *fpath, const struct stat *_, int __, struct FTW *___)
+{
 	if (lchown(fpath, recursive_uid, recursive_gid) == -1) {
 		bb_perror_msg("chown failed for %s", fpath);
 		return -1;
@@ -467,7 +488,7 @@ int main(int argc, char **argv)
 	bb_applet_name = basename(argv[0]);
 
 	while ((opt = getopt(argc, argv, "d:")) != -1) {
-		switch(opt) {
+		switch (opt) {
 			case 'd':
 				table = bb_xfopen((line=optarg), "r");
 				break;
@@ -528,8 +549,7 @@ int main(int argc, char **argv)
 		if ((2 > sscanf(line, "%4095s %c %o %40s %40s %u %u %u %u %u", name,
 						&type, &mode, user, group, &major,
 						&minor, &start, &increment, &count)) ||
-				((major | minor | start | count | increment) > 0xfffff))
-		{
+				((major | minor | start | count | increment) > 0xfffff)) {
 			if (*line=='\0' || *line=='#' || isspace(*line))
 				continue;
 			bb_error_msg("line %d invalid: '%s'\n", linenum, line);
@@ -565,16 +585,17 @@ int main(int argc, char **argv)
 				ret = EXIT_FAILURE;
 				goto loop;
 			}
-			if ((mode != -1) && (chmod(full_name, mode) < 0)){
+			if ((mode != -1) && (chmod(full_name, mode) < 0)) {
 				bb_perror_msg("line %d: chmod failed for %s", linenum, full_name);
 				ret = EXIT_FAILURE;
 				goto loop;
 			}
 		} else if (type == 'f' || type == 'F') {
 			struct stat st;
+
 			if ((stat(full_name, &st) < 0 || !S_ISREG(st.st_mode))) {
 				if (type == 'F') {
-					continue; /*Ignore optional files*/
+					continue; /* Ignore optional files */
 				}
 				bb_perror_msg("line %d: regular file '%s' does not exist", linenum, full_name);
 				ret = EXIT_FAILURE;
@@ -585,7 +606,7 @@ int main(int argc, char **argv)
 				ret = EXIT_FAILURE;
 				goto loop;
 			}
-			if ((mode != -1) && (chmod(full_name, mode) < 0)){
+			if ((mode != -1) && (chmod(full_name, mode) < 0)) {
 				bb_perror_msg("line %d: chmod failed for %s", linenum, full_name);
 				ret = EXIT_FAILURE;
 				goto loop;
@@ -599,8 +620,7 @@ int main(int argc, char **argv)
 				ret = EXIT_FAILURE;
 				goto loop;
 			}
-		} else
-		{
+		} else {
 			dev_t rdev;
 			unsigned i;
 			char *full_name_inc;
@@ -619,7 +639,7 @@ int main(int argc, char **argv)
 				goto loop;
 			}
 
-			full_name_inc = xmalloc(strlen(full_name) + sizeof(int)*3 + 2);
+			full_name_inc = xmalloc(strlen(full_name) + sizeof(int) * 3 + 2);
 			if (count)
 				count--;
 			for (i = start; i <= start + count; i++) {
-- 
2.25.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/2] package/makedevs: allow recursive on directory with symlinks

[Arnout Vandecappelle]

On 22/12/2021 09:35, Joachim Wiberg wrote:
> When using BR2_ROOTFS_DEVICE_TABLE to change ownership of /etc, like so:
> 
>    /etc        r  -1 root     wheel     - - - - -
> 
> makdevs fails due to it trying to chown() a symlink:

  The problem is actually a *dangling* symlink, not a symlink per se.


> 
>    makedevs: chown failed for /src/myLinux/output/build/buildroot-fs/ext2/target/etc/mtab: No such file or directory
>    makedevs: line 25: recursive failed for /src/myLinux/output/build/buildroot-fs/ext2/target/etc: No such file or directory
>    make[2]: *** [fs/ext2/ext2.mk:63: /src/myLinux/output/images/rootfs.ext2] Error 1
>    make[1]: *** [Makefile:84: _all] Error 2
>    make[1]: Leaving directory '/src/myLinux/buildroot'
> 
> This patch changes chown() to lchown() in two cases in makedevs.c when
> the argument can be a symlink.
> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---
>   package/makedevs/makedevs.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/package/makedevs/makedevs.c b/package/makedevs/makedevs.c
> index c57b964f5c..634ae7c3d8 100644
> --- a/package/makedevs/makedevs.c
> +++ b/package/makedevs/makedevs.c
> @@ -440,7 +440,7 @@ void bb_show_usage(void)
>   int bb_recursive(const char *fpath, const struct stat *sb,
>   		int tflag, struct FTW *ftwbuf){
>   
> -	if (chown(fpath, recursive_uid, recursive_gid) == -1) {
> +	if (lchown(fpath, recursive_uid, recursive_gid) == -1) {

  In this v2 you change the chown, but the chmod below still remains. Doesn't it 
have the same problem with a dangling symlink?

  Regards,
  Arnout

>   		bb_perror_msg("chown failed for %s", fpath);
>   		return -1;
>   	}
> @@ -628,7 +628,7 @@ int main(int argc, char **argv)
>   				if (mknod(full_name_inc, mode, rdev) < 0) {
>   					bb_perror_msg("line %d: can't create node %s", linenum, full_name_inc);
>   					ret = EXIT_FAILURE;
> -				} else if (chown(full_name_inc, uid, gid) < 0) {
> +				} else if (lchown(full_name_inc, uid, gid) < 0) {
>   					bb_perror_msg("line %d: can't chown %s", linenum, full_name_inc);
>   					ret = EXIT_FAILURE;
>   				} else if (chmod(full_name_inc, mode) < 0) {
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 2/2] package/makedevs: coding style and whitespace cleanup

[Arnout Vandecappelle]

On 22/12/2021 09:35, Joachim Wiberg wrote:
> This program is cobbled up with parts from all over the place, mostly
> BusyBox.  This patch is a modest attempt to clean up the coding style
> and fix some whitespace issues.  Some changes, e.g., comments are for
> consistency with the rest fo the program.

  Although I'm OK with the changes per se, I general I don't believe much in 
manually maintained coding style. So I'd propose to at least include a 
.clang-format file in the top level directory that defines our coding style. We 
wouldn't enforce it with CI or anything (at least, not for the time being), but 
at least it makes it easy to apply the coding style and to know that it's correct.

  As to which coding style: something close to kernel is preferred.

  Regards,
  Arnout

> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---
>   package/makedevs/makedevs.c | 74 +++++++++++++++++++++++--------------
>   1 file changed, 47 insertions(+), 27 deletions(-)
> 
> diff --git a/package/makedevs/makedevs.c b/package/makedevs/makedevs.c
> index 634ae7c3d8..d321b9651a 100644
> --- a/package/makedevs/makedevs.c
> +++ b/package/makedevs/makedevs.c
> @@ -44,7 +44,7 @@ uid_t recursive_uid;
>   gid_t recursive_gid;
>   unsigned int recursive_mode;
>   #define PASSWD_PATH "etc/passwd"  /* MUST be relative */
> -#define GROUP_PATH "etc/group"  /* MUST be relative */
> +#define GROUP_PATH  "etc/group"   /* MUST be relative */
>   
>   void bb_verror_msg(const char *s, va_list p)
>   {
> @@ -76,10 +76,14 @@ void bb_error_msg_and_die(const char *s, ...)
>   
>   void bb_vperror_msg(const char *s, va_list p)
>   {
> -	int err=errno;
> -	if(s == 0) s = "";
> +	int err = errno;
> +
> +	if (s == 0)
> +	  s = "";
>   	bb_verror_msg(s, p);
> -	if (*s) s = ": ";
> +
> +	if (*s)
> +	  s = ": ";
>   	fprintf(stderr, "%s%s\n", s, strerror(err));
>   }
>   
> @@ -99,14 +103,17 @@ void bb_perror_msg_and_die(const char *s, ...)
>   	va_start(p, s);
>   	bb_vperror_msg(s, p);
>   	va_end(p);
> +
>   	exit(1);
>   }
>   
>   FILE *bb_xfopen(const char *path, const char *mode)
>   {
>   	FILE *fp;
> +
>   	if ((fp = fopen(path, mode)) == NULL)
>   		bb_perror_msg_and_die("%s", path);
> +
>   	return fp;
>   }
>   
> @@ -154,8 +161,10 @@ int bb_make_directory (char *path, long mode, int flags)
>   		}
>   
>   		if (mkdir(path, 0777) < 0) {
> -			/* If we failed for any other reason than the directory
> -			 * already exists, output a diagnostic and return -1.*/
> +			/*
> +			 * If we failed for any other reason than the directory
> +			 * already exists, output a diagnostic and return -1.
> +			 */
>   			if ((errno != EEXIST && errno != EISDIR)
>   					|| !(flags & FILEUTILS_RECUR)
>   					|| (stat(path, &st) < 0 || !S_ISDIR(st.st_mode))) {
> @@ -163,9 +172,12 @@ int bb_make_directory (char *path, long mode, int flags)
>   				umask(mask);
>   				break;
>   			}
> -			/* Since the directory exists, don't attempt to change
> +
> +			/*
> +			 * Since the directory exists, don't attempt to change
>   			 * permissions if it was the full target.  Note that
> -			 * this is not an error conditon. */
> +			 * this is not an error conditon.
> +			 */
>   			if (!c) {
>   				umask(mask);
>   				return 0;
> @@ -173,11 +185,13 @@ int bb_make_directory (char *path, long mode, int flags)
>   		}
>   
>   		if (!c) {
> -			/* Done.  If necessary, updated perms on the newly
> +			/*
> +			 * Done.  If necessary, updated perms on the newly
>   			 * created directory.  Failure to update here _is_
> -			 * an error.*/
> +			 * an error.
> +			 */
>   			umask(mask);
> -			if ((mode != -1) && (chmod(path, mode) < 0)){
> +			if ((mode != -1) && (chmod(path, mode) < 0)) {
>   				fail_msg = "set permissions of";
>   				break;
>   			}
> @@ -198,16 +212,20 @@ const char * const bb_msg_memory_exhausted = "memory exhausted";
>   void *xmalloc(size_t size)
>   {
>   	void *ptr = malloc(size);
> +
>   	if (ptr == NULL && size != 0)
>   		bb_error_msg_and_die(bb_msg_memory_exhausted);
> +
>   	return ptr;
>   }
>   
>   void *xcalloc(size_t nmemb, size_t size)
>   {
>   	void *ptr = calloc(nmemb, size);
> +
>   	if (ptr == NULL && nmemb != 0 && size != 0)
>   		bb_error_msg_and_die(bb_msg_memory_exhausted);
> +
>   	return ptr;
>   }
>   
> @@ -216,6 +234,7 @@ void *xrealloc(void *ptr, size_t size)
>   	ptr = realloc(ptr, size);
>   	if (ptr == NULL && size != 0)
>   		bb_error_msg_and_die(bb_msg_memory_exhausted);
> +
>   	return ptr;
>   }
>   
> @@ -249,6 +268,7 @@ char *private_get_line_from_file(FILE *file, int c)
>   		}
>   		linebuf[idx] = 0;
>   	}
> +
>   	return linebuf;
>   }
>   
> @@ -263,7 +283,7 @@ long my_getpwnam(const char *name)
>   	FILE *stream;
>   
>   	stream = bb_xfopen(PASSWD_PATH, "r");
> -	while(1) {
> +	while (1) {
>   		errno = 0;
>   		myuser = fgetpwent(stream);
>   		if (myuser == NULL)
> @@ -284,7 +304,7 @@ long my_getgrnam(const char *name)
>   	FILE *stream;
>   
>   	stream = bb_xfopen(GROUP_PATH, "r");
> -	while(1) {
> +	while (1) {
>   		errno = 0;
>   		mygroup = fgetgrent(stream);
>   		if (mygroup == NULL)
> @@ -312,14 +332,16 @@ unsigned long get_ug_id(const char *s, long (*my_getxxnam)(const char *))
>   	return r;
>   }
>   
> -char * last_char_is(const char *s, int c)
> +char *last_char_is(const char *s, int c)
>   {
>   	char *sret = (char *)s;
> +
>   	if (sret) {
>   		sret = strrchr(sret, c);
> -		if(sret != NULL && *(sret+1) != 0)
> +		if (sret != NULL && *(sret+1) != 0)
>   			sret = NULL;
>   	}
> +
>   	return sret;
>   }
>   
> @@ -437,9 +459,8 @@ void bb_show_usage(void)
>   	exit(1);
>   }
>   
> -int bb_recursive(const char *fpath, const struct stat *sb,
> -		int tflag, struct FTW *ftwbuf){
> -
> +int bb_recursive(const char *fpath, const struct stat *_, int __, struct FTW *___)
> +{
>   	if (lchown(fpath, recursive_uid, recursive_gid) == -1) {
>   		bb_perror_msg("chown failed for %s", fpath);
>   		return -1;
> @@ -467,7 +488,7 @@ int main(int argc, char **argv)
>   	bb_applet_name = basename(argv[0]);
>   
>   	while ((opt = getopt(argc, argv, "d:")) != -1) {
> -		switch(opt) {
> +		switch (opt) {
>   			case 'd':
>   				table = bb_xfopen((line=optarg), "r");
>   				break;
> @@ -528,8 +549,7 @@ int main(int argc, char **argv)
>   		if ((2 > sscanf(line, "%4095s %c %o %40s %40s %u %u %u %u %u", name,
>   						&type, &mode, user, group, &major,
>   						&minor, &start, &increment, &count)) ||
> -				((major | minor | start | count | increment) > 0xfffff))
> -		{
> +				((major | minor | start | count | increment) > 0xfffff)) {
>   			if (*line=='\0' || *line=='#' || isspace(*line))
>   				continue;
>   			bb_error_msg("line %d invalid: '%s'\n", linenum, line);
> @@ -565,16 +585,17 @@ int main(int argc, char **argv)
>   				ret = EXIT_FAILURE;
>   				goto loop;
>   			}
> -			if ((mode != -1) && (chmod(full_name, mode) < 0)){
> +			if ((mode != -1) && (chmod(full_name, mode) < 0)) {
>   				bb_perror_msg("line %d: chmod failed for %s", linenum, full_name);
>   				ret = EXIT_FAILURE;
>   				goto loop;
>   			}
>   		} else if (type == 'f' || type == 'F') {
>   			struct stat st;
> +
>   			if ((stat(full_name, &st) < 0 || !S_ISREG(st.st_mode))) {
>   				if (type == 'F') {
> -					continue; /*Ignore optional files*/
> +					continue; /* Ignore optional files */
>   				}
>   				bb_perror_msg("line %d: regular file '%s' does not exist", linenum, full_name);
>   				ret = EXIT_FAILURE;
> @@ -585,7 +606,7 @@ int main(int argc, char **argv)
>   				ret = EXIT_FAILURE;
>   				goto loop;
>   			}
> -			if ((mode != -1) && (chmod(full_name, mode) < 0)){
> +			if ((mode != -1) && (chmod(full_name, mode) < 0)) {
>   				bb_perror_msg("line %d: chmod failed for %s", linenum, full_name);
>   				ret = EXIT_FAILURE;
>   				goto loop;
> @@ -599,8 +620,7 @@ int main(int argc, char **argv)
>   				ret = EXIT_FAILURE;
>   				goto loop;
>   			}
> -		} else
> -		{
> +		} else {
>   			dev_t rdev;
>   			unsigned i;
>   			char *full_name_inc;
> @@ -619,7 +639,7 @@ int main(int argc, char **argv)
>   				goto loop;
>   			}
>   
> -			full_name_inc = xmalloc(strlen(full_name) + sizeof(int)*3 + 2);
> +			full_name_inc = xmalloc(strlen(full_name) + sizeof(int) * 3 + 2);
>   			if (count)
>   				count--;
>   			for (i = start; i <= start + count; i++) {
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/2] package/makedevs: allow recursive on directory with symlinks

[Joachim Wiberg]

On 12/22/21 3:06 PM, Arnout Vandecappelle wrote:
> On 22/12/2021 09:35, Joachim Wiberg wrote:
>> When using BR2_ROOTFS_DEVICE_TABLE to change ownership of /etc, like so:
>>    /etc        r  -1 root     wheel     - - - - -
>> makdevs fails due to it trying to chown() a symlink:
>>    makedevs: chown failed for
>> /src/myLinux/output/build/buildroot-fs/ext2/target/etc/mtab: No such
>> file or directory
> 
>  The problem is actually a *dangling* symlink, not a symlink per se.

Ouch, you're completely right!  The v1 patch was much more on point, I
should know better than relying on strangers in IRC chat rooms ;-)

>>   -    if (chown(fpath, recursive_uid, recursive_gid) == -1) {
>> +    if (lchown(fpath, recursive_uid, recursive_gid) == -1) {
>  In this v2 you change the chown, but the chmod below still remains.
> Doesn't it have the same problem with a dangling symlink?

Indeed it does.  Seems I didn't test that case properly.  I.e., worked
fine in my particular use-case.  I've done a separate test program to
verify, and we need something similar to this at the beginning of the
bb_recursive() function:

	if (type == FTW_SL) {
		if (access(fpath, F_OK)) {
			warn("skipping %s, dangling symlink", fpath);
			return 0;
		}
	}

Thanks for the awesome feedback, I'll get right on a v3!


Best regards
 /Joachim

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 2/2] package/makedevs: coding style and whitespace cleanup

[Joachim Wiberg]

On 12/22/21 3:12 PM, Arnout Vandecappelle wrote:
> On 22/12/2021 09:35, Joachim Wiberg wrote:
>> This patch is a modest attempt to clean up the coding style and fix
>> some whitespace issues.  Some changes, e.g., comments are for
>> consistency with the rest for the program.
>  Although I'm OK with the changes per se, I general I don't believe much
> in manually maintained coding style. So I'd propose to at least include
> a .clang-format file in the top level directory that defines our coding
> style. We wouldn't enforce it with CI or anything (at least, not for the
> time being), but at least it makes it easy to apply the coding style and
> to know that it's correct.

OK, that's a little bit more than I bargained for.  Just wanted to clean
it up a bit while I was in that part of the tree.  But I'll give it a
go, it's Christmas (and DEC 22!!) after all :-)

We don't seem to have any .clang-format file at the moment, so if I copy
the latest version from kernel.org, where in the tree should it live?
I'm thinking we might want to reuse it for other in-tree tools, maybe
support/misc/, or perhaps each package should supply its own?

> As to which coding style: something close to kernel is preferred.

Suspected as much, thanks!

Best regards
 /Joachim
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 2/2] package/makedevs: coding style and whitespace cleanup

[Arnout Vandecappelle]

On 22/12/2021 20:32, Joachim Wiberg wrote:
> On 12/22/21 3:12 PM, Arnout Vandecappelle wrote:
>> On 22/12/2021 09:35, Joachim Wiberg wrote:
>>> This patch is a modest attempt to clean up the coding style and fix
>>> some whitespace issues.  Some changes, e.g., comments are for
>>> consistency with the rest for the program.
>>   Although I'm OK with the changes per se, I general I don't believe much
>> in manually maintained coding style. So I'd propose to at least include
>> a .clang-format file in the top level directory that defines our coding
>> style. We wouldn't enforce it with CI or anything (at least, not for the
>> time being), but at least it makes it easy to apply the coding style and
>> to know that it's correct.
> 
> OK, that's a little bit more than I bargained for.  Just wanted to clean
> it up a bit while I was in that part of the tree.  But I'll give it a
> go, it's Christmas (and DEC 22!!) after all :-)
> 
> We don't seem to have any .clang-format file at the moment, so if I copy
> the latest version from kernel.org, where in the tree should it live?

  At the top level, so clang-format and editors find it out of the box.

  Regards,
  Arnout

> I'm thinking we might want to reuse it for other in-tree tools, maybe
> support/misc/, or perhaps each package should supply its own?
> 
>> As to which coding style: something close to kernel is preferred.
> 
> Suspected as much, thanks!
> 
> Best regards
>   /Joachim
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 2/2] package/makedevs: coding style and whitespace cleanup

[Joachim Wiberg]

On 12/22/21 9:24 PM, Arnout Vandecappelle wrote:
> On 22/12/2021 20:32, Joachim Wiberg wrote:
>> On 12/22/21 3:12 PM, Arnout Vandecappelle wrote:
>>> Although I'm OK with the changes per se, I general I don't believe much
>>> in manually maintained coding style. So I'd propose to at least include
>>> a .clang-format file in the top level directory that defines our coding
>>> style. We wouldn't enforce it with CI or anything (at least, not for the
>>> time being), but at least it makes it easy to apply the coding style and
>>> to know that it's correct.
>> We don't seem to have any .clang-format file at the moment, so if I copy
>> the latest version from kernel.org, where in the tree should it live?
>  At the top level, so clang-format and editors find it out of the box.

Apologies, I see now you wrote that already in your original reply, but
it seems my eyes failed me.

I'm compiling a v3 of my patch set now, which includes the .clang-format
as a separate commit.  I expect there may be some discussion regarding
max line lengths and such, but hopefully it won't be too controversial.

Best regards
 /Joachim

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot