8:09 a.m.
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 :|