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

disk: Default to direct-io=off in losetup wrapper #17

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

ckyrouac
Copy link
Collaborator

losetup defaults to on when the arg is not present

I mistakenly tested the original wrapper changes with my patched image that included the bootc fix. Verified this works on my machine.

losetup defaults to on when the arg is not present

Signed-off-by: ckyrouac <chriskyrouac@gmail.com>
@ckyrouac ckyrouac requested a review from cgwalters April 29, 2024 12:44
@cgwalters cgwalters merged commit 873d784 into containers:main Apr 29, 2024
2 checks passed
@cgwalters
Copy link
Contributor

losetup defaults to on when the arg is not present

Wait sorry...are you sure? We went over this in containers/bootc#375 (comment)

@ckyrouac
Copy link
Collaborator Author

that's the behavior I see on my machine, and this is in the man page:

       --direct-io[=on|off]
           Enable or disable direct I/O for the backing file. The optional argument can be either on or off. If the optional argument is omitted, it defaults to on.
❯ losetup --version
losetup from util-linux 2.39.4

@ckyrouac
Copy link
Collaborator Author

also I meant to say "direct-io" defaults to on

@ckyrouac
Copy link
Collaborator Author

https://github.com/util-linux/util-linux/blob/926b6077333554924756ba648c9df38c803c48bc/sys-utils/losetup.c#L818

not familiar with getopt_long but maybe this clause is always executed which is why it defaults to on?

@cgwalters
Copy link
Contributor

It's confusingly phrased...I took a stab at clarifying the upstream docs util-linux/util-linux#3002

not familiar with getopt_long but maybe this clause is always executed which is why it defaults to on?

That bit should only be executed if the argument was provided.

@ckyrouac
Copy link
Collaborator Author

That bit should only be executed if the argument was provided.
makes sense

ah, there was a separate issue preventing the partitions being created when I tried losetup without the direct-io argument earlier. Just did another test and it defaults to off for me when the arg is omitted. There's also a bug in the in-line bash script that threw me off, it passes the original arguments instead of the modified args. I guess adding --direct-io=off at the end of the command overrides the previous arg that set it to on. I'll just leave it as-is since it's working.

@cgwalters
Copy link
Contributor

There's also a bug in the in-line bash script that threw me off, it passes the original arguments instead of the modified args.

haha oh man, yes

@germag
Copy link
Collaborator

germag commented Apr 30, 2024

I guess adding --direct-io=off at the end of the command overrides the previous arg that set it to on. I'll just leave it as-is since it's working.

Yes, it's an unintended behavior of getopt, but now it's a "de facto" standard

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.

None yet

3 participants