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

[Daniel Veillard]

On Fri, Sep 25, 2009 at 02:09:54PM +0100, Daniel P. Berrange wrote:
> On Fri, Sep 25, 2009 at 02:32:39PM +0200, Daniel Veillard wrote:
> >   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.

  Okay, let's keep it that way. Ultimately we could add another API
to create the Stream.

> > > 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()

  okay

> > > > > +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.

  yes for read it's not a problem

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

  ah write 0 lenght, yes that's another way to pass the signal.

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

 okidoc, I just wanted to doub;e-check :-)

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

  Ah, okay !

ACK

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/