[libvirt] [PATCH 3/6] Add vol-create-upload, vol-upload and vol-download commands to virsh

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110321/2d4259a4/attachment-0001.sig>