[Qemu-devel] [PATCH 00/10] FORTIFY_SOURCE followup

[Qemu-devel] [PATCH 00/10] FORTIFY_SOURCE followup

[Juan Quintela]

Hi

This series make:
- all block *_create() functions return -errno instead of -1
- this makes that we can end writting errno/error at bdrv_create()
   callers (qemu-img)
- once there found a double free problem in the error handling of
  vmdk, fixed it.
- slirp: also check that system() was able to fork (amit noticed it)
- daemonize: if we are unable to write into the pipe, print a message and exit.
  We can't really recover from that error (amit noticed it).

Please review and apply.

Later, Juan.

Juan Quintela (10):
  cow: return errno instead of -1
  slirp: check system() success
  qcow2: return errno instead of -1
  qcow: return errno instead of -1
  vmdk: return errno instead of -1
  vl: exit if we are not able to write into the pipe
  vmdk: make vmdk_snapshot_create return -errno
  vmdk: fix double free
  vmdk: share cleanup code
  block: print errno on error

 block/cow.c   |    5 +--
 block/qcow.c  |    8 ++--
 block/qcow2.c |   18 +++++-----
 block/vmdk.c  |  106 +++++++++++++++++++++++++++++++++++++--------------------
 net/slirp.c   |    2 +-
 qemu-img.c    |    4 +-
 vl.c          |    1 +
 7 files changed, 88 insertions(+), 56 deletions(-)

[Qemu-devel] [PATCH 01/10] cow: return errno instead of -1

[Juan Quintela]

Remove not needed ret = 0 assignment.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 block/cow.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/block/cow.c b/block/cow.c
index 3733385..97e9745 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -224,7 +224,7 @@ static int cow_create(const char *filename, QEMUOptionParameter *options)
     cow_fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
               0644);
     if (cow_fd < 0)
-        return -1;
+        return -errno;
     memset(&cow_header, 0, sizeof(cow_header));
     cow_header.magic = cpu_to_be32(COW_MAGIC);
     cow_header.version = cpu_to_be32(COW_VERSION);
@@ -251,7 +251,7 @@ static int cow_create(const char *filename, QEMUOptionParameter *options)
     cow_header.size = cpu_to_be64(image_sectors * 512);
     ret = qemu_write_full(cow_fd, &cow_header, sizeof(cow_header));
     if (ret != sizeof(cow_header)) {
-        ret = -1;
+        ret = -errno;
         goto exit;
     }

@@ -262,7 +262,6 @@ static int cow_create(const char *filename, QEMUOptionParameter *options)
         goto exit;
     }

-    ret = 0;
 exit:
     close(cow_fd);
     return ret;
-- 
1.6.5.2

[Qemu-devel] [PATCH 02/10] slirp: check system() success

[Juan Quintela]

we shouldn't call W*() macros until we check that fork worked.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 net/slirp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/slirp.c b/net/slirp.c
index 317cca7..7f846ec 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -472,7 +472,7 @@ static void slirp_smb_cleanup(SlirpState *s)
     if (s->smb_dir[0] != '\0') {
         snprintf(cmd, sizeof(cmd), "rm -rf %s", s->smb_dir);
         ret = system(cmd);
-        if (!WIFEXITED(ret)) {
+        if (ret == -1 || !WIFEXITED(ret)) {
             qemu_error("'%s' failed.\n", cmd);
         } else if (WEXITSTATUS(ret)) {
             qemu_error("'%s' failed. Error code: %d\n",
-- 
1.6.5.2

[Qemu-devel] [PATCH 03/10] qcow2: return errno instead of -1

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 block/qcow2.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index bf8170e..5b6dad9 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -851,7 +851,7 @@ static int qcow_create2(const char *filename, int64_t total_size,

     fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
     if (fd < 0)
-        return -1;
+        return -errno;
     memset(&header, 0, sizeof(header));
     header.magic = cpu_to_be32(QCOW_MAGIC);
     header.version = cpu_to_be32(QCOW_VERSION);
@@ -930,7 +930,7 @@ static int qcow_create2(const char *filename, int64_t total_size,
     /* write all the data */
     ret = qemu_write_full(fd, &header, sizeof(header));
     if (ret != sizeof(header)) {
-        ret = -1;
+        ret = -errno;
         goto exit;
     }
     if (backing_file) {
@@ -943,25 +943,25 @@ static int qcow_create2(const char *filename, int64_t total_size,
             cpu_to_be32s(&ext_bf.len);
             ret = qemu_write_full(fd, &ext_bf, sizeof(ext_bf));
             if (ret != sizeof(ext_bf)) {
-                ret = -1;
+                ret = -errno;
                 goto exit;
             }
             ret = qemu_write_full(fd, backing_format, backing_format_len);
             if (ret != backing_format_len) {
-                ret = -1;
+                ret = -errno;
                 goto exit;
             }
             if (padding > 0) {
                 ret = qemu_write_full(fd, zero, padding);
                 if (ret != padding) {
-                    ret = -1;
+                    ret = -errno;
                     goto exit;
                 }
             }
         }
         ret = qemu_write_full(fd, backing_file, backing_filename_len);
         if (ret != backing_filename_len) {
-            ret = -1;
+            ret = -errno;
             goto exit;
         }
     }
@@ -970,14 +970,14 @@ static int qcow_create2(const char *filename, int64_t total_size,
     for(i = 0;i < l1_size; i++) {
         ret = qemu_write_full(fd, &tmp, sizeof(tmp));
         if (ret != sizeof(tmp)) {
-            ret = -1;
+            ret = -errno;
             goto exit;
         }
     }
     lseek(fd, s->refcount_table_offset, SEEK_SET);
     ret = qemu_write_full(fd, s->refcount_table, s->cluster_size);
     if (ret != s->cluster_size) {
-        ret = -1;
+        ret = -errno;
         goto exit;
     }

@@ -985,7 +985,7 @@ static int qcow_create2(const char *filename, int64_t total_size,
     ret = qemu_write_full(fd, s->refcount_block,
 		    ref_clusters * s->cluster_size);
     if (ret != ref_clusters * s->cluster_size) {
-        ret = -1;
+        ret = -errno;
         goto exit;
     }

-- 
1.6.5.2

[Qemu-devel] [PATCH 04/10] qcow: return errno instead of -1

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 block/qcow.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 003db1e..c619984 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -766,7 +766,7 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options)

     fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
     if (fd < 0)
-        return -1;
+        return -errno;
     memset(&header, 0, sizeof(header));
     header.magic = cpu_to_be32(QCOW_MAGIC);
     header.version = cpu_to_be32(QCOW_VERSION);
@@ -804,14 +804,14 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options)
     /* write all the data */
     ret = qemu_write_full(fd, &header, sizeof(header));
     if (ret != sizeof(header)) {
-        ret = -1;
+        ret = -errno;
         goto exit;
     }

     if (backing_file) {
         ret = qemu_write_full(fd, backing_file, backing_filename_len);
         if (ret != backing_filename_len) {
-            ret = -1;
+            ret = -errno;
             goto exit;
         }

@@ -821,7 +821,7 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options)
     for(i = 0;i < l1_size; i++) {
         ret = qemu_write_full(fd, &tmp, sizeof(tmp));
         if (ret != sizeof(tmp)) {
-            ret = -1;
+            ret = -errno;
             goto exit;
         }
     }
-- 
1.6.5.2

[Qemu-devel] [PATCH 05/10] vmdk: return errno instead of -1

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 block/vmdk.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 56c28a0..5b1d197 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -740,7 +740,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
     fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
               0644);
     if (fd < 0)
-        return -1;
+        return -errno;
     magic = cpu_to_be32(VMDK4_MAGIC);
     memset(&header, 0, sizeof(header));
     header.version = cpu_to_le32(1);
@@ -777,18 +777,18 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
     /* write all the data */
     ret = qemu_write_full(fd, &magic, sizeof(magic));
     if (ret != sizeof(magic)) {
-        ret = -1;
+        ret = -errno;
         goto exit;
     }
     ret = qemu_write_full(fd, &header, sizeof(header));
     if (ret != sizeof(header)) {
-        ret = -1;
+        ret = -errno;
         goto exit;
     }

     ret = ftruncate(fd, header.grain_offset << 9);
     if (ret < 0) {
-        ret = -1;
+        ret = -errno;
         goto exit;
     }

@@ -798,7 +798,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
          i < gt_count; i++, tmp += gt_size) {
         ret = qemu_write_full(fd, &tmp, sizeof(tmp));
         if (ret != sizeof(tmp)) {
-            ret = -1;
+            ret = -errno;
             goto exit;
         }
     }
@@ -809,7 +809,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
          i < gt_count; i++, tmp += gt_size) {
         ret = qemu_write_full(fd, &tmp, sizeof(tmp));
         if (ret != sizeof(tmp)) {
-            ret = -1;
+            ret = -errno;
             goto exit;
         }
     }
@@ -831,7 +831,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
     lseek(fd, le64_to_cpu(header.desc_offset) << 9, SEEK_SET);
     ret = qemu_write_full(fd, desc, strlen(desc));
     if (ret != strlen(desc)) {
-        ret = -1;
+        ret = -errno;
         goto exit;
     }

-- 
1.6.5.2

[Qemu-devel] [PATCH 06/10] vl: exit if we are not able to write into the pipe

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 vl.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/vl.c b/vl.c
index db7a178..119c7e4 100644
--- a/vl.c
+++ b/vl.c
@@ -5745,6 +5745,7 @@ int main(int argc, char **argv, char **envp)
             uint8_t status = 1;
             if (write(fds[1], &status, 1) != 1) {
                 perror("daemonize. Writing to pipe\n");
+                exit(1);
             }
         } else
 #endif
-- 
1.6.5.2

[Qemu-devel] Re: [PATCH 06/10] vl: exit if we are not able to write into the pipe

[Kevin Wolf]

Am 03.03.2010 13:06, schrieb Juan Quintela:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  vl.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index db7a178..119c7e4 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -5745,6 +5745,7 @@ int main(int argc, char **argv, char **envp)
>              uint8_t status = 1;
>              if (write(fds[1], &status, 1) != 1) {
>                  perror("daemonize. Writing to pipe\n");
> +                exit(1);
>              }
>          } else
>  #endif

This is already the handling code for a fatal error, it's just trying to
tell the parent process that it went wrong. Completing the context:

            fprintf(stderr, "Could not acquire pid file: %s\n",
strerror(errno));
        exit(1);
    }

So there is already an exit(1), this patch doesn't change anything.

Kevin

[Qemu-devel] Re: [PATCH 06/10] vl: exit if we are not able to write into the pipe

[Juan Quintela]

Kevin Wolf <kwolf@redhat.com> wrote:
> Am 03.03.2010 13:06, schrieb Juan Quintela:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  vl.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>> 
>> diff --git a/vl.c b/vl.c
>> index db7a178..119c7e4 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -5745,6 +5745,7 @@ int main(int argc, char **argv, char **envp)
>>              uint8_t status = 1;
>>              if (write(fds[1], &status, 1) != 1) {
>>                  perror("daemonize. Writing to pipe\n");
>> +                exit(1);
>>              }
>>          } else
>>  #endif
>
> This is already the handling code for a fatal error, it's just trying to
> tell the parent process that it went wrong. Completing the context:
>
>             fprintf(stderr, "Could not acquire pid file: %s\n",
> strerror(errno));
>         exit(1);
>     }
>
> So there is already an exit(1), this patch doesn't change anything.
>
> Kevin

Else without braces :(

You are right.  Would remove on respin.

Later, Juan.

[Qemu-devel] [PATCH 07/10] vmdk: make vmdk_snapshot_create return -errno

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 block/vmdk.c |   79 ++++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 58 insertions(+), 21 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 5b1d197..67a690e 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -187,6 +187,7 @@ static int vmdk_is_cid_valid(BlockDriverState *bs)
 static int vmdk_snapshot_create(const char *filename, const char *backing_file)
 {
     int snp_fd, p_fd;
+    int ret;
     uint32_t p_cid;
     char *p_name, *gd_buf, *rgd_buf;
     const char *real_filename, *temp_str;
@@ -211,35 +212,49 @@ static int vmdk_snapshot_create(const char *filename, const char *backing_file)

     snp_fd = open(filename, O_RDWR | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, 0644);
     if (snp_fd < 0)
-        return -1;
+        return -errno;
     p_fd = open(backing_file, O_RDONLY | O_BINARY | O_LARGEFILE);
     if (p_fd < 0) {
         close(snp_fd);
-        return -1;
+        return -errno;
     }

     /* read the header */
-    if (lseek(p_fd, 0x0, SEEK_SET) == -1)
+    if (lseek(p_fd, 0x0, SEEK_SET) == -1) {
+        ret = -errno;
         goto fail;
-    if (read(p_fd, hdr, HEADER_SIZE) != HEADER_SIZE)
+    }
+    if (read(p_fd, hdr, HEADER_SIZE) != HEADER_SIZE) {
+        ret = -errno;
         goto fail;
+    }

     /* write the header */
-    if (lseek(snp_fd, 0x0, SEEK_SET) == -1)
+    if (lseek(snp_fd, 0x0, SEEK_SET) == -1) {
+        ret = -errno;
         goto fail;
-    if (write(snp_fd, hdr, HEADER_SIZE) == -1)
+    }
+    if (write(snp_fd, hdr, HEADER_SIZE) == -1) {
+        ret = -errno;
         goto fail;
+    }

     memset(&header, 0, sizeof(header));
     memcpy(&header,&hdr[4], sizeof(header)); // skip the VMDK4_MAGIC

-    if (ftruncate(snp_fd, header.grain_offset << 9))
+    if (ftruncate(snp_fd, header.grain_offset << 9)) {
+        ret = -errno;
         goto fail;
+    }
     /* the descriptor offset = 0x200 */
-    if (lseek(p_fd, 0x200, SEEK_SET) == -1)
+    if (lseek(p_fd, 0x200, SEEK_SET) == -1) {
+        ret = -errno;
         goto fail;
-    if (read(p_fd, p_desc, DESC_SIZE) != DESC_SIZE)
+    }
+    if (read(p_fd, p_desc, DESC_SIZE) != DESC_SIZE) {
+        ret = -errno;
         goto fail;
+    }

     if ((p_name = strstr(p_desc,"CID")) != NULL) {
         p_name += sizeof("CID");
@@ -258,10 +273,14 @@ static int vmdk_snapshot_create(const char *filename, const char *backing_file)
              (uint32_t)header.capacity, real_filename);

     /* write the descriptor */
-    if (lseek(snp_fd, 0x200, SEEK_SET) == -1)
+    if (lseek(snp_fd, 0x200, SEEK_SET) == -1) {
+        ret = -errno;
         goto fail;
-    if (write(snp_fd, s_desc, strlen(s_desc)) == -1)
+    }
+    if (write(snp_fd, s_desc, strlen(s_desc)) == -1) {
+        ret = -errno;
         goto fail;
+    }

     gd_offset = header.gd_offset * SECTOR_SIZE;     // offset of GD table
     rgd_offset = header.rgd_offset * SECTOR_SIZE;   // offset of RGD table
@@ -271,33 +290,51 @@ static int vmdk_snapshot_create(const char *filename, const char *backing_file)
      * 512 GTE per GT, each GTE points to grain
      */
     gt_size = (int64_t)header.num_gtes_per_gte * header.granularity * SECTOR_SIZE;
-    if (!gt_size)
+    if (!gt_size) {
+        ret = -EINVAL;
         goto fail;
+    }
     gde_entries = (uint32_t)(capacity / gt_size);  // number of gde/rgde
     gd_size = gde_entries * sizeof(uint32_t);

     /* write RGD */
     rgd_buf = qemu_malloc(gd_size);
-    if (lseek(p_fd, rgd_offset, SEEK_SET) == -1)
+    if (lseek(p_fd, rgd_offset, SEEK_SET) == -1) {
+        ret = -errno;
         goto fail_rgd;
-    if (read(p_fd, rgd_buf, gd_size) != gd_size)
+    }
+    if (read(p_fd, rgd_buf, gd_size) != gd_size) {
+        ret = -errno;
         goto fail_rgd;
-    if (lseek(snp_fd, rgd_offset, SEEK_SET) == -1)
+    }
+    if (lseek(snp_fd, rgd_offset, SEEK_SET) == -1) {
+        ret = -errno;
         goto fail_rgd;
-    if (write(snp_fd, rgd_buf, gd_size) == -1)
+    }
+    if (write(snp_fd, rgd_buf, gd_size) == -1) {
+        ret = -errno;
         goto fail_rgd;
+    }
     qemu_free(rgd_buf);

     /* write GD */
     gd_buf = qemu_malloc(gd_size);
-    if (lseek(p_fd, gd_offset, SEEK_SET) == -1)
+    if (lseek(p_fd, gd_offset, SEEK_SET) == -1) {
+        ret = -errno;
         goto fail_gd;
-    if (read(p_fd, gd_buf, gd_size) != gd_size)
+    }
+    if (read(p_fd, gd_buf, gd_size) != gd_size) {
+        ret = -errno;
         goto fail_gd;
-    if (lseek(snp_fd, gd_offset, SEEK_SET) == -1)
+    }
+    if (lseek(snp_fd, gd_offset, SEEK_SET) == -1) {
+        ret = -errno;
         goto fail_gd;
-    if (write(snp_fd, gd_buf, gd_size) == -1)
+    }
+    if (write(snp_fd, gd_buf, gd_size) == -1) {
+        ret = -errno;
         goto fail_gd;
+    }
     qemu_free(gd_buf);

     close(p_fd);
@@ -311,7 +348,7 @@ static int vmdk_snapshot_create(const char *filename, const char *backing_file)
     fail:
     close(p_fd);
     close(snp_fd);
-    return -1;
+    return ret;
 }

 static void vmdk_parent_close(BlockDriverState *bs)
-- 
1.6.5.2

[Qemu-devel] [PATCH 08/10] vmdk: fix double free

[Juan Quintela]

fail_gd error case would also free rgd_buf that was already freed

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

diff --git a/block/vmdk.c b/block/vmdk.c
index 67a690e..819c1c9 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -315,7 +315,6 @@ static int vmdk_snapshot_create(const char *filename, const char *backing_file)
         ret = -errno;
         goto fail_rgd;
     }
-    qemu_free(rgd_buf);

     /* write GD */
     gd_buf = qemu_malloc(gd_size);
@@ -336,6 +335,7 @@ static int vmdk_snapshot_create(const char *filename, const char *backing_file)
         goto fail_gd;
     }
     qemu_free(gd_buf);
+    qemu_free(rgd_buf);

     close(p_fd);
     close(snp_fd);
-- 
1.6.5.2

[Qemu-devel] [PATCH 09/10] vmdk: share cleanup code

[Juan Quintela]

cleanup code is identical for error/success cases.  Only difference
are goto labels.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 block/vmdk.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 819c1c9..007fca4 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -334,18 +334,13 @@ static int vmdk_snapshot_create(const char *filename, const char *backing_file)
         ret = -errno;
         goto fail_gd;
     }
-    qemu_free(gd_buf);
-    qemu_free(rgd_buf);
-
-    close(p_fd);
-    close(snp_fd);
-    return 0;
+    ret = 0;

-    fail_gd:
+fail_gd:
     qemu_free(gd_buf);
-    fail_rgd:
+fail_rgd:
     qemu_free(rgd_buf);
-    fail:
+fail:
     close(p_fd);
     close(snp_fd);
     return ret;
-- 
1.6.5.2

[Qemu-devel] [PATCH 10/10] block: print errno on error

[Juan Quintela]

Now that we changed all create calls to return errno, just print it.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 qemu-img.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 0c9f2d4..f6c40fb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -374,7 +374,7 @@ static int img_create(int argc, char **argv)
         } else if (ret == -EFBIG) {
             error("The image size is too large for file format '%s'", fmt);
         } else {
-            error("Error while formatting");
+            error("Error (%s) while formatting for file format '%s'", strerror(ret), fmt);
         }
     }
     return 0;
@@ -687,7 +687,7 @@ static int img_convert(int argc, char **argv)
         } else if (ret == -EFBIG) {
             error("The image size is too large for file format '%s'", out_fmt);
         } else {
-            error("Error while formatting '%s'", out_filename);
+            error("Error (%s) while formatting file '%s'", strerror(ret), out_filename);
         }
     }

-- 
1.6.5.2

[Qemu-devel] Re: [PATCH 10/10] block: print errno on error

[Kevin Wolf]

Am 03.03.2010 13:07, schrieb Juan Quintela:
> Now that we changed all create calls to return errno, just print it.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  qemu-img.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 0c9f2d4..f6c40fb 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -374,7 +374,7 @@ static int img_create(int argc, char **argv)
>          } else if (ret == -EFBIG) {
>              error("The image size is too large for file format '%s'", fmt);
>          } else {
> -            error("Error while formatting");
> +            error("Error (%s) while formatting for file format '%s'", strerror(ret), fmt);
>          }
>      }
>      return 0;
> @@ -687,7 +687,7 @@ static int img_convert(int argc, char **argv)
>          } else if (ret == -EFBIG) {
>              error("The image size is too large for file format '%s'", out_fmt);
>          } else {
> -            error("Error while formatting '%s'", out_filename);
> +            error("Error (%s) while formatting file '%s'", strerror(ret), out_filename);
>          }
>      }
> 

