On Mon, 09.03.15 18:39, Lennart Poettering (lenn...@poettering.net) wrote: Didier, Martin,
> I looked in more detail at the fsckd code that was commited a few > weeks ago, and I cannot say I liked what I saw. The code still has all > kinds of issues, including memory corruptions, fd handling errors, and > tons and tons of incorrect error handling (I think more times the code > passes an incorrect error to log_xyz_errno() than a correct one!). And > there are conceptional issues with the code too (for example, it's not > OK to keep /dev/console open, this breaks the SAK key logic...) > > Please, be more careful with complex code like this, this needs more > rounds of review before something like this can be merged... > > Next time, please be more careful! So I fixed a number of issues, but there are some bits I am not really keen to fix, mostly because it's hard for me to test as I don't use a file system that still requires fsck... - /dev/console needs to be opened only on demand but not continously, to keep the SAK window as small as possible. - plymouth_feedback_handler() is really broken. Is it supposed to read from a SOCK_STREAM socket? If so, are all messages exactly 6 bytes long? If not: the parser will be completely confused by multiple incoming messages which are coalesced... Also, previously it would read uninitialized data, if the bytes we read are shorter than 6... I "fixed" that now with a safety check, so that we don't process uninitialized data anymore, but this really needs to be fixed properly. - A clear regime needs to be enforced: either functions log errors on their own, or they don't. In the latter case the calling function needs to log errors instead. Currently some of the functions (like send_message_plymouth_socket()) log some errors but don't log others. And sometimes if they currently do log the functions invoking them log each error a second time! (As is the case for send_message_plymouth() which calls send_message_plymouth_socket()). - We should avoid mixing stack allocation calls and function calls. See man page of alloca() about this. Hence: invoking strjoina() around gettext calls is *not* OK. And there's probably more to fix... Anyway, please look into fixing this, I am kinda relying on patches for this, as I don't use this myself. Fedora isn't set up for it, nor do I use a file system that still requires fsck at boot... I am not sure I want to ship this like this in the current state, on the next release, the code is not ready yet, and I am very concerned that this ends up in many boots like this! Thanks, Lennart -- Lennart Poettering, Red Hat _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel