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

[Zbigniew Jędrzejewski-Szmek]

On Fri, Nov 28, 2014 at 09:27:43PM +0100, Lennart Poettering wrote:
> 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.
This is done in a my follow up patch.

> 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. 
I fixed one place in a follow up patch, but I might have missed
some.

As for the DEFINE_TRIVIAL_CLEANUP: yeah, I looked at that, but
thought that since mnt_free_* accept NULLs the extra check added
by DEFINE_TRIVIAL_CLEANUP would be unnecessary. Looking at it again,
this kind of micro-opt is stupid. I'll remove those custom functions
in favour of 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.)

Zbyszek