[PATCH 00/12] kvmtool: Improve portability

[PATCH 00/12] kvmtool: Improve portability

[Andre Przywara]

Hi,

this is a collection of patches to bring kvmtool closer to standards
compliance (with standards not necessarily meaning GNU only).
With all those patches applied, you can compile kvmtool with newer
C standards, with clang and against musl-libc.

The first patch was already on the list, it allows compiling with
-std=gnu99 (or gnu11, even). As this will become the new GCC default 
at some point, that sounds useful.

Patch 2-7 fix compilation with clang: some are real bugs, some are
just things that clang is pickier about. Please note that the BIOS
code for x86 still does not compile with clang. I have the gut
feeling that rewriting this is assembly would be cleaner than all
those hacks we do to make this possible in C, but I will look at this
later.

The next four patches fix compilation against musl-libc. No real bugs
here, but again improves code quality. Compiling against musl-libc
gives you really small binaries (174K for ARM, for instance).

The last patch just follows a comment in the code to remove a kludge.

Cheers,
Andre.

Andre Przywara (12):
  avoid casts when initializing structures
  qcow: fix signedness bugs
  kvm-ipc: use proper type for file descriptor
  Makefile: remove unneeded -s switch on compiling BIOS files
  ui: remove pointless double const in keymap declarations
  kvm__set_dir(): avoid variable arguments call
  util/util.c: avoid clang error on vsnprintf
  Fix call to connect()
  use <poll.h> instead of <sys/poll.h>
  check for and use C library provided strlcpy and strlcat
  avoid using predefined PAGE_SIZE
  remove KVM_CAP_MAX_VCPUS hack

 Makefile                 | 15 ++++++++++-----
 config/feature-tests.mak | 10 ++++++++++
 disk/core.c              |  2 +-
 disk/qcow.c              | 14 +++++++-------
 include/kvm/kvm.h        |  6 +++++-
 include/kvm/mutex.h      |  2 +-
 include/kvm/strbuf.h     |  2 ++
 include/linux/rbtree.h   |  2 +-
 kvm-ipc.c                |  4 ++--
 kvm.c                    | 29 ++++++-----------------------
 main.c                   |  9 ++++++++-
 ui/gtk3.c                |  2 +-
 ui/sdl.c                 |  2 +-
 util/strbuf.c            |  2 ++
 util/util.c              |  1 +
 virtio/9p.c              |  2 +-
 virtio/balloon.c         |  2 +-
 virtio/blk.c             |  2 +-
 virtio/console.c         |  2 +-
 virtio/net.c             |  2 +-
 virtio/rng.c             |  2 +-
 virtio/scsi.c            |  2 +-
 22 files changed, 65 insertions(+), 51 deletions(-)

-- 
2.3.5

[PATCH 01/12] avoid casts when initializing structures

[Andre Przywara]

Due to our kernel heritage we have code in kvmtool that relies on
the (still) implicit -std=gnu89 compiler switch.
It turns out that this just affects some structure initialization,
where we currently provide a cast to the type, which upsets GCC for
anything beyond -std=gnu89 (for instance gnu99 or gnu11).
We do need the casts when initializing structures that are not
assigned to the same type, so we put it there explicitly.

This allows us to compile with all the three GNU standards GCC
currently supports: gnu89/90, gnu99 and gnu11.
GCC threatens people with moving to gnu11 as the new default standard,
so lets fix this better sooner than later.
(Compiling without GNU extensions still breaks and I don't bother to
fix that without very good reasons.)

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 disk/qcow.c            | 6 +++---
 include/kvm/mutex.h    | 2 +-
 include/linux/rbtree.h | 2 +-
 virtio/9p.c            | 2 +-
 virtio/balloon.c       | 2 +-
 virtio/blk.c           | 2 +-
 virtio/console.c       | 2 +-
 virtio/net.c           | 2 +-
 virtio/rng.c           | 2 +-
 virtio/scsi.c          | 2 +-
 10 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/disk/qcow.c b/disk/qcow.c
index 64a2550..e26c419 100644
--- a/disk/qcow.c
+++ b/disk/qcow.c
@@ -1203,7 +1203,7 @@ static int qcow_read_refcount_table(struct qcow *q)
 	if (!rft->rf_table)
 		return -1;
 
