Error / status codes

[steven-bellock]

This is a rough proposal for libspdm-specific error / status codes, so that we can discuss it at the 6-Dec-2021 meeting. The bits are in order of least-granularity to most-granularity, so a libspdm integrator is free to look at only the most-significant bit or all 32 bits (and every bit in between). Error Origin is a little iffy. Not sure if we need it, but it might have its uses.

Error Status (returned by functions).

status[31]    - Error Bit
                - 0: SUCCESS (No error.)
                - 1: ERROR (Error.)
status[30:29] - Error Origin.                
                - 0: SUCCESS (No error.)
                - 1: INTEGRATOR (Error originated with libspdm integrator.)
                    - Overlaps with asserts.
                - 2: INTERNAL (Error originated internally to Requester or Responder.)
                    - For example, Crypto module self-test failed.
                - 3: PEER (Error originated externally with peer.)     
                    - For example, validation of a request or response failed.
status[28:24] - Reserved
status[23:16] - Error Source
                - 0: SUCCESS
                - 1: LIBSPDM_CORE
                - 2: CRYPTO
                - 3: TRANSPORT
                - 4: SYSTEM
                    - For example, malloc failed.
                - 5: MEASUREMENT
                - 6 to 255: Reserved.
status[15:0]  - Error Code (within the context of the Error Source).

In addition, extended error information can be stored in the libspdm context. For example if the Requester receives an ERROR response then the extended error information would contain the RequestResponseCode and ERROR response's code and data.

[steven-bellock]

Examples (note that naming is not final, just used for illustrative purposes.)

// Requester
// Random number generation for CHALLENGE.nonce fails.
status = libspdm_challenge(...);
// status == {ERROR, INTERNAL, CRYPTO, RNG_FAIL}

// Requester
// Responder returns a malformed certificate to Requester.
status = libspdm_get_certificate(...);
// status == {ERROR, PEER, CRYPTO, BAD_CERT}

// Responder
// Responder attempts to receive a request but the transport layer's CRC fails.
status = libspdm_responder_dispatch_message(...);
// status == {ERROR, PEER, TRANSPORT, CRC_FAIL}

// Responder
// Responder attempts to collect measurements in response to a GET_MEASUREMENT but it fails.
status = libspdm_responder_dispatch_message(...);
// status == {ERROR, INTERNAL, MEASUREMENT, MEAS_FAIL}

[taprinz]

I like the breakdown - I think we might want to add a bit or two to "error origin" part of status - I could see us wanting to add another category we discover while breaking down things into codes. I'm not sure whether we should use one definition as "success", as it precludes us from ever including any sort of "warning" or "informational" style return code.

Additionally, I would personally prefer named defines in the actual source itself, like so:

#define RETURN_CRYPTO_BAD_CERT  LIBSPDM_STATUS(ERROR, CRYPTO, PEER,     0x00)
#define RETURN_CRYPTO_RNG_FAIL  LIBSPDM_STATUS(ERROR, CRYPTO, INTERNAL, 0x01)

// Used as 
return RETURN_CRYPTO_BAD_CERT;
// as opposed to 
return LIBSPDM_STATUS(ERROR, INTERNAL, CRYPTO, BAD_CERT);

When we use a lot of them, it'd make it a bit less verbose, and encourages some consistency in the usage of error types.

[jyao1]

Agree with @taprinz. The code should use simple status MACRO.

[richkong88]

I agree with @taprinz with the origin status as well. I have been debugging issues and end up getting very generic error messages. These are in environments where there is no debugger and it is difficult to collect logs, so it's nice if more detailed error info is sent in the response.

What I typically see is ErrorCode = some value and ErrorData = 0. There may be multiple places in a single response handler using the same ErrorCode. This occurs almost everywhere and makes it difficult to identify the specific "if" statement that is failing.

It would be very helpful if every "if" statement that can fail, had a unique way to identify it, at least within a particular flow. For example, all Error Data values within the Get Measurements flow were distinct, but could overlap with those in a different flow such as Negotiate Algorithms.

Also adding on to the MACRO usage. I would prefer if we had some macro for checking success and/failure so you could go:

status = libspdm_get_measurements_hash();
if (LIBSPDM_FAILED(status)) {

}

[jyao1]

Does RETURN_ERROR meet your requirement ? (https://github.com/DMTF/libspdm/blob/main/include/hal/base.h#L853)

we need rename it to LIBSPDM_RETURN_ERROR.

[richkong88]

Yes. That macro works.

I would also like to add that we should return, whatever error status we decide on, from all functions that can have errors. Some functions currently use boolean as the return value. These should be converted as well.

[taprinz]

agree with @rkongintel regarding boolean versus libspdm return values. We should avoid mixing the two unless absolutely necessary - as it's easy to mix up the handling between the two, with potentially bad unintended consequences - for example, checking for RETURN_ERROR on a function that returns boolean will always return success.

Also, what about LIBSPDM_ERROR() versus LIBSPDM_RETURN_ERROR()? The former is a bit less verbose

[richkong88]

Can we make it a verb? LIBSPDM_IS_ERROR(X)

[richkong88]

Have we bottomed out on this? I can take a stab at implementing it to see how it goes.

[steven-bellock]

Have we bottomed out on this? I can take a stab at implementing it to see how it goes.

Let me get the framework (header file with macros) in place and then we can start the long process of dishing out error codes.