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.
>