[LEDE-DEV] [PATCH mountd] filesystem mount options in uci config

[John Crispin]

On 13/06/2016 09:47, Olivier Hardouin wrote:
> Hi John,
> 
> Thanks for your comment.
> You're right, I need to add some check before getting uci options.
> 
> Regarding the fs name, I propose to use per default the same driver name
> as defined in fs_names (and not have the fstype parameter in uci).
> I would still keep the fstype in uci as optional parameter in case an
> alternative driver has to be used (e.g. paragon for ntfs).
> 

Hi,

> What do you think about it?

make it optional in that case please. i would prefer to not list ext4
int he ext4 section for it to work. if the fs is not explicitly defined
use the sane default

	John


> 
> Olivier
> 
> 
> On Mon, Jun 13, 2016 at 7:03 AM, John Crispin <john at phrozen.org
> <mailto:john at phrozen.org>> wrote:
> 
>     Hi,
> 
>     comment inline.
> 
>     On 10/06/2016 11:18, olivier.hardouin at gmail.com
>     <mailto:olivier.hardouin at gmail.com> wrote:
>     > Move (previously hardcoded) mount option to UCI to allow different
>     configuration
>     > like charset (utf-8 or iso) and filesystem driver (if alternative
>     ones are used).
>     > The fs names are changed in lowercase to comply with UCI general
>     naming.
>     >
>     > Signed-off-by: Olivier Hardouin <olivier.hardouin at gmail.com
>     <mailto:olivier.hardouin at gmail.com>>
>     > ---
>     >  mount.c | 69
>     ++++++++++++++++++++++++++---------------------------------------
>     >  1 file changed, 28 insertions(+), 41 deletions(-)
>     >
>     > diff --git a/mount.c b/mount.c
>     > index 8892040..c8f7ea6 100644
>     > --- a/mount.c
>     > +++ b/mount.c
>     > @@ -51,15 +51,15 @@ struct mount {
>     >  char *fs_names[] = {
>     >       "",
>     >       "",
>     > -     "MBR",
>     > -     "EXT2",
>     > -     "EXT3",
>     > -     "FAT",
>     > -     "HFSPLUS",
>     > +     "mbr",
>     > +     "ext2",
>     > +     "ext3",
>     > +     "fat",
>     > +     "hfsplus",
>     >       "",
>     > -     "NTFS",
>     > +     "ntfs",
>     >       "",
>     > -     "EXT4"
>     > +     "ext4"
>     >  };
>     >
>     >  #define MAX_MOUNTED          32
>     > @@ -227,42 +227,29 @@ int mount_new(char *path, char *dev)
>     >       pid = autofs_safe_fork();
>     >       if(!pid)
>     >       {
>     > -             if(mount->fs == EXFAT)
>     > +             if(mount->fs > MBR && mount->fs <= EXT4)
>     >               {
>     > -                     log_printf("mount -t exfat -o
>     rw,uid=1000,gid=1000 /dev/%s %s", mount->dev, tmp);
>     > -                     ret = system_printf("mount -t exfat -o
>     rw,uid=1000,gid=1000 /dev/%s %s", mount->dev, tmp);
>     > -             }
>     > -             if(mount->fs == FAT)
>     > -             {
>     > -                     log_printf("mount -t vfat -o
>     rw,uid=1000,gid=1000 /dev/%s %s", mount->dev, tmp);
>     > -                     ret = system_printf("mount -t vfat -o
>     rw,uid=1000,gid=1000 /dev/%s %s", mount->dev, tmp);
>     > -             }
>     > -             if(mount->fs == EXT4)
>     > -             {
>     > -                     log_printf("mount -t ext4 -o rw,defaults
>     /dev/%s %s", mount->dev, tmp);
>     > -                     ret = system_printf("mount -t ext4 -o
>     rw,defaults /dev/%s %s", mount->dev, tmp);
>     > -             }
>     > -             if(mount->fs == EXT3)
>     > -             {
>     > -                     log_printf("mount -t ext3 -o rw,defaults
>     /dev/%s %s", mount->dev, tmp);
>     > -                     ret = system_printf("mount -t ext3 -o
>     rw,defaults /dev/%s %s", mount->dev, tmp);
>     > -             }
>     > -             if(mount->fs == EXT2)
>     > -             {
>     > -                     log_printf("mount -t ext2 -o rw,defaults
>     /dev/%s %s", mount->dev, tmp);
>     > -                     ret = system_printf("mount -t ext2 -o
>     rw,defaults /dev/%s %s", mount->dev, tmp);
>     > -             }
>     > -             if(mount->fs == HFSPLUS)
>     > -             {
>     > -                     log_printf("mount -t hfsplus -o
>     rw,defaults,uid=1000,gid=1000 /dev/%s %s", mount->dev, tmp);
>     > -                     ret = system_printf("mount -t hfsplus -o
>     rw,defaults,uid=1000,gid=1000 /dev/%s %s", mount->dev, tmp);
>     > -             }
>     > -             if(mount->fs == NTFS)
>     > -             {
>     > -                     log_printf("ntfs-3g /dev/%s %s -o force",
>     mount->dev, tmp);
>     > -                     ret = system_printf("ntfs-3g /dev/%s %s -o
>     force", mount->dev, tmp);
>     > +                     struct uci_context *ctx;
>     > +                     char *options, *fstype;
>     > +                     ctx = ucix_init("mountd");
>     > +                     options = ucix_get_option(ctx, "mountd",
>     fs_names[mount->fs], "options");
>     > +                     fstype = ucix_get_option(ctx, "mountd",
>     fs_names[mount->fs], "fstype");
> 
>     this might be NULL but is used below without checking for NULL. i am not
>     sure these even needs to be loaded from uci as it is the same as
>     fs_names[mount->fs].
> 
>     could you look into this and then resend the patch ?
> 
>             John
> 
>     > +                     ucix_cleanup(ctx);
>     > +                     if(!fstype)
>     > +                     {
>     > +                             log_printf("mounting /dev/%s failed,
>     expecting 'fstype' uci parameter (kernel driver) for %s",
>     mount->dev, fs_names[mount->fs]);
>     > +                     } else {
>     > +                             if(!options)
>     > +                             {
>     > +                                     log_printf("mount -t %s
>     /dev/%s %s", fstype, mount->dev, tmp);
>     > +                                     ret = system_printf("mount
>     -t %s /dev/%s %s", fstype, mount->dev, tmp);
>     > +                             } else {
>     > +                                     log_printf("mount -t %s -o
>     %s /dev/%s %s", fstype, options, mount->dev, tmp);
>     > +                                     ret = system_printf("mount
>     -t %s -o %s /dev/%s %s", fstype, options, mount->dev, tmp);
>     > +                             }
>     > +                     }
>     > +                     exit(WEXITSTATUS(ret));
>     >               }
>     > -             exit(WEXITSTATUS(ret));
>     >       }
>     >       pid = waitpid(pid, &ret, 0);
>     >       ret = WEXITSTATUS(ret);
>     >
> 
>