Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

issuegen: do not use staged file, and fix bug in udev add/remove invocation #39

Merged
merged 2 commits into from
Apr 20, 2020
Merged

issuegen: do not use staged file, and fix bug in udev add/remove invocation #39

merged 2 commits into from
Apr 20, 2020

Conversation

rfairley
Copy link
Contributor

Fixes: #35

No functional changes, contains two bugfixes. Please see commit messages for additional detail.

issuegen: fix target path for udev remove
874cd51 
Noticed in passing I had not updated the target path correctly
for the remove case previously. Should refactor so that the target
path is defined in one place, but quickly fixing for this commit.
@rfairley rfairley mentioned this pull request Apr 17, 2020
Closed
@dustymabe
Copy link
Member

I don't know if we are solving the problem here. It still seems like we could have two processes writing to $generated at the same time, right? I think you'd either want to keep the old "staged" model and just use a new temporary file for each invocation OR aggregate all the content you want to write to the file in a bash variable and then write it all out in one command.

@rfairley rfairley force-pushed the rfairley-race-fix branch from c1e7734 to 92a3781 Compare April 20, 2020 16:28
@rfairley
Copy link
Contributor Author

I don't know if we are solving the problem here. It still seems like we could have two processes writing to $generated at the same time, right?

Right - the output could become corrupted still by the two processes writing to the generated file at the same time. This change just avoids having the service fail from the error missing the common staged file - but agreed it is not really solving the problem, it would be better to ensure that the output to the file is always correct.

I think you'd either want to keep the old "staged" model and just use a new temporary file for each invocation OR aggregate all the content you want to write to the file in a bash variable and then write it all out in one command.

A new temporary file for each SGTM! Will update.

@dustymabe
Copy link
Member

dustymabe commented Apr 20, 2020 •
edited
Loading

A new temporary file for each SGTM! Will update.

it would be pretty easy to just use variables and not have to use a staged file I think. Something like:

generated_string=''
generated_string+=$(cat /etc/${ISSUE_DIR_PRIVATE}/* 2>/dev/null)
generated_string+=$(cat /run/${ISSUE_DIR_PRIVATE}/* 2>/dev/null)
generated_string+=$(cat /usr/lib/${ISSUE_DIR_PRIVATE}/* 2>/dev/null)

echo "${generated_string}" > "${generated}"

@rfairley rfairley mentioned this pull request Apr 20, 2020
Closed
@rfairley rfairley force-pushed the rfairley-race-fix branch 3 times, most recently from 4c3dcfd to 475e8a6 Compare April 20, 2020 17:57
@rfairley
Copy link
Contributor Author

Updated now - works well with variables.

Also rebased #40 which follows up.

rfairley pushed a commit to rfairley/console-login-helper-messages that referenced this pull request Apr 20, 2020
motdgen: do not share a staged file
b9a816d 
For the same reason as in coreos#39
(copying the phrasing below):

With the `staged` file shared, there would be potential for two or
more processes executing `motdgen` to write to it resulting in
corrupted output, or the error in the `cat` command due to missing
file reported in coreos#35 (comment). Currently, this is not a problem with
motdgen, but could be if `motdgen` were invoked by something like the
udev rules that invoke `issuegen`.

Instead, write the intermediate output to a variable before writing
to the final output file. This ensures only valid output is written
to the issue file shown to the terminal.

Additionally, perform code tidyups similar to those done for
`issuegen` in coreos#40.
@rfairley rfairley force-pushed the rfairley-race-fix branch from 475e8a6 to 80ecdf7 Compare April 20, 2020 18:24
@rfairley
Copy link
Contributor Author

^ Re-pushed for a minor correction in the commit message.

@rfairley rfairley mentioned this pull request Apr 20, 2020
Merged
dustymabe
dustymabe approved these changes Apr 20, 2020
View reviewed changes
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. A few optional comments.

@dustymabe dustymabe mentioned this pull request Apr 20, 2020
Closed
issuegen: do not share the staged file
52d0963 
With the `staged` file shared, there was potential for two or
more processes executing `issuegen` to write to it, resulting
in corrupted output, or the error in the `cat` command due to
missing file reported in coreos#35 (comment).

Instead, write the intermediate output to a variable before writing
to the final output file. This ensures only valid output is written
to the issue file shown to the terminal.

If large issue snippets are ever passed, a tempfile is another
option for storing intermediate output to avoid storing snippets
in memory. However, practically, snippet sizes should be small, as
they are intended to be read on a terminal.
@rfairley rfairley force-pushed the rfairley-race-fix branch from 80ecdf7 to 52d0963 Compare April 20, 2020 21:40
@rfairley
Copy link
Contributor Author

Will merge now - suggested fixups were added after approval.

@rfairley rfairley merged commit 32c8fab into coreos:master Apr 20, 2020
@rfairley rfairley deleted the rfairley-race-fix branch April 20, 2020 21:51
rfairley pushed a commit to rfairley/console-login-helper-messages that referenced this pull request Apr 20, 2020
motdgen: do not share a staged file
88dab77 
For the same reason as in coreos#39
(copying the phrasing below):

With the `staged` file shared, there would be potential for two or
more processes executing `motdgen` to write to it resulting in
corrupted output, or the error in the `cat` command due to missing
file reported in coreos#35 (comment). Currently, this is not a problem with
motdgen, but could be if `motdgen` were invoked by something like the
udev rules that invoke `issuegen`.

Instead, write the intermediate output to a variable before writing
to the final output file. This ensures only valid output is written
to the issue file shown to the terminal.

Additionally, perform code tidyups similar to those done for
`issuegen` in coreos#40.
rfairley pushed a commit to rfairley/console-login-helper-messages that referenced this pull request Apr 20, 2020
motdgen: do not share a staged file
c15a140 
For the same reason as in coreos#39
(copying the phrasing below):

With the `staged` file shared, there would be potential for two or
more processes executing `motdgen` to write to it resulting in
corrupted output, or the error in the `cat` command due to missing
file reported in coreos#35 (comment). Currently, this is not a problem with
motdgen, but could be if `motdgen` were invoked by something like the
udev rules that invoke `issuegen`.

Instead, write the intermediate output to a variable before writing
to the final output file. This ensures only valid output is written
to the issue file shown to the terminal.

Additionally, perform code tidyups similar to those done for
`issuegen` in coreos#40.
rfairley pushed a commit to rfairley/console-login-helper-messages that referenced this pull request May 18, 2020
motdgen: do not share a staged file
a88708f 
For the same reason as in coreos#39
(copying the phrasing below):

With the `staged` file shared, there would be potential for two or
more processes executing `motdgen` to write to it resulting in
corrupted output, or the error in the `cat` command due to missing
file reported in coreos#35 (comment). Currently, this is not a problem with
motdgen, but could be if `motdgen` were invoked by something like the
udev rules that invoke `issuegen`.

Instead, write the intermediate output to a variable before writing
to the final output file. This ensures only valid output is written
to the issue file shown to the terminal.

Additionally, perform code tidyups similar to those done for
`issuegen` in coreos#40.
rfairley pushed a commit to rfairley/console-login-helper-messages that referenced this pull request May 22, 2020
motdgen: do not share a staged file
0a5346c 
For the same reason as in coreos#39
(copying the phrasing below):

With the `staged` file shared, there would be potential for two or
more processes executing `motdgen` to write to it resulting in
corrupted output, or the error in the `cat` command due to missing
file reported in coreos#35 (comment). Currently, this is not a problem with
motdgen, but could be if `motdgen` were invoked by something like the
udev rules that invoke `issuegen`.

Instead, write the intermediate output to a variable before writing
to the final output file. This ensures only valid output is written
to the issue file shown to the terminal.

Additionally, perform code tidyups similar to those done for
`issuegen` in coreos#40.
rfairley pushed a commit to rfairley/console-login-helper-messages that referenced this pull request Jul 1, 2020
motdgen: do not share a staged file
5df99eb 
For the same reason as in coreos#39
(copying the phrasing below):

With the `staged` file shared, there would be potential for two or
more processes executing `motdgen` to write to it resulting in
corrupted output, or the error in the `cat` command due to missing
file reported in coreos#35 (comment). Currently, this is not a problem with
motdgen, but could be if `motdgen` were invoked by something like the
udev rules that invoke `issuegen`.

Instead, write the intermediate output to a variable before writing
to the final output file. This ensures only valid output is written
to the issue file shown to the terminal.

Additionally, perform code tidyups similar to those done for
`issuegen` in coreos#40.
rfairley pushed a commit to rfairley/console-login-helper-messages that referenced this pull request Jul 2, 2020
motdgen: do not share a staged file
6afc0a4 
For the same reason as in coreos#39
(copying the phrasing below):

With the `staged` file shared, there would be potential for two or
more processes executing `motdgen` to write to it resulting in
corrupted output, or the error in the `cat` command due to missing
file reported in coreos#35 (comment). Currently, this is not a problem with
motdgen, but could be if `motdgen` were invoked by something like the
udev rules that invoke `issuegen`.

Instead, write the intermediate output to a variable before writing
to the final output file. This ensures only valid output is written
to the issue file shown to the terminal.

Additionally, perform code tidyups similar to those done for
`issuegen` in coreos#40.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dustymabe dustymabe dustymabe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

racy use of shared file
2 participants
@rfairley @dustymabe