9:13 p.m.
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(-)
9:13 p.m.
New subject: [libvirt] [PATCH 1/9] Add public API for volume wiping
---
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
8:10 a.m.
New subject: [libvirt] [PATCH 1/9] Add public API for volume wiping
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:13 p.m.
New subject: [libvirt] [PATCH 2/9] Define the internal driver API for vol wiping
---
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
8:11 a.m.
New subject: [libvirt] [PATCH 2/9] Define the internal driver API for vol wiping
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
6:31 p.m.
New subject: [libvirt] [PATCH 2/9] Define the internal driver API for vol wiping
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:11 a.m.
New subject: [libvirt] [PATCH 2/9] Define the internal driver API for vol wiping
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 :|
9:13 p.m.
New subject: [libvirt] [PATCH 3/9] Add vol wiping to ESX storage driver struct
---
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
9:13 p.m.
New subject: [libvirt] [PATCH 4/9] Implement the public API for vol wiping
---
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
8:13 a.m.
New subject: [libvirt] [PATCH 4/9] Implement the public API for vol wiping
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:13 p.m.
New subject: [libvirt] [PATCH 5/9] Define wire protocol format for vol wiping
---
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
8:14 a.m.
New subject: [libvirt] [PATCH 5/9] Define wire protocol format for vol wiping
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:13 p.m.
New subject: [libvirt] [PATCH 6/9] Implement RPC client for vol wiping
---
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
8:15 a.m.
New subject: [libvirt] [PATCH 6/9] Implement RPC client for vol wiping
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:13 p.m.
New subject: [libvirt] [PATCH 7/9] Implement the remote dispatch bits of vol wiping
* 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
8:21 a.m.
New subject: [libvirt] [PATCH 7/9] Implement the remote dispatch bits of vol wiping
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:13 p.m.
New subject: [libvirt] [PATCH 8/9] Simplified version of volume wiping based on feedback from the list.
---
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
9:13 a.m.
New subject: [libvirt] [PATCH 8/9] Simplified version of volume wiping based on feedback from the list.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:49 a.m.
New subject: [libvirt] [PATCH 8/9] Simplified version of volume wiping based on feedback from the list.
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.
10:24 a.m.
New subject: [libvirt] [PATCH 8/9] Simplified version of volume wiping based on feedback from the list.
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?
10:34 a.m.
New subject: [libvirt] [PATCH 8/9] Simplified version of volume wiping based on feedback from the list.
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 :|
9:13 p.m.
New subject: [libvirt] [PATCH 9/9] Virsh support for vol wiping
---
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
5:40 a.m.
New subject: [libvirt] [PATCH 9/9] Virsh support for vol wiping
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:14 a.m.
New subject: [libvirt] [PATCH 9/9] Virsh support for vol wiping
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5:33 a.m.
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 :|
1:48 p.m.
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
5362
days inactive
5365
days old
25 comments
6 participants
participants (6)
-
Daniel P. Berrange -
Daniel Veillard -
Dave Allan -
David Allan
-
Eric Blake -
Laine Stump