[Buildroot] [PATCH 1/2] makedevs: resync device creation with upstream busybox

[Buildroot] [PATCH 1/2] makedevs: resync device creation with upstream busybox

[Arnout Vandecappelle]

In upstream busbyox, the code to create devices has been simplified:
the code for a single and for multiple devices is no longer duplicated.

Since we are going to change the device creation code next, it's
convenient to have only one copy to modify.

There are two behavioural changes with this, but they were introduced
silently together with other commits in upstream busybox.

- When mknod() fails, the chmod was still done. This is pointless so it
  is no longer done now.

- There was a check for mode != -1; however, a mode of -1 would not
  have worked anyway because all bits would be set for mknod(), which
  would fail. So this check is redundant.

Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
 package/makedevs/makedevs.c | 47 +++++++++++++++------------------------------
 1 file changed, 15 insertions(+), 32 deletions(-)

diff --git a/package/makedevs/makedevs.c b/package/makedevs/makedevs.c
index cacb144..7092b14 100644
--- a/package/makedevs/makedevs.c
+++ b/package/makedevs/makedevs.c
@@ -599,6 +599,8 @@ int main(int argc, char **argv)
 		} else
 		{
 			dev_t rdev;
+			unsigned i;
+			char *full_name_inc;
 
 			if (type == 'p') {
 				mode |= S_IFIFO;
@@ -614,43 +616,24 @@ int main(int argc, char **argv)
 				goto loop;
 			}
 
-			if (count > 0) {
-				int i;
-				char *full_name_inc;
-
-				full_name_inc = xmalloc(strlen(full_name) + 8);
-				for (i = 0; i < count; i++) {
-					sprintf(full_name_inc, "%s%d", full_name, start + i);
-					rdev = makedev(major, minor + i * increment);
-					if (mknod(full_name_inc, mode, rdev) == -1) {
-						bb_perror_msg("line %d: Couldnt create node %s", linenum, full_name_inc);
-						ret = EXIT_FAILURE;
-					}
-					else if (chown(full_name_inc, uid, gid) == -1) {
-						bb_perror_msg("line %d: chown failed for %s", linenum, full_name_inc);
-						ret = EXIT_FAILURE;
-					}
-					if ((mode != -1) && (chmod(full_name_inc, mode) < 0)){
-						bb_perror_msg("line %d: chmod failed for %s", linenum, full_name_inc);
-						ret = EXIT_FAILURE;
-					}
-				}
-				free(full_name_inc);
-			} else {
-				rdev = makedev(major, minor);
-				if (mknod(full_name, mode, rdev) == -1) {
-					bb_perror_msg("line %d: Couldnt create node %s", linenum, full_name);
+			full_name_inc = xmalloc(strlen(full_name) + sizeof(int)*3 + 2);
+			if (count)
+				count--;
+			for (i = start; i <= start + count; i++) {
+				sprintf(full_name_inc, count ? "%s%u" : "%s", full_name, i);
+				rdev = makedev(major, minor + (i - start) * increment);
+				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, uid, gid) == -1) {
-					bb_perror_msg("line %d: chown failed for %s", linenum, full_name);
+				} else if (chown(full_name_inc, uid, gid) < 0) {
+					bb_perror_msg("line %d: can't chown %s", linenum, full_name_inc);
 					ret = EXIT_FAILURE;
-				}
-				if ((mode != -1) && (chmod(full_name, mode) < 0)){
-					bb_perror_msg("line %d: chmod failed for %s", linenum, full_name);
+				} else if (chmod(full_name_inc, mode) < 0) {
+					bb_perror_msg("line %d: can't chmod %s", linenum, full_name_inc);
 					ret = EXIT_FAILURE;
 				}
 			}
+			free(full_name_inc);
 		}
 loop:
 		free(line);
-- 
2.9.3

[Buildroot] [PATCH 2/2] makedevs: make device node creation idempotent

[Arnout Vandecappelle]

We use makedevs to create device nodes in the target rootfs. However,
this can be called several times, e.g. when building several filesystem
images or when rebuilding. When makedevs is called the second time, the
device node already exists so mknod() errors out.

This wasn't noticed before because fakeroot's mknod() wrapper
(incorrectly) does _not_ error out when the file exists already. Now
we switched from fakeroot to pseudo, the problem becomes apparent.

