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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list