Re: [libvirt] [PATCH 3/6] Add vol-create-upload, vol-upload and vol-download commands to virsh
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)