Keyword VPD: Read from hardware and update the same on D-bus #457

PriyangaRamasamy · 2024-11-06T06:54:55Z

Keyword's value gets updated on D-bus irrespective of verifying the keyword size.

For example, For a 4 byte keyword, if user gives more than 4 bytes of data to
update, hardware update will accept only 4 bytes but D-bus update will accept
every extra byte given by the user. This will create a mismatch between hardware
and D-bus.

So to avoid such mismatches we came up with the design of updating the keyword's
value in hardware first ( which takes care of size checks) and copy the same
in other sources. With this approach we maintain the reliability of keyword
updates across all other sources.

This commit implements the portion where we read the keyword's value from hardware
(assuming the hardware write is already done from Manager class) and write the same
on D-bus.

Test:
busctl call com.ibm.VPD.Manager /com/ibm/VPD/Manager com.ibm.VPD.Manager ReadKeyword sv "/tmp/keyword.dat" s "SN"
v ay 12 89 72 51 48 66 71 55 56 66 48 49 52

branupama · 2024-11-18T06:26:38Z

Am confused about what this commit "read & update DBus" ?
because Test result shows read Keyword value and prints it on console.

branupama · 2024-11-18T06:28:47Z

include/keyword_vpd_parser.hpp

+     * @return On success return the value read. On failure throw exception.
+     */
+    types::DbusVariantType
+        readKeywordFromHardware(const types::ReadVpdParams i_paramsToReadData);


why "readKeywordFromHardware"
as parser itself is on hardware, should it be "readKeyword" ?

or this parser class reads keyword from DBus as well ?

this is the method overriden from parser_interface class.

yes I know,
but are we going to introduce readKeywordFrom DBus in the future ?. I wanted to understand the reason.

am not sure about the future :)
either IBM/any other company can implement readKeywordFromDBus.

Now If we focus on present, this API reads keyword's value from hardware. that's it.

branupama · 2024-11-18T06:48:24Z

include/keyword_vpd_parser.hpp

+     * @brief API to iterate through the VPD vector to find the given keyword.
+     *
+     * This API iterates through VPD vector using m_vpdIterator and finds the
+     * given keyword. m_vpdIterator points to the keyword name if find is


its findKeyword API, why "m_vpdIterator" should be modified ?
instead you can return the pointer of the found keyword(or the position), incase if you need this data.

And also we can't parse() or populateVpdMap() on this object as 'm_vpdIterator' is moved.

And also we can't parse() or populateVpdMap() on this object as 'm_vpdIterator' is moved.

why can't you move again?

the API name is findKeyword(), getter API's shouldn't modify anything.

why can't you move again?
is this is a suggestion or asking we can move it. ?

This isn't a getter API. this just checks if the keyword is found or not. since we have m_iterator as a member variable which is common to any member of the class, this API uses it to iterate through the vector.

Remember findKeyword() is not modifying the content of the vector. It is using the iterator variable to iterate through the vector. If m_iterator shouldn't be modified, then give me the purpose of having it as a non-const class member.

Also this API acts as a common API to perform both read and write keyword. PR for write keyword #459

why can't you move again?
is this is a suggestion or asking we can move it. ?

Am telling you, if you wish to perform parse/populateVPDMap - you can move the iterator wherever you want to.

branupama · 2024-11-18T06:49:09Z

src/keyword_vpd_parser.cpp

