#375 Add support for IPv6 LAN Configuration Parameters

[Dmitry Bazhenov]

This patch adds support for IPMI v2.0 IPv6-related LAN configuration parameters.

[Zdenek Styblik]

I did give this very quick look and I didn't like what I saw. Sorry. :-( I'll try again ... later.

[Dmitry Bazhenov]

I found an error in printing DHCP DUID. Here's an updated patch. Looking forward for comments.

[Dmitry Bazhenov]

Hi Zdenek,

This is the most recent, debugged patch for LAN IP6 configuration parameters.
I can't understand why this ticket takes so long to be accepted. It does not break any unrelated functionality and brings new and useful stuff which many would like to have.

Reagrds, Dmitry

[Zdenek Styblik]

Hello Dmitry,

This is the most recent, debugged patch for LAN IP6 configuration parameters.

yet even one year later it still has windows new-lines. Skimming quickly through the patch, I remain unsold on wrapping help text, or text of any kind, with structure. Please, sell it to me as to fellow programmer.

I can't understand why this ticket takes so long to be accepted. It does not break any unrelated functionality and brings new and useful stuff which many would like to have.

Please, help me out here and tell me where to begin to answer your question. It's not like you're new to this project, are you?
I admit there weren't that many commits between now and then, but is it possible other people do something differently to get their stuff in? Just a thought.

Regards,
Z.

[Dmitry Bazhenov]

Zdenek,

I fixed new line endings, my bad.

For the rest, it was my design decision to make parsing of the command line more structured.
I still believe that this decision is better than one used in ipmi_lanp.c, for example.
It seems like it is a matter of taste. I can't waste more time redoing a big piece of code without any reasonable argument. If someone would suggest a better decision and volounteers to rewrite the code or suggests an alternative patch, I won't resist since the main purpose of this patch is to add the needed functionality. The patch adds it, compiles without errors, and works, which is the main idea of the tool.

Regards,
Dmitry

[Dmitry Bazhenov]

Added ticket ID and description.

[Dmitry Bazhenov]

Made a complete rework of the patch.
Implemented a confguration parameter framework which leverages parameter handling and introduces such abilities as saving and loading parameters to/from files (which I find very convenient feature to have). For this purpose has reworked command line parsing (and moved the implementation from src/ipmishell.c to lib/helper.c).

[Dmitry Bazhenov]

Hello, Zdenek,

Now that the release is done, please, consider reviewing the updated patch.

Regards,
Dmitry

[Dmitry Bazhenov]

Latest commits caused a merge conflict. Updated the patch to resolve the conflict.

[Dmitry Bazhenov]

Fix line endings in some files, use unix format.

[Zdenek Styblik]

I did read through the patch, but I don't know what to say.

[Zdenek Styblik]

Let me put it this way. Anyone who's willing to put time and effort to get this in, feel free to ping me. I'm happy to discuss.

[Dmitry Rakhchev]

I have some time to invest into this, however I do not want to waste it, so let's discuss what need to be done first.

I've browsed existing patch and propose the following reworks:

1. Do not touch ipmishell parsing code. It looks possible to get rid of "struct cmdargs", "cmdexp" and "cmdfree".
2. On the other hand the ability to save and load settings looks appeal. I propose the following approach: use "save" command that will write script, that can be piped to ipmishell later for "loading".

What do you think about the possibility to give control over lock/discard/commit to the user? I think normal "set" shall do automatic locking by default, but if a special arg ("nocommit") is given, leave it to the user. This can be useful when you need to update lan settings over lan to prevent loosing the connection to the board after first parameter(s) is updated and comitted.

This way, I expect the following subcommands to the "lan6":

help -- print usage
print -- print parameters
set [nocommit] [<set_sel> [<block_sel>]] <param> <value>-- set parameter
lock,
commit,
discard-- control "Set In Progress"
save -- write a script for ipmishell to set parameters to currently effective values

I appreciate if you can provide some hints on the code style you expect. Normally I try to mimic the style used in the file I'm hacking, but it looks like you are disappointed with the style of original author.

Any other suggestions are welcome as well.

[Zdenek Styblik]

Hi,

I will have to re-read the patch itself, but off-the-bat:

• licence header is missing. Despite BSD is assumed, it should be present in each and every file.
• I strongly disagree with the introduction of str2uchar_hex() and alike. str2uchar() and friends for sure have short-coming of hard-coded base=10. To be true, I know about this limitation for some time. However, I don't think, I disagree, with fix of form "Let's add more functions!". str2* functions should be extended. Unfortunately, this means more work. This MUST be provided as a separate patch.
• mac2str() is introduced. MAC address is being parsed somewhere in the code already. I believe it's in lib/ipmi_lanp.c. So, I don't understand why code isn't being reused. And it's even possible that mac2str() is superior to whatever is already in codebase. In that case, I expect it, and functions alike, to be placed in lib/helper.c(I don't know of better place) and calls being made to such functions from various modules, old code removed. Yes, it means even more work, but less clutter and duplication. This too MUST be provided as a separate patch.
• buf2str() prototype has been changed, but not the function itself? Such change should go in separate patch as well.
• buf2str2() is missing documentation. Also, I'm not sure 2 is really good distinction ... but ok. However, wouldn't it make sense to simply extendbuf2str() instead? Say, if (sep > 0) { rather than introducing new function. Up for discussion/consideration, but doesn't it make sense? On the other hand, it would mean bigger memory footprint.
• Size of those static buffers in buf2str() and buf2str2() should be set through #define, so the size is known outside of those functions. Nice to have.

Actually, let's do these first and then we can talk more. I don't want to read into (this gigantic) patch again right now and then later on, again, because I did couple times before.

About the cmdexp, quick look guess is, it moves original code elsewhere and hopefully makes the whole thing cleaner. I will have to give it some look and read.

I appreciate if you can provide some hints on the code style you expect.

Expected code style is described at https://sourceforge.net/p/ipmitool/wiki/Coding%20Standards/ . It's not perfect, that's for sure. What is written there, however, applies to the rest of the code that haven't been discussed. Also, it's the reason why I don't want to review it now.

Normally I try to mimic the style used in the file I'm hacking, but it looks like you are disappointed with the style of original author.

That's commendable and that's how it should be. However, it's not possible with IPMI tool since each and every part has different style.

Z.

[Dmitry Rakhchev]

Good, I'll try to produce mentioned separate patches first, they make sense even without ipv6.

[Zdenek Styblik]

Cool. btw please, don't worry about code formatting in lanp6 module right now. In my opinion, it would be waste of time right now and you'd be p***ed off in the end if there are any changes required. Just replace what's being changed(as outlined above) and we will get to formatting later.
I hope you haven't done already! :\

Thanks,
Z.

[Dmitry Rakhchev]

Here's what was done:

1. all changes to existing code formatted as separate patches (1-6)
2. patch 2 removes warning related to buf2str argument (uint8_t * vs char *) This is not required for the final target and can be omitted.
3. patch 4 reworks handling of the Kg key. Kg is sequence of bytes, probably with zeroes in the middle, so it is incorrect to use strlen() to determine length. Without this changes -y 010203040500060708090a0b0c0d0e0f1011121314 incorrectly use 010203040500000000000000000000000000000000 as the Kg. This is not required for the final target and can be omitted. Probably make sense to integrate this as a separate bugfix.
4. patch 7 is Dmitry's original code modified. I've removed parser part, since it is possible to re-use 'shell' command to achieve the same result, changed syntax description accordingly and fixed few bugs (at least two were present in original patch). I've tried to approach code style, but did not put much effort into it.

I hope patches 1,3,5,6 cover your tasks above.

Waiting for your comments.

Dmitry

[Zdenek Styblik]

Hello,

sorry, I'm quite busy with life. I did give a look to patches 1-6.

• in 01
  • char * buf2str_extended( -> char *<NEWLINE>buf2str_extended( + don't indent *. This is common for all files and also params/variables.
  • int i, sz, left; should be one per line
  • buf2str_extended should check whether *buf if NULL or not.
• in 03
  • comments are longer than 80chars
• in 04
  • "malloc failure" should be prefixed with ipmitool: to be consistent
• in 06
  • unsigned int m1 = 0; in str2mac() these should be uint8_t since all following checks are for uint8_t

I was a bit confused by 03 and 04 and I was kind of wandering whether it shouldn't be squashed into one commit. But I guess it's ok now(=> don't do it).

Other than that, I don't think I have more comments.

Thanks,
Z.

[Dmitry Rakhchev]

Hello Zdenek,

I hope I have killed all formatting issues now, at least for the 1-6 part.

For 04: I removed "malloc failure" message, since it cannot happen.
For 06:unsigned int vs uint8_t: sscanf argument corresponding to "%02x" shall be pointer to unsigned int. Passing anything else(even uint32_t *) is looking for trouble.

Updated patches attached.

I will be on a vacation without access to computer until mid-August, so do not expect quick response.

Regards, Dmitry

[Zdenek Styblik]

I've merged patches 1-6. Thanks! I believe there was goto out; missing when ipmi_parse_hex() returns (-1). Please, see attached patch.
Anyway, IPv6 patch is next then.

For 06:unsigned int vs uint8_t: sscanf argument corresponding to "%02x" shall be pointer to unsigned int. Passing anything else(even uint32_t *) is looking for trouble.

You're right. Later on, I've realized why those checks are there. It's because ssanf() to unsigned int which is, few lines later, being casted to uint8_t. And we're making sure there is no overflow. :)
It's good not to listen to and accept everything I say. :)

I will be on a vacation without access to computer until mid-August, so do not expect quick response.

Have a nice vacation. I'm quite dead in the water at the moment anyway :(

Z.

[Zdenek Styblik]

Ok, I've read through and here's the list:

• • CFGP_SAVE, / output parameter data in form that + can be parsed back / ~ this comment style seems to be "broken".
• IPMI_CFGP ~ macros shouldn't be used, if possible.
• struct ipmi_cfgp { ~ is using bitfields which are GCC specific
• ctx->v = d->next; free(d); ~ missing d = NULL; and I would say this is a common for all occurrences of free()
• return ret ? -1 : 0; ~ please, can this be changed to if() block?

Other than that, I guess it's fine.

Thanks,
Z.

[Dmitry Rakhchev]

CFGP_SAVE, / output parameter data in form that + can be parsed back / ~ this comment style seems to be "broken".
IPMI_CFGP ~ macros shouldn't be used, if possible.
ctx->v = d->next; free(d); ~ missing d = NULL; and I would say this is a common for all occurrences of free()
return ret ? -1 : 0; ~ please, can this be changed to if() block?
Fixed.

struct ipmi_cfgp { ~ is using bitfields which are GCC specific
Excerpt from pre-C89 draft:

A bit-field may have type int , unsigned int , or signed int .
Whether the high-order bit position of a ``plain'' int bit-field is
treated as a sign bit is implementation-defined.  A bit-field is
interpreted as an integral type consisting of the specified number of
bits.

I assume using unsigned int as bit-field type is strictly conformant.
Another free source that confirms this is C90 Rationale document:

Three types of bit-fields are now defined: plain int calls for implementation-defined signedness (as in K&R), signed int calls for assuredly signed fields, and unsigned int calls for unsigned fields. The old constraints on bit-fields crossing word boundaries have been
relaxed, since so many properties of bit-fields are implementation dependent anyway.

Its using other integer types(not [unsigned | signed] int) as type for bit-field is a gcc extension. It is quite handy in describing IPMI structures, but non-portable.

If you insist I may replace bit-field with unsigned char.

[Zdenek Styblik]

I see. I didn't know bitfields are part of spec. But then, I don't really go for bitfields ... ever :) Cool!

It is quite handy in describing IPMI structures, but non-portable.

Indeed. Although, portability can be achieved at the cost of either helper functions or higher memory consumption. Bitfields aren't completely for free either, although probably really cheap(guessing).

If you insist I may replace bit-field with unsigned char.

Hm? Sorry, but I don't follow. struct in question is conformant, resp. is using unsigned int, doesn't it? Also, I don't insist on that many things. ;)

Z.