[systemd-devel] [systemd-commits] 9 commits - configure.ac .gitignore Makefile.am Makefile-man.am man/systemd-fsckd.service.xml man/systemd-fsck at .service.xml po/de.po po/el.po po/fr.po po/hu.po po/it.po po/pl.po po/POTFILES.in po/pt_BR.po po/ru.po po/sv.po po/uk.po src/fsck src/fsckd src/shared test/mocks units/.gitignore units/systemd-fsckd.service.in units/systemd-fsckd.socket units/systemd-fsck-root.service.in units/systemd-fsck at .service.in
lennart at poettering.net
Mon Mar 9 11:11:08 PDT 2015
On Mon, 09.03.15 18:39, Lennart Poettering (lennart at poettering.net) wrote:
> 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
- 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!
Lennart Poettering, Red Hat
More information about the systemd-devel