[systemd-devel] [PATCH v2 5/5] mount: auto-detect iSCSI and FCoE as requiring network

[Lennart Poettering]

On Fri, 28.11.14 20:50, Zbigniew Jędrzejewski-Szmek (zbyszek at in.waw.pl) wrote:

> On Sun, Nov 23, 2014 at 08:33:41PM -0800, Chris Leech wrote:
> > This adds auto detection for iSCSI and some FCoE drivers and treats
> > mounts to file-systems on those devices as remote-fs.
> > 
> > Signed-off-by: Chris Leech <cleech at redhat.com>
> No need for this.
> 
> I now pushed patches 1-4 with some small changes here and there.
> Since libmount is not optional, I removed if from the version string.
> 
> This patch I didn't push: this seems like something that would be better
> done through udev rules, by setting some tags. Then systemd could simply
> check for the presence of a tag on the device without having any
> special knowledge about iscsi and firends.

Honestly, I am really not sure I like this patch.

One one hand we now have a hard dep on libmount. Which I figure is
mostly OK. However, what I find really weird about this is that even
though libmount is supposed to abstract access to the "utab" away, it
doesn't sufficiently: we still hardcode the utab path now so that we
can watch it. I mean, either we use an abstracted interface or we
don't. THis really smells to me as either libmount should provide some
form of inotify iface to utab, or we should parse utab directly. If
libmount is the only and official API for utab, then we should
that. If the utab file however is API too, then we can well go ahead
and parse it directly.

(Karel, could you please comment on this?)

THe new code looks also buggy to me. For example, am I missing
something or is nobody creating /run/mount? I mean, we need to make
sure that it exists before we can install an inotify watch on it.

Then, the patch uses strcmp() and ints as bools, and things, misses
error checking on unit_add_dependency_by_name(), defines its own
desctruors instead of using DEFINE_TRIVIAL_CLEANUP. 

Also, it leaves the cescape() calls in for what we read from libmount,
which makes a lot of alarm bells ring in my head: doesn't libmount do
that on its own?

Anyway, this patch really looks like it should have gone through a
couple of more revisions before it got merged. And it raises a couple
of general questions regarding utab and libmount and what is API and
what isn't.

(Sorry that I didn't find time to review this more quickly.)

Lennart

-- 
Lennart Poettering, Red Hat