I think it should be strerror(-ret) in both cases.

Kevin

[Qemu-devel] Re: [PATCH 10/10] block: print errno on error

[Paolo Bonzini]

>> diff --git a/qemu-img.c b/qemu-img.c
>> index 0c9f2d4..f6c40fb 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -374,7 +374,7 @@ static int img_create(int argc, char **argv)
>>           } else if (ret == -EFBIG) {
>>               error("The image size is too large for file format '%s'", fmt);
>>           } else {
>> -            error("Error while formatting");
>> +            error("Error (%s) while formatting for file format '%s'", strerror(ret), fmt);
>>           }
>>       }
>>       return 0;
>> @@ -687,7 +687,7 @@ static int img_convert(int argc, char **argv)
>>           } else if (ret == -EFBIG) {
>>               error("The image size is too large for file format '%s'", out_fmt);
>>           } else {
>> -            error("Error while formatting '%s'", out_filename);
>> +            error("Error (%s) while formatting file '%s'", strerror(ret), out_filename);
>>           }
>>       }
>>
>
> I think it should be strerror(-ret) in both cases.

Yes; also, since you are at it, I think that respectively

error("%s: error while creating %s image: %s", filename, fmt,
       strerror(-ret);

error(%s: error while converting to %s: %s", out_filename, fmt,
       strerror(-ret);

would be more consistent with usual error messages:

$ cat fdsfds
cat: fdsfds: No such file or directory

Paolo

[Qemu-devel] Re: [PATCH 10/10] block: print errno on error

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> diff --git a/qemu-img.c b/qemu-img.c
>>> index 0c9f2d4..f6c40fb 100644
>>> --- a/qemu-img.c
>>> +++ b/qemu-img.c
>>> @@ -374,7 +374,7 @@ static int img_create(int argc, char **argv)
>>>           } else if (ret == -EFBIG) {
>>>               error("The image size is too large for file format '%s'", fmt);
>>>           } else {
>>> -            error("Error while formatting");
>>> +            error("Error (%s) while formatting for file format '%s'", strerror(ret), fmt);
>>>           }
>>>       }
>>>       return 0;
>>> @@ -687,7 +687,7 @@ static int img_convert(int argc, char **argv)
>>>           } else if (ret == -EFBIG) {
>>>               error("The image size is too large for file format '%s'", out_fmt);
>>>           } else {
>>> -            error("Error while formatting '%s'", out_filename);
>>> +            error("Error (%s) while formatting file '%s'", strerror(ret), out_filename);
>>>           }
>>>       }
>>>
>>
>> I think it should be strerror(-ret) in both cases.

