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

[Laine Stump]

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.