--- src/esx/esx_storage_driver.c | 39 +++------------------------------------ 1 files changed, 3 insertions(+), 36 deletions(-) diff --git a/src/esx/esx_storage_driver.c b/src/esx/esx_storage_driver.c index d09831a..84f0339 100644 --- a/src/esx/esx_storage_driver.c +++ b/src/esx/esx_storage_driver.c @@ -70,42 +70,9 @@ esxStorageClose(virConnectPtr conn) static virStorageDriver esxStorageDriver = { - "ESX", /* name */ - esxStorageOpen, /* open */ - esxStorageClose, /* close */ - NULL, /* numOfPools */ - NULL, /* listPools */ - NULL, /* numOfDefinedPools */ - NULL, /* listDefinedPools */ - NULL, /* findPoolSources */ - NULL, /* poolLookupByName */ - NULL, /* poolLookupByUUID */ - NULL, /* poolLookupByVolume */ - NULL, /* poolCreateXML */ - NULL, /* poolDefineXML */ - NULL, /* poolBuild */ - NULL, /* poolUndefine */ - NULL, /* poolCreate */ - NULL, /* poolDestroy */ - NULL, /* poolDelete */ - NULL, /* poolRefresh */ - NULL, /* poolGetInfo */ - NULL, /* poolGetXMLDesc */ - NULL, /* poolGetAutostart */ - NULL, /* poolSetAutostart */ - NULL, /* poolNumOfVolumes */ - NULL, /* poolListVolumes */ - NULL, /* volLookupByName */ - NULL, /* volLookupByKey */ - NULL, /* volLookupByPath */ - NULL, /* volCreateXML */ - NULL, /* volCreateXMLFrom */ - NULL, /* volDelete */ - NULL, /* volGetInfo */ - NULL, /* volGetXMLDesc */ - NULL, /* volGetPath */ - NULL, /* poolIsActive */ - NULL, /* poolIsPersistent */ + .name = "ESX", + .open = esxStorageOpen, + .close = esxStorageClose }; -- 1.6.5.5

On 03/02/2010 05:33 PM, Matthias Bolte wrote: Ok, now I remember the thread; I thought it was odd that you had it in this style. I'll put it back to that style & add the field. Dave

On 03/03/2010 03:06 AM, Daniel Veillard wrote: Ok. I prefer the other style, but if that's what we're doing, I'll do it that way. Attached is a replacement patch [not an incremental]. Dave

On Tue, Mar 02, 2010 at 05:13:27PM -0500, David Allan wrote: How about we call this virStorageVolWipe() instead. I'm thinking perhaps some of the 'flags' we might add in te future would imply writing something that is not just zeros, at which point the current name is a little odd. eg we might have VIR_STORAGE_VOL_WIPE_ZEROS = 0, /* fill with zeros */ VIR_STORAGE_VOL_WIPE_PATTERN = 2 /* fill with 123456 */ VIR_STORAGE_VOL_WIPE_RANDOM = 1 /* fill with random data */ zeroes of course being the default Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--- src/libvirt.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 47 insertions(+), 0 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index d9242bc..1434874 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -8468,6 +8468,53 @@ error: /** + * virStorageVolZeroOut: + * @vol: pointer to storage volume + * @flags: future flags, use 0 for now + * + * Ensure that future reads from the storage volume return zeroes. + * + * Returns 0 on success, or -1 on error + */ +int +virStorageVolZeroOut(virStorageVolPtr vol, + unsigned int flags) +{ + virConnectPtr conn; + VIR_DEBUG("vol=%p, flags=%u", vol, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_STORAGE_VOL(vol)) { + virLibStorageVolError(NULL, VIR_ERR_INVALID_STORAGE_VOL, __FUNCTION__); + virDispatchError(NULL); + return (-1); + } + + conn = vol->conn; + if (conn->flags & VIR_CONNECT_RO) { + virLibStorageVolError(vol, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (conn->storageDriver && conn->storageDriver->volZeroOut) { + int ret; + ret = conn->storageDriver->volZeroOut(vol, flags); + if (ret < 0) { + goto error; + } + return ret; + } + + virLibConnError(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(vol->conn); + return -1; +} + + +/** * virStorageVolFree: * @vol: pointer to storage volume * -- 1.6.5.5

--- src/remote/remote_driver.c | 27 +++++++++++++++++++++++++++ 1 files changed, 27 insertions(+), 0 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b7b2e09..739f6bb 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -5538,6 +5538,32 @@ done: } static int +remoteStorageVolZeroOut(virStorageVolPtr vol, + unsigned int flags) +{ + int rv = -1; + remote_storage_vol_zero_out_args args; + struct private_data *priv = vol->conn->storagePrivateData; + + remoteDriverLock(priv); + + make_nonnull_storage_vol(&args.vol, vol); + args.flags = flags; + + if (call(vol->conn, priv, 0, REMOTE_PROC_STORAGE_VOL_ZERO_OUT, + (xdrproc_t) xdr_remote_storage_vol_zero_out_args, (char *) &args, + (xdrproc_t) xdr_void, (char *) NULL) == -1) + goto done; + + rv = 0; + +done: + remoteDriverUnlock(priv); + return rv; +} + + +static int remoteStorageVolGetInfo (virStorageVolPtr vol, virStorageVolInfoPtr info) { int rv = -1; @@ -9202,6 +9228,7 @@ static virStorageDriver storage_driver = { .volCreateXML = remoteStorageVolCreateXML, .volCreateXMLFrom = remoteStorageVolCreateXMLFrom, .volDelete = remoteStorageVolDelete, + .volZeroOut = remoteStorageVolZeroOut, .volGetInfo = remoteStorageVolGetInfo, .volGetXMLDesc = remoteStorageVolDumpXML, .volGetPath = remoteStorageVolGetPath, -- 1.6.5.5

--- src/storage/storage_driver.c | 218 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 218 insertions(+), 0 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 6b1045a..9e63689 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -26,6 +26,9 @@ #include <stdio.h> #include <unistd.h> #include <sys/types.h> +#include <sys/param.h> +#include <fcntl.h> + #if HAVE_PWD_H #include <pwd.h> #endif @@ -1518,6 +1521,220 @@ cleanup: return ret; } + +/* If the volume we're zeroing is already a sparse file, we simply + * truncate and extend it to its original size, filling it with + * zeroes. This behavior is guaranteed by POSIX: + * + * http://www.opengroup.org/onlinepubs/9699919799/functions/ftruncate.html + * + * If fildes refers to a regular file, the ftruncate() function shall + * cause the size of the file to be truncated to length. If the size + * of the file previously exceeded length, the extra data shall no + * longer be available to reads on the file. If the file previously + * was smaller than this size, ftruncate() shall increase the size of + * the file. If the file size is increased, the extended area shall + * appear as if it were zero-filled. + */ +static int +storageVolumeZeroSparseFile(virStorageVolDefPtr vol, + struct stat *st, + int fd) +{ + int ret = -1; + char errbuf[64]; + + 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))); + goto out; + } + + 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, + 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) +{ + int ret = -1, written; + size_t remaining, write_size; + char errbuf[64]; + + VIR_DEBUG("extent logical start: %zu len: %zu ", + extent_start, 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, + virStrerror(errno, errbuf, sizeof(errbuf))); + goto out; + } + + remaining = extent_length; + while (remaining > 0) { + + write_size = (st->st_blksize < remaining) ? st->st_blksize : 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 %d bytes)"), + vol->target.path, + virStrerror(errno, errbuf, sizeof(errbuf)), + write_size); + goto out; + } + + *bytes_zeroed += written; + remaining -= written; + } + + VIR_DEBUG("Wrote %zu bytes to volume with path '%s'", + *bytes_zeroed, vol->target.path); + + ret = 0; + +out: + return ret; +} + + +static int +storageVolumeZeroOutInternal(virStorageVolDefPtr def) +{ + int ret = -1, fd = -1; + char errbuf[64]; + struct stat st; + char *writebuf; + size_t bytes_zeroed = 0; + + VIR_DEBUG("Zeroing out 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))); + 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, + virStrerror(errno, errbuf, sizeof(errbuf))); + goto out; + } + + if (S_ISREG(st.st_mode) && st.st_blocks < (st.st_size / DEV_BSIZE)) { + ret = storageVolumeZeroSparseFile(def, &st, fd); + } else { + + if (VIR_ALLOC_N(writebuf, st.st_blksize) != 0) { + virReportOOMError(); + goto out; + } + + ret = storageZeroExtent(def, + &st, + fd, + 0, + def->allocation, + writebuf, + &bytes_zeroed); + } + +out: + VIR_FREE(writebuf); + + if (fd != -1) { + close(fd); + } + + return ret; +} + + +static int +storageVolumeZeroOut(virStorageVolPtr obj, + unsigned int flags ATTRIBUTE_UNUSED) +{ + virStorageDriverStatePtr driver = obj->conn->storagePrivateData; + virStoragePoolObjPtr pool; + virStorageVolDefPtr vol = NULL; + int ret = -1; + + storageDriverLock(driver); + pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); + storageDriverUnlock(driver); + + if (!pool) { + virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL, + "%s", _("no storage pool with matching uuid")); + goto out; + } + + if (!virStoragePoolObjIsActive(pool)) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("storage pool is not active")); + goto out; + } + + vol = virStorageVolDefFindByName(pool, obj->name); + + if (vol == NULL) { + virStorageReportError(VIR_ERR_NO_STORAGE_VOL, + _("no storage vol with matching name '%s'"), + obj->name); + goto out; + } + + if (vol->building) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + _("volume '%s' is still being allocated."), + vol->name); + goto out; + } + + if (storageVolumeZeroOutInternal(vol) == -1) { + goto out; + } + + ret = 0; + +out: + if (pool) { + virStoragePoolObjUnlock(pool); + } + + return ret; + +} + static int storageVolumeDelete(virStorageVolPtr obj, unsigned int flags) { @@ -1775,6 +1992,7 @@ static virStorageDriver storageDriver = { .volCreateXML = storageVolumeCreateXML, .volCreateXMLFrom = storageVolumeCreateXMLFrom, .volDelete = storageVolumeDelete, + .volZeroOut = storageVolumeZeroOut, .volGetInfo = storageVolumeGetInfo, .volGetXMLDesc = storageVolumeGetXMLDesc, .volGetPath = storageVolumeGetPath, -- 1.6.5.5

According to David Allan on 3/2/2010 3:13 PM: Unrelated to your patch, but is there any reason we don't use N_() as the idiomatic counterpart to _(), rather than spelling out gettext_noop everywhere? -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Wed, Mar 03, 2010 at 11:51:26AM -0500, Dave Allan wrote: Historical accident :-) Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Tue, Mar 02, 2010 at 05:13:34PM -0500, David Allan wrote: Following my API name suggestion, I'd call this 'vol-wipe' too Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|