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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list