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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list