[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 01/11] Add public API definition for data stream handling



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]