[Qemu-devel] [PATCH 00/14] gcc extra warning fixes

[Qemu-devel] [PATCH 00/14] gcc extra warning fixes

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Hi,

I started building QEMU with some more aggressive error flags to see
what dropped out. I started fixing up some of the cases, removing
unused arguments to functions, comparisons of unsigned types against
negative values etc. and a few other minor changes to avoid compiler
warnings.

Set of 14 patches following.

Cheers,
Jes

Jes Sorensen (14):
  Remove unused argument for nbd_client()
  Respect return value from nbd_client()
  Fix repeated typo: was "end if list" instead of "end of list"
  Zero initialize timespec struct explicitly
  Remove unused argument for check_for_block_signature()
  Remove unused argument for encrypt_sectors()
  Remove unused argument for get_whole_cluster()
  Remove unused argument for qcow2_encrypt_sectors()
  Remove unused arguments for add_aio_request() and free_aio_req()
  Zero json struct with memset() instea of = {} to keep compiler happy.
  Remove unused function arguments
  size_t is unsigned, so (foo >= 0) is always true
  Change DPRINTF() to do{}while(0) to avoid compiler warning
  load_multiboot(): get_image_size() returns int

 block/qcow.c          |   13 ++++++-------
 block/qcow2-cluster.c |   17 ++++++++---------
 block/qcow2.c         |   10 +++++-----
 block/qcow2.h         |    7 +++----
 block/raw.c           |    4 ++--
 block/sheepdog.c      |   20 ++++++++++----------
 block/vmdk.c          |    4 ++--
 hw/multiboot.c        |    2 +-
 json-parser.c         |   39 +++++++++++++++++++--------------------
 linux-aio.c           |    2 +-
 nbd.c                 |    4 ++--
 nbd.h                 |    2 +-
 qemu-config.c         |   12 ++++++------
 qemu-nbd.c            |    5 ++++-
 qjson.c               |    3 ++-
 slirp/bootp.c         |    2 +-
 ui/vnc-enc-tight.c    |    8 ++++----
 17 files changed, 77 insertions(+), 77 deletions(-)

-- 
1.7.2.2

[Qemu-devel] [PATCH 01/14] Remove unused argument for nbd_client()

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 nbd.c      |    4 ++--
 nbd.h      |    2 +-
 qemu-nbd.c |    2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/nbd.c b/nbd.c
index a9f295f..7049998 100644
--- a/nbd.c
+++ b/nbd.c
@@ -393,7 +393,7 @@ int nbd_disconnect(int fd)
 	return 0;
 }
 
-int nbd_client(int fd, int csock)
+int nbd_client(int fd)
 {
 	int ret;
 	int serrno;
@@ -427,7 +427,7 @@ int nbd_disconnect(int fd)
     return -1;
 }
 
-int nbd_client(int fd, int csock)
+int nbd_client(int fd)
 {
     errno = ENOTSUP;
     return -1;
diff --git a/nbd.h b/nbd.h
index 5a1fbdf..f103c3c 100644
--- a/nbd.h
+++ b/nbd.h
@@ -55,7 +55,7 @@ int nbd_send_request(int csock, struct nbd_request *request);
 int nbd_receive_reply(int csock, struct nbd_reply *reply);
 int nbd_trip(BlockDriverState *bs, int csock, off_t size, uint64_t dev_offset,
              off_t *offset, bool readonly, uint8_t *data, int data_size);
-int nbd_client(int fd, int csock);
+int nbd_client(int fd);
 int nbd_disconnect(int fd);
 
 #endif
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 4e607cf..9cc8f47 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -417,7 +417,7 @@ int main(int argc, char **argv)
 
             show_parts(device);
 
-            nbd_client(fd, sock);
+            nbd_client(fd);
             close(fd);
  out:
             kill(pid, SIGTERM);
-- 
1.7.2.2

[Qemu-devel] [PATCH 02/14] Respect return value from nbd_client()

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 qemu-nbd.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 9cc8f47..67ce50b 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -417,7 +417,10 @@ int main(int argc, char **argv)
 
             show_parts(device);
 
-            nbd_client(fd);
+            ret = nbd_client(fd);
+            if (ret) {
+                ret = 1;
+            }
             close(fd);
  out:
             kill(pid, SIGTERM);
-- 
1.7.2.2

[Qemu-devel] [PATCH 03/14] Fix repeated typo: was "end if list" instead of "end of list"

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 qemu-config.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/qemu-config.c b/qemu-config.c
index 3abe655..1efdbec 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -80,7 +80,7 @@ static QemuOptsList qemu_drive_opts = {
             .name = "readonly",
             .type = QEMU_OPT_BOOL,
         },
-        { /* end if list */ }
+        { /* end of list */ }
     },
 };
 
@@ -147,7 +147,7 @@ static QemuOptsList qemu_chardev_opts = {
             .name = "signal",
             .type = QEMU_OPT_BOOL,
         },
-        { /* end if list */ }
+        { /* end of list */ }
     },
 };
 
@@ -203,7 +203,7 @@ static QemuOptsList qemu_device_opts = {
          * sanity checking will happen later
          * when setting device properties
          */
-        { /* end if list */ }
+        { /* end of list */ }
     },
 };
 
@@ -247,7 +247,7 @@ static QemuOptsList qemu_rtc_opts = {
             .name = "driftfix",
             .type = QEMU_OPT_STRING,
         },
-        { /* end if list */ }
+        { /* end of list */ }
     },
 };
 
@@ -265,7 +265,7 @@ static QemuOptsList qemu_global_opts = {
             .name = "value",
             .type = QEMU_OPT_STRING,
         },
-        { /* end if list */ }
+        { /* end of list */ }
     },
 };
 
@@ -284,7 +284,7 @@ static QemuOptsList qemu_mon_opts = {
             .name = "default",
             .type = QEMU_OPT_BOOL,
         },
-        { /* end if list */ }
+        { /* end of list */ }
     },
 };
 
-- 
1.7.2.2

[Qemu-devel] [PATCH 04/14] Zero initialize timespec struct explicitly

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 linux-aio.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/linux-aio.c b/linux-aio.c
index 68f4b3d..3240996 100644
--- a/linux-aio.c
+++ b/linux-aio.c
@@ -118,7 +118,7 @@ static void qemu_laio_completion_cb(void *opaque)
         struct io_event events[MAX_EVENTS];
         uint64_t val;
         ssize_t ret;