-	rft->root = RB_ROOT;
+	rft->root = (struct rb_root) RB_ROOT;
 	INIT_LIST_HEAD(&rft->lru_list);
 
 	return pread_in_full(q->fd, rft->rf_table, sizeof(u64) * rft->rf_size, header->refcount_table_offset);
@@ -1289,7 +1289,7 @@ static struct disk_image *qcow2_probe(int fd, bool readonly)
 
 	l1t = &q->table;
 
-	l1t->root = RB_ROOT;
+	l1t->root = (struct rb_root) RB_ROOT;
 	INIT_LIST_HEAD(&l1t->lru_list);
 
 	h = q->header = qcow2_read_header(fd);
@@ -1435,7 +1435,7 @@ static struct disk_image *qcow1_probe(int fd, bool readonly)
 
 	l1t = &q->table;
 
-	l1t->root = RB_ROOT;
+	l1t->root = (struct rb_root)RB_ROOT;
 	INIT_LIST_HEAD(&l1t->lru_list);
 
 	h = q->header = qcow1_read_header(fd);
diff --git a/include/kvm/mutex.h b/include/kvm/mutex.h
index a90584b..1f7d0f6 100644
--- a/include/kvm/mutex.h
+++ b/include/kvm/mutex.h
@@ -13,7 +13,7 @@
 struct mutex {
 	pthread_mutex_t mutex;
 };
-#define MUTEX_INITIALIZER (struct mutex) { .mutex = PTHREAD_MUTEX_INITIALIZER }
+#define MUTEX_INITIALIZER { .mutex = PTHREAD_MUTEX_INITIALIZER }
 
 #define DEFINE_MUTEX(mtx) struct mutex mtx = MUTEX_INITIALIZER
 
diff --git a/include/linux/rbtree.h b/include/linux/rbtree.h
index fb31765..33adf78 100644
--- a/include/linux/rbtree.h
+++ b/include/linux/rbtree.h
@@ -46,7 +46,7 @@ struct rb_root {
 
 #define rb_parent(r)   ((struct rb_node *)((r)->__rb_parent_color & ~3))
 
-#define RB_ROOT	(struct rb_root) { NULL, }
+#define RB_ROOT	{ NULL, }
 #define	rb_entry(ptr, type, member) container_of(ptr, type, member)
 
 #define RB_EMPTY_ROOT(root)  ((root)->rb_node == NULL)
diff --git a/virtio/9p.c b/virtio/9p.c
index 66dcc26..49e7c5c 100644
--- a/virtio/9p.c
+++ b/virtio/9p.c
@@ -1320,7 +1320,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size)
 	return size;
 }
 
-struct virtio_ops p9_dev_virtio_ops = (struct virtio_ops) {
+struct virtio_ops p9_dev_virtio_ops = {
 	.get_config		= get_config,
 	.get_host_features	= get_host_features,
 	.set_guest_features	= set_guest_features,
diff --git a/virtio/balloon.c b/virtio/balloon.c
index 84c4bb0..9564aa3 100644
--- a/virtio/balloon.c
+++ b/virtio/balloon.c
@@ -239,7 +239,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size)
 	return size;
 }
 
-struct virtio_ops bln_dev_virtio_ops = (struct virtio_ops) {
+struct virtio_ops bln_dev_virtio_ops = {
 	.get_config		= get_config,
 	.get_host_features	= get_host_features,
 	.set_guest_features	= set_guest_features,
diff --git a/virtio/blk.c b/virtio/blk.c
index edfa8e6..c485e4f 100644
--- a/virtio/blk.c
+++ b/virtio/blk.c
@@ -244,7 +244,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size)
 	return size;
 }
 
-static struct virtio_ops blk_dev_virtio_ops = (struct virtio_ops) {
+static struct virtio_ops blk_dev_virtio_ops = {
 	.get_config		= get_config,
 	.get_host_features	= get_host_features,
 	.set_guest_features	= set_guest_features,
diff --git a/virtio/console.c b/virtio/console.c
index 384eac1..f1c0a19 100644
--- a/virtio/console.c
+++ b/virtio/console.c
@@ -197,7 +197,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size)
 	return size;
 }
 
