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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/