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

Add no-create-imex-channels command line option #286

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

elezar
Copy link
Member

@elezar elezar commented Sep 30, 2024

This change adds a no-create-imex-channels command line option to opt-out of the creation of imex channel device nodes.

Note that the creation of device nodes is only triggered if --load-kmods is specified.

@elezar elezar requested a review from klueska September 30, 2024 13:37
@elezar elezar self-assigned this Sep 30, 2024
@elezar elezar force-pushed the add-no-create-imex-channels-option branch 2 times, most recently from da38879 to 37ff12e Compare September 30, 2024 13:47
Copy link
Collaborator

@ArangoGutierrez ArangoGutierrez left a comment

Choose a reason for hiding this comment

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

LGTM

src/cli/main.c Outdated
@@ -32,6 +32,7 @@ static struct argp usage = {
{"user", 'u', "UID[:GID]", OPTION_ARG_OPTIONAL, "User and group to use for privilege separation", -1},
{"root", 'r', "PATH", 0, "Path to the driver root directory", -1},
{"ldcache", 'l', "FILE", 0, "Path to the system's DSO cache", -1},
{"no-create-imex-channels", 0x80, NULL, 0, "Skip the creation of IMEX channel device nodes", -1},
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind using enable and disable language? Maybe "Disable creating IMEX channel device nodes" or we need "Disable automatically creating IMEX channel device nodes"?

Choose a reason for hiding this comment

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

Concerning the name of the flag, is there precedent for naming the option as no-*? What about disable-imex-channel-creation?

Copy link
Member Author

Choose a reason for hiding this comment

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

The no-* prefix is used for other negative options across this project:

        {"no-glvnd", OPT_NO_GLVND},
        {"no-uvm", OPT_NO_UVM},
        {"no-modeset", OPT_NO_MODESET},
        {"no-mps", OPT_NO_MPS},

and

                {"no-cgroups", 0x84, NULL, 0, "Don't use cgroup enforcement", -1},
                {"no-devbind", 0x85, NULL, 0, "Don't bind mount devices", -1},

In addition to more recently added options. I'm happy to update, but was trying to be locally consistent.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for Chris' comment--if we can avoid "no" prefixes for new projects that'll be a win. I want to think that I noticed the precedent and refrained from commenting.

Thanks Evan for doing more digging. While I prefer "enable" and "disable", based on the precedent of "Don't ...", I'd like to revise my comment to a request for "Don't automatically create IMEX channel device nodes".

Choose a reason for hiding this comment

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

Thanks for the context. I am okay to keep the naming as-is considering the large precedent.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have updated the usage string. Is there a more resonable no--prefixed string that we could use here? Otherwise I will stick with what we currently have.

@mikemckiernan I definitely agree on avoiding this in newer projects!

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

This change adds a no-create-imex-channels command line option to opt-out
of the creation of imex channel device nodes.

Note that the creation of device nodes is only triggered if --load-kmods
is specified.

Signed-off-by: Evan Lezar <[email protected]>
@elezar elezar force-pushed the add-no-create-imex-channels-option branch from 37ff12e to 1f2ad79 Compare October 14, 2024 22:05
@elezar elezar merged commit 63d366e into NVIDIA:main Oct 17, 2024
3 checks passed
@elezar elezar deleted the add-no-create-imex-channels-option branch October 17, 2024 15:59
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.

5 participants