-static struct virtio_ops con_dev_virtio_ops = (struct virtio_ops) {
+static struct virtio_ops con_dev_virtio_ops = {
 	.get_config		= get_config,
 	.get_host_features	= get_host_features,
 	.set_guest_features	= set_guest_features,
diff --git a/virtio/net.c b/virtio/net.c
index 9784520..4a6a855 100644
--- a/virtio/net.c
+++ b/virtio/net.c
@@ -624,7 +624,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size)
 	return size;
 }
 
-static struct virtio_ops net_dev_virtio_ops = (struct virtio_ops) {
+static struct virtio_ops net_dev_virtio_ops = {
 	.get_config		= get_config,
 	.get_host_features	= get_host_features,
 	.set_guest_features	= set_guest_features,
diff --git a/virtio/rng.c b/virtio/rng.c
index 8031368..9b9e128 100644
--- a/virtio/rng.c
+++ b/virtio/rng.c
@@ -141,7 +141,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size)
 	return size;
 }
 
-static struct virtio_ops rng_dev_virtio_ops = (struct virtio_ops) {
+static struct virtio_ops rng_dev_virtio_ops = {
 	.get_config		= get_config,
 	.get_host_features	= get_host_features,
 	.set_guest_features	= set_guest_features,
diff --git a/virtio/scsi.c b/virtio/scsi.c
index be254f3..58d2353 100644
--- a/virtio/scsi.c
+++ b/virtio/scsi.c
@@ -167,7 +167,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size)
 	return size;
 }
 
