Re: [libvirt] [PATCH 04/10] python: Implement virStreamSend/Recv
On 06/16/2011 10:44 AM, Daniel Veillard wrote:
On Wed, Jun 15, 2011 at 09:23:13PM -0400, Cole Robinson wrote:
> The return values for the python version are different that the C version
> of virStreamSend: on success we return a string, an error raises an exception,
> and if the stream would block we return int(-2). We need to do this
> since strings aren't passed by reference in python.
I find this a bit bizarre, either we return a string or we return an
integer, but if we don't have any other way to provide the information
Since we provide the wrapper what about returning
(code#, "string value")
allowing to have only one type on return instead (i.e. just change
recv() in the override below)
Certainly from a C perspective it's bizarre, but with dynamic typing I think
this makes the interface much simpler without much confusion. -2 is just a
special value here, much like "" means EOF. The code essentially looks like:
ret = stream.recv(1024)
if ret == "":
return # EOL
if ret == -2:
return # E_WOULDBLOCK
And only users of NONBLOCK flag need to handle this case. Even if they forget
to do so, their code would likely error quickly if they try to perform any
string operations on the returned value.
The benefit of returning a tuple is that it makes the multiple return values
clear to an API user who doesn't read the docs, however I think it increases
the chance of client bugs and makes client code uglier. Users who aren't using
the NONBLOCK flag will still have to handle to at least acknowledge the return
code which has no meaning in their case. For NONBLOCK users, what do we set
the string value to if the error code is -2?
- string "": However this is a value with special meaning, and if the user
mishandles the error code or outright doesn't check it, handling this value
may cause hard do diagnose behavior
- None: If the error code is mishandled, this would likely fail in the same
way as the original proposal, which is okay. But if the user was simply doing
a check like 'if not stringdata:' for an EOL check, they would incorrectly
match this case as well, which has the same problem as "" (unlike a value of
-2)
Thanks,
Cole
> Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
> ---
> python/generator.py | 9 +++--
> python/libvirt-override-virStream.py | 35 +++++++++++++++++++
> python/libvirt-override.c | 62 ++++++++++++++++++++++++++++++++++
> python/typewrappers.c | 14 ++++++++
> python/typewrappers.h | 1 +
> 5 files changed, 117 insertions(+), 4 deletions(-)
> +
> + def recv(self, nbytes):
> + """Write a series of bytes to the stream. This method may
> + block the calling application for an arbitrary amount
> + of time.
> +
> + Errors are not guaranteed to be reported synchronously
> + with the call, but may instead be delayed until a
> + subsequent call.
> +
> + On success, the received data is returned. On failure, an
> + exception is raised. If the stream is a NONBLOCK stream and
> + the request would block, integer -2 is returned.
> + """
> + ret = libvirtmod.virStreamRecv(self._o, nbytes)
> + if ret == None: raise libvirtError ('virStreamRecv() failed')
> + return ret
otherwise looks fine to me,
Daniel