On Fri, 28.11.14 20:50, Zbigniew Jędrzejewski-Szmek (zbys...@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 <cle...@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 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel