On Fri, 22 Sep 2017 14:01:04 -0700
Brandon Williams <bmw...@google.com> wrote:
> > +static void process_capabilities(int len)
> > +{
> > + int nul_location = strlen(packet_buffer);
>
> It may make more sense to not rely on accessing a global buffer here
> directly and instead pass in the buff you're working on, much like your
> are doing with len.
I wanted to preserve the existing code's behavior of using the global
buffer, and it didn't make sense for me to alias it (like the existing
code does).
I pass len in because I need to read beyond NUL.
> I'm not the biggest fan of dynamically allocating this and then using it
> to compare. Maybe we can check to make sure that the oid matches the
> null_oid and that the name matches the "capabilities^{}" string? That
> way you can avoid the allocation?
The dynamic allocation happens only once per process, since it is
static. To check the oid matches null_oid, I would have to parse it
first, and that seemed unnecessary.
Ideally I would just check again "000...000 capabilities^{}", but
writing it in source code would be error-prone, I think.
> > +static int process_ref(struct ref ***list, unsigned int flags,
> > + struct oid_array *extra_have)
>
> So from comparing this to the current code it doesn't look like there is
> a check in 'process_ref' that ensures that a 'capabilities^{}' line
> doesn't show up after a normal ref, or am I missing something?
Ah...yes, you're right. I'll fix this by adding a check in
process_ref().
This is getting more complicated than I thought, so I'll wait a while
for other comments before sending out an updated patch.
