Comment 26 for bug 1647485

There was further discussion in #ubuntu-devel and #ubuntu-release on
this review. See irclogs.ubuntu.com for details.

I have reviewed the patch carefully. As far as I can tell, it does what
it is supposed to do, and I don't see any bugs.

However, this code, particularly the udev_event_apply_format function,
seems to have grown organically and could be significantly improved
IMHO. This patch makes the state of the code worse, and increasingly
fragile. It took hours for me to review the patches for correctness as a
consequence, and I think that this and future changes and reviews are
far more prone to error than they need to be.

Some specifics:

1) The mechanism employed of redirecting s and l (destination buffer and
length remaining count) means that the code inside the redirection is
incredibly fragile. The code inside the switch should be pulled out into
a separate function. Then the redirection would become unnecessary. On
IRC, Dan acknowledged that this may require a large number of parameters
to be provided to the factored out function, but this is in part my
point. Unintended interactions could happen through any of those
parameters. Isolating them would be helpful, since that would identify
and reduce the interactions that need to be checked.

2) IMHO, the patch should use the sizeof(x)/sizeof(x[0]) idiom to get
the buffer lengths, rather than assuming that they are defined by the
same macro. I think likely that someone will attempt to change the array
length by assuming it is only necessary to change it in one place. Dan
suggested on IRC that the opposite point is that changing it from a
buffer to a pointer would cause the sizeof idiom to break, but I think
it's far more likely that a developer will check how a variable is used
before changing its type.

3) The upstream PR refers to tests, but does not add a test for this bug
being fixed. Given the fragility of this fix, I think we should do that.

We have to maintain this code for another four years in Xenial. If we
have to touch this function again in that time, I think we'll need to
spend even more time fixing and reviewing it.

I realise that the fix has already been accepted upstream. But as we (in
Ubuntu) originated the patch, I think we have a responsibility to make
it cleaner. Please refactor it and send that upstream.

I understand that this SRU is necessary to fix an important regression
in Ubuntu, so I don't think it's appropriate to block this given that I
can't find a bug in it. However, I've written this up to register my
unhappiness with this patch.

I appreciate the previous state of the code made it difficult to fix in
a better way; IMHO, this is a reason to refactor it first when sending
the development fix upstream. Perhaps the SRU would then need to have
been a different, more minimal and hacky patch, but that could be
focused on minimising regression risk and fixing just the problem at
hand rather than the general case.

If you add a test for this bug, please bundle it with the next SRU of
this package in Xenial.

I note that Zesty is not Fix Released. However, since this is a
regression fix, I don't think it would be appropriate to wait. Please
get the fix landed in Zesty as soon as possible; I certainly expect the
Zesty fix to be able to land well before we are ready to release this in
to Xenial/Yakkety -updates.

Given that this is a regression fix, we might expect to cut short the
usual 7-day aging period. However, given the fragility of the code
involved and because this is not a straight revert, I don't think that is
warranted in this case.