[libvirt] [PATCH 0/3] Fix examples/ under mingw
My recent patch to make all of the examples work independently of gnulib worked fine on Linux, but failed on mingw: https://travis-ci.org/libvirt/libvirt/jobs/476898635 The whole point of gnulib is to work around portability pitfalls so we don't have to bend over backwards thinking about it in our code; and it would be possible to fix this bug by linking the problematic example binaries against gnulib after all. But since the examples are still small and self-contained enough that using manual workarounds wasn't too daunting, that's the approach I took here. I'm pushing this series as a build-breaker fix in a couple of hours, or sooner if I get a review. Eric Blake (3): examples: Work around mingw printf() weakness examples: Work around lack of mingw sigaction() examples: Work around lack of mingw localtime_r() cfg.mk | 2 +- examples/admin/client_info.c | 6 +++++- examples/admin/list_clients.c | 6 +++++- examples/domtop/domtop.c | 13 ++++++------- examples/object-events/event-test.c | 16 +++++++--------- 5 files changed, 24 insertions(+), 19 deletions(-) -- 2.20.1
mingw lacks %lld and %zu support in printf(); we were getting it from gnulib. But since commit acf522e8 stopped linking examples against gnulib, we are getting a build failure due to -Wformat flagging these strings. Keep the examples standalone, and work around mingw by using manual casts to types we can portably print. Signed-off-by: Eric Blake <eblake@redhat.com> --- examples/domtop/domtop.c | 3 ++- examples/object-events/event-test.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/examples/domtop/domtop.c b/examples/domtop/domtop.c index 4224e4c9ec..ada5a064fb 100644 --- a/examples/domtop/domtop.c +++ b/examples/domtop/domtop.c @@ -241,7 +241,8 @@ print_cpu_usage(const char *dom_name, if (delim) printf("\t"); - printf("CPU%zu: %.2lf", cpu + i, usage); + /* mingw lacks %zu */ + printf("CPU%u: %.2lf", (unsigned)(cpu + i), usage); delim = true; } diff --git a/examples/object-events/event-test.c b/examples/object-events/event-test.c index 53d95b10d0..0c99fb33e3 100644 --- a/examples/object-events/event-test.c +++ b/examples/object-events/event-test.c @@ -948,10 +948,11 @@ myDomainEventBlockThresholdCallback(virConnectPtr conn ATTRIBUTE_UNUSED, unsigned long long excess, void *opaque ATTRIBUTE_UNUSED) { + /* Casts to uint64_t to work around mingw not knowing %lld */ printf("%s EVENT: Domain %s(%d) block threshold callback dev '%s'(%s), " - "threshold: '%llu', excess: '%llu'", + "threshold: '%" PRIu64 "', excess: '%" PRIu64 "'", __func__, virDomainGetName(dom), virDomainGetID(dom), - dev, NULLSTR(path), threshold, excess); + dev, NULLSTR(path), (uint64_t)threshold, (uint64_t)excess); return 0; } -- 2.20.1
mingw lacks sigaction(); we were getting it from gnulib. But since commit acf522e8 stopped linking examples against gnulib, we are getting a build failure. Keep the examples standalone, and work around mingw by using signal() instead. Signed-off-by: Eric Blake <eblake@redhat.com> --- examples/domtop/domtop.c | 10 ++++------ examples/object-events/event-test.c | 11 ++++------- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/examples/domtop/domtop.c b/examples/domtop/domtop.c index ada5a064fb..008065c651 100644 --- a/examples/domtop/domtop.c +++ b/examples/domtop/domtop.c @@ -266,10 +266,6 @@ do_top(virConnectPtr conn, int max_id = 0; int nparams = 0, then_nparams = 0, now_nparams = 0; virTypedParameterPtr then_params = NULL, now_params = NULL; - struct sigaction action_stop; - - memset(&action_stop, 0, sizeof(action_stop)); - action_stop.sa_handler = stop; /* Lookup the domain */ if (!(dom = virDomainLookupByName(conn, dom_name))) { @@ -295,8 +291,10 @@ do_top(virConnectPtr conn, goto cleanup; } - sigaction(SIGTERM, &action_stop, NULL); - sigaction(SIGINT, &action_stop, NULL); + /* The ideal program would use sigaction to set this handler, but + * this way is portable to mingw. */ + signal(SIGTERM, stop); + signal(SIGINT, stop); run_top = true; while (run_top) { diff --git a/examples/object-events/event-test.c b/examples/object-events/event-test.c index 0c99fb33e3..fcf4492470 100644 --- a/examples/object-events/event-test.c +++ b/examples/object-events/event-test.c @@ -1147,13 +1147,8 @@ main(int argc, char **argv) virConnectPtr dconn = NULL; int callback1ret = -1; int callback16ret = -1; - struct sigaction action_stop; size_t i; - memset(&action_stop, 0, sizeof(action_stop)); - - action_stop.sa_handler = stop; - if (argc > 1 && STREQ(argv[1], "--help")) { printf("%s uri\n", argv[0]); goto cleanup; @@ -1184,8 +1179,10 @@ main(int argc, char **argv) goto cleanup; } - sigaction(SIGTERM, &action_stop, NULL); - sigaction(SIGINT, &action_stop, NULL); + /* The ideal program would use sigaction to set this handler, but + * this way is portable to mingw. */ + signal(SIGTERM, stop); + signal(SIGINT, stop); printf("Registering event callbacks\n"); -- 2.20.1
mingw lacks localtime_r(); we were getting it from gnulib. But since commit acf522e8 stopped linking examples against gnulib, we are getting a build failure. Keep the examples standalone, and work around mingw by using the non-reentrant localtime() (safe since our examples are single-threaded), and add a necessary exemption to our syntax check. Signed-off-by: Eric Blake <eblake@redhat.com> --- cfg.mk | 2 +- examples/admin/client_info.c | 6 +++++- examples/admin/list_clients.c | 6 +++++- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/cfg.mk b/cfg.mk index d9a8a44dc4..a75bfef86e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1233,7 +1233,7 @@ exclude_file_name_regexp--sc_prohibit_newline_at_end_of_diagnostic = \ ^src/rpc/gendispatch\.pl$$ exclude_file_name_regexp--sc_prohibit_nonreentrant = \ - ^((po|tests)/|docs/.*(py|js|html\.in)|run.in$$|tools/wireshark/util/genxdrstub\.pl$$) + ^((po|tests|examples/admin)/|docs/.*(py|js|html\.in)|run.in$$|tools/wireshark/util/genxdrstub\.pl$$) exclude_file_name_regexp--sc_prohibit_select = \ ^cfg\.mk$$ diff --git a/examples/admin/client_info.c b/examples/admin/client_info.c index 2c46ccf514..f3f62a656b 100644 --- a/examples/admin/client_info.c +++ b/examples/admin/client_info.c @@ -30,9 +30,13 @@ exampleGetTimeStr(time_t then) { char *ret = NULL; struct tm timeinfo; + struct tm *timeinfop; - if (!localtime_r(&then, &timeinfo)) + /* localtime_r() is smarter, but since mingw lacks it and this + * example is single-threaded, we can get away with localtime */ + if (!(timeinfop = localtime(&then))) return NULL; + timeinfo = *timeinfop; if (!(ret = calloc(64, sizeof(char)))) return NULL; diff --git a/examples/admin/list_clients.c b/examples/admin/list_clients.c index 15205a7bd1..5cf8e4c2a6 100644 --- a/examples/admin/list_clients.c +++ b/examples/admin/list_clients.c @@ -28,9 +28,13 @@ exampleGetTimeStr(time_t then) { char *ret = NULL; struct tm timeinfo; + struct tm *timeinfop; - if (!localtime_r(&then, &timeinfo)) + /* localtime_r() is smarter, but since mingw lacks it and this + * example is single-threaded, we can get away with localtime */ + if (!(timeinfop = localtime(&then))) return NULL; + timeinfo = *timeinfop; if (!(ret = calloc(64, sizeof(char)))) return NULL; -- 2.20.1
On 1/8/19 2:56 PM, Eric Blake wrote:
My recent patch to make all of the examples work independently of gnulib worked fine on Linux, but failed on mingw: https://travis-ci.org/libvirt/libvirt/jobs/476898635
The whole point of gnulib is to work around portability pitfalls so we don't have to bend over backwards thinking about it in our code; and it would be possible to fix this bug by linking the problematic example binaries against gnulib after all. But since the examples are still small and self-contained enough that using manual workarounds wasn't too daunting, that's the approach I took here.
I'm pushing this series as a build-breaker fix in a couple of hours, or sooner if I get a review.
Eric Blake (3): examples: Work around mingw printf() weakness examples: Work around lack of mingw sigaction() examples: Work around lack of mingw localtime_r()
cfg.mk | 2 +- examples/admin/client_info.c | 6 +++++- examples/admin/list_clients.c | 6 +++++- examples/domtop/domtop.c | 13 ++++++------- examples/object-events/event-test.c | 16 +++++++--------- 5 files changed, 24 insertions(+), 19 deletions(-)
Everything seems reasonable to me... Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Tue, 2019-01-08 at 13:56 -0600, Eric Blake wrote:
My recent patch to make all of the examples work independently of gnulib worked fine on Linux, but failed on mingw: https://travis-ci.org/libvirt/libvirt/jobs/476898635
The whole point of gnulib is to work around portability pitfalls so we don't have to bend over backwards thinking about it in our code; and it would be possible to fix this bug by linking the problematic example binaries against gnulib after all. But since the examples are still small and self-contained enough that using manual workarounds wasn't too daunting, that's the approach I took here.
It's kind of a done deal now, but I still wonder if it was the right approach. The way I see it, our examples are supposed to illustrate how to use libvirt itself, not how to write C code that is portable to a multitude of platforms: with that goal in mind, taking advantage of gnulib makes perfect sense, as it allows us to put the focus on the usage of libvirt rather than the surrounding compatibility gunk. I would probably have a different opinion about this if the workarounds were related to eg. using certain types as opposed to others when passing data to libvirt functions, but as far as I can tell that's not the case. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Jan 09, 2019 at 01:33:44PM +0100, Andrea Bolognani wrote:
On Tue, 2019-01-08 at 13:56 -0600, Eric Blake wrote:
My recent patch to make all of the examples work independently of gnulib worked fine on Linux, but failed on mingw: https://travis-ci.org/libvirt/libvirt/jobs/476898635
The whole point of gnulib is to work around portability pitfalls so we don't have to bend over backwards thinking about it in our code; and it would be possible to fix this bug by linking the problematic example binaries against gnulib after all. But since the examples are still small and self-contained enough that using manual workarounds wasn't too daunting, that's the approach I took here.
It's kind of a done deal now, but I still wonder if it was the right approach.
The way I see it, our examples are supposed to illustrate how to use libvirt itself, not how to write C code that is portable to a multitude of platforms: with that goal in mind, taking advantage of gnulib makes perfect sense, as it allows us to put the focus on the usage of libvirt rather than the surrounding compatibility gunk.
Using gnulib is not an easy thing if you are not familiar with it, largely because of the painful autotools integration it imposes. It is important that the examples are both simple to read, and simple to build standalone. We've always considered that the examples should be possible to build using nothing more than the C compiler and pkg-config. eg $CC `pkg-config --cflags --libs libvirt` -o foo foo.c The benefits of gnulib are not compelling enough to be worth the complexity that it brings in for the examples. 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 :|
On Wed, 2019-01-09 at 12:53 +0000, Daniel P. Berrangé wrote:
On Wed, Jan 09, 2019 at 01:33:44PM +0100, Andrea Bolognani wrote:
The way I see it, our examples are supposed to illustrate how to use libvirt itself, not how to write C code that is portable to a multitude of platforms: with that goal in mind, taking advantage of gnulib makes perfect sense, as it allows us to put the focus on the usage of libvirt rather than the surrounding compatibility gunk.
Using gnulib is not an easy thing if you are not familiar with it, largely because of the painful autotools integration it imposes. It is important that the examples are both simple to read, and simple to build standalone. We've always considered that the examples should be possible to build using nothing more than the C compiler and pkg-config. eg
$CC `pkg-config --cflags --libs libvirt` -o foo foo.c
The benefits of gnulib are not compelling enough to be worth the complexity that it brings in for the examples.
I would see a point in making them buildable in a standalone fashion if 1) we installed them, eg. under /usr/share/doc/libvirt/examples 2) we also installed, for each of them, a plain Makefile that calls $CC and pkg-config as seen above Until that's the case, the gnulib dependency makes complete sense to me, because you're not going to install libvirt using your package manager, then clone the git repository and figure out yourself how to compile the examples, just to poke around a bit. Anyway, I've noticed that the apparmor/ directory contains files that we end up installing on the system in non-documentation directories, which feels all kinds of wrong. Am I missing something, or should we move that stuff somewhere else? -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Jan 09, 2019 at 03:46:19PM +0100, Andrea Bolognani wrote:
On Wed, 2019-01-09 at 12:53 +0000, Daniel P. Berrangé wrote:
On Wed, Jan 09, 2019 at 01:33:44PM +0100, Andrea Bolognani wrote:
The way I see it, our examples are supposed to illustrate how to use libvirt itself, not how to write C code that is portable to a multitude of platforms: with that goal in mind, taking advantage of gnulib makes perfect sense, as it allows us to put the focus on the usage of libvirt rather than the surrounding compatibility gunk.
Using gnulib is not an easy thing if you are not familiar with it, largely because of the painful autotools integration it imposes. It is important that the examples are both simple to read, and simple to build standalone. We've always considered that the examples should be possible to build using nothing more than the C compiler and pkg-config. eg
$CC `pkg-config --cflags --libs libvirt` -o foo foo.c
The benefits of gnulib are not compelling enough to be worth the complexity that it brings in for the examples.
I would see a point in making them buildable in a standalone fashion if
1) we installed them, eg. under /usr/share/doc/libvirt/examples 2) we also installed, for each of them, a plain Makefile that calls $CC and pkg-config as seen above
Until that's the case, the gnulib dependency makes complete sense to me, because you're not going to install libvirt using your package manager, then clone the git repository and figure out yourself how to compile the examples, just to poke around a bit.
I don't really agree. There's no need to clone the git repo to use the examples. I've given people direct links to the gitweb viewer for example programs and just told them to build using the $CC arg above. Sure we could install them & provide a plain Makefile, but that's tangential to use of gnulib IMHO.
Anyway, I've noticed that the apparmor/ directory contains files that we end up installing on the system in non-documentation directories, which feels all kinds of wrong. Am I missing something, or should we move that stuff somewhere else?
Yes, those look like they should be in src/security instead. 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 :|
On Wed, 2019-01-09 at 14:54 +0000, Daniel P. Berrangé wrote: [...]
I don't really agree. There's no need to clone the git repo to use the examples. I've given people direct links to the gitweb viewer for example programs and just told them to build using the $CC arg above. Sure we could install them & provide a plain Makefile, but that's tangential to use of gnulib IMHO.
Okay, fair enough.
Anyway, I've noticed that the apparmor/ directory contains files that we end up installing on the system in non-documentation directories, which feels all kinds of wrong. Am I missing something, or should we move that stuff somewhere else?
Yes, those look like they should be in src/security instead.
I'm cooking a patch, I'll post it in a while. -- Andrea Bolognani / Red Hat / Virtualization
On 1/9/19 7:33 AM, Andrea Bolognani wrote:
On Tue, 2019-01-08 at 13:56 -0600, Eric Blake wrote:
My recent patch to make all of the examples work independently of gnulib worked fine on Linux, but failed on mingw: https://travis-ci.org/libvirt/libvirt/jobs/476898635
The whole point of gnulib is to work around portability pitfalls so we don't have to bend over backwards thinking about it in our code; and it would be possible to fix this bug by linking the problematic example binaries against gnulib after all. But since the examples are still small and self-contained enough that using manual workarounds wasn't too daunting, that's the approach I took here.
It's kind of a done deal now, but I still wonder if it was the right approach.
The way I see it, our examples are supposed to illustrate how to use libvirt itself, not how to write C code that is portable to a multitude of platforms: with that goal in mind, taking advantage of gnulib makes perfect sense, as it allows us to put the focus on the usage of libvirt rather than the surrounding compatibility gunk.
Not everyone that would write to libvirt API's would have installed gnulib. Asked differently, is having gnulib installed a prerequisite for having libvirt installed? What about libvirt-devel? While altering these examples to adhere to some least common denominator architecture is perhaps less optimal - it does show one can generate architecture agnostic examples as long as they understand issues surrounding portability. An alternative one could generate patches for would be #ifdef'd code or perhaps even simply comments indicating the alternative methodology that is less portable. For those without git history it's perhaps harder to figure out. John
I would probably have a different opinion about this if the workarounds were related to eg. using certain types as opposed to others when passing data to libvirt functions, but as far as I can tell that's not the case.
participants (4)
-
Andrea Bolognani -
Daniel P. Berrangé -
Eric Blake -
John Ferlan