Skip to content
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

Jump to bottom

Updates to resolve getting a response for wrong URI in Update #1143

Open
wants to merge 1 commit into
base: 1110
Choose a base branch
from
Open

Updates to resolve getting a response for wrong URI in Update #1143

wants to merge 1 commit into from

Conversation

abiasojo
Copy link
Contributor

@abiasojo abiasojo commented Feb 7, 2025 •
edited
Loading

Updates to resolve getting a response for wrong URI in Update
Service URI (/redfish/v1/UpdateService/FirmwareInventory/)

Tested with the correct URI and results are correct.
Tested with wrong URI and 404 is returned

@abiasojo abiasojo requested review from baemyung and gtmills February 7, 2025 23:57
baemyung
baemyung reviewed Feb 8, 2025
View reviewed changes
Copy link
Contributor

@baemyung baemyung left a comment

Choose a reason for hiding this comment

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

It seems also needed to be upstreamed.

@baemyung baemyung requested a review from jeaaustx February 8, 2025 13:02
gtmills
gtmills reviewed Feb 10, 2025
View reviewed changes
redfish-core/lib/update_service.hpp Outdated
@@ -1262,7 +1262,15 @@ inline void requestRoutesSoftwareInventory(App& app)
continue;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of line 1255 - 1265 could we go with object_path as in:

sdbusplus::message::object_path path(obj.first);
std::string id = path.filename(); 

if (id.empty())
{
     BMCWEB_LOG_DEBUG("Failed to find software id in {}", obj.first);
     continue;
}
           
if (id != *swId)
{
  continue;
}
if (obj.second.empty())
{
    continue; 
}
found = true;

https://github.com/openbmc/bmcweb/blob/80d2ef31c343ec9c562778848b9d62fb6ce57c07/redfish-core/include/utils/chassis_utils.hpp#L57

https://github.com/openbmc/bmcweb/blob/80d2ef31c343ec9c562778848b9d62fb6ce57c07/redfish-core/src/dbus_log_watcher.cpp#L118

Copy link
Contributor

Choose a reason for hiding this comment

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

Reading README (https://github.com/openbmc/phosphor-dbus-interfaces/blob/master/yaml/xyz/openbmc_project/Software/README.md, or https://gerrit.openbmc.org/plugins/gitiles/openbmc/docs/+/refs/changes/80/24980/2/REST+REDFISH-cheatsheet.md) , the dbus path name for image seems like

/xyz/openbmc_project/software/<swId>/

Then, can we use dbus::utility::checkDbusPathExists() instead of `getSubTree()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated to use obj_path and tested.

jeaaustx
jeaaustx reviewed Feb 10, 2025
View reviewed changes
Copy link
Contributor

@jeaaustx jeaaustx left a comment

Choose a reason for hiding this comment

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

I think it will be helpful to include the command output or more details of the problem being fixed by this change in the commit message. It will help in later releases when this patch is being rebased to assure the change is correctly folded forward.

(Might also be helpful to include the defect number.)

redfish-core/lib/update_service.hpp Outdated
@@ -1262,7 +1262,15 @@ inline void requestRoutesSoftwareInventory(App& app)
continue;
}

found = true;
std::string swIdpath = "/xyz/openbmc_project/software/";
Copy link
Contributor

Choose a reason for hiding this comment

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

Above on L1230-1231 we are setting the @odata.id value before doing the async D-Bus call. On failure that will be part of the response. I think for Redfish that is not supposed to be in the response on failure. Seems like it shouldn't be set until we have confirmed the swId is valid.

@gtmills Please confirm if my understanding is correct or not.

Copy link
Contributor Author

@abiasojo abiasojo Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@jeaaustx and @gtmills this is what the output looks like for a wrong URI after the fix.
Is this okay?

curl -k -H "X-Auth-Token: $bmc_token" -X GET https://$bmc_ip/redfish/v1/UpdateService/FirmwareInventory/b8
{
"@odata.id": "/redfish/v1/UpdateService/FirmwareInventory/b8",
"error": {
"@Message.ExtendedInfo": [
{
"@odata.type": "#Message.v1_1_1.Message",
"Message": "The resource at the URI '/redfish/v1/UpdateService/FirmwareInventory/b8' was not found.",
"MessageArgs": [
"/redfish/v1/UpdateService/FirmwareInventory/b8"
],
"MessageId": "Base.1.19.ResourceMissingAtURI",
"MessageSeverity": "Critical",
"Resolution": "Place a valid resource at the URI or correct the URI and resubmit the request."
}
],
"code": "Base.1.19.ResourceMissingAtURI",
"message": "The resource at the URI '/redfish/v1/UpdateService/FirmwareInventory/b8' was not found."
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we should not be filling in "@odata.id": on a 404

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to not fill in "@odata.id" on a 404

@abiasojo abiasojo force-pushed the 1110-wrongURIresp branch 2 times, most recently from f23643f to 7c9b325 Compare February 12, 2025 02:22
baemyung
baemyung approved these changes Feb 12, 2025
View reviewed changes
Copy link
Contributor

@baemyung baemyung left a comment

Choose a reason for hiding this comment

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

Other than a minor comment, I think it seems good.

redfish-core/lib/update_service.hpp
BMCWEB_LOG_DEBUG("Failed to find software id in {}",
obj.first);
continue;
}
Copy link
Contributor

@baemyung baemyung Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think L1257-L1262 is not needed, as L1263 if(id != *swId) will cover this part.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am fine with it since

bmcweb/redfish-core/include/utils/chassis_utils.hpp

Line 49 in 5f88ee7

if (chassisName.empty())

has it

@abiasojo
Fix for defect 609923
d3faf09 
Updates to resolve getting a response for wrong URI in Update
Service URI (/redfish/v1/UpdateService/FirmwareInventory/)

After the fix a wrong URI that partially matches the last
characters in a valid URI now gets a 404 as shown below

curl -k -H "X-Auth-Token: $bmc_token" -X GET https://$bmc_ip/redfish/v1/UpdateService/FirmwareInventory/b8
{
  "@odata.id": "/redfish/v1/UpdateService/FirmwareInventory/b8",
  "error": {
    "@Message.ExtendedInfo": [
      {
        "@odata.type": "#Message.v1_1_1.Message",
        "Message": "The resource at the URI '/redfish/v1/UpdateService/FirmwareInventory/b8' was not found.",
        "MessageArgs": [
          "/redfish/v1/UpdateService/FirmwareInventory/b8"
        ],
        "MessageId": "Base.1.19.ResourceMissingAtURI",
        "MessageSeverity": "Critical",
        "Resolution": "Place a valid resource at the URI or correct the URI and resubmit the request."
      }
    ],
    "code": "Base.1.19.ResourceMissingAtURI",
    "message": "The resource at the URI '/redfish/v1/UpdateService/FirmwareInventory/b8' was not found."
  }
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gtmills gtmills gtmills left review comments

@jeaaustx jeaaustx jeaaustx left review comments

@baemyung baemyung baemyung approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@abiasojo @gtmills @baemyung @jeaaustx