[Ipmitool-devel] [PATCH] Fixed bug where PICMG Get Device Locator was never run.

[Dan Gora]

This code was moved to ipmi_main.c from open.c and in the change a bug
was introduced where the PICMG Get Device Locator command would not
be run unless the user explicitly set my_addr to 0 with the -m command.

The more sensible thing to do is to run this unless the user explictly
overrides it with the -m option.

Signed-off-by: Dan Gora <***@adax.com>
---
ipmitool/lib/ipmi_main.c | 14 +++++++++-----
1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/ipmitool/lib/ipmi_main.c b/ipmitool/lib/ipmi_main.c
index ce98ef4..a2f3500 100644
--- a/ipmitool/lib/ipmi_main.c
+++ b/ipmitool/lib/ipmi_main.c
@@ -363,7 +363,7 @@ ipmi_main(int argc, char ** argv,
uint8_t transit_addr = 0;
uint8_t transit_channel = 0;
uint8_t target_lun = 0;
- uint8_t my_addr = 0x20;
+ uint8_t my_addr = 0;
uint16_t my_long_packet_size=0;
uint8_t my_long_packet_set=0;
uint8_t lookupbit = 0x10; /* use name-only lookup by default */
@@ -854,6 +854,9 @@ ipmi_main(int argc, char ** argv,
if (my_addr) {
ipmi_main_intf->my_addr = my_addr;
} else {
+ /* Use the default for the payload source address */
+ my_addr = 0x20;
+
/* Check if PICMG extension is available to use the function
* GetDeviceLocator to retreive i2c address PICMG hack to set
* right IPMB address, If extension is not supported, should
@@ -861,7 +864,6 @@ ipmi_main(int argc, char ** argv,
* PICMG Extension Version 2.0 (PICMG 3.0 Revision 1.0 ATCA) to
* PICMG Extension Version 2.3 (PICMG 3.0 Revision 3.0 ATCA)
*/
-
/* First, check if PICMG extension is available and supported */
struct ipmi_rq req;
struct ipmi_rs *rsp;
@@ -880,10 +882,12 @@ ipmi_main(int argc, char ** argv,
rsp = ipmi_main_intf->sendrecv(ipmi_main_intf, &req);
if (rsp && !rsp->ccode) {
if ( (rsp->data[0] == 0) &&
- ((rsp->data[1] & 0x0F) == PICMG_ATCA_MAJOR_VERSION) ) {
+ ((rsp->data[1] & 0x0F) == PICMG_ATCA_MAJOR_VERSION)){
version_accepted = 1;
- lprintf(LOG_INFO, "Discovered PICMG Extension %d.%d",
- (rsp->data[1] & 0x0f), (rsp->data[1] >> 4));
+ lprintf(LOG_INFO,
+ "Discovered PICMG Extension %d.%d",
+ (rsp->data[1] & 0x0f),
+ (rsp->data[1] >> 4));
}
}

[Zdenek Styblik]

Post by Dan Gora
This code was moved to ipmi_main.c from open.c and in the change a bug
was introduced where the PICMG Get Device Locator command would not
be run unless the user explicitly set my_addr to 0 with the -m command.
The more sensible thing to do is to run this unless the user explictly
overrides it with the -m option.

I'll leave this one to Jim, or somebody else to decide, since this
could be connected to IPMI bridging.

However, I NACK part 3 and 4 of this patch. Ok, 3rd one deletes empty
line. The 4th one, I really don't get what kind of issue you had with
formatting there, but it looks ok to me as it is now.

Z.

Post by Dan Gora
---
ipmitool/lib/ipmi_main.c | 14 +++++++++-----
1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/ipmitool/lib/ipmi_main.c b/ipmitool/lib/ipmi_main.c
index ce98ef4..a2f3500 100644
--- a/ipmitool/lib/ipmi_main.c
+++ b/ipmitool/lib/ipmi_main.c
@@ -363,7 +363,7 @@ ipmi_main(int argc, char ** argv,
uint8_t transit_addr = 0;
uint8_t transit_channel = 0;
uint8_t target_lun = 0;
- uint8_t my_addr = 0x20;
+ uint8_t my_addr = 0;
uint16_t my_long_packet_size=0;
uint8_t my_long_packet_set=0;
uint8_t lookupbit = 0x10; /* use name-only lookup by default */
@@ -854,6 +854,9 @@ ipmi_main(int argc, char ** argv,
if (my_addr) {
ipmi_main_intf->my_addr = my_addr;
} else {
+ /* Use the default for the payload source address */
+ my_addr = 0x20;
+
/* Check if PICMG extension is available to use the function
* GetDeviceLocator to retreive i2c address PICMG hack to set
* right IPMB address, If extension is not supported, should
@@ -861,7 +864,6 @@ ipmi_main(int argc, char ** argv,
* PICMG Extension Version 2.0 (PICMG 3.0 Revision 1.0 ATCA) to
* PICMG Extension Version 2.3 (PICMG 3.0 Revision 3.0 ATCA)
*/
-
/* First, check if PICMG extension is available and supported */
struct ipmi_rq req;
struct ipmi_rs *rsp;
@@ -880,10 +882,12 @@ ipmi_main(int argc, char ** argv,
rsp = ipmi_main_intf->sendrecv(ipmi_main_intf, &req);
if (rsp && !rsp->ccode) {
if ( (rsp->data[0] == 0) &&
- ((rsp->data[1] & 0x0F) == PICMG_ATCA_MAJOR_VERSION) ) {
+ ((rsp->data[1] & 0x0F) == PICMG_ATCA_MAJOR_VERSION)){
version_accepted = 1;
- lprintf(LOG_INFO, "Discovered PICMG Extension %d.%d",
- (rsp->data[1] & 0x0f), (rsp->data[1] >> 4));
+ lprintf(LOG_INFO,
+ "Discovered PICMG Extension %d.%d",
+ (rsp->data[1] & 0x0f),
+ (rsp->data[1] >> 4));
}
}
--
1.7.7
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
http://p.sf.net/sfu/appdyn_d2d_mar
_______________________________________________
Ipmitool-devel mailing list
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel

[Jim Mankovich]

Z,

I've been working on ipmitool bridging issues and ran across the same issue
as Dan, so his fix to always run the PICMG discovery code whenever -m is
not specified is just fine with me.

Post by Zdenek Styblik

Post by Dan Gora
This code was moved to ipmi_main.c from open.c and in the change a bug
was introduced where the PICMG Get Device Locator command would not
be run unless the user explicitly set my_addr to 0 with the -m command.
The more sensible thing to do is to run this unless the user explictly
overrides it with the -m option.

I'll leave this one to Jim, or somebody else to decide, since this
could be connected to IPMI bridging.
However, I NACK part 3 and 4 of this patch. Ok, 3rd one deletes empty
line. The 4th one, I really don't get what kind of issue you had with
formatting there, but it looks ok to me as it is now.
Z.

Post by Dan Gora
---
ipmitool/lib/ipmi_main.c | 14 +++++++++-----
1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/ipmitool/lib/ipmi_main.c b/ipmitool/lib/ipmi_main.c
index ce98ef4..a2f3500 100644
--- a/ipmitool/lib/ipmi_main.c
+++ b/ipmitool/lib/ipmi_main.c
@@ -363,7 +363,7 @@ ipmi_main(int argc, char ** argv,
uint8_t transit_addr = 0;
uint8_t transit_channel = 0;
uint8_t target_lun = 0;
- uint8_t my_addr = 0x20;
+ uint8_t my_addr = 0;
uint16_t my_long_packet_size=0;
uint8_t my_long_packet_set=0;
uint8_t lookupbit = 0x10; /* use name-only lookup by default */
@@ -854,6 +854,9 @@ ipmi_main(int argc, char ** argv,
if (my_addr) {
ipmi_main_intf->my_addr = my_addr;
} else {
+ /* Use the default for the payload source address */
+ my_addr = 0x20;
+
/* Check if PICMG extension is available to use the function
* GetDeviceLocator to retreive i2c address PICMG hack to set
* right IPMB address, If extension is not supported, should
@@ -861,7 +864,6 @@ ipmi_main(int argc, char ** argv,
* PICMG Extension Version 2.0 (PICMG 3.0 Revision 1.0 ATCA) to
* PICMG Extension Version 2.3 (PICMG 3.0 Revision 3.0 ATCA)
*/
-
/* First, check if PICMG extension is available and supported */
struct ipmi_rq req;
struct ipmi_rs *rsp;
@@ -880,10 +882,12 @@ ipmi_main(int argc, char ** argv,
rsp = ipmi_main_intf->sendrecv(ipmi_main_intf, &req);
if (rsp && !rsp->ccode) {
if ( (rsp->data[0] == 0) &&
- ((rsp->data[1] & 0x0F) == PICMG_ATCA_MAJOR_VERSION) ) {
+ ((rsp->data[1] & 0x0F) == PICMG_ATCA_MAJOR_VERSION)){
version_accepted = 1;
- lprintf(LOG_INFO, "Discovered PICMG Extension %d.%d",
- (rsp->data[1] & 0x0f), (rsp->data[1] >> 4));
+ lprintf(LOG_INFO,
+ "Discovered PICMG Extension %d.%d",
+ (rsp->data[1] & 0x0f),
+ (rsp->data[1] >> 4));
}
}
--
1.7.7
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
http://p.sf.net/sfu/appdyn_d2d_mar
_______________________________________________
Ipmitool-devel mailing list
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel

[Dan Gora]

On Thu, Apr 4, 2013 at 10:35 AM, Zdenek Styblik

Post by Zdenek Styblik

Post by Dan Gora
This code was moved to ipmi_main.c from open.c and in the change a bug
was introduced where the PICMG Get Device Locator command would not
be run unless the user explicitly set my_addr to 0 with the -m command.
The more sensible thing to do is to run this unless the user explictly
overrides it with the -m option.

I'll leave this one to Jim, or somebody else to decide, since this
could be connected to IPMI bridging.
However, I NACK part 3 and 4 of this patch. Ok, 3rd one deletes empty
line. The 4th one, I really don't get what kind of issue you had with
formatting there, but it looks ok to me as it is now.

So that the lines don't wrap around 80 columns... I'm trying to
improve the code formatting of things a bit as it's a real hodge-podge
of styles. ipmi_ekanalyzer.c is a total mess, but I can see that it
was just lifted whole-hog from ipmiutils.

That said it seems kind of silly to make me have to redo this whole
patch because of one removed line (ie NACKing part 3 (I assume you
mean chunk 3 of the patch, right?)

thanks
dan

Post by Zdenek Styblik
Z.

Post by Dan Gora
---
ipmitool/lib/ipmi_main.c | 14 +++++++++-----
1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/ipmitool/lib/ipmi_main.c b/ipmitool/lib/ipmi_main.c
index ce98ef4..a2f3500 100644
--- a/ipmitool/lib/ipmi_main.c
+++ b/ipmitool/lib/ipmi_main.c
@@ -363,7 +363,7 @@ ipmi_main(int argc, char ** argv,
uint8_t transit_addr = 0;
uint8_t transit_channel = 0;
uint8_t target_lun = 0;
- uint8_t my_addr = 0x20;
+ uint8_t my_addr = 0;
uint16_t my_long_packet_size=0;
uint8_t my_long_packet_set=0;
uint8_t lookupbit = 0x10; /* use name-only lookup by default */
@@ -854,6 +854,9 @@ ipmi_main(int argc, char ** argv,
if (my_addr) {
ipmi_main_intf->my_addr = my_addr;
} else {
+ /* Use the default for the payload source address */
+ my_addr = 0x20;
+
/* Check if PICMG extension is available to use the function
* GetDeviceLocator to retreive i2c address PICMG hack to set
* right IPMB address, If extension is not supported, should
@@ -861,7 +864,6 @@ ipmi_main(int argc, char ** argv,
* PICMG Extension Version 2.0 (PICMG 3.0 Revision 1.0 ATCA) to
* PICMG Extension Version 2.3 (PICMG 3.0 Revision 3.0 ATCA)
*/
-
/* First, check if PICMG extension is available and supported */
struct ipmi_rq req;
struct ipmi_rs *rsp;
@@ -880,10 +882,12 @@ ipmi_main(int argc, char ** argv,
rsp = ipmi_main_intf->sendrecv(ipmi_main_intf, &req);
if (rsp && !rsp->ccode) {
if ( (rsp->data[0] == 0) &&
- ((rsp->data[1] & 0x0F) == PICMG_ATCA_MAJOR_VERSION) ) {
+ ((rsp->data[1] & 0x0F) == PICMG_ATCA_MAJOR_VERSION)){
version_accepted = 1;
- lprintf(LOG_INFO, "Discovered PICMG Extension %d.%d",
- (rsp->data[1] & 0x0f), (rsp->data[1] >> 4));
+ lprintf(LOG_INFO,
+ "Discovered PICMG Extension %d.%d",
+ (rsp->data[1] & 0x0f),
+ (rsp->data[1] >> 4));
}
}
--
1.7.7

[Zdenek Styblik]

Post by Dan Gora
On Thu, Apr 4, 2013 at 10:35 AM, Zdenek Styblik

Post by Zdenek Styblik

Post by Dan Gora
This code was moved to ipmi_main.c from open.c and in the change a bug
was introduced where the PICMG Get Device Locator command would not
be run unless the user explicitly set my_addr to 0 with the -m command.
The more sensible thing to do is to run this unless the user explictly
overrides it with the -m option.

I'll leave this one to Jim, or somebody else to decide, since this
could be connected to IPMI bridging.
However, I NACK part 3 and 4 of this patch. Ok, 3rd one deletes empty
line. The 4th one, I really don't get what kind of issue you had with
formatting there, but it looks ok to me as it is now.

So that the lines don't wrap around 80 columns... I'm trying to
improve the code formatting of things a bit as it's a real hodge-podge
of styles. ipmi_ekanalyzer.c is a total mess, but I can see that it
was just lifted whole-hog from ipmiutils.

Yes, I've guessed what it was about. Part/chunk 3 was alright-ish.
Part/chunk 4 does nothing what you say. These lines end at 61/66 thus
I see no reason why they should be split up. Or am I missing
something?

Post by Dan Gora
That said it seems kind of silly to make me have to redo this whole
patch because of one removed line (ie NACKing part 3 (I assume you
mean chunk 3 of the patch, right?)

You don't have to re-do anything. I guess anybody who's going to
commit that can handle text editor and "delete" key ;)

Thanks,
Z.

Post by Dan Gora
thanks
dan

Post by Zdenek Styblik
Z.

Post by Dan Gora
---
ipmitool/lib/ipmi_main.c | 14 +++++++++-----
1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/ipmitool/lib/ipmi_main.c b/ipmitool/lib/ipmi_main.c
index ce98ef4..a2f3500 100644
--- a/ipmitool/lib/ipmi_main.c
+++ b/ipmitool/lib/ipmi_main.c
@@ -363,7 +363,7 @@ ipmi_main(int argc, char ** argv,
uint8_t transit_addr = 0;
uint8_t transit_channel = 0;
uint8_t target_lun = 0;
- uint8_t my_addr = 0x20;
+ uint8_t my_addr = 0;
uint16_t my_long_packet_size=0;
uint8_t my_long_packet_set=0;
uint8_t lookupbit = 0x10; /* use name-only lookup by default */
@@ -854,6 +854,9 @@ ipmi_main(int argc, char ** argv,
if (my_addr) {
ipmi_main_intf->my_addr = my_addr;
} else {
+ /* Use the default for the payload source address */
+ my_addr = 0x20;
+
/* Check if PICMG extension is available to use the function
* GetDeviceLocator to retreive i2c address PICMG hack to set
* right IPMB address, If extension is not supported, should
@@ -861,7 +864,6 @@ ipmi_main(int argc, char ** argv,
* PICMG Extension Version 2.0 (PICMG 3.0 Revision 1.0 ATCA) to
* PICMG Extension Version 2.3 (PICMG 3.0 Revision 3.0 ATCA)
*/
-
/* First, check if PICMG extension is available and supported */
struct ipmi_rq req;
struct ipmi_rs *rsp;
@@ -880,10 +882,12 @@ ipmi_main(int argc, char ** argv,
rsp = ipmi_main_intf->sendrecv(ipmi_main_intf, &req);
if (rsp && !rsp->ccode) {
if ( (rsp->data[0] == 0) &&
- ((rsp->data[1] & 0x0F) == PICMG_ATCA_MAJOR_VERSION) ) {
+ ((rsp->data[1] & 0x0F) == PICMG_ATCA_MAJOR_VERSION)){
version_accepted = 1;
- lprintf(LOG_INFO, "Discovered PICMG Extension %d.%d",
- (rsp->data[1] & 0x0f), (rsp->data[1] >> 4));
+ lprintf(LOG_INFO,
+ "Discovered PICMG Extension %d.%d",
+ (rsp->data[1] & 0x0f),
+ (rsp->data[1] >> 4));
}
}
--
1.7.7

[Dan Gora]

Post by Zdenek Styblik

Post by Dan Gora
So that the lines don't wrap around 80 columns... I'm trying to
improve the code formatting of things a bit as it's a real hodge-podge
of styles. ipmi_ekanalyzer.c is a total mess, but I can see that it
was just lifted whole-hog from ipmiutils.

Yes, I've guessed what it was about. Part/chunk 3 was alright-ish.
Part/chunk 4 does nothing what you say. These lines end at 61/66 thus
I see no reason why they should be split up. Or am I missing
something?

You must not be using 8 space tabstops then, because with 8 space
tabstops they did in fact wrap. With chunk 4 applied, the line:

((rsp->data[1] & 0x0F) == PICMG_ATCA_MAJOR_VERSION)){

has the closing brace exactly on column 80.

I attached screenshots of what it looks like before and after so that
you can see what I'm talking about.

[Zdenek Styblik]

Post by Dan Gora

Post by Zdenek Styblik

Post by Dan Gora
So that the lines don't wrap around 80 columns... I'm trying to
improve the code formatting of things a bit as it's a real hodge-podge
of styles. ipmi_ekanalyzer.c is a total mess, but I can see that it
was just lifted whole-hog from ipmiutils.

Yes, I've guessed what it was about. Part/chunk 3 was alright-ish.
Part/chunk 4 does nothing what you say. These lines end at 61/66 thus
I see no reason why they should be split up. Or am I missing
something?

You must not be using 8 space tabstops then, because with 8 space

Yes, I'm using 2, and? Is there some standard for tab stop width? For
example Python is using 4 white spaces as tabs, just saying.
And is that supposed to be an argument for doing ') {' -> '){'? If
this is supposed to be "better code" formatting, then imo it isn't.
Sorry.

Z.

Post by Dan Gora
((rsp->data[1] & 0x0F) == PICMG_ATCA_MAJOR_VERSION)){
has the closing brace exactly on column 80.
I attached screenshots of what it looks like before and after so that
you can see what I'm talking about.

[Dan Gora]

Post by Zdenek Styblik

Post by Dan Gora

Post by Zdenek Styblik

Post by Dan Gora
So that the lines don't wrap around 80 columns... I'm trying to
improve the code formatting of things a bit as it's a real hodge-podge
of styles. ipmi_ekanalyzer.c is a total mess, but I can see that it
was just lifted whole-hog from ipmiutils.

Yes, I've guessed what it was about. Part/chunk 3 was alright-ish.
Part/chunk 4 does nothing what you say. These lines end at 61/66 thus
I see no reason why they should be split up. Or am I missing
something?

You must not be using 8 space tabstops then, because with 8 space

Yes, I'm using 2, and? Is there some standard for tab stop width?

yeah.. 8 spaces. It's been that forever as far as I know..

I try to follow this more or less:

https://www.kernel.org/doc/Documentation/CodingStyle

Post by Zdenek Styblik
For
example Python is using 4 white spaces as tabs, just saying.

I don't know.. I've never used python...

Post by Zdenek Styblik
And is that supposed to be an argument for doing ') {' -> '){'? If

No, it's an argument for doing the opposite, to add a space after the
closing parathesis and before the opening bracket so that it's easier
to see.

Post by Zdenek Styblik
this is supposed to be "better code" formatting, then imo it isn't.

Well at least it's consistent, which the current code base is anything but.

thanks
dan

[Jim Mankovich]

I would advocate always using tab for indentation. Then you can change it
to be any number of spaces in your editor. This is what I generally do, and for
many of the files in ipmitool I set my editor to have a tab stop of 4 so that
the lines don't wrap.

Post by Zdenek Styblik

Post by Dan Gora

Post by Zdenek Styblik

Post by Dan Gora
So that the lines don't wrap around 80 columns... I'm trying to
improve the code formatting of things a bit as it's a real hodge-podge
of styles. ipmi_ekanalyzer.c is a total mess, but I can see that it
was just lifted whole-hog from ipmiutils.

Yes, I've guessed what it was about. Part/chunk 3 was alright-ish.
Part/chunk 4 does nothing what you say. These lines end at 61/66 thus
I see no reason why they should be split up. Or am I missing
something?

You must not be using 8 space tabstops then, because with 8 space

Yes, I'm using 2, and? Is there some standard for tab stop width? For
example Python is using 4 white spaces as tabs, just saying.
And is that supposed to be an argument for doing ') {' -> '){'? If
this is supposed to be "better code" formatting, then imo it isn't.
Sorry.
Z.

Post by Dan Gora
((rsp->data[1] & 0x0F) == PICMG_ATCA_MAJOR_VERSION)){
has the closing brace exactly on column 80.
I attached screenshots of what it looks like before and after so that
you can see what I'm talking about.

[Dan Gora]

Post by Jim Mankovich
I would advocate always using tab for indentation. Then you can change it
to be any number of spaces in your editor. This is what I generally do, and for
many of the files in ipmitool I set my editor to have a tab stop of 4 so that
the lines don't wrap.

yes I do use tabs for indentation, the problem is that other people
have to read your code too and have no idea how many spaces that you
use for tabstops. So code that looks perfectly reasonable with 4
space tabstops looks like an utter mess with 8.

There is no reason to use anything other than 8 for tabstops. This is
what most sane people expect and what most editors default to.

If ipmitool had a stated, documented, consistent coding style, I would
use it. However it just has a mess right now, so I'm using a known,
documented, widely used style that's used in many many other open
source projects.

thanks
d

[Zdenek Styblik]

Post by Dan Gora

Post by Jim Mankovich
I would advocate always using tab for indentation. Then you can change it
to be any number of spaces in your editor. This is what I generally do, and for
many of the files in ipmitool I set my editor to have a tab stop of 4 so that
the lines don't wrap.

yes I do use tabs for indentation, the problem is that other people
have to read your code too and have no idea how many spaces that you
use for tabstops. So code that looks perfectly reasonable with 4
space tabstops looks like an utter mess with 8.

What? How's that relevant what number we use for tab stops? It's
tabulator, not a bunch of white spaces.

Z.

[Dan Gora]

Post by Zdenek Styblik

Post by Dan Gora

Post by Jim Mankovich
I would advocate always using tab for indentation. Then you can change it
to be any number of spaces in your editor. This is what I generally do, and for
many of the files in ipmitool I set my editor to have a tab stop of 4 so that
the lines don't wrap.

yes I do use tabs for indentation, the problem is that other people
have to read your code too and have no idea how many spaces that you
use for tabstops. So code that looks perfectly reasonable with 4
space tabstops looks like an utter mess with 8.

What? How's that relevant what number we use for tab stops? It's
tabulator, not a bunch of white spaces.

I don't understand what you are saying....

If you write code which goes up to column 80 with tabstops set to 4,
then type ':set ts=8' in vi, you'll see that the lines will all wrap,
right? It turns it into an utter mess.

How is a random person going to know how many spaces you use for
tabstops that you write the code with?

That's why you use the standard number: 8.


d

[Zdenek Styblik]

On Thu, Apr 4, 2013 at 10:00 PM, Dan Gora <***@adax.com> wrote:
[...]

Post by Dan Gora

Post by Zdenek Styblik
What? How's that relevant what number we use for tab stops? It's
tabulator, not a bunch of white spaces.

I don't understand what you are saying....
If you write code which goes up to column 80 with tabstops set to 4,
then type ':set ts=8' in vi, you'll see that the lines will all wrap,
right? It turns it into an utter mess.

Nope, it doesn't and I have ':set tw=80' on.

Post by Dan Gora
How is a random person going to know how many spaces you use for
tabstops that you write the code with?

Again, we're not using spaces for indentation. Although you can set %
vi; to auto-magically convert tabs into spaces. So when you press
tabulator key it inserts white spaces. We're not using it.

``Now, from this point on, things get a little bit confusing.''

Z.

[Dan Gora]

Post by Zdenek Styblik
Again, we're not using spaces for indentation. Although you can set %
vi; to auto-magically convert tabs into spaces. So when you press
tabulator key it inserts white spaces. We're not using it.

Again, I have no idea what you are talking about trying to convert
tabs to spaces.

I showed you what the code looks like with sensible defaults in a text
editor. I humbly suggest that the "after" case is much easier to
read.

thanks
dan

[Zdenek Styblik]

On Thu, Apr 4, 2013 at 10:07 PM, Zdenek Styblik

Post by Zdenek Styblik
[...]

Post by Dan Gora

Post by Zdenek Styblik
What? How's that relevant what number we use for tab stops? It's
tabulator, not a bunch of white spaces.

I don't understand what you are saying....
If you write code which goes up to column 80 with tabstops set to 4,
then type ':set ts=8' in vi, you'll see that the lines will all wrap,
right? It turns it into an utter mess.

Nope, it doesn't and I have ':set tw=80' on.

And if you meant that line just ... err ... "overflows" to the next
line, resp. new "virtual" line is created. Yeah, sure. So?
Perhaps it would be better to use white spaces for indentation after
all. At least there wouldn't be need for such discussions(1 hour gone,
FYI).

Peace,
Z.

Post by Zdenek Styblik

Post by Dan Gora
How is a random person going to know how many spaces you use for
tabstops that you write the code with?

Again, we're not using spaces for indentation. Although you can set %
vi; to auto-magically convert tabs into spaces. So when you press
tabulator key it inserts white spaces. We're not using it.
``Now, from this point on, things get a little bit confusing.''
Z.

[Dan Gora]

Post by Zdenek Styblik
On Thu, Apr 4, 2013 at 10:07 PM, Zdenek Styblik

Post by Zdenek Styblik
[...]

Post by Dan Gora

Post by Zdenek Styblik
What? How's that relevant what number we use for tab stops? It's
tabulator, not a bunch of white spaces.

I don't understand what you are saying....
If you write code which goes up to column 80 with tabstops set to 4,
then type ':set ts=8' in vi, you'll see that the lines will all wrap,
right? It turns it into an utter mess.

Nope, it doesn't and I have ':set tw=80' on.

And if you meant that line just ... err ... "overflows" to the next
line, resp. new "virtual" line is created. Yeah, sure. So?

So? It's very, very hard to read!

Post by Zdenek Styblik
Perhaps it would be better to use white spaces for indentation after
all. At least there wouldn't be need for such discussions(1 hour gone,
FYI).

No, it would not.. Just use 8 chars for tabstops like everyone else!

If it's no big deal then just accept the patch!

thanks
dan

[Zdenek Styblik]

Post by Dan Gora

Post by Zdenek Styblik
On Thu, Apr 4, 2013 at 10:07 PM, Zdenek Styblik

Post by Zdenek Styblik
[...]

Post by Dan Gora

Post by Zdenek Styblik
What? How's that relevant what number we use for tab stops? It's
tabulator, not a bunch of white spaces.

I don't understand what you are saying....
If you write code which goes up to column 80 with tabstops set to 4,
then type ':set ts=8' in vi, you'll see that the lines will all wrap,
right? It turns it into an utter mess.

Nope, it doesn't and I have ':set tw=80' on.

And if you meant that line just ... err ... "overflows" to the next
line, resp. new "virtual" line is created. Yeah, sure. So?

So? It's very, very hard to read!

Post by Zdenek Styblik
Perhaps it would be better to use white spaces for indentation after
all. At least there wouldn't be need for such discussions(1 hour gone,
FYI).

No, it would not.. Just use 8 chars for tabstops like everyone else!

And why is that? Why white spaces instead tabs wouldn't fix the issue
we're having here?

Post by Dan Gora
If it's no big deal then just accept the patch!

Do you know what the big deal is? Sacrificing readability of the code
for 80 chars width. That's kind of big deal, at least for me. I don't
consider it to be better formatting, nor better readable, and not
consistent. Because that space should be there ') {'.
I said which parts of the patch I'm going to commit. For those "code
formatting" changes, I'm afraid you'll have to get somebody else.
Sorry.

Z.

[Dan Gora]

Post by Zdenek Styblik

Post by Dan Gora

Post by Zdenek Styblik
On Thu, Apr 4, 2013 at 10:07 PM, Zdenek Styblik

Post by Zdenek Styblik
[...]

Post by Dan Gora

Post by Zdenek Styblik
What? How's that relevant what number we use for tab stops? It's
tabulator, not a bunch of white spaces.

I don't understand what you are saying....
If you write code which goes up to column 80 with tabstops set to 4,
then type ':set ts=8' in vi, you'll see that the lines will all wrap,
right? It turns it into an utter mess.

Nope, it doesn't and I have ':set tw=80' on.

And if you meant that line just ... err ... "overflows" to the next
line, resp. new "virtual" line is created. Yeah, sure. So?

So? It's very, very hard to read!

Post by Zdenek Styblik
Perhaps it would be better to use white spaces for indentation after
all. At least there wouldn't be need for such discussions(1 hour gone,
FYI).

No, it would not.. Just use 8 chars for tabstops like everyone else!

And why is that? Why white spaces instead tabs wouldn't fix the issue
we're having here?

ugh... have you ever seen or changed code which has spaces as
indentation before?

It's a massive pain in the butt hitting the space bar 24 times to get
3 levels of indendation.

Post by Zdenek Styblik
Do you know what the big deal is? Sacrificing readability of the code
for 80 chars width. That's kind of big deal, at least for me. I don't

Again, it's much MORE readable now. How is it less readable? Even
with 2 space tabs!

Post by Zdenek Styblik
consider it to be better formatting, nor better readable, and not
consistent. Because that space should be there ') {'.

What? I added the space, not removed it...

Post by Zdenek Styblik
I said which parts of the patch I'm going to commit. For those "code
formatting" changes, I'm afraid you'll have to get somebody else.
Sorry.

Ok, whatever.. If every patch is going to degrade into a code
formatting argument that most people had back in the early 1990's,
then fine. I'm just going to give up then. Let me know when you guys
come up with a documented code formatting convention.

thanks
dan

[Dan Gora]

Post by Zdenek Styblik
consider it to be better formatting, nor better readable, and not
consistent. Because that space should be there ') {'.

And really, if you going to argue with me about joining the closing
parenthesis and an opening bracket so that it will all stay on one
line, you should go through and fix the other 395 instances of it.
It's not like this is the first time that this has appeared in your
otherwise pristine code...

dg:speedy:ipmitool(master) => find . |xargs egrep '\){'|wc
395 2305 26404

dan


--
Dan Gora
Software Engineer

Adax, Inc.
Av Dona Maria Alves, 1070 Casa 5
Centro
Ubatuba, SP
CEP 11680-000
Brasil

Tel: [phone] (Brazil and outside of US)
: [phone] (Inside of US)
: dan_gora (Skype)

email: ***@adax.com

[Jim Mankovich]

I don't have any problem with the code formatting. I personally
leave code formatting alone in ipmitool for any code I don't explicitly
need to make a functional change to.

Post by Dan Gora

Post by Zdenek Styblik

Post by Dan Gora

Post by Zdenek Styblik
On Thu, Apr 4, 2013 at 10:07 PM, Zdenek Styblik

Post by Zdenek Styblik
[...]

Post by Dan Gora

Post by Zdenek Styblik
What? How's that relevant what number we use for tab stops? It's
tabulator, not a bunch of white spaces.

I don't understand what you are saying....
If you write code which goes up to column 80 with tabstops set to 4,
then type ':set ts=8' in vi, you'll see that the lines will all wrap,
right? It turns it into an utter mess.

Nope, it doesn't and I have ':set tw=80' on.

And if you meant that line just ... err ... "overflows" to the next
line, resp. new "virtual" line is created. Yeah, sure. So?

So? It's very, very hard to read!

Post by Zdenek Styblik
Perhaps it would be better to use white spaces for indentation after
all. At least there wouldn't be need for such discussions(1 hour gone,
FYI).

No, it would not.. Just use 8 chars for tabstops like everyone else!

And why is that? Why white spaces instead tabs wouldn't fix the issue
we're having here?

ugh... have you ever seen or changed code which has spaces as
indentation before?
It's a massive pain in the butt hitting the space bar 24 times to get
3 levels of indendation.

Post by Zdenek Styblik
Do you know what the big deal is? Sacrificing readability of the code
for 80 chars width. That's kind of big deal, at least for me. I don't

Again, it's much MORE readable now. How is it less readable? Even
with 2 space tabs!

Post by Zdenek Styblik
consider it to be better formatting, nor better readable, and not
consistent. Because that space should be there ') {'.

What? I added the space, not removed it...

Post by Zdenek Styblik
I said which parts of the patch I'm going to commit. For those "code
formatting" changes, I'm afraid you'll have to get somebody else.
Sorry.

Ok, whatever.. If every patch is going to degrade into a code
formatting argument that most people had back in the early 1990's,
then fine. I'm just going to give up then. Let me know when you guys
come up with a documented code formatting convention.
thanks
dan

[Dan Gora]

Post by Jim Mankovich
I don't have any problem with the code formatting. I personally
leave code formatting alone in ipmitool for any code I don't explicitly
need to make a functional change to.

I do too.. really... I do have better things to do with my time!
However I cannot change code that I cannot read, and I cannot read
code which wraps over 80 columns all the time.

No, I'm not hard-core about it. Of course there are exceptions.
However if we cannot all agree on simple things like "use 8 char
tabstops" and "_try_ to limit lines to 80 chars", then there is no
hope at all. That's why I try and follow the linux kernel coding
style.

I thought that the two screen shots showed my point pretty clearly. I
cannot for the life of me understand how someone would think that the
'before' case is somehow easier to read.

Really I don't think that patches should be disqualified for coding
style unless there is something really egregious, especially when
there is no documented coding style and the current code is pretty
clearly a mash of many different ones.

thanks
dan

[Jim Mankovich]

Dan,

My intent was to say that I did not have any problem with the
change you made to re-format the code. I think following the
linux kernel coding style is the right way to go.

Post by Dan Gora

Post by Jim Mankovich
I don't have any problem with the code formatting. I personally
leave code formatting alone in ipmitool for any code I don't explicitly
need to make a functional change to.

I do too.. really... I do have better things to do with my time!
However I cannot change code that I cannot read, and I cannot read
code which wraps over 80 columns all the time.
No, I'm not hard-core about it. Of course there are exceptions.
However if we cannot all agree on simple things like "use 8 char
tabstops" and "_try_ to limit lines to 80 chars", then there is no
hope at all. That's why I try and follow the linux kernel coding
style.
I thought that the two screen shots showed my point pretty clearly. I
cannot for the life of me understand how someone would think that the
'before' case is somehow easier to read.
Really I don't think that patches should be disqualified for coding
style unless there is something really egregious, especially when
there is no documented coding style and the current code is pretty
clearly a mash of many different ones.
thanks
dan