[v2] makedevs: Do not return error if the fifo exisits

Submitted by Saul Wold on Oct. 1, 2013, 7:15 p.m. | Patch ID: 59169

Details

Message ID 1380654945-17787-1-git-send-email-sgw@linux.intel.com
State Accepted
Commit 3a4b0e7973bef43f16058137e64600e2f890b117
Headers show

Commit Message

Saul Wold Oct. 1, 2013, 7:15 p.m. 
This ensures that makedevs will not cause image creation failures
when it encounters a pipe (fifo) that exists from a previous image.
This handles mode changes and it will correctly fail for dangling
symlinks.

[YOCTO #5288]

Signed-off-by: Saul Wold <sgw@linux.intel.com>

makedev

Signed-off-by: Saul Wold <sgw@linux.intel.com>

makedev

Signed-off-by: Saul Wold <sgw@linux.intel.com>
---
 .../makedevs/makedevs-1.0.0/makedevs.c                 |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox 

diff --git a/meta/recipes-devtools/makedevs/makedevs-1.0.0/makedevs.c b/meta/recipes-devtools/makedevs/makedevs-1.0.0/makedevs.c
index 5d2c45b..53700c6 100644
--- a/meta/recipes-devtools/makedevs/makedevs-1.0.0/makedevs.c
+++ b/meta/recipes-devtools/makedevs/makedevs-1.0.0/makedevs.c
@@ -274,8 +274,20 @@  static void add_new_file(char *name, char *path, unsigned long uid,
 static void add_new_fifo(char *name, char *path, unsigned long uid,
 				  unsigned long gid, unsigned long mode)
 {
-	if (mknod(path, mode, 0))
-		error_msg_and_die("%s: file can not be created with mknod!", path);
+	int status;
+	struct stat sb;
+
+	memset(&sb, 0, sizeof(struct stat));
+	status = stat(path, &sb);
+
+
+	/* Update the mode if we exist and are a fifo already */
+	if (status >= 0 && S_ISFIFO(sb.st_mode)) {
+		chmod(path, mode);
+	} else {
+		if (mknod(path, mode, 0))
+			error_msg_and_die("%s: file can not be created with mknod!", path);
+	}
 	chown(path, uid, gid);
 //	printf("File: %s %s  UID: %ld  GID: %ld  MODE: %04lo\n",
 //			path, name, gid, uid, mode);

Comments

Phil Blundell Oct. 1, 2013, 7:49 p.m. 
On Tue, 2013-10-01 at 12:15 -0700, Saul Wold wrote:
> +	int status;
> +	struct stat sb;
> +
> +	memset(&sb, 0, sizeof(struct stat));
> +	status = stat(path, &sb);

Don't you want lstat() there?  Also, I think *stat() is guaranteed to
fill in all of sb if it returns anything other than an error, so the
memset() may be redundant.

I sort of wonder whether just unlink()ing the destination prior to
calling mknod would be a simpler and more robust way of fixing this
problem.

Also, on a tangential note, you seemed to have rather a surfeit of
signed-off-by lines in your email. 

p.
Saul Wold Oct. 1, 2013, 7:58 p.m. 
On 10/01/2013 12:49 PM, Phil Blundell wrote:
> On Tue, 2013-10-01 at 12:15 -0700, Saul Wold wrote:
>> +	int status;
>> +	struct stat sb;
>> +
>> +	memset(&sb, 0, sizeof(struct stat));
>> +	status = stat(path, &sb);
>
> Don't you want lstat() there?  Also, I think *stat() is guaranteed to
> fill in all of sb if it returns anything other than an error, so the
> memset() may be redundant.
>
I was keeping the same code style from the file function in the same code.

I chose to use stat() to maintain the same failure and error handling we 
have currently.

> I sort of wonder whether just unlink()ing the destination prior to
> calling mknod would be a simpler and more robust way of fixing this
> problem.
>
I was attempting to get a point fix for the release, we can worry about 
a more robust handling of the error / upgrade case in 1.5.1 or 1.6.

> Also, on a tangential note, you seemed to have rather a surfeit of
> signed-off-by lines in your email.
>
Fixed on the branch!

Sau!

> p.
>
>
>
>
Phil Blundell Oct. 1, 2013, 8:07 p.m. 
On Tue, 2013-10-01 at 12:58 -0700, Saul Wold wrote:
> On 10/01/2013 12:49 PM, Phil Blundell wrote:
> > On Tue, 2013-10-01 at 12:15 -0700, Saul Wold wrote:
> >> +	int status;
> >> +	struct stat sb;
> >> +
> >> +	memset(&sb, 0, sizeof(struct stat));
> >> +	status = stat(path, &sb);
> >
> > Don't you want lstat() there?  Also, I think *stat() is guaranteed to
> > fill in all of sb if it returns anything other than an error, so the
> > memset() may be redundant.
> >
> I was keeping the same code style from the file function in the same code.
> 
> I chose to use stat() to maintain the same failure and error handling we 
> have currently.

I'm not quite sure I understand the last sentence you wrote above.  Can
you expand on why stat() rather than lstat() is the right thing?

>I was attempting to get a point fix for the release, we can worry
>about a more robust handling of the error / upgrade case in 1.5.1 
>or 1.6.

Righto.  I am blissfully ignorant of the criteria for the release so I
am happy to defer to your judgement on that.

p.
Saul Wold Oct. 1, 2013, 8:34 p.m. 
On 10/01/2013 01:07 PM, Phil Blundell wrote:
> On Tue, 2013-10-01 at 12:58 -0700, Saul Wold wrote:
>> On 10/01/2013 12:49 PM, Phil Blundell wrote:
>>> On Tue, 2013-10-01 at 12:15 -0700, Saul Wold wrote:
>>>> +	int status;
>>>> +	struct stat sb;
>>>> +
>>>> +	memset(&sb, 0, sizeof(struct stat));
>>>> +	status = stat(path, &sb);
>>>
>>> Don't you want lstat() there?  Also, I think *stat() is guaranteed to
>>> fill in all of sb if it returns anything other than an error, so the
>>> memset() may be redundant.
>>>
>> I was keeping the same code style from the file function in the same code.
>>
>> I chose to use stat() to maintain the same failure and error handling we
>> have currently.
>
> I'm not quite sure I understand the last sentence you wrote above.  Can
> you expand on why stat() rather than lstat() is the right thing?
>
To be able to handle the dangling symbolic link that Khem mentioned, or 
more like not handle it, let it fall through and fail.

Not that this case is going to happen very often since our 1 pipe fifo 
is the same and not likely going to be a link.

Sau!

>> I was attempting to get a point fix for the release, we can worry
>> about a more robust handling of the error / upgrade case in 1.5.1
>> or 1.6.
>
> Righto.  I am blissfully ignorant of the criteria for the release so I
> am happy to defer to your judgement on that.
>
> p.
>
>
>
>