[Monotone-devel] Re: nvm.ssh-agent review

[Zack Weinberg]

On 2/13/07, Justin Patrin <address@hidden> wrote:
> On 2/11/07, Zack Weinberg <address@hidden> wrote:
> > My biggest concern has to do with the way you are serializing and
> > deserializing packets -- I think you're working way, way too hard.
> > You have a stream interface, and the packet format seems geared to
> > serial reading -- why do you read the entire packet in one go and then
> > chop it up with string manipulations?  On the write side, you do need
> > to construct the packet in memory so you can set its length fields,
> > but I think stringstreams would be a big help.
>
> Well, the problem is that the packet construction is recursive. You
> need the length of the entire key, for example, before inserting it
> into the packet data, then the length of all of that data before
> inserting it into the packet itself. It's possible that stringstreams
> could be better but what is there works now. I'd suggest if we want to
> rewrite this we do it after this branch lands.

Fair enough.  I am tempted to try it, if only to convince myself that
you're right, but I certainly don't have time to do it now, and it
shouldn't hold things up.

> > Consider use of widen<> in e.g. get_long.
>
> Hmmm, widen<> seems to convert a single char to a wider format. How
> would I use it to widen 4 chars to a long?

It's just our preferred way to write (u32)((unsigned char)x).
widen<TO,FROM>(FROM x) is static_cast<TO>(x) with two additional
properties: first, if x is not default-convertible to type FROM, you
get an error, and second, if FROM is signed and TO isn't, it
zero-extends instead of sign-extends.  This is exactly what you want
instead of the cast chains in get_long --

ssh_agent::get_long(char const * buf)
{
 L(FL("ssh_agent: get_long: %u %u %u %u")
   % widen<u32,char>(buf[0])
   % widen<u32,char>(buf[1])
   % widen<u32,char>(buf[2])
   % widen<u32,char>(buf[3]));
 return ((widen<u32,char>(buf[0]) << 24)
         | (widen<u32,char>(buf[1]) << 16)
         | (widen<u32,char>(buf[2]) << 8)
         | widen<u32,char>(buf[3]));
}

Looking at your code again as I am in order to write that down, here's
something else: You probably want to comment out most of the low-level
L()s, they're only going to be of use to people debugging the packet
decoding code, not to users trying to figure out why their keys aren't
recognized.

zw