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