-        struct timespec ts = { 0 };
+        struct timespec ts = { 0, 0 };
         int nevents, i;
 
         do {
-- 
1.7.2.2

[Qemu-devel] [PATCH 05/14] Remove unused argument for check_for_block_signature()

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 block/raw.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/raw.c b/block/raw.c
index 61e6748..fc057d0 100644
--- a/block/raw.c
+++ b/block/raw.c
@@ -12,7 +12,7 @@ static int raw_open(BlockDriverState *bs, int flags)
 /* check for the user attempting to write something that looks like a
    block format header to the beginning of the image and fail out.
 */
-static int check_for_block_signature(BlockDriverState *bs, const uint8_t *buf)
+static int check_for_block_signature(const uint8_t *buf)
 {
     static const uint8_t signatures[][4] = {
         { 'Q', 'F', 'I', 0xfb }, /* qcow/qcow2 */
@@ -42,7 +42,7 @@ static int check_write_unsafe(BlockDriverState *bs, int64_t sector_num,
     }
 
     if (sector_num == 0 && nb_sectors > 0) {
-        return check_for_block_signature(bs, buf);
+        return check_for_block_signature(buf);
     }
 
     return 0;
-- 
1.7.2.2

[Qemu-devel] [PATCH 06/14] Remove unused argument for encrypt_sectors()

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 block/qcow.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 816103d..c4d8cb3 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -214,9 +214,8 @@ static int qcow_set_key(BlockDriverState *bs, const char *key)
 /* The crypt function is compatible with the linux cryptoloop
    algorithm for < 4 GB images. NOTE: out_buf == in_buf is
    supported */
-static void encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
-                            uint8_t *out_buf, const uint8_t *in_buf,
-                            int nb_sectors, int enc,
+static void encrypt_sectors(int64_t sector_num, uint8_t *out_buf,
+                            const uint8_t *in_buf, int nb_sectors, int enc,
                             const AES_KEY *key)
 {
     union {
@@ -351,7 +350,7 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
                     memset(s->cluster_data + 512, 0x00, 512);
                     for(i = 0; i < s->cluster_sectors; i++) {
                         if (i < n_start || i >= n_end) {
-                            encrypt_sectors(s, start_sect + i,
+                            encrypt_sectors(start_sect + i,
                                             s->cluster_data,
                                             s->cluster_data + 512, 1, 1,
                                             &s->aes_encrypt_key);
@@ -474,7 +473,7 @@ static int qcow_read(BlockDriverState *bs, int64_t sector_num,
             if (ret != n * 512)
                 return -1;
             if (s->crypt_method) {
-                encrypt_sectors(s, sector_num, buf, buf, n, 0,
+                encrypt_sectors(sector_num, buf, buf, n, 0,
                                 &s->aes_decrypt_key);
             }
         }
@@ -558,7 +557,7 @@ static void qcow_aio_read_cb(void *opaque, int ret)
         /* nothing to do */
     } else {
         if (s->crypt_method) {
-            encrypt_sectors(s, acb->sector_num, acb->buf, acb->buf,
+            encrypt_sectors(acb->sector_num, acb->buf, acb->buf,
                             acb->n, 0,
                             &s->aes_decrypt_key);
         }
@@ -687,7 +686,7 @@ static void qcow_aio_write_cb(void *opaque, int ret)
                 goto done;
             }
         }
-        encrypt_sectors(s, acb->sector_num, acb->cluster_data, acb->buf,
+        encrypt_sectors(acb->sector_num, acb->cluster_data, acb->buf,
                         acb->n, 1, &s->aes_encrypt_key);
         src_buf = acb->cluster_data;
     } else {
-- 
1.7.2.2

[Qemu-devel] [PATCH 07/14] Remove unused argument for get_whole_cluster()

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 block/vmdk.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 2d4ba42..54ad66f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -437,7 +437,7 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, VmdkMetaData *m_data,
                                    uint64_t offset, int allocate);
 
 static int get_whole_cluster(BlockDriverState *bs, uint64_t cluster_offset,
-                             uint64_t offset, int allocate)
+                             uint64_t offset)
 {
     BDRVVmdkState *s = bs->opaque;
     uint8_t  whole_grain[s->cluster_sectors*512];        // 128 sectors * 512 bytes each = grain size 64KB
@@ -552,7 +552,7 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, VmdkMetaData *m_data,
          * This problem may occur because of insufficient space on host disk
          * or inappropriate VM shutdown.
          */
-        if (get_whole_cluster(bs, cluster_offset, offset, allocate) == -1)
+        if (get_whole_cluster(bs, cluster_offset, offset) == -1)
             return 0;
 
         if (m_data) {
-- 
1.7.2.2

[Qemu-devel] [PATCH 08/14] Remove unused argument for qcow2_encrypt_sectors()

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 block/qcow2-cluster.c |   17 ++++++++---------
 block/qcow2.c         |   10 +++++-----
 block/qcow2.h         |    7 +++----
 3 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 166922f..14aaf7a 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -319,9 +319,8 @@ static int count_contiguous_free_clusters(uint64_t nb_clusters, uint64_t *l2_tab
 /* The crypt function is compatible with the linux cryptoloop
    algorithm for < 4 GB images. NOTE: out_buf == in_buf is
    supported */
-void qcow2_encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
-                           uint8_t *out_buf, const uint8_t *in_buf,
-                           int nb_sectors, int enc,
+void qcow2_encrypt_sectors(int64_t sector_num, uint8_t *out_buf,
+                           const uint8_t *in_buf, int nb_sectors, int enc,
                            const AES_KEY *key)
 {
     union {
@@ -382,8 +381,8 @@ static int qcow_read(BlockDriverState *bs, int64_t sector_num,
             if (ret != n * 512)
                 return -1;
             if (s->crypt_method) {
-                qcow2_encrypt_sectors(s, sector_num, buf, buf, n, 0,
-                                &s->aes_decrypt_key);
+                qcow2_encrypt_sectors(sector_num, buf, buf, n, 0,
+                                      &s->aes_decrypt_key);
             }
         }
         nb_sectors -= n;
@@ -407,10 +406,10 @@ static int copy_sectors(BlockDriverState *bs, uint64_t start_sect,
     if (ret < 0)
         return ret;
     if (s->crypt_method) {
-        qcow2_encrypt_sectors(s, start_sect + n_start,
-                        s->cluster_data,
-                        s->cluster_data, n, 1,
-                        &s->aes_encrypt_key);
+        qcow2_encrypt_sectors(start_sect + n_start,
+                              s->cluster_data,
+                              s->cluster_data, n, 1,
+                              &s->aes_encrypt_key);
     }
     BLKDBG_EVENT(bs->file, BLKDBG_COW_WRITE);
     ret = bdrv_write_sync(bs->file, (cluster_offset >> 9) + n_start,
diff --git a/block/qcow2.c b/block/qcow2.c
index a53014d..e7ba7b4 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -397,9 +397,9 @@ static void qcow_aio_read_cb(void *opaque, int ret)
         /* nothing to do */
     } else {
         if (s->crypt_method) {
-            qcow2_encrypt_sectors(s, acb->sector_num, acb->buf, acb->buf,
-                            acb->cur_nr_sectors, 0,
-                            &s->aes_decrypt_key);
+            qcow2_encrypt_sectors(acb->sector_num, acb->buf, acb->buf,
+                                  acb->cur_nr_sectors, 0,
+                                  &s->aes_decrypt_key);
         }
     }
 
@@ -609,8 +609,8 @@ static void qcow_aio_write_cb(void *opaque, int ret)
             acb->cluster_data = qemu_mallocz(QCOW_MAX_CRYPT_CLUSTERS *
                                              s->cluster_size);
         }
-        qcow2_encrypt_sectors(s, acb->sector_num, acb->cluster_data, acb->buf,
-                        acb->cur_nr_sectors, 1, &s->aes_encrypt_key);
+        qcow2_encrypt_sectors(acb->sector_num, acb->cluster_data, acb->buf,
+                              acb->cur_nr_sectors, 1, &s->aes_encrypt_key);
         src_buf = acb->cluster_data;
     } else {
         src_buf = acb->buf;
diff --git a/block/qcow2.h b/block/qcow2.h
index 3ff162e..477b0f9 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -191,10 +191,9 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res);
 int qcow2_grow_l1_table(BlockDriverState *bs, int min_size);
 void qcow2_l2_cache_reset(BlockDriverState *bs);
 int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset);
-void qcow2_encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
-                     uint8_t *out_buf, const uint8_t *in_buf,
-                     int nb_sectors, int enc,
-                     const AES_KEY *key);
+void qcow2_encrypt_sectors(int64_t sector_num, uint8_t *out_buf,
+                           const uint8_t *in_buf, int nb_sectors, int enc,
+                           const AES_KEY *key);
 
 int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
     int *num, uint64_t *cluster_offset);
-- 
1.7.2.2

[Qemu-devel] [PATCH 09/14] Remove unused arguments for add_aio_request() and free_aio_req()

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 block/sheepdog.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 81aa564..2ef8655 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -394,7 +394,7 @@ static inline AIOReq *alloc_aio_req(BDRVSheepdogState *s, SheepdogAIOCB *acb,
     return aio_req;
 }
 
-static inline int free_aio_req(BDRVSheepdogState *s, AIOReq *aio_req)
+static inline int free_aio_req(AIOReq *aio_req)
 {
     SheepdogAIOCB *acb = aio_req->aiocb;
     QLIST_REMOVE(aio_req, outstanding_aio_siblings);
@@ -755,7 +755,7 @@ out:
 }
 
 static int add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req,
-                           struct iovec *iov, int niov, int create,
+                           struct iovec *iov, int create,
                            enum AIOCBState aiocb_type);
 
 /*
@@ -779,10 +779,10 @@ static void send_pending_req(BDRVSheepdogState *s, uint64_t oid, uint32_t id)
 
         acb = aio_req->aiocb;
         ret = add_aio_request(s, aio_req, acb->qiov->iov,
-                              acb->qiov->niov, 0, acb->aiocb_type);
+                              0, acb->aiocb_type);
         if (ret < 0) {
             error_report("add_aio_request is failed\n");
-            free_aio_req(s, aio_req);
+            free_aio_req(aio_req);
             if (QLIST_EMPTY(&acb->aioreq_head)) {
                 sd_finish_aiocb(acb);
             }
@@ -872,7 +872,7 @@ static void aio_read_response(void *opaque)
         error_report("%s\n", sd_strerror(rsp.result));
     }
 
-    rest = free_aio_req(s, aio_req);
+    rest = free_aio_req(aio_req);
     if (!rest) {
         /*
          * We've finished all requests which belong to the AIOCB, so
@@ -1064,7 +1064,7 @@ out:
 }
 
 static int add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req,
-                           struct iovec *iov, int niov, int create,
+                           struct iovec *iov, int create,
                            enum AIOCBState aiocb_type)
 {
     int nr_copies = s->inode.nr_copies;
@@ -1460,9 +1460,9 @@ static void sd_write_done(SheepdogAIOCB *acb)
         iov.iov_len = sizeof(s->inode);
         aio_req = alloc_aio_req(s, acb, vid_to_vdi_oid(s->inode.vdi_id),
                                 data_len, offset, 0, 0, offset);
-        ret = add_aio_request(s, aio_req, &iov, 1, 0, AIOCB_WRITE_UDATA);
+        ret = add_aio_request(s, aio_req, &iov, 0, AIOCB_WRITE_UDATA);
         if (ret) {
-            free_aio_req(s, aio_req);
+            free_aio_req(aio_req);
             acb->ret = -EIO;
             goto out;
         }
@@ -1613,11 +1613,11 @@ static void sd_readv_writev_bh_cb(void *p)
             }
         }
 
-        ret = add_aio_request(s, aio_req, acb->qiov->iov, acb->qiov->niov,
+        ret = add_aio_request(s, aio_req, acb->qiov->iov,
                               create, acb->aiocb_type);
         if (ret < 0) {
             error_report("add_aio_request is failed\n");
-            free_aio_req(s, aio_req);
+            free_aio_req(aio_req);
             acb->ret = -EIO;
             goto out;
         }
-- 
1.7.2.2

[Qemu-devel] [PATCH 10/14] Zero json struct with memset() instea of = {} to keep compiler happy.

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

This keeps the compiler happy when building with -Wextra while
effectively generating the same code.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 qjson.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/qjson.c b/qjson.c
index e4ee433..f259547 100644
--- a/qjson.c
+++ b/qjson.c
@@ -36,8 +36,9 @@ static void parse_json(JSONMessageParser *parser, QList *tokens)
 
 QObject *qobject_from_jsonv(const char *string, va_list *ap)
 {
-    JSONParsingState state = {};
+    JSONParsingState state;
 
+    memset(&state, 0, sizeof(state));
     state.ap = ap;
 
     json_message_parser_init(&state.parser, parse_json);
-- 
1.7.2.2

[Qemu-devel] [PATCH 11/14] Remove unused function arguments

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 json-parser.c |   39 +++++++++++++++++++--------------------
 1 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/json-parser.c b/json-parser.c
index 70b9b6f..68ff9c1 100644
--- a/json-parser.c
+++ b/json-parser.c
@@ -91,7 +91,7 @@ static int token_is_escape(QObject *obj, const char *value)
 /**
  * Error handler
  */
-static void parse_error(JSONParserContext *ctxt, QObject *token, const char *msg, ...)
+static void parse_error(const char *msg, ...)
 {
     va_list ap;
     va_start(ap, msg);
@@ -165,7 +165,7 @@ static int hex2decimal(char ch)
  *      \t
  *      \u four-hex-digits 
  */
-static QString *qstring_from_escaped_str(JSONParserContext *ctxt, QObject *token)
+static QString *qstring_from_escaped_str(QObject *token)
 {
     const char *ptr = token_get_value(token);
     QString *str;
@@ -232,8 +232,7 @@ static QString *qstring_from_escaped_str(JSONParserContext *ctxt, QObject *token
                     if (qemu_isxdigit(*ptr)) {
                         unicode_char |= hex2decimal(*ptr) << ((3 - i) * 4);
                     } else {
-                        parse_error(ctxt, token,
-                                    "invalid hex escape sequence in string");
+                        parse_error("invalid hex escape sequence in string");
                         goto out;
                     }
                     ptr++;
@@ -243,7 +242,7 @@ static QString *qstring_from_escaped_str(JSONParserContext *ctxt, QObject *token
                 qstring_append(str, utf8_char);
             }   break;
             default:
-                parse_error(ctxt, token, "invalid escape sequence in string");
+                parse_error("invalid escape sequence in string");
                 goto out;
             }
         } else {
@@ -274,19 +273,19 @@ static int parse_pair(JSONParserContext *ctxt, QDict *dict, QList **tokens, va_l
     peek = qlist_peek(working);
     key = parse_value(ctxt, &working, ap);
     if (!key || qobject_type(key) != QTYPE_QSTRING) {
-        parse_error(ctxt, peek, "key is not a string in object");
+        parse_error("key is not a string in object");
         goto out;
     }
 
     token = qlist_pop(working);
     if (!token_is_operator(token, ':')) {
-        parse_error(ctxt, token, "missing : in object pair");
+        parse_error("missing : in object pair");
         goto out;
     }
 
     value = parse_value(ctxt, &working, ap);
     if (value == NULL) {
-        parse_error(ctxt, token, "Missing value in dict");
+        parse_error("Missing value in dict");
         goto out;
     }
 
@@ -331,7 +330,7 @@ static QObject *parse_object(JSONParserContext *ctxt, QList **tokens, va_list *a
         token = qlist_pop(working);
         while (!token_is_operator(token, '}')) {
             if (!token_is_operator(token, ',')) {
-                parse_error(ctxt, token, "expected separator in dict");
+                parse_error("expected separator in dict");
                 goto out;
             }
             qobject_decref(token);
@@ -384,7 +383,7 @@ static QObject *parse_array(JSONParserContext *ctxt, QList **tokens, va_list *ap
 
         obj = parse_value(ctxt, &working, ap);
         if (obj == NULL) {
-            parse_error(ctxt, token, "expecting value");
+            parse_error("expecting value");
             goto out;
         }
 
@@ -393,7 +392,7 @@ static QObject *parse_array(JSONParserContext *ctxt, QList **tokens, va_list *ap
         token = qlist_pop(working);
         while (!token_is_operator(token, ']')) {
             if (!token_is_operator(token, ',')) {
-                parse_error(ctxt, token, "expected separator in list");
+                parse_error("expected separator in list");
                 goto out;
             }
 
@@ -402,7 +401,7 @@ static QObject *parse_array(JSONParserContext *ctxt, QList **tokens, va_list *ap
 
             obj = parse_value(ctxt, &working, ap);
             if (obj == NULL) {
-                parse_error(ctxt, token, "expecting value");
+                parse_error("expecting value");
                 goto out;
             }
 
@@ -431,7 +430,7 @@ out:
     return NULL;
 }
 
-static QObject *parse_keyword(JSONParserContext *ctxt, QList **tokens)
+static QObject *parse_keyword(QList **tokens)
 {
     QObject *token, *ret;
     QList *working = qlist_copy(*tokens);
@@ -447,7 +446,7 @@ static QObject *parse_keyword(JSONParserContext *ctxt, QList **tokens)
     } else if (token_is_keyword(token, "false")) {
         ret = QOBJECT(qbool_from_int(false));
     } else {
-        parse_error(ctxt, token, "invalid keyword `%s'", token_get_value(token));
+        parse_error("invalid keyword `%s'", token_get_value(token));
         goto out;
     }
 
@@ -464,7 +463,7 @@ out:
     return NULL;
 }
 
-static QObject *parse_escape(JSONParserContext *ctxt, QList **tokens, va_list *ap)
+static QObject *parse_escape(QList **tokens, va_list *ap)
 {
     QObject *token = NULL, *obj;
     QList *working = qlist_copy(*tokens);
@@ -507,7 +506,7 @@ out:
     return NULL;
 }
 
-static QObject *parse_literal(JSONParserContext *ctxt, QList **tokens)
+static QObject *parse_literal(QList **tokens)
 {
     QObject *token, *obj;
     QList *working = qlist_copy(*tokens);
@@ -515,7 +514,7 @@ static QObject *parse_literal(JSONParserContext *ctxt, QList **tokens)
     token = qlist_pop(working);
     switch (token_get_type(token)) {
     case JSON_STRING:
-        obj = QOBJECT(qstring_from_escaped_str(ctxt, token));
+        obj = QOBJECT(qstring_from_escaped_str(token));
         break;
     case JSON_INTEGER:
         obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 10)));
@@ -550,13 +549,13 @@ static QObject *parse_value(JSONParserContext *ctxt, QList **tokens, va_list *ap
         obj = parse_array(ctxt, tokens, ap);
     }
     if (obj == NULL) {
-        obj = parse_escape(ctxt, tokens, ap);
+        obj = parse_escape(tokens, ap);
     }
     if (obj == NULL) {
-        obj = parse_keyword(ctxt, tokens);
+        obj = parse_keyword(tokens);
     } 
     if (obj == NULL) {
-        obj = parse_literal(ctxt, tokens);
+        obj = parse_literal(tokens);
     }
 
     return obj;
-- 
1.7.2.2

[Qemu-devel] [PATCH 12/14] size_t is unsigned, so (foo >= 0) is always true

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 ui/vnc-enc-tight.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
index c4c9c3b..df975af 100644
--- a/ui/vnc-enc-tight.c
+++ b/ui/vnc-enc-tight.c
@@ -926,7 +926,7 @@ static int send_full_color_rect(VncState *vs, int x, int y, int w, int h)
                                 tight_conf[vs->tight.compression].raw_zlib_level,
                                 Z_DEFAULT_STRATEGY);
 
-    return (bytes >= 0);
+    return 1;
 }
 
 static int send_solid_rect(VncState *vs)
@@ -1001,7 +1001,7 @@ static int send_mono_rect(VncState *vs, int x, int y,
     vs->tight.tight.offset = bytes;
 
     bytes = tight_compress_data(vs, stream, bytes, level, Z_DEFAULT_STRATEGY);
-    return (bytes >= 0);
+    return 1;
 }
 
 struct palette_cb_priv {
@@ -1057,7 +1057,7 @@ static bool send_gradient_rect(VncState *vs, int x, int y, int w, int h)
 
     bytes = tight_compress_data(vs, stream, bytes,
                                 level, Z_FILTERED);
-    return (bytes >= 0);
+    return 1;
 }
 
 static int send_palette_rect(VncState *vs, int x, int y,
@@ -1118,7 +1118,7 @@ static int send_palette_rect(VncState *vs, int x, int y,
 
     bytes = tight_compress_data(vs, stream, bytes,
                                 level, Z_DEFAULT_STRATEGY);
-    return (bytes >= 0);
+    return 1;
 }
 
 #if defined(CONFIG_VNC_JPEG) || defined(CONFIG_VNC_PNG)
-- 
1.7.2.2

[Qemu-devel] [PATCH 13/14] Change DPRINTF() to do{}while(0) to avoid compiler warning

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 slirp/bootp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/slirp/bootp.c b/slirp/bootp.c
index 3e4e881..41460ff 100644
--- a/slirp/bootp.c
+++ b/slirp/bootp.c
@@ -33,7 +33,7 @@ static const uint8_t rfc1533_cookie[] = { RFC1533_COOKIE };
 #define DPRINTF(fmt, ...) \
 do if (slirp_debug & DBG_CALL) { fprintf(dfd, fmt, ##  __VA_ARGS__); fflush(dfd); } while (0)
 #else
-#define DPRINTF(fmt, ...)
+#define DPRINTF(fmt, ...) do{}while(0)
 #endif
 
 static BOOTPClient *get_new_addr(Slirp *slirp, struct in_addr *paddr,
-- 
1.7.2.2

[Qemu-devel] [PATCH 14/14] load_multiboot(): get_image_size() returns int

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Do not store return of get_image_size() in a uint32_t as it makes it
impossible to detect error returns from get_image_size.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 hw/multiboot.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/multiboot.c b/hw/multiboot.c
index dc980e6..f9097a2 100644
--- a/hw/multiboot.c
+++ b/hw/multiboot.c
@@ -252,7 +252,7 @@ int load_multiboot(void *fw_cfg,
 
         do {
             char *next_space;
-            uint32_t mb_mod_length;
+            int mb_mod_length;
             uint32_t offs = mbs.mb_buf_size;
 
             next_initrd = strchr(initrd_filename, ',');
-- 
1.7.2.2

[Qemu-devel] Re: [PATCH 10/14] Zero json struct with memset() instea of = {} to keep compiler happy.

[Anthony Liguori]

On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> This keeps the compiler happy when building with -Wextra while
> effectively generating the same code.
>
> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
>    

What's GCC's compliant?

Regards,

Anthony Liguori

> ---
>   qjson.c |    3 ++-
>   1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/qjson.c b/qjson.c
> index e4ee433..f259547 100644
> --- a/qjson.c
> +++ b/qjson.c
> @@ -36,8 +36,9 @@ static void parse_json(JSONMessageParser *parser, QList *tokens)
>
>   QObject *qobject_from_jsonv(const char *string, va_list *ap)
>   {
> -    JSONParsingState state = {};
> +    JSONParsingState state;
>
> +    memset(&state, 0, sizeof(state));
>       state.ap = ap;
>
>       json_message_parser_init(&state.parser, parse_json);
>    

[Qemu-devel] Re: [PATCH 12/14] size_t is unsigned, so (foo >= 0) is always true

[Anthony Liguori]

On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
>    

This is the wrong fix, bytes should be a ssize_t or an int because 
tight_compress_data can return error.

Regards,

Anthony Liguori

> ---
>   ui/vnc-enc-tight.c |    8 ++++----
>   1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
> index c4c9c3b..df975af 100644
> --- a/ui/vnc-enc-tight.c
> +++ b/ui/vnc-enc-tight.c
> @@ -926,7 +926,7 @@ static int send_full_color_rect(VncState *vs, int x, int y, int w, int h)
>                                   tight_conf[vs->tight.compression].raw_zlib_level,
>                                   Z_DEFAULT_STRATEGY);
>
> -    return (bytes>= 0);
> +    return 1;
>   }
>
>   static int send_solid_rect(VncState *vs)
> @@ -1001,7 +1001,7 @@ static int send_mono_rect(VncState *vs, int x, int y,
>       vs->tight.tight.offset = bytes;
>
>       bytes = tight_compress_data(vs, stream, bytes, level, Z_DEFAULT_STRATEGY);
> -    return (bytes>= 0);
> +    return 1;
>   }
>
>   struct palette_cb_priv {
> @@ -1057,7 +1057,7 @@ static bool send_gradient_rect(VncState *vs, int x, int y, int w, int h)
>
>       bytes = tight_compress_data(vs, stream, bytes,
>                                   level, Z_FILTERED);
> -    return (bytes>= 0);
> +    return 1;
>   }
>
>   static int send_palette_rect(VncState *vs, int x, int y,
> @@ -1118,7 +1118,7 @@ static int send_palette_rect(VncState *vs, int x, int y,
>
>       bytes = tight_compress_data(vs, stream, bytes,
>                                   level, Z_DEFAULT_STRATEGY);
> -    return (bytes>= 0);
> +    return 1;
>   }
>
>   #if defined(CONFIG_VNC_JPEG) || defined(CONFIG_VNC_PNG)
>    

[Qemu-devel] Re: [PATCH 10/14] Zero json struct with memset() instea of = {} to keep compiler happy.

[Paolo Bonzini]

On 08/30/2010 05:35 PM, Jes.Sorensen@redhat.com wrote:
> -    JSONParsingState state = {};
> +    JSONParsingState state;
>
> +    memset(&state, 0, sizeof(state));
>       state.ap = ap;
>

    JSONParsingState state = { .ap = ap };

achieves the same.

Paolo

[Qemu-devel] Re: [PATCH 10/14] Zero json struct with memset() instea of = {} to keep compiler happy.

[Jes Sorensen]

On 08/30/10 17:39, Anthony Liguori wrote:
> On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>>
>> This keeps the compiler happy when building with -Wextra while
>> effectively generating the same code.
>>
>> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
>>    
> 
> What's GCC's compliant?

cc1: warnings being treated as errors
qjson.c: In function 'qobject_from_jsonv':
qjson.c:39: error: missing initializer
qjson.c:39: error: (near initialization for 'state.parser')
make: *** [qjson.o] Error 1

We have a lot of these where we try to init a struct element {}.

Yes it's technically legal. However it's painful when you try to apply
more aggressive warning flags looking for real bugs.

I would suggest we modify the coding style to ask people to not init a
struct like this.

Cheers,
Jes

[Qemu-devel] Re: [PATCH 04/14] Zero initialize timespec struct explicitly

[Anthony Liguori]

On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
> ---
>   linux-aio.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/linux-aio.c b/linux-aio.c
> index 68f4b3d..3240996 100644
> --- a/linux-aio.c
> +++ b/linux-aio.c
> @@ -118,7 +118,7 @@ static void qemu_laio_completion_cb(void *opaque)
>           struct io_event events[MAX_EVENTS];
>           uint64_t val;
>           ssize_t ret;
> -        struct timespec ts = { 0 };
> +        struct timespec ts = { 0, 0 };
>    

I don't like these.  What's wrong with { } or { 0 }?  Implicit zeroing 
of members is a critical feature of structure initialization so if there 
is something wrong with this, it's important to know why because 
otherwise we've got a massive amount of broken code.

Regards,

Anthony Liguori

>           int nevents, i;
>
>           do {
>    

[Qemu-devel] Re: [PATCH 10/14] Zero json struct with memset() instea of = {} to keep compiler happy.

[Anthony Liguori]

On 08/30/2010 10:43 AM, Jes Sorensen wrote:
> On 08/30/10 17:39, Anthony Liguori wrote:
>    
>> On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
>>      
>>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>>>
>>> This keeps the compiler happy when building with -Wextra while
>>> effectively generating the same code.
>>>
>>> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
>>>
>>>        
>> What's GCC's compliant?
>>      
> cc1: warnings being treated as errors
> qjson.c: In function 'qobject_from_jsonv':
> qjson.c:39: error: missing initializer
> qjson.c:39: error: (near initialization for 'state.parser')
> make: *** [qjson.o] Error 1
>
> We have a lot of these where we try to init a struct element {}.
>
> Yes it's technically legal. However it's painful when you try to apply
> more aggressive warning flags looking for real bugs.
>    

No, this is GCC being stupid.

> I would suggest we modify the coding style to ask people to not init a
> struct like this.
>    

How else do you terminate a list?  IOW:

MyDeviceInfo device_infos[] = {
   {"foo", 0, 2},
   {"bar", 0, 1},
   {} /* or { 0 } */
};

This is such a pervasive idiom that there's simply no way that GCC can 
possibly try to warn against this.  Plus, it's entirely reasonable.

I think this is just a false positive in GCC.  Otherwise, there's a ton 
of code that it should be throwing warnings against.

Regards,

Anthony Liguori

> Cheers,
> Jes
>    

[Qemu-devel] Re: [PATCH 10/14] Zero json struct with memset() instea of = {} to keep compiler happy.

[Jes Sorensen]

On 08/30/10 17:48, Anthony Liguori wrote:
> On 08/30/2010 10:43 AM, Jes Sorensen wrote:
>> Yes it's technically legal. However it's painful when you try to apply
>> more aggressive warning flags looking for real bugs. 
> 
> No, this is GCC being stupid.
> 
>> I would suggest we modify the coding style to ask people to not init a
>> struct like this.
>>    
> 
> How else do you terminate a list?  IOW:
> 
> MyDeviceInfo device_infos[] = {
>   {"foo", 0, 2},
>   {"bar", 0, 1},
>   {} /* or { 0 } */
> };
> 
> This is such a pervasive idiom that there's simply no way that GCC can
> possibly try to warn against this.  Plus, it's entirely reasonable.
> 
> I think this is just a false positive in GCC.  Otherwise, there's a ton
> of code that it should be throwing warnings against

I believe the comma after the last case takes care of terminating the list.

I agree that it would be nice to get gcc to not moan about this specific
case, however I will argue that my change is worth it to be able to use
the error flags, even if it is gcc being stupid.

Cheers,
Jes

[Qemu-devel] Re: [PATCH 04/14] Zero initialize timespec struct explicitly

[Jes Sorensen]

On 08/30/10 17:43, Anthony Liguori wrote:
> On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>>
>> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
>> ---
>>   linux-aio.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/linux-aio.c b/linux-aio.c
>> index 68f4b3d..3240996 100644
>> --- a/linux-aio.c
>> +++ b/linux-aio.c
>> @@ -118,7 +118,7 @@ static void qemu_laio_completion_cb(void *opaque)
>>           struct io_event events[MAX_EVENTS];
>>           uint64_t val;
>>           ssize_t ret;
>> -        struct timespec ts = { 0 };
>> +        struct timespec ts = { 0, 0 };
>>    
> 
> I don't like these.  What's wrong with { } or { 0 }?  Implicit zeroing
> of members is a critical feature of structure initialization so if there
> is something wrong with this, it's important to know why because
> otherwise we've got a massive amount of broken code.

The specific case above is really inconsistent. Either do {} or {0, 0},
doing just {0} means it is initializing just one element in the struct.
That is broken IMHO.

Cheers,
Jes

Re: [Qemu-devel] Re: [PATCH 10/14] Zero json struct with memset() instea of = {} to keep compiler happy.

[Anthony Liguori]

On 08/30/2010 10:42 AM, Paolo Bonzini wrote:
> On 08/30/2010 05:35 PM, Jes.Sorensen@redhat.com wrote:
>> -    JSONParsingState state = {};
>> +    JSONParsingState state;
>>
>> +    memset(&state, 0, sizeof(state));
>>       state.ap = ap;
>>
>
>    JSONParsingState state = { .ap = ap };
>
> achieves the same.

But the fundamental is, what problem does GCC have with the original?  
If there isn't a reasonable answer, then I'm inclined to think this 
warning mode is a waste of time.

Regards,

Anthony Liguori

> Paolo
>

Re: [Qemu-devel] Re: [PATCH 10/14] Zero json struct with memset() instea of = {} to keep compiler happy.

[Paolo Bonzini]

On 08/30/2010 06:15 PM, Anthony Liguori wrote:
> On 08/30/2010 10:42 AM, Paolo Bonzini wrote:
>> On 08/30/2010 05:35 PM, Jes.Sorensen@redhat.com wrote:
>>> - JSONParsingState state = {};
>>> + JSONParsingState state;
>>>
>>> + memset(&state, 0, sizeof(state));
>>> state.ap = ap;
>>>
>>
>> JSONParsingState state = { .ap = ap };
>>
>> achieves the same.
>
> But the fundamental is, what problem does GCC have with the original? If
> there isn't a reasonable answer, then I'm inclined to think this warning
> mode is a waste of time.

It falls under the "missing fields in initializer" warning.  Arguably, 
an empty initializer should be special cased, but it isn't.

I agree that Jes's original patch is ugly, but the C99 initializer is an 
improvement.

Paolo

[Qemu-devel] Re: [PATCH 11/14] Remove unused function arguments

[Paolo Bonzini]

On 08/30/2010 05:35 PM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen<Jes.Sorensen@redhat.com>

I think it's better to leave ctxt in for future extensibility.

You can mark it as __attribute__((unused)) to suppress the warning.

Paolo

Re: [Qemu-devel] Re: [PATCH 10/14] Zero json struct with memset() instea of = {} to keep compiler happy.

[Anthony Liguori]

On 08/30/2010 11:18 AM, Paolo Bonzini wrote:
> On 08/30/2010 06:15 PM, Anthony Liguori wrote:
>> On 08/30/2010 10:42 AM, Paolo Bonzini wrote:
>>> On 08/30/2010 05:35 PM, Jes.Sorensen@redhat.com wrote:
>>>> - JSONParsingState state = {};
>>>> + JSONParsingState state;
>>>>
>>>> + memset(&state, 0, sizeof(state));
>>>> state.ap = ap;
>>>>
>>>
>>> JSONParsingState state = { .ap = ap };
>>>
>>> achieves the same.
>>
>> But the fundamental is, what problem does GCC have with the original? If
>> there isn't a reasonable answer, then I'm inclined to think this warning
>> mode is a waste of time.
>
> It falls under the "missing fields in initializer" warning.  Arguably, 
> an empty initializer should be special cased, but it isn't.

So the warning is for old style initializer lists?  I disagree that it's 
a valid warning.  First, {} is ambiguous as it can be an empty list of 
c99 initializers and an empty list of c89 initializers.

But even for c89 initializers, it's very common practice to omit 
initializers and rely on the defaulted value.  For instance, { 0 } is 
quite pervasive as an idiom.

>
> I agree that Jes's original patch is ugly, but the C99 initializer is 
> an improvement.

Yes, I'm fine with your patch on it's own but I disagree with GCC's 
warnings here.

Regards,

Anthony Liguori

> Paolo

Re: [Qemu-devel] Re: [PATCH 10/14] Zero json struct with memset() instea of = {} to keep compiler happy.

[Paolo Bonzini]

On 08/30/2010 06:29 PM, Anthony Liguori wrote:
>> Arguably, an empty initializer should be special cased, but it
>> isn't.
>
> So the warning is for old style initializer lists?  I disagree that
> it's a valid warning.  First, {} is ambiguous as it can be an empty
> list of c99 initializers and an empty list of c89 initializers.

Yes, that's what I meant.  {} should be special cased as an empty list
will rarely be a bug, even when C89 initializers are used.

> But even for c89 initializers, it's very common practice to omit
> initializers and rely on the defaulted value.  For instance, { 0 } is
> quite pervasive as an idiom.

That's why the warning is only in -Wextra.  Those are warnings that may 
clash with someone's style conventions.

Paolo

Re: [Qemu-devel] Re: [PATCH 10/14] Zero json struct with memset() instea of = {} to keep compiler happy.

[Nathan Froyd]

On Mon, Aug 30, 2010 at 10:48:55AM -0500, Anthony Liguori wrote:
> No, this is GCC being stupid.
>
> How else do you terminate a list?  IOW:
>
> MyDeviceInfo device_infos[] = {
>   {"foo", 0, 2},
>   {"bar", 0, 1},
>   {} /* or { 0 } */
> };
>
> This is such a pervasive idiom that there's simply no way that GCC can  
> possibly try to warn against this.  Plus, it's entirely reasonable.
>
> I think this is just a false positive in GCC.  Otherwise, there's a ton  
> of code that it should be throwing warnings against.

Well, it sounds like Jes was compiling QEMU was extra warning flags, and
I doubt people do much beyond -Wall and maybe one or two others.

I could see petitioning GCC to only complain if -pedantic.

-Nathan

Re: [Qemu-devel] Re: [PATCH 04/14] Zero initialize timespec struct explicitly

[malc]

On Mon, 30 Aug 2010, Anthony Liguori wrote:

> On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
> > From: Jes Sorensen<Jes.Sorensen@redhat.com>
> > 
> > Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
> > ---
> >   linux-aio.c |    2 +-
> >   1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/linux-aio.c b/linux-aio.c
> > index 68f4b3d..3240996 100644
> > --- a/linux-aio.c
> > +++ b/linux-aio.c
> > @@ -118,7 +118,7 @@ static void qemu_laio_completion_cb(void *opaque)
> >           struct io_event events[MAX_EVENTS];
> >           uint64_t val;
> >           ssize_t ret;
> > -        struct timespec ts = { 0 };
> > +        struct timespec ts = { 0, 0 };
> >    
> 
> I don't like these.  What's wrong with { } or { 0 }?  Implicit zeroing of
> members is a critical feature of structure initialization so if there is
> something wrong with this, it's important to know why because otherwise we've
> got a massive amount of broken code.
> 

Apart from gcc complaining about fields not being initialized explicitly
there's nothing wrong with it.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Re: [PATCH 04/14] Zero initialize timespec struct explicitly

[malc]

On Mon, 30 Aug 2010, Jes Sorensen wrote:

> On 08/30/10 17:43, Anthony Liguori wrote:
> > On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
> >> From: Jes Sorensen<Jes.Sorensen@redhat.com>
> >>
> >> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
> >> ---
> >>   linux-aio.c |    2 +-
> >>   1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/linux-aio.c b/linux-aio.c
> >> index 68f4b3d..3240996 100644
> >> --- a/linux-aio.c
> >> +++ b/linux-aio.c
> >> @@ -118,7 +118,7 @@ static void qemu_laio_completion_cb(void *opaque)
> >>           struct io_event events[MAX_EVENTS];
> >>           uint64_t val;
> >>           ssize_t ret;
> >> -        struct timespec ts = { 0 };
> >> +        struct timespec ts = { 0, 0 };
> >>    
> > 
> > I don't like these.  What's wrong with { } or { 0 }?  Implicit zeroing
> > of members is a critical feature of structure initialization so if there
> > is something wrong with this, it's important to know why because
> > otherwise we've got a massive amount of broken code.
> 
> The specific case above is really inconsistent. Either do {} or {0, 0},
> doing just {0} means it is initializing just one element in the struct.
> That is broken IMHO.
> 

No it doesn't mean that. In this particular case all the fields of ts
will be set to zero, for specific wording look at 6.7.9#21

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Re: [PATCH 10/14] Zero json struct with memset() instea of = {} to keep compiler happy.

[malc]

On Mon, 30 Aug 2010, Anthony Liguori wrote:

> On 08/30/2010 10:43 AM, Jes Sorensen wrote:
> > On 08/30/10 17:39, Anthony Liguori wrote:
> >    
> > > On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
> > >      
> > > > From: Jes Sorensen<Jes.Sorensen@redhat.com>
> > > > 
> > > > This keeps the compiler happy when building with -Wextra while
> > > > effectively generating the same code.
> > > > 
> > > > Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
> > > > 
> > > >        
> > > What's GCC's compliant?
> > >      
> > cc1: warnings being treated as errors
> > qjson.c: In function 'qobject_from_jsonv':
> > qjson.c:39: error: missing initializer
> > qjson.c:39: error: (near initialization for 'state.parser')
> > make: *** [qjson.o] Error 1
> > 
> > We have a lot of these where we try to init a struct element {}.
> > 
> > Yes it's technically legal. However it's painful when you try to apply
> > more aggressive warning flags looking for real bugs.
> >    
> 
> No, this is GCC being stupid.

Nonsense, it would have been stupid if it warned without asking for this
warning, this is GCC being intelligent.

[..snip..]

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Re: [PATCH 10/14] Zero json struct with memset() instea of = {} to keep compiler happy.

[Avi Kivity]

  On 08/30/2010 06:53 PM, Jes Sorensen wrote:
> On 08/30/10 17:48, Anthony Liguori wrote:
>> On 08/30/2010 10:43 AM, Jes Sorensen wrote:
>>> Yes it's technically legal. However it's painful when you try to apply
>>> more aggressive warning flags looking for real bugs.
>> No, this is GCC being stupid.
>>
>>> I would suggest we modify the coding style to ask people to not init a
>>> struct like this.
>>>
>> How else do you terminate a list?  IOW:
>>
>> MyDeviceInfo device_infos[] = {
>>    {"foo", 0, 2},
>>    {"bar", 0, 1},
>>    {} /* or { 0 } */
>> };
>>
>> This is such a pervasive idiom that there's simply no way that GCC can
>> possibly try to warn against this.  Plus, it's entirely reasonable.
>>
>> I think this is just a false positive in GCC.  Otherwise, there's a ton
>> of code that it should be throwing warnings against
> I believe the comma after the last case takes care of terminating the list.

It only makes patches that add items prettier.

> I agree that it would be nice to get gcc to not moan about this specific
> case, however I will argue that my change is worth it to be able to use
> the error flags, even if it is gcc being stupid.

If the flags make gcc stupid, why use them?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] Re: [PATCH 11/14] Remove unused function arguments

[Anthony Liguori]

On 08/30/2010 11:24 AM, Paolo Bonzini wrote:
> On 08/30/2010 05:35 PM, Jes.Sorensen@redhat.com wrote:
>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> I think it's better to leave ctxt in for future extensibility.
>
> You can mark it as __attribute__((unused)) to suppress the warning.

That seriously ugifies the code IMHO.

Regards,

Anthony Liguori

> Paolo
>

Re: [Qemu-devel] Re: [PATCH 04/14] Zero initialize timespec struct explicitly

[Jes Sorensen]

On 08/30/10 18:56, malc wrote:
> On Mon, 30 Aug 2010, Anthony Liguori wrote:
> 
>> On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
>>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>>> diff --git a/linux-aio.c b/linux-aio.c
>>> index 68f4b3d..3240996 100644
>>> --- a/linux-aio.c
>>> +++ b/linux-aio.c
>>> @@ -118,7 +118,7 @@ static void qemu_laio_completion_cb(void *opaque)
>>>           struct io_event events[MAX_EVENTS];
>>>           uint64_t val;
>>>           ssize_t ret;
>>> -        struct timespec ts = { 0 };
>>> +        struct timespec ts = { 0, 0 };
>>>    
>>
>> I don't like these.  What's wrong with { } or { 0 }?  Implicit zeroing of
>> members is a critical feature of structure initialization so if there is
>> something wrong with this, it's important to know why because otherwise we've
>> got a massive amount of broken code.
>>
> 
> Apart from gcc complaining about fields not being initialized explicitly
> there's nothing wrong with it.

Sure it compiles, it works, but it's not pretty. What does it mean if
you write = { 1 } in the above case?

Anyway the whole point of my patch is this, if we fix things like these,
we are much more likely to be able to catch real bugs using some of
gcc's checking. The patch I submitted is harmless code wise, but it
makes it that bit easier to catch other bugs in the code. In my book
that adds a lot of value.

Jes

Re: [Qemu-devel] Re: [PATCH 04/14] Zero initialize timespec struct explicitly

[Anthony Liguori]

On 08/30/2010 12:38 PM, Jes Sorensen wrote:
> On 08/30/10 18:56, malc wrote:
>    
>> On Mon, 30 Aug 2010, Anthony Liguori wrote:
>>
>>      
>>> On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
>>>        
>>>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>>>> diff --git a/linux-aio.c b/linux-aio.c
>>>> index 68f4b3d..3240996 100644
>>>> --- a/linux-aio.c
>>>> +++ b/linux-aio.c
>>>> @@ -118,7 +118,7 @@ static void qemu_laio_completion_cb(void *opaque)
>>>>            struct io_event events[MAX_EVENTS];
>>>>            uint64_t val;
>>>>            ssize_t ret;
>>>> -        struct timespec ts = { 0 };
>>>> +        struct timespec ts = { 0, 0 };
>>>>
>>>>          
>>> I don't like these.  What's wrong with { } or { 0 }?  Implicit zeroing of
>>> members is a critical feature of structure initialization so if there is
>>> something wrong with this, it's important to know why because otherwise we've
>>> got a massive amount of broken code.
>>>
>>>        
>> Apart from gcc complaining about fields not being initialized explicitly
>> there's nothing wrong with it.
>>      
> Sure it compiles, it works, but it's not pretty. What does it mean if
> you write = { 1 } in the above case?
>    

Initialize first field to 1 and all remaining fields to 0.

That's precisely what it means and there's a lot of code written today 
that relies on this behavior.  No doubt, c99 initializers are an 
improvement but { 0 } still looks better to me than {}.

However, I wouldn't object to replacing {0} with {}.  Avoiding {} in 
favor of memset is crazy though.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 04/14] Zero initialize timespec struct explicitly

[malc]

On Mon, 30 Aug 2010, Jes Sorensen wrote:

> On 08/30/10 18:56, malc wrote:
> > On Mon, 30 Aug 2010, Anthony Liguori wrote:
> > 
> >> On 08/30/2010 10:35 AM, Jes.Sorensen@redhat.com wrote:
> >>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
> >>> diff --git a/linux-aio.c b/linux-aio.c
> >>> index 68f4b3d..3240996 100644
> >>> --- a/linux-aio.c
> >>> +++ b/linux-aio.c
> >>> @@ -118,7 +118,7 @@ static void qemu_laio_completion_cb(void *opaque)
> >>>           struct io_event events[MAX_EVENTS];
> >>>           uint64_t val;
> >>>           ssize_t ret;
> >>> -        struct timespec ts = { 0 };
> >>> +        struct timespec ts = { 0, 0 };
> >>>    
> >>
> >> I don't like these.  What's wrong with { } or { 0 }?  Implicit zeroing of
> >> members is a critical feature of structure initialization so if there is
> >> something wrong with this, it's important to know why because otherwise we've
> >> got a massive amount of broken code.
> >>
> > 
> > Apart from gcc complaining about fields not being initialized explicitly
> > there's nothing wrong with it.
> 
> Sure it compiles, it works, but it's not pretty. What does it mean if
> you write = { 1 } in the above case?

It means exactly what standard says it should mean, i.e. set tv_sec to 1
and tv_nsec to 0.

> 
> Anyway the whole point of my patch is this, if we fix things like these,
> we are much more likely to be able to catch real bugs using some of
> gcc's checking. The patch I submitted is harmless code wise, but it
> makes it that bit easier to catch other bugs in the code. In my book
> that adds a lot of value.

FWIW I wasn't arguing against the patch.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Re: [PATCH 10/14] Zero json struct with memset() instea of = {} to keep compiler happy.

[Jes Sorensen]

On 08/30/10 18:55, Nathan Froyd wrote:
> On Mon, Aug 30, 2010 at 10:48:55AM -0500, Anthony Liguori wrote:
>> No, this is GCC being stupid.
>>
>> How else do you terminate a list?  IOW:
>>
>> MyDeviceInfo device_infos[] = {
>>   {"foo", 0, 2},
>>   {"bar", 0, 1},
>>   {} /* or { 0 } */
>> };
>>
>> This is such a pervasive idiom that there's simply no way that GCC can  
>> possibly try to warn against this.  Plus, it's entirely reasonable.
>>
>> I think this is just a false positive in GCC.  Otherwise, there's a ton  
>> of code that it should be throwing warnings against.
> 
> Well, it sounds like Jes was compiling QEMU was extra warning flags, and
> I doubt people do much beyond -Wall and maybe one or two others.

I was! I am not arguing that we should always compile QEMU with such
aggressive flags, but I my search did turn up several genuine bugs. What
I do argue in favor of is when it makes sense to make small IMHO
harmless changes to the code, that makes it easier to build QEMU with
aggressive C flags in order to catch real bugs.

Cheers,
Jes

Re: [Qemu-devel] Re: [PATCH 10/14] Zero json struct with memset() instea of = {} to keep compiler happy.

[Jes Sorensen]

On 08/30/10 18:29, Anthony Liguori wrote:
> On 08/30/2010 11:18 AM, Paolo Bonzini wrote:
>> It falls under the "missing fields in initializer" warning.  Arguably,
>> an empty initializer should be special cased, but it isn't.
> 
> So the warning is for old style initializer lists?  I disagree that it's
> a valid warning.  First, {} is ambiguous as it can be an empty list of
> c99 initializers and an empty list of c89 initializers.
> 
> But even for c89 initializers, it's very common practice to omit
> initializers and rely on the defaulted value.  For instance, { 0 } is
> quite pervasive as an idiom.
> 
>>
>> I agree that Jes's original patch is ugly, but the C99 initializer is
>> an improvement.
> 
> Yes, I'm fine with your patch on it's own but I disagree with GCC's
> warnings here.

So if we pulled in Paolo's version instead, would you be happy with that?

Personally I find the memset approach prettier since it's explicit what
it does, but that is obviously down to personal preference.

Cheers,
Jes

Re: [Qemu-devel] [PATCH 00/14] gcc extra warning fixes

[Blue Swirl]

On Mon, Aug 30, 2010 at 3:35 PM,  <Jes.Sorensen@redhat.com> wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> Hi,
>
> I started building QEMU with some more aggressive error flags to see
> what dropped out. I started fixing up some of the cases, removing
> unused arguments to functions, comparisons of unsigned types against
> negative values etc. and a few other minor changes to avoid compiler
> warnings.

If the patches make all such warnings disappear, you could also
consider the same approach as taken by
7ccfb2eb5f9d91bdb4139cb420a3b5f8deb2f6e8 and
ac41a6206fe9e1506010cd0aa9cf56ed3b37ae19.

If a GCC flag can be enabled, it should not be just -Wextra, because
that means different things on different GCC versions, so please check
also
 a316e3788df2781fda119e801e9b3d753f89b752.

Re: [Qemu-devel] Re: [PATCH 10/14] Zero json struct with memset() instea of = {} to keep compiler happy.

[Richard Henderson]

On 08/30/2010 08:48 AM, Anthony Liguori wrote:
> MyDeviceInfo device_infos[] = { {"foo", 0, 2}, {"bar", 0, 1}, {} /*
> or { 0 } */ };
> 
> This is such a pervasive idiom that there's simply no way that GCC
> can possibly try to warn against this.

GCC only warns for this if you explicitly ask for it.
I.e. -Wmissing-field-initializers.

That flag is included in -Wextra, which Jes used.

Traditionally this flag is used when you are initializing structures
like the above, but you make a change to the structure involved, so
you may need to update all initializers to take the new field into
account.  Some projects have found this idiom useful.

That said, today I'd not bother with this flag and use designated
initializers now.


r~

Re: [Qemu-devel] [PATCH 00/14] gcc extra warning fixes

[Jes Sorensen]

On 08/30/10 21:25, Blue Swirl wrote:
> On Mon, Aug 30, 2010 at 3:35 PM,  <Jes.Sorensen@redhat.com> wrote:
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> Hi,
>>
>> I started building QEMU with some more aggressive error flags to see
>> what dropped out. I started fixing up some of the cases, removing
>> unused arguments to functions, comparisons of unsigned types against
>> negative values etc. and a few other minor changes to avoid compiler
>> warnings.
> 
> If the patches make all such warnings disappear, you could also
> consider the same approach as taken by
> 7ccfb2eb5f9d91bdb4139cb420a3b5f8deb2f6e8 and
> ac41a6206fe9e1506010cd0aa9cf56ed3b37ae19.
> 
> If a GCC flag can be enabled, it should not be just -Wextra, because
> that means different things on different GCC versions, so please check
> also
>  a316e3788df2781fda119e801e9b3d753f89b752.

The one GCC flag I think we should enable, and probably was the one that
caught the most bugs in my patch set is this one: -Wtype-limits

There was only one false positive with this flag in block/blkdebug.c
where it bails on a < 0 comparison of an enum. enum doesn't seem to be
defined to being signed or unsigned, but a quick cast in that case would
make that single false positive go away.

Cheers,
Jes

Re: [Qemu-devel] [PATCH 00/14] gcc extra warning fixes

[Blue Swirl]

On Tue, Aug 31, 2010 at 7:21 AM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
> On 08/30/10 21:25, Blue Swirl wrote:
>> On Mon, Aug 30, 2010 at 3:35 PM,  <Jes.Sorensen@redhat.com> wrote:
>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>
>>> Hi,
>>>
>>> I started building QEMU with some more aggressive error flags to see
>>> what dropped out. I started fixing up some of the cases, removing
>>> unused arguments to functions, comparisons of unsigned types against
>>> negative values etc. and a few other minor changes to avoid compiler
>>> warnings.
>>
>> If the patches make all such warnings disappear, you could also
>> consider the same approach as taken by
>> 7ccfb2eb5f9d91bdb4139cb420a3b5f8deb2f6e8 and
>> ac41a6206fe9e1506010cd0aa9cf56ed3b37ae19.
>>
>> If a GCC flag can be enabled, it should not be just -Wextra, because
>> that means different things on different GCC versions, so please check
>> also
>>  a316e3788df2781fda119e801e9b3d753f89b752.
>
> The one GCC flag I think we should enable, and probably was the one that
> caught the most bugs in my patch set is this one: -Wtype-limits
>
> There was only one false positive with this flag in block/blkdebug.c
> where it bails on a < 0 comparison of an enum. enum doesn't seem to be
> defined to being signed or unsigned, but a quick cast in that case would
> make that single false positive go away.

IIRC sparse or clang also complained about MIPS enums which don't fit
signed 32 bits. The problem could be solved by adding
BLKDEBUG_EVENT_MIN=1 and changing the comparison to check against
that.

But I found more cases in linux-user/{syscall,mmap,flatload}.c and
target-{mips,ppc}/op_helper.c. These should be fixed too before
enabling the flag.