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 {flag?} MPI function #718

Merged
merged 3 commits into from
Jan 15, 2024
Merged

Add {flag?} MPI function #718

merged 3 commits into from
Jan 15, 2024

Conversation

wyld-sw
Copy link
Member

@wyld-sw wyld-sw commented Jan 12, 2024

This adds an MPI counterpart to the FLAG? MUF primitive. The check logic has been extracted into its own function.

Functionality

Both MPI and MUF versions check a single flag, which can be a prefix of a flag alias (regardless of object type). The check can also include any number of "!" characters on the left-hand side of the string for alternating negation.

This may be the first object-facing MPI function I've contributed, so let me know if I missed something there.

Consistency

This check now recognizes BOUND, HIDE, SETUID, and XPRESS as flag aliases. SET also was changed to recognize them.

Minor change

The code for FLAG? currently:

  • allows "" as the flag to check, and eventually returns 0.
  • allows "U" as the flag to check, and eventually returns 0.
  • does not allow "!" as the flag to check, and aborts with "Unknown flag"

This seems inconsistent, as the first two are similar conditions to the third. I've changed the third case to return 0 (with a bonus of returning 0 more quickly in the first case). I can see a corner case where one would want to abort when checking for "!", but it doesn't seem that compelling. Feel free to disagree, and I can change it to pass the error condition back to {flag?} and FLAG?.

Copy link
Collaborator

@tanabi tanabi left a comment

Choose a reason for hiding this comment

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

One necessary change, one suggestion/comment. Looks really good! :)

src/p_db.c Outdated
@@ -1071,7 +1071,8 @@ prim_set(PRIM_PROTOTYPE)
|| string_prefix("autostart", flag)
|| string_prefix("abate", flag))
tmp = ABODE;
else if (string_prefix("builder", flag))
else if (string_prefix("builder", flag)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we should note in both this code and in has_flag that any change needs to be made in both places?

Ideally, we'd unify these 2 case statements in some way since it looks like this has a lot of overlap with has_flag's code. However, there's also such a thing as optimizing yourself until you're weary, so if you rather put a TODO and a note that both places need to be changed, I wouldn't judge you :)

If you wanted to go the extra mile, though, I'd say make a str_to_flag call that has this case statement, then both has_flag and prim_set can use it (along with anywhere else this mess lives).

Copy link
Member Author

@wyld-sw wyld-sw Jan 13, 2024

Choose a reason for hiding this comment

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

str_to_flag is definitely coming - just wanted to see if I could be clever with ALL the flag checking. I can certainly add str_to_flag to this PR in the meantime so it definitely doesn't wait for a unified process.

It may have been added when I got to fully reviewing p_db.c :)

Copy link
Member Author

@wyld-sw wyld-sw Jan 13, 2024

Choose a reason for hiding this comment

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

I think both changes are made. Please review; I think everything's in order...

I'm not too thrilled about the duplicate "truewizard" check, but it seemed like the best way (right now?) to preserve the different meanings for SET and FLAG?.

do_set is on notice as needing a change, too, but it uses both the str_to_flag logic and the restricted function (while introducing its own restrictions) and will take more thought.

@tanabi tanabi merged commit 402d60d into fuzzball-muck:master Jan 15, 2024
0 of 16 checks passed
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.

2 participants