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)