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.