Re: [PATCH] mount_tmpfs -P option for populating after mounting (simplified)
ping.
On Sun, 13 Sep 2015 14:50:19 +0100
bytevolc...@safe-mail.net wrote:
> Hello,
>
> Although a patch like this was submitted before, this one is more
> "focused". It may be useful for those moving from MFS who use this
> option to pre-populate an MFS mount with a collection of files. This
> will refuse to work if the path specified for -P is not a directory.
> If the copy is unsuccessful, unmount and inform the user.
>
> A few questions on this though:
>
> 1. Is there a better way than executing pax, to copy all the files in
> a directory?
>
> 2. The do_exec() function was sort of copied from sbin/newfs/newfs.c.
> It doesn't feel like the best way of handling this, but some research
> suggests it is better than calling system() due to security issues,
> one that springs to mind is a malformed input directory to the -P
> option. Is there a simple way to sanitise the input for use with
> system()?
>
> 3. I noticed a pattern of functions that have about 2-3 lines of code
> in them. Is it best to keep things that way, or is it best to merge
> them into a single function (eg. copy_dir() merged into the
> mount_tmpfs() function)?
>
> 4. Are variable declarations to be at the beginning of a function
> block, or the beginning of the immediate block? I feel limiting scope
> is a good idea, but would like some feedback on this as I cannot see
> anything in style(9).
>
> 5. Is mount_tmpfs.h really necessary, given nothing in the tree seems
> to use anything related to the mount_tmpfs() function which is
> internal to the mount_tmpfs binary?
>
>
> -
>
> Index: sbin/mount_tmpfs/mount_tmpfs.8
> ===
> RCS file: /cvs/src/sbin/mount_tmpfs/mount_tmpfs.8,v
> retrieving revision 1.4
> diff -u -p -r1.4 mount_tmpfs.8
> --- sbin/mount_tmpfs/mount_tmpfs.816 Nov 2014 02:22:10
> - 1.4 +++ sbin/mount_tmpfs/mount_tmpfs.8 13 Sep
> 2015 13:03:34 - @@ -41,6 +41,7 @@
> .Op Fl m Ar mode
> .Op Fl n Ar nodes
> .Op Fl o Ar options
> +.Op Fl P Ar directory
> .Op Fl s Ar size
> .Op Fl u Ar user
> .Ar tmpfs
> @@ -80,6 +81,8 @@ flag followed by a comma-separated strin
> See the
> .Xr mount 8
> man page for possible options and their meanings.
> +.It Fl P Ar directory
> +Populate the created tmpfs file system with the contents of the
> directory. .It Fl s Ar size
> Specifies the total file system size in bytes.
> If zero is given (the default), the available amount of memory
> (including @@ -136,6 +139,10 @@ and
> .Ox 5.5 .
> .Sh CAVEATS
> The update of mount options (through mount -u) is currently not
> supported. +The
> +.Fl P
> +option will produce an error if the mount is read-only, or if the
> files +cannot be copied from the specified template directory.
> .Sh BUGS
> File system meta-data is not pageable.
> If there is not enough main memory to hold this information, the
> system may Index: sbin/mount_tmpfs/mount_tmpfs.c
> ===
> RCS file: /cvs/src/sbin/mount_tmpfs/mount_tmpfs.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 mount_tmpfs.c
> --- sbin/mount_tmpfs/mount_tmpfs.c16 Jan 2015 06:39:59
> - 1.5 +++ sbin/mount_tmpfs/mount_tmpfs.c 13 Sep
> 2015 13:03:34 - @@ -34,6 +34,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -41,6 +42,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -68,12 +70,15 @@ static inta_num(const char *, const cha
> static mode_ta_mask(const char *);
> static void pathadj(const char *, char *);
>
> +static int do_exec(const char *, const char *, char *const[]);
> +static int copy_dir(char *, char *);
> +
> /*
> -
> */ void
> mount_tmpfs_parseargs(int argc, char *argv[],
> struct tmpfs_args *args, int *mntflags,
> - char *canon_dev, char *canon_dir)
> + char *canon_dev, char *canon_dir, char *pop_dir)
> {
> int gidset, modeset, uidset; /* Ought to be 'bool'. */
> int ch;
> @@ -95,7 +100,7 @@ mount_tmpfs_parseargs(int argc, char *ar
> modeset = 0; mode = 0;
>
> optind = optreset = 1;
> - while ((ch = getopt(argc, argv, "g:m:n:o:s:u:")) != -1 ) {
> + while ((ch = getopt(argc, argv, "P:g:m:n:o:s:u:")) != -1 ) {
> switch (ch) {
> case 'g':
> gid = a_gid(optarg);
> @@ -131,6 +136,10 @@ mount_tmpfs_parseargs(int argc, char *ar
> uidset = 1;
> break;
>
> + case 'P':
> + strlcpy(pop_dir, optarg, PATH_MAX);
> + break;
> +
> case '?':
> default:
> usage();
> @@ -161,7 +170,8 @@ usage(void)
> extern char *__progname;
> (void)fprintf(stderr,
> "usage: %s [-g group] [-m mode] [-n nodes] [-o
[PATCH] mount_tmpfs -P option for populating after mounting (simplified)
Hello,
Although a patch like this was submitted before, this one is more "focused". It
may be useful for those moving from MFS who use this option to pre-populate an
MFS mount with a collection of files.
This will refuse to work if the path specified for -P is not a directory.
If the copy is unsuccessful, unmount and inform the user.
A few questions on this though:
1. Is there a better way than executing pax, to copy all the files in a
directory?
2. The do_exec() function was sort of copied from sbin/newfs/newfs.c. It
doesn't feel like the best way of handling this, but some research suggests it
is better than calling system() due to security issues, one that springs to
mind is a malformed input directory to the -P option. Is there a simple way to
sanitise the input for use with system()?
3. I noticed a pattern of functions that have about 2-3 lines of code in them.
Is it best to keep things that way, or is it best to merge them into a single
function (eg. copy_dir() merged into the mount_tmpfs() function)?
4. Are variable declarations to be at the beginning of a function block, or the
beginning of the immediate block? I feel limiting scope is a good idea, but
would like some feedback on this as I cannot see anything in style(9).
5. Is mount_tmpfs.h really necessary, given nothing in the tree seems to use
anything related to the mount_tmpfs() function which is internal to the
mount_tmpfs binary?
-
Index: sbin/mount_tmpfs/mount_tmpfs.8
===
RCS file: /cvs/src/sbin/mount_tmpfs/mount_tmpfs.8,v
retrieving revision 1.4
diff -u -p -r1.4 mount_tmpfs.8
--- sbin/mount_tmpfs/mount_tmpfs.8 16 Nov 2014 02:22:10 - 1.4
+++ sbin/mount_tmpfs/mount_tmpfs.8 13 Sep 2015 13:03:34 -
@@ -41,6 +41,7 @@
.Op Fl m Ar mode
.Op Fl n Ar nodes
.Op Fl o Ar options
+.Op Fl P Ar directory
.Op Fl s Ar size
.Op Fl u Ar user
.Ar tmpfs
@@ -80,6 +81,8 @@ flag followed by a comma-separated strin
See the
.Xr mount 8
man page for possible options and their meanings.
+.It Fl P Ar directory
+Populate the created tmpfs file system with the contents of the directory.
.It Fl s Ar size
Specifies the total file system size in bytes.
If zero is given (the default), the available amount of memory (including
@@ -136,6 +139,10 @@ and
.Ox 5.5 .
.Sh CAVEATS
The update of mount options (through mount -u) is currently not supported.
+The
+.Fl P
+option will produce an error if the mount is read-only, or if the files
+cannot be copied from the specified template directory.
.Sh BUGS
File system meta-data is not pageable.
If there is not enough main memory to hold this information, the system may
Index: sbin/mount_tmpfs/mount_tmpfs.c
===
RCS file: /cvs/src/sbin/mount_tmpfs/mount_tmpfs.c,v
retrieving revision 1.5
diff -u -p -r1.5 mount_tmpfs.c
--- sbin/mount_tmpfs/mount_tmpfs.c 16 Jan 2015 06:39:59 - 1.5
+++ sbin/mount_tmpfs/mount_tmpfs.c 13 Sep 2015 13:03:34 -
@@ -34,6 +34,7 @@
#include
#include
#include
+#include
#include
#include
@@ -41,6 +42,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -68,12 +70,15 @@ static int a_num(const char *, const cha
static mode_t a_mask(const char *);
static voidpathadj(const char *, char *);
+static int do_exec(const char *, const char *, char *const[]);
+static int copy_dir(char *, char *);
+
/* - */
void
mount_tmpfs_parseargs(int argc, char *argv[],
struct tmpfs_args *args, int *mntflags,
- char *canon_dev, char *canon_dir)
+ char *canon_dev, char *canon_dir, char *pop_dir)
{
int gidset, modeset, uidset; /* Ought to be 'bool'. */
int ch;
@@ -95,7 +100,7 @@ mount_tmpfs_parseargs(int argc, char *ar
modeset = 0; mode = 0;
optind = optreset = 1;
- while ((ch = getopt(argc, argv, "g:m:n:o:s:u:")) != -1 ) {
+ while ((ch = getopt(argc, argv, "P:g:m:n:o:s:u:")) != -1 ) {
switch (ch) {
case 'g':
gid = a_gid(optarg);
@@ -131,6 +136,10 @@ mount_tmpfs_parseargs(int argc, char *ar
uidset = 1;
break;
+ case 'P':
+ strlcpy(pop_dir, optarg, PATH_MAX);
+ break;
+
case '?':
default:
usage();
@@ -161,7 +170,8 @@ usage(void)
extern char *__progname;
(void)fprintf(stderr,
"usage: %s [-g group] [-m mode] [-n nodes] [-o options] [-s size]\n"
- " [-u user] tmpfs mount_point\n", __progname);
+ " [-P directory] [-u user] tmpfs mount_point\n",
+ __progname);
exit(1);
}
@@ -172,14 +182,33 @@ mount_tmpfs(int argc, char *argv[])
{
