[libvirt] [PATCH 0/2] Fix the latest virpcitest failure

See 2/2 for explanation. Michal Prívozník (2): ci: Allow gdb in containers virpcimock: Mock __open_2() Makefile.ci | 1 + tests/virpcimock.c | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+) -- 2.21.0

The gdb requires ptrace capability, but the way we run containers now is that they drop every capability. Preserve SYS_PTRACE then. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Makefile.ci | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile.ci b/Makefile.ci index 8857c953b2..977e0445c6 100644 --- a/Makefile.ci +++ b/Makefile.ci @@ -167,6 +167,7 @@ CI_ENGINE_ARGS = \ --volume $(CI_HOST_SRCDIR):$(CI_CONT_SRCDIR):z \ --workdir $(CI_CONT_SRCDIR) \ --ulimit nofile=$(CI_ULIMIT_FILES):$(CI_ULIMIT_FILES) \ + --cap-add=SYS_PTRACE \ $(NULL) ci-check-engine: -- 2.21.0

On Thu, Aug 15, 2019 at 05:15:56PM +0200, Michal Privoznik wrote:
The gdb requires ptrace capability, but the way we run containers now is that they drop every capability. Preserve SYS_PTRACE then.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
Makes sense to me, so I can give you my: Reviewed-by: Erik Skultety <eskultet@redhat.com> ...but I'd wait and let others to comment.
Makefile.ci | 1 + 1 file changed, 1 insertion(+)
diff --git a/Makefile.ci b/Makefile.ci index 8857c953b2..977e0445c6 100644 --- a/Makefile.ci +++ b/Makefile.ci @@ -167,6 +167,7 @@ CI_ENGINE_ARGS = \ --volume $(CI_HOST_SRCDIR):$(CI_CONT_SRCDIR):z \ --workdir $(CI_CONT_SRCDIR) \ --ulimit nofile=$(CI_ULIMIT_FILES):$(CI_ULIMIT_FILES) \ + --cap-add=SYS_PTRACE \ $(NULL)
ci-check-engine: -- 2.21.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Hold on to your hat, this is going to be a wild ride. As nearly nothing in glic, nor open() is a real function. Just look into bits/fcntl2.h and you'll see that open() is actually a thin wrapper that calls either __open_alias() or __open_2(). Now, before 801ebb5edb6 the open() done in virPCIDeviceConfigOpenInternal() had a constant oflags (we were opening the pci config with O_RDWR). And since we were not passing any mode nor O_CREAT the wrapper decided to call __open_alias() which was open() provided by our mock. So far so good. But after the referenced commit, the oflags is no longer compile time constant and therefore the wrapper calls __open_2() which we don't mock and thus the real __open_2() from glibc was called and thus we did try to open real path from host's /sys. This of course fails with variety of errors. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcimock.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/virpcimock.c b/tests/virpcimock.c index beb5e1490d..829d61cd3f 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -32,6 +32,7 @@ static int (*real_access)(const char *path, int mode); static int (*real_open)(const char *path, int flags, ...); +static int (*real___open_2)(const char *path, int flags); static int (*real_close)(int fd); static DIR * (*real_opendir)(const char *name); static char *(*real_virFileCanonicalizePath)(const char *path); @@ -805,6 +806,7 @@ init_syms(void) VIR_MOCK_REAL_INIT(access); VIR_MOCK_REAL_INIT(open); + VIR_MOCK_REAL_INIT(__open_2); VIR_MOCK_REAL_INIT(close); VIR_MOCK_REAL_INIT(opendir); VIR_MOCK_REAL_INIT(virFileCanonicalizePath); @@ -932,6 +934,33 @@ open(const char *path, int flags, ...) return ret; } + +int +__open_2(const char *path, int flags) +{ + VIR_AUTOFREE(char *) newpath = NULL; + int ret; + + init_syms(); + + if (STRPREFIX(path, SYSFS_PCI_PREFIX) && + getrealpath(&newpath, path) < 0) + return -1; + + ret = real___open_2(newpath ? newpath : path, flags); + + /* Catch both: /sys/bus/pci/drivers/... and + * /sys/bus/pci/device/.../driver/... */ + if (ret >= 0 && STRPREFIX(path, SYSFS_PCI_PREFIX) && + strstr(path, "driver") && add_fd(ret, path) < 0) { + real_close(ret); + ret = -1; + } + + return ret; +} + + DIR * opendir(const char *path) { -- 2.21.0

On Thu, Aug 15, 2019 at 05:15:57PM +0200, Michal Privoznik wrote:
Hold on to your hat, this is going to be a wild ride. As nearly nothing in glic, nor open() is a real function. Just look into bits/fcntl2.h and you'll see that open() is actually a thin wrapper that calls either __open_alias() or __open_2(). Now, before 801ebb5edb6 the open() done in virPCIDeviceConfigOpenInternal() had a constant oflags (we were opening the pci config with O_RDWR). And since we were not passing any mode nor O_CREAT the wrapper decided to call __open_alias() which was open() provided by our mock. So far so good. But after the referenced commit, the oflags is no longer compile time constant and therefore the wrapper calls __open_2() which we don't mock and thus the real __open_2() from glibc was called and thus we did try to open real path from host's /sys. This of course fails with variety of errors.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
Reviewed-by: Erik Skultety <eskultet@redhat.com>

On Thu, Aug 15, 2019 at 05:15:57PM +0200, Michal Privoznik wrote:
Hold on to your hat, this is going to be a wild ride. As nearly nothing in glic, nor open() is a real function. Just look into
s/glic/glibc/
bits/fcntl2.h and you'll see that open() is actually a thin wrapper that calls either __open_alias() or __open_2(). Now, before 801ebb5edb6 the open() done in virPCIDeviceConfigOpenInternal() had a constant oflags (we were opening the pci config with O_RDWR). And since we were not passing any mode nor O_CREAT the wrapper decided to call __open_alias() which was open() provided by our mock. So far so good. But after the referenced commit, the oflags is no longer compile time constant and therefore the wrapper calls __open_2() which we don't mock and thus the real __open_2() from glibc was called and thus we did try to open real path from host's /sys. This of course fails with variety of errors.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcimock.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
Thanks for tracking this down! Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Erik Skultety
-
Ján Tomko
-
Michal Privoznik