On 04/25/2017 03:06 AM, Michal Privoznik wrote:
> On 04/07/2017 06:30 PM, John Ferlan wrote:
>> Add bool 'zero_end' and logic that would allow a caller to wipe specific
>> portions of a target device either from the beginning (the default) or
>> from the end when zero_end is true.
>>
>> This will allow for this code to wipe out partition table information
>> from a device.
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>> src/storage/storage_util.c | 48 +++++++++++++++++++++++++++++++++-------------
>> 1 file changed, 35 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
>> index a2d89af..c1734e7 100644
>> --- a/src/storage/storage_util.c
>> +++ b/src/storage/storage_util.c
>> @@ -2516,24 +2516,44 @@ static int
>> storageBackendWipeLocal(const char *path,
>> int fd,
>> unsigned long long wipe_len,
>> - size_t writebuf_length)
>> + size_t writebuf_length,
>> + bool zero_end)
>> {
>> int ret = -1, written = 0;
>> unsigned long long remaining = 0;
>> + off_t size;
>> size_t write_size = 0;
>> char *writebuf = NULL;
>>
>> - VIR_DEBUG("wiping start: 0 len: %llu", wipe_len);
>> -
>> if (VIR_ALLOC_N(writebuf, writebuf_length) < 0)
>> goto cleanup;
>>
>> - if (lseek(fd, 0, SEEK_SET) < 0) {
>> - virReportSystemError(errno,
>> - _("Failed to seek to the start in volume "
>> - "with path '%s'"),
>> - path);
>> - goto cleanup;
>> + if (!zero_end) {
>> + if (lseek(fd, 0, SEEK_SET) < 0) {
>> + virReportSystemError(errno,
>> + _("Failed to seek to the start in volume "
>> + "with path '%s'"),
>> + path);
>> + goto cleanup;
>> + }
>> + VIR_DEBUG("wiping start: 0 len: %llu", wipe_len);
>> + } else {
>> + if ((size = lseek(fd, 0, SEEK_END)) == (off_t)-1) {
>> + virReportSystemError(errno,
>> + _("Failed to seek to the end in volume "
>> + "with path '%s'"),
>> + path);
>> + goto cleanup;
>> + }
>> + size -= wipe_len;
>> + if (lseek(fd, size, SEEK_SET) < 0) {
>> + virReportSystemError(errno,
>> + _("Failed to seek to %zd bytes in volume "
>> + "with path '%s'"),
>> + size, path);
>
> Is off_t really ssize_t? I think we should typecast it.
>
OK - I do (now) have a recollection of printing an off_t cause some sort
of issue for some arch, but I forget which one...
>> + goto cleanup;
>> + }
>
> Or, instead of these two seeks:
>
> if ((size = lseek(fd, -wipe_len, SEEK_END)) < 0) {
> virReportSystemError();
> goto cleanup;
> }
>
>> + VIR_DEBUG("wiping start: %zd len: %llu", size, wipe_len);
>
> This DEBUG is the same as in the other body for the if statement. While
> it's just VIR_DEBUG I wouldn't care, but just consider the following
> for a moment:
>
> Move this VIR_DEBUG right after this if statement and initialize @size
> to zero (and obviously drop the other VIR_DEBUG with hardcoded 0). That
> way we know what the current position is and how long section is to be
> wiped.
>
> Or even better:
>
> + if (!zero_end) {
> + if ((size = lseek(fd, 0, SEEK_SET)) < 0) {
> + virReportSystemError(errno,
> + _("Failed to seek to the start in volume "
> + "with path '%s'"),
> + path);
> + goto cleanup;
> + }
> + } else {
> + if ((size = lseek(fd, -wipe_len, SEEK_END)) < 0) {
> + virReportSystemError(errno,
> + _("Failed to seek to %llu bytes to the end in volume "
> + "with path '%s'"),
> + wipe_len, path);
> + goto cleanup;
> + }
> }
>
> + VIR_DEBUG("wiping start: %zd len: %llu", (ssize_t) size, wipe_len);
>
>
I'll go with this...
Thanks
John
>> }
>>
>> remaining = wipe_len;
>> @@ -2573,7 +2593,8 @@ storageBackendWipeLocal(const char *path,
>> static int
>> storageBackendVolWipeLocalFile(const char *path,
>> unsigned int algorithm,
>> - unsigned long long allocation)
>> + unsigned long long allocation,
>> + bool zero_end)
>> {
>> int ret = -1, fd = -1;
>> const char *alg_char = NULL;
>> @@ -2648,7 +2669,8 @@ storageBackendVolWipeLocalFile(const char *path,
>> if (S_ISREG(st.st_mode) && st.st_blocks < (st.st_size / DEV_BSIZE)) {
>> ret = storageBackendVolZeroSparseFileLocal(path, st.st_size, fd);
>> } else {
>> - ret = storageBackendWipeLocal(path, fd, allocation, st.st_blksize);
>> + ret = storageBackendWipeLocal(path, fd, allocation, st.st_blksize,
>> + zero_end);
>> }
>> if (ret < 0)
>> goto cleanup;
>> @@ -2686,7 +2708,7 @@ storageBackendVolWipePloop(virStorageVolDefPtr vol,
>> goto cleanup;
>>
>> if (storageBackendVolWipeLocalFile(target_path, algorithm,
>> - vol->target.allocation) < 0)
>> + vol->target.allocation, false) < 0)
>> goto cleanup;
>>
>> if (virFileRemove(disk_desc, 0, 0) < 0) {
>> @@ -2735,7 +2757,7 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED,
>> ret = storageBackendVolWipePloop(vol, algorithm);
>> } else {
>> ret = storageBackendVolWipeLocalFile(vol->target.path, algorithm,
>> - vol->target.allocation);
>> + vol->target.allocation, false);
>> }
>>
>> return ret;
>>
>
> Michal
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
