Re: [PATCH] Only load a module if the filesystem is supported (#490795, #494108).

[Chris Lumens <clumens redhat com>]

> > For filesystems that we officially support, there is no change here.  For
> > those that require a cmdline option for support, module loading has now
> > moved from within the loader to inside the __init__ methods for the formats
> > themselves.  The intention here is to avoid kernel errors in modules that
> > the user never even wants to have involved.
> 
> The commit message needs some updating...

Can do.

> > diff --git a/loader/loader.c b/loader/loader.c
> > index 98ea24c..7b3eb0c 100644
> > --- a/loader/loader.c
> > +++ b/loader/loader.c
> > @@ -1919,7 +1919,7 @@ int main(int argc, char ** argv) {
> >      if (isVioConsole())
> >          setenv("TERM", "vt100", 1);
> >  
> > -    mlLoadModuleSet("cramfs:vfat:nfs:floppy:edd:pcspkr:squashfs:ext4:ext2:iscsi_tcp:iscsi_ibft");
> > +    mlLoadModuleSet("cramfs:floppy:edd:pcspkr:squashfs:iscsi_tcp:iscsi_ibft");
> 
> cramfs and squashfs shouldn't need explicit loads.  We should then check
> some of the others and see if they're being loaded by udev also (pcspkr
> and floppy are the two that immediately spring to mind)

Okay, I might do this as round two, once this original stuff goes in.

> > @@ -2071,7 +2071,7 @@ int main(int argc, char ** argv) {
> >      stop_fw_loader(&loaderData);
> >      start_fw_loader(&loaderData);
> >  
> > -    mlLoadModuleSet("raid0:raid1:raid5:raid6:raid456:raid10:linear:fat:msdos:gfs2:reiserfs:jfs:xfs:btrfs:dm-mod:dm-zero:dm-mirror:dm-snapshot:dm-multipath:dm-round-robin:dm-crypt:cbc:sha256:lrw:xts");
> > +    mlLoadModuleSet("raid0:raid1:raid5:raid6:raid456:raid10:linear:dm-mod:dm-zero:dm-mirror:dm-snapshot:dm-multipath:dm-round-robin:dm-crypt:cbc:sha256:lrw:xts");
> 
> Mostly an aside, but maybe we should implement similar for raid
> personalities, dm stuff, etc...
> 
> If we could lose all mlLoadModuleSet() calls like this, it would be
> super-fly 

Agreed.  I'll look into that as part of a later patch as well.

> > diff --git a/storage/formats/fs.py b/storage/formats/fs.py
> > @@ -427,6 +431,23 @@ class FS(DeviceFormat):
> >          if rc >= 4:
> >              raise FSError("filesystem check failed: %s" % rc)
> >  
> > +    def loadModule(self):
> > +        """Load whatever kernel module is required to support this filesystem."""
> > +        if not self._module or self._module in kernel_filesystems:
> > +            return
> > +
> > +        try:
> > +            rc = iutil.execWithRedirect("modprobe", [self._module],
> > +                                        stdout="/dev/tty5", stderr="/dev/tty5",
> > +                                        searchPath=1)
> > +        except Exception as e:
> > +            log.error("Could not load kernel module %s: %s" % (self._module, e))
> > +            self._supported = False
> > +
> > +        if rc:
> > +            log.error("Could not load kernel module %s" % self._module)
> > +            self._supported = False
> > +
> 
> I still think we need a check against /proc/filesystems here to see if
> support already exists as built-in vs modular.

That's what the "self._module in kernel_filesystems" bit is about.

> Also, I wonder if we're
> going to have any filesystems that need multiple modules (so a list, not
> just a string)

I debated this myself too and chose a string because I didn't see any
existing cases of needing more than one module.  But that's easy enough
to fix.

> > @@ -764,6 +786,7 @@ class Ext3FS(Ext2FS):
> >      _type = "ext3"
> >      _defaultFormatOptions = ["-t", "ext3"]
> >      _migrationTarget = "ext4"
> > +    _module = "ext2"
> 
> The module here should be ext3.  But it's built-in instead of modular

Okay, I can change it to ext3 just to be absolutely correct.

- Chris