On Fri, Nov 28, 2014 at 09:27:43PM +0100, Lennart Poettering wrote: > 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. 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 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