+    if (const types::Keyword* l_kwData =
+            std::get_if<types::Keyword>(&i_paramsToReadData))
+    {
+        l_keyword = *l_kwData;


need to check is this value is empty or not .

branupama · 2024-11-18T06:50:52Z

src/keyword_vpd_parser.cpp

+    std::advance(m_vpdIterator, constants::ONE_BYTE);
+
+    // Read the keyword's value and return
+    return types::DbusVariantType{types::BinaryVector(


don't do operation on return statement, just return the value instead found from the operation.

do the operation and send the final value in the return statement, Dont do multiple things in return statement.
This is the review comment got it for my PR.

What I realised from this is

It avoids any possible error calculations made at return statement.

For developer testing or in future code debugging, you can add print statement to print the value for debugging.

in case we need logging, we can easily add without rewriting the return statement.

It avoids any possible error calculations made at return statement.
it's developer's responsibility to check on the calculations. if there are any errors we can fix it with defect. Having a separate variable doesn't guarantee that the calculation will be error free.

For developer testing or in future code debugging, you can add print statement to print the value for debugging.
Code doesn't stop you to add print statements for debugging.

in case we need logging, we can easily add without rewriting the return statement.
Now in this case is it required to log a message with variant<vector<uint8_t> contents?

In my case, I don't need to construct an object for std::vector and std::variant just to return it back to the caller. In place construction optimises the memory usage.

branupama · 2024-11-18T06:51:45Z

src/keyword_vpd_parser.cpp

+
+int KeywordVpdParser::findKeyword(const std::string& i_keyword)
+{
+    m_vpdIterator = m_keywordVpdVector.begin();


should we check i_keyword is empty or not before doing further operations.

branupama · 2024-11-18T06:53:56Z

src/keyword_vpd_parser.cpp

+    if (findKeyword(l_keyword) != 0)
+    {
+        logging::logMessage("Keyword " + l_keyword + " not found.");
+        throw types::DbusInvalidArgument();


why DBUs error thrown here ?
its h/w operation right .

but the request is from Dbus

This is KeywordVpdParser class, Reader can't know its request from DBus.
Request can come from any where, we can't use DBus error here.

We should keep API's generic and shouldn't bind to one use case. Future needs may come from different place.

IPZ read/write also has similar logging when it fails. So let's discuss this as part of error logging story.

branupama · 2024-11-18T06:58:07Z

src/keyword_vpd_parser.cpp

+        std::string l_keywordName(m_vpdIterator,
+                                  m_vpdIterator + constants::TWO_BYTES);
+
+        if (l_keywordName == i_keyword)


can we use STR_CMP_SUCCESS here ?

why not == ?

we can do same thing in number of ways,
if we are following certain things it should be followed every where.

For string comparison we are using compare() api only in the current code.
As I couldn't recall when this is introduced, we can get clarity from the team

If == still comes under C++ standard, then why we should restrict using it?

branupama · 2024-11-18T07:04:37Z

src/keyword_vpd_parser.cpp

@@ -133,4 +133,91 @@ void KeywordVpdParser::checkNextBytesValidity(uint8_t i_numberOfBytes)
    }
 }

+types::DbusVariantType KeywordVpdParser::readKeywordFromHardware(


can we make it simple,
use populateVpdMap() which gets the keyword map for us.
use this map to get the particular keyword value.

In this way we no need to implement reading logic here .

that won't be simple when there is a read keyword request from D-bus. as for each and every request, populateVpdMap() creates the complete map of keyword-value pairs which is not required in this case.

It depends on the where the keywond, it can be found at the beginning or can be found at the end.

I don't need a map with all keyword values for my case. so why should I use populateVpdMap()

PriyangaRamasamy · 2024-11-18T10:24:34Z

Am confused about what this commit "read & update DBus" ?
because Test result shows read Keyword value and prints it on console.

it is similar to how we implemented writes for other types of VPD.
there is an order of execution for writeKeyword request:

write on hardware -> if it succeeds then,
read the written value from hardware -> if it succeeds then,
write the value (found from step2) on D-bus --> by this way we update the Dbus with correct size of the keyword irrespective of what the user gave.

Keyword's value gets updated on D-bus irrespective of verifying the keyword size. For example, For a 4 byte keyword, if user gives more than 4 bytes of data to update, hardware update will accept only 4 bytes but D-bus update will accept every extra byte given by the user. This will create a mismatch between hardware and D-bus. So to avoid such mismatches we came up with the design of updating the keyword's value in hardware first ( which takes care of size checks) and copy the same in other sources. With this approach we maintain the reliability of keyword updates across all other sources. This commit implements the portion where we read the keyword's value from hardware (assuming the hardware write is already done from Manager class) and write the same on D-bus. Test: busctl call com.ibm.VPD.Manager /com/ibm/VPD/Manager com.ibm.VPD.Manager ReadKeyword sv "/tmp/keyword.dat" s "SN" v ay 12 89 72 51 48 66 71 55 56 66 48 49 52 Change-Id: I60d7a2b418ad5e451e46f288bde3477794145ec0 Signed-off-by: Priyanga Ramasamy <[email protected]>

PriyangaRamasamy requested review from SunnySrivastava1984, branupama and jinuthomas November 6, 2024 13:25

branupama reviewed Nov 18, 2024

View reviewed changes

PriyangaRamasamy force-pushed the keyword-vpd-read-from-hw branch from 21ccd5f to 163ad8a Compare November 18, 2024 10:34

PriyangaRamasamy requested a review from branupama November 18, 2024 10:38

PriyangaRamasamy force-pushed the keyword-vpd-read-from-hw branch from 163ad8a to 3322dd8 Compare November 18, 2024 10:47

PriyangaRamasamy force-pushed the keyword-vpd-read-from-hw branch from 3322dd8 to 4b744f2 Compare November 18, 2024 11:34

PriyangaRamasamy changed the title ~~Keyword VPD: Read from hardware and update on D-bus~~ Keyword VPD: Read from hardware and update the same on D-bus Nov 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keyword VPD: Read from hardware and update the same on D-bus #457

Keyword VPD: Read from hardware and update the same on D-bus #457

PriyangaRamasamy commented Nov 6, 2024 •

edited

Loading

branupama commented Nov 18, 2024 •

edited

Loading

branupama Nov 18, 2024

PriyangaRamasamy Nov 18, 2024

branupama Nov 18, 2024

PriyangaRamasamy Nov 18, 2024

branupama Nov 18, 2024

PriyangaRamasamy Nov 18, 2024

branupama Nov 18, 2024

PriyangaRamasamy Nov 18, 2024

PriyangaRamasamy Nov 18, 2024

branupama Nov 18, 2024

PriyangaRamasamy Nov 18, 2024

branupama Nov 18, 2024

PriyangaRamasamy Nov 18, 2024

branupama Nov 18, 2024

PriyangaRamasamy Nov 18, 2024

branupama Nov 18, 2024

PriyangaRamasamy Nov 18, 2024

branupama Nov 18, 2024

PriyangaRamasamy Nov 18, 2024

branupama Nov 18, 2024

PriyangaRamasamy Nov 18, 2024

branupama Nov 18, 2024

PriyangaRamasamy Nov 18, 2024

branupama Nov 18, 2024

PriyangaRamasamy Nov 18, 2024

branupama Nov 18, 2024

PriyangaRamasamy Nov 18, 2024

branupama Nov 18, 2024

PriyangaRamasamy Nov 18, 2024

PriyangaRamasamy commented Nov 18, 2024

Keyword VPD: Read from hardware and update the same on D-bus #457

Are you sure you want to change the base?

Keyword VPD: Read from hardware and update the same on D-bus #457

Conversation

PriyangaRamasamy commented Nov 6, 2024 • edited Loading

branupama commented Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PriyangaRamasamy commented Nov 18, 2024

PriyangaRamasamy commented Nov 6, 2024 •

edited

Loading

branupama commented Nov 18, 2024 •

edited

Loading