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

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.

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

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