There are several separate issues involving error-handling that Greg pointed out in his 8/23 email.
This KanBoard/github issue in-particular is meant to deal with the cases where Greg didn't understand that we were indeed actually handling the errors, but were passing the error code back to the firmware via a controlvm response. This confusion arose because our functions are NOT actually returning the error code in a function return result, but are instead just sending a controlvm response.
We and Greg agreed (on 9/7) that it is ok to address these cases by developing a scheme whereby all of the functions that currently directly send a controlvm response, will be changed to return their status code to a common dispatch function, whose job it will be to send this status code back to the firmware (via controlvm response). This might even let us consolidate/eliminate a few of the other functions we have that deal with various ways of formatting firmware responses, by encompassing their logic into this common dispatch function.
That strategy should make it clearer and more explicit as to what is going on here.
The snippets below indicate the initial 8/23 comments from Greg that will be addressed by this issue.
>
> if (pending_msg_hdr->id != (u32)cmd_id)
> return;
>
> controlvm_respond(pending_msg_hdr, response);
> }
Ah, no error handling anyway, so who cares about checking anything?
> }
>
> static void
> device_responder(enum controlvm_id cmd_id,
> struct controlvm_message_header *pending_msg_hdr,
> int response)
> {
> if (!pending_msg_hdr)
> return; /* no controlvm response needed */
>
> if (pending_msg_hdr->id != (u32)cmd_id)
> return;
>
> controlvm_respond(pending_msg_hdr, response);
> }
Same as above.
> }
> }
>
> out_respond:
> bus_responder(cmd, pmsg_hdr, response);
error handling? I'm a broken record, I know. But really, your lack of
robustness is amazing for something that is supposed to be relied on.
> goto out_respond;
> }
>
> memcpy(pmsg_hdr, msg_hdr,
> sizeof(struct controlvm_message_header));
> dev_info->pending_msg_hdr = pmsg_hdr;
> }
>
> if (response >= 0) {
> switch (cmd) {
> case CONTROLVM_DEVICE_CREATE:
> chipset_device_create(dev_info);
> break;
> case CONTROLVM_DEVICE_CHANGESTATE:
> /* ServerReady / ServerRunning / SegmentStateRunning */
> if (state.alive == segment_state_running.alive &&
> state.operating ==
> segment_state_running.operating) {
> chipset_device_resume(dev_info);
> }
> /* ServerNotReady / ServerLost / SegmentStateStandby */
> else if (state.alive == segment_state_standby.alive &&
> state.operating ==
> segment_state_standby.operating) {
> /*
> * technically this is standby case
> * where server is lost
> */
> chipset_device_pause(dev_info);
> }
> break;
> case CONTROLVM_DEVICE_DESTROY:
> chipset_device_destroy(dev_info);
> break;
> }
> }
>
> out_respond:
> device_responder(cmd, pmsg_hdr, response);
Wait! We ran out of memory! but we didn't do anything about it :(
{sniff}
> }
>
> static void
> bus_create(struct controlvm_message *inmsg)
> {
> struct controlvm_message_packet *cmd = &inmsg->cmd;
> u32 bus_no = cmd->create_bus.bus_no;
> int rc = CONTROLVM_RESP_SUCCESS;
> struct visor_device *bus_info;
> struct visorchannel *visorchannel;
>
> bus_info = visorbus_get_device_by_id(bus_no, BUS_ROOT_DEVICE, NULL);
> if (bus_info && (bus_info->state.created == 1)) {
> POSTCODE_LINUX_3(BUS_CREATE_FAILURE_PC, bus_no,
> POSTCODE_SEVERITY_ERR);
> rc = -CONTROLVM_RESP_ERROR_ALREADY_DONE;
> goto out_bus_epilog;
> }
> bus_info = kzalloc(sizeof(*bus_info), GFP_KERNEL);
> if (!bus_info) {
> POSTCODE_LINUX_3(BUS_CREATE_FAILURE_PC, bus_no,
> POSTCODE_SEVERITY_ERR);
> rc = -CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
> goto out_bus_epilog;
Same as above, at least you are consistant, that should count for
something...
> }
> bus_epilog(bus_info, CONTROLVM_BUS_CONFIGURE, &inmsg->hdr,
> rc, inmsg->hdr.flags.response_expected == 1);
I like errors, too bad you ate them all.
Greg uses this as an example of how to do it right:
> }
>
> static void
> my_device_create(struct controlvm_message *inmsg)
> {
> struct controlvm_message_packet *cmd = &inmsg->cmd;
> u32 bus_no = cmd->create_device.bus_no;
> u32 dev_no = cmd->create_device.dev_no;
> struct visor_device *dev_info = NULL;
> struct visor_device *bus_info;
> struct visorchannel *visorchannel;
> int rc = CONTROLVM_RESP_SUCCESS;
>
> bus_info = visorbus_get_device_by_id(bus_no, BUS_ROOT_DEVICE, NULL);
> if (!bus_info) {
> POSTCODE_LINUX_4(DEVICE_CREATE_FAILURE_PC, dev_no, bus_no,
> POSTCODE_SEVERITY_ERR);
> rc = -CONTROLVM_RESP_ERROR_BUS_INVALID;
> goto out_respond;
> }
>
> if (bus_info->state.created == 0) {
> POSTCODE_LINUX_4(DEVICE_CREATE_FAILURE_PC, dev_no, bus_no,
> POSTCODE_SEVERITY_ERR);
> rc = -CONTROLVM_RESP_ERROR_BUS_INVALID;
> goto out_respond;
See, this is better, fix the function above to look a bit like this (fix
the error codes, and the looney tracing mess.
Like here, I don't think he understands that we are responding, but with
an error code:
> }
>
> dev_info = visorbus_get_device_by_id(bus_no, dev_no, NULL);
> if (dev_info && (dev_info->state.created == 1)) {
> POSTCODE_LINUX_4(DEVICE_CREATE_FAILURE_PC, dev_no, bus_no,
> POSTCODE_SEVERITY_ERR);
> rc = -CONTROLVM_RESP_ERROR_ALREADY_DONE;
> goto out_respond;
> }
>
> dev_info = kzalloc(sizeof(*dev_info), GFP_KERNEL);
> if (!dev_info) {
> POSTCODE_LINUX_4(DEVICE_CREATE_FAILURE_PC, dev_no, bus_no,
> POSTCODE_SEVERITY_ERR);
> rc = -CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
> goto out_respond;
But it's an ERROR!!! {sigh}
> }
>
> dev_info->chipset_bus_no = bus_no;
> dev_info->chipset_dev_no = dev_no;
> dev_info->inst = cmd->create_device.dev_inst_uuid;
>
> /* not sure where the best place to set the 'parent' */
> dev_info->device.parent = &bus_info->device;
>
> POSTCODE_LINUX_4(DEVICE_CREATE_ENTRY_PC, dev_no, bus_no,
> POSTCODE_SEVERITY_INFO);
>
> visorchannel =
> visorchannel_create_with_lock(cmd->create_device.channel_addr,
> cmd->create_device.channel_bytes,
> GFP_KERNEL,
> cmd->create_device.data_type_uuid);
>
> if (!visorchannel) {
> POSTCODE_LINUX_4(DEVICE_CREATE_FAILURE_PC, dev_no, bus_no,
> POSTCODE_SEVERITY_ERR);
> rc = -CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
> kfree(dev_info);
> dev_info = NULL;
> goto out_respond;
> }
> dev_info->visorchannel = visorchannel;
> dev_info->channel_type_guid = cmd->create_device.data_type_uuid;
> if (uuid_le_cmp(cmd->create_device.data_type_uuid,
> spar_vhba_channel_protocol_uuid) == 0)
> save_crash_message(inmsg, CRASH_DEV);
>
> POSTCODE_LINUX_4(DEVICE_CREATE_EXIT_PC, dev_no, bus_no,
> POSTCODE_SEVERITY_INFO);
> out_respond:
> device_epilog(dev_info, segment_state_running,
> CONTROLVM_DEVICE_CREATE, &inmsg->hdr, rc,
> inmsg->hdr.flags.response_expected == 1, 1);
> }
>
> static void
You really don't like returning errors, should I just stop now? I'm
just over half way done with the file, and still I'm not at the
functions that made me want to respond to this code...
Oh look, 3 more hours left in my flight. I guess it's either this code
review or rewatching the Lego movie for the zillionth time. Tempting...
> my_device_changestate(struct controlvm_message *inmsg)
> {
> struct controlvm_message_packet *cmd = &inmsg->cmd;
> u32 bus_no = cmd->device_change_state.bus_no;
> u32 dev_no = cmd->device_change_state.dev_no;
> struct spar_segment_state state = cmd->device_change_state.state;
> struct visor_device *dev_info;
> int rc = CONTROLVM_RESP_SUCCESS;
>
> dev_info = visorbus_get_device_by_id(bus_no, dev_no, NULL);
> if (!dev_info) {
> POSTCODE_LINUX_4(DEVICE_CHANGESTATE_FAILURE_PC, dev_no, bus_no,
> POSTCODE_SEVERITY_ERR);
> rc = -CONTROLVM_RESP_ERROR_DEVICE_INVALID;
> } else if (dev_info->state.created == 0) {
> POSTCODE_LINUX_4(DEVICE_CHANGESTATE_FAILURE_PC, dev_no, bus_no,
> POSTCODE_SEVERITY_ERR);
> rc = -CONTROLVM_RESP_ERROR_DEVICE_INVALID;
> }
> if ((rc >= CONTROLVM_RESP_SUCCESS) && dev_info)
> device_epilog(dev_info, state,
> CONTROLVM_DEVICE_CHANGESTATE, &inmsg->hdr, rc,
> inmsg->hdr.flags.response_expected == 1, 1);
You know that I'm going to say...
> }
>
> static void
> my_device_destroy(struct controlvm_message *inmsg)
> {
> struct controlvm_message_packet *cmd = &inmsg->cmd;
> u32 bus_no = cmd->destroy_device.bus_no;
> u32 dev_no = cmd->destroy_device.dev_no;
> struct visor_device *dev_info;
> int rc = CONTROLVM_RESP_SUCCESS;
>
> dev_info = visorbus_get_device_by_id(bus_no, dev_no, NULL);
> if (!dev_info)
> rc = -CONTROLVM_RESP_ERROR_DEVICE_INVALID;
> else if (dev_info->state.created == 0)
> rc = -CONTROLVM_RESP_ERROR_ALREADY_DONE;
>
> if ((rc >= CONTROLVM_RESP_SUCCESS) && dev_info)
> device_epilog(dev_info, segment_state_running,
> CONTROLVM_DEVICE_DESTROY, &inmsg->hdr, rc,
> inmsg->hdr.flags.response_expected == 1, 1);
same here.
> if (msg_hdr->flags.response_expected)
> controlvm_respond(msg_hdr, rc);
But you don't propagate it up? I guess you are always ready, even if
your hardware said it wasn't.
> if (msg_hdr->flags.response_expected)
> controlvm_respond(msg_hdr, rc);
and you ignore it...
>
> if (rc != CONTROLVM_RESP_SUCCESS)
> rc = -rc;
> if (msg_hdr->flags.response_expected)
> controlvm_respond(msg_hdr, rc);
again...
>
> bus_responder(CONTROLVM_BUS_CREATE, bus_info->pending_msg_hdr,
> response);
>
> kfree(bus_info->pending_msg_hdr);
> bus_info->pending_msg_hdr = NULL;
It always works?
> {
> if (response >= 0)
> dev_info->state.created = 1;
>
> device_responder(CONTROLVM_DEVICE_CREATE, dev_info->pending_msg_hdr,
> response);
>
> kfree(dev_info->pending_msg_hdr);
> dev_info->pending_msg_hdr = NULL;
No errors?
KanBoard-24534
There are several separate issues involving error-handling that Greg pointed out in his 8/23 email.
This KanBoard/github issue in-particular is meant to deal with the cases where Greg didn't understand that we were indeed actually handling the errors, but were passing the error code back to the firmware via a controlvm response. This confusion arose because our functions are NOT actually returning the error code in a function return result, but are instead just sending a controlvm response.
We and Greg agreed (on 9/7) that it is ok to address these cases by developing a scheme whereby all of the functions that currently directly send a controlvm response, will be changed to return their status code to a common dispatch function, whose job it will be to send this status code back to the firmware (via controlvm response). This might even let us consolidate/eliminate a few of the other functions we have that deal with various ways of formatting firmware responses, by encompassing their logic into this common dispatch function.
That strategy should make it clearer and more explicit as to what is going on here.
The snippets below indicate the initial 8/23 comments from Greg that will be addressed by this issue.
Greg uses this as an example of how to do it right:
Like here, I don't think he understands that we are responding, but with an error code: