[libvirt] [PATCH 0/9] Vol Zeroing API
This patch set contains a new API for zeroing out volumes. I called it zero out rather than overwrite because it doesn't overwrite anything in the case of sparse files. It does guarantee that any subsequent reads from the volume will return zeroes. The ESX driver change is part of the patch. Without it, the struct is missing its initializer and fails to build. I'm not sure what our position on the style of struct initializers is; there are both forms in the code. If this style is heresy, we should add a statement to that effect in HACKING. Dave David Allan (9): Fix ESX storage driver struct initializer Add public API for volume zeroing Define the internal driver API for vol zeroing Implement the public API for vol zeroing Define wire protocol format for vol zeroing Implement RPC client for vol zeroing Implement the remote dispatch bits of vol zeroing Simplified version of volume zeroing based on feedback from the list. Virsh support for vol zeroing daemon/remote.c | 32 +++++ daemon/remote_dispatch_args.h | 1 + daemon/remote_dispatch_prototypes.h | 8 ++ daemon/remote_dispatch_table.h | 5 + include/libvirt/libvirt.h.in | 2 + src/driver.h | 5 + src/esx/esx_storage_driver.c | 39 +------ src/libvirt.c | 47 ++++++++ src/libvirt_public.syms | 1 + src/remote/remote_driver.c | 27 +++++ src/remote/remote_protocol.c | 11 ++ src/remote/remote_protocol.h | 9 ++ src/remote/remote_protocol.x | 8 +- src/storage/storage_driver.c | 218 +++++++++++++++++++++++++++++++++++ tools/virsh.c | 42 +++++++ 15 files changed, 418 insertions(+), 37 deletions(-)
--- 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
2010/3/2 David Allan <dallan@redhat.com>:
--- 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
There was some discussion on the list about which struct initialization style to use. The result was to prefer the old style, one argument was that it provides some form of todo list in the codebase itself. I even have a patch laying around that converts the dot-name style to the old style. Matthias
On 03/02/2010 05:33 PM, Matthias Bolte wrote:
2010/3/2 David Allan<dallan@redhat.com>:
--- 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
There was some discussion on the list about which struct initialization style to use. The result was to prefer the old style, one argument was that it provides some form of todo list in the codebase itself.
I even have a patch laying around that converts the dot-name style to the old style.
Matthias
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 Tue, Mar 02, 2010 at 09:13:17PM -0500, Dave Allan wrote:
On 03/02/2010 05:33 PM, Matthias Bolte wrote:
2010/3/2 David Allan<dallan@redhat.com>:
--- 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
There was some discussion on the list about which struct initialization style to use. The result was to prefer the old style, one argument was that it provides some form of todo list in the codebase itself.
I even have a patch laying around that converts the dot-name style to the old style.
Matthias
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.
Yep, the point is that with the old style you can immediately see what entry points from a driver are missing, thanks, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 03/03/2010 03:06 AM, Daniel Veillard wrote:
On Tue, Mar 02, 2010 at 09:13:17PM -0500, Dave Allan wrote:
On 03/02/2010 05:33 PM, Matthias Bolte wrote:
2010/3/2 David Allan<dallan@redhat.com>:
--- 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
There was some discussion on the list about which struct initialization style to use. The result was to prefer the old style, one argument was that it provides some form of todo list in the codebase itself.
I even have a patch laying around that converts the dot-name style to the old style.
Matthias
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.
Yep, the point is that with the old style you can immediately see what entry points from a driver are missing,
thanks,
Daniel
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
--- include/libvirt/libvirt.h.in | 2 ++ src/libvirt_public.syms | 1 + 2 files changed, 3 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 0d1b5b5..6f755c5 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1224,6 +1224,8 @@ virStorageVolPtr virStorageVolCreateXMLFrom (virStoragePoolPtr pool, unsigned int flags); int virStorageVolDelete (virStorageVolPtr vol, unsigned int flags); +int virStorageVolZeroOut (virStorageVolPtr vol, + unsigned int flags); int virStorageVolRef (virStorageVolPtr vol); int virStorageVolFree (virStorageVolPtr vol); diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 64e7505..260cbc1 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -356,6 +356,7 @@ LIBVIRT_0.7.7 { virConnectBaselineCPU; virDomainGetJobInfo; virDomainAbortJob; + virStorageVolZeroOut; } LIBVIRT_0.7.5; # .... define new API here using predicted next version number .... -- 1.6.5.5
On Tue, Mar 02, 2010 at 05:13:27PM -0500, David Allan wrote:
--- include/libvirt/libvirt.h.in | 2 ++ src/libvirt_public.syms | 1 + 2 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 0d1b5b5..6f755c5 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1224,6 +1224,8 @@ virStorageVolPtr virStorageVolCreateXMLFrom (virStoragePoolPtr pool, unsigned int flags); int virStorageVolDelete (virStorageVolPtr vol, unsigned int flags); +int virStorageVolZeroOut (virStorageVolPtr vol, + unsigned int flags); int virStorageVolRef (virStorageVolPtr vol); int virStorageVolFree (virStorageVolPtr vol);
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/driver.h | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/src/driver.h b/src/driver.h index 7b7dfea..f7b41e9 100644 --- a/src/driver.h +++ b/src/driver.h @@ -722,6 +722,10 @@ typedef int unsigned int flags); typedef int + (*virDrvStorageVolZeroOut) (virStorageVolPtr vol, + unsigned int flags); + +typedef int (*virDrvStorageVolGetInfo) (virStorageVolPtr vol, virStorageVolInfoPtr info); typedef char * @@ -790,6 +794,7 @@ struct _virStorageDriver { virDrvStorageVolCreateXML volCreateXML; virDrvStorageVolCreateXMLFrom volCreateXMLFrom; virDrvStorageVolDelete volDelete; + virDrvStorageVolZeroOut volZeroOut; virDrvStorageVolGetInfo volGetInfo; virDrvStorageVolGetXMLDesc volGetXMLDesc; virDrvStorageVolGetPath volGetPath; -- 1.6.5.5
--- 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
According to David Allan on 3/2/2010 3:13 PM:
+ * @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);
To be future-proof, we should explicitly reject non-zero flags. That way, if a newer client contacts an older server with a new flag supported only by the client, the client doesn't get the mistaken impression that the server can honor the flag. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Mar 02, 2010 at 04:31:19PM -0700, Eric Blake wrote:
According to David Allan on 3/2/2010 3:13 PM:
+ * @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);
To be future-proof, we should explicitly reject non-zero flags. That way, if a newer client contacts an older server with a new flag supported only by the client, the client doesn't get the mistaken impression that the server can honor the flag.
Agreed, but it should be done in the storage driver implementation, rather than this public API entry point. Since different implementations of the API may support different flags. ie put it in PATH 8 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/remote/remote_protocol.c | 11 +++++++++++ src/remote/remote_protocol.h | 9 +++++++++ src/remote/remote_protocol.x | 8 +++++++- 3 files changed, 27 insertions(+), 1 deletions(-) diff --git a/src/remote/remote_protocol.c b/src/remote/remote_protocol.c index 701acab..bd52be0 100644 --- a/src/remote/remote_protocol.c +++ b/src/remote/remote_protocol.c @@ -2286,6 +2286,17 @@ xdr_remote_storage_vol_delete_args (XDR *xdrs, remote_storage_vol_delete_args *o } bool_t +xdr_remote_storage_vol_zero_out_args (XDR *xdrs, remote_storage_vol_zero_out_args *objp) +{ + + if (!xdr_remote_nonnull_storage_vol (xdrs, &objp->vol)) + return FALSE; + if (!xdr_u_int (xdrs, &objp->flags)) + return FALSE; + return TRUE; +} + +bool_t xdr_remote_storage_vol_dump_xml_args (XDR *xdrs, remote_storage_vol_dump_xml_args *objp) { diff --git a/src/remote/remote_protocol.h b/src/remote/remote_protocol.h index e06d73f..f98f512 100644 --- a/src/remote/remote_protocol.h +++ b/src/remote/remote_protocol.h @@ -1295,6 +1295,12 @@ struct remote_storage_vol_delete_args { }; typedef struct remote_storage_vol_delete_args remote_storage_vol_delete_args; +struct remote_storage_vol_zero_out_args { + remote_nonnull_storage_vol vol; + u_int flags; +}; +typedef struct remote_storage_vol_zero_out_args remote_storage_vol_zero_out_args; + struct remote_storage_vol_dump_xml_args { remote_nonnull_storage_vol vol; u_int flags; @@ -1872,6 +1878,7 @@ enum remote_procedure { REMOTE_PROC_CPU_BASELINE = 162, REMOTE_PROC_DOMAIN_GET_JOB_INFO = 163, REMOTE_PROC_DOMAIN_ABORT_JOB = 164, + REMOTE_PROC_STORAGE_VOL_ZERO_OUT = 165, }; typedef enum remote_procedure remote_procedure; @@ -2110,6 +2117,7 @@ extern bool_t xdr_remote_storage_vol_create_xml_ret (XDR *, remote_storage_vol_ extern bool_t xdr_remote_storage_vol_create_xml_from_args (XDR *, remote_storage_vol_create_xml_from_args*); extern bool_t xdr_remote_storage_vol_create_xml_from_ret (XDR *, remote_storage_vol_create_xml_from_ret*); extern bool_t xdr_remote_storage_vol_delete_args (XDR *, remote_storage_vol_delete_args*); +extern bool_t xdr_remote_storage_vol_zero_out_args (XDR *, remote_storage_vol_zero_out_args*); extern bool_t xdr_remote_storage_vol_dump_xml_args (XDR *, remote_storage_vol_dump_xml_args*); extern bool_t xdr_remote_storage_vol_dump_xml_ret (XDR *, remote_storage_vol_dump_xml_ret*); extern bool_t xdr_remote_storage_vol_get_info_args (XDR *, remote_storage_vol_get_info_args*); @@ -2393,6 +2401,7 @@ extern bool_t xdr_remote_storage_vol_create_xml_ret (); extern bool_t xdr_remote_storage_vol_create_xml_from_args (); extern bool_t xdr_remote_storage_vol_create_xml_from_ret (); extern bool_t xdr_remote_storage_vol_delete_args (); +extern bool_t xdr_remote_storage_vol_zero_out_args (); extern bool_t xdr_remote_storage_vol_dump_xml_args (); extern bool_t xdr_remote_storage_vol_dump_xml_ret (); extern bool_t xdr_remote_storage_vol_get_info_args (); diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 5e33da5..d44da4d 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1169,6 +1169,11 @@ struct remote_storage_vol_delete_args { unsigned flags; }; +struct remote_storage_vol_zero_out_args { + remote_nonnull_storage_vol vol; + unsigned flags; +}; + struct remote_storage_vol_dump_xml_args { remote_nonnull_storage_vol vol; unsigned flags; @@ -1703,7 +1708,8 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_DETACH_DEVICE_FLAGS = 161, REMOTE_PROC_CPU_BASELINE = 162, REMOTE_PROC_DOMAIN_GET_JOB_INFO = 163, - REMOTE_PROC_DOMAIN_ABORT_JOB = 164 + REMOTE_PROC_DOMAIN_ABORT_JOB = 164, + REMOTE_PROC_STORAGE_VOL_ZERO_OUT = 165 /* * Notice how the entries are grouped in sets of 10 ? -- 1.6.5.5
According to David Allan on 3/2/2010 3:13 PM:
bool_t +xdr_remote_storage_vol_zero_out_args (XDR *xdrs, remote_storage_vol_zero_out_args *objp) +{ + + if (!xdr_remote_nonnull_storage_vol (xdrs, &objp->vol)) + return FALSE; + if (!xdr_u_int (xdrs, &objp->flags)) + return FALSE; + return TRUE;
Funky indentation. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 03/02/2010 06:33 PM, Eric Blake wrote:
According to David Allan on 3/2/2010 3:13 PM:
bool_t +xdr_remote_storage_vol_zero_out_args (XDR *xdrs, remote_storage_vol_zero_out_args *objp) +{ + + if (!xdr_remote_nonnull_storage_vol (xdrs,&objp->vol)) + return FALSE; + if (!xdr_u_int (xdrs,&objp->flags)) + return FALSE; + return TRUE;
Funky indentation.
Bleah...this is a generated file, and the funky indentation is throughout, not surprisingly. I'll fix it, and see if I can fix the generator as well. Dave
--- 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
* I had to remove daemon/remote_dispatch* and do a make remote.c in daemon in order to get the dispatch files regenerated properly. The make was failing before I did that. --- daemon/remote.c | 32 ++++++++++++++++++++++++++++++++ daemon/remote_dispatch_args.h | 1 + daemon/remote_dispatch_prototypes.h | 8 ++++++++ daemon/remote_dispatch_table.h | 5 +++++ 4 files changed, 46 insertions(+), 0 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index d4713b2..7621044 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4273,6 +4273,38 @@ remoteDispatchStorageVolDelete (struct qemud_server *server ATTRIBUTE_UNUSED, } static int +remoteDispatchStorageVolZeroOut(struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client ATTRIBUTE_UNUSED, + virConnectPtr conn, + remote_message_header *hdr ATTRIBUTE_UNUSED, + remote_error *rerr, + remote_storage_vol_zero_out_args *args, + void *ret ATTRIBUTE_UNUSED) +{ + int retval = -1; + virStorageVolPtr vol; + + vol = get_nonnull_storage_vol(conn, args->vol); + if (vol == NULL) { + remoteDispatchConnError(rerr, conn); + goto out; + } + + if (virStorageVolZeroOut(vol, args->flags) == -1) { + remoteDispatchConnError(rerr, conn); + goto out; + } + + retval = 0; + +out: + if (vol != NULL) { + virStorageVolFree(vol); + } + return retval; +} + +static int remoteDispatchStorageVolGetInfo (struct qemud_server *server ATTRIBUTE_UNUSED, struct qemud_client *client ATTRIBUTE_UNUSED, virConnectPtr conn, diff --git a/daemon/remote_dispatch_args.h b/daemon/remote_dispatch_args.h index f97155b..3ddbec4 100644 --- a/daemon/remote_dispatch_args.h +++ b/daemon/remote_dispatch_args.h @@ -140,3 +140,4 @@ remote_cpu_baseline_args val_remote_cpu_baseline_args; remote_domain_get_job_info_args val_remote_domain_get_job_info_args; remote_domain_abort_job_args val_remote_domain_abort_job_args; + remote_storage_vol_zero_out_args val_remote_storage_vol_zero_out_args; diff --git a/daemon/remote_dispatch_prototypes.h b/daemon/remote_dispatch_prototypes.h index b81c8c3..db8e323 100644 --- a/daemon/remote_dispatch_prototypes.h +++ b/daemon/remote_dispatch_prototypes.h @@ -1298,6 +1298,14 @@ static int remoteDispatchStorageVolLookupByPath( remote_error *err, remote_storage_vol_lookup_by_path_args *args, remote_storage_vol_lookup_by_path_ret *ret); +static int remoteDispatchStorageVolZeroOut( + struct qemud_server *server, + struct qemud_client *client, + virConnectPtr conn, + remote_message_header *hdr, + remote_error *err, + remote_storage_vol_zero_out_args *args, + void *ret); static int remoteDispatchSupportsFeature( struct qemud_server *server, struct qemud_client *client, diff --git a/daemon/remote_dispatch_table.h b/daemon/remote_dispatch_table.h index 5ad6bff..11d9744 100644 --- a/daemon/remote_dispatch_table.h +++ b/daemon/remote_dispatch_table.h @@ -827,3 +827,8 @@ .args_filter = (xdrproc_t) xdr_remote_domain_abort_job_args, .ret_filter = (xdrproc_t) xdr_void, }, +{ /* StorageVolZeroOut => 165 */ + .fn = (dispatch_fn) remoteDispatchStorageVolZeroOut, + .args_filter = (xdrproc_t) xdr_remote_storage_vol_zero_out_args, + .ret_filter = (xdrproc_t) xdr_void, +}, -- 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:
+ 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,
off_t is not guaranteed to be the same as long long. You need a cast.
+ 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)
Is size_t the right type for the extent, or do we want off_t?
+ size_t remaining, 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);
%d and write_size are not compatible.
+ memset(&st, 0, sizeof(st)); + + if (fstat(fd, &st) == -1) {
The memset is wasted work. stat() is sufficient for initializing st. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 03/02/2010 06:44 PM, Eric Blake wrote:
According to David Allan on 3/2/2010 3:13 PM:
+ 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,
off_t is not guaranteed to be the same as long long. You need a cast.
+ 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)
Is size_t the right type for the extent, or do we want off_t?
+ size_t remaining, 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);
%d and write_size are not compatible.
+ memset(&st, 0, sizeof(st)); + + if (fstat(fd,&st) == -1) {
The memset is wasted work. stat() is sufficient for initializing st.
Good catches. Attached is an incremental patch that addresses all your comments. The funky indentation you pointed out is created by rpcgen, not our generation scripts, so I'm not going to attempt to address it at the moment. Dave
On Tue, Mar 02, 2010 at 05:13:33PM -0500, David Allan wrote:
@@ -1518,6 +1521,220 @@ cleanup: 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);
This is a little fragile, because its making storageZeroExtent() assume that the passed in 'writebuf' is st->st_blksize. I think it'd be better if a 'writebuflen' were passed in from the caller
+ 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));
That's redundant I believe - at least no other fstat calls ever memset their buffer ahead of time.
+ + 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);
Add st.st_blksize as a param here
+ } + +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;
As Eric suggested, this would be a good time to do an explicit 'flags == 0' check, so that callers can detect whether a flag is supported or not.
+ + 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, --
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 :|
David Allan wrote:
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c ...
Hi Dave,
+static int +storageVolumeZeroSparseFile(virStorageVolDefPtr vol, + struct stat *st, + int fd)
Since the only use of "st" is for st->st_size, please consider using a simpler "size_t size" parameter. If you opt to keep the "struct stat *" pointer parameter, it should be "const".
+{ + 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); ... +static int +storageZeroExtent(virStorageVolDefPtr vol, + struct stat *st, + int fd, + size_t extent_start, + size_t extent_length, + char *writebuf, + size_t *bytes_zeroed) +{
Since the only use of "st" is for st->st_blksize, please consider using a simpler "size_t blksize" parameter.
+ 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; ...
--- tools/virsh.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 42 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index a6a637d..e52b011 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5221,6 +5221,47 @@ cmdVolDelete(vshControl *ctl, const vshCmd *cmd) /* + * "vol-zero-out" command + */ +static const vshCmdInfo info_vol_zero_out[] = { + {"help", gettext_noop("zero out a vol")}, + {"desc", gettext_noop("Ensure further reads from a volume return zeroes.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_vol_zero_out[] = { + {"pool", VSH_OT_STRING, 0, gettext_noop("pool name or uuid")}, + {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("vol name, key or path")}, + {NULL, 0, 0, NULL} +}; + +static int +cmdVolZeroOut(vshControl *ctl, const vshCmd *cmd) +{ + virStorageVolPtr vol; + int ret = TRUE; + char *name; + + if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) + return FALSE; + + if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", &name))) { + return FALSE; + } + + if (virStorageVolZeroOut(vol, 0) == 0) { + vshPrint(ctl, _("Vol %s zeroed out\n"), name); + } else { + vshError(ctl, _("Failed to zero out vol %s"), name); + ret = FALSE; + } + + virStorageVolFree(vol); + return ret; +} + + +/* * "vol-info" command */ static const vshCmdInfo info_vol_info[] = { @@ -7758,6 +7799,7 @@ static const vshCmdDef commands[] = { {"vol-create-as", cmdVolCreateAs, opts_vol_create_as, info_vol_create_as}, {"vol-clone", cmdVolClone, opts_vol_clone, info_vol_clone}, {"vol-delete", cmdVolDelete, opts_vol_delete, info_vol_delete}, + {"vol-zero-out", cmdVolZeroOut, opts_vol_zero_out, info_vol_zero_out}, {"vol-dumpxml", cmdVolDumpXML, opts_vol_dumpxml, info_vol_dumpxml}, {"vol-info", cmdVolInfo, opts_vol_info, info_vol_info}, {"vol-list", cmdVolList, opts_vol_list, info_vol_list}, -- 1.6.5.5
According to David Allan on 3/2/2010 3:13 PM:
--- tools/virsh.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 42 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index a6a637d..e52b011 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5221,6 +5221,47 @@ cmdVolDelete(vshControl *ctl, const vshCmd *cmd)
/* + * "vol-zero-out" command + */ +static const vshCmdInfo info_vol_zero_out[] = { + {"help", gettext_noop("zero out a vol")}, + {"desc", gettext_noop("Ensure further reads from a volume return zeroes.")},
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 03/02/2010 06:46 PM, Eric Blake wrote:
According to David Allan on 3/2/2010 3:13 PM:
--- tools/virsh.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 42 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index a6a637d..e52b011 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5221,6 +5221,47 @@ cmdVolDelete(vshControl *ctl, const vshCmd *cmd)
/* + * "vol-zero-out" command + */ +static const vshCmdInfo info_vol_zero_out[] = { + {"help", gettext_noop("zero out a vol")}, + {"desc", gettext_noop("Ensure further reads from a volume return zeroes.")},
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?
I assume inertia, but someone else may have a better answer.
On Wed, Mar 03, 2010 at 11:51:26AM -0500, Dave Allan wrote:
On 03/02/2010 06:46 PM, Eric Blake wrote:
According to David Allan on 3/2/2010 3:13 PM:
--- tools/virsh.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 42 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index a6a637d..e52b011 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5221,6 +5221,47 @@ cmdVolDelete(vshControl *ctl, const vshCmd *cmd)
/* + * "vol-zero-out" command + */ +static const vshCmdInfo info_vol_zero_out[] = { + {"help", gettext_noop("zero out a vol")}, + {"desc", gettext_noop("Ensure further reads from a volume return zeroes.")},
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?
I assume inertia, but someone else may have a better answer.
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 :|
Eric Blake wrote:
According to David Allan on 3/2/2010 3:13 PM:
--- tools/virsh.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 42 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index a6a637d..e52b011 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5221,6 +5221,47 @@ cmdVolDelete(vshControl *ctl, const vshCmd *cmd)
/* + * "vol-zero-out" command + */ +static const vshCmdInfo info_vol_zero_out[] = { + {"help", gettext_noop("zero out a vol")}, + {"desc", gettext_noop("Ensure further reads from a volume return zeroes.")},
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?
That'd be a fine change, because it'd shorten some lines, making things slightly more readable. Plus, with 440 occurrences, it'd decrease the size of virsh.c by over 4 KB ;-)
On Tue, Mar 02, 2010 at 05:13:34PM -0500, David Allan wrote:
--- tools/virsh.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 42 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index a6a637d..e52b011 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5221,6 +5221,47 @@ cmdVolDelete(vshControl *ctl, const vshCmd *cmd)
/* + * "vol-zero-out" command + */ +static const vshCmdInfo info_vol_zero_out[] = { + {"help", gettext_noop("zero out a vol")}, + {"desc", gettext_noop("Ensure further reads from a volume return zeroes.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_vol_zero_out[] = { + {"pool", VSH_OT_STRING, 0, gettext_noop("pool name or uuid")}, + {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("vol name, key or path")}, + {NULL, 0, 0, NULL} +}; + +static int +cmdVolZeroOut(vshControl *ctl, const vshCmd *cmd) +{ + virStorageVolPtr vol; + int ret = TRUE; + char *name; + + if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) + return FALSE; + + if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", &name))) { + return FALSE; + } + + if (virStorageVolZeroOut(vol, 0) == 0) { + vshPrint(ctl, _("Vol %s zeroed out\n"), name); + } else { + vshError(ctl, _("Failed to zero out vol %s"), name); + ret = FALSE; + } + + virStorageVolFree(vol); + return ret; +} + + +/* * "vol-info" command */ static const vshCmdInfo info_vol_info[] = { @@ -7758,6 +7799,7 @@ static const vshCmdDef commands[] = { {"vol-create-as", cmdVolCreateAs, opts_vol_create_as, info_vol_create_as}, {"vol-clone", cmdVolClone, opts_vol_clone, info_vol_clone}, {"vol-delete", cmdVolDelete, opts_vol_delete, info_vol_delete}, + {"vol-zero-out", cmdVolZeroOut, opts_vol_zero_out, info_vol_zero_out},
Following my API name suggestion, I'd call this 'vol-wipe' too
{"vol-dumpxml", cmdVolDumpXML, opts_vol_dumpxml, info_vol_dumpxml}, {"vol-info", cmdVolInfo, opts_vol_info, info_vol_info}, {"vol-list", cmdVolList, opts_vol_list, info_vol_list},
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 :|
participants (7)
-
Daniel P. Berrange -
Daniel Veillard -
Dave Allan -
David Allan
-
Eric Blake -
Jim Meyering -
Matthias Bolte