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

warning messages requested by the BIOS team #20

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sugudsl
Copy link
Collaborator

@sugudsl sugudsl commented Oct 22, 2021

No description provided.

} else {
printf("BIOS Setting changed\n");
printf("Setting will not change until reboot\n");
printf("Please check whether the setting name is Valid\n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

lower case v in Valid

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

corrected

printf("BIOS Setting changed\n");
printf("Setting will not change until reboot\n");
printf("Please check whether the setting name is Valid\n");
printf("please complete authentication if BIOS password is set\n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Capital P in please

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Corrected

printf("Setting will not change until reboot\n");
printf("Please check whether the setting name is Valid\n");
printf("please complete authentication if BIOS password is set\n");
printf("Do you want to continue (Y/N):");
Copy link
Collaborator

Choose a reason for hiding this comment

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

So I personally think the first 'warning' is pointless. If they're not using a valid name the call will fail, and they're not going to use an invalid name deliberately.
I would put the authentication warning if the set fails as a possible reason it fails. But it would be much better to probe the interface to confirm if admin password is set and to print this message if it's required. Blindly printing it everytime is just annoying.
Consider adding a flag to skip the continue check - adding this will make scripting thinklmi harder.

Copy link
Collaborator Author

@sugudsl sugudsl Mar 15, 2022

Choose a reason for hiding this comment

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

Removed the first warning.
Probing and printing the warning only if the password is set, will take it as an improvement for the next release.
Removed the Y/N check for setting the setting. Retained for the TPM type change

} else {
printf("BIOS Setting changed\n");
printf("Setting will not change until reboot\n");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your indentation in the above looks wonky - check your editor settings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My vim editor displays this correctly, however there a tab, which I removed


printf("Before switching TPM, please make sure the TPM is not in use and all TPM related applications\n");
printf("must be disabled, otherwise the TPM will be cleared and you may not be able to access your data\n");
printf("\n Do you wish to continue(Y/N):");
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above - consider adding a nocheck option to bypass the continue check for easier scripting

} else {
printf("Tpm type changed\n");
printf("Setting will not change until reboot\n");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tpm in the above perrors should be capitalised (TPM)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Corrected

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