On Fri, Nov 03, 2023 at 08:35:08PM +0000, Stephan, Alexander wrote:
> Hi Willy,
>
> Thanks for the review. No problem for calling me Stephan, I am totally used
> to that, my teachers did that for years.
Oh I was sure you were used to but anyway I don't like calling people
incorrectly.
> > Yeah I agree, that part is not pretty. And as often in such code, not
> > handling errors makes them even more complicated to unroll as you
> > experienced in the list loop you added, which could otherwise have been
> > addressed using just a goto and a few free().
>
> > Do not hesitate to propose extra patches to improve this, contributions
> > that make the code more reliable and maintainable are totally welcome!
>
> Sure, if I have some extra time on my hands. Actually, a colleague of mine is
> preparing some patches like this. Since there are many small changes (e.g.,
> adding a free), should can we batch them or are individual commits required?
It depends. The rule of thumb for this is to think about the risk of breakage
and the ability to bisect (and possibly revert) later. But it's clear that
some API changes need to be performed atomically and can sometimes be a bit
larger than initially expected. Among the things we practice if some large
area changes are needed, is to try to make a first set of preparation patches
that apply most of the visual changes without changing semantics (e.g. move
some code into a separate function, or reindent 500 lines at once in an
"if (1)" block etc). And only then the patches that perform the real changes
are applied. This has the benefit that the first patch promises not to
change the logic so that it cannot break on bisect and doesn't need to be
analyzed later, and the patches that change the logic are much smaller.
> > Are you sure it was not a side effect of a debugging session with some
> > temporary code in it ?
>
> Pretty sure, yes. I'll will get back to this at the beginning of next week,
> if it's okay. I compiled with a debug flag, but I wouldn't usually expect
> this to be an issue.
OK let's try to figure that later. I'll merge your code in its current
form for now.
> > So if that's OK for you I can change it now before merging.
>
> Ah, I had used a SMP_VAL_* before, but I was not 100% about the meaning. Then
> I fell back to the proxy. Feel free to change it!
;-) Will do.
> > Yes very likely. Originally the code didn't check for allocation errors
> > during parsing because it was the boot phase, and we used to consider that
> > a memory allocation error would not always be reportable anyway, and given
> > that it was at boot time, the result was going to be the same.
> > But much later the code began to be a bit more dynamic, and such practices
> > are no longer acceptable in parts that can be called at run time (e.g.
> > during an "add server" on the CLI). It's particularly difficult to address
> > some of these historic remains but from time to time we manage to pass an
> > extra pointer to some functions to write an error, and make a function
> > return a set of flags among ERR_{WARN,FATAL,ALERT,ABORT}. But it takes a
> > lot of time > for little to no perceived value for the vast majority of
> > users :-(
>
> Interesting how that came to be, no accusations. I see that this is not a
> high priority. Not sure if I have the time to provide a fix here but I'll
> keep it mind.
OK. No rush but similarly, patches welcome of course.
> > Another point, in make_proxy_line_v2() I'm seeing that you've placed
> > everything under "if (strm)". I understand why, it's because you didn't
> > want to call build_logline() without a stream. That made me think "hmmm we
> > already have the ability to do that", and I found it, you can use
> > sess_build_logline() instead, that takes the session separately, and
> > supports an optional stream. That would be useful for health checks for
> > example, since they don't have streams. But there's a catch: in function
> > make_proxy_line_v2() we don't have the session at the moment, and
> > build_logline() extracts it from the stream. Normally during a session
> > establishment, the session is present in connection->owner, thus
> > remote->owner in make_proxy_line_v2(). I suggest that you give this a
> > try to confirm that it works for you, this way we'd be sure that health
> > checks can also send the ppv2 fields. But that's not critical given that
> > there's no bad behavior right now, it's just that fields will be ignored,
> > so this can be done in a subsequent patch.
>
> Again, I don't mind if you make the SMP_VAL_*.
OK will do it.
> I tried the
> sess_build_logline() and it seems to work perfectly. The tests also still
> pass, so looks good. I would like to provide a second patch for this next
> week, if this okay.
Sure, that would be great!
thanks and have a nice week-end!
Willy