oops, yes.

> Yes; also, since you are at it, I think that respectively
>
> error("%s: error while creating %s image: %s", filename, fmt,
>       strerror(-ret);
>
> error(%s: error while converting to %s: %s", out_filename, fmt,
>       strerror(-ret);
>
> would be more consistent with usual error messages:
>
> $ cat fdsfds
> cat: fdsfds: No such file or directory

I agree.  I just didn't want to change it too much.

> Paolo

[Qemu-devel] [PATCH v2 00/10] FORTIFY_SOURCE followup

[Juan Quintela]

Hi

Series updated with yesterday comments

v2:
   - drop un-needed slirp exit() patch (kevin)
   - add qemu_write_full() documentation (danp)
   - use strerror(-errno) (kevin, pbonzini)

Please review and apply.

Later, Juan.


v1:
This series make:
- all block *_create() functions return -errno instead of -1
- this makes that we can end writting errno/error at bdrv_create()
   callers (qemu-img)
- once there found a double free problem in the error handling of
  vmdk, fixed it.
- slirp: also check that system() was able to fork (amit noticed it)
- daemonize: if we are unable to write into the pipe, print a message and exit.
  We can't really recover from that error (amit noticed it).

Juan Quintela (10):
  cow: return errno instead of -1
  slirp: check system() success
  qcow2: return errno instead of -1
  qcow: return errno instead of -1
  vmdk: return errno instead of -1
  vmdk: make vmdk_snapshot_create return -errno
  vmdk: fix double free
  vmdk: share cleanup code
  block: print errno on error
  documentation: qemu_write_full don't work with non-blocking fd's

 block/cow.c   |    5 +--
 block/qcow.c  |    8 ++--
 block/qcow2.c |   18 +++++-----
 block/vmdk.c  |  106 +++++++++++++++++++++++++++++++++++++--------------------
 net/slirp.c   |    2 +-
 osdep.c       |    5 +++
 qemu-img.c    |    4 +-
 7 files changed, 92 insertions(+), 56 deletions(-)

[Qemu-devel] [PATCH 02/10] slirp: check system() success

[Juan Quintela]

we shouldn't call W*() macros until we check that fork worked.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 net/slirp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/slirp.c b/net/slirp.c
index 317cca7..7f846ec 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -472,7 +472,7 @@ static void slirp_smb_cleanup(SlirpState *s)
     if (s->smb_dir[0] != '\0') {
         snprintf(cmd, sizeof(cmd), "rm -rf %s", s->smb_dir);
         ret = system(cmd);
-        if (!WIFEXITED(ret)) {
+        if (ret == -1 || !WIFEXITED(ret)) {
             qemu_error("'%s' failed.\n", cmd);
         } else if (WEXITSTATUS(ret)) {
             qemu_error("'%s' failed. Error code: %d\n",
-- 
1.6.6.1