Fix error-handling around AddResponseData

[bzbarsky-apple]

Problem

Right now we have various code that does:

1. Call CommandHandler::AddResponseData
2. If that fails add an error status.

But if AddResponseData failed the TLV is going to be in some broken state, so this error-handling code is just wrong.

Proposed Solution

Move adding on error status into AddResponseData, remove it from all consumers.

[erjiaqing]

I would suggest making the encoding logic "atomic" (i.e. rollback on failure, just like what we do for attributes) instead of encode a status silently, and then provide a wrapper AddResponseDataOrStatus (just like CheckSuccess in src/app/clusters/general-commissioning-server/general-commissioning-server.cpp) for "try to add a response, on failure, encode a status". Since we are expecting (or even assuming) the AddResponseData to success.

After checking the related code, the of error code for AddResponseData could be one of:

1. We are running out of buffer pool, in this case, rollback + encode status will also fail in this case.
2. We are trying to encode something that is too big to fit into the response, there could be some bug when preparing the data since we are assuming the data can fir into the response.

[bzbarsky-apple]

and then provide a wrapper AddResponseDataOrStatus

Which do we expect consumers to use most? We should have AddResponse have the behavior most consumers want, and have a longer name for the less-commonly-desired behavior....

We are running out of buffer pool, in this case, rollback + encode status will also fail in this case.

Agreed.

We are trying to encode something that is too big to fit into the response, there could be some bug when preparing the data since we are assuming the data can fir into the response.

There can absolutely be bugs in the cluster impl, or bugs in the client (sending multiple commands in a single invoke whose response commands don't all fit in one packet).

I'm not actually sure what the best option is for the latter case.... If we get to a point we can't even encode a status in the space left, what should we do?

[bzbarsky-apple]

@CamWms @mrjerryjohns thoughts on the above?

[mrjerryjohns]

I'd agree with updating AddResponse to internally, blit a status if it can on failure. But it's still possible that some steps here could fail, in which case, it stands to reason that AddResponse should only return an error if it encountered an error in the fallback path of encoding the status.

[mrjerryjohns]

More-over, trying to add a status after that should actually fail to prevent double encoding of a status.

[erjiaqing]

More-over, trying to add a status after that should actually fail to prevent double encoding of a status.

The internal state of command handler will reject encoding more status, also, when AddStatus failed, the command handler cannot encode anything probably.