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

[Daniel P. Berrange]

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 :|