<div dir="ltr"><div>Hi Martin,</div><div><br></div><div>Nice start on the README. I took a moment to make the python part a bit more "pythonic", and pushed it to a branch on github:</div><div><a href="https://github.com/sbjorn/strongswan/commit/f6672dcfdc7eaf39ae8952c9c328068e1d7ca3ec">https://github.com/sbjorn/strongswan/commit/f6672dcfdc7eaf39ae8952c9c328068e1d7ca3ec</a><br></div><div><a href="https://github.com/sbjorn/strongswan/commit/7b78cef93dd9c0b37401b9d359c634050a8e9ef0">https://github.com/sbjorn/strongswan/commit/7b78cef93dd9c0b37401b9d359c634050a8e9ef0</a><br></div><div><br></div><div>I've taken a look at your code changes. I definitely agree that implicit error checking is nicer, and it will make the experience of using the library much smoother. There is a problem with your implementation of [2] however, you are discarding any errors for commands that require event streaming -- only the events are returned -- there is no way to know whether the command was successful.</div><div><br></div>While generators is a good idea for response streaming, it is not a great fit here. The current implementation works on the assumption that the generator is exhausted before another command starts. There is no way to guarantee this, as the user may draw elements from the generator as he/she wishes. This will make the consecutive command fail in best case, and in the worst case, return the wrong result. Another problem is that no errors are reported due to [2].<div><br></div><div>Here is an example:</div><blockquote style="margin:0px 0px 0px 40px;border:none;padding:0px"><div><div><font face="monospace, monospace">>>> import vici</font></div></div><div><div><font face="monospace, monospace">>>> s = vici.Session()</font></div></div><div><div><font face="monospace, monospace"><br></font></div></div><div><div><font face="monospace, monospace">>>> g = s.list_certs()</font></div></div><div><div><font face="monospace, monospace">>>> msg = next(g)</font></div></div><div><div><font face="monospace, monospace">>>> print s.version()</font></div></div><div><font face="monospace, monospace"><br></font></div><div><div><font face="monospace, monospace">Traceback (most recent call last):</font></div></div><div><div><font face="monospace, monospace">  File "scr.py", line 6, in <module></font></div></div><div><div><font face="monospace, monospace">    print(s.version())</font></div></div><div><div><font face="monospace, monospace">  File "vici/session.py", line 21, in version</font></div></div><div><div><font face="monospace, monospace">    return self.handler.request("version")</font></div></div><div><div><font face="monospace, monospace">  File "vici/session.py", line 238, in request</font></div></div><div><div><font face="monospace, monospace">    response=Packet.CMD_RESPONSE</font></div></div><div><div><font face="monospace, monospace">vici.exception.SessionException: Unexpected response type 7, expected '1' (CMD_RESPONSE)</font></div></div></blockquote><div><div><div><br></div><div><div>For this to be implemented properly, the way the library currently send and receive messages must be rethought.</div></div></div></div><div><br></div><div><br></div><div>Kind regards,</div><div>Björn </div></div><div class="gmail_extra"><br><div class="gmail_quote">2015-03-02 15:45 GMT+01:00 Martin Willi <span dir="ltr"><<a href="mailto:martin@strongswan.org" target="_blank">martin@strongswan.org</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Björn,<br>
<span class=""><br>
> Your README.rst is not yet part of it, probably we should integrate<br>
</span>> that to the vici README.md.<br>
<br>
I've updated our vici README.md [1] to include some basic steps to get<br>
started with the python library.<br>
<br>
In addition, I'd like to propose a minor change to the main Session<br>
command calls. Instead of returning a CommandResult, I'd prefer to raise<br>
a CommandException if a command failed [2].<br>
<br>
While I think implicit error checking with exceptions makes sense, the<br>
main reason for that change is that it allows us to return generators<br>
for streamed objects [3]. I think that is favorable, especially as some<br>
longer lasting commands return immediate feedback when called<br>
interactively. For example, the initiate() command directly returns<br>
streamed log messages, long before the command finally succeeds or<br>
fails.<br>
<br>
Please let me know what you think about these changes.<br>
<br>
Best regards<br>
Martin<br>
<br>
[1]<a href="https://github.com/strongswan/strongswan/blob/vici-python/src/libcharon/plugins/vici/README.md#vici-python-egg" target="_blank">https://github.com/strongswan/strongswan/blob/vici-python/src/libcharon/plugins/vici/README.md#vici-python-egg</a><br>
[2]<a href="http://git.strongswan.org/?p=strongswan.git;a=commitdiff;h=713d2f42" target="_blank">http://git.strongswan.org/?p=strongswan.git;a=commitdiff;h=713d2f42</a><br>
[3]<a href="http://git.strongswan.org/?p=strongswan.git;a=commitdiff;h=ec599c65" target="_blank">http://git.strongswan.org/?p=strongswan.git;a=commitdiff;h=ec599c65</a><br>
<br>
</blockquote></div><br></div>