7:32 a.m.
On Fri, Sep 25, 2009 at 12:25:50PM +0100, Daniel P. Berrange wrote:
On Fri, Sep 25, 2009 at 12:09:34PM +0200, Daniel Veillard wrote:
> On Mon, Aug 24, 2009 at 09:51:04PM +0100, Daniel P. Berrange wrote:
> > @@ -1448,6 +1466,81 @@ void virEventRegisterImpl(virEventAddHandleFunc
addHandle,
> > virEventAddTimeoutFunc addTimeout,
> > virEventUpdateTimeoutFunc updateTimeout,
> > virEventRemoveTimeoutFunc removeTimeout);
> > +
> > +enum {
> > + VIR_STREAM_NONBLOCK = (1 << 0),
> > +};
> > +
> > +virStreamPtr virStreamNew(virConnectPtr conn,
> > + unsigned int flags);
>
> Would flags be sufficient if we were to encode some priorities to
> streams? If we end up limiting the number of active streams, giving
> higher priority or some reserved slots for quicker operations may
> be important, flags should be sufficient for this if the need arise
> so I don't see a problem but I raise the point.
I guess it would be sufficient if you wanted todo LOW/MEDIUM/HIGH
static priorties.
yeah, I just wanted to make sure it didn't contradict something you
may have in mind :-)
If we wanted to get more advanced, we could always introduce an
extra API, since there is always a point between virStreamNew()
and then using it, eg in virConnectUploadFile(), when you can
do more setup calls. We already allow event callbacks to be
registered this way, so we could in future add a
virStreamSetPriority(virStreamPtr st, ....options ...);
right,
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 ?
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.
>
> > +int virStreamRef(virStreamPtr st);
> > +
> > +int virStreamSend(virStreamPtr st,
> > + const char *data,
> > + size_t nbytes);
> > +
> > +int virStreamRecv(virStreamPtr st,
> > + char *data,
> > + size_t nbytes);
> > +
> > +
> > +typedef int (*virStreamSourceFunc)(virStreamPtr st,
> > + char *data,
> > + size_t nbytes,
> > + void *opaque);
>
> I think the signature is fine but we need to document all the
> arguments and the return values.
>
> > +int virStreamSendAll(virStreamPtr st,
> > + virStreamSourceFunc handler,
> > + void *opaque);
>
> Hum, I had to look at the comment to really understand. I'm not
> 100% sure, maybe we should allow for handler() to be called multiple
> time, not just once.
> For example I would allow virStreamSendAll() code to call handler
> multiple time, until the handler like a read() returns 0 (or -1 on
> error), in any case the signature should be documented fully.
This method is essentially just a convenient way to call the
virStreamSend() in a loop, for apps that are happy todo blocking
data I/O. As such the handler() will definitely be called multiple
times to fetch the data to be sent. You can see how it was used
later on in this patch, where virStreamSendAll is implemented.
I expect most apps would use virStreamSend() though, to allow
them todo interruptible & non-blocking I/O
Okay, but let's describe all args and return values for the callbacks.
> > +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.
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.
[...]
> > +int virStreamFinish(virStreamPtr st);
> > +int virStreamAbort(virStreamPtr st);
>
> For those 2 maybe add a flag, to allow for example background disconnection.
> Even if the stream wasn't created ASYNC, we may want sometime to
> abruptly end without waiting.
The virStreamAbort() operation is always asynchronous, regardless of
whether the stream is non-blocking. The virStreamFinish() operation
has to be synchronous, because for it to be useful you need to do
a round-trip to flush any error being sent back from the server. So
I don't think we need any flags here. If you want to close it abruptly
then virStreamAbort() is suitable, if you want to close it cleanly
then virStreamFinish() is suitable.
hum, okay ...
[...]
> > + * virConnectUploadFile(conn, st);
> > + * virStreamSendAll(st, mysource, &fd);
> > + * virStreamFree(st);
> > + * close(fd);
>
>
> Hum, the clasic example of blocking I/Os is if people use multithreaded
> apps.
> What is a second thread calls calls virStreamAbort() while
> virStreamSendAll() is in progress, e.g. the user clicked on an abort
> button from the UI ?
> Same question for virStreamRecvAll() ?
If you want to abort a stream part way through, then you should not
be using virStreamSendAll/RecvAll. These APIs are optional convenience
APIs for apps which are happy todo a blocking operation. Apps which
need ability to abort part way through can use the virStreamRecv()
and virStreamSend() APIs instead.
Callback programming is hard, people get very easilly confused by them
and handling of errors can turn really nasty if you use them a lot. So
I think it's important to have foolproof synchronous transfers, even if
you think they may be a bit abused, it's really easier on the
programmer.
That all said, it is safe for another thread to call
virStreamAbort(),
it will cause this Send/RecvAll method to return an error on the next
virStreamSend/Recv call it makes
good :-)
>
> > + * Returns 0 if all the data was succesfully sent. The stream
> > + * will be marked as finished on success, so the caller need
> > + * only call virStreamFree().
> > + *
> > + * Returns -1 upon any error, with the stream being marked as
> > + * aborted, so the caller need only call virStreamFree()
> > + */
> > +int virStreamSendAll(virStreamPtr stream,
> > + virStreamSourceFunc handler,
> > + void *opaque)
>
>
> > + * 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.
I still think the callbacks behaviour should be clarified especially
behaviour and reporting in case of error.
thanks,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/