[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 Poettering]

On Mon, 09.03.15 18:39, Lennart Poettering (lennart at 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