Code review comment for lp:~raharper/curtin/trunk.lp1709284-mount-options

On Fri, Aug 18, 2017 at 3:21 PM, Scott Moser <email address hidden> wrote:

> generally fine with this, but i would like some warnings on this section.
>

Something generic along mount options might be incompatible between OS
releases
or more specific about certain ones?

> its basically impossible to do this without somewhat deep knowledge of
> your target OS.
>

Indeed.

>
> Also, I suppose that curtin probably *should* mount the filesystem with
> the options that are provided when it mounts the filesystem. Other wise,
> it would mount the filesystem during install with one set of options and
> then the boot would mount with a different set. That could potentially be
> hazardous.
>

That's a really good point.

>
>
> Diff comments:
>
> > === modified file 'curtin/commands/block_meta.py'
> > --- curtin/commands/block_meta.py 2017-05-26 14:07:29 +0000
> > +++ curtin/commands/block_meta.py 2017-08-15 19:45:43 +0000
> > @@ -656,20 +674,18 @@
> >
> > # Add volume to fstab
> > if state['fstab']:
> > - with open(state['fstab'], "a") as fp:
> > - location = get_path_to_storage_volume(volume.get('id'),
> > - storage_config)
> > - uuid = block.get_volume_uuid(volume_path)
> > - if len(uuid) > 0:
> > - location = "UUID=%s" % uuid
> > -
> > - if filesystem.get('fstype') in ["fat", "fat12", "fat16",
> "fat32",
> > - "fat64"]:
> > - fstype = "vfat"
> > - else:
> > - fstype = filesystem.get('fstype')
> > - fp.write("%s %s %s %s 0 0\n" % (location, path, fstype,
> > - ",".join(options)))
> > + uuid = block.get_volume_uuid(volume_path)
> > + location = ("UUID=%s" % uuid) if uuid else (
> > + get_path_to_storage_volume(volume.get('id'),
> > + storage_config))
> > +
> > + fstype = filesystem.get('fstype')
> > + if fstype in ["fat", "fat12", "fat16", "fat32", "fat64"]:
> > + fstype = "vfat"
> > +
> > + fstab_entry = "%s %s %s %s 0 0\n" % (location, path, fstype,
>
> seems better as ' '.join([location, path, fstype, ",".join(options), "0",
> "0")
>
> then you could also '\t'.join()
>
> > + ",".join(options))
> > + util.write_file(state['fstab'], fstab_entry, omode='a')
> > else:
> > LOG.info("fstab not in environment, so not writing")
> >
> >
> > === modified file 'doc/topics/storage.rst'
> > --- doc/topics/storage.rst 2017-04-21 17:36:27 +0000
> > +++ doc/topics/storage.rst 2017-08-15 19:45:43 +0000
> > @@ -358,6 +358,10 @@
> > config. The target device must already contain a valid filesystem and be
> > accessible.
> >
> > +**options**: *<mount(8) comma-separated options string>*
> > +
> > +The ``options`` key will replace the default options value of
> ``defaults``.
> > +
>
> lets mention here that options are often times OS or OS version specific,
> and behavior of non-implemented options is not well defined. simply
> putting 'options' into a entry in /etc/fstab does not guarantee that the
> filesystem is mounted with those options.
>
> > .. note::
> >
> > If the specified device refers to an iSCSI device, the corresponding
>
>
> --
> https://code.launchpad.net/~raharper/curtin/trunk.
> lp1709284-mount-options/+merge/328814
> You are the owner of lp:~raharper/curtin/trunk.lp1709284-mount-options.
>