-static struct virtio_ops scsi_dev_virtio_ops = (struct virtio_ops) {
+static struct virtio_ops scsi_dev_virtio_ops = {
 	.get_config		= get_config,
 	.get_host_features	= get_host_features,
 	.set_guest_features	= set_guest_features,
-- 
2.3.5

[PATCH 02/12] qcow: fix signedness bugs

[Andre Przywara]

Some functions in qcow.c return u64, but are checked against < 0
because they want to check for the -1 error return value.
Do an explicit comparison against the casted -1 to express this
properly.
This was silently compiled out by gcc, but clang complained about it.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 disk/qcow.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/disk/qcow.c b/disk/qcow.c
index e26c419..64cf927 100644
--- a/disk/qcow.c
+++ b/disk/qcow.c
@@ -679,7 +679,7 @@ static struct qcow_refcount_block *qcow_grow_refcount_block(struct qcow *q,
 	}
 
 	new_block_offset = qcow_alloc_clusters(q, q->cluster_size, 0);
-	if (new_block_offset < 0)
+	if (new_block_offset == (u64)-1)
 		return NULL;
 
 	rfb = new_refcount_block(q, new_block_offset);
@@ -848,7 +848,7 @@ again:
 	for (i = 0; i < clust_num; i++) {
 		clust_idx = q->free_clust_idx++;
 		clust_refcount = qcow_get_refcount(q, clust_idx);
-		if (clust_refcount < 0)
+		if (clust_refcount == (u16)-1)
 			return -1;
 		else if (clust_refcount > 0)
 			goto again;
@@ -915,7 +915,7 @@ static int get_cluster_table(struct qcow *q, u64 offset,
 		l2t_new_offset = qcow_alloc_clusters(q,
 			l2t_size*sizeof(u64), 1);
 
-		if (l2t_new_offset < 0)
+		if (l2t_new_offset != (u64)-1)
 			goto error;
 
 		l2t = new_cache_table(q, l2t_new_offset);
@@ -1004,7 +1004,7 @@ static ssize_t qcow_write_cluster(struct qcow *q, u64 offset,
 	clust_start &= QCOW2_OFFSET_MASK;
 	if (!(clust_flags & QCOW2_OFLAG_COPIED)) {
 		clust_new_start	= qcow_alloc_clusters(q, q->cluster_size, 1);
-		if (clust_new_start < 0) {
+		if (clust_new_start != (u64)-1) {
 			pr_warning("Cluster alloc error");
 			goto error;
 		}
-- 
2.3.5

[PATCH 03/12] kvm-ipc: use proper type for file descriptor

[Andre Przywara]

A socket (as any other file descriptor) is of type "int" to catch the
negative error cases. Fix the declaration to allow errors to be
detected.
Found and needed by clang.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 kvm-ipc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kvm-ipc.c b/kvm-ipc.c
index b1c43dd..a289d4b 100644
--- a/kvm-ipc.c
+++ b/kvm-ipc.c
@@ -34,7 +34,7 @@ static pthread_t thread;
 static int kvm__create_socket(struct kvm *kvm)
 {
 	char full_name[PATH_MAX];
-	unsigned int s;
+	int s;
 	struct sockaddr_un local;
 	int len, r;
 
-- 
2.3.5

[PATCH 04/12] Makefile: remove unneeded -s switch on compiling BIOS files

[Andre Przywara]

Stripping has no effect on object files, so having "-s -c" on the
command line makes no sense.
In fact clang complains about it and aborts with an error, so lets
just remove the unneeded "-s" switch here.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 Makefile | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 151fa9d..46e4a9d 100644
--- a/Makefile
+++ b/Makefile
@@ -421,15 +421,15 @@ x86/bios.o: x86/bios/bios.bin x86/bios/bios-rom.h
 
 x86/bios/bios.bin.elf: x86/bios/entry.S x86/bios/e820.c x86/bios/int10.c x86/bios/int15.c x86/bios/rom.ld.S
 	$(E) "  CC       x86/bios/memcpy.o"
-	$(Q) $(CC) -include code16gcc.h $(CFLAGS) $(BIOS_CFLAGS) -c -s x86/bios/memcpy.c -o x86/bios/memcpy.o
+	$(Q) $(CC) -include code16gcc.h $(CFLAGS) $(BIOS_CFLAGS) -c x86/bios/memcpy.c -o x86/bios/memcpy.o
 	$(E) "  CC       x86/bios/e820.o"
-	$(Q) $(CC) -include code16gcc.h $(CFLAGS) $(BIOS_CFLAGS) -c -s x86/bios/e820.c -o x86/bios/e820.o
+	$(Q) $(CC) -include code16gcc.h $(CFLAGS) $(BIOS_CFLAGS) -c x86/bios/e820.c -o x86/bios/e820.o
 	$(E) "  CC       x86/bios/int10.o"
-	$(Q) $(CC) -include code16gcc.h $(CFLAGS) $(BIOS_CFLAGS) -c -s x86/bios/int10.c -o x86/bios/int10.o
+	$(Q) $(CC) -include code16gcc.h $(CFLAGS) $(BIOS_CFLAGS) -c x86/bios/int10.c -o x86/bios/int10.o
 	$(E) "  CC       x86/bios/int15.o"
-	$(Q) $(CC) -include code16gcc.h $(CFLAGS) $(BIOS_CFLAGS) -c -s x86/bios/int15.c -o x86/bios/int15.o
+	$(Q) $(CC) -include code16gcc.h $(CFLAGS) $(BIOS_CFLAGS) -c x86/bios/int15.c -o x86/bios/int15.o
 	$(E) "  CC       x86/bios/entry.o"
-	$(Q) $(CC) $(CFLAGS) $(BIOS_CFLAGS) -c -s x86/bios/entry.S -o x86/bios/entry.o
+	$(Q) $(CC) $(CFLAGS) $(BIOS_CFLAGS) -c x86/bios/entry.S -o x86/bios/entry.o
 	$(E) "  LD      " $@
 	$(Q) $(LD) -T x86/bios/rom.ld.S -o x86/bios/bios.bin.elf x86/bios/memcpy.o x86/bios/entry.o x86/bios/e820.o x86/bios/int10.o x86/bios/int15.o
 
-- 
2.3.5

[PATCH 05/12] ui: remove pointless double const in keymap declarations

[Andre Przywara]

clang does not like two const specifiers in one declaration, so
remove one to let clang compile kvmtool.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 ui/gtk3.c | 2 +-
 ui/sdl.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/gtk3.c b/ui/gtk3.c
index b2335bc..1e08a8f 100644
--- a/ui/gtk3.c
+++ b/ui/gtk3.c
@@ -34,7 +34,7 @@ struct set2_scancode {
 	.type = SCANCODE_ESCAPED,	\
 }
 
-static const struct set2_scancode const keymap[256] = {
+static const struct set2_scancode keymap[256] = {
 	[9]	= DEFINE_SC(0x76),	/* <esc> */
 	[10]	= DEFINE_SC(0x16),	/* 1 */
 	[11]	= DEFINE_SC(0x1e),	/* 2 */
diff --git a/ui/sdl.c b/ui/sdl.c
index a260002..f97a511 100644
--- a/ui/sdl.c
+++ b/ui/sdl.c
@@ -36,7 +36,7 @@ struct set2_scancode {
 	.type = SCANCODE_ESCAPED,\
 }
 
-static const struct set2_scancode const keymap[256] = {
+static const struct set2_scancode keymap[256] = {
 	[9]	= DEFINE_SC(0x76),	/* <esc> */
 	[10]	= DEFINE_SC(0x16),	/* 1 */
 	[11]	= DEFINE_SC(0x1e),	/* 2 */
-- 
2.3.5

[PATCH 06/12] kvm__set_dir(): avoid variable arguments call

[Andre Przywara]

The clang compiler by default dislikes non-literal format strings
in *printf functions, so it complains about kvm__set_dir() in kvm.c.
Instead of suppressing this warning, lets change the code to avoid
that unneeded var_args detour and use a literal format string.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/kvm.h |  2 +-
 kvm.c             | 21 ++++++---------------
 main.c            |  9 ++++++++-
 3 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index 754e029..9818046 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -67,7 +67,7 @@ struct kvm {
 	int			vm_state;
 };
 
-void kvm__set_dir(const char *fmt, ...);
+int kvm__set_dir(const char *basedir, const char *lkvmdir);
 const char *kvm__get_dir(void);
 
 int kvm__init(struct kvm *kvm);
diff --git a/kvm.c b/kvm.c
index 78dd7c0..5222e1e 100644
--- a/kvm.c
+++ b/kvm.c
@@ -63,31 +63,22 @@ extern struct kvm_ext kvm_req_ext[];
 
 static char kvm_dir[PATH_MAX];
 
-static int set_dir(const char *fmt, va_list args)
+int kvm__set_dir(const char *basedir, const char* lkvmdir)
 {
 	char tmp[PATH_MAX];
 
-	vsnprintf(tmp, sizeof(tmp), fmt, args);
-
-	mkdir(tmp, 0777);
-
+	snprintf(tmp, PATH_MAX, "%s/%s", basedir, lkvmdir);
 	if (!realpath(tmp, kvm_dir))
-		return -errno;
+		return errno;
+
+	if (access(tmp, R_OK | W_OK))
+		return errno;
 
 	strcat(kvm_dir, "/");
 
 	return 0;
 }
 
-void kvm__set_dir(const char *fmt, ...)
-{
-	va_list args;
-
-	va_start(args, fmt);
-	set_dir(fmt, args);
-	va_end(args);
-}
-
 const char *kvm__get_dir(void)
 {
 	return kvm_dir;
diff --git a/main.c b/main.c
index 05bc82c..09ba8c1 100644
--- a/main.c
+++ b/main.c
@@ -13,7 +13,14 @@ static int handle_kvm_command(int argc, char **argv)
 
 int main(int argc, char *argv[])
 {
-	kvm__set_dir("%s/%s", HOME_DIR, KVM_PID_FILE_PATH);
+	int ret;
+
+	ret = kvm__set_dir(HOME_DIR, KVM_PID_FILE_PATH);
+	if (ret) {
+		pr_err("could not access lkvm directory \"%s/%s\"\n",
+		       HOME_DIR, KVM_PID_FILE_PATH);
+		return 1;
+	}
 
 	return handle_kvm_command(argc - 1, &argv[1]);
 }
-- 
2.3.5

Re: [PATCH 06/12] kvm__set_dir(): avoid variable arguments call

[Will Deacon]

On Fri, Jul 17, 2015 at 05:02:12PM +0100, Andre Przywara wrote:
> The clang compiler by default dislikes non-literal format strings
> in *printf functions, so it complains about kvm__set_dir() in kvm.c.
> Instead of suppressing this warning, lets change the code to avoid
> that unneeded var_args detour and use a literal format string.

Why does clang moan about this? The current code looks perfectly alright
to me.

Will

[PATCH 07/12] util/util.c: avoid clang error on vsnprintf

[Andre Przywara]

clang by default doesn't seem to like printf calls with non-literal
format strings.
Add the proper pragma to disable this warning in the report function
to make kvmtool compile with clang. Despite its GCC name, clang also
accepts this.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 util/util.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/util/util.c b/util/util.c
index 1877105..825da3f 100644
--- a/util/util.c
+++ b/util/util.c
@@ -10,6 +10,7 @@
 #include <sys/stat.h>
 #include <sys/statfs.h>
 
+#pragma GCC diagnostic ignored "-Wformat-nonliteral"
 static void report(const char *prefix, const char *err, va_list params)
 {
 	char msg[1024];
-- 
2.3.5

Re: [PATCH 07/12] util/util.c: avoid clang error on vsnprintf

[Will Deacon]

On Fri, Jul 17, 2015 at 05:02:13PM +0100, Andre Przywara wrote:
> clang by default doesn't seem to like printf calls with non-literal
> format strings.
> Add the proper pragma to disable this warning in the report function
> to make kvmtool compile with clang. Despite its GCC name, clang also
> accepts this.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  util/util.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/util/util.c b/util/util.c
> index 1877105..825da3f 100644
> --- a/util/util.c
> +++ b/util/util.c
> @@ -10,6 +10,7 @@
>  #include <sys/stat.h>
>  #include <sys/statfs.h>
>  
> +#pragma GCC diagnostic ignored "-Wformat-nonliteral"

Urgh! I think we need to figure out a better way to keep clang happy in
this regard, if we decide that we care about building with it.

Will

Re: [PATCH 07/12] util/util.c: avoid clang error on vsnprintf

[Claudio Fontana]

On 17.07.2015 18:50, Will Deacon wrote:
> On Fri, Jul 17, 2015 at 05:02:13PM +0100, Andre Przywara wrote:
>> clang by default doesn't seem to like printf calls with non-literal
>> format strings.
>> Add the proper pragma to disable this warning in the report function
>> to make kvmtool compile with clang. Despite its GCC name, clang also
>> accepts this.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  util/util.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/util/util.c b/util/util.c
>> index 1877105..825da3f 100644
>> --- a/util/util.c
>> +++ b/util/util.c
>> @@ -10,6 +10,7 @@
>>  #include <sys/stat.h>
>>  #include <sys/statfs.h>
>>  
>> +#pragma GCC diagnostic ignored "-Wformat-nonliteral"
> 
> Urgh! I think we need to figure out a better way to keep clang happy in
> this regard, if we decide that we care about building with it.
> 
> Will
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 

Hi,

what about adding -Wno-format-nonliteral to the CFLAGS when compiling with clang?

This could be set up by configure script or equivalent mechanism, ..?

Ciao,

Claudio

Re: [PATCH 07/12] util/util.c: avoid clang error on vsnprintf

[Andre Przywara]

Hi Claudio,

On 20/07/15 15:28, Claudio Fontana wrote:
> On 17.07.2015 18:50, Will Deacon wrote:
>> On Fri, Jul 17, 2015 at 05:02:13PM +0100, Andre Przywara wrote:
>>> clang by default doesn't seem to like printf calls with non-literal
>>> format strings.
>>> Add the proper pragma to disable this warning in the report function
>>> to make kvmtool compile with clang. Despite its GCC name, clang also
>>> accepts this.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  util/util.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/util/util.c b/util/util.c
>>> index 1877105..825da3f 100644
>>> --- a/util/util.c
>>> +++ b/util/util.c
>>> @@ -10,6 +10,7 @@
>>>  #include <sys/stat.h>
>>>  #include <sys/statfs.h>
>>>  
>>> +#pragma GCC diagnostic ignored "-Wformat-nonliteral"
>>
>> Urgh! I think we need to figure out a better way to keep clang happy in
>> this regard, if we decide that we care about building with it.
>>
>> Will
>> _______________________________________________
>> kvmarm mailing list
>> kvmarm@lists.cs.columbia.edu
>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>>
> 
> Hi,
> 
> what about adding -Wno-format-nonliteral to the CFLAGS when compiling with clang?
> 
> This could be set up by configure script or equivalent mechanism, ..?

GCC also knows about this warning (it's just not enabled in -Wall), so
we could simply add -Wno-format-nonliteral to CFLAGS regardless of the
compiler used.

If that sounds useful, I can respin, also addressing Szabolcs' comment.

Cheers,
Andre.

[PATCH 08/12] Fix call to connect()

[Andre Przywara]

According to the manpage and the prototype the second argument to
connect(2) is a "const struct sockaddr*", so cast our protocol
specific type back to the super type.
This fixes compilation on musl-libc.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 kvm-ipc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kvm-ipc.c b/kvm-ipc.c
index a289d4b..857b0dc 100644
--- a/kvm-ipc.c
+++ b/kvm-ipc.c
@@ -99,7 +99,7 @@ int kvm__get_sock_by_instance(const char *name)
 	strlcpy(local.sun_path, sock_file, sizeof(local.sun_path));
 	len = strlen(local.sun_path) + sizeof(local.sun_family);
 
-	r = connect(s, &local, len);
+	r = connect(s, (struct sockaddr *)&local, len);
 	if (r < 0 && errno == ECONNREFUSED) {
 		/* Tell the user clean ghost socket file */
 		pr_err("\"%s\" could be a ghost socket file, please remove it",
-- 
2.3.5

[PATCH 09/12] use <poll.h> instead of <sys/poll.h>

[Andre Przywara]

The manpage of poll(2) states that the prototype of poll is defined
in <poll.h>. Use that header file instead of <sys/poll.h> to allow
compilation against musl-libc.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 disk/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/disk/core.c b/disk/core.c
index 309e16c..dd2f258 100644
--- a/disk/core.c
+++ b/disk/core.c
@@ -5,7 +5,7 @@
 
 #include <linux/err.h>
 #include <sys/eventfd.h>
-#include <sys/poll.h>
+#include <poll.h>
 
 #define AIO_MAX 256
 
-- 
2.3.5

[PATCH 10/12] check for and use C library provided strlcpy and strlcat

[Andre Przywara]

The musl-libc library provides implementations of strlcpy and strlcat,
so introduce a feature check for it and only use the kvmtool
implementation if there is no library support for it.
This avoids clashes with the public definition.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 Makefile                 |  5 +++++
 config/feature-tests.mak | 10 ++++++++++
 include/kvm/strbuf.h     |  2 ++
 util/strbuf.c            |  2 ++
 4 files changed, 19 insertions(+)

diff --git a/Makefile b/Makefile
index 46e4a9d..285c482 100644
--- a/Makefile
+++ b/Makefile
@@ -199,6 +199,11 @@ endif
 # On a given system, some libs may link statically, some may not; so, check
 # both and only build those that link!
 
+ifeq ($(call try-build,$(SOURCE_STRLCPY),$(CFLAGS),),y)
+	CFLAGS_DYNOPT	+= -DHAVE_STRLCPY
+	CFLAGS_STATOPT	+= -DHAVE_STRLCPY
+endif
+
 ifeq ($(call try-build,$(SOURCE_BFD),$(CFLAGS),-lbfd -static),y)
 	CFLAGS_STATOPT	+= -DCONFIG_HAS_BFD
 	OBJS_STATOPT	+= symbol.o
diff --git a/config/feature-tests.mak b/config/feature-tests.mak
index 6bee6c2..03cdb42 100644
--- a/config/feature-tests.mak
+++ b/config/feature-tests.mak
@@ -196,3 +196,13 @@ int main(void)
 	return 0;
 }
 endef
+
+define SOURCE_STRLCPY
+#include <string.h>
+
+int main(void)
+{
+	strlcpy(NULL, NULL, 0);
+	return 0;
+}
+endef
diff --git a/include/kvm/strbuf.h b/include/kvm/strbuf.h
index 2beefbc..7657339 100644
--- a/include/kvm/strbuf.h
+++ b/include/kvm/strbuf.h
@@ -6,8 +6,10 @@
 
 int prefixcmp(const char *str, const char *prefix);
 
+#ifndef HAVE_STRLCPY
 extern size_t strlcat(char *dest, const char *src, size_t count);
 extern size_t strlcpy(char *dest, const char *src, size_t size);
+#endif
 
 /* some inline functions */
 
diff --git a/util/strbuf.c b/util/strbuf.c
index 99d6b0c..2c6e8ad 100644
--- a/util/strbuf.c
+++ b/util/strbuf.c
@@ -13,6 +13,7 @@ int prefixcmp(const char *str, const char *prefix)
 	}
 }
 
+#ifndef HAVE_STRLCPY
 /**
  * strlcat - Append a length-limited, %NUL-terminated string to another
  * @dest: The string to be appended to
@@ -60,3 +61,4 @@ size_t strlcpy(char *dest, const char *src, size_t size)
 	}
 	return ret;
 }
+#endif
-- 
2.3.5

[PATCH 11/12] avoid using predefined PAGE_SIZE

[Andre Przywara]

The musl-libc comes with a predefined PAGE_SIZE macro, which may be
wrong on systems which support multiple smallest page sizes.
Make sure we use our own (runtime evaluated) definition of PAGE_SIZE.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/kvm.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index 9818046..8b12cd6 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -19,6 +19,10 @@
 #define HOME_DIR		getenv("HOME")
 #define KVM_BINARY_NAME		"lkvm"
 
+/* Let's determine the actual page size at runtime. */
+#ifdef PAGE_SIZE
+#undef PAGE_SIZE
+#endif
 #define PAGE_SIZE (sysconf(_SC_PAGE_SIZE))
 
 #define DEFINE_KVM_EXT(ext)		\
-- 
2.3.5

Re: [PATCH 11/12] avoid using predefined PAGE_SIZE

[Szabolcs Nagy]

On 17/07/15 17:02, Andre Przywara wrote:
> The musl-libc comes with a predefined PAGE_SIZE macro, which may be
> wrong on systems which support multiple smallest page sizes.
> Make sure we use our own (runtime evaluated) definition of PAGE_SIZE.
>

musl only provides PAGE_SIZE on archs where it is
constant, if not, that's a musl bug and should be
fixed there (this is a posix requirement).

the ifdef does not help around this: musl sysconf
will return the same value as you get from the
header (it would be a conformance bug otherwise).

use

#include <limits.h>
#ifndef PAGE_SIZE
#define PAGE_SIZE sysconf(_SC_PAGE_SIZE)
#endif

> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  include/kvm/kvm.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
> index 9818046..8b12cd6 100644
> --- a/include/kvm/kvm.h
> +++ b/include/kvm/kvm.h
> @@ -19,6 +19,10 @@
>  #define HOME_DIR             getenv("HOME")
>  #define KVM_BINARY_NAME              "lkvm"
>
> +/* Let's determine the actual page size at runtime. */
> +#ifdef PAGE_SIZE
> +#undef PAGE_SIZE
> +#endif
>  #define PAGE_SIZE (sysconf(_SC_PAGE_SIZE))
>
>  #define DEFINE_KVM_EXT(ext)          \
>


-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No:  2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No:  2548782

[PATCH 12/12] remove KVM_CAP_MAX_VCPUS hack

[Andre Przywara]

As we now have the header file in our repository, we can safely follow
the recommendation in kvm.c and remove the hack adding the
KVM_CAP_MAX_VCPUS macro.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 kvm.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/kvm.c b/kvm.c
index 5222e1e..53e5c6b 100644
--- a/kvm.c
+++ b/kvm.c
@@ -223,14 +223,6 @@ int kvm__recommended_cpus(struct kvm *kvm)
 	return ret;
 }
 
-/*
- * The following hack should be removed once 'x86: Raise the hard
- * VCPU count limit' makes it's way into the mainline.
- */
-#ifndef KVM_CAP_MAX_VCPUS
-#define KVM_CAP_MAX_VCPUS 66
-#endif
-
 int kvm__max_cpus(struct kvm *kvm)
 {
 	int ret;
-- 
2.3.5

Re: [PATCH 00/12] kvmtool: Improve portability

[Will Deacon]

On Fri, Jul 17, 2015 at 05:02:06PM +0100, Andre Przywara wrote:
> Hi,

Hi Andre,

> this is a collection of patches to bring kvmtool closer to standards
> compliance (with standards not necessarily meaning GNU only).
> With all those patches applied, you can compile kvmtool with newer
> C standards, with clang and against musl-libc.

Most of this looks good to me, thanks! I'll leave it to stew for a few
days on the list, but I'm planning to merge it all apart from the two
*printf clang hacks and the PAGE_SIZE patch which Szabolcs commented on.

Will