Re: [PATCH 2/2] Comment out the fstab lines with unknown filesystem rather then ignoring them

[David Cantrell]

On Tue, Jun 26, 2012 at 03:40:49PM +0200, Vratislav Podzimek wrote:
> On Tue, 2012-06-26 at 09:34 -0400, David Cantrell wrote:
> > Comments and questions below.
> >
> > On Tue, Jun 26, 2012 at 02:11:19PM +0200, Vratislav Podzimek wrote:
> > > When doing upgrade, there is no need to ignore the fstab lines with filesystem
> > > Anaconda does not support. We can leave the lines as they are and just comment
> > > them out to prevent mounting problems during the next boot.
> > > Resolves: rhbz#754213
> > >
> > > fsset.py |   22 ++++++++++++++++++++--
> > >  1 files changed, 20 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fsset.py b/fsset.py
> > > index 9636003..64d948d 100644
> > > --- a/fsset.py
> > > +++ b/fsset.py
> > > @@ -1297,6 +1297,20 @@ class AutoFileSystem(PsudoFileSystem):
> > >
> > > fileSystemTypeRegister(AutoFileSystem())
> > >
> > > +class unknownFileSystem(FileSystemType):
> > > def __init__(self, fstype):
> > >    FileSystemType.__init__(self)
> > >    self.partedFileSystemType = None
> > >    self.formattable = 0
> > >    self.checked = 0
> > >    self.linuxnativefs = 0
> > >    self.supported = 0
> > >    self.name = fstype
> > > def formatDevice(self, entry, progress, chroot='/'):
> > >    pass
> > >
> > > class BindFileSystem(PsudoFileSystem):
> > >      def __init__(self):
> > >          PsudoFileSystem.__init__(self, "bind")
> > > @@ -1456,6 +1470,10 @@ class FileSystemSet:
> > >                  options = entry.getOptions()
> > >                  if entry.mountpoint == "/" and options is not None:
> > >                      options = options.replace(",_netdev", ",_rnetdev")
> > >            # comment out lines with unknown filesystem
> > >            if isinstance(entry.fsystem, unknownFileSystem):
> > >                device = "#" + device
> >
> > Using isinstance() isn't my favorite, but there is clearly precedent for it
> > in rhel5-branch.
>
> Yeah, and I don't know about any simple way to replace it with anything
> else here.

It's ok for rhel5-branch.
> > >              fstab = fstab + format % (device, entry.mountpoint,
> > >                                        entry.fsystem.getName(),
> > >                                        options, entry.fsck,
> > >
> > > @@ -2969,8 +2987,8 @@ def readFstab (anaconda):
> > >              break
> > >          # "none" is valid as an fs type for bind mounts (#151458)
> > >          if fsystem is None and (string.find(fields[3], "bind") == -1):
> > >        continue
> > >        fsystem = unknownFileSystem(fstotry[0])
> >
> > Why fstotry[0] here?
>
> Because fstotry is a list here even if there is only one filesystem on
> the fstab's line (only one item in the list).

Is this going to work for lines with the fs field containing 'ext2,ext3' or
something like that?  And if it's broken, we're ending up with a comment out
with missing data.

Maybe a better approach is to let readFstab do its thing and then for lines
that would have been thrown out, stuff those lines as-is in to a list which
we then read from later and place in the newly generated /etc/fstab but with
each entry prefixed with '#' so they are commented out.

But maybe I'm misinterpreting what's happening here.