[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 8/9] Simplified version of volume zeroing based on feedback from the list.



On 03/02/2010 06:44 PM, Eric Blake wrote:
According to David Allan on 3/2/2010 3:13 PM:
+    ret = ftruncate(fd, st->st_size);
+    if (ret == -1) {
+        virReportSystemError(ret,
+                             _("Failed to truncate volume with "
+                               "path '%s' to %llu bytes: '%s'\n"),
+                             vol->target.path, st->st_size,

off_t is not guaranteed to be the same as long long.  You need a cast.

+                             virStrerror(errno, errbuf, sizeof(errbuf)));
+    }
+
+out:
+    return ret;
+}
+
+
+static int
+storageZeroExtent(virStorageVolDefPtr vol,
+                  struct stat *st,
+                  int fd,
+                  size_t extent_start,
+                  size_t extent_length,
+                  char *writebuf,
+                  size_t *bytes_zeroed)

Is size_t the right type for the extent, or do we want off_t?

+    size_t remaining, write_size;
...
+        if (written<  0) {
+            virReportSystemError(written,
+                                 _("Failed to write to storage volume with "
+                                   "path '%s': '%s' "
+                                   "(attempted to write %d bytes)"),
+                                 vol->target.path,
+                                 virStrerror(errno, errbuf, sizeof(errbuf)),
+                                 write_size);

%d and write_size are not compatible.

+    memset(&st, 0, sizeof(st));
+
+    if (fstat(fd,&st) == -1) {

The memset is wasted work.  stat() is sufficient for initializing st.


Good catches. Attached is an incremental patch that addresses all your comments. The funky indentation you pointed out is created by rpcgen, not our generation scripts, so I'm not going to attempt to address it at the moment.

Dave
>From bb7f29e134fb376567136786ed62f6252111c80a Mon Sep 17 00:00:00 2001
From: David Allan <dallan redhat com>
Date: Wed, 3 Mar 2010 00:28:56 -0500
Subject: [PATCH 1/1] Fixes per Eric Blake

---
 src/libvirt.c                |    5 +++++
 src/storage/storage_driver.c |   26 +++++++++++++-------------
 2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/src/libvirt.c b/src/libvirt.c
index 1434874..353b8d8 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -8497,6 +8497,11 @@ virStorageVolZeroOut(virStorageVolPtr vol,
         goto error;
     }

+    if (flags != 0) {
+        virLibStorageVolError(vol, VIR_ERR_INVALID_ARG, __FUNCTION__);
+        goto error;
+    }
+
     if (conn->storageDriver && conn->storageDriver->volZeroOut) {
         int ret;
         ret = conn->storageDriver->volZeroOut(vol, flags);
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 9e63689..b23353d 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1558,8 +1558,8 @@ storageVolumeZeroSparseFile(virStorageVolDefPtr vol,
     if (ret == -1) {
         virReportSystemError(ret,
                              _("Failed to truncate volume with "
-                               "path '%s' to %llu bytes: '%s'\n"),
-                             vol->target.path, st->st_size,
+                               "path '%s' to %ju bytes: '%s'\n"),
+                             vol->target.path, (intmax_t)st->st_size,
                              virStrerror(errno, errbuf, sizeof(errbuf)));
     }

@@ -1572,22 +1572,24 @@ static int
 storageZeroExtent(virStorageVolDefPtr vol,
                   struct stat *st,
                   int fd,
-                  size_t extent_start,
-                  size_t extent_length,
+                  off_t extent_start,
+                  off_t extent_length,
                   char *writebuf,
                   size_t *bytes_zeroed)
 {
     int ret = -1, written;
-    size_t remaining, write_size;
+    off_t remaining;
+    size_t write_size;
     char errbuf[64];

-    VIR_DEBUG("extent logical start: %zu len: %zu ",
-              extent_start, extent_length);
+    VIR_DEBUG("extent logical start: %ju len: %ju",
+              (intmax_t)extent_start, (intmax_t)extent_length);

     if ((ret = lseek(fd, extent_start, SEEK_SET)) < 0) {
-        virReportSystemError(ret, "Failed to seek to position %zu in volume "
-                             "with path '%s': '%s'",
-                             extent_start, vol->target.path,
+        virReportSystemError(ret,
+                             _("Failed to seek to position %ju in volume "
+                               "with path '%s': '%s'"),
+                             (intmax_t)extent_start, vol->target.path,
                              virStrerror(errno, errbuf, sizeof(errbuf)));
         goto out;
     }
@@ -1601,7 +1603,7 @@ storageZeroExtent(virStorageVolDefPtr vol,
             virReportSystemError(written,
                                  _("Failed to write to storage volume with "
                                    "path '%s': '%s' "
-                                   "(attempted to write %d bytes)"),
+                                   "(attempted to write %zu bytes)"),
                                  vol->target.path,
                                  virStrerror(errno, errbuf, sizeof(errbuf)),
                                  write_size);
@@ -1641,8 +1643,6 @@ storageVolumeZeroOutInternal(virStorageVolDefPtr def)
         goto out;
     }

-    memset(&st, 0, sizeof(st));
-
     if (fstat(fd, &st) == -1) {
         VIR_ERROR("Failed to stat storage volume with path '%s': '%s'",
                   def->target.path,
-- 
1.6.5.5


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]