* [PATCH phosphor-host-ipmid] ipmi daemon return code modification
@ 2016-03-08 20:10 OpenBMC Patches
2016-03-08 20:10 ` OpenBMC Patches
0 siblings, 1 reply; 6+ messages in thread
From: OpenBMC Patches @ 2016-03-08 20:10 UTC (permalink / raw)
To: openbmc
1) Modified the return code for error ipmi rc's
2) Added Channel number 8 support in Channel Info command
https://github.com/openbmc/phosphor-host-ipmid/pull/74
tomjose (1):
ipmi daemon return code modification
apphandler.C | 2 +-
ipmid.C | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)
--
2.7.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH phosphor-host-ipmid] ipmi daemon return code modification
2016-03-08 20:10 [PATCH phosphor-host-ipmid] ipmi daemon return code modification OpenBMC Patches
@ 2016-03-08 20:10 ` OpenBMC Patches
2016-03-09 2:31 ` Cyril Bur
0 siblings, 1 reply; 6+ messages in thread
From: OpenBMC Patches @ 2016-03-08 20:10 UTC (permalink / raw)
To: openbmc; +Cc: tomjose
From: tomjose <tomjoseph@in.ibm.com>
---
apphandler.C | 2 +-
ipmid.C | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/apphandler.C b/apphandler.C
index a921643..fc6c811 100644
--- a/apphandler.C
+++ b/apphandler.C
@@ -358,7 +358,7 @@ ipmi_ret_t ipmi_app_channel_info(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
printf("IPMI APP GET CHANNEL INFO\n");
// I"m only supporting channel 1. 0xE is the 'default channel'
- if (*p == 0xe || *p == 1) {
+ if (*p == 0xe || *p == 1 || *p == 8) {
*data_len = sizeof(resp);
memcpy(response, resp, *data_len);
diff --git a/ipmid.C b/ipmid.C
index 6726a27..728ba0b 100644
--- a/ipmid.C
+++ b/ipmid.C
@@ -267,7 +267,6 @@ static int handle_ipmi_command(sd_bus_message *m, void *user_data, sd_bus_error
if(r != 0)
{
fprintf(stderr,"ERROR:[0x%X] handling NetFn:[0x%X], Cmd:[0x%X]\n",r, netfn, cmd);
- return -1;
}
fprintf(ipmiio, "IPMI Response:\n");
--
2.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH phosphor-host-ipmid] ipmi daemon return code modification
2016-03-08 20:10 ` OpenBMC Patches
@ 2016-03-09 2:31 ` Cyril Bur
2016-03-09 17:01 ` Patrick Williams
0 siblings, 1 reply; 6+ messages in thread
From: Cyril Bur @ 2016-03-09 2:31 UTC (permalink / raw)
To: OpenBMC Patches; +Cc: openbmc, tomjose
On Tue, 8 Mar 2016 14:10:33 -0600
OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:
> From: tomjose <tomjoseph@in.ibm.com>
>
> ---
> apphandler.C | 2 +-
> ipmid.C | 1 -
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/apphandler.C b/apphandler.C
> index a921643..fc6c811 100644
> --- a/apphandler.C
> +++ b/apphandler.C
> @@ -358,7 +358,7 @@ ipmi_ret_t ipmi_app_channel_info(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> printf("IPMI APP GET CHANNEL INFO\n");
>
> // I"m only supporting channel 1. 0xE is the 'default channel'
> - if (*p == 0xe || *p == 1) {
> + if (*p == 0xe || *p == 1 || *p == 8) {
>
So I take it that the comment is no longer valid? Can we take a break on magic
numbers or at least quote and link the spec nearby?
> *data_len = sizeof(resp);
> memcpy(response, resp, *data_len);
> diff --git a/ipmid.C b/ipmid.C
> index 6726a27..728ba0b 100644
> --- a/ipmid.C
> +++ b/ipmid.C
> @@ -267,7 +267,6 @@ static int handle_ipmi_command(sd_bus_message *m, void *user_data, sd_bus_error
> if(r != 0)
> {
> fprintf(stderr,"ERROR:[0x%X] handling NetFn:[0x%X], Cmd:[0x%X]\n",r, netfn, cmd);
> - return -1;
> }
Was this change intended?
>
> fprintf(ipmiio, "IPMI Response:\n");
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH phosphor-host-ipmid] ipmi daemon return code modification
2016-03-09 2:31 ` Cyril Bur
@ 2016-03-09 17:01 ` Patrick Williams
2016-03-09 18:56 ` Chris Austen
[not found] ` <201603091856.u29Iua5p031849@d03av01.boulder.ibm.com>
0 siblings, 2 replies; 6+ messages in thread
From: Patrick Williams @ 2016-03-09 17:01 UTC (permalink / raw)
To: Cyril Bur; +Cc: OpenBMC Patches, openbmc, tomjose
[-- Attachment #1: Type: text/plain, Size: 2004 bytes --]
I agree with Cyril's comments. This code should not have been merged in
the current state.
The first line directly contradicts the comments. Need to update the
comment to explain why '8' is needed now. And removing an error path is
non-obvious. Even the commit message doesn't really give any details on
what / why this change was made.
On Wed, Mar 09, 2016 at 01:31:23PM +1100, Cyril Bur wrote:
> On Tue, 8 Mar 2016 14:10:33 -0600
> OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:
>
> > From: tomjose <tomjoseph@in.ibm.com>
> >
> > ---
> > apphandler.C | 2 +-
> > ipmid.C | 1 -
> > 2 files changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/apphandler.C b/apphandler.C
> > index a921643..fc6c811 100644
> > --- a/apphandler.C
> > +++ b/apphandler.C
> > @@ -358,7 +358,7 @@ ipmi_ret_t ipmi_app_channel_info(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> > printf("IPMI APP GET CHANNEL INFO\n");
> >
> > // I"m only supporting channel 1. 0xE is the 'default channel'
> > - if (*p == 0xe || *p == 1) {
> > + if (*p == 0xe || *p == 1 || *p == 8) {
> >
>
> So I take it that the comment is no longer valid? Can we take a break on magic
> numbers or at least quote and link the spec nearby?
>
> > *data_len = sizeof(resp);
> > memcpy(response, resp, *data_len);
> > diff --git a/ipmid.C b/ipmid.C
> > index 6726a27..728ba0b 100644
> > --- a/ipmid.C
> > +++ b/ipmid.C
> > @@ -267,7 +267,6 @@ static int handle_ipmi_command(sd_bus_message *m, void *user_data, sd_bus_error
> > if(r != 0)
> > {
> > fprintf(stderr,"ERROR:[0x%X] handling NetFn:[0x%X], Cmd:[0x%X]\n",r, netfn, cmd);
> > - return -1;
> > }
>
> Was this change intended?
>
> >
> > fprintf(ipmiio, "IPMI Response:\n");
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc
--
Patrick Williams
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH phosphor-host-ipmid] ipmi daemon return code modification
2016-03-09 17:01 ` Patrick Williams
@ 2016-03-09 18:56 ` Chris Austen
[not found] ` <201603091856.u29Iua5p031849@d03av01.boulder.ibm.com>
1 sibling, 0 replies; 6+ messages in thread
From: Chris Austen @ 2016-03-09 18:56 UTC (permalink / raw)
To: Patrick Williams; +Cc: openbmc, Tom Joseph2, OpenBMC Patches, Cyril Bur
[-- Attachment #1: Type: text/plain, Size: 2993 bytes --]
Patrick is there work needed to fix something here? If so open an issue
I think the lesson here is we need more integration testing (aka warm hand offs) with Foxconn. I chose channel 1 without thinking too much about it. Turns out they use channel 8.
Sent from my iPhone using IBM Verse
On Mar 9, 2016, 9:09:21 AM, patrick@stwcx.xyz wrote:
From: patrick@stwcx.xyz
To: cyrilbur@gmail.com
Cc: openbmc@lists.ozlabs.org, tomjoseph@in.ibm.com, openbmc-patches@stwcx.xyz
Date: Mar 9, 2016 9:09:21 AM
Subject: Re: [PATCH phosphor-host-ipmid] ipmi daemon return code modification
I agree with Cyril's comments. This code should not have been merged in
the current state.
The first line directly contradicts the comments. Need to update the
comment to explain why '8' is needed now. And removing an error path is
non-obvious. Even the commit message doesn't really give any details on
what / why this change was made.
On Wed, Mar 09, 2016 at 01:31:23PM +1100, Cyril Bur wrote:
> On Tue, 8 Mar 2016 14:10:33 -0600
> OpenBMC Patches wrote:
>
> > From: tomjose
> >
> > ---
> > apphandler.C | 2 +-
> > ipmid.C | 1 -
> > 2 files changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/apphandler.C b/apphandler.C
> > index a921643..fc6c811 100644
> > --- a/apphandler.C
> > +++ b/apphandler.C
> > @@ -358,7 +358,7 @@ ipmi_ret_t ipmi_app_channel_info(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> > printf("IPMI APP GET CHANNEL INFO\n");
> >
> > // I"m only supporting channel 1. 0xE is the 'default channel'
> > - if (*p == 0xe || *p == 1) {
> > + if (*p == 0xe || *p == 1 || *p == 8) {
> >
>
> So I take it that the comment is no longer valid? Can we take a break on magic
> numbers or at least quote and link the spec nearby?
>
> > *data_len = sizeof(resp);
> > memcpy(response, resp, *data_len);
> > diff --git a/ipmid.C b/ipmid.C
> > index 6726a27..728ba0b 100644
> > --- a/ipmid.C
> > +++ b/ipmid.C
> > @@ -267,7 +267,6 @@ static int handle_ipmi_command(sd_bus_message *m, void *user_data, sd_bus_error
> > if(r != 0)
> > {
> > fprintf(stderr,"ERROR:[0x%X] handling NetFn:[0x%X], Cmd:[0x%X]\n",r, netfn, cmd);
> > - return -1;
> > }
>
> Was this change intended?
>
> >
> > fprintf(ipmiio, "IPMI Response:\n");
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc
--
Patrick Williams
_______________________________________________
openbmc mailing list
openbmc@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/openbmc
[-- Attachment #2: Type: text/html, Size: 5419 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH phosphor-host-ipmid] ipmi daemon return code modification
[not found] ` <201603091856.u29Iua5p031849@d03av01.boulder.ibm.com>
@ 2016-03-18 16:04 ` Patrick Williams
0 siblings, 0 replies; 6+ messages in thread
From: Patrick Williams @ 2016-03-18 16:04 UTC (permalink / raw)
To: Chris Austen; +Cc: openbmc, Tom Joseph2, OpenBMC Patches, Cyril Bur
[-- Attachment #1: Type: text/plain, Size: 3306 bytes --]
On Wed, Mar 09, 2016 at 06:56:17PM +0000, Chris Austen wrote:
> Patrick is there work needed to fix something here? If so open an issue
>
Opened https://github.com/openbmc/phosphor-host-ipmid/issues/80
> I think the lesson here is we need more integration testing (aka warm hand offs) with Foxconn. I chose channel 1 without thinking too much about it. Turns out they use channel 8.
>
>
>
> Sent from my iPhone using IBM Verse
>
> On Mar 9, 2016, 9:09:21 AM, patrick@stwcx.xyz wrote:
>
> From: patrick@stwcx.xyz
> To: cyrilbur@gmail.com
> Cc: openbmc@lists.ozlabs.org, tomjoseph@in.ibm.com, openbmc-patches@stwcx.xyz
> Date: Mar 9, 2016 9:09:21 AM
> Subject: Re: [PATCH phosphor-host-ipmid] ipmi daemon return code modification
>
>
> I agree with Cyril's comments. This code should not have been merged in
> the current state.
> The first line directly contradicts the comments. Need to update the
> comment to explain why '8' is needed now. And removing an error path is
> non-obvious. Even the commit message doesn't really give any details on
> what / why this change was made.
> On Wed, Mar 09, 2016 at 01:31:23PM +1100, Cyril Bur wrote:
> > On Tue, 8 Mar 2016 14:10:33 -0600
> > OpenBMC Patches wrote:
> >
> > > From: tomjose
> > >
> > > ---
> > > apphandler.C | 2 +-
> > > ipmid.C | 1 -
> > > 2 files changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/apphandler.C b/apphandler.C
> > > index a921643..fc6c811 100644
> > > --- a/apphandler.C
> > > +++ b/apphandler.C
> > > @@ -358,7 +358,7 @@ ipmi_ret_t ipmi_app_channel_info(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> > > printf("IPMI APP GET CHANNEL INFO\n");
> > >
> > > // I"m only supporting channel 1. 0xE is the 'default channel'
> > > - if (*p == 0xe || *p == 1) {
> > > + if (*p == 0xe || *p == 1 || *p == 8) {
> > >
> >
> > So I take it that the comment is no longer valid? Can we take a break on magic
> > numbers or at least quote and link the spec nearby?
> >
> > > *data_len = sizeof(resp);
> > > memcpy(response, resp, *data_len);
> > > diff --git a/ipmid.C b/ipmid.C
> > > index 6726a27..728ba0b 100644
> > > --- a/ipmid.C
> > > +++ b/ipmid.C
> > > @@ -267,7 +267,6 @@ static int handle_ipmi_command(sd_bus_message *m, void *user_data, sd_bus_error
> > > if(r != 0)
> > > {
> > > fprintf(stderr,"ERROR:[0x%X] handling NetFn:[0x%X], Cmd:[0x%X]\n",r, netfn, cmd);
> > > - return -1;
> > > }
> >
> > Was this change intended?
> >
> > >
> > > fprintf(ipmiio, "IPMI Response:\n");
> >
> > _______________________________________________
> > openbmc mailing list
> > openbmc@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/openbmc
> --
> Patrick Williams
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc
--
Patrick Williams
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-03-18 16:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-08 20:10 [PATCH phosphor-host-ipmid] ipmi daemon return code modification OpenBMC Patches
2016-03-08 20:10 ` OpenBMC Patches
2016-03-09 2:31 ` Cyril Bur
2016-03-09 17:01 ` Patrick Williams
2016-03-09 18:56 ` Chris Austen
[not found] ` <201603091856.u29Iua5p031849@d03av01.boulder.ibm.com>
2016-03-18 16:04 ` Patrick Williams
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.