Skip to content

1120 Trace URI on CRITICAL message Response #1323

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.

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

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

Conversation

abiasojo
Copy link
Contributor

Trace URI on CRITICAL message Response content provided but code was no-content or not_modified.

@abiasojo abiasojo force-pushed the 1120-criticalmsgrspnocntnt branch from 106e863 to 377de0e Compare June 18, 2025 21:11
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.

Please add some example test statements, and also like Redfish Validator run

@@ -207,7 +207,8 @@ class HTTP2Connection :

completeResponseFields(stream.accept, res);
res.addHeader(boost::beast::http::field::date, getCachedDateStr());
res.preparePayload();
crow::Request& thisReq = *it->second.req;
res.preparePayload(thisReq.url().path());
Copy link
Contributor

Choose a reason for hiding this comment

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

L204 already has this

 Http2StreamData& stream = it->second;

So, these L210-L211 can be like

res.preparePayload(stream->url().path());

Copy link
Contributor Author

@abiasojo abiasojo Jun 19, 2025

Choose a reason for hiding this comment

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

It would be
res.preparePayload(stream.req->url().path());

@@ -186,7 +186,7 @@ struct Response
return response.body().payloadSize();
}

void preparePayload()
void preparePayload(std::string_view s)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the argument can be more readable like

void preparePayload(std::string_view url)

"allowed to have a body",
logPtr(this));
"allowed to have a body \"{}\"",
logPtr(this), s);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to specify url like this?

"allowed to have a body, for url {}",

@gtmills
Copy link
Contributor

gtmills commented Jun 19, 2025

Will want to go upstream here

Trace URI on CRITICAL message Response content provided but code
was no-content or not_modified.

curl -k -H "X-Auth-Token: $bmc_token" -H "Content-Type: application/json" -X PATCH -d '{"LocationIndicatorActive":true}' https://$bmc/redfish/v1/Managers/bmc

curl -k -H "Content-Type: application/json" -d '{"PowerRestorePolicy":"LastState"}' -X PATCH https://${bmc}/redfish/v1/Systems/system

root@p10bmc:~# journalctl | grep 'Response content'
Jun 19 16:51:33 p10bmc bmcwebd[1034]: [http_response.hpp:213] 0xb75210 Response content provided but code was no-content or not_modified, which aren't allowed t
o have a body for url : "/redfish/v1/Managers/bmc"
Jun 19 16:52:00 p10bmc bmcwebd[1034]: [http_response.hpp:213] 0xb75210 Response content provided but code was no-content or not_modified, which aren't allowed t
o have a body for url : "/redfish/v1/Systems/system"

Signed-off-by: Abiola Asojo <abiola.asojo@ibm.com>
@abiasojo abiasojo force-pushed the 1120-criticalmsgrspnocntnt branch from 377de0e to 2d85598 Compare June 19, 2025 17:40
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 looks good to me.
(Please also upstream this and add Change-Id into this PR commit).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants