lenses/fstab.aug: Allow individual mount options to be empty

[rwmjones]

Mount allows mount-option fields to be empty (I think it ignores them), but augeas gave an error. Allow this to be parsed, preserving the empty option on write.

Example:
/dev/mapper/vg00-vartmp /var/tmp xfs rw,,nodev,nosuid,noexec,relatime 0 0

Fixes: https://issues.redhat.com/browse/RHEL-77279
Fixes: #832
Signed-off-by: Richard W.M. Jones [email protected]

[rwmjones]

I don't like the full solution here. I think commit 5246ef0 was a hack, not a good fix for that problem. This adds something like another hack on top of that commit.

I would prefer a solution where we allow option fields in the list to be empty, which would be a generalisation of both this PR & commit 5246ef0. However I could not work out how to actually express that as an Augeas lens, just getting lots of ambiguous parsing problems that I was unable to fix.

[georgehansper]

I'll have a look at this

[georgehansper]

I agree thata commit 5246ef0 fixed a single use-case but didn't address the much boarder issue of other stray commas in the mount options

The previous fix treats the trailing comma as an "optional part of the space", which does not seem correct to me either

My first instinct was just to "make everything optional", but this approach has some pitfalls

It is possible to construct a lens where everything is optional, like this

  let comma       = del "," ","
  let mntoptlabel = /[^,#= \n\t]*/
  let value       = [ label "value" . del "=" "=" .  store /[^ \t\n=,#]*/  ]
  let opt         = [ label "opt" . store mntoptlabel . value? ]
  let mntopt_list = ( opt ? . ( comma . opt? )* )

The above lens snippet would accept a wide range of option combinations, such as

	rw,,nodev
	rw,,nodev,,,
	,,,

One aspect of this snippet, is that the extra commas result in additional opt nodes in the tree
I like this, because the structure of the tree more closely resembles the structure of the original file, rather than quietly removing any extra commas

There is a problem with the above. Given an fstab entry like this

/dev/fd0  /mnt/floppy	vfat    , 0 0

This generates a tree like this

/files/etc/fstab/6/spec = "/dev/fd0"
/files/etc/fstab/6/file = "/mnt/floppy"
/files/etc/fstab/6/vfstype = "vfat"
/files/etc/fstab/6/opt[1] = ""
/files/etc/fstab/6/opt[2] = ""
/files/etc/fstab/6/dump = "0"
/files/etc/fstab/6/passno = "0"

Notice the there are 2 empty opt entries, which is consistant with the input
If we delete one of these opt entries

rm /files/etc/fstab/6/opt[2]

The resulting preview looks like this

/dev/fd0  /mnt/floppy	vfat     0 0

which is incorrect syntax for /etc/fstab

This forces us to ask the question, what actually valid for "mount options"?

I would suggest that the minimal set of mount-options should insist that the 1st option is not empty
Subsequent mount-options may be empty, so we can accomodate empty options anywhere else

That would give us an improved snippet like this

  let comma       = del "," ","
  let value       = [ label "value" . del "=" "=" . store /[^ \t\n=,#]*/ ]
  let opt1        = [ label "opt" . store /[^,#= \n\t]+/ . value? ]
  let opt         = [ label "opt" . store /[^,#= \n\t]*/ . value? ]
  let mntopt_list = ( opt1 
                    | opt ? . ( comma . opt? )+
                    )

Curiously, this still accepts a single ',' as a list of mount-options
If we delete the 2nd empty opt[2] (as above), instead of omitting the mount-options altogether, it instead in puts in the single ',' again

/dev/fd0  /mnt/floppy vfat  , 0 0

I did notice one other thing when experimenting with the above
This lens creates empty entries of the form:

/files/etc/fstab/6/opt[1] = ""
/files/etc/fstab/6/opt[2] = ""

If we replace the empty string with a NULL like this

set /files/etc/fstab/6/opt[2]

then the preview fails (ie the reverse translate fails)

We can make this completely optional by making the "store" operation optional, like this

  let value       = [ label "value" . del "=" "=" . store /[^ \t\n=,#]*/ ]
  let opt1        = [ label "opt" .   store /[^,#= \n\t]+/ . value?    ]
  let opt         = [ label "opt" . ( store /[^,#= \n\t]*/ . value? )? ]
  let mntopt_list = ( opt1
                    | opt ? . ( comma . opt? )+
                    )

This appears to act as an indicator that there does not need to be a string stored for an empty opt, as long as the opt node exists

I think this improves the usability of the lens

I would suggest that the original lens seems quite confusing, and would suggest the following alternative
As per your PR, I have noted that vfstype can be a comma-separated list, but does not require the item=value syntax which was inadertantly included in the previous versions

  let sep_tab = Sep.tab
  let sep_spc = Sep.space

  let comma   = Sep.comma
  let eol     = Util.eol

  let comment = Util.comment
  let empty   = Util.empty

  let file    = /[^# \t\n]+/
  let spec    = /[^,# \n\t][^ \n\t]*/

  (* vfstype - a list of allowed filesystem type, typically just a short string like ext4 or iso9660, but it may
               be a comma-separated list, and contain entries like fuse.sshfs
  *)
  let vfstype_list =
      let lns = [ label "vfstype" . store /[^,#= \n\t]+/ ] in
         Build.opt_list lns comma

  (* An option label can't contain comma, comment, equals, or space *)
  let opt_1_label = /[^,#= \n\t]+/
  let opt_n_label = /[^,#= \n\t]*/

  let value       = [ label "value" . del "=" "=" . ( store /[^ \t\n=,#]*/ ) ]
  let opt_1       = [ label "opt" .   store opt_1_label . value?    ]
  let opt_n       = [ label "opt" . ( store opt_n_label . value? )? ]

  let mntopt_list = ( opt_1
                    | opt_n ? . ( comma . opt_n ? )+
                    )

  let record = [ seq "mntent" .
                   Util.indent .
                   [ label "spec" . store spec ] . sep_tab .
                   [ label "file" . store file ] . sep_tab .
                   vfstype_list . ( sep_tab .
                   mntopt_list  .   ( sep_tab .
                   [ label "dump"   . store /[0-9]+/ ] . ( sep_spc .
                   [ label "passno" . store /[0-9]+/ ]   )?
                                    )?
                                  )?
                 . Util.comment_or_eol
               ]

[rwmjones]

Thanks. I'll just note that the proper way to express "no mount options" is to put defaults in that field, but that may not be possible to express in Augeas. I'll see if I can rework the whole lens as suggested.

[rwmjones]

let opt_n = [ label "opt" . ( store opt_n_label . value? )? ]

This line doesn't compile with:

/home/rjones/d/augeas/lenses/fstab.aug:28.2-.69:Failed to compile opt_n
/home/rjones/d/augeas/lenses/fstab.aug:28.36-.67:exception: illegal optional expression: /([^,#= 
        ]*)(((())(=)([^         
=,#]*))?)/ matches the empty word

I guess you cannot use (something?)? as an expression? This is pushing beyond the limits of my understanding of the Augeas parser however.

I will push an update that contains your version that I'm testing.