Refactor communication infra

Saggi Mizrahi smizrahi at redhat.com
Tue Jan 15 20:58:26 UTC 2013


As I said in my comment on the patch.

This is just so  I don't have to change it to.

try:
    port = cfg["port"]
except KeyEerror:
    raise ValueError("cfg")

And then, have code handling the error upwards in the stack.
It is just there to get things passed with minimal code.

Further more, if it was an http server instead of a json-rpc server
no one would have minded if I had put 80 as the default port even
lower down the stack meaning that having the default set at any level
is not such a taboo.

In any case, since for Dan it is more important to change it now and
have everyone blocked the patch for another iteration is more
important then pushing it in and arguing about it later I'll send
another version today.

----- Original Message -----
> From: "Dan Kenigsberg" <danken at redhat.com>
> To: "Saggi Mizrahi" <smizrahi at redhat.com>, "Adam Litke" <agl at us.ibm.com>
> Cc: "Vinzenz Feenstra" <vfeenstr at redhat.com>, vdsm-patches at fedorahosted.org
> Sent: Tuesday, January 15, 2013 8:40:17 AM
> Subject: Re: Refactor communication infra
> 
> On Mon, Jan 07, 2013 at 03:10:38PM -0500, danken at redhat.com wrote:
> > Dan Kenigsberg has posted comments on this change.
> > 
> > Change subject: Refactor communication infra
> > ......................................................................
> > 
> > 
> > Patch Set 15: (2 inline comments)
> > 
> > ....................................................
> > File vdsm_api/BindingJsonRpc.py
> > Line 37:         self._reactors = reactors
> > Line 38:
> > Line 39:     def _createTcpReactor(self, cfg):
> > Line 40:         address = cfg.get("ip", "0.0.0.0")
> > Line 41:         port = cfg.get("port", 4044)
> > sorry, I do not follow. which error handling code would have been
> > required? to handle the case of not having cfg['ip'] ? having a
> > default here only hide such an error instead of crying out loud
> > about it.
> 
> Saggi, Adam, could you help me here?
> 
> I do not understand the need for the default port here.
> Could you elaborate on the justification?
> 
> 


More information about the vdsm-patches mailing list