[libvirt] [PATCH] virprocess: Extend list of platforms for setns wrapper
Currently, the setns() wrapper is supported only for x86_64 and i686 which leaves us failing to build on other platforms like arm, aarch64 and so on. This means, that the wrapper needs to be extended to those platforms and make to fail on runtime not compile time. The syscall numbers for other platforms was fetched using this command: kernel.git $ git grep "define.*__NR_setns" | grep -e arm -e powerpc -e s390 arch/arm/include/uapi/asm/unistd.h:#define __NR_setns (__NR_SYSCALL_BASE+375) arch/arm64/include/asm/unistd32.h:#define __NR_setns 375 arch/powerpc/include/uapi/asm/unistd.h:#define __NR_setns 350 arch/s390/include/uapi/asm/unistd.h:#define __NR_setns 339 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virprocess.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 3dae1bd..eac49f5 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -71,27 +71,33 @@ VIR_LOG_INIT("util.process"); # define __NR_setns 308 # elif defined(__i386__) # define __NR_setns 346 -# else -# error "__NR_setns is not defined" +# elif defined(__arm__) +# define __NR_setns 375 +# elif defined(__aarch64__) +# define __NR_setns 375 +# elif defined(__powerpc__) +# define __NR_setns 350 +# elif defined(__s390__) +# define __NR_setns 339 # endif #endif #ifndef HAVE_SETNS -# ifndef WIN32 +# ifdef __NR_setns # include <sys/syscall.h> static inline int setns(int fd, int nstype) { return syscall(__NR_setns, fd, nstype); } -# else +# else /* __NR_setns */ static inline int setns(int fd ATTRIBUTE_UNUSED, int nstype ATTRIBUTE_UNUSED) { virReportSystemError(ENOSYS, "%s", - _("Namespaces are not supported on windows.")); + _("Namespaces are not supported on this platform.")); return -1; } -# endif /* WIN32 */ +# endif /* __NR_setns */ #endif /* HAVE_SETNS */ /** -- 1.8.5.5
On 09/15/2014 03:43 PM, Michal Privoznik wrote:
Currently, the setns() wrapper is supported only for x86_64 and i686 which leaves us failing to build on other platforms like arm, aarch64 and so on. This means, that the wrapper needs to be extended to those platforms and make to fail on runtime not compile time.
The syscall numbers for other platforms was fetched using this command:
kernel.git $ git grep "define.*__NR_setns" | grep -e arm -e powerpc -e s390 arch/arm/include/uapi/asm/unistd.h:#define __NR_setns (__NR_SYSCALL_BASE+375) arch/arm64/include/asm/unistd32.h:#define __NR_setns 375 arch/powerpc/include/uapi/asm/unistd.h:#define __NR_setns 350 arch/s390/include/uapi/asm/unistd.h:#define __NR_setns 339
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virprocess.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
ACK Jan
On 09/15/2014 03:43 PM, Michal Privoznik wrote:
Currently, the setns() wrapper is supported only for x86_64 and i686 which leaves us failing to build on other platforms like arm, aarch64 and so on. This means, that the wrapper needs to be extended to those platforms and make to fail on runtime not compile time.
The syscall numbers for other platforms was fetched using this command:
kernel.git $ git grep "define.*__NR_setns" | grep -e arm -e powerpc -e s390 arch/arm/include/uapi/asm/unistd.h:#define __NR_setns (__NR_SYSCALL_BASE+375) arch/arm64/include/asm/unistd32.h:#define __NR_setns 375 arch/powerpc/include/uapi/asm/unistd.h:#define __NR_setns 350 arch/s390/include/uapi/asm/unistd.h:#define __NR_setns 339
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virprocess.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 3dae1bd..eac49f5 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -71,27 +71,33 @@ VIR_LOG_INIT("util.process"); # define __NR_setns 308 # elif defined(__i386__) # define __NR_setns 346 -# else -# error "__NR_setns is not defined" +# elif defined(__arm__) +# define __NR_setns 375 +# elif defined(__aarch64__) +# define __NR_setns 375 +# elif defined(__powerpc__) +# define __NR_setns 350 +# elif defined(__s390__) +# define __NR_setns 339 # endif #endif
#ifndef HAVE_SETNS -# ifndef WIN32 +# ifdef __NR_setns
This would work only if the macro wasn't defined in kernel, but it seems that it's defined also for WIN32.
# include <sys/syscall.h>
static inline int setns(int fd, int nstype) { return syscall(__NR_setns, fd, nstype); } -# else +# else /* __NR_setns */ static inline int setns(int fd ATTRIBUTE_UNUSED, int nstype ATTRIBUTE_UNUSED) { virReportSystemError(ENOSYS, "%s", - _("Namespaces are not supported on windows.")); + _("Namespaces are not supported on this platform.")); return -1; } -# endif /* WIN32 */ +# endif /* __NR_setns */ #endif /* HAVE_SETNS */
/**
NACK as it again breaks the MinGW build. Pavel
On 15.09.2014 16:20, Pavel Hrdina wrote:
On 09/15/2014 03:43 PM, Michal Privoznik wrote:
Currently, the setns() wrapper is supported only for x86_64 and i686 which leaves us failing to build on other platforms like arm, aarch64 and so on. This means, that the wrapper needs to be extended to those platforms and make to fail on runtime not compile time.
The syscall numbers for other platforms was fetched using this command:
kernel.git $ git grep "define.*__NR_setns" | grep -e arm -e powerpc -e s390 arch/arm/include/uapi/asm/unistd.h:#define __NR_setns (__NR_SYSCALL_BASE+375) arch/arm64/include/asm/unistd32.h:#define __NR_setns 375 arch/powerpc/include/uapi/asm/unistd.h:#define __NR_setns 350 arch/s390/include/uapi/asm/unistd.h:#define __NR_setns 339
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virprocess.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 3dae1bd..eac49f5 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -71,27 +71,33 @@ VIR_LOG_INIT("util.process"); # define __NR_setns 308 # elif defined(__i386__) # define __NR_setns 346 -# else -# error "__NR_setns is not defined" +# elif defined(__arm__) +# define __NR_setns 375 +# elif defined(__aarch64__) +# define __NR_setns 375 +# elif defined(__powerpc__) +# define __NR_setns 350 +# elif defined(__s390__) +# define __NR_setns 339 # endif #endif
#ifndef HAVE_SETNS -# ifndef WIN32 +# ifdef __NR_setns
This would work only if the macro wasn't defined in kernel, but it seems that it's defined also for WIN32.
Oh my, why the heck do they claim something is supported when it's clearly not?! This needs to be squashed in then: diff --git a/src/util/virprocess.c b/src/util/virprocess.c index eac49f5..806e7f9 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -83,21 +83,21 @@ VIR_LOG_INIT("util.process"); #endif #ifndef HAVE_SETNS -# ifdef __NR_setns +# if defined(__NR_setns) && !defined(WIN32) # include <sys/syscall.h> static inline int setns(int fd, int nstype) { return syscall(__NR_setns, fd, nstype); } -# else /* __NR_setns */ +# else /* __NR_setns && !WIN32 */ static inline int setns(int fd ATTRIBUTE_UNUSED, int nstype ATTRIBUTE_UNUSED) { virReportSystemError(ENOSYS, "%s", _("Namespaces are not supported on this platform.")); return -1; } -# endif /* __NR_setns */ +# endif /* __NR_setns && !WIN32 */ #endif /* HAVE_SETNS */ /** The other approach would be just to require new enough glibc. Opinions? Michal
On Mon, Sep 15, 2014 at 05:08:01PM +0200, Michal Privoznik wrote:
On 15.09.2014 16:20, Pavel Hrdina wrote:
On 09/15/2014 03:43 PM, Michal Privoznik wrote:
Currently, the setns() wrapper is supported only for x86_64 and i686 which leaves us failing to build on other platforms like arm, aarch64 and so on. This means, that the wrapper needs to be extended to those platforms and make to fail on runtime not compile time.
The syscall numbers for other platforms was fetched using this command:
kernel.git $ git grep "define.*__NR_setns" | grep -e arm -e powerpc -e s390 arch/arm/include/uapi/asm/unistd.h:#define __NR_setns (__NR_SYSCALL_BASE+375) arch/arm64/include/asm/unistd32.h:#define __NR_setns 375 arch/powerpc/include/uapi/asm/unistd.h:#define __NR_setns 350 arch/s390/include/uapi/asm/unistd.h:#define __NR_setns 339
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virprocess.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 3dae1bd..eac49f5 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -71,27 +71,33 @@ VIR_LOG_INIT("util.process"); # define __NR_setns 308 # elif defined(__i386__) # define __NR_setns 346 -# else -# error "__NR_setns is not defined" +# elif defined(__arm__) +# define __NR_setns 375 +# elif defined(__aarch64__) +# define __NR_setns 375 +# elif defined(__powerpc__) +# define __NR_setns 350 +# elif defined(__s390__) +# define __NR_setns 339 # endif #endif
#ifndef HAVE_SETNS -# ifndef WIN32 +# ifdef __NR_setns
This would work only if the macro wasn't defined in kernel, but it seems that it's defined also for WIN32.
Oh my, why the heck do they claim something is supported when it's clearly not?! This needs to be squashed in then:
diff --git a/src/util/virprocess.c b/src/util/virprocess.c index eac49f5..806e7f9 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -83,21 +83,21 @@ VIR_LOG_INIT("util.process"); #endif
#ifndef HAVE_SETNS -# ifdef __NR_setns +# if defined(__NR_setns) && !defined(WIN32) # include <sys/syscall.h>
static inline int setns(int fd, int nstype) { return syscall(__NR_setns, fd, nstype); } -# else /* __NR_setns */ +# else /* __NR_setns && !WIN32 */ static inline int setns(int fd ATTRIBUTE_UNUSED, int nstype ATTRIBUTE_UNUSED) { virReportSystemError(ENOSYS, "%s", _("Namespaces are not supported on this platform.")); return -1; } -# endif /* __NR_setns */ +# endif /* __NR_setns && !WIN32 */ #endif /* HAVE_SETNS */
/**
The other approach would be just to require new enough glibc. Opinions?
Definitely require what we need. Or at least don't redefine __NR_* macros. I expressed my opinion in another thread since I started writing the message before I had this mail in my INBOX. Martin
On Mon, Sep 15, 2014 at 03:43:55PM +0200, Michal Privoznik wrote:
Currently, the setns() wrapper is supported only for x86_64 and i686 which leaves us failing to build on other platforms like arm, aarch64 and so on. This means, that the wrapper needs to be extended to those platforms and make to fail on runtime not compile time.
The syscall numbers for other platforms was fetched using this command:
kernel.git $ git grep "define.*__NR_setns" | grep -e arm -e powerpc -e s390 arch/arm/include/uapi/asm/unistd.h:#define __NR_setns (__NR_SYSCALL_BASE+375) arch/arm64/include/asm/unistd32.h:#define __NR_setns 375 arch/powerpc/include/uapi/asm/unistd.h:#define __NR_setns 350 arch/s390/include/uapi/asm/unistd.h:#define __NR_setns 339
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virprocess.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
NACK, we shouldn't be duplicating syscall definitions. There should be AC_CHECK_FUNCS([setns]) (instead of AC_CHECK_FUNCS_ONCE() for the syscall) and having with_lxc = "yes" and ac_cv_func_setns != "yes" should result in an error. If you really don't want to do this, you could do syscall(SYS_setns, ...); but definitely not duplicate __NR_.* macros from anywhere. The fact that it starts with double underscore should tell you that. We are already doing that syscall(SYS_...) trick on two places in the code, but it is appropriately encapsulated in defined(SYS_...) and they are not redefined anywhere. The cleanest thing would be just to require new enough kernel/libc that supports what we need. Martin
diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 3dae1bd..eac49f5 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -71,27 +71,33 @@ VIR_LOG_INIT("util.process"); # define __NR_setns 308 # elif defined(__i386__) # define __NR_setns 346 -# else -# error "__NR_setns is not defined" +# elif defined(__arm__) +# define __NR_setns 375 +# elif defined(__aarch64__) +# define __NR_setns 375 +# elif defined(__powerpc__) +# define __NR_setns 350 +# elif defined(__s390__) +# define __NR_setns 339 # endif #endif
#ifndef HAVE_SETNS -# ifndef WIN32 +# ifdef __NR_setns # include <sys/syscall.h>
static inline int setns(int fd, int nstype) { return syscall(__NR_setns, fd, nstype); } -# else +# else /* __NR_setns */ static inline int setns(int fd ATTRIBUTE_UNUSED, int nstype ATTRIBUTE_UNUSED) { virReportSystemError(ENOSYS, "%s", - _("Namespaces are not supported on windows.")); + _("Namespaces are not supported on this platform.")); return -1; } -# endif /* WIN32 */ +# endif /* __NR_setns */ #endif /* HAVE_SETNS */
/** -- 1.8.5.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 15.09.2014 17:15, Martin Kletzander wrote:
On Mon, Sep 15, 2014 at 03:43:55PM +0200, Michal Privoznik wrote:
Currently, the setns() wrapper is supported only for x86_64 and i686 which leaves us failing to build on other platforms like arm, aarch64 and so on. This means, that the wrapper needs to be extended to those platforms and make to fail on runtime not compile time.
The syscall numbers for other platforms was fetched using this command:
kernel.git $ git grep "define.*__NR_setns" | grep -e arm -e powerpc -e s390 arch/arm/include/uapi/asm/unistd.h:#define __NR_setns (__NR_SYSCALL_BASE+375) arch/arm64/include/asm/unistd32.h:#define __NR_setns 375 arch/powerpc/include/uapi/asm/unistd.h:#define __NR_setns 350 arch/s390/include/uapi/asm/unistd.h:#define __NR_setns 339
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virprocess.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
NACK, we shouldn't be duplicating syscall definitions. There should be AC_CHECK_FUNCS([setns]) (instead of AC_CHECK_FUNCS_ONCE() for the syscall) and having with_lxc = "yes" and ac_cv_func_setns != "yes" should result in an error.
The only problem with this might be that on systems with older glibc (and there is plenty of them) libvirt will fail to build / miss this feature. And it's not that the kernel doesn't support the namesapces. But let me see if I can get some ACKs on that approach you're suggesting. Michal
On Mon, Sep 15, 2014 at 05:20:46PM +0200, Michal Privoznik wrote:
On 15.09.2014 17:15, Martin Kletzander wrote:
On Mon, Sep 15, 2014 at 03:43:55PM +0200, Michal Privoznik wrote:
Currently, the setns() wrapper is supported only for x86_64 and i686 which leaves us failing to build on other platforms like arm, aarch64 and so on. This means, that the wrapper needs to be extended to those platforms and make to fail on runtime not compile time.
The syscall numbers for other platforms was fetched using this command:
kernel.git $ git grep "define.*__NR_setns" | grep -e arm -e powerpc -e s390 arch/arm/include/uapi/asm/unistd.h:#define __NR_setns (__NR_SYSCALL_BASE+375) arch/arm64/include/asm/unistd32.h:#define __NR_setns 375 arch/powerpc/include/uapi/asm/unistd.h:#define __NR_setns 350 arch/s390/include/uapi/asm/unistd.h:#define __NR_setns 339
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virprocess.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
NACK, we shouldn't be duplicating syscall definitions. There should be AC_CHECK_FUNCS([setns]) (instead of AC_CHECK_FUNCS_ONCE() for the syscall) and having with_lxc = "yes" and ac_cv_func_setns != "yes" should result in an error.
The only problem with this might be that on systems with older glibc (and there is plenty of them) libvirt will fail to build / miss this feature. And it's not that the kernel doesn't support the namesapces. But let me see if I can get some ACKs on that approach you're suggesting.
That's basically what the code did before we added the #define or NR_setns. We took the patch specifically to help Debian where the kernel has it but glibc is outdated. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Mon, Sep 15, 2014 at 04:22:18PM +0100, Daniel P. Berrange wrote:
On Mon, Sep 15, 2014 at 05:20:46PM +0200, Michal Privoznik wrote:
On 15.09.2014 17:15, Martin Kletzander wrote:
On Mon, Sep 15, 2014 at 03:43:55PM +0200, Michal Privoznik wrote:
Currently, the setns() wrapper is supported only for x86_64 and i686 which leaves us failing to build on other platforms like arm, aarch64 and so on. This means, that the wrapper needs to be extended to those platforms and make to fail on runtime not compile time.
The syscall numbers for other platforms was fetched using this command:
kernel.git $ git grep "define.*__NR_setns" | grep -e arm -e powerpc -e s390 arch/arm/include/uapi/asm/unistd.h:#define __NR_setns (__NR_SYSCALL_BASE+375) arch/arm64/include/asm/unistd32.h:#define __NR_setns 375 arch/powerpc/include/uapi/asm/unistd.h:#define __NR_setns 350 arch/s390/include/uapi/asm/unistd.h:#define __NR_setns 339
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virprocess.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
NACK, we shouldn't be duplicating syscall definitions. There should be AC_CHECK_FUNCS([setns]) (instead of AC_CHECK_FUNCS_ONCE() for the syscall) and having with_lxc = "yes" and ac_cv_func_setns != "yes" should result in an error.
The only problem with this might be that on systems with older glibc (and there is plenty of them) libvirt will fail to build / miss this feature. And it's not that the kernel doesn't support the namesapces. But let me see if I can get some ACKs on that approach you're suggesting.
That's basically what the code did before we added the #define or NR_setns. We took the patch specifically to help Debian where the kernel has it but glibc is outdated.
Either Debian should patch their glibc or we should at lease use SYS_setns IMHO.
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 15.09.2014 17:32, Martin Kletzander wrote:
On Mon, Sep 15, 2014 at 04:22:18PM +0100, Daniel P. Berrange wrote:
On Mon, Sep 15, 2014 at 05:20:46PM +0200, Michal Privoznik wrote:
On 15.09.2014 17:15, Martin Kletzander wrote:
On Mon, Sep 15, 2014 at 03:43:55PM +0200, Michal Privoznik wrote:
Currently, the setns() wrapper is supported only for x86_64 and i686 which leaves us failing to build on other platforms like arm, aarch64 and so on. This means, that the wrapper needs to be extended to those platforms and make to fail on runtime not compile time.
The syscall numbers for other platforms was fetched using this command:
kernel.git $ git grep "define.*__NR_setns" | grep -e arm -e powerpc -e s390 arch/arm/include/uapi/asm/unistd.h:#define __NR_setns (__NR_SYSCALL_BASE+375) arch/arm64/include/asm/unistd32.h:#define __NR_setns 375 arch/powerpc/include/uapi/asm/unistd.h:#define __NR_setns 350 arch/s390/include/uapi/asm/unistd.h:#define __NR_setns 339
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virprocess.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
NACK, we shouldn't be duplicating syscall definitions. There should be AC_CHECK_FUNCS([setns]) (instead of AC_CHECK_FUNCS_ONCE() for the syscall) and having with_lxc = "yes" and ac_cv_func_setns != "yes" should result in an error.
The only problem with this might be that on systems with older glibc (and there is plenty of them) libvirt will fail to build / miss this feature. And it's not that the kernel doesn't support the namesapces. But let me see if I can get some ACKs on that approach you're suggesting.
That's basically what the code did before we added the #define or NR_setns. We took the patch specifically to help Debian where the kernel has it but glibc is outdated.
Either Debian should patch their glibc or we should at lease use SYS_setns IMHO.
That's not gonna fly either. On my system, the SYS_setns is declared in: # grep -r SYS_setns /usr/include/ /usr/include/bits/syscall.h:#define SYS_setns __NR_setns And the syscall.h belongs to glibc, not kernel headers. So we are back at the start. Michal
On Mon, Sep 15, 2014 at 05:36:16PM +0200, Michal Privoznik wrote:
On 15.09.2014 17:32, Martin Kletzander wrote:
On Mon, Sep 15, 2014 at 04:22:18PM +0100, Daniel P. Berrange wrote:
On Mon, Sep 15, 2014 at 05:20:46PM +0200, Michal Privoznik wrote:
On 15.09.2014 17:15, Martin Kletzander wrote:
On Mon, Sep 15, 2014 at 03:43:55PM +0200, Michal Privoznik wrote:
Currently, the setns() wrapper is supported only for x86_64 and i686 which leaves us failing to build on other platforms like arm, aarch64 and so on. This means, that the wrapper needs to be extended to those platforms and make to fail on runtime not compile time.
The syscall numbers for other platforms was fetched using this command:
kernel.git $ git grep "define.*__NR_setns" | grep -e arm -e powerpc -e s390 arch/arm/include/uapi/asm/unistd.h:#define __NR_setns (__NR_SYSCALL_BASE+375) arch/arm64/include/asm/unistd32.h:#define __NR_setns 375 arch/powerpc/include/uapi/asm/unistd.h:#define __NR_setns 350 arch/s390/include/uapi/asm/unistd.h:#define __NR_setns 339
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virprocess.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
NACK, we shouldn't be duplicating syscall definitions. There should be AC_CHECK_FUNCS([setns]) (instead of AC_CHECK_FUNCS_ONCE() for the syscall) and having with_lxc = "yes" and ac_cv_func_setns != "yes" should result in an error.
The only problem with this might be that on systems with older glibc (and there is plenty of them) libvirt will fail to build / miss this feature. And it's not that the kernel doesn't support the namesapces. But let me see if I can get some ACKs on that approach you're suggesting.
That's basically what the code did before we added the #define or NR_setns. We took the patch specifically to help Debian where the kernel has it but glibc is outdated.
Either Debian should patch their glibc or we should at lease use SYS_setns IMHO.
That's not gonna fly either. On my system, the SYS_setns is declared in:
# grep -r SYS_setns /usr/include/ /usr/include/bits/syscall.h:#define SYS_setns __NR_setns
And the syscall.h belongs to glibc, not kernel headers. So we are back at the start.
Well, I'd argue that we're not :) Distros could make our lives easier by not requiring us to guess their kernel's syscall numbers :)
Michal
On Mon, Sep 15, 2014 at 05:44:17PM +0200, Martin Kletzander wrote:
On Mon, Sep 15, 2014 at 05:36:16PM +0200, Michal Privoznik wrote:
On 15.09.2014 17:32, Martin Kletzander wrote:
On Mon, Sep 15, 2014 at 04:22:18PM +0100, Daniel P. Berrange wrote:
On Mon, Sep 15, 2014 at 05:20:46PM +0200, Michal Privoznik wrote:
On 15.09.2014 17:15, Martin Kletzander wrote:
On Mon, Sep 15, 2014 at 03:43:55PM +0200, Michal Privoznik wrote: >Currently, the setns() wrapper is supported only for x86_64 and i686 >which leaves us failing to build on other platforms like arm, aarch64 >and so on. This means, that the wrapper needs to be extended to those >platforms and make to fail on runtime not compile time. > >The syscall numbers for other platforms was fetched using this >command: > >kernel.git $ git grep "define.*__NR_setns" | grep -e arm -e powerpc -e >s390 >arch/arm/include/uapi/asm/unistd.h:#define >__NR_setns (__NR_SYSCALL_BASE+375) >arch/arm64/include/asm/unistd32.h:#define __NR_setns 375 >arch/powerpc/include/uapi/asm/unistd.h:#define >__NR_setns 350 >arch/s390/include/uapi/asm/unistd.h:#define __NR_setns 339 > >Signed-off-by: Michal Privoznik <mprivozn@redhat.com> >--- >src/util/virprocess.c | 18 ++++++++++++------ >1 file changed, 12 insertions(+), 6 deletions(-) >
NACK, we shouldn't be duplicating syscall definitions. There should be AC_CHECK_FUNCS([setns]) (instead of AC_CHECK_FUNCS_ONCE() for the syscall) and having with_lxc = "yes" and ac_cv_func_setns != "yes" should result in an error.
The only problem with this might be that on systems with older glibc (and there is plenty of them) libvirt will fail to build / miss this feature. And it's not that the kernel doesn't support the namesapces. But let me see if I can get some ACKs on that approach you're suggesting.
That's basically what the code did before we added the #define or NR_setns. We took the patch specifically to help Debian where the kernel has it but glibc is outdated.
Either Debian should patch their glibc or we should at lease use SYS_setns IMHO.
That's not gonna fly either. On my system, the SYS_setns is declared in:
# grep -r SYS_setns /usr/include/ /usr/include/bits/syscall.h:#define SYS_setns __NR_setns
And the syscall.h belongs to glibc, not kernel headers. So we are back at the start.
Well, I'd argue that we're not :) Distros could make our lives easier by not requiring us to guess their kernel's syscall numbers :)
Well we're not guessing - syscalls numbers are standardized ABI that does not change per distro. Realistically rebasing glibc in existing distros to pull in new functions isn't something distros are going todo. So unless we do a workaround in libvirt, users on that distro are facing a regression due us switching a bunch of LXC APIs to use setns() for a previous security fix. So I think defining NR_setns in libvirt code is the least bad, from many unpleasant options. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Mon, Sep 15, 2014 at 04:50:23PM +0100, Daniel P. Berrange wrote:
On Mon, Sep 15, 2014 at 05:44:17PM +0200, Martin Kletzander wrote:
On Mon, Sep 15, 2014 at 05:36:16PM +0200, Michal Privoznik wrote:
On 15.09.2014 17:32, Martin Kletzander wrote:
On Mon, Sep 15, 2014 at 04:22:18PM +0100, Daniel P. Berrange wrote:
On Mon, Sep 15, 2014 at 05:20:46PM +0200, Michal Privoznik wrote:
On 15.09.2014 17:15, Martin Kletzander wrote: >On Mon, Sep 15, 2014 at 03:43:55PM +0200, Michal Privoznik wrote: >>Currently, the setns() wrapper is supported only for x86_64 and i686 >>which leaves us failing to build on other platforms like arm, aarch64 >>and so on. This means, that the wrapper needs to be extended to those >>platforms and make to fail on runtime not compile time. >> >>The syscall numbers for other platforms was fetched using this >>command: >> >>kernel.git $ git grep "define.*__NR_setns" | grep -e arm -e powerpc -e >>s390 >>arch/arm/include/uapi/asm/unistd.h:#define >>__NR_setns (__NR_SYSCALL_BASE+375) >>arch/arm64/include/asm/unistd32.h:#define __NR_setns 375 >>arch/powerpc/include/uapi/asm/unistd.h:#define >>__NR_setns 350 >>arch/s390/include/uapi/asm/unistd.h:#define __NR_setns 339 >> >>Signed-off-by: Michal Privoznik <mprivozn@redhat.com> >>--- >>src/util/virprocess.c | 18 ++++++++++++------ >>1 file changed, 12 insertions(+), 6 deletions(-) >> > >NACK, we shouldn't be duplicating syscall definitions. There should >be AC_CHECK_FUNCS([setns]) (instead of AC_CHECK_FUNCS_ONCE() for the >syscall) and having with_lxc = "yes" and ac_cv_func_setns != "yes" >should result in an error.
The only problem with this might be that on systems with older glibc (and there is plenty of them) libvirt will fail to build / miss this feature. And it's not that the kernel doesn't support the namesapces. But let me see if I can get some ACKs on that approach you're suggesting.
That's basically what the code did before we added the #define or NR_setns. We took the patch specifically to help Debian where the kernel has it but glibc is outdated.
Either Debian should patch their glibc or we should at lease use SYS_setns IMHO.
That's not gonna fly either. On my system, the SYS_setns is declared in:
# grep -r SYS_setns /usr/include/ /usr/include/bits/syscall.h:#define SYS_setns __NR_setns
And the syscall.h belongs to glibc, not kernel headers. So we are back at the start.
BTW this only means that the glibc was compiled against kernel that is new enough. glibc doesn't keep SYS_syscall numbers around, for linux the file bits/syscall.h is generated at compile time against kernel. So if we do what I proposed and debian compiled that glibc with new enough kernel, then it would all be unicorns spitting rainbows around here. Unless we're doing it for debian stable, which is a completely different story (using circa 7 years old glibc with 2.5 years old kernel). Can't we at least include asm/unistd.h or something similar and user the __NR_setns defined there?
Well, I'd argue that we're not :) Distros could make our lives easier by not requiring us to guess their kernel's syscall numbers :)
Well we're not guessing - syscalls numbers are standardized ABI that does not change per distro.
Well, I looked at it from another point. Syscall numbers are part of kernel ABI and distros don't grant you binary compatibility (or at least for now, unfortunately). By guessing, I meant that there were numerous conditionals and if somebody uses an architecture that is not considered by us (e.g. hppa) or some new one comes along (e.g. ia64be), there's a slight chance that in the future we might get a wrong number in there and find that our pretty late.
Realistically rebasing glibc in existing distros to pull in new functions isn't something distros are going todo.
Well, they don't have to rebase, just pull back some needed commits. Or maybe just rebuild the package with kernel headers having same versions as those being distributed. But that depends on what versions we're talking about here. I missed the discussion about who wanted this, but *even* if they are running debian stable, it should be just a few-liner for glibc with a rebuild
So unless we do a workaround in libvirt, users on that distro are facing a regression due us switching a bunch of LXC APIs to use setns() for a previous security fix. So I think defining NR_setns in libvirt code is the least bad, from many unpleasant options.
From my understanding, making shipped versions match together is distros' job. Users on that distro may force the maintainers to fix that. But anyway, if you decide to go with an uneasy way of maintaining our own syscall definitions in freshly baked versions of libvirt just for rusty old glibc and laziness of some other people... I can't force you not to do that. So it's up to you guys now. Have a nice day, Martin
On 16.09.2014 08:53, Martin Kletzander wrote:
On Mon, Sep 15, 2014 at 04:50:23PM +0100, Daniel P. Berrange wrote:
On Mon, Sep 15, 2014 at 05:44:17PM +0200, Martin Kletzander wrote:
On 15.09.2014 17:32, Martin Kletzander wrote:
On Mon, Sep 15, 2014 at 04:22:18PM +0100, Daniel P. Berrange wrote:
On Mon, Sep 15, 2014 at 05:20:46PM +0200, Michal Privoznik wrote: >On 15.09.2014 17:15, Martin Kletzander wrote: >>On Mon, Sep 15, 2014 at 03:43:55PM +0200, Michal Privoznik wrote: >>>Currently, the setns() wrapper is supported only for x86_64 and i686 >>>which leaves us failing to build on other platforms like arm, aarch64 >>>and so on. This means, that the wrapper needs to be extended to
>>>platforms and make to fail on runtime not compile time. >>> >>>The syscall numbers for other platforms was fetched using this >>>command: >>> >>>kernel.git $ git grep "define.*__NR_setns" | grep -e arm -e
>>>s390 >>>arch/arm/include/uapi/asm/unistd.h:#define >>>__NR_setns (__NR_SYSCALL_BASE+375) >>>arch/arm64/include/asm/unistd32.h:#define __NR_setns 375 >>>arch/powerpc/include/uapi/asm/unistd.h:#define >>>__NR_setns 350 >>>arch/s390/include/uapi/asm/unistd.h:#define __NR_setns 339 >>> >>>Signed-off-by: Michal Privoznik <mprivozn@redhat.com> >>>--- >>>src/util/virprocess.c | 18 ++++++++++++------ >>>1 file changed, 12 insertions(+), 6 deletions(-) >>> >> >>NACK, we shouldn't be duplicating syscall definitions. There should >>be AC_CHECK_FUNCS([setns]) (instead of AC_CHECK_FUNCS_ONCE() for
On Mon, Sep 15, 2014 at 05:36:16PM +0200, Michal Privoznik wrote: those powerpc -e the
>>syscall) and having with_lxc = "yes" and ac_cv_func_setns != "yes" >>should result in an error. > >The only problem with this might be that on systems with older glibc >(and >there is plenty of them) libvirt will fail to build / miss this >feature. And >it's not that the kernel doesn't support the namesapces. But let me >see if I >can get some ACKs on that approach you're suggesting.
That's basically what the code did before we added the #define or NR_setns. We took the patch specifically to help Debian where the kernel has it but glibc is outdated.
Either Debian should patch their glibc or we should at lease use SYS_setns IMHO.
That's not gonna fly either. On my system, the SYS_setns is declared in:
# grep -r SYS_setns /usr/include/ /usr/include/bits/syscall.h:#define SYS_setns __NR_setns
And the syscall.h belongs to glibc, not kernel headers. So we are back at the start.
BTW this only means that the glibc was compiled against kernel that is new enough. glibc doesn't keep SYS_syscall numbers around, for linux the file bits/syscall.h is generated at compile time against kernel. So if we do what I proposed and debian compiled that glibc with new enough kernel, then it would all be unicorns spitting rainbows around here. Unless we're doing it for debian stable, which is a completely different story (using circa 7 years old glibc with 2.5 years old kernel).
Can't we at least include asm/unistd.h or something similar and user the __NR_setns defined there?
Well, I'd argue that we're not :) Distros could make our lives easier by not requiring us to guess their kernel's syscall numbers :)
Well we're not guessing - syscalls numbers are standardized ABI that does not change per distro.
Well, I looked at it from another point. Syscall numbers are part of kernel ABI and distros don't grant you binary compatibility (or at least for now, unfortunately).
By guessing, I meant that there were numerous conditionals and if somebody uses an architecture that is not considered by us (e.g. hppa) or some new one comes along (e.g. ia64be), there's a slight chance that in the future we might get a wrong number in there and find that our pretty late.
Realistically rebasing glibc in existing distros to pull in new functions isn't something distros are going todo.
Well, they don't have to rebase, just pull back some needed commits. Or maybe just rebuild the package with kernel headers having same versions as those being distributed. But that depends on what versions we're talking about here. I missed the discussion about who wanted this, but *even* if they are running debian stable, it should be just a few-liner for glibc with a rebuild
So unless we do a workaround in libvirt, users on that distro are facing a regression due us switching a bunch of LXC APIs to use setns() for a previous security fix. So I think defining NR_setns in libvirt code is the least bad, from many unpleasant options.
From my understanding, making shipped versions match together is distros' job. Users on that distro may force the maintainers to fix that.
But anyway, if you decide to go with an uneasy way of maintaining our own syscall definitions in freshly baked versions of libvirt just for rusty old glibc and laziness of some other people... I can't force you not to do that. So it's up to you guys now.
Have a nice day, Martin
Okay, So now that I have green flag I'm pushing this. Maybe in the future we can drop this, once Debian rebase its glibc. Michal
participants (5)
-
Daniel P. Berrange -
Ján Tomko -
Martin Kletzander -
Michal Privoznik -
Pavel Hrdina