[libvirt] [PATCH 0/9] Rebased volume wiping API patches
Nobody ack'd (because of general lack of time, I hope) what I think is the final version of the volume wiping API patches, so here is the set again, rebased to the current head. I don't think there is anything controversial in here, as they incorporate all the feedback on previous versions. Dave David Allan (9): Add public API for volume wiping Define the internal driver API for vol wiping Add vol wiping to ESX storage driver struct Implement the public API for vol wiping Define wire protocol format for vol wiping Implement RPC client for vol wiping Implement the remote dispatch bits of vol wiping Simplified version of volume wiping based on feedback from the list. Virsh support for vol wiping 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 | 1 + src/libvirt.c | 47 ++++++++ src/libvirt_public.syms | 5 + 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 | 224 +++++++++++++++++++++++++++++++++++ tools/virsh.c | 42 +++++++ 15 files changed, 426 insertions(+), 1 deletions(-)
--- include/libvirt/libvirt.h.in | 2 ++ src/libvirt_public.syms | 5 +++++ 2 files changed, 7 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 0d1b5b5..1430a95 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 virStorageVolWipe (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..8c72ec6 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -358,4 +358,9 @@ LIBVIRT_0.7.7 { virDomainAbortJob; } LIBVIRT_0.7.5; +LIBVIRT_0.7.8 { + global: + virStorageVolWipe; +} LIBVIRT_0.7.7; + # .... define new API here using predicted next version number .... -- 1.6.5.5
On Mon, Mar 15, 2010 at 10:13:23PM -0400, David Allan wrote:
--- include/libvirt/libvirt.h.in | 2 ++ src/libvirt_public.syms | 5 +++++ 2 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 0d1b5b5..1430a95 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 virStorageVolWipe (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..8c72ec6 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -358,4 +358,9 @@ LIBVIRT_0.7.7 { virDomainAbortJob; } LIBVIRT_0.7.5;
+LIBVIRT_0.7.8 { + global: + virStorageVolWipe; +} LIBVIRT_0.7.7; + # .... define new API here using predicted next version number ....
Fine, I'm not fond of 'wipe' but I think that reflects the intent well, so fine, ACK, 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/
--- src/driver.h | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/src/driver.h b/src/driver.h index a64bba0..1a511eb 100644 --- a/src/driver.h +++ b/src/driver.h @@ -723,6 +723,10 @@ typedef int unsigned int flags); typedef int + (*virDrvStorageVolWipe) (virStorageVolPtr vol, + unsigned int flags); + +typedef int (*virDrvStorageVolGetInfo) (virStorageVolPtr vol, virStorageVolInfoPtr info); typedef char * @@ -791,6 +795,7 @@ struct _virStorageDriver { virDrvStorageVolCreateXML volCreateXML; virDrvStorageVolCreateXMLFrom volCreateXMLFrom; virDrvStorageVolDelete volDelete; + virDrvStorageVolWipe volWipe; virDrvStorageVolGetInfo volGetInfo; virDrvStorageVolGetXMLDesc volGetXMLDesc; virDrvStorageVolGetPath volGetPath; -- 1.6.5.5
On Mon, Mar 15, 2010 at 10:13:24PM -0400, David Allan wrote:
--- src/driver.h | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/src/driver.h b/src/driver.h index a64bba0..1a511eb 100644 --- a/src/driver.h +++ b/src/driver.h @@ -723,6 +723,10 @@ typedef int unsigned int flags);
typedef int + (*virDrvStorageVolWipe) (virStorageVolPtr vol, + unsigned int flags); + +typedef int (*virDrvStorageVolGetInfo) (virStorageVolPtr vol, virStorageVolInfoPtr info); typedef char * @@ -791,6 +795,7 @@ struct _virStorageDriver { virDrvStorageVolCreateXML volCreateXML; virDrvStorageVolCreateXMLFrom volCreateXMLFrom; virDrvStorageVolDelete volDelete; + virDrvStorageVolWipe volWipe; virDrvStorageVolGetInfo volGetInfo; virDrvStorageVolGetXMLDesc volGetXMLDesc; virDrvStorageVolGetPath volGetPath;
logical considered 1/9, ACK, 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/15/2010 08:13 PM, David Allan wrote:
@@ -791,6 +795,7 @@ struct _virStorageDriver { virDrvStorageVolCreateXML volCreateXML; virDrvStorageVolCreateXMLFrom volCreateXMLFrom; virDrvStorageVolDelete volDelete; + virDrvStorageVolWipe volWipe; virDrvStorageVolGetInfo volGetInfo; virDrvStorageVolGetXMLDesc volGetXMLDesc; virDrvStorageVolGetPath volGetPath;
Any reason this was introduced in the middle of the struct, rather than at the end? Sticking stuff in the middle tends to be asking for off-by-one shifts of all the remaining elements in the struct, especially if you ever mix objects compiled across the time when the element was added. Whereas always sticking new entries at the end allows you to maintain a measure of back-compatibility, where an older object still fits in the first half of the struct, and given enough version/sizing information, a newer server can correctly assume that struct members beyond a given offset are effectively null when talking to a client that was compiled against the older struct definition. On the other hand, you probably aren't the first to make a modification like this, so this is more an issue of me asking about the code to learn about it than it is a request for you to make a change. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Mar 17, 2010 at 05:31:07PM -0600, Eric Blake wrote:
On 03/15/2010 08:13 PM, David Allan wrote:
@@ -791,6 +795,7 @@ struct _virStorageDriver { virDrvStorageVolCreateXML volCreateXML; virDrvStorageVolCreateXMLFrom volCreateXMLFrom; virDrvStorageVolDelete volDelete; + virDrvStorageVolWipe volWipe; virDrvStorageVolGetInfo volGetInfo; virDrvStorageVolGetXMLDesc volGetXMLDesc; virDrvStorageVolGetPath volGetPath;
Any reason this was introduced in the middle of the struct, rather than at the end? Sticking stuff in the middle tends to be asking for off-by-one shifts of all the remaining elements in the struct, especially if you ever mix objects compiled across the time when the element was added. Whereas always sticking new entries at the end allows you to maintain a measure of back-compatibility, where an older object still fits in the first half of the struct, and given enough version/sizing information, a newer server can correctly assume that struct members beyond a given offset are effectively null when talking to a client that was compiled against the older struct definition.
This is purely internal code, so there's no compatability issues with struct member placement in this case. The only files where we have to be careful for ABI compatability are include/libvirt/*.h and src/remote/remote_protocol.x (RPC wire format) 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/esx/esx_storage_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/esx/esx_storage_driver.c b/src/esx/esx_storage_driver.c index b920f3b..7b073a6 100644 --- a/src/esx/esx_storage_driver.c +++ b/src/esx/esx_storage_driver.c @@ -102,6 +102,7 @@ static virStorageDriver esxStorageDriver = { NULL, /* volCreateXML */ NULL, /* volCreateXMLFrom */ NULL, /* volDelete */ + NULL, /* volWipe */ NULL, /* volGetInfo */ NULL, /* volGetXMLDesc */ NULL, /* 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 1d9b878..74b075b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -8479,6 +8479,53 @@ error: /** + * virStorageVolWipe: + * @vol: pointer to storage volume + * @flags: future flags, use 0 for now + * + * Ensure data previously on a volume is not accessible to future reads + * + * Returns 0 on success, or -1 on error + */ +int +virStorageVolWipe(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->volWipe) { + int ret; + ret = conn->storageDriver->volWipe(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
On Mon, Mar 15, 2010 at 10:13:26PM -0400, David Allan wrote:
--- src/libvirt.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 47 insertions(+), 0 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index 1d9b878..74b075b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -8479,6 +8479,53 @@ error:
/** + * virStorageVolWipe: + * @vol: pointer to storage volume + * @flags: future flags, use 0 for now
"optional flags" sounds better to me
+ * + * Ensure data previously on a volume is not accessible to future reads + * + * Returns 0 on success, or -1 on error + */ +int +virStorageVolWipe(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->volWipe) { + int ret; + ret = conn->storageDriver->volWipe(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 *
ACK, 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/
--- 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..6bd9019 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_wipe_args (XDR *xdrs, remote_storage_vol_wipe_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 f76e6e5..c492ffd 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_wipe_args { + remote_nonnull_storage_vol vol; + u_int flags; +}; +typedef struct remote_storage_vol_wipe_args remote_storage_vol_wipe_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_WIPE = 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_wipe_args (XDR *, remote_storage_vol_wipe_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_wipe_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..868a2c2 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_wipe_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_WIPE = 165 /* * Notice how the entries are grouped in sets of 10 ? -- 1.6.5.5
On Mon, Mar 15, 2010 at 10:13:27PM -0400, David Allan wrote:
--- 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(-)
ACK, 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/
--- 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 11513bd..0ee038e 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -5538,6 +5538,32 @@ done: } static int +remoteStorageVolWipe(virStorageVolPtr vol, + unsigned int flags) +{ + int rv = -1; + remote_storage_vol_wipe_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_WIPE, + (xdrproc_t) xdr_remote_storage_vol_wipe_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, + .volWipe = remoteStorageVolWipe, .volGetInfo = remoteStorageVolGetInfo, .volGetXMLDesc = remoteStorageVolDumpXML, .volGetPath = remoteStorageVolGetPath, -- 1.6.5.5
On Mon, Mar 15, 2010 at 10:13:28PM -0400, David Allan wrote:
--- 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 11513bd..0ee038e 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -5538,6 +5538,32 @@ done: }
static int +remoteStorageVolWipe(virStorageVolPtr vol, + unsigned int flags) +{ + int rv = -1; + remote_storage_vol_wipe_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_WIPE, + (xdrproc_t) xdr_remote_storage_vol_wipe_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, + .volWipe = remoteStorageVolWipe, .volGetInfo = remoteStorageVolGetInfo, .volGetXMLDesc = remoteStorageVolDumpXML, .volGetPath = remoteStorageVolGetPath,
ACK, At some point I would like to convert the storage_driver definition to use the old format, which allow to easilly see what are the missing entry points, like we do for domains, but not urgent :-) 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/
* 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 7c4339f..178b03e 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4273,6 +4273,38 @@ remoteDispatchStorageVolDelete (struct qemud_server *server ATTRIBUTE_UNUSED, } static int +remoteDispatchStorageVolWipe(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_wipe_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 (virStorageVolWipe(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..32a6d78 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_wipe_args val_remote_storage_vol_wipe_args; diff --git a/daemon/remote_dispatch_prototypes.h b/daemon/remote_dispatch_prototypes.h index b81c8c3..4c944f6 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 remoteDispatchStorageVolWipe( + struct qemud_server *server, + struct qemud_client *client, + virConnectPtr conn, + remote_message_header *hdr, + remote_error *err, + remote_storage_vol_wipe_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..28e7d9a 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, }, +{ /* StorageVolWipe => 165 */ + .fn = (dispatch_fn) remoteDispatchStorageVolWipe, + .args_filter = (xdrproc_t) xdr_remote_storage_vol_wipe_args, + .ret_filter = (xdrproc_t) xdr_void, +}, -- 1.6.5.5
On Mon, Mar 15, 2010 at 10:13:29PM -0400, David Allan wrote:
* 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.
Strange, we have an "make rpcgen" to rebuild the remote headers in src/Makefile.am , but for the daemon I though everything was automatic... ACK, 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/
--- src/storage/storage_driver.c | 224 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 224 insertions(+), 0 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ca2540f..1961fa0 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,226 @@ cleanup: return ret; } + +/* If the volume we're wiping 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, + off_t size, + 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, 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))); + } + +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; + off_t remaining = 0; + size_t write_size = 0; + char errbuf[64]; + + 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))); + 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); + 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]; + 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))); + 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))); + goto out; + } + + if (S_ISREG(st.st_mode) && st.st_blocks < (st.st_size / DEV_BSIZE)) { + ret = storageVolumeZeroSparseFile(def, st.st_size, fd); + } else { + + if (VIR_ALLOC_N(writebuf, st.st_blksize) != 0) { + virReportOOMError(); + goto out; + } + + ret = storageWipeExtent(def, + fd, + 0, + def->allocation, + writebuf, + st.st_blksize, + &bytes_wiped); + } + +out: + VIR_FREE(writebuf); + + if (fd != -1) { + close(fd); + } + + return ret; +} + + +static int +storageVolumeWipe(virStorageVolPtr obj, + unsigned int flags) +{ + virStorageDriverStatePtr driver = obj->conn->storagePrivateData; + virStoragePoolObjPtr pool = NULL; + virStorageVolDefPtr vol = NULL; + int ret = -1; + + if (flags != 0) { + virStorageReportError(VIR_ERR_INVALID_ARG, + _("Unsupported flags (0x%x) passed to '%s'"), flags, __FUNCTION__); + goto out; + } + + 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 (storageVolumeWipeInternal(vol) == -1) { + goto out; + } + + ret = 0; + +out: + if (pool) { + virStoragePoolObjUnlock(pool); + } + + return ret; + +} + static int storageVolumeDelete(virStorageVolPtr obj, unsigned int flags) { @@ -1775,6 +1998,7 @@ static virStorageDriver storageDriver = { .volCreateXML = storageVolumeCreateXML, .volCreateXMLFrom = storageVolumeCreateXMLFrom, .volDelete = storageVolumeDelete, + .volWipe = storageVolumeWipe, .volGetInfo = storageVolumeGetInfo, .volGetXMLDesc = storageVolumeGetXMLDesc, .volGetPath = storageVolumeGetPath, -- 1.6.5.5
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(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ca2540f..1961fa0 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,226 @@ cleanup: return ret; }
+ +/* If the volume we're wiping 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, + off_t size, + int fd) +{ + int ret = -1;
no need to initialize here
+ 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,
+ 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, 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))); + } + +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
+ 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))); + 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); + 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]; + 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))); + 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))); + goto out; + } + + if (S_ISREG(st.st_mode) && st.st_blocks < (st.st_size / DEV_BSIZE)) { + ret = storageVolumeZeroSparseFile(def, st.st_size, fd); + } else { + + if (VIR_ALLOC_N(writebuf, st.st_blksize) != 0) { + virReportOOMError(); + goto out; + }
Urgh .... that sounds quite inefficient to me. Assuming a block size of 512, that means 2000 safewrite/write/syscall() per megabyte. Can we at least increase that to 64K, the loop on remaining on the called function should insure that there is no problem at the end of the extent
+ ret = storageWipeExtent(def, + fd, + 0, + def->allocation, + writebuf, + st.st_blksize, + &bytes_wiped); + } + +out: + VIR_FREE(writebuf); + + if (fd != -1) { + close(fd); + } + + return ret; +} + + +static int +storageVolumeWipe(virStorageVolPtr obj, + unsigned int flags) +{ + virStorageDriverStatePtr driver = obj->conn->storagePrivateData; + virStoragePoolObjPtr pool = NULL; + virStorageVolDefPtr vol = NULL; + int ret = -1; + + if (flags != 0) { + virStorageReportError(VIR_ERR_INVALID_ARG, + _("Unsupported flags (0x%x) passed to '%s'"), flags, __FUNCTION__); + goto out; + } + + 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 (storageVolumeWipeInternal(vol) == -1) { + goto out; + } + + ret = 0; + +out: + if (pool) { + virStoragePoolObjUnlock(pool); + } + + return ret; + +}
Hum, where did we lock the pool ? Does virStoragePoolObjFindByName() lock it automatically, I think I remember seomthing like that but I would prefer to be sure.
static int storageVolumeDelete(virStorageVolPtr obj, unsigned int flags) { @@ -1775,6 +1998,7 @@ static virStorageDriver storageDriver = { .volCreateXML = storageVolumeCreateXML, .volCreateXMLFrom = storageVolumeCreateXMLFrom, .volDelete = storageVolumeDelete, + .volWipe = storageVolumeWipe, .volGetInfo = storageVolumeGetInfo, .volGetXMLDesc = storageVolumeGetXMLDesc, .volGetPath = storageVolumeGetPath,
ACK once the wiping buffer size is incremented to something decent :-) 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/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.
On 03/17/2010 10:49 AM, Laine Stump wrote:
>
>>> + 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)));
> + ret = ftruncate(fd, size);
>
> 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)));
Oops - that last line is (probably obviously) an artifact of cut-paste
that I meant to delete.
>
>>> + 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?
I had meant to mention that I've seen this in at least one other place
as well. Is there any reason for using VIR_ERROR like this, or is it
just a historical artifact?
On Wed, Mar 17, 2010 at 11:24:03AM -0400, Laine Stump wrote:
> On 03/17/2010 10:49 AM, Laine Stump wrote:
> >
> >>>+ 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)));
> >+ ret = ftruncate(fd, size);
> >
> >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)));
>
> Oops - that last line is (probably obviously) an artifact of cut-paste
> that I meant to delete.
>
> >
> >>>+ 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?
>
> I had meant to mention that I've seen this in at least one other place
> as well. Is there any reason for using VIR_ERROR like this, or is it
> just a historical artifact?
Not often. We use VIR_ERROR in contexts we don't want to report a problem
to the user, or where their is no end user to report to. MNost of the time
using virReportSystemError is the right answer
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 :|
--- tools/virsh.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 42 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index aa85ee6..37ff471 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5271,6 +5271,47 @@ cmdVolDelete(vshControl *ctl, const vshCmd *cmd) /* + * "vol-wipe" command + */ +static const vshCmdInfo info_vol_wipe[] = { + {"help", gettext_noop("wipe a vol")}, + {"desc", gettext_noop("Ensure data previously on a volume is not accessible to future reads")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_vol_wipe[] = { + {"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 +cmdVolWipe(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 (virStorageVolWipe(vol, 0) == 0) { + vshPrint(ctl, _("Vol %s wiped\n"), name); + } else { + vshError(ctl, _("Failed to wipe vol %s"), name); + ret = FALSE; + } + + virStorageVolFree(vol); + return ret; +} + + +/* * "vol-info" command */ static const vshCmdInfo info_vol_info[] = { @@ -7809,6 +7850,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-wipe", cmdVolWipe, opts_vol_wipe, info_vol_wipe}, {"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
On 03/15/2010 08:13 PM, David Allan wrote:
+static const vshCmdInfo info_vol_wipe[] = { + {"help", gettext_noop("wipe a vol")}, + {"desc", gettext_noop("Ensure data previously on a volume is not accessible to future reads")},
Your rebase missed that we prefer N_, not gettext_noop, now. Hmm, that means I should add a check to 'make syntax-check'... -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Mar 15, 2010 at 10:13:31PM -0400, 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 aa85ee6..37ff471 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5271,6 +5271,47 @@ cmdVolDelete(vshControl *ctl, const vshCmd *cmd)
/* + * "vol-wipe" command + */ +static const vshCmdInfo info_vol_wipe[] = { + {"help", gettext_noop("wipe a vol")}, + {"desc", gettext_noop("Ensure data previously on a volume is not accessible to future reads")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_vol_wipe[] = { + {"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 +cmdVolWipe(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 (virStorageVolWipe(vol, 0) == 0) { + vshPrint(ctl, _("Vol %s wiped\n"), name); + } else { + vshError(ctl, _("Failed to wipe vol %s"), name); + ret = FALSE; + } + + virStorageVolFree(vol); + return ret; +} + + +/* * "vol-info" command */ static const vshCmdInfo info_vol_info[] = { @@ -7809,6 +7850,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-wipe", cmdVolWipe, opts_vol_wipe, info_vol_wipe}, {"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},
ACK, 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 Mon, Mar 15, 2010 at 10:13:22PM -0400, David Allan wrote:
Nobody ack'd (because of general lack of time, I hope) what I think is the final version of the volume wiping API patches, so here is the set again, rebased to the current head. I don't think there is anything controversial in here, as they incorporate all the feedback on previous versions.
ACK to the whole series. Just remember to fix the gettext_noop bit that Eric mentioned before committing 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 :|
On 03/17/2010 06:33 AM, Daniel P. Berrange wrote:
On Mon, Mar 15, 2010 at 10:13:22PM -0400, David Allan wrote:
Nobody ack'd (because of general lack of time, I hope) what I think is the final version of the volume wiping API patches, so here is the set again, rebased to the current head. I don't think there is anything controversial in here, as they incorporate all the feedback on previous versions.
ACK to the whole series. Just remember to fix the gettext_noop bit that Eric mentioned before committing
Regards, Daniel
Ok, I fixed the few things people pointed out & pushed the series. Dave
participants (6)
-
Daniel P. Berrange -
Daniel Veillard -
Dave Allan -
David Allan
-
Eric Blake -
Laine Stump