Before creating the device node, check if it already exists and if so,
if it has the correct device type and number. Change of mode and
ownership is still done.

This approach was preferred over removing the target files before
creating them, which would be simpler. However, when e.g. a file exists
as a normal file and makedevs specifies it as a device node, that
really is an error so we should detect it.

The other types don't have to be changed. The 'd' (directory) type is
already OK because it already only creates directories if they don't
exist yet. The 'f' (file mode) and 'r' (recursive) types only operate
on files and directories that exist already.

Patch also sent upstream to busybox.

Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Reported-by: Fabio Estevam <festevam@gmail.com>
---
v2: check if pre-existing file is of correct type instead of simply
    unlinking (Yann).
---
 package/makedevs/makedevs.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/package/makedevs/makedevs.c b/package/makedevs/makedevs.c
index 7092b14..bcf0e0f 100644
--- a/package/makedevs/makedevs.c
+++ b/package/makedevs/makedevs.c
@@ -601,6 +601,7 @@ int main(int argc, char **argv)
 			dev_t rdev;
 			unsigned i;
 			char *full_name_inc;
+			struct stat st;
 
 			if (type == 'p') {
 				mode |= S_IFIFO;
@@ -622,10 +623,23 @@ int main(int argc, char **argv)
 			for (i = start; i <= start + count; i++) {
 				sprintf(full_name_inc, count ? "%s%u" : "%s", full_name, i);
 				rdev = makedev(major, minor + (i - start) * increment);
-				if (mknod(full_name_inc, mode, rdev) < 0) {
+				if (stat(full_name_inc, &st) == 0) {
+					if ((mode & S_IFMT) != (st.st_mode & S_IFMT)) {
+						bb_error_msg("line %d: node %s exists but is of wrong file type", linenum, full_name_inc);
+						ret = EXIT_FAILURE;
+						continue;
+					}
+					if (st.st_rdev != rdev) {
+						bb_error_msg("line %d: node %s exists but is wrong device number", linenum, full_name_inc);
+						ret = EXIT_FAILURE;
+						continue;
+					}
+				} else 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) {
+					continue;
+				}
+				if (chown(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.9.3

[Buildroot] [PATCH 1/2] makedevs: resync device creation with upstream busybox

[Fabio Estevam]

On Sat, Nov 5, 2016 at 11:38 AM, Arnout Vandecappelle (Essensium/Mind)
<arnout@mind.be> wrote:
> In upstream busbyox, the code to create devices has been simplified:
> the code for a single and for multiple devices is no longer duplicated.
>
> Since we are going to change the device creation code next, it's
> convenient to have only one copy to modify.
>
> There are two behavioural changes with this, but they were introduced
> silently together with other commits in upstream busybox.
>
> - When mknod() fails, the chmod was still done. This is pointless so it
>   is no longer done now.
>
> - There was a check for mode != -1; however, a mode of -1 would not
>   have worked anyway because all bits would be set for mknod(), which
>   would fail. So this check is redundant.
>
> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

Tested-by: Fabio Estevam <festevam@gmail.com>

[Buildroot] [PATCH 2/2] makedevs: make device node creation idempotent

[Fabio Estevam]

Hi Arnout,

On Sat, Nov 5, 2016 at 11:38 AM, Arnout Vandecappelle (Essensium/Mind)
<arnout@mind.be> wrote:
> We use makedevs to create device nodes in the target rootfs. However,
> this can be called several times, e.g. when building several filesystem
> images or when rebuilding. When makedevs is called the second time, the
> device node already exists so mknod() errors out.
>
> This wasn't noticed before because fakeroot's mknod() wrapper
> (incorrectly) does _not_ error out when the file exists already. Now
> we switched from fakeroot to pseudo, the problem becomes apparent.
>
> Before creating the device node, check if it already exists and if so,
> if it has the correct device type and number. Change of mode and
> ownership is still done.
>
> This approach was preferred over removing the target files before
> creating them, which would be simpler. However, when e.g. a file exists
> as a normal file and makedevs specifies it as a device node, that
> really is an error so we should detect it.
>
> The other types don't have to be changed. The 'd' (directory) type is
> already OK because it already only creates directories if they don't
> exist yet. The 'f' (file mode) and 'r' (recursive) types only operate
> on files and directories that exist already.
>
> Patch also sent upstream to busybox.
>
> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> Reported-by: Fabio Estevam <festevam@gmail.com>

This fixes the build error I was seeing, thanks:

Tested-by: Fabio Estevam <festevam@gmail.com>

[Buildroot] [PATCH 1/2] makedevs: resync device creation with upstream busybox

[Yann E. MORIN]

Arnout, All,

On 2016-11-05 14:38 +0100, Arnout Vandecappelle (Essensium/Mind) spake thusly:
> In upstream busbyox, the code to create devices has been simplified:
> the code for a single and for multiple devices is no longer duplicated.
> 
> Since we are going to change the device creation code next, it's
> convenient to have only one copy to modify.
> 
> There are two behavioural changes with this, but they were introduced
> silently together with other commits in upstream busybox.
> 
> - When mknod() fails, the chmod was still done. This is pointless so it
>   is no longer done now.
> 
> - There was a check for mode != -1; however, a mode of -1 would not
>   have worked anyway because all bits would be set for mknod(), which
>   would fail. So this check is redundant.
> 
> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

Acked-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

> ---
>  package/makedevs/makedevs.c | 47 +++++++++++++++------------------------------
>  1 file changed, 15 insertions(+), 32 deletions(-)
> 
> diff --git a/package/makedevs/makedevs.c b/package/makedevs/makedevs.c
> index cacb144..7092b14 100644
> --- a/package/makedevs/makedevs.c
> +++ b/package/makedevs/makedevs.c
> @@ -599,6 +599,8 @@ int main(int argc, char **argv)
>  		} else
>  		{
>  			dev_t rdev;
> +			unsigned i;
> +			char *full_name_inc;
>  
>  			if (type == 'p') {
>  				mode |= S_IFIFO;
> @@ -614,43 +616,24 @@ int main(int argc, char **argv)
>  				goto loop;
>  			}
>  
> -			if (count > 0) {
> -				int i;
> -				char *full_name_inc;
> -
> -				full_name_inc = xmalloc(strlen(full_name) + 8);
> -				for (i = 0; i < count; i++) {
> -					sprintf(full_name_inc, "%s%d", full_name, start + i);
> -					rdev = makedev(major, minor + i * increment);
> -					if (mknod(full_name_inc, mode, rdev) == -1) {
> -						bb_perror_msg("line %d: Couldnt create node %s", linenum, full_name_inc);
> -						ret = EXIT_FAILURE;
> -					}
> -					else if (chown(full_name_inc, uid, gid) == -1) {
> -						bb_perror_msg("line %d: chown failed for %s", linenum, full_name_inc);
> -						ret = EXIT_FAILURE;
> -					}
> -					if ((mode != -1) && (chmod(full_name_inc, mode) < 0)){
> -						bb_perror_msg("line %d: chmod failed for %s", linenum, full_name_inc);
> -						ret = EXIT_FAILURE;
> -					}
> -				}
> -				free(full_name_inc);
> -			} else {
> -				rdev = makedev(major, minor);
> -				if (mknod(full_name, mode, rdev) == -1) {
> -					bb_perror_msg("line %d: Couldnt create node %s", linenum, full_name);
> +			full_name_inc = xmalloc(strlen(full_name) + sizeof(int)*3 + 2);
> +			if (count)
> +				count--;
> +			for (i = start; i <= start + count; i++) {
> +				sprintf(full_name_inc, count ? "%s%u" : "%s", full_name, i);
> +				rdev = makedev(major, minor + (i - start) * increment);
> +				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, uid, gid) == -1) {
> -					bb_perror_msg("line %d: chown failed for %s", linenum, full_name);
> +				} else if (chown(full_name_inc, uid, gid) < 0) {
> +					bb_perror_msg("line %d: can't chown %s", linenum, full_name_inc);
>  					ret = EXIT_FAILURE;
> -				}
> -				if ((mode != -1) && (chmod(full_name, mode) < 0)){
> -					bb_perror_msg("line %d: chmod failed for %s", linenum, full_name);
> +				} else if (chmod(full_name_inc, mode) < 0) {
> +					bb_perror_msg("line %d: can't chmod %s", linenum, full_name_inc);
>  					ret = EXIT_FAILURE;
>  				}
>  			}
> +			free(full_name_inc);
>  		}
>  loop:
>  		free(line);
> -- 
> 2.9.3
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 2/2] makedevs: make device node creation idempotent

[Yann E. MORIN]

Arnout, All,

On 2016-11-05 14:38 +0100, Arnout Vandecappelle (Essensium/Mind) spake thusly:
> We use makedevs to create device nodes in the target rootfs. However,
> this can be called several times, e.g. when building several filesystem
> images or when rebuilding. When makedevs is called the second time, the
> device node already exists so mknod() errors out.
> 
> This wasn't noticed before because fakeroot's mknod() wrapper
> (incorrectly) does _not_ error out when the file exists already. Now
> we switched from fakeroot to pseudo, the problem becomes apparent.
> 
> Before creating the device node, check if it already exists and if so,
> if it has the correct device type and number. Change of mode and
> ownership is still done.
> 
> This approach was preferred over removing the target files before
> creating them, which would be simpler. However, when e.g. a file exists
> as a normal file and makedevs specifies it as a device node, that
> really is an error so we should detect it.
> 
> The other types don't have to be changed. The 'd' (directory) type is
> already OK because it already only creates directories if they don't
> exist yet. The 'f' (file mode) and 'r' (recursive) types only operate
> on files and directories that exist already.
> 
> Patch also sent upstream to busybox.
> 
> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> Reported-by: Fabio Estevam <festevam@gmail.com>

Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Thank you! :-)

Regards,
Yann E. MORIN.

> ---
> v2: check if pre-existing file is of correct type instead of simply
>     unlinking (Yann).
> ---
>  package/makedevs/makedevs.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/package/makedevs/makedevs.c b/package/makedevs/makedevs.c
> index 7092b14..bcf0e0f 100644
> --- a/package/makedevs/makedevs.c
> +++ b/package/makedevs/makedevs.c
> @@ -601,6 +601,7 @@ int main(int argc, char **argv)
>  			dev_t rdev;
>  			unsigned i;
>  			char *full_name_inc;
> +			struct stat st;
>  
>  			if (type == 'p') {
>  				mode |= S_IFIFO;
> @@ -622,10 +623,23 @@ int main(int argc, char **argv)
>  			for (i = start; i <= start + count; i++) {
>  				sprintf(full_name_inc, count ? "%s%u" : "%s", full_name, i);
>  				rdev = makedev(major, minor + (i - start) * increment);
> -				if (mknod(full_name_inc, mode, rdev) < 0) {
> +				if (stat(full_name_inc, &st) == 0) {
> +					if ((mode & S_IFMT) != (st.st_mode & S_IFMT)) {
> +						bb_error_msg("line %d: node %s exists but is of wrong file type", linenum, full_name_inc);
> +						ret = EXIT_FAILURE;
> +						continue;
> +					}
> +					if (st.st_rdev != rdev) {
> +						bb_error_msg("line %d: node %s exists but is wrong device number", linenum, full_name_inc);
> +						ret = EXIT_FAILURE;
> +						continue;
> +					}
> +				} else 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) {
> +					continue;
> +				}
> +				if (chown(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.9.3
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/2] makedevs: resync device creation with upstream busybox

[Thomas Petazzoni]

Hello,

On Sat, 5 Nov 2016 14:38:11 +0100, Arnout Vandecappelle
(Essensium/Mind) wrote:
> In upstream busbyox, the code to create devices has been simplified:
> the code for a single and for multiple devices is no longer duplicated.
> 
> Since we are going to change the device creation code next, it's
> convenient to have only one copy to modify.
> 
> There are two behavioural changes with this, but they were introduced
> silently together with other commits in upstream busybox.
> 
> - When mknod() fails, the chmod was still done. This is pointless so it
>   is no longer done now.
> 
> - There was a check for mode != -1; however, a mode of -1 would not
>   have worked anyway because all bits would be set for mknod(), which
>   would fail. So this check is redundant.
> 
> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> ---
>  package/makedevs/makedevs.c | 47 +++++++++++++++------------------------------
>  1 file changed, 15 insertions(+), 32 deletions(-)

Both applied to master, thanks a lot!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com