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.

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

driver: remove mktemp tool dependency in ShellDriver #1481

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

flxzt
Copy link
Contributor

@flxzt flxzt commented Aug 20, 2024

Description

This removes the use of the mktemp tool on targets when transferring files through the ShellDriver.
It is replaced by touch /tmp/tmp-<random-uuid> which should be always available since touch is part of the GNU core utilities.

Checklist

  • PR has been tested

@jluebbe
Copy link
Member

jluebbe commented Aug 20, 2024

This is not equivalent to the old behavior, as touch may create files readable by other users. Also, touch doesn't fail if the file already exists.

At least, this change needs a good reasoning why this changes is an improvement and doesn't break existing usecases.

@flxzt
Copy link
Contributor Author

flxzt commented Aug 20, 2024

I can only speak for my use-case but it is the following: I came across a target that has only the most basic tools available, not much more than coreutils and does not have the ability to easily add additional tools when testing the production image.
I had hoped this dependency removal is something which benefits other users with similar targets as well.
I can't see how it breaks existing use-cases since the created tempfile really is only used for a short while until it's contents are dd'ed to the actual destination.

I didn't know that file permissions are a concern here, let me add a chmod 600 which should replicate the mktemp's default behaviour (unless you have a different suggestion).

Regarding not failing when a file with the same name already exists - if really wanted a check can be added before calling touch, but the probability of collision of a UUID4 which the to-be-created file contains in it's path according to wikipedia : number of random version-4 UUIDs which need to be generated in order to have a 50% probability of at least one collision is 2.71 quintillion source.

raise ExecutionError('Could not make temporary file on target')
tmpfile = tmpfile[0]
tmpfile = f'/tmp/tmp-{uuid4()}'
self._run_check(f'touch {tmpfile}')
Copy link
Contributor

Choose a reason for hiding this comment

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

How about only doing this as a workaround when mktemp fails? Then we can keep using the normal approach for most cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would work for me - as a fallback. What are your thoughts about it @jluebbe ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants