picmg led get: fix the interpretation and format #298

PengWang-SMARTM · 2021-05-28T18:23:15Z

The picmg led get command result misinterprets the override state by
looking at data offset 2 rather than 5. The format and interpretation
can be improved as well.

Resolves #297

Signed-off-by: peng.wang@smartm.com peng.wang@smartm.com

The picmg led get command result misinterpret the override state by looking at data offset 2 rather than 5. The format and interpretation can be improved as well. Signed-off-by: peng.wang@smartm.com <peng.wang@smartm.com>

AlexanderAmelkin · 2021-06-01T14:34:05Z

Thank you. Could you please post examples of the old and new output for the command?

PengWang-SMARTM · 2021-06-01T15:25:31Z

Thank you. Could you please post examples of the old and new output for the command?

It's posted in Issue #297

Before:

# ipmitool raw 0x2c 8 0 0 1
 00 03 ff 00 02 0a 14 03 00
# ipmitool picmg led get 0 1
LED states:                                               3     [LOCAL CONTROL|OVERRIDE]
  Local Control function:     ff  [ON]
  Local Control On-Duration:  0
  Local Control Color:        2  [RED]
  Override function:     a  [ON]
  Override On-Duration:  14
  Override Color:        3  [GREEN]

After:

LED states:                       3     [OVERRIDE | HAS LOCAL CONTROL]
  Local Control function:         ff    [ON]
  Local Control Color:            2     [RED]
> Override Control function:      a     [BLINKING]
  Blink On:Off Duration:          200:100 ms
  Override Control Color:         3     [GREEN]
  Lamp Test Duration:             0 ms

or

LED states:                       7     [LAMPTEST | OVERRIDE | HAS LOCAL CONTROL]
  Local Control function:         ff    [ON]
  Local Control Color:            2     [RED]
> Override Control function:      a     [BLINKING]
  Blink On:Off Duration:          200:100 ms
  Override Control Color:         3     [GREEN]
> Lamp Test Duration:             12700 ms

AlexanderAmelkin · 2021-06-01T19:17:54Z

lib/ipmi_picmg.c

-	if (!(rsp->data[1] & 0x1)) {
-		printf("[NO LOCAL CONTROL]\n");
-		return 0;
+	if (rsp->data_len != 5 && rsp->data_len != 8 && rsp->data_len != 9) {


What are those numbers? Please declare macros or enums for them.

AlexanderAmelkin · 2021-06-01T19:20:03Z

lib/ipmi_picmg.c

@@ -1345,6 +1345,8 @@ ipmi_picmg_get_led_state(struct ipmi_intf * intf, char ** argv)
 {
 	struct ipmi_rs * rsp;
 	struct ipmi_rq req;
+	int has_local = 0, override = 0, lamp_test = 0;
+	int local_blink = 0, override_blink = 0;


if these are boolean, please use bool from #include <stdbool.h>.

AlexanderAmelkin · 2021-06-01T19:20:48Z

lib/ipmi_picmg.c

 	if (rsp->data[2] == 0x0) {
 		printf("[OFF]\n");
 	} else if (rsp->data[2] == 0xff) {
 		printf("[ON]\n");
-	} else {
+	} else if (rsp->data[2] <= 0xfa) {
+		local_blink = 1;


AlexanderAmelkin · 2021-06-01T19:23:04Z

lib/ipmi_picmg.c

+			(rsp->data[3] &&  rsp->data[3] <= 0xfa) ? "" :
+				"[RESERVED]",
+			(rsp->data[3] * 10), (rsp->data[2] * 10));


Oh, so much magic...

AlexanderAmelkin · 2021-06-01T19:28:16Z

lib/ipmi_picmg.c


-	if (rsp->data[1] & 0x2) {
-		printf("|OVERRIDE");
+	if (rsp->data[1] & 0x8) {


While you're at it, could you please replace all the magic (at least in the affected code) with some meaningful macros/enums ? Please do that in a separate commit (first commit to replace magic, second one to actually fix the issue). Can you please do that?

Since I'm new to Git, I'm not sure if I understand you correctly. When you say separate commits, do you mean two pulled branches? Should I create a separated issue/branch to create magic/macros?

No, I mean a single branch (this one, bugfix/297-fix-picmg-led-get) with two separate commits in it on top of ipmitool/master branch. Git is different from subversion and cvs. In those systems you only have one operation, that is 'commit'. In git you have a 'commit' operation that commits an annotated chunk of your changes into your working copy, and then you have a 'push' operation that puts all the changes you have in your working copy into the remote repository (to github). That is what I mean. You replace the magic numbers, then do git add -u; git commit, then you fix the issue and again do git add -u; git commit. Then you do a single git push -f to replace the branch with your new set of changes. Then I can see during the review where you only did cosmetics and where you've actually changed the code.

I hope it's more clear now.

Thanks.

Thank you for the detailed instructions.

However, since I've pushed the changes without magic numbers replaced, can I replace the magic numbers as the second step, and then do "git add -u; git commit; git push -f"?

Or should I delete and recreate the branch?

By the way, could you please point me a file as an example of the coding convention for the magic numbers I should follow?

@PengWang-SMARTM, no, please do not delete the branch, that will ruin the connection to this PR.
If I want to reorder commits I usually do "interactive rebase", but in this particular case I think a non-interactive rebase is the best strategy:

You did your code changes on branch bugfix/297-fix-picmg-led-get

Now you create a new temporary local branch for magic cleanup:
git checkout -b tmpbranch origin/master (that is, the new branch doesn't contain your code changes)

You do you magic cleanup and commit it to tmpbranch:
git add -u; git commit -s

You switch to bugfix/297-fix-picmg-led-get:
git checkout bugfix/297-fix-picmg-led-get

You rebase this branch to your tmpbranch:
git rebase tmpbranch

It will probably stop in the middle of the process complaining about conflicts.
You edit the conflicting files (you can see them with git status), resolve the conflicts, and continue.
Make sure there are no more conflict markers (>>>) in the conflicting files before you do this:
git add -u; git rebase --continue

You forcefully push this branch to github:
git push -f

Voila!

As to conventions regarding magic, it is simple: don't use magic. Use macros, enums, or constants instead of dire numbers. When one uses magic numbers, it is always a hard guess whether this particular '32' is the same '32' as two lines below or are they semantically different.

That's reflected here:
https://github.com/ipmitool/ipmitool/wiki/Coding-Standards#magic-numbers

This project needs a lot of this magic cleanup. That's an enormously huge task. So, to eat that elephant a bite at a time, I ask the contributors to try and eliminate the ancient magic wherever possible with their new contributions.

Yes, replacing all magic numbers is a huge task. I initially didn't plan to touch the code that are not part of this issue. At the same time, I was trying to keep the same coding style at least within a same file. Otherwise, the final code would look weird as only a small portion of the code uses macros while the rest uses magic numbers.

I personally prefer to maintain the coding style consistency over fixing the numbers partially. That is, I wish having a dedicated issue/thread to fix the magic numbers. All other fixes/threads should keep the coding style the same. If the file was using macros, it should keep using macros. However, if the file was using raw numbers, it should keep using raw numbers.

Another argument is, for this project, a lot of magic numbers are from the specification. Anyone who works on the code shall be familiar with the corresponding specifications. Therefore, s/he must know the meaning of those raw numbers. From this point of view, raw numbers are not really a big concern. On the other hand, it may be helpful to mention the chapter of the specification(s) the code is implementing.

I respectfully but strongly disagree here. Magic numbers are NEVER allowed it a good maintainable code, regardless of them being by a specification or not. They ALWAYS MUST be represented as human-readable textual identifiers. There are tons of literature and online discussions on that topic that I don't want to repeat here.

Please, just follow the Coding Style currently in effect for this project and do not use magic numbers ever. I will strictly NOT accept any PRs with magic numbers.

picmg led get: fix the interpretation and format

228ba11

The picmg led get command result misinterpret the override state by looking at data offset 2 rather than 5. The format and interpretation can be improved as well. Signed-off-by: peng.wang@smartm.com <peng.wang@smartm.com>

AlexanderAmelkin suggested changes Jun 1, 2021

View reviewed changes

AlexanderAmelkin reviewed Jun 1, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

picmg led get: fix the interpretation and format #298

picmg led get: fix the interpretation and format #298

PengWang-SMARTM commented May 28, 2021 •

edited

AlexanderAmelkin commented Jun 1, 2021

PengWang-SMARTM commented Jun 1, 2021 •

edited

AlexanderAmelkin Jun 1, 2021

AlexanderAmelkin Jun 1, 2021

AlexanderAmelkin Jun 1, 2021

AlexanderAmelkin Jun 1, 2021

AlexanderAmelkin Jun 1, 2021

PengWang-SMARTM Jun 1, 2021

AlexanderAmelkin Jun 15, 2021

PengWang-SMARTM Jun 15, 2021

PengWang-SMARTM Jun 15, 2021

AlexanderAmelkin Jun 16, 2021

AlexanderAmelkin Jun 16, 2021

AlexanderAmelkin Jun 16, 2021

PengWang-SMARTM Nov 7, 2021

AlexanderAmelkin Mar 9, 2022

picmg led get: fix the interpretation and format #298

Are you sure you want to change the base?

picmg led get: fix the interpretation and format #298

Conversation

PengWang-SMARTM commented May 28, 2021 • edited

AlexanderAmelkin commented Jun 1, 2021

PengWang-SMARTM commented Jun 1, 2021 • edited

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

PengWang-SMARTM commented May 28, 2021 •

edited

PengWang-SMARTM commented Jun 1, 2021 •

edited