[libvirt] [PATCH 01/11] Add public API definition for data stream handling

[Daniel P. Berrange]

On Fri, Sep 25, 2009 at 02:32:39PM +0200, Daniel Veillard wrote:
> On Fri, Sep 25, 2009 at 12:25:50PM +0100, Daniel P. Berrange wrote:
> 
> > if we needed more than just plain flags.
> > 
> > 
> > On the subject of flags though, I've never been entirely sure
> > about whether it would be worth mandating the use of a flag(s)
> > for indicating I/O direction at time of stream creation. 
> > Currently this is implicit base on the API that the stream is
> > later used with,
> > 
> > eg, currently
> > 
> >      virStreamPtr st = virStreamNew(conn, 0);
> > 
> >      virConnectUploadFile(conn, st, filename);
> > 
> > implicitly configures the stream for writing, but I constantly
> > wonder whether we ought to make it explicit via a flag like
> > 
> >     virStreamPtr st = virStreamNew(conn, VIR_STREAM_WRITE);
> > 
> >     virConnectUploadFile(conn, st, filename);
> > 
> > and require that the call of virStreamNew() provide either
> > VIR_STREAM_WRITE, or VIR_STREAM_READ, or both. ANd then also
> > have the methods using streams like virConnectUploadFile
> > check that the flags match.
> 
>   Hum, this would then raise the signal that stream can be used
> both ways, do we really want to suggest this at the API level,
> I can see how we're gonna use this internally, but aren't we opening
> the door to much complexity ?

Yeah, that's more or less why I left it out so far - I've not
yet found a case where I absolutely needed the WRITE/READ
flags to be set explicitly by apps.

> 
> > If we wanted to mandate use of READ/WRITE flags for stream
> > creation, we'd obviously need todo it from teh start, since
> > we couldn't add that as a mandatory flag once the API is
> > released & in use by apps.
> 
>   yes that's a good point, a design issue too. If you really expect API
> usage for both read and write, then I would say we should make those
> flags mandatory. The only point is that our existing flags use in APIs
> are just fine with 0 as being the 'default', and we would break this
> but it's not a big deal IMHO, that will be caught immediately.

There is one likely API  where we'd have a full read+write stream.
I've thought about adding ability to tunnel a serial port PTY 
over libvirt, so 'virsh console' could be made to work remotely.
eg, something like

    virDomainOpenConsole(virDomainPtr dom, virStreamPtr stream
                         const char *consolename);

In this case you'd be reading & writing from /to the same
stream. It still wouldn't really require that we set the
READ+WRITE flags when doing virStreamNew()

> > > > +typedef int (*virStreamSinkFunc)(virStreamPtr st,
> > > > +                                 const char *data,
> > > > +                                 size_t nbytes,
> > > > +                                 void *opaque);
> > > 
> > >   Same thing do we allow a sink function to be called repeatedly ?
> > > If we want to allow this in some ways we will need an extra argument
> > > to indicate the end of the stream. Even if we don't plan this yet, I
> > > would suggest to add a flags to allow for this possibility in the
> > > future. With a chunk size of 256K at the protocol level it may not
> > > be a good idea to keep the full data in memory, so I would allow
> > > for this interface to call the sink multiple times. And IMHO it's best
> > > to pass the indication of end of transfer directly at the sink level
> > > rather than wait for the virStreamFree() coming from the user.
> > > 
> > > > +int virStreamRecvAll(virStreamPtr st,
> > > > +                     virStreamSinkFunc handler,
> > > > +                     void *opaque);
> > >
> > >   Okay
> > 
> > Same as for SendAll, this API will invoke the handler multilpe
> > times to write out data that is being received. In both cases
> > the implementation is invoking the handler with 64kb buffers
> > to avoid pulling lots of data into memory.
> 
>   Okay, but I think being able to indicate there that a packet is the
> last one may be important, for example if the application design prefer
> to initiate the closure of the transfer (close/sync/...) as soon as
> possible.

Actually in the case of the 'source' function, the app already
knows when its got to the end, because its source funtion will
be returning '0' for end-of-file.

For the 'sink' function we'd have to make sure we called it once
at the end with a length of '0' to indicate EOF in that direction.
I can't remember offhand if we'll do that already or not.

>   Also how flexible are we in the design with callbacks taling a long
> time to complete, for example reads crossing the network, or slow
> output devices ? Maybe this should be hinted in the callback
> descriptions.

These callbacks are the app's responsibility & execute in its 
context, so libvirt doesn't care whether they are slow or fast
to execute. Everything internal to libvirt relating to streams
is non-blocking & fast.

> > > > + *   virConnectUploadFile(conn, st);
> > > > + *   virStreamSendAll(st, mysource, &fd);
> > > > + *   virStreamFree(st);
> > > > + *   close(fd);
> > > 


> > > > + *   virConnectUploadFile(conn, st);
> > > > + *   virStreamRecvAll(st, mysink, &fd);
> > > > + *   virStreamFree(st);
> > > > + *   close(fd);
> > > 
> > >  Would the current API allow for the sink callback to close the fd()
> > > at the end of the transfer ? Right now, I don't think so because we
> > > don't know what is the last callback (assuming multiple ones).
> > 
> > The sink/source callbacks do not need to close the FD since this is easily
> > done with RecvAll/SendAll returns control to the application. THis is
> > in fact important, because it is not until RecvAll/SendAll returns that
> > you can call virStreamFinish to check for success. If it did not suceed
> > then you may want do other cleanup before closing the FD, such as 
> > unlinking the file
> 
>   Hum, the two example for RecvAll and SendAll don't suggest
> virStreamFinish() to be called to get the status, I would expect error
> reporting to show up as the result code from RecvAll and SendAll.

Actually these 2 code examples are wrong. There should be a call
to virStreamFinish in there, before the virStreamFree. This was
not required in an earlier version of my patch, because SendAll
would call Finish for you, but I realized this made it impossible
for callers to detect certain error conditions. So the app should
always call either Finish or Abort once they're done with I/O.
I'll update the example

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|