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" <[email protected]>
> To: "Saggi Mizrahi" <[email protected]>, "Adam Litke" <[email protected]>
> Cc: "Vinzenz Feenstra" <[email protected]>, [email protected]
> Sent: Tuesday, January 15, 2013 8:40:17 AM
> Subject: Re: Refactor communication infra
>
> On Mon, Jan 07, 2013 at 03:10:38PM -0500, [email protected] 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?
>
>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
