Skip to content

Add missing parameter to on_switch_shutdown_request method. #2567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
liuh-80 merged 5 commits into from
Jan 11, 2023
Merged

Add missing parameter to on_switch_shutdown_request method. #2567

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3972394
Fix orchagent stuch when receive switch shutdown request issue
liuh-80 Dec 12, 2022
06b7da8
Add missing parameter
liuh-80 Dec 12, 2022
3723965
include unistd.h
liuh-80 Dec 12, 2022
9969c65
Revert exit change
liuh-80 Jan 5, 2023
83dd0a3
Add comments to code
liuh-80 Jan 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions orchagent/notifications.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,12 @@ void on_bfd_session_state_change(uint32_t count, sai_bfd_session_state_notificat
// which causes concurrency access to the DB
}

void on_switch_shutdown_request()
void on_switch_shutdown_request(sai_object_id_t switch_id)
Copy link
Copy Markdown
Contributor Author

@liuh-80 liuh-80 Dec 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to sairedis code, this method must have a switch_id parameter:

    switchNotifications.on_switch_shutdown_request(m_switchId);

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jan 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch_id

I agree this is a valid fix. How to prevent future mistake?

Where do you find the ground truth? Add a code comment to refer to it? Possible to add a check?

Copy link
Copy Markdown
Contributor Author

@liuh-80 liuh-80 Jan 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently I didn't find a solution to prevent future mistake.
This is a design issue the sai_attribute_t use a void* for function pointer, so currently can't add a check.

The ground truth is a header file generated by a script, it's difficult to read:
https://github.com/opencomputeproject/SAI/blob/master/meta/parse.pl#L4076

So currently I add caller code as ground of truth:
https://github.com/sonic-net/sonic-sairedis/blob/master/meta/NotificationSwitchShutdownRequest.cpp#L49

{
SWSS_LOG_ENTER();

/* TODO: Later a better restart story will be told here */
SWSS_LOG_ERROR("Syncd stopped");

exit(EXIT_FAILURE);
_exit(EXIT_FAILURE);
}
2 changes: 1 addition & 1 deletion orchagent/notifications.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@ extern "C" {
void on_fdb_event(uint32_t count, sai_fdb_event_notification_data_t *data);
void on_port_state_change(uint32_t count, sai_port_oper_status_notification_t *data);
void on_bfd_session_state_change(uint32_t count, sai_bfd_session_state_notification_t *data);
void on_switch_shutdown_request();
void on_switch_shutdown_request(sai_object_id_t switch_id);