3:04 a.m.
Compiling libvirt on alpine showed a bunch of things that are
potentially wrong, we just haven't noticed elsewhere.
Martin Kletzander (5):
syntax-check: Rework mock-noinline to get all files at once
Include poll.h instead of sys/poll.h
Include sys/wait.h instead of wait.h
nwfilter: Avoid memory alignment issues
tests: Allow expansion of mocked stat symbols
build-aux/syntax-check.mk | 2 +-
scripts/mock-noinline.py | 2 +-
src/lxc/lxc_driver.c | 4 ++--
src/network/bridge_driver.c | 2 +-
src/nwfilter/nwfilter_learnipaddr.c | 15 ++++++++-------
src/openvz/openvz_driver.c | 2 +-
src/qemu/qemu_driver.c | 2 +-
src/security/security_apparmor.c | 2 +-
src/vz/vz_driver.c | 2 +-
tests/virmock.h | 4 +++-
10 files changed, 20 insertions(+), 17 deletions(-)
--
2.35.1
3:04 a.m.
New subject: [libvirt PATCH 1/5] syntax-check: Rework mock-noinline to get all files at once
The script can break if the number of files does not fit one invocation and
xargs has to split it. Instead pipe the list of files directly into the script
and in the script read them from stdin instead of the arguments.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
build-aux/syntax-check.mk | 2 +-
scripts/mock-noinline.py | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 9407581d0eb3..a8c9153b20ae 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -1549,7 +1549,7 @@ sc_spacing-check:
{ echo '$(ME): incorrect formatting' 1>&2; exit 1; }
sc_mock-noinline:
- $(AM_V_GEN)$(VC_LIST_EXCEPT) | $(GREP) '\.[ch]$$' | $(RUNUTF8) xargs \
+ $(AM_V_GEN)$(VC_LIST_EXCEPT) | $(GREP) '\.[ch]$$' | $(RUNUTF8) \
$(PYTHON) $(top_srcdir)/scripts/mock-noinline.py
sc_header-ifdef:
diff --git a/scripts/mock-noinline.py b/scripts/mock-noinline.py
index 712550cb7a4c..13c296603d3a 100644
--- a/scripts/mock-noinline.py
+++ b/scripts/mock-noinline.py
@@ -63,7 +63,7 @@ def scan_overrides(filename):
mocked[name] = "%s:%d" % (filename, lineno)
-for filename in sys.argv[1:]:
+for filename in sys.stdin.readlines():
if filename.endswith(".h"):
scan_annotations(filename)
elif filename.endswith("mock.c"):
--
2.35.1
3:04 a.m.
New subject: [libvirt PATCH 2/5] Include poll.h instead of sys/poll.h
That is the proper POSIX way.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/lxc/lxc_driver.c | 2 +-
src/network/bridge_driver.c | 2 +-
src/openvz/openvz_driver.c | 2 +-
src/qemu/qemu_driver.c | 2 +-
src/vz/vz_driver.c | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 020ec257aef1..8c041540ab47 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -30,7 +30,7 @@
#endif
#include <sys/stat.h>
-#include <sys/poll.h>
+#include <poll.h>
#include <unistd.h>
#include <wait.h>
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index d6ae05360b24..67c5a111b350 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -22,7 +22,7 @@
#include <config.h>
#include <sys/types.h>
-#include <sys/poll.h>
+#include <poll.h>
#include <stdarg.h>
#include <unistd.h>
#include <sys/utsname.h>
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index aa1db095400b..53214586a550 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -24,7 +24,7 @@
#include <config.h>
#include <sys/types.h>
-#include <sys/poll.h>
+#include <poll.h>
#include <stdarg.h>
#include <unistd.h>
#include <sys/stat.h>
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8337eed510d8..812a900fa8b5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -22,7 +22,7 @@
#include <config.h>
#include <sys/types.h>
-#include <sys/poll.h>
+#include <poll.h>
#include <sys/time.h>
#include <dirent.h>
#include <stdarg.h>
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index fc91b6dddf45..fdd776282e82 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -24,7 +24,7 @@
#include <config.h>
#include <sys/types.h>
-#include <sys/poll.h>
+#include <poll.h>
#include <stdarg.h>
#include <unistd.h>
#include <sys/stat.h>
--
2.35.1
7:43 a.m.
New subject: [libvirt PATCH 2/5] Include poll.h instead of sys/poll.h
On Mon, Mar 07, 2022 at 10:04:03AM +0100, Martin Kletzander wrote:
That is the proper POSIX way.
Indeed, but if someone wants to do more cleanup we should just
stop using poll, and call g_poll instead. This is a transparent
wrapper of poll on systems which have it, or a compatible impl
on systems lacking it. With g_poll we don't need to care about
poll.h at all.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
3:04 a.m.
New subject: [libvirt PATCH 3/5] Include sys/wait.h instead of wait.h
That is the proper POSIX way.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/lxc/lxc_driver.c | 2 +-
src/security/security_apparmor.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 8c041540ab47..84d1f3935863 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -32,7 +32,7 @@
#include <sys/stat.h>
#include <poll.h>
#include <unistd.h>
-#include <wait.h>
+#include <sys/wait.h>
#include "virerror.h"
#include "virlog.h"
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index d1087aa10cf8..8f7acba9800c 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -26,7 +26,7 @@
#include <fcntl.h>
#include <sys/apparmor.h>
#include <unistd.h>
-#include <wait.h>
+#include <sys/wait.h>
#include "internal.h"
--
2.35.1
3:04 a.m.
New subject: [libvirt PATCH 4/5] nwfilter: Avoid memory alignment issues
The returned packet can have less strict alignment (u_char) than the struct
(ether_header) we are casting it to, so to avoid alignment issues just copy the
header into the struct on the stack.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/nwfilter/nwfilter_learnipaddr.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c
index fb552bd1e60e..99bffdc4fbdb 100644
--- a/src/nwfilter/nwfilter_learnipaddr.c
+++ b/src/nwfilter/nwfilter_learnipaddr.c
@@ -384,7 +384,7 @@ learnIPAddressThread(void *arg)
struct bpf_program fp;
struct pcap_pkthdr header;
const u_char *packet;
- struct ether_header *ether_hdr;
+ struct ether_header ether_hdr;
struct ether_vlan_header *vlan_hdr;
virNWFilterIPAddrLearnReq *req = arg;
uint32_t vmaddr = 0, bcastaddr = 0;
@@ -506,13 +506,14 @@ learnIPAddressThread(void *arg)
}
if (header.len >= sizeof(struct ether_header)) {
- ether_hdr = (struct ether_header*)packet;
+ /* Avoid alignment issues */
+ memcpy(ðer_hdr, packet, sizeof(struct ether_header));
- switch (ntohs(ether_hdr->ether_type)) {
+ switch (ntohs(ether_hdr.ether_type)) {
case ETHERTYPE_IP:
ethHdrSize = sizeof(struct ether_header);
- etherType = ntohs(ether_hdr->ether_type);
+ etherType = ntohs(ether_hdr.ether_type);
break;
case ETHERTYPE_VLAN:
@@ -528,7 +529,7 @@ learnIPAddressThread(void *arg)
continue;
}
- if (virMacAddrCmpRaw(&req->binding->mac, ether_hdr->ether_shost)
== 0) {
+ if (virMacAddrCmpRaw(&req->binding->mac, ether_hdr.ether_shost) ==
0) {
/* packets from the VM */
if (etherType == ETHERTYPE_IP &&
@@ -568,9 +569,9 @@ learnIPAddressThread(void *arg)
}
}
} else if (virMacAddrCmpRaw(&req->binding->mac,
- ether_hdr->ether_dhost) == 0 ||
+ ether_hdr.ether_dhost) == 0 ||
/* allow Broadcast replies from DHCP server */
- virMacAddrIsBroadcastRaw(ether_hdr->ether_dhost)) {
+ virMacAddrIsBroadcastRaw(ether_hdr.ether_dhost)) {
/* packets to the VM */
if (etherType == ETHERTYPE_IP &&
(header.len >= ethHdrSize +
--
2.35.1
3:04 a.m.
New subject: [libvirt PATCH 5/5] tests: Allow expansion of mocked stat symbols
When libc uses a define to rewrite stat64 to stat our mocks do not work if they
are chained because the symbol that we are looking up is being stringified and
therefore preventing the stat64->stat expansion per C-preprocessor rules. One
stringification macro is just enough to make it work.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
tests/virmock.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tests/virmock.h b/tests/virmock.h
index 5250e733a156..300ba17174d4 100644
--- a/tests/virmock.h
+++ b/tests/virmock.h
@@ -296,12 +296,14 @@
do {} while (0)
#endif
+#define VIR_MOCK_STRINGIFY_SYMBOL(name) #name
+
#define VIR_MOCK_REAL_INIT(name) \
do { \
VIR_MOCK_REAL_INIT_MAIN(name, #name); \
if (real_##name == NULL && \
!(real_##name = dlsym(RTLD_NEXT, \
- #name))) { \
+ VIR_MOCK_STRINGIFY_SYMBOL(name)))) { \
fprintf(stderr, "Missing symbol '" #name "'\n");
\
abort(); \
} \
--
2.35.1
4:01 a.m.
New subject: [libvirt PATCH 5/5] tests: Allow expansion of mocked stat symbols
On Mon, Mar 07, 2022 at 10:04:06AM +0100, Martin Kletzander wrote:
When libc uses a define to rewrite stat64 to stat our mocks do not
work if they
are chained because the symbol that we are looking up is being stringified and
therefore preventing the stat64->stat expansion per C-preprocessor rules. One
stringification macro is just enough to make it work.
This doesn't sound right to me.
If we're implementing a mock for 'stat64', we should not be looking
up 'stat'. We should be implmenting a mock for 'stat' instead.
It sounds more like we got MOCK_STAT and MOCK_STAT64 incorrectly
defined surely.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
4:43 a.m.
New subject: [libvirt PATCH 5/5] tests: Allow expansion of mocked stat symbols
On Mon, Mar 07, 2022 at 10:01:40AM +0000, Daniel P. Berrangé wrote:
On Mon, Mar 07, 2022 at 10:04:06AM +0100, Martin Kletzander wrote:
> When libc uses a define to rewrite stat64 to stat our mocks do not work if they
> are chained because the symbol that we are looking up is being stringified and
> therefore preventing the stat64->stat expansion per C-preprocessor rules. One
> stringification macro is just enough to make it work.
This doesn't sound right to me.
If we're implementing a mock for 'stat64', we should not be looking
up 'stat'. We should be implmenting a mock for 'stat' instead.
It sounds more like we got MOCK_STAT and MOCK_STAT64 incorrectly
defined surely.
I spent three days trying to figure this out. But with this fix we are
actually doing what you are suggesting. All the (l)stat64 are being
replaced by (l)stat, so even:
int lstat64(const char *path, struct stat64 *statbuf)
is actually rewritten to
int lstat(const char *path, struct stat *statbuf)
because the define applies to the struct name as well. The only
difference is that internally it will use the real_lstat64 function
pointer, but the function declaration is correct. And if you enable
debugging it will say that lstat64 is being redirected, but that's a
pretty minor issue, if it is one at all.
What we could do is rewrite the macros so that all of the above is
properly expanded/substituted, but I do not think that is the way we
want to do things as it looked a bit messy (I probably still have that
commit somewhere.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
4:44 a.m.
New subject: [libvirt PATCH 5/5] tests: Allow expansion of mocked stat symbols
On Mon, Mar 07, 2022 at 11:43:05AM +0100, Martin Kletzander wrote:
On Mon, Mar 07, 2022 at 10:01:40AM +0000, Daniel P. Berrangé wrote:
> On Mon, Mar 07, 2022 at 10:04:06AM +0100, Martin Kletzander wrote:
> > When libc uses a define to rewrite stat64 to stat our mocks do not work if
they
> > are chained because the symbol that we are looking up is being stringified and
> > therefore preventing the stat64->stat expansion per C-preprocessor rules.
One
> > stringification macro is just enough to make it work.
>
> This doesn't sound right to me.
>
> If we're implementing a mock for 'stat64', we should not be looking
> up 'stat'. We should be implmenting a mock for 'stat' instead.
>
> It sounds more like we got MOCK_STAT and MOCK_STAT64 incorrectly
> defined surely.
>
I spent three days trying to figure this out. But with this fix we are
actually doing what you are suggesting. All the (l)stat64 are being
replaced by (l)stat, so even:
int lstat64(const char *path, struct stat64 *statbuf)
is actually rewritten to
int lstat(const char *path, struct stat *statbuf)
because the define applies to the struct name as well. The only
difference is that internally it will use the real_lstat64 function
pointer, but the function declaration is correct. And if you enable
debugging it will say that lstat64 is being redirected, but that's a
pretty minor issue, if it is one at all.
What we could do is rewrite the macros so that all of the above is
properly expanded/substituted, but I do not think that is the way we
want to do things as it looked a bit messy (I probably still have that
commit somewhere.
Ok, thanks for the explanation.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
3:27 a.m.
On 3/7/22 10:04, Martin Kletzander wrote:
Compiling libvirt on alpine showed a bunch of things that are
potentially wrong, we just haven't noticed elsewhere.
Martin Kletzander (5):
syntax-check: Rework mock-noinline to get all files at once
Include poll.h instead of sys/poll.h
Include sys/wait.h instead of wait.h
nwfilter: Avoid memory alignment issues
tests: Allow expansion of mocked stat symbols
build-aux/syntax-check.mk | 2 +-
scripts/mock-noinline.py | 2 +-
src/lxc/lxc_driver.c | 4 ++--
src/network/bridge_driver.c | 2 +-
src/nwfilter/nwfilter_learnipaddr.c | 15 ++++++++-------
src/openvz/openvz_driver.c | 2 +-
src/qemu/qemu_driver.c | 2 +-
src/security/security_apparmor.c | 2 +-
src/vz/vz_driver.c | 2 +-
tests/virmock.h | 4 +++-
10 files changed, 20 insertions(+), 17 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Apparently, I don't know how C preprocessor works.
Michal
3:28 a.m.
On a Monday in 2022, Martin Kletzander wrote:
Compiling libvirt on alpine showed a bunch of things that are
potentially wrong, we just haven't noticed elsewhere.
Martin Kletzander (5):
syntax-check: Rework mock-noinline to get all files at once
Include poll.h instead of sys/poll.h
Include sys/wait.h instead of wait.h
nwfilter: Avoid memory alignment issues
tests: Allow expansion of mocked stat symbols
build-aux/syntax-check.mk | 2 +-
scripts/mock-noinline.py | 2 +-
src/lxc/lxc_driver.c | 4 ++--
src/network/bridge_driver.c | 2 +-
src/nwfilter/nwfilter_learnipaddr.c | 15 ++++++++-------
src/openvz/openvz_driver.c | 2 +-
src/qemu/qemu_driver.c | 2 +-
src/security/security_apparmor.c | 2 +-
src/vz/vz_driver.c | 2 +-
tests/virmock.h | 4 +++-
10 files changed, 20 insertions(+), 17 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1024
days inactive
1024
days old
11 comments
4 participants
participants (4)
-
Daniel P. Berrangé -
Ján Tomko -
Martin Kletzander -
Michal Prívozník