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

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



On 03/17/2010 10:13 AM, Daniel Veillard wrote:
On Mon, Mar 15, 2010 at 10:13:30PM -0400, David Allan wrote:
---
  src/storage/storage_driver.c |  224 ++++++++++++++++++++++++++++++++++++++++++
  1 files changed, 224 insertions(+), 0 deletions(-)


[...]
+    char errbuf[64];
   While the 1024 used many places in the code is probably a bit too
much, 64 chars might be a bit small for an errno return description
I would bump it to 128,


Actually, you shouldn't need the errbuf at all - see my comments on the use of errbuf below.


+    ret = ftruncate(fd, 0);
+    if (ret == -1) {
+        virReportSystemError(ret,
+                             _("Failed to truncate volume with "
+                               "path '%s' to 0 bytes: '%s'"),
+                             vol->target.path,
+                             virStrerror(errno, errbuf, sizeof(errbuf)));

Since ftruncate returns a -1 on failure, and you're sending that return value to virRerportSystemError (which would barf on the -1) and then manually printing out the strerror(). Instead, this should be changed to:


               virReportSystemError(errno,

                             _("Failed to truncate volume with "
                             "path '%s' to 0 bytes"),
                             vol->target.path);


+        goto out;
+    }
+
+    ret = ftruncate(fd, size);
+    if (ret == -1) {
+        virReportSystemError(ret,
+                             _("Failed to truncate volume with "
+                               "path '%s' to %ju bytes: '%s'\n"),
+                             vol->target.path, (intmax_t)size,
+                             virStrerror(errno, errbuf, sizeof(errbuf)));

Likewise, this should be:

         virReportSystemError(errno,
                              _("Failed to truncate volume with "
                             "path '%s' to %ju bytes),
                             vol->target.path, (intmax_t)size);
                             virStrerror(errno, errbuf, sizeof(errbuf)));



+    }
+
+out:
+    return ret;
+}
+
+
+static int
+storageWipeExtent(virStorageVolDefPtr vol,
+                  int fd,
+                  off_t extent_start,
+                  off_t extent_length,
+                  char *writebuf,
+                  size_t writebuf_length,
+                  size_t *bytes_wiped)
+{
+    int ret = -1, written = 0;
   no need to initialize ret here

+    off_t remaining = 0;
+    size_t write_size = 0;
+    char errbuf[64];
   same 128 vs 64

Again, you shouldn't need errbuf at all.

+    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 %ju in volume "
+                               "with path '%s': '%s'"),
+                             (intmax_t)extent_start, vol->target.path,
+                             virStrerror(errno, errbuf, sizeof(errbuf)));

Same comment: lseek returns -1, not an errno. replace "ret" with errno, remove the final %s, and get rid of the virStrerror() arg.

+        goto out;
+    }
+
+    remaining = extent_length;
+    while (remaining>  0) {
+
+        write_size = (writebuf_length<  remaining) ? writebuf_length : remaining;
+        written = safewrite(fd, writebuf, write_size);
+        if (written<  0) {
+            virReportSystemError(written,
+                                 _("Failed to write to storage volume with "
+                                   "path '%s': '%s' "
+                                   "(attempted to write %zu bytes)"),
+                                 vol->target.path,
+                                 virStrerror(errno, errbuf, sizeof(errbuf)),
+                                 write_size);

Again - safewrite returns # of bytes written, or -1, not an errno. Replace "written" with errno, and get rid of virStrError() and the extra %s.


+            goto out;
+        }
+
+        *bytes_wiped += written;
+        remaining -= written;
+    }
+
+    VIR_DEBUG("Wrote %zu bytes to volume with path '%s'",
+              *bytes_wiped, vol->target.path);
+
+    ret = 0;
+
+out:
+    return ret;
+}
+
+
+static int
+storageVolumeWipeInternal(virStorageVolDefPtr def)
+{
+    int ret = -1, fd = -1;
+    char errbuf[64];

You don't need errbuf.

+    struct stat st;
+    char *writebuf = NULL;
+    size_t bytes_wiped = 0;
+
+    VIR_DEBUG("Wiping volume with path '%s'", def->target.path);
+
+    fd = open(def->target.path, O_RDWR);
+    if (fd == -1) {
+        VIR_ERROR("Failed to open storage volume with path '%s': '%s'",
+                  def->target.path,
+                  virStrerror(errno, errbuf, sizeof(errbuf)));

Not sure why you're using VIR_ERROR() + manually adding virStrerror() - isn't this the same thing as virReportSystemError?
+        goto out;
+    }
+
+    if (fstat(fd,&st) == -1) {
+        VIR_ERROR("Failed to stat storage volume with path '%s': '%s'",
+                  def->target.path,
+                  virStrerror(errno, errbuf, sizeof(errbuf)));

And again.



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