Re: [systemd-devel] [PATCH v2] Do not clear parent mount flags when setting up namespaces

2015-01-03 Thread Topi Miettinen 
On 01/02/15 23:53, Djalal Harouni wrote:
 Hi,
 
 On Thu, Jan 01, 2015 at 10:36:39PM +0200, Topi Miettinen wrote:
 Copy parent directory mount flags when setting up a namespace and
 don't accidentally clear mount flags later.
 As noted by Colin in the other email, there should be a git log message
 for why the change.
 
 Yes thank you, I see that in one of the replies of v1 of the patch you
 say why, so just perhaps use it in the commit log and code comment ?
 
 
 ---
  src/core/namespace.c |  4 ++--
  src/shared/util.c| 19 +--
  src/shared/util.h|  2 ++
  3 files changed, 21 insertions(+), 4 deletions(-)

 diff --git a/src/core/namespace.c b/src/core/namespace.c
 index 4b8dbdd..6859b6a 100644
 --- a/src/core/namespace.c
 +++ b/src/core/namespace.c
 @@ -159,7 +159,7 @@ static int mount_dev(BindMount *m) {
  
  dev = strappenda(temporary_mount, /dev);
  (void)mkdir(dev, 0755);
 -if (mount(tmpfs, dev, tmpfs, MS_NOSUID|MS_STRICTATIME, 
 mode=755)  0) {
 +if (mount(tmpfs, dev, tmpfs, 
 get_mount_flags(/dev)|MS_NOSUID|MS_STRICTATIME, mode=755)  0) {
 There is no need for this function to be a parameter
 
 
  r = -errno;
  goto fail;
  }
 @@ -282,7 +282,7 @@ static int mount_kdbus(BindMount *m) {
  
  root = strappenda(temporary_mount, /kdbus);
  (void)mkdir(root, 0755);
 -if (mount(tmpfs, root, tmpfs, MS_NOSUID|MS_STRICTATIME, 
 mode=777)  0) {
 +if (mount(tmpfs, root, tmpfs, 
 get_mount_flags(/sys/fs/kdbus)|MS_NOSUID|MS_STRICTATIME, mode=777)  0) {
  r = -errno;
  goto fail;
  }
 diff --git a/src/shared/util.c b/src/shared/util.c
 index dfaf7f7..8ff5073 100644
 --- a/src/shared/util.c
 +++ b/src/shared/util.c
 @@ -61,6 +61,7 @@
  #include sys/personality.h
  #include sys/xattr.h
  #include libgen.h
 +#include sys/statvfs.h
  #undef basename
  
  #ifdef HAVE_SYS_AUXV_H
 @@ -6858,6 +6859,15 @@ int umount_recursive(const char *prefix, int flags) {
  return r ? r : n;
  }
  
 +unsigned long get_mount_flags(const char *path) {
 +struct statvfs buf;
 +
 +if (statvfs(path, buf)  0)
 +return 0;
 IMO here it should return an errno since this is a helper. In that case
 perhaps just open code the statvfs() or improve the helper ?

I'll make it return errno, for general use it's of course good to know
if there was a problem.

 
 Thanks!
 

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH v2] Do not clear parent mount flags when setting up namespaces

2015-01-03 Thread Topi Miettinen 
On 01/02/15 21:49, Colin Walters wrote:
 On Thu, Jan 1, 2015, at 03:36 PM, Topi Miettinen wrote:
 Copy parent directory mount flags when setting up a namespace and
 don't accidentally clear mount flags later.
 
 I think unless they're obvious, git commits should at least have a brief 
 rationale for *why* you're making the change, not just *what* the change is.  
 That way someone later editing the code has an idea what they might break if 
 they changed it.
 
 Is it something like mounting the tmpfs with options like size=?

It looks like fs specific options do not change when doing the bind
mount. The options that do get cleared unless copied are nosuid, nodev
and noexec flags, maybe also MS_SYNCHRONOUS, MS_POSIXACL etc.

 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel
 

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH v2] Do not clear parent mount flags when setting up namespaces

2015-01-02 Thread Djalal Harouni 
Hi,

On Thu, Jan 01, 2015 at 10:36:39PM +0200, Topi Miettinen wrote:
 Copy parent directory mount flags when setting up a namespace and
 don't accidentally clear mount flags later.
As noted by Colin in the other email, there should be a git log message
for why the change.

Yes thank you, I see that in one of the replies of v1 of the patch you
say why, so just perhaps use it in the commit log and code comment ?


 ---
  src/core/namespace.c |  4 ++--
  src/shared/util.c| 19 +--
  src/shared/util.h|  2 ++
  3 files changed, 21 insertions(+), 4 deletions(-)
 
 diff --git a/src/core/namespace.c b/src/core/namespace.c
 index 4b8dbdd..6859b6a 100644
 --- a/src/core/namespace.c
 +++ b/src/core/namespace.c
 @@ -159,7 +159,7 @@ static int mount_dev(BindMount *m) {
  
  dev = strappenda(temporary_mount, /dev);
  (void)mkdir(dev, 0755);
 -if (mount(tmpfs, dev, tmpfs, MS_NOSUID|MS_STRICTATIME, 
 mode=755)  0) {
 +if (mount(tmpfs, dev, tmpfs, 
 get_mount_flags(/dev)|MS_NOSUID|MS_STRICTATIME, mode=755)  0) {
There is no need for this function to be a parameter


  r = -errno;
  goto fail;
  }
 @@ -282,7 +282,7 @@ static int mount_kdbus(BindMount *m) {
  
  root = strappenda(temporary_mount, /kdbus);
  (void)mkdir(root, 0755);
 -if (mount(tmpfs, root, tmpfs, MS_NOSUID|MS_STRICTATIME, 
 mode=777)  0) {
 +if (mount(tmpfs, root, tmpfs, 
 get_mount_flags(/sys/fs/kdbus)|MS_NOSUID|MS_STRICTATIME, mode=777)  0) {
  r = -errno;
  goto fail;
  }
 diff --git a/src/shared/util.c b/src/shared/util.c
 index dfaf7f7..8ff5073 100644
 --- a/src/shared/util.c
 +++ b/src/shared/util.c
 @@ -61,6 +61,7 @@
  #include sys/personality.h
  #include sys/xattr.h
  #include libgen.h
 +#include sys/statvfs.h
  #undef basename
  
  #ifdef HAVE_SYS_AUXV_H
 @@ -6858,6 +6859,15 @@ int umount_recursive(const char *prefix, int flags) {
  return r ? r : n;
  }
  
 +unsigned long get_mount_flags(const char *path) {
 +struct statvfs buf;
 +
 +if (statvfs(path, buf)  0)
 +return 0;
IMO here it should return an errno since this is a helper. In that case
perhaps just open code the statvfs() or improve the helper ?

Thanks!

-- 
Djalal Harouni
http://opendz.org
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH v2] Do not clear parent mount flags when setting up namespaces

2015-01-02 Thread Colin Walters 
On Thu, Jan 1, 2015, at 03:36 PM, Topi Miettinen wrote:
 Copy parent directory mount flags when setting up a namespace and
 don't accidentally clear mount flags later.

I think unless they're obvious, git commits should at least have a brief 
rationale for *why* you're making the change, not just *what* the change is.  
That way someone later editing the code has an idea what they might break if 
they changed it.

Is it something like mounting the tmpfs with options like size=?
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH v2] Do not clear parent mount flags when setting up namespaces

2015-01-01 Thread Topi Miettinen 
Copy parent directory mount flags when setting up a namespace and
don't accidentally clear mount flags later.
---
 src/core/namespace.c |  4 ++--
 src/shared/util.c| 19 +--
 src/shared/util.h|  2 ++
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/src/core/namespace.c b/src/core/namespace.c
index 4b8dbdd..6859b6a 100644
--- a/src/core/namespace.c
+++ b/src/core/namespace.c
@@ -159,7 +159,7 @@ static int mount_dev(BindMount *m) {
 
 dev = strappenda(temporary_mount, /dev);
 (void)mkdir(dev, 0755);
-if (mount(tmpfs, dev, tmpfs, MS_NOSUID|MS_STRICTATIME, mode=755) 
 0) {
+if (mount(tmpfs, dev, tmpfs, 
get_mount_flags(/dev)|MS_NOSUID|MS_STRICTATIME, mode=755)  0) {
 r = -errno;
 goto fail;
 }
@@ -282,7 +282,7 @@ static int mount_kdbus(BindMount *m) {
 
 root = strappenda(temporary_mount, /kdbus);
 (void)mkdir(root, 0755);
-if (mount(tmpfs, root, tmpfs, MS_NOSUID|MS_STRICTATIME, 
mode=777)  0) {
+if (mount(tmpfs, root, tmpfs, 
get_mount_flags(/sys/fs/kdbus)|MS_NOSUID|MS_STRICTATIME, mode=777)  0) {
 r = -errno;
 goto fail;
 }
diff --git a/src/shared/util.c b/src/shared/util.c
index dfaf7f7..8ff5073 100644
--- a/src/shared/util.c
+++ b/src/shared/util.c
@@ -61,6 +61,7 @@
 #include sys/personality.h
 #include sys/xattr.h
 #include libgen.h
+#include sys/statvfs.h
 #undef basename
 
 #ifdef HAVE_SYS_AUXV_H
@@ -6858,6 +6859,15 @@ int umount_recursive(const char *prefix, int flags) {
 return r ? r : n;
 }
 
+unsigned long get_mount_flags(const char *path) {
+struct statvfs buf;
+
+if (statvfs(path, buf)  0)
+return 0;
+
+return buf.f_flag;
+}
+
 int bind_remount_recursive(const char *prefix, bool ro) {
 _cleanup_set_free_free_ Set *done = NULL;
 _cleanup_free_ char *cleaned = NULL;
@@ -6892,6 +6902,7 @@ int bind_remount_recursive(const char *prefix, bool ro) {
 _cleanup_set_free_free_ Set *todo = NULL;
 bool top_autofs = false;
 char *x;
+unsigned long orig_flags;
 
 todo = set_new(string_hash_ops);
 if (!todo)
@@ -6969,7 +6980,9 @@ int bind_remount_recursive(const char *prefix, bool ro) {
 if (mount(cleaned, cleaned, NULL, MS_BIND|MS_REC, 
NULL)  0)
 return -errno;
 
-if (mount(NULL, prefix, NULL, MS_BIND|MS_REMOUNT|(ro ? 
MS_RDONLY : 0), NULL)  0)
+orig_flags = get_mount_flags(prefix);
+orig_flags = ~MS_RDONLY;
+if (mount(NULL, prefix, NULL, 
orig_flags|MS_BIND|MS_REMOUNT|(ro ? MS_RDONLY : 0), NULL)  0)
 return -errno;
 
 x = strdup(cleaned);
@@ -6989,7 +7002,9 @@ int bind_remount_recursive(const char *prefix, bool ro) {
 if (r  0)
 return r;
 
-if (mount(NULL, x, NULL, MS_BIND|MS_REMOUNT|(ro ? 
MS_RDONLY : 0), NULL)  0) {
+orig_flags = get_mount_flags(x);
+orig_flags = ~MS_RDONLY;
+if (mount(NULL, x, NULL, 
orig_flags|MS_BIND|MS_REMOUNT|(ro ? MS_RDONLY : 0), NULL)  0) {
 
 /* Deal with mount points that are
  * obstructed by a later mount */
diff --git a/src/shared/util.h b/src/shared/util.h
index a131a3c..4b3070a 100644
--- a/src/shared/util.h
+++ b/src/shared/util.h
@@ -1021,6 +1021,8 @@ union file_handle_union {
 
 int update_reboot_param_file(const char *param);
 
+unsigned long get_mount_flags(const char *path);
+
 int umount_recursive(const char *target, int flags);
 
 int bind_remount_recursive(const char *prefix, bool ro);
-- 
2.1.4

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel