[libvirt] [PATCH 0/6] v2: Allow data upload/download to/from storage volumes
An update of http://www.redhat.com/archives/libvir-list/2011-March/msg00655.html Addressing all the feedback from v1 series. The most notable change is that 'iohelper' now takes an explicit flags+mode parameter, instead of the operation name.
qemuDomainSetMemoryParameters and qemuDomainGetMemoryParameters forgot to do a check on virDomainIsActive(), resulting in bogus error messages from later parts of their impl * src/qemu/qemu_driver.c: Add missing checks on virDomainIsActive() --- src/qemu/qemu_driver.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1266a0e..4ad8639 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4680,6 +4680,12 @@ static int qemuDomainSetMemoryParameters(virDomainPtr dom, goto cleanup; } + if (!virDomainObjIsActive (vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto cleanup; + } + if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("cannot find cgroup for domain %s"), vm->def->name); @@ -4783,6 +4789,12 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, goto cleanup; } + if (!virDomainObjIsActive (vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto cleanup; + } + if ((*nparams) == 0) { /* Current number of memory parameters supported by cgroups */ *nparams = QEMU_NB_MEM_PARAM; -- 1.7.4
On 03/18/2011 10:36 AM, Daniel P. Berrange wrote:
qemuDomainSetMemoryParameters and qemuDomainGetMemoryParameters forgot to do a check on virDomainIsActive(), resulting in bogus error messages from later parts of their impl
* src/qemu/qemu_driver.c: Add missing checks on virDomainIsActive() --- src/qemu/qemu_driver.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)
In looking at the code, at least qemuSetSchedulerParameters and qemuDomain{Set,Get}BlkioParameters have the same bug, given the level of copy and paste between those two APIs. Maybe qemu_cgroup.h should provide a helper function that does both the virDomainObjIsActive and virCgroupForDomain check in a single call, to avoid duplication all over qemu_driver.c?
+ if (!virDomainObjIsActive (vm)) {
Why the space before (vm)? This patch seems unrelated to the upload/download block API addition at hand, but is independently useful. NACK on this version since it is incomplete, so looking forward to v2 as a separate thread. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
New APIs are added allowing streaming of content to/from storage volumes. A new API for creating volumes is also added allowing the content to be provided immediately at time of creation * include/libvirt/libvirt.h.in: Add virStorageVolUpload and virStorageVolDownload APIs * src/driver.h, src/libvirt.c, src/libvirt_public.syms: Stub code for new APIs * src/storage/storage_driver.c, src/esx/esx_storage_driver.c: Add dummy entries in driver table for new APIs --- include/libvirt/libvirt.h.in | 10 ++++ include/libvirt/virterror.h | 1 + src/driver.h | 14 +++++ src/esx/esx_storage_driver.c | 2 + src/libvirt.c | 123 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 2 + src/storage/storage_driver.c | 2 + src/util/virterror.c | 6 ++ 8 files changed, 160 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index fcca39d..bd728f2 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1500,6 +1500,16 @@ virStorageVolPtr virStorageVolCreateXMLFrom (virStoragePoolPtr pool, const char *xmldesc, virStorageVolPtr clonevol, unsigned int flags); +int virStorageVolDownload (virStorageVolPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags); +int virStorageVolUpload (virStorageVolPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags); int virStorageVolDelete (virStorageVolPtr vol, unsigned int flags); int virStorageVolWipe (virStorageVolPtr vol, diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index 1d8275b..59f7731 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -230,6 +230,7 @@ typedef enum { VIR_ERR_HOOK_SCRIPT_FAILED = 70, /* a synchronous hook script failed */ VIR_ERR_INVALID_DOMAIN_SNAPSHOT = 71,/* invalid domain snapshot */ VIR_ERR_NO_DOMAIN_SNAPSHOT = 72, /* domain snapshot not found */ + VIR_ERR_INVALID_STREAM = 73 /* stream pointer not valid */ } virErrorNumber; /** diff --git a/src/driver.h b/src/driver.h index f03d290..2ec8529 100644 --- a/src/driver.h +++ b/src/driver.h @@ -900,6 +900,18 @@ typedef virStorageVolPtr const char *xmldesc, virStorageVolPtr clone, unsigned int flags); +typedef int + (*virDrvStorageVolDownload) (virStorageVolPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags); +typedef int + (*virDrvStorageVolUpload) (virStorageVolPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags); typedef int (*virDrvStoragePoolIsActive)(virStoragePoolPtr pool); @@ -954,6 +966,8 @@ struct _virStorageDriver { virDrvStorageVolLookupByPath volLookupByPath; virDrvStorageVolCreateXML volCreateXML; virDrvStorageVolCreateXMLFrom volCreateXMLFrom; + virDrvStorageVolDownload volDownload; + virDrvStorageVolUpload volUpload; virDrvStorageVolDelete volDelete; virDrvStorageVolWipe volWipe; virDrvStorageVolGetInfo volGetInfo; diff --git a/src/esx/esx_storage_driver.c b/src/esx/esx_storage_driver.c index 136a90b..9e4dd9e 100644 --- a/src/esx/esx_storage_driver.c +++ b/src/esx/esx_storage_driver.c @@ -1671,6 +1671,8 @@ static virStorageDriver esxStorageDriver = { esxStorageVolumeLookupByPath, /* volLookupByPath */ esxStorageVolumeCreateXML, /* volCreateXML */ esxStorageVolumeCreateXMLFrom, /* volCreateXMLFrom */ + NULL, /* volDownload */ + NULL, /* volUpload */ esxStorageVolumeDelete, /* volDelete */ esxStorageVolumeWipe, /* volWipe */ esxStorageVolumeGetInfo, /* volGetInfo */ diff --git a/src/libvirt.c b/src/libvirt.c index 33bb17c..e94c940 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -9065,6 +9065,129 @@ error: /** + * virStorageVolDownload: + * @pool: pointer to volume to download + * @stream: stream to use as output + * @offset: position to start reading from + * @length: limit on amount of data to download + * @flags: flags for creation (unused, pass 0) + * + * Download the content of the volume as a stream. If @length + * is zero, then the entire file contents will be downloaded. + * + * Returns 0, or -1 upon error. + */ +int +virStorageVolDownload(virStorageVolPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags) +{ + VIR_DEBUG("vol=%p stream=%p offset=%llu length=%llu flags=%u", vol, stream, offset, length, flags); + + virResetLastError(); + + if (!VIR_IS_STORAGE_VOL(vol)) { + virLibConnError(VIR_ERR_INVALID_STORAGE_VOL, __FUNCTION__); + return -1; + } + + if (!VIR_IS_STREAM(stream)) { + virLibConnError(VIR_ERR_INVALID_STREAM, __FUNCTION__); + return -1; + } + + if (vol->conn->flags & VIR_CONNECT_RO || + stream->conn->flags & VIR_CONNECT_RO) { + virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (vol->conn->storageDriver && + vol->conn->storageDriver->volDownload) { + int ret; + ret = vol->conn->storageDriver->volDownload(vol, + stream, + offset, + length, + flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(vol->conn); + return -1; +} + + +/** + * virStorageVolUpload: + * @pool: pointer to volume to download + * @stream: stream to use as output + * @offset: position to start writing to + * @length: limit on amount of data to upload + * @flags: flags for creation (unused, pass 0) + * + * Upload new content to the volume from a stream. If @length + * is non-zero, and an error will be raised if an attempt is + * made to upload greater than @length bytes of data. + * + * Returns 0, or -1 upon error. + */ +int +virStorageVolUpload(virStorageVolPtr vol, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags) +{ + VIR_DEBUG("vol=%p stream=%p offset=%llu length=%llu flags=%u", vol, stream, offset, length, flags); + + virResetLastError(); + + if (!VIR_IS_STORAGE_VOL(vol)) { + virLibConnError(VIR_ERR_INVALID_STORAGE_VOL, __FUNCTION__); + return -1; + } + + if (!VIR_IS_STREAM(stream)) { + virLibConnError(VIR_ERR_INVALID_STREAM, __FUNCTION__); + return -1; + } + + if (vol->conn->flags & VIR_CONNECT_RO || + stream->conn->flags & VIR_CONNECT_RO) { + virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (vol->conn->storageDriver && + vol->conn->storageDriver->volUpload) { + int ret; + ret = vol->conn->storageDriver->volUpload(vol, + stream, + offset, + length, + flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(vol->conn); + return -1; +} + + +/** * virStorageVolDelete: * @vol: pointer to storage volume * @flags: future flags, use 0 for now diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index c027bf7..dfe0196 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -431,6 +431,8 @@ LIBVIRT_0.9.0 { virDomainSetMemoryFlags; virEventRegisterDefaultImpl; virEventRunDefaultImpl; + virStorageVolDownload; + virStorageVolUpload; } LIBVIRT_0.8.8; # .... define new API here using predicted next version number .... diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 5373025..ce528cf 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1989,6 +1989,8 @@ static virStorageDriver storageDriver = { .volLookupByPath = storageVolumeLookupByPath, .volCreateXML = storageVolumeCreateXML, .volCreateXMLFrom = storageVolumeCreateXMLFrom, + .volDownload = NULL, + .volUpload = NULL, .volDelete = storageVolumeDelete, .volWipe = storageVolumeWipe, .volGetInfo = storageVolumeGetInfo, diff --git a/src/util/virterror.c b/src/util/virterror.c index 160c953..b7d8924 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -1201,6 +1201,12 @@ virErrorMsg(virErrorNumber error, const char *info) else errmsg = _("Domain snapshot not found: %s"); break; + case VIR_ERR_INVALID_STREAM: + if (info == NULL) + errmsg = _("invalid stream pointer"); + else + errmsg = _("invalid stream pointer in %s"); + break; } return (errmsg); } -- 1.7.4
On 03/18/2011 10:36 AM, Daniel P. Berrange wrote:
New APIs are added allowing streaming of content to/from storage volumes. A new API for creating volumes is also added allowing the content to be provided immediately at time of creation
Delete this last sentence; it's stale information from an earlier version of the commit.
* include/libvirt/libvirt.h.in: Add virStorageVolUpload and virStorageVolDownload APIs * src/driver.h, src/libvirt.c, src/libvirt_public.syms: Stub code for new APIs * src/storage/storage_driver.c, src/esx/esx_storage_driver.c: Add dummy entries in driver table for new APIs --- include/libvirt/libvirt.h.in | 10 ++++ include/libvirt/virterror.h | 1 + src/driver.h | 14 +++++ src/esx/esx_storage_driver.c | 2 + src/libvirt.c | 123 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 2 + src/storage/storage_driver.c | 2 + src/util/virterror.c | 6 ++ 8 files changed, 160 insertions(+), 0 deletions(-)
+++ b/src/libvirt.c @@ -9065,6 +9065,129 @@ error:
/** + * virStorageVolDownload: + * @pool: pointer to volume to download + * @stream: stream to use as output + * @offset: position to start reading from
Since there are two effective files (the stream and the volume), it would help to make it clear which file the offset applies to (it can somewhat be implied, because streams are not seekable and thus do not have an offset, but more wording never hurts).
+ * @length: limit on amount of data to download + * @flags: flags for creation (unused, pass 0)
What is being created? The stream and volume already exist.
+ * + * Download the content of the volume as a stream. If @length + * is zero, then the entire file contents will be downloaded.
You have to use offset=0 to get the entire file. Also, mention that it is an error if length+offset > size of volume. What happens if the stream hits EOF before all data read from the volume has been written to the stream? Should the API support a way to tell how many bytes were successfully downloaded in the case of a short stream? That is, instead of returning 0 on success, should Upload and Download return a ull with how many bytes were transferred in/out of the stream? These days, efficient sparse handling is a cool kernel feature waiting to be exploited (see coreutils 8.10 use of FIEMAP in cp). Should we make it easier to detect holes in a volume, by exposing some of this information back through this API?
+ +/** + * virStorageVolUpload: + * @pool: pointer to volume to download + * @stream: stream to use as output
Two lines with too much copy and paste.
+ * @offset: position to start writing to + * @length: limit on amount of data to upload + * @flags: flags for creation (unused, pass 0) + * + * Upload new content to the volume from a stream. If @length + * is non-zero, and an error will be raised if an attempt is + * made to upload greater than @length bytes of data.
I'm still a bit confused on the semantics. Is this the desired interpretation: Suppose the volume has size S > 1. offset=0 length=0 => read S bytes from stream to fill S bytes of volume offset=1 length=0 => read S-1 bytes from stream offset=1 length=S => error (length+offset > S) offset=0 length=1 => read 1 byte from stream, write 1 byte to volume In all cases, if stream supplies fewer bytes than the resulting size to be written to the volume, then does this fail after writing all supplied bytes? Should the API tell how many bytes were successfully transferred to the volume in the case of a short stream? Is this a case where it might be worth letting flags=1 imply that all remaining bytes of volume should be set to NUL (that is, when uploading from a smaller source to a larger destination volume, do we want to guarantee that the tail end of volume has been wiped)? I'm thinking that if we return the length of bytes read from the stream, then anyone that cares can do a subsequent upload starting from that offset to upload NULs to any further bytes. For that matter, that sounds like a great use case for wiping a portion of a volume (aka punching holes into an image). virStorageVolWipe can only wipe the entire volume, but virStorageVolUpload could be used to intentionally wipe any given offset and length via a flag. So, does this look any better as a proposed API? /** * virStorageVolDownload: * @pool: pointer to volume to download from * @stream: stream to use as output * @offset: position in @pool to start reading from * @length: limit on amount of data to download * @flags: future flags (unused, pass 0) * * Download the content of the volume as a stream. If @length * is zero, then the remaining contents after @offset will be * downloaded. It is an error if @length + @offset exceeds * the size of the volume. * * Returns number of bytes written to stream, or -1 upon error. */ unsigned long long virStorageVolDownload(virStorageVolPtr vol, virStreamPtr stream, unsigned long long offset, unsigned long long length, unsigned int flags); enum { VIR_STORAGE_VOLUME_UPLOAD_WIPE = 1, }; /** * virStorageVolUpload: * @pool: pointer to volume to upload to * @stream: stream to use as input * @offset: position in @pool to start writing to * @length: limit on amount of data to upload * @flags: VIR_STORAGE_VOLUME_UPLOAD flags * * Upload new content to the volume from a stream. It is an * error if @length + @offset exceeds the size of the * volume. If @length is zero, then use the volume size minus * @length bytes of data. * * If @flags is zero, data is read from stream; if it is * VIR_STORAGE_VOLUME_UPLOAD_WIPE, NUL bytes are written * instead and @stream may be NULL. * * Returns the bytes 0, or -1 upon error. */ int virStorageVolUpload(virStorageVolPtr vol, virStreamPtr stream, unsigned long long offset, unsigned long long length, unsigned int flags); virStorageVolWipe(vol, flags) is shorthand for virStorageVolUpload(vol, NULL, 0, 0, VIR_STORAGE_VOLUME_UPLOAD_WIPE | flags) The rest of this patch looks great, but I think we have to nail the right API in libvirt.c (with possible fallout to the types of the driver callback functions) before committing anything, so this will need a v3. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Mar 21, 2011 at 03:04:39PM -0600, Eric Blake wrote:
On 03/18/2011 10:36 AM, Daniel P. Berrange wrote:
New APIs are added allowing streaming of content to/from storage volumes. A new API for creating volumes is also added allowing the content to be provided immediately at time of creation
Delete this last sentence; it's stale information from an earlier version of the commit.
* include/libvirt/libvirt.h.in: Add virStorageVolUpload and virStorageVolDownload APIs * src/driver.h, src/libvirt.c, src/libvirt_public.syms: Stub code for new APIs * src/storage/storage_driver.c, src/esx/esx_storage_driver.c: Add dummy entries in driver table for new APIs --- include/libvirt/libvirt.h.in | 10 ++++ include/libvirt/virterror.h | 1 + src/driver.h | 14 +++++ src/esx/esx_storage_driver.c | 2 + src/libvirt.c | 123 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 2 + src/storage/storage_driver.c | 2 + src/util/virterror.c | 6 ++ 8 files changed, 160 insertions(+), 0 deletions(-)
+++ b/src/libvirt.c @@ -9065,6 +9065,129 @@ error:
/** + * virStorageVolDownload: + * @pool: pointer to volume to download + * @stream: stream to use as output + * @offset: position to start reading from
Since there are two effective files (the stream and the volume), it would help to make it clear which file the offset applies to (it can somewhat be implied, because streams are not seekable and thus do not have an offset, but more wording never hurts).
+ * @length: limit on amount of data to download + * @flags: flags for creation (unused, pass 0)
What is being created? The stream and volume already exist.
+ * + * Download the content of the volume as a stream. If @length + * is zero, then the entire file contents will be downloaded.
You have to use offset=0 to get the entire file.
Also, mention that it is an error if length+offset > size of volume.
What happens if the stream hits EOF before all data read from the volume has been written to the stream? Should the API support a way to tell how many bytes were successfully downloaded in the case of a short stream? That is, instead of returning 0 on success, should Upload and Download return a ull with how many bytes were transferred in/out of the stream?
Everything related to actual I/O is done virStreamRecv/virStreamSend. These APIs merely open the volume and associate a stream with it. So all these questions about number of bytes / EOF are not relevant in this context.
These days, efficient sparse handling is a cool kernel feature waiting to be exploited (see coreutils 8.10 use of FIEMAP in cp). Should we make it easier to detect holes in a volume, by exposing some of this information back through this API?
The concept of sparseness doesn't really fit into streams because they're not seekable. With download, to avoid repeated round-trips, the client doesn't actually issue individual read() calls to pull data over the RPC. Instead the server pushes the data down to the client continuously until EOF on the server.
+ +/** + * virStorageVolUpload: + * @pool: pointer to volume to download + * @stream: stream to use as output
Two lines with too much copy and paste.
+ * @offset: position to start writing to + * @length: limit on amount of data to upload + * @flags: flags for creation (unused, pass 0) + * + * Upload new content to the volume from a stream. If @length + * is non-zero, and an error will be raised if an attempt is + * made to upload greater than @length bytes of data.
I'm still a bit confused on the semantics. Is this the desired interpretation:
Suppose the volume has size S > 1. offset=0 length=0 => read S bytes from stream to fill S bytes of volume offset=1 length=0 => read S-1 bytes from stream offset=1 length=S => error (length+offset > S) offset=0 length=1 => read 1 byte from stream, write 1 byte to volume
For upload, actually the 'length' parameter is not all that much use and only really included for completeness. The client just continuously pushes data upto the server. The server will merely return an error if the client pushes more than 'length' bytes of data, or if offset+length exceeds the size of the volume.
In all cases, if stream supplies fewer bytes than the resulting size to be written to the volume, then does this fail after writing all supplied bytes? Should the API tell how many bytes were successfully transferred to the volume in the case of a short stream? Is this a case where it might be worth letting flags=1 imply that all remaining bytes of volume should be set to NUL (that is, when uploading from a smaller source to a larger destination volume, do we want to guarantee that the tail end of volume has been wiped)? I'm thinking that if we return the length of bytes read from the stream, then anyone that cares can do a subsequent upload starting from that offset to upload NULs to any further bytes.
For that matter, that sounds like a great use case for wiping a portion of a volume (aka punching holes into an image). virStorageVolWipe can only wipe the entire volume, but virStorageVolUpload could be used to intentionally wipe any given offset and length via a flag.
I don't think wiping really fits in practically with the way streams operate, since this isn't a synchronous API.
/** * virStorageVolDownload: * @pool: pointer to volume to download from * @stream: stream to use as output * @offset: position in @pool to start reading from * @length: limit on amount of data to download * @flags: future flags (unused, pass 0) * * Download the content of the volume as a stream. If @length * is zero, then the remaining contents after @offset will be * downloaded. It is an error if @length + @offset exceeds * the size of the volume. * * Returns number of bytes written to stream, or -1 upon error. */ unsigned long long virStorageVolDownload(virStorageVolPtr vol, virStreamPtr stream, unsigned long long offset, unsigned long long length, unsigned int flags);
enum { VIR_STORAGE_VOLUME_UPLOAD_WIPE = 1, };
/** * virStorageVolUpload: * @pool: pointer to volume to upload to * @stream: stream to use as input * @offset: position in @pool to start writing to * @length: limit on amount of data to upload * @flags: VIR_STORAGE_VOLUME_UPLOAD flags * * Upload new content to the volume from a stream. It is an * error if @length + @offset exceeds the size of the * volume. If @length is zero, then use the volume size minus * @length bytes of data. * * If @flags is zero, data is read from stream; if it is * VIR_STORAGE_VOLUME_UPLOAD_WIPE, NUL bytes are written * instead and @stream may be NULL. * * Returns the bytes 0, or -1 upon error. */ int virStorageVolUpload(virStorageVolPtr vol, virStreamPtr stream, unsigned long long offset, unsigned long long length, unsigned int flags);
virStorageVolWipe(vol, flags) is shorthand for virStorageVolUpload(vol, NULL, 0, 0, VIR_STORAGE_VOLUME_UPLOAD_WIPE | flags)
The rest of this patch looks great, but I think we have to nail the right API in libvirt.c (with possible fallout to the types of the driver callback functions) before committing anything, so this will need a v3.
None of this is really possible, since streams just don't operate in this way. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/22/2011 05:45 AM, Daniel P. Berrange wrote:
What happens if the stream hits EOF before all data read from the volume has been written to the stream? Should the API support a way to tell how many bytes were successfully downloaded in the case of a short stream? That is, instead of returning 0 on success, should Upload and Download return a ull with how many bytes were transferred in/out of the stream?
Everything related to actual I/O is done virStreamRecv/virStreamSend. These APIs merely open the volume and associate a stream with it. So all these questions about number of bytes / EOF are not relevant in this context.
Yeah, I figured that out in patch 3/6. Still, there were some improvements to make to the wording.
These days, efficient sparse handling is a cool kernel feature waiting to be exploited (see coreutils 8.10 use of FIEMAP in cp). Should we make it easier to detect holes in a volume, by exposing some of this information back through this API?
The concept of sparseness doesn't really fit into streams because they're not seekable. With download, to avoid repeated round-trips, the client doesn't actually issue individual read() calls to pull data over the RPC. Instead the server pushes the data down to the client continuously until EOF on the server.
Then we can save sparseness for another day and another patch. I imagine it might be possible to extend streams to support some OOB sparseness data (if both server and client know how to interpret OOB, then both sides can skip over holes in the transfer; if either side lacks support for OOB, then holes are transferred as normal data).
For that matter, that sounds like a great use case for wiping a portion of a volume (aka punching holes into an image). virStorageVolWipe can only wipe the entire volume, but virStorageVolUpload could be used to intentionally wipe any given offset and length via a flag.
I don't think wiping really fits in practically with the way streams operate, since this isn't a synchronous API.
Agreed. So with that, here's the incremental changes I recommend that you squash in, and then you have my: ACK diff --git i/src/libvirt.c w/src/libvirt.c index f09421f..df7df7a 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -9067,14 +9067,19 @@ error: /** * virStorageVolDownload: - * @pool: pointer to volume to download + * @pool: pointer to volume to download from * @stream: stream to use as output - * @offset: position to start reading from + * @offset: position in @pool to start reading from * @length: limit on amount of data to download - * @flags: flags for creation (unused, pass 0) + * @flags: future flags (unused, pass 0) * * Download the content of the volume as a stream. If @length - * is zero, then the entire file contents will be downloaded. + * is zero, then the contents of volume after @offset will be downloaded. + * + * This call sets up an asynchronous stream; subsequent use of + * stream APIs is necessary to determine how much data is successfully + * transferred. Use caution when multiple streams are visiting the + * same volume simultaneously. * * Returns 0, or -1 upon error. */ @@ -9085,7 +9090,8 @@ virStorageVolDownload(virStorageVolPtr vol, unsigned long long length, unsigned int flags) { - VIR_DEBUG("vol=%p stream=%p offset=%llu length=%llu flags=%u", vol, stream, offset, length, flags); + VIR_DEBUG("vol=%p stream=%p offset=%llu length=%llu flags=%u", + vol, stream, offset, length, flags); virResetLastError(); @@ -9128,15 +9134,21 @@ error: /** * virStorageVolUpload: - * @pool: pointer to volume to download - * @stream: stream to use as output - * @offset: position to start writing to + * @pool: pointer to volume to upload into + * @stream: stream to use as input + * @offset: position in @pool to start writing to * @length: limit on amount of data to upload - * @flags: flags for creation (unused, pass 0) + * @flags: future flags (unused, pass 0) + * + * Upload new content to the volume from a stream. This call will + * fail if @offset + @length exceeds the volume size; otherwise, + * if @length is non-zero, the stream will raise an error if an + * attempt is made to upload greater than @length bytes of data. * - * Upload new content to the volume from a stream. If @length - * is non-zero, and an error will be raised if an attempt is - * made to upload greater than @length bytes of data. + * This call sets up an asynchronous stream; subsequent use of + * stream APIs is necessary to determine how much data is successfully + * transferred. Use caution when multiple streams are visiting the + * same volume simultaneously. * * Returns 0, or -1 upon error. */ -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Mar 22, 2011 at 08:22:05AM -0600, Eric Blake wrote:
On 03/22/2011 05:45 AM, Daniel P. Berrange wrote:
What happens if the stream hits EOF before all data read from the volume has been written to the stream? Should the API support a way to tell how many bytes were successfully downloaded in the case of a short stream? That is, instead of returning 0 on success, should Upload and Download return a ull with how many bytes were transferred in/out of the stream?
Everything related to actual I/O is done virStreamRecv/virStreamSend. These APIs merely open the volume and associate a stream with it. So all these questions about number of bytes / EOF are not relevant in this context.
Yeah, I figured that out in patch 3/6.
Still, there were some improvements to make to the wording.
These days, efficient sparse handling is a cool kernel feature waiting to be exploited (see coreutils 8.10 use of FIEMAP in cp). Should we make it easier to detect holes in a volume, by exposing some of this information back through this API?
The concept of sparseness doesn't really fit into streams because they're not seekable. With download, to avoid repeated round-trips, the client doesn't actually issue individual read() calls to pull data over the RPC. Instead the server pushes the data down to the client continuously until EOF on the server.
Then we can save sparseness for another day and another patch. I imagine it might be possible to extend streams to support some OOB sparseness data (if both server and client know how to interpret OOB, then both sides can skip over holes in the transfer; if either side lacks support for OOB, then holes are transferred as normal data).
For that matter, that sounds like a great use case for wiping a portion of a volume (aka punching holes into an image). virStorageVolWipe can only wipe the entire volume, but virStorageVolUpload could be used to intentionally wipe any given offset and length via a flag.
I don't think wiping really fits in practically with the way streams operate, since this isn't a synchronous API.
Agreed. So with that, here's the incremental changes I recommend that you squash in, and then you have my:
ACK
diff --git i/src/libvirt.c w/src/libvirt.c index f09421f..df7df7a 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -9067,14 +9067,19 @@ error:
/** * virStorageVolDownload: - * @pool: pointer to volume to download + * @pool: pointer to volume to download from * @stream: stream to use as output - * @offset: position to start reading from + * @offset: position in @pool to start reading from * @length: limit on amount of data to download - * @flags: flags for creation (unused, pass 0) + * @flags: future flags (unused, pass 0) * * Download the content of the volume as a stream. If @length - * is zero, then the entire file contents will be downloaded. + * is zero, then the contents of volume after @offset will be downloaded. + * + * This call sets up an asynchronous stream; subsequent use of + * stream APIs is necessary to determine how much data is successfully + * transferred. Use caution when multiple streams are visiting the + * same volume simultaneously. * * Returns 0, or -1 upon error. */ @@ -9085,7 +9090,8 @@ virStorageVolDownload(virStorageVolPtr vol, unsigned long long length, unsigned int flags) { - VIR_DEBUG("vol=%p stream=%p offset=%llu length=%llu flags=%u", vol, stream, offset, length, flags); + VIR_DEBUG("vol=%p stream=%p offset=%llu length=%llu flags=%u", + vol, stream, offset, length, flags);
virResetLastError();
@@ -9128,15 +9134,21 @@ error:
/** * virStorageVolUpload: - * @pool: pointer to volume to download - * @stream: stream to use as output - * @offset: position to start writing to + * @pool: pointer to volume to upload into + * @stream: stream to use as input + * @offset: position in @pool to start writing to * @length: limit on amount of data to upload - * @flags: flags for creation (unused, pass 0) + * @flags: future flags (unused, pass 0) + * + * Upload new content to the volume from a stream. This call will + * fail if @offset + @length exceeds the volume size; otherwise, + * if @length is non-zero, the stream will raise an error if an + * attempt is made to upload greater than @length bytes of data. * - * Upload new content to the volume from a stream. If @length - * is non-zero, and an error will be raised if an attempt is - * made to upload greater than @length bytes of data. + * This call sets up an asynchronous stream; subsequent use of + * stream APIs is necessary to determine how much data is successfully + * transferred. Use caution when multiple streams are visiting the + * same volume simultaneously. * * Returns 0, or -1 upon error. */
I think we can change that last sentence to "If another stream is currently writing to the volume, the results may be unpredictable" since multiple readers has no risk at all. Only multiple writers, or a mix of readers+writer(s) Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
* tools/virsh.c: Add vol-create-upload, vol-upload and vol-download commands --- tools/virsh.c | 218 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 218 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 50ca50f..e30d39c 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -266,6 +266,9 @@ static int vshCommandOptString(const vshCmd *cmd, const char *name, static int vshCommandOptLongLong(const vshCmd *cmd, const char *name, long long *value) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; +static int vshCommandOptULongLong(const vshCmd *cmd, const char *name, + unsigned long long *value) + ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; static int vshCommandOptBool(const vshCmd *cmd, const char *name); static char *vshCommandOptArgv(const vshCmd *cmd, int count); @@ -7082,6 +7085,197 @@ cleanup: return ret; } + +/* + * "vol-upload" command + */ +static const vshCmdInfo info_vol_upload[] = { + {"help", N_("upload a file into a volume")}, + {"desc", N_("Upload a file into a volume")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_vol_upload[] = { + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, + {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, + {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file")}, + {"offset", VSH_OT_INT, 0, N_("volume offset to upload to") }, + {"length", VSH_OT_INT, 0, N_("amount of data to upload") }, + {NULL, 0, 0, NULL} +}; + +static int +cmdVolUploadSource(virStreamPtr st ATTRIBUTE_UNUSED, + char *bytes, size_t nbytes, void *opaque) +{ + int *fd = opaque; + + return saferead(*fd, bytes, nbytes); +} + +static int +cmdVolUpload (vshControl *ctl, const vshCmd *cmd) +{ + const char *file = NULL; + virStorageVolPtr vol = NULL; + int ret = FALSE; + int fd = -1; + virStreamPtr st = NULL; + const char *name = NULL; + unsigned long long offset = 0, length = 0; + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto cleanup; + + if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", &name))) { + return FALSE; + } + + if (vshCommandOptULongLong(cmd, "offset", &offset) < 0) + return FALSE; + + if (vshCommandOptULongLong(cmd, "length", &length) < 0) + return FALSE; + + if (vshCommandOptString(cmd, "file", &file) < 0) + goto cleanup; + + if ((fd = open(file, O_RDONLY)) < 0) { + vshError(ctl, "cannot read %s", file); + goto cleanup; + } + + st = virStreamNew(ctl->conn, 0); + if (virStorageVolUpload(vol, st, offset, length, 0) < 0) { + vshError(ctl, "cannot upload to volume %s", name); + goto cleanup; + } + + if (virStreamSendAll(st, cmdVolUploadSource, &fd) < 0) { + vshError(ctl, "cannot send data to volume %s", name); + goto cleanup; + } + + if (VIR_CLOSE(fd) < 0) { + vshError(ctl, "cannot close file %s", file); + virStreamAbort(st); + goto cleanup; + } + + if (virStreamFinish(st) < 0) { + vshError(ctl, "cannot close volume %s", name); + goto cleanup; + } + + ret = TRUE; + +cleanup: + if (vol) + virStorageVolFree(vol); + if (st) + virStreamFree(st); + VIR_FORCE_CLOSE(fd); + return ret; +} + + + +/* + * "vol-download" command + */ +static const vshCmdInfo info_vol_download[] = { + {"help", N_("Download a volume to a file")}, + {"desc", N_("Download a volume to a file")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_vol_download[] = { + {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, + {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, + {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file")}, + {"offset", VSH_OT_INT, 0, N_("volume offset to download from") }, + {"length", VSH_OT_INT, 0, N_("amount of data to download") }, + {NULL, 0, 0, NULL} +}; + + +static int +cmdVolDownloadSink(virStreamPtr st ATTRIBUTE_UNUSED, + const char *bytes, size_t nbytes, void *opaque) +{ + int *fd = opaque; + + return safewrite(*fd, bytes, nbytes); +} + +static int +cmdVolDownload (vshControl *ctl, const vshCmd *cmd) +{ + const char *file = NULL; + virStorageVolPtr vol = NULL; + int ret = FALSE; + int fd = -1; + virStreamPtr st = NULL; + const char *name = NULL; + unsigned long long offset = 0, length = 0; + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto cleanup; + + if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", &name))) { + return FALSE; + } + + if (vshCommandOptULongLong(cmd, "offset", &offset) < 0) + return FALSE; + + if (vshCommandOptULongLong(cmd, "length", &length) < 0) + return FALSE; + + if (vshCommandOptString(cmd, "file", &file) < 0) + goto cleanup; + + if ((fd = open(file, O_WRONLY|O_TRUNC|O_CREAT, 0666)) < 0) { + vshError(ctl, "cannot create %s", file); + goto cleanup; + } + + st = virStreamNew(ctl->conn, 0); + if (virStorageVolDownload(vol, st, offset, length, 0) < 0) { + vshError(ctl, "cannot download from volume %s", name); + goto cleanup; + } + + if (virStreamRecvAll(st, cmdVolDownloadSink, &fd) < 0) { + vshError(ctl, "cannot receive data from volume %s", name); + goto cleanup; + } + + if (VIR_CLOSE(fd) < 0) { + vshError(ctl, "cannot close file %s", file); + virStreamAbort(st); + goto cleanup; + } + + if (virStreamFinish(st) < 0) { + vshError(ctl, "cannot close volume %s", name); + goto cleanup; + } + + ret = TRUE; + +cleanup: + if (ret == FALSE) + unlink(file); + if (vol) + virStorageVolFree(vol); + if (st) + virStreamFree(st); + VIR_FORCE_CLOSE(fd); + return ret; +} + + /* * "vol-delete" command */ @@ -9901,6 +10095,7 @@ cmdEdit (vshControl *ctl, const vshCmd *cmd) return ret; } + /* * "net-edit" command */ @@ -10538,6 +10733,7 @@ static const vshCmdDef storageVolCmds[] = { {"vol-create", cmdVolCreate, opts_vol_create, info_vol_create}, {"vol-create-from", cmdVolCreateFrom, opts_vol_create_from, info_vol_create_from}, {"vol-delete", cmdVolDelete, opts_vol_delete, info_vol_delete}, + {"vol-download", cmdVolDownload, opts_vol_download, info_vol_download }, {"vol-dumpxml", cmdVolDumpXML, opts_vol_dumpxml, info_vol_dumpxml}, {"vol-info", cmdVolInfo, opts_vol_info, info_vol_info}, {"vol-key", cmdVolKey, opts_vol_key, info_vol_key}, @@ -10545,6 +10741,7 @@ static const vshCmdDef storageVolCmds[] = { {"vol-name", cmdVolName, opts_vol_name, info_vol_name}, {"vol-path", cmdVolPath, opts_vol_path, info_vol_path}, {"vol-pool", cmdVolPool, opts_vol_pool, info_vol_pool}, + {"vol-upload", cmdVolUpload, opts_vol_upload, info_vol_upload }, {"vol-wipe", cmdVolWipe, opts_vol_wipe, info_vol_wipe}, {NULL, NULL, NULL, NULL} }; @@ -11037,6 +11234,27 @@ vshCommandOptLongLong(const vshCmd *cmd, const char *name, return ret; } +static int +vshCommandOptULongLong(const vshCmd *cmd, const char *name, + unsigned long long *value) +{ + vshCmdOpt *arg = vshCommandOpt(cmd, name); + int ret = 0; + unsigned long long num; + char *end_p = NULL; + + if ((arg != NULL) && (arg->data != NULL)) { + num = strtoull(arg->data, &end_p, 10); + ret = -1; + if ((arg->data != end_p) && (*end_p == 0)) { + *value = num; + ret = 1; + } + } + return ret; +} + + /* * Returns TRUE/FALSE if the option exists */ -- 1.7.4
On 03/18/2011 10:36 AM, Daniel P. Berrange wrote:
* tools/virsh.c: Add vol-create-upload, vol-upload and vol-download commands
Another stale commit message. s/vol-create-upload, //
+static int +cmdVolUpload (vshControl *ctl, const vshCmd *cmd) +{ + const char *file = NULL; + virStorageVolPtr vol = NULL; + int ret = FALSE; + int fd = -1; + virStreamPtr st = NULL; + const char *name = NULL; + unsigned long long offset = 0, length = 0; + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto cleanup; + + if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", &name))) { + return FALSE; + } + + if (vshCommandOptULongLong(cmd, "offset", &offset) < 0) + return FALSE; + + if (vshCommandOptULongLong(cmd, "length", &length) < 0) + return FALSE;
This returns FALSE without an error message, not to mention that it leaks vol. Should be: if (vshCommandOptULongLong(cmd, "offset", &offset) < 0 || vshCommandOptULongLong(cmd, "length", &length) < 0) { vshError(ctl, "%s", _("Unable to parse integer parameter")); goto cleanup; }
+ st = virStreamNew(ctl->conn, 0); + if (virStorageVolUpload(vol, st, offset, length, 0) < 0) { + vshError(ctl, "cannot upload to volume %s", name); + goto cleanup; + }
Oh. I see - virStorageVolUpload _can't_ return how many bytes were read. It is the start of an asynchronous data transfer, and can only return 0 if the stream was successfully tied to the volume...
+ + if (virStreamSendAll(st, cmdVolUploadSource, &fd) < 0) { + vshError(ctl, "cannot send data to volume %s", name); + goto cleanup; + }
...and it isn't until the virStreamSendAll runs that you know how many bytes were transferred. That impacts some of my comments to patch 2/6. Do we need any protection that a volume can only be tied to one stream at a time, so if an upload or download is already in progress, then attempts to attach another stream will fail? This is a potentially long-running transaction. Should virsh be taught how to do SIGINT interruption of the command, or even how to do a progress indicator of how many bytes remain to pass through the stream?
+ if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", &name))) { + return FALSE; + } + + if (vshCommandOptULongLong(cmd, "offset", &offset) < 0) + return FALSE; + + if (vshCommandOptULongLong(cmd, "length", &length) < 0) + return FALSE;
Same usage problem and leak of vol.
+ + if (vshCommandOptString(cmd, "file", &file) < 0) + goto cleanup; + + if ((fd = open(file, O_WRONLY|O_TRUNC|O_CREAT, 0666)) < 0) {
No optional --mode command for specifying the permissions to give to a newly created file?
+ vshError(ctl, "cannot create %s", file); + goto cleanup; + } + + st = virStreamNew(ctl->conn, 0); + if (virStorageVolDownload(vol, st, offset, length, 0) < 0) { + vshError(ctl, "cannot download from volume %s", name); + goto cleanup; + } + + if (virStreamRecvAll(st, cmdVolDownloadSink, &fd) < 0) { + vshError(ctl, "cannot receive data from volume %s", name); + goto cleanup; + } + + if (VIR_CLOSE(fd) < 0) { + vshError(ctl, "cannot close file %s", file); + virStreamAbort(st); + goto cleanup; + } + + if (virStreamFinish(st) < 0) { + vshError(ctl, "cannot close volume %s", name); + goto cleanup; + } + + ret = TRUE; + +cleanup: + if (ret == FALSE) + unlink(file);
Is it wise to blindly unlink() the file even if we didn't create it? If you insist on blind unlink(), then I'd feel more comfortable with O_EXCL on open(), but I don't know that we can justify forbidding to overwrite existing files. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Mar 21, 2011 at 03:23:24PM -0600, Eric Blake wrote:
On 03/18/2011 10:36 AM, Daniel P. Berrange wrote:
* tools/virsh.c: Add vol-create-upload, vol-upload and vol-download commands
Another stale commit message.
s/vol-create-upload, //
+static int +cmdVolUpload (vshControl *ctl, const vshCmd *cmd) +{ + const char *file = NULL; + virStorageVolPtr vol = NULL; + int ret = FALSE; + int fd = -1; + virStreamPtr st = NULL; + const char *name = NULL; + unsigned long long offset = 0, length = 0; + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto cleanup; + + if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", &name))) { + return FALSE; + } + + if (vshCommandOptULongLong(cmd, "offset", &offset) < 0) + return FALSE; + + if (vshCommandOptULongLong(cmd, "length", &length) < 0) + return FALSE;
This returns FALSE without an error message, not to mention that it leaks vol. Should be:
if (vshCommandOptULongLong(cmd, "offset", &offset) < 0 || vshCommandOptULongLong(cmd, "length", &length) < 0) { vshError(ctl, "%s", _("Unable to parse integer parameter")); goto cleanup; }
+ st = virStreamNew(ctl->conn, 0); + if (virStorageVolUpload(vol, st, offset, length, 0) < 0) { + vshError(ctl, "cannot upload to volume %s", name); + goto cleanup; + }
Oh. I see - virStorageVolUpload _can't_ return how many bytes were read. It is the start of an asynchronous data transfer, and can only return 0 if the stream was successfully tied to the volume...
+ + if (virStreamSendAll(st, cmdVolUploadSource, &fd) < 0) { + vshError(ctl, "cannot send data to volume %s", name); + goto cleanup; + }
...and it isn't until the virStreamSendAll runs that you know how many bytes were transferred. That impacts some of my comments to patch 2/6.
Do we need any protection that a volume can only be tied to one stream at a time, so if an upload or download is already in progress, then attempts to attach another stream will fail?
Server side, we don't really maintain any state against the volume for open streams, so nothing seriously bad would occurr if multiple streams were open for the same file. Of course you can't expect particularly useful results if you upload to the same volume, but there's no compelling reasons to explicitly forbid.
This is a potentially long-running transaction. Should virsh be taught how to do SIGINT interruption of the command, or even how to do a progress indicator of how many bytes remain to pass through the stream?
Doing a progress indicator is a possibility, but it would require virsh to use virStreamSend/Recv directly instead of the SendAll/ RecvAll helpers. I think this can be done as an enhancement later.
+ if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", &name))) { + return FALSE; + } + + if (vshCommandOptULongLong(cmd, "offset", &offset) < 0) + return FALSE; + + if (vshCommandOptULongLong(cmd, "length", &length) < 0) + return FALSE;
Same usage problem and leak of vol.
+ + if (vshCommandOptString(cmd, "file", &file) < 0) + goto cleanup; + + if ((fd = open(file, O_WRONLY|O_TRUNC|O_CREAT, 0666)) < 0) {
No optional --mode command for specifying the permissions to give to a newly created file?
The automatic application of the clients' umask is sufficient IMHO.
+ vshError(ctl, "cannot create %s", file); + goto cleanup; + } + + st = virStreamNew(ctl->conn, 0); + if (virStorageVolDownload(vol, st, offset, length, 0) < 0) { + vshError(ctl, "cannot download from volume %s", name); + goto cleanup; + } + + if (virStreamRecvAll(st, cmdVolDownloadSink, &fd) < 0) { + vshError(ctl, "cannot receive data from volume %s", name); + goto cleanup; + } + + if (VIR_CLOSE(fd) < 0) { + vshError(ctl, "cannot close file %s", file); + virStreamAbort(st); + goto cleanup; + } + + if (virStreamFinish(st) < 0) { + vshError(ctl, "cannot close volume %s", name); + goto cleanup; + } + + ret = TRUE; + +cleanup: + if (ret == FALSE) + unlink(file);
Is it wise to blindly unlink() the file even if we didn't create it? If you insist on blind unlink(), then I'd feel more comfortable with O_EXCL on open(), but I don't know that we can justify forbidding to overwrite existing files.
Well we could open O_EXCL and then retry without it, if it fails with EEXIST Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
* src/storage/storage_driver.c: Wire up upload/download APIs --- src/storage/storage_driver.c | 133 +++++++++++++++++++++++++++++++++++++++++- 1 files changed, 131 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ce528cf..706db74 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -46,6 +46,7 @@ #include "storage_backend.h" #include "logging.h" #include "files.h" +#include "fdstream.h" #include "configmake.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -1528,6 +1529,134 @@ cleanup: } +static int +storageVolumeDownload(virStorageVolPtr obj, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags) +{ + virStorageDriverStatePtr driver = obj->conn->storagePrivateData; + virStoragePoolObjPtr pool = NULL; + virStorageVolDefPtr vol = NULL; + int ret = -1; + + virCheckFlags(0, -1); + + storageDriverLock(driver); + pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); + storageDriverUnlock(driver); + + if (!pool) { + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, + "%s", _("no storage pool with matching uuid")); + goto out; + } + + if (!virStoragePoolObjIsActive(pool)) { + virStorageReportError(VIR_ERR_OPERATION_INVALID, + "%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 (virFDStreamOpenFile(stream, + vol->target.path, + offset, length, + O_RDONLY) < 0) + goto out; + + ret = 0; + +out: + if (pool) + virStoragePoolObjUnlock(pool); + + return ret; +} + + +static int +storageVolumeUpload(virStorageVolPtr obj, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags) +{ + virStorageDriverStatePtr driver = obj->conn->storagePrivateData; + virStoragePoolObjPtr pool = NULL; + virStorageVolDefPtr vol = NULL; + int ret = -1; + + virCheckFlags(0, -1); + + storageDriverLock(driver); + pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); + storageDriverUnlock(driver); + + if (!pool) { + virStorageReportError(VIR_ERR_NO_STORAGE_POOL, + "%s", _("no storage pool with matching uuid")); + goto out; + } + + if (!virStoragePoolObjIsActive(pool)) { + virStorageReportError(VIR_ERR_OPERATION_INVALID, + "%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; + } + + + /* Not using O_CREAT because the file is required to + * already exist at this point */ + if (virFDStreamOpenFile(stream, + vol->target.path, + offset, length, + O_WRONLY) < 0) + goto out; + + ret = 0; + +out: + if (pool) + virStoragePoolObjUnlock(pool); + + 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: @@ -1989,8 +2118,8 @@ static virStorageDriver storageDriver = { .volLookupByPath = storageVolumeLookupByPath, .volCreateXML = storageVolumeCreateXML, .volCreateXMLFrom = storageVolumeCreateXMLFrom, - .volDownload = NULL, - .volUpload = NULL, + .volDownload = storageVolumeDownload, + .volUpload = storageVolumeUpload, .volDelete = storageVolumeDelete, .volWipe = storageVolumeWipe, .volGetInfo = storageVolumeGetInfo, -- 1.7.4
On 03/18/2011 10:36 AM, Daniel P. Berrange wrote:
* src/storage/storage_driver.c: Wire up upload/download APIs --- src/storage/storage_driver.c | 133 +++++++++++++++++++++++++++++++++++++++++- 1 files changed, 131 insertions(+), 2 deletions(-)
+static int +storageVolumeDownload(virStorageVolPtr obj, + virStreamPtr stream, + unsigned long long offset, + unsigned long long length, + unsigned int flags) +{ + if (vol->building) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + _("volume '%s' is still being allocated."), + vol->name); + goto out; + } + + if (virFDStreamOpenFile(stream, + vol->target.path, + offset, length, + O_RDONLY) < 0) + goto out;
Should we prohibit more than one stream attached to a volume at a time? It could be rather confusing if simultaneous transfers are taking place to the same volume, especially if the transfers overlap in offsets of the volume being addressed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* daemon/remote.c, src/remote/remote_driver.c: Implementation of storage vol upload/download APIs * src/remote/remote_protocol.x: Wire protocol definition for upload/download * daemon/remote_dispatch_args.h, daemon/remote_dispatch_prototypes.h, daemon/remote_dispatch_table.h, src/remote/remote_protocol.h, src/remote/remote_protocol.c: Re-generate --- daemon/remote.c | 92 +++++++++++++++++++++++++++++++++++ daemon/remote_dispatch_args.h | 2 + daemon/remote_dispatch_prototypes.h | 16 ++++++ daemon/remote_dispatch_table.h | 10 ++++ src/remote/remote_driver.c | 87 +++++++++++++++++++++++++++++++++ src/remote/remote_protocol.c | 30 +++++++++++ src/remote/remote_protocol.h | 22 ++++++++ src/remote/remote_protocol.x | 19 +++++++- src/remote_protocol-structs | 12 +++++ 9 files changed, 289 insertions(+), 1 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index f410982..7e8103c 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5816,7 +5816,99 @@ remoteDispatchNodeDeviceDestroy(struct qemud_server *server ATTRIBUTE_UNUSED, virNodeDeviceFree(dev); return 0; } +static int remoteDispatchStorageVolUpload(struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client, + virConnectPtr conn, + remote_message_header *hdr, + remote_error *rerr, + remote_storage_vol_upload_args *args, + void *ret ATTRIBUTE_UNUSED) +{ + int rv = -1; + struct qemud_client_stream *stream = NULL; + virStorageVolPtr vol; + + vol = get_nonnull_storage_vol(conn, args->vol); + if (vol == NULL) { + remoteDispatchConnError(rerr, conn); + goto cleanup; + } + + stream = remoteCreateClientStream(conn, hdr); + if (!stream) { + remoteDispatchConnError(rerr, conn); + goto cleanup; + } + + if (virStorageVolUpload(vol, stream->st, + args->offset, args->length, + args->flags) < 0) { + remoteDispatchConnError(rerr, conn); + goto cleanup; + } + + if (remoteAddClientStream(client, stream, 0) < 0) { + remoteDispatchConnError(rerr, conn); + virStreamAbort(stream->st); + goto cleanup; + } + + rv = 0; + +cleanup: + if (vol) + virStorageVolFree(vol); + if (stream && rv != 0) + remoteFreeClientStream(client, stream); + return rv; +} +static int remoteDispatchStorageVolDownload(struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client, + virConnectPtr conn, + remote_message_header *hdr, + remote_error *rerr, + remote_storage_vol_download_args *args, + void *ret ATTRIBUTE_UNUSED) +{ + int rv = -1; + struct qemud_client_stream *stream = NULL; + virStorageVolPtr vol; + + vol = get_nonnull_storage_vol (conn, args->vol); + if (vol == NULL) { + remoteDispatchConnError(rerr, conn); + goto cleanup; + } + + stream = remoteCreateClientStream(conn, hdr); + if (!stream) { + remoteDispatchConnError(rerr, conn); + goto cleanup; + } + + if (virStorageVolDownload(vol, stream->st, + args->offset, args->length, + args->flags) < 0) { + remoteDispatchConnError(rerr, conn); + goto cleanup; + } + + if (remoteAddClientStream(client, stream, 1) < 0) { + remoteDispatchConnError(rerr, conn); + virStreamAbort(stream->st); + goto cleanup; + } + + rv = 0; + +cleanup: + if (vol) + virStorageVolFree(vol); + if (stream && rv != 0) + remoteFreeClientStream(client, stream); + return rv; +} /*************************** diff --git a/daemon/remote_dispatch_args.h b/daemon/remote_dispatch_args.h index b32ae1f..da378c1 100644 --- a/daemon/remote_dispatch_args.h +++ b/daemon/remote_dispatch_args.h @@ -175,3 +175,5 @@ remote_domain_set_memory_flags_args val_remote_domain_set_memory_flags_args; remote_domain_set_blkio_parameters_args val_remote_domain_set_blkio_parameters_args; remote_domain_get_blkio_parameters_args val_remote_domain_get_blkio_parameters_args; + remote_storage_vol_upload_args val_remote_storage_vol_upload_args; + remote_storage_vol_download_args val_remote_storage_vol_download_args; diff --git a/daemon/remote_dispatch_prototypes.h b/daemon/remote_dispatch_prototypes.h index 4db6c76..65f7f8e 100644 --- a/daemon/remote_dispatch_prototypes.h +++ b/daemon/remote_dispatch_prototypes.h @@ -1530,6 +1530,14 @@ static int remoteDispatchStorageVolDelete( remote_error *err, remote_storage_vol_delete_args *args, void *ret); +static int remoteDispatchStorageVolDownload( + struct qemud_server *server, + struct qemud_client *client, + virConnectPtr conn, + remote_message_header *hdr, + remote_error *err, + remote_storage_vol_download_args *args, + void *ret); static int remoteDispatchStorageVolDumpXml( struct qemud_server *server, struct qemud_client *client, @@ -1578,6 +1586,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 remoteDispatchStorageVolUpload( + struct qemud_server *server, + struct qemud_client *client, + virConnectPtr conn, + remote_message_header *hdr, + remote_error *err, + remote_storage_vol_upload_args *args, + void *ret); static int remoteDispatchStorageVolWipe( struct qemud_server *server, struct qemud_client *client, diff --git a/daemon/remote_dispatch_table.h b/daemon/remote_dispatch_table.h index c50d038..06b083e 100644 --- a/daemon/remote_dispatch_table.h +++ b/daemon/remote_dispatch_table.h @@ -1037,3 +1037,13 @@ .args_filter = (xdrproc_t) xdr_remote_domain_get_blkio_parameters_args, .ret_filter = (xdrproc_t) xdr_remote_domain_get_blkio_parameters_ret, }, +{ /* StorageVolUpload => 207 */ + .fn = (dispatch_fn) remoteDispatchStorageVolUpload, + .args_filter = (xdrproc_t) xdr_remote_storage_vol_upload_args, + .ret_filter = (xdrproc_t) xdr_void, +}, +{ /* StorageVolDownload => 208 */ + .fn = (dispatch_fn) remoteDispatchStorageVolDownload, + .args_filter = (xdrproc_t) xdr_remote_storage_vol_download_args, + .ret_filter = (xdrproc_t) xdr_void, +}, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 8aa8801..16db847 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -9563,6 +9563,91 @@ done: return rv; } +static int +remoteStorageVolUpload(virStorageVolPtr vol, + virStreamPtr st, + unsigned long long offset, + unsigned long long length, + unsigned int flags) +{ + struct private_data *priv = vol->conn->privateData; + struct private_stream_data *privst = NULL; + int rv = -1; + remote_storage_vol_upload_args args; + + remoteDriverLock(priv); + + if (!(privst = remoteStreamOpen(st, 1, + REMOTE_PROC_STORAGE_VOL_UPLOAD, + priv->counter))) + goto done; + + st->driver = &remoteStreamDrv; + st->privateData = privst; + + make_nonnull_storage_vol(&args.vol, vol); + args.offset = offset; + args.length = length; + args.flags = flags; + + if (call (vol->conn, priv, 0, REMOTE_PROC_STORAGE_VOL_UPLOAD, + (xdrproc_t) xdr_remote_storage_vol_upload_args, (char *) &args, + (xdrproc_t) xdr_void, NULL) == -1) { + remoteStreamRelease(st); + goto done; + } + + rv = 0; + +done: + remoteDriverUnlock(priv); + + return rv; +} + + +static int +remoteStorageVolDownload(virStorageVolPtr vol, + virStreamPtr st, + unsigned long long offset, + unsigned long long length, + unsigned int flags) +{ + struct private_data *priv = vol->conn->privateData; + struct private_stream_data *privst = NULL; + int rv = -1; + remote_storage_vol_download_args args; + + remoteDriverLock(priv); + + if (!(privst = remoteStreamOpen(st, 1, + REMOTE_PROC_STORAGE_VOL_DOWNLOAD, + priv->counter))) + goto done; + + st->driver = &remoteStreamDrv; + st->privateData = privst; + + make_nonnull_storage_vol(&args.vol, vol); + args.offset = offset; + args.length = length; + args.flags = flags; + + if (call (vol->conn, priv, 0, REMOTE_PROC_STORAGE_VOL_DOWNLOAD, + (xdrproc_t) xdr_remote_storage_vol_download_args, (char *) &args, + (xdrproc_t) xdr_void, NULL) == -1) { + remoteStreamRelease(st); + goto done; + } + + rv = 0; + +done: + remoteDriverUnlock(priv); + + return rv; +} + static int remoteDomainOpenConsole(virDomainPtr dom, @@ -11233,6 +11318,8 @@ static virStorageDriver storage_driver = { .volLookupByPath = remoteStorageVolLookupByPath, .volCreateXML = remoteStorageVolCreateXML, .volCreateXMLFrom = remoteStorageVolCreateXMLFrom, + .volDownload = remoteStorageVolDownload, + .volUpload = remoteStorageVolUpload, .volDelete = remoteStorageVolDelete, .volWipe = remoteStorageVolWipe, .volGetInfo = remoteStorageVolGetInfo, diff --git a/src/remote/remote_protocol.c b/src/remote/remote_protocol.c index ea2bdf7..9cadcb7 100644 --- a/src/remote/remote_protocol.c +++ b/src/remote/remote_protocol.c @@ -3859,6 +3859,36 @@ xdr_remote_domain_open_console_args (XDR *xdrs, remote_domain_open_console_args } bool_t +xdr_remote_storage_vol_upload_args (XDR *xdrs, remote_storage_vol_upload_args *objp) +{ + + if (!xdr_remote_nonnull_storage_vol (xdrs, &objp->vol)) + return FALSE; + if (!xdr_uint64_t (xdrs, &objp->offset)) + return FALSE; + if (!xdr_uint64_t (xdrs, &objp->length)) + return FALSE; + if (!xdr_u_int (xdrs, &objp->flags)) + return FALSE; + return TRUE; +} + +bool_t +xdr_remote_storage_vol_download_args (XDR *xdrs, remote_storage_vol_download_args *objp) +{ + + if (!xdr_remote_nonnull_storage_vol (xdrs, &objp->vol)) + return FALSE; + if (!xdr_uint64_t (xdrs, &objp->offset)) + return FALSE; + if (!xdr_uint64_t (xdrs, &objp->length)) + return FALSE; + if (!xdr_u_int (xdrs, &objp->flags)) + return FALSE; + return TRUE; +} + +bool_t xdr_remote_procedure (XDR *xdrs, remote_procedure *objp) { diff --git a/src/remote/remote_protocol.h b/src/remote/remote_protocol.h index a55f7c4..4bf2b0f 100644 --- a/src/remote/remote_protocol.h +++ b/src/remote/remote_protocol.h @@ -2177,6 +2177,22 @@ struct remote_domain_open_console_args { u_int flags; }; typedef struct remote_domain_open_console_args remote_domain_open_console_args; + +struct remote_storage_vol_upload_args { + remote_nonnull_storage_vol vol; + uint64_t offset; + uint64_t length; + u_int flags; +}; +typedef struct remote_storage_vol_upload_args remote_storage_vol_upload_args; + +struct remote_storage_vol_download_args { + remote_nonnull_storage_vol vol; + uint64_t offset; + uint64_t length; + u_int flags; +}; +typedef struct remote_storage_vol_download_args remote_storage_vol_download_args; #define REMOTE_PROGRAM 0x20008086 #define REMOTE_PROTOCOL_VERSION 1 @@ -2387,6 +2403,8 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SET_MEMORY_FLAGS = 204, REMOTE_PROC_DOMAIN_SET_BLKIO_PARAMETERS = 205, REMOTE_PROC_DOMAIN_GET_BLKIO_PARAMETERS = 206, + REMOTE_PROC_STORAGE_VOL_UPLOAD = 207, + REMOTE_PROC_STORAGE_VOL_DOWNLOAD = 208, }; typedef enum remote_procedure remote_procedure; @@ -2767,6 +2785,8 @@ extern bool_t xdr_remote_domain_snapshot_current_ret (XDR *, remote_domain_snap extern bool_t xdr_remote_domain_revert_to_snapshot_args (XDR *, remote_domain_revert_to_snapshot_args*); extern bool_t xdr_remote_domain_snapshot_delete_args (XDR *, remote_domain_snapshot_delete_args*); extern bool_t xdr_remote_domain_open_console_args (XDR *, remote_domain_open_console_args*); +extern bool_t xdr_remote_storage_vol_upload_args (XDR *, remote_storage_vol_upload_args*); +extern bool_t xdr_remote_storage_vol_download_args (XDR *, remote_storage_vol_download_args*); extern bool_t xdr_remote_procedure (XDR *, remote_procedure*); extern bool_t xdr_remote_message_type (XDR *, remote_message_type*); extern bool_t xdr_remote_message_status (XDR *, remote_message_status*); @@ -3121,6 +3141,8 @@ extern bool_t xdr_remote_domain_snapshot_current_ret (); extern bool_t xdr_remote_domain_revert_to_snapshot_args (); extern bool_t xdr_remote_domain_snapshot_delete_args (); extern bool_t xdr_remote_domain_open_console_args (); +extern bool_t xdr_remote_storage_vol_upload_args (); +extern bool_t xdr_remote_storage_vol_download_args (); extern bool_t xdr_remote_procedure (); extern bool_t xdr_remote_message_type (); extern bool_t xdr_remote_message_status (); diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index aa710a4..4bcad65 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1920,6 +1920,21 @@ struct remote_domain_open_console_args { unsigned int flags; }; +struct remote_storage_vol_upload_args { + remote_nonnull_storage_vol vol; + unsigned hyper offset; + unsigned hyper length; + unsigned int flags; +}; + +struct remote_storage_vol_download_args { + remote_nonnull_storage_vol vol; + unsigned hyper offset; + unsigned hyper length; + unsigned int flags; +}; + + /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -2152,7 +2167,9 @@ enum remote_procedure { REMOTE_PROC_GET_SYSINFO = 203, REMOTE_PROC_DOMAIN_SET_MEMORY_FLAGS = 204, REMOTE_PROC_DOMAIN_SET_BLKIO_PARAMETERS = 205, - REMOTE_PROC_DOMAIN_GET_BLKIO_PARAMETERS = 206 + REMOTE_PROC_DOMAIN_GET_BLKIO_PARAMETERS = 206, + REMOTE_PROC_STORAGE_VOL_UPLOAD = 207, + REMOTE_PROC_STORAGE_VOL_DOWNLOAD = 208 /* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 35d0c1c..8a8fb0c 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1409,6 +1409,18 @@ struct remote_domain_open_console_args { remote_string devname; u_int flags; }; +struct remote_storage_vol_upload_args { + remote_nonnull_storage_vol vol; + uint64_t offset; + uint64_t length; + u_int flags; +}; +struct remote_storage_vol_download_args { + remote_nonnull_storage_vol vol; + uint64_t offset; + uint64_t length; + u_int flags; +}; struct remote_message_header { u_int prog; u_int vers; -- 1.7.4
On 03/18/2011 10:36 AM, Daniel P. Berrange wrote:
* daemon/remote.c, src/remote/remote_driver.c: Implementation of storage vol upload/download APIs * src/remote/remote_protocol.x: Wire protocol definition for upload/download * daemon/remote_dispatch_args.h, daemon/remote_dispatch_prototypes.h, daemon/remote_dispatch_table.h, src/remote/remote_protocol.h, src/remote/remote_protocol.c: Re-generate +++ b/src/remote/remote_driver.c @@ -9563,6 +9563,91 @@ done: return rv; }
+static int +remoteStorageVolUpload(virStorageVolPtr vol, + virStreamPtr st, + unsigned long long offset, + unsigned long long length, + unsigned int flags) +{ + struct private_data *priv = vol->conn->privateData; + struct private_stream_data *privst = NULL; + int rv = -1; + remote_storage_vol_upload_args args; + + remoteDriverLock(priv); + + if (!(privst = remoteStreamOpen(st, 1,
This looks a bit like a magic number, especially since the output parameter of remoteStreamOpen is documented as ATTRIBUTE_UNUSED. Overall, this patch looks pretty mechanical on top of the earlier patches. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The daemon loops over the linked list of streams when a client quits, closing any that the client hadn't already closed. Except it didn't ever move to the next element in the list! * daemon/stream.c: Fix loop over linked list of streams --- daemon/stream.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/daemon/stream.c b/daemon/stream.c index 967aea2..b94e3df 100644 --- a/daemon/stream.c +++ b/daemon/stream.c @@ -343,6 +343,7 @@ remoteRemoveClientStream(struct qemud_client *client, filter->next = filter->next->next; break; } + filter = filter->next; } } -- 1.7.4
On 03/18/2011 10:36 AM, Daniel P. Berrange wrote:
The daemon loops over the linked list of streams when a client quits, closing any that the client hadn't already closed. Except it didn't ever move to the next element in the list!
* daemon/stream.c: Fix loop over linked list of streams --- daemon/stream.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/daemon/stream.c b/daemon/stream.c index 967aea2..b94e3df 100644 --- a/daemon/stream.c +++ b/daemon/stream.c @@ -343,6 +343,7 @@ remoteRemoveClientStream(struct qemud_client *client, filter->next = filter->next->next; break; } + filter = filter->next;
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 03/18/2011 10:36 AM, Daniel P. Berrange wrote:
An update of
http://www.redhat.com/archives/libvir-list/2011-March/msg00655.html
Addressing all the feedback from v1 series. The most notable change is that 'iohelper' now takes an explicit flags+mode parameter, instead of the operation name.
Except that I didn't see iohelper anywhere in this v2 series. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Mar 21, 2011 at 03:39:28PM -0600, Eric Blake wrote:
On 03/18/2011 10:36 AM, Daniel P. Berrange wrote:
An update of
http://www.redhat.com/archives/libvir-list/2011-March/msg00655.html
Addressing all the feedback from v1 series. The most notable change is that 'iohelper' now takes an explicit flags+mode parameter, instead of the operation name.
Except that I didn't see iohelper anywhere in this v2 series.
Urgh, the perils of picking a subset of patches out of a large merged series. I've sent iohelper now - it should be been the first patch instead of the unrelated first patch Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
The O_NONBLOCK flag doesn't work as desired on plain files or block devices. Introduce an I/O helper program that does the blocking I/O operations, communicating over a pipe that can support O_NONBLOCK * src/fdstream.c, src/fdstream.h: Add non-blocking I/O on plain files/block devices * src/Makefile.am, src/util/iohelper.c: I/O helper program * src/qemu/qemu_driver.c, src/lxc/lxc_driver.c, src/uml/uml_driver.c, src/xen/xen_driver.c: Update for streams API change --- po/POTFILES.in | 1 + src/Makefile.am | 12 +++ src/fdstream.c | 222 ++++++++++++++++++++++++++++++++++++------------ src/fdstream.h | 5 + src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/uml/uml_driver.c | 2 +- src/util/iohelper.c | 208 +++++++++++++++++++++++++++++++++++++++++++++ src/xen/xen_driver.c | 2 +- 9 files changed, 396 insertions(+), 60 deletions(-) create mode 100644 src/util/iohelper.c diff --git a/po/POTFILES.in b/po/POTFILES.in index 805e5ca..12adb3e 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -94,6 +94,7 @@ src/util/event_poll.c src/util/hash.c src/util/hooks.c src/util/hostusb.c +src/util/iohelper.c src/util/interface.c src/util/iptables.c src/util/json.c diff --git a/src/Makefile.am b/src/Makefile.am index c3729a6..1d8115b 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -380,6 +380,9 @@ STORAGE_DRIVER_DISK_SOURCES = \ STORAGE_HELPER_DISK_SOURCES = \ storage/parthelper.c +UTIL_IO_HELPER_SOURCES = \ + util/iohelper.c + # Network filters NWFILTER_DRIVER_SOURCES = \ nwfilter/nwfilter_driver.h nwfilter/nwfilter_driver.c \ @@ -1203,6 +1206,15 @@ EXTRA_DIST += $(LIBVIRT_QEMU_SYMBOL_FILE) libexec_PROGRAMS = +libexec_PROGRAMS += libvirt_iohelper +libvirt_iohelper_SOURCES = $(UTIL_IO_HELPER_SOURCES) +libvirt_iohelper_LDFLAGS = $(WARN_LDFLAGS) $(AM_LDFLAGS) +libvirt_iohelper_LDADD = \ + libvirt_util.la \ + ../gnulib/lib/libgnu.la + +libvirt_iohelper_CFLAGS = $(AM_CFLAGS) + if WITH_STORAGE_DISK if WITH_LIBVIRTD libexec_PROGRAMS += libvirt_parthelper diff --git a/src/fdstream.c b/src/fdstream.c index 701fafc..6d1ad95 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -34,10 +34,12 @@ #include "fdstream.h" #include "virterror_internal.h" #include "datatypes.h" +#include "logging.h" #include "memory.h" #include "event.h" #include "util.h" #include "files.h" +#include "configmake.h" #define VIR_FROM_THIS VIR_FROM_STREAMS #define streamsReportError(code, ...) \ @@ -47,6 +49,10 @@ /* Tunnelled migration stream support */ struct virFDStreamData { int fd; + int errfd; + virCommandPtr cmd; + unsigned long long offset; + unsigned long long length; int watch; unsigned int cbRemoved; @@ -206,6 +212,28 @@ static int virFDStreamFree(struct virFDStreamData *fdst) { int ret; ret = VIR_CLOSE(fdst->fd); + if (fdst->cmd) { + ssize_t len = 1024; + char buf[len]; + int status; + if ((len = saferead(fdst->errfd, buf, sizeof(buf)-1)) < 0) + buf[0] = '\0'; + else + buf[len] = '\0'; + + if (virCommandWait(fdst->cmd, &status) < 0) { + ret = -1; + } else if (status != 0) { + if (buf[0] == '\0') + streamsReportError(VIR_ERR_INTERNAL_ERROR, + _("I/O helper exited with status %d"), status); + else + streamsReportError(VIR_ERR_INTERNAL_ERROR, "%s", + buf); + ret = -1; + } + virCommandFree(fdst->cmd); + } VIR_FREE(fdst); return ret; } @@ -217,6 +245,8 @@ virFDStreamClose(virStreamPtr st) struct virFDStreamData *fdst = st->privateData; int ret; + VIR_DEBUG("st=%p", st); + if (!fdst) return 0; @@ -250,6 +280,18 @@ static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) virMutexLock(&fdst->lock); + if (fdst->length) { + if (fdst->length == fdst->offset) { + virReportSystemError(ENOSPC, "%s", + _("cannot write to stream")); + virMutexUnlock(&fdst->lock); + return -1; + } + + if ((fdst->length - fdst->offset) < nbytes) + nbytes = fdst->length - fdst->offset; + } + retry: ret = write(fdst->fd, bytes, nbytes); if (ret < 0) { @@ -262,6 +304,8 @@ retry: virReportSystemError(errno, "%s", _("cannot write to stream")); } + } else if (fdst->length) { + fdst->offset += ret; } virMutexUnlock(&fdst->lock); @@ -288,6 +332,16 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) virMutexLock(&fdst->lock); + if (fdst->length) { + if (fdst->length == fdst->offset) { + virMutexUnlock(&fdst->lock); + return 0; + } + + if ((fdst->length - fdst->offset) < nbytes) + nbytes = fdst->length - fdst->offset; + } + retry: ret = read(fdst->fd, bytes, nbytes); if (ret < 0) { @@ -300,6 +354,8 @@ retry: virReportSystemError(errno, "%s", _("cannot read from stream")); } + } else if (fdst->length) { + fdst->offset += ret; } virMutexUnlock(&fdst->lock); @@ -317,11 +373,17 @@ static virStreamDriver virFDStreamDrv = { .streamRemoveCallback = virFDStreamRemoveCallback }; -int virFDStreamOpen(virStreamPtr st, - int fd) +static int virFDStreamOpenInternal(virStreamPtr st, + int fd, + virCommandPtr cmd, + int errfd, + unsigned long long length) { struct virFDStreamData *fdst; + VIR_DEBUG("st=%p fd=%d cmd=%p errfd=%d length=%llu", + st, fd, cmd, errfd, length); + if ((st->flags & VIR_STREAM_NONBLOCK) && virSetNonBlock(fd) < 0) return -1; @@ -332,6 +394,9 @@ int virFDStreamOpen(virStreamPtr st, } fdst->fd = fd; + fdst->cmd = cmd; + fdst->errfd = errfd; + fdst->length = length; if (virMutexInit(&fdst->lock) < 0) { VIR_FREE(fdst); streamsReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -346,6 +411,13 @@ int virFDStreamOpen(virStreamPtr st, } +int virFDStreamOpen(virStreamPtr st, + int fd) +{ + return virFDStreamOpenInternal(st, fd, NULL, -1, 0); +} + + #if HAVE_SYS_UN_H int virFDStreamConnectUNIX(virStreamPtr st, const char *path, @@ -387,7 +459,7 @@ int virFDStreamConnectUNIX(virStreamPtr st, goto error; } while ((++i <= timeout*5) && (usleep(.2 * 1000000) <= 0)); - if (virFDStreamOpen(st, fd) < 0) + if (virFDStreamOpenInternal(st, fd, NULL, -1, 0) < 0) goto error; return 0; @@ -406,19 +478,28 @@ int virFDStreamConnectUNIX(virStreamPtr st ATTRIBUTE_UNUSED, } #endif -int virFDStreamOpenFile(virStreamPtr st, - const char *path, - int flags) +static int +virFDStreamOpenFileInternal(virStreamPtr st, + const char *path, + unsigned long long offset, + unsigned long long length, + int flags, + int mode) { - int fd; + int fd = -1; + int fds[2] = { -1, -1 }; struct stat sb; + virCommandPtr cmd = NULL; + int errfd = -1; - if (flags & O_CREAT) { - streamsReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unexpected O_CREAT flag when opening existing file")); - } + VIR_DEBUG("st=%p path=%s flags=%d offset=%llu length=%llu mode=%d", + st, path, flags, offset, length, mode); - if ((fd = open(path, flags)) < 0) { + if (flags & O_CREAT) + fd = open(path, flags, mode); + else + fd = open(path, flags); + if (fd < 0) { virReportSystemError(errno, _("Unable to open stream for '%s'"), path); @@ -440,64 +521,93 @@ int virFDStreamOpenFile(virStreamPtr st, if ((st->flags & VIR_STREAM_NONBLOCK) && (!S_ISCHR(sb.st_mode) && !S_ISFIFO(sb.st_mode))) { - streamsReportError(VIR_ERR_INTERNAL_ERROR, - _("Non-blocking I/O is not supported on %s"), - path); - goto error; - } + int childfd; - if (virFDStreamOpen(st, fd) < 0) - goto error; + if ((flags & O_RDWR) == O_RDWR) { + streamsReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: Cannot request read and write flags together"), + path); + goto error; + } - return 0; + VIR_FORCE_CLOSE(fd); + if (pipe(fds) < 0) { + virReportSystemError(errno, "%s", + _("Unable to create pipe")); + goto error; + } -error: - VIR_FORCE_CLOSE(fd); - return -1; -} + cmd = virCommandNewArgList(LIBEXECDIR "/libvirt_iohelper", + path, + NULL); + virCommandAddArgFormat(cmd, "%d", flags); + virCommandAddArgFormat(cmd, "%d", mode); + virCommandAddArgFormat(cmd, "%llu", offset); + virCommandAddArgFormat(cmd, "%llu", length); -int virFDStreamCreateFile(virStreamPtr st, - const char *path, - int flags, - mode_t mode) -{ - int fd = open(path, flags, mode); - struct stat sb; + if (!cmd) + goto error; - if (fd < 0) { - virReportSystemError(errno, - _("Unable to open stream for '%s'"), - path); - return -1; - } + //virCommandDaemonize(cmd); + if (flags == O_RDONLY) { + childfd = fds[1]; + fd = fds[0]; + virCommandSetOutputFD(cmd, &childfd); + } else { + childfd = fds[0]; + fd = fds[1]; + virCommandSetInputFD(cmd, childfd); + } + virCommandSetErrorFD(cmd, &errfd); - if (fstat(fd, &sb) < 0) { - virReportSystemError(errno, - _("Unable to access stream for '%s'"), - path); - goto error; - } + if (virCommandRunAsync(cmd, NULL) < 0) + goto error; - /* Thanks to the POSIX i/o model, we can't reliably get - * non-blocking I/O on block devs/regular files. To - * support those we need to fork a helper process todo - * the I/O so we just have a fifo. Or use AIO :-( - */ - if ((st->flags & VIR_STREAM_NONBLOCK) && - (!S_ISCHR(sb.st_mode) && - !S_ISFIFO(sb.st_mode))) { - streamsReportError(VIR_ERR_INTERNAL_ERROR, - _("Non-blocking I/O is not supported on %s"), - path); - goto error; + VIR_FORCE_CLOSE(childfd); + } else { + if (offset && + lseek(fd, offset, SEEK_SET) != offset) { + virReportSystemError(errno, + _("Unable to seek %s to %llu"), + path, offset); + goto error; + } } - if (virFDStreamOpen(st, fd) < 0) + if (virFDStreamOpenInternal(st, fd, cmd, errfd, length) < 0) goto error; return 0; error: + virCommandFree(cmd); + VIR_FORCE_CLOSE(fds[0]); + VIR_FORCE_CLOSE(fds[1]); VIR_FORCE_CLOSE(fd); return -1; } + +int virFDStreamOpenFile(virStreamPtr st, + const char *path, + unsigned long long offset, + unsigned long long length, + int flags) +{ + if (flags & O_CREAT) { + streamsReportError(VIR_ERR_INTERNAL_ERROR, + _("Attempt to create %s without specifying mode"), + path); + return -1; + } + return virFDStreamOpenFileInternal(st, path, offset, length, flags, 0); +} + +int virFDStreamCreateFile(virStreamPtr st, + const char *path, + unsigned long long offset, + unsigned long long length, + int flags, + mode_t mode) +{ + return virFDStreamOpenFileInternal(st, path, offset, length, flags, mode); +} diff --git a/src/fdstream.h b/src/fdstream.h index 53cbaa7..6b395b6 100644 --- a/src/fdstream.h +++ b/src/fdstream.h @@ -24,6 +24,7 @@ # define __VIR_FDSTREAM_H_ # include "internal.h" +# include "command.h" int virFDStreamOpen(virStreamPtr st, int fd); @@ -34,9 +35,13 @@ int virFDStreamConnectUNIX(virStreamPtr st, int virFDStreamOpenFile(virStreamPtr st, const char *path, + unsigned long long offset, + unsigned long long length, int flags); int virFDStreamCreateFile(virStreamPtr st, const char *path, + unsigned long long offset, + unsigned long long length, int flags, mode_t mode); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 60d4204..815c5f6 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2776,7 +2776,7 @@ lxcDomainOpenConsole(virDomainPtr dom, goto cleanup; } - if (virFDStreamOpenFile(st, chr->source.data.file.path, O_RDWR) < 0) + if (virFDStreamOpenFile(st, chr->source.data.file.path, 0, 0, O_RDWR) < 0) goto cleanup; ret = 0; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c7d4262..2cb36b3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7015,7 +7015,7 @@ qemuDomainOpenConsole(virDomainPtr dom, goto cleanup; } - if (virFDStreamOpenFile(st, chr->source.data.file.path, O_RDWR) < 0) + if (virFDStreamOpenFile(st, chr->source.data.file.path, 0, 0, O_RDWR) < 0) goto cleanup; ret = 0; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 538d5f7..d364597 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -2126,7 +2126,7 @@ umlDomainOpenConsole(virDomainPtr dom, goto cleanup; } - if (virFDStreamOpenFile(st, chr->source.data.file.path, O_RDWR) < 0) + if (virFDStreamOpenFile(st, chr->source.data.file.path, 0, 0, O_RDWR) < 0) goto cleanup; ret = 0; diff --git a/src/util/iohelper.c b/src/util/iohelper.c new file mode 100644 index 0000000..07aef34 --- /dev/null +++ b/src/util/iohelper.c @@ -0,0 +1,208 @@ +/* + * iohelper.c: Helper program to perform I/O operations on files + * + * Copyright (C) 2011 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel P. Berrange <berrange@redhat.com> + * + * Current support + * - Read existing file + * - Write existing file + * - Create & write new file + */ + +#include <config.h> + +#include <locale.h> +#include <unistd.h> +#include <fcntl.h> +#include <stdio.h> +#include <stdlib.h> + +#include "util.h" +#include "threads.h" +#include "files.h" +#include "memory.h" +#include "virterror_internal.h" +#include "configmake.h" + +#define VIR_FROM_THIS VIR_FROM_STORAGE + +static int runIO(const char *path, + int flags, + int mode, + unsigned long long offset, + unsigned long long length) +{ + char *buf = NULL; + size_t buflen = 1024*1024; + int fd; + int ret = -1; + int fdin, fdout; + const char *fdinname, *fdoutname; + + if (flags & O_CREAT) { + fd = open(path, flags, mode); + } else { + fd = open(path, flags); + } + if (fd < 0) { + virReportSystemError(errno, _("Unable to open %s"), path); + goto cleanup; + } + + if (offset) { + if (lseek(fd, offset, SEEK_SET) < 0) { + virReportSystemError(errno, _("Unable to seek %s to %llu"), + path, offset); + goto cleanup; + } + } + + if (VIR_ALLOC_N(buf, buflen) < 0) { + virReportOOMError(); + goto cleanup; + } + + switch (flags & O_ACCMODE) { + case O_RDONLY: + fdin = fd; + fdinname = path; + fdout = STDOUT_FILENO; + fdoutname = "stdout"; + break; + case O_WRONLY: + fdin = STDIN_FILENO; + fdinname = "stdin"; + fdout = fd; + fdoutname = path; + break; + + case O_RDWR: + default: + virReportSystemError(EINVAL, + _("Unable to process file with flags %d"), + (flags & O_ACCMODE)); + goto cleanup; + } + + offset = 0; + while (1) { + ssize_t got; + + if (length && + (length - offset) < buflen) + buflen = length - offset; + + if (buflen == 0) + break; + + if ((got = saferead(fdin, buf, buflen)) < 0) { + virReportSystemError(errno, _("Unable to read %s"), fdinname); + goto cleanup; + } + if (got == 0) + break; + offset += got; + if (safewrite(fdout, buf, got) < 0) { + virReportSystemError(errno, _("Unable to write %s"), fdoutname); + goto cleanup; + } + } + + ret = 0; + +cleanup: + if (VIR_CLOSE(fd) < 0 && + ret == 0) { + virReportSystemError(errno, _("Unable to close %s"), path); + ret = -1; + } + + VIR_FREE(buf); + return ret; +} + +int main(int argc, char **argv) +{ + const char *path; + const char *op; + virErrorPtr err; + unsigned long long offset; + unsigned long long length; + int flags; + int mode; + + if (setlocale(LC_ALL, "") == NULL || + bindtextdomain(PACKAGE, LOCALEDIR) == NULL || + textdomain(PACKAGE) == NULL) { + fprintf(stderr, _("%s: initialization failed\n"), argv[0]); + exit(EXIT_FAILURE); + } + + if (virThreadInitialize() < 0 || + virErrorInitialize() < 0 || + virRandomInitialize(time(NULL) ^ getpid())) { + fprintf(stderr, _("%s: initialization failed\n"), argv[0]); + exit(EXIT_FAILURE); + } + + if (argc != 6) { + fprintf(stderr, _("%s: syntax FILENAME FLAGS MODE OFFSET LENGTH\n"), argv[0]); + exit(EXIT_FAILURE); + } + + path = argv[1]; + op = argv[2]; + + if (virStrToLong_i(argv[2], NULL, 10, &flags) < 0) { + fprintf(stderr, _("%s: malformed file flags %s"), argv[0], argv[2]); + exit(EXIT_FAILURE); + } + + if (virStrToLong_i(argv[3], NULL, 10, &mode) < 0) { + fprintf(stderr, _("%s: malformed file mode %s"), argv[0], argv[3]); + exit(EXIT_FAILURE); + } + + if (virStrToLong_ull(argv[4], NULL, 10, &offset) < 0) { + fprintf(stderr, _("%s: malformed file offset %s"), argv[0], argv[4]); + exit(EXIT_FAILURE); + } + if (virStrToLong_ull(argv[5], NULL, 10, &length) < 0) { + fprintf(stderr, _("%s: malformed file length %s"), argv[0], argv[5]); + exit(EXIT_FAILURE); + } + + if ((flags & O_RDWR) == O_RDWR) { + exit(EXIT_FAILURE); + } + + if (runIO(path, flags, mode, offset, length) < 0) + goto error; + + return 0; + +error: + err = virGetLastError(); + if (err) { + fprintf(stderr, "%s: %s\n", argv[0], err->message); + } else { + fprintf(stderr, _("%s: unknown failure with %s\n"), argv[0], path); + } + exit(EXIT_FAILURE); +} diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 1162f63..de304c1 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -2019,7 +2019,7 @@ xenUnifiedDomainOpenConsole(virDomainPtr dom, goto cleanup; } - if (virFDStreamOpenFile(st, chr->source.data.file.path, O_RDWR) < 0) + if (virFDStreamOpenFile(st, chr->source.data.file.path, 0, 0, O_RDWR) < 0) goto cleanup; ret = 0; -- 1.7.4
On 03/22/2011 05:29 AM, Daniel P. Berrange wrote:
The O_NONBLOCK flag doesn't work as desired on plain files or block devices. Introduce an I/O helper program that does the blocking I/O operations, communicating over a pipe that can support O_NONBLOCK
* src/fdstream.c, src/fdstream.h: Add non-blocking I/O on plain files/block devices * src/Makefile.am, src/util/iohelper.c: I/O helper program * src/qemu/qemu_driver.c, src/lxc/lxc_driver.c, src/uml/uml_driver.c, src/xen/xen_driver.c: Update for streams API change --- po/POTFILES.in | 1 + src/Makefile.am | 12 +++ src/fdstream.c | 222 ++++++++++++++++++++++++++++++++++++------------ src/fdstream.h | 5 + src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/uml/uml_driver.c | 2 +- src/util/iohelper.c | 208 +++++++++++++++++++++++++++++++++++++++++++++ src/xen/xen_driver.c | 2 +- 9 files changed, 396 insertions(+), 60 deletions(-) create mode 100644 src/util/iohelper.c
@@ -206,6 +212,28 @@ static int virFDStreamFree(struct virFDStreamData *fdst) { int ret; ret = VIR_CLOSE(fdst->fd); + if (fdst->cmd) { + ssize_t len = 1024; + char buf[len];
This is a variable-sized buffer, which is not required by C89 (it was added in C99)...
+ int status; + if ((len = saferead(fdst->errfd, buf, sizeof(buf)-1)) < 0)
so does sizeof(buf) give the right result on all compilers? Or should you change to char buf[1024]?
+ } else if (status != 0) { + if (buf[0] == '\0') + streamsReportError(VIR_ERR_INTERNAL_ERROR, + _("I/O helper exited with status %d"), status);
status includes normal exit and signals. This should probably use WIFEXITED and WEXITSTATUS to avoid printing values shifted by 8. For that matter, I just noticed that virCommandWait should probably be more careful in how it interprets status.
+ cmd = virCommandNewArgList(LIBEXECDIR "/libvirt_iohelper", + path, + NULL); + virCommandAddArgFormat(cmd, "%d", flags); + virCommandAddArgFormat(cmd, "%d", mode); + virCommandAddArgFormat(cmd, "%llu", offset); + virCommandAddArgFormat(cmd, "%llu", length);
+ if (!cmd) + goto error;
That only catches allocation failure, but not all other failures. Since virCommandRunAsync will also catch the same error, are these two lines redundant?
- if (fd < 0) { - virReportSystemError(errno, - _("Unable to open stream for '%s'"), - path); - return -1; - } + //virCommandDaemonize(cmd);
Any reason to keep this comment? I don't see any reason to daemonize the iohelper.
- if (virFDStreamOpen(st, fd) < 0) + if (virFDStreamOpenInternal(st, fd, cmd, errfd, length) < 0) goto error;
return 0;
error: + virCommandFree(cmd);
If virFDStreamOpenInternal fails, but we've already spawned the child process, virCommandFree won't reap that process.
+ VIR_FORCE_CLOSE(fds[0]); + VIR_FORCE_CLOSE(fds[1]); VIR_FORCE_CLOSE(fd);
Then again, once the fds are closed, the child should eventually die naturally from EOF or SIGPIPE. But it does raise the question of whether this cleanup need any reorganization to reap any child process on error.
+int virFDStreamCreateFile(virStreamPtr st, + const char *path, + unsigned long long offset, + unsigned long long length, + int flags, + mode_t mode) +{ + return virFDStreamOpenFileInternal(st, path, offset, length, flags, mode);
Should this fail if (flags&O_CREAT) == 0?
+++ b/src/util/iohelper.c
+ if (offset) { + if (lseek(fd, offset, SEEK_SET) < 0) { + virReportSystemError(errno, _("Unable to seek %s to %llu"), + path, offset); + goto cleanup; + } + } +
+ + offset = 0;
Did you really intend to zero out the offset here?
+int main(int argc, char **argv) +{ + const char *path; + const char *op;
Dead variable.
+ virErrorPtr err; + unsigned long long offset; + unsigned long long length; + int flags; + int mode; + + if (setlocale(LC_ALL, "") == NULL || + bindtextdomain(PACKAGE, LOCALEDIR) == NULL || + textdomain(PACKAGE) == NULL) { + fprintf(stderr, _("%s: initialization failed\n"), argv[0]); + exit(EXIT_FAILURE); + } + + if (virThreadInitialize() < 0 || + virErrorInitialize() < 0 || + virRandomInitialize(time(NULL) ^ getpid())) { + fprintf(stderr, _("%s: initialization failed\n"), argv[0]); + exit(EXIT_FAILURE); + } + + if (argc != 6) { + fprintf(stderr, _("%s: syntax FILENAME FLAGS MODE OFFSET LENGTH\n"), argv[0]); + exit(EXIT_FAILURE); + } + + path = argv[1]; + op = argv[2];
Dead assignment.
+ + if ((flags & O_RDWR) == O_RDWR) { + exit(EXIT_FAILURE); + }
Won't work. It should be: if ((flags & O_ACCMODE) == O_RDWR) But runIO makes the same check, so you could just omit it from here. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake