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 commands for configuring systemd console-mode #5

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

Conversation

silkeh
Copy link
Member

@silkeh silkeh commented Jun 23, 2024

Add set-console-mode and get-console-mode commands for managing the console-mode in systemd.

Resolves #2.

@ikeycode
Copy link
Member

For any newly landing commands/modes please also open a linked issue over in blsforme so I can keep track and ensure compat is maintained, ty :)

return _write_sysconf_file(self, "console_mode", NULL);
}

return _write_sysconf_file(self, "console_mode", mode);
Copy link
Member

Choose a reason for hiding this comment

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

The NULL check seems a bit pointless here as you still pass NULL. Perhaps it's better to ensure all the self vtable functions do an assert against self being valid per the older APIS?

LOG_ERROR("Failed to parse config file, defaulting to no timeout");
return -1;
}

return t_val;
}

char *boot_manager_get_console_mode(BootManager *self)
{
char *value = (char*)malloc(64);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I like this use of magic buffer lengths and mallocs. Also note this isn't a zero initialised (calloc) call, so any failures will result in a garbage value floating around, i.e. not NULL. Would it not be better to simply read a single line from a file (getline on an fp), free the internal getline pointer due to alloca crap in some implementations, then strdup the return. The console-mode check could just directly wrap it, whereas the timeout mode would simply pull it in as an autofree() and convert to a number (atoi)

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'm not sure I like this use of magic buffer lengths and mallocs.

I definitely don't. I changed it as suggested.

{
char list[][5] = {"", "0", "1", "2", "auto", "max", "keep"};

for(size_t i = 0; i < sizeof list; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why there isn't a space after for - is this why the clang-format file was changed? Looks inconsistent :D

Copy link
Member Author

Choose a reason for hiding this comment

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

is this why the clang-format file was changed?

No, it was changed because my editor didn't pick it up (as it was invalid). I probably wrote this line before fixing the file 😞.

I ran a clang-format on all the changed bits, so those should be OK now.

return false;
}

bool cbm_command_set_console_mode(int argc, char **argv)
Copy link
Member

Choose a reason for hiding this comment

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

For the benefit of looking back at this project for blsforme work could we return to using doxygen style comments for all non static APIs?

* published by the Free Software Foundation; either version 2.1
* of the License, or (at your option) any later version.
*/

Copy link
Member

Choose a reason for hiding this comment

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

TBH the Solus fork needs to add a copyright section to impacted files since The Events, post 2018. Otherwise only Intel is benefiting from protection of their (and my old) copyrights, rather than Solus for current works.

Copy link
Member

@ikeycode ikeycode left a comment

Choose a reason for hiding this comment

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

minor clean up changes requested :)

@silkeh silkeh force-pushed the console-mode-support branch 2 times, most recently from 4a33b97 to 20eddb7 Compare June 26, 2024 13:12
@silkeh silkeh force-pushed the console-mode-support branch from 20eddb7 to 24e6a66 Compare June 26, 2024 13:16
@silkeh
Copy link
Member Author

silkeh commented Jun 26, 2024

@ikeycode: Thanks for the review. I think I addressed all changes. Sorry about the messy force-pushes, actual diff can be seen here.

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.

RFE: Add support for setting console-mode in systemd-boot
2 participants