From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e23smtp09.au.ibm.com (e23smtp09.au.ibm.com [202.81.31.142]) (using TLSv1 with cipher CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 7F9EF1A0102 for ; Wed, 6 Jan 2016 17:14:15 +1100 (AEDT) Received: from localhost by e23smtp09.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 6 Jan 2016 16:14:13 +1000 Received: from d23dlp01.au.ibm.com (202.81.31.203) by e23smtp09.au.ibm.com (202.81.31.206) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Wed, 6 Jan 2016 16:14:12 +1000 X-IBM-Helo: d23dlp01.au.ibm.com X-IBM-MailFrom: shgoupf@cn.ibm.com X-IBM-RcptTo: openbmc@lists.ozlabs.org Received: from d23relay10.au.ibm.com (d23relay10.au.ibm.com [9.190.26.77]) by d23dlp01.au.ibm.com (Postfix) with ESMTP id 230B02CE8054 for ; Wed, 6 Jan 2016 17:14:12 +1100 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay10.au.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u066E4Wx52822134 for ; Wed, 6 Jan 2016 17:14:12 +1100 Received: from d23av03.au.ibm.com (localhost [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u066Ddct024397 for ; Wed, 6 Jan 2016 17:13:39 +1100 Received: from e39.co.us.ibm.com (e39.boulder.ibm.com [9.17.249.49]) by d23av03.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id u066Dbgg023586 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 6 Jan 2016 17:13:38 +1100 Message-Id: <201601060613.u066Dbgg023586@d23av03.au.ibm.com> Received: from localhost by e39.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 5 Jan 2016 23:13:13 -0700 Received: from smtp.notes.na.collabserv.com (192.155.248.91) by e39.co.us.ibm.com (192.168.2.139) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256/256) Tue, 5 Jan 2016 23:13:10 -0700 X-IBM-Helo: smtp.notes.na.collabserv.com X-IBM-MailFrom: shgoupf@cn.ibm.com X-IBM-RcptTo: openbmc@lists.ozlabs.org Received: from /spool/local by smtp.notes.na.collabserv.com with smtp.notes.na.collabserv.com ESMTP for from ; Wed, 6 Jan 2016 06:13:09 -0000 Received: from us1a3-smtp02.a3.dal06.isc4sb.com (10.106.154.103) by smtp.notes.na.collabserv.com (10.106.227.143) with smtp.notes.na.collabserv.com ESMTP; Wed, 6 Jan 2016 06:13:06 -0000 Received: from us1a3-mail141.a3.dal06.isc4sb.com ([10.146.38.85]) by us1a3-smtp02.a3.dal06.isc4sb.com with ESMTP id 2016010606135038-32176 ; Wed, 6 Jan 2016 06:13:50 +0000 Subject: Re: [PATCH phosphor-host-ipmid v4] Add get/set boot option ipmid command support with correct DBUS property handling. In-Reply-To: <20160106160934.493e446f@camb691> From: "Peng Fei BG Gou" To: cyrilbur@gmail.com Cc: openbmc-patches@stwcx.xyz, openbmc@lists.ozlabs.org Date: Wed, 6 Jan 2016 06:13:06 +0000 Sensitivity: MIME-Version: 1.0 References: <20160106160934.493e446f@camb691>, <1451956226-19954-1-git-send-email-openbmc-patches@stwcx.xyz><1451956226-19954-2-git-send-email-openbmc-patches@stwcx.xyz> Importance: Normal X-Priority: 3 (Normal) X-Mailer: Lotus Domino Web Server Build V851SAAS_12072015_FP3 December 17, 2015 X-LLNOutbound: False X-Disclaimed: 25511 X-TNEFEvaluated: 1 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable x-cbid: 16010606-0033-0000-0000-000002AF96AD X-IBM-ISS-SpamDetectors: Score=0.397008; BY=0; FL=0; FP=0; FZ=0; HX=0; KW=0; PH=0; SC=0.397008; ST=0; TS=0; UL=0; ISC= X-IBM-ISS-DetailInfo: BY=3.00004758; HX=3.00000237; KW=3.00000007; PH=3.00000004; SC=3.00000130; SDB=6.00641199; UDB=6.00288616; UTC=2016-01-06 06:13:08 x-cbparentid: 16010606-4778-0000-0000-000001765683 X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER X-BeenThere: openbmc@lists.ozlabs.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Development list for OpenBMC List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Jan 2016 06:14:16 -0000
Hey Cyril,
 
Thanks for your detailed review on this patch.
 
Just want to answer (most of) your questions in batch:
1) This patch intended to add the functionality of ipmi s= et/get boot option, with dbus property and object mapper used, so I believe= all of the contents in this patch should be considered as a whole. We shou= ldn't split them into multiple patches.
2) atoi is an alternative, but I don't see there is any r= eason why to use it since sscanf/sprintf works fine for now. I don't believ= e there is too much performance concern there. 
3) For those sd=5Fbus method call with signatures ("s", "= v", "s{ass}", etc.), I believe the current way I use it is valid since I ha= ve tested them already.
4) For the commented out error handling, I believe it's j= ust a work around, but it did works according to my test. So I believe it's= OK for this patch to have it and we should revisit it later.
5) For the way how return code is handled, I believe it i= s working fine.
6) For the coding style stuff, such as spaces between fun= ction names and the bracket, I don't think it's a major issue. Maybe it's a= minor issue that they are not consistent with some piece of code elsewhere= . It can be fixed with my auto style tool very easily. 
 
In a sum, thanks for your effort on reviewing this, but I= believe most of your current concerns either are invalid or are only = minor issues.
 
GOU, Peng Fei (=E8=8B= =9F=E9=B9=8F=E9=A3=9E), Ph.D.
OpenPower Team.
+86-21-609-28631
 
 
----- Original message -----
From: Cyril Bur <cyri= lbur@gmail.com>
To: OpenBMC Patches <openbmc-patches@stwcx.xyz>= , Peng Fei BG Gou/China/IBM@IBMCN
Cc: openbmc@lists.ozlabs.org
Subjec= t: Re: [PATCH phosphor-host-ipmid v4] Add get/set boot option ipmid command= support with correct DBUS property handling.
Date: Wed, Jan 6, 2016 1:1= 0 PM
 
On Mon,  4 Jan 2016 19:10:26 -0600
OpenBMC Patches <openbmc= -patches@stwcx.xyz> wrote:

> From: shgoupf <shgoupf@cn.ibm.= com>
>

Hi again,

> 1) Add support for IPMI get/se= t boot option command.
>     a) boot options stored in dbus= property.
>     b) IPMI get boot option command get boot o= ption from the dbus
>     property.
>     c= ) IPMI set boot option coomand set boot option to the dbus
>   &= nbsp; property.
> 2) Two methods to handle the dbus property set and = get:
>     a) dbus=5Fset=5Fproperty()
>    = b) dbus=5Fget=5Fproperty()
> 3) The property is stored as a 10 chara= cter strings which representsd 5-byte information.
> 4) ipmid set met= hod is registered and implemented since petitboot will use it to clear the = boot options.
> 5) Get service name via object mapper
>   =   a) The connection name is got via objectmapper.
>    = ; b) The method used to get the connection name is object=5Fmapper=5Fget=5F= connection().
>     c) dbus=5Fget=5Fproperty/dbus=5Fset=5Fp= roperty will get the connection name via the above method instead of hard c= oding.
> 6) Error code are properly handled.
> 7) Use sprinf/ss= canf for int/string conversion.
>     a) Instead of reinven= ting the wheel by defining methods converting int to string, use sprintf/ss= canf should be a more clean and robust way.

Plus 1 for not reinventi= ng the wheel but perhaps consider using the best tool
for the job, sscan= f can and does do what you want however atoi, atol, atoll,
strtol, strto= ll, strtoq functions are all designed for this exact purpose. They
will = perform much better than sscanf, also the str* versions give great control<= br>and error detection capabilities.


> 8) Fix issues addresse= d by the community.
>     a) change malloc heap to stack lo= cal variable.
>     b) Fix multi line comment style from "/= /" to "/**/".
>     c) Add defines for return codes.
>= ;     d) Add more comments.

It looks like you're adding mo= re and more into this same patch. A single patch
should be one clearly d= efined piece of work. So you're implementing some new
functionality, tha= t should be its own patch. You're also adding some error
handling paths = - also, its own patch. You've changed some whitespace (some of
which was= a bad thing) but the good whitespace changes should also be their own
p= atch.

>  chassishandler.C | 492 ++++++++++++++++++++++++++++= +++++++++++++++++----------
>  chassishandler.h |  18 ++>  2 files changed, 423 insertions(+), 87 deletions(-)
>
= > diff --git a/chassishandler.C b/chassishandler.C
> index 1389db9= ..4d941e8 100644
> --- a/chassishandler.C
> +++ b/chassishandle= r.C
> @@ -5,11 +5,235 @@
>  #include <stdint.h>
&= gt;  
>  // OpenBMC Chassis Manager dbus framework
> = -const char  *chassis=5Fbus=5Fname      =3D  "org.= openbmc.control.Chassis";
> -const char  *chassis=5Fobject=5Fnam= e   =3D  "/org/openbmc/control/chassis0";
> -const char &nb= sp;*chassis=5Fintf=5Fname     =3D  "org.openbmc.control.Chas= sis";
> +const char * chassis=5Fbus=5Fname      =3D &n= bsp;"org.openbmc.control.Chassis";
> +const char * chassis=5Fobject= =5Fname   =3D  "/org/openbmc/control/chassis0";
> +const ch= ar * chassis=5Fintf=5Fname     =3D  "org.openbmc.control.Cha= ssis";
>  

Not necessary, if anything you should remove t= he double space but what you've
done doesn't look good.

> -voi= d register=5Fnetfn=5Fchassis=5Ffunctions() =5F=5Fattribute=5F=5F((construct= or));
> +// Host settings in dbus
> +// Service name should be = referenced by connection name got via object mapper
> +const char * s= ettings=5Fobject=5Fname  =3D  "/org/openbmc/settings/host0";
&= gt; +const char * settings=5Fintf=5Fname    =3D  "org.freede= sktop.DBus.Properties";
> +
> +const char * objmapper=5Fservice= =5Fname =3D  "org.openbmc.objectmapper";
> +const char * objmapp= er=5Fobject=5Fname  =3D  "/org/openbmc/objectmapper/objectmapper"= ;
> +const char * objmapper=5Fintf=5Fname    =3D  "org= .openbmc.objectmapper.ObjectMapper";
> +

We probably want to t= ry to keep a consistent style. I looked at ipmid.C and
that file isn't c= onsistent but it seems to be mostly "type *name;" and that's
the most co= mmon way everywhere else with C code.

> +int object=5Fmapper=5Fge= t=5Fconnection (char ** buf, const char * obj=5Fpath)
> +{
> + =    sd=5Fbus=5Ferror error =3D SD=5FBUS=5FERROR=5FNULL;
> + =    sd=5Fbus=5Fmessage * m =3D NULL;
> +    sd=5Fb= us * bus =3D NULL;
> +    char * temp=5Fbuf =3D NULL, *intf= =3D NULL;
> +    size=5Ft buf=5Fsize =3D 0;
> + &nbs= p;  int r;
> +
> +    // Open the system bus whe= re most system services are provided.
> +    r =3D sd=5Fbus= =5Fopen=5Fsystem (&bus);
> +

As an added stylistic note, a= gain for consistency, ipmid.C, you should avoid
having a space between t= he function name and the parameters

> +    if (r < 0= ) {
> +        fprintf (stderr, "Failed to connec= t to system bus: %s\n", strerror (-r));
> +       &nbs= p;goto finish;
> +    }
> +
> +    /*=
> +     * Bus, service, object path, interface and method = are provided to call
> +     * the method.
> +  =   * Signatures and input arguments are provided by the arguments at t= he
> +     * end.
> +     */
> + &nbs= p;  r =3D sd=5Fbus=5Fcall=5Fmethod (bus,
> +      = ;                     &nb= sp;objmapper=5Fservice=5Fname,             &n= bsp;        /* service to contact */
> +   &= nbsp;                    =    objmapper=5Fobject=5Fname,          =             /* object path */
> + &nbs= p;                     &n= bsp;    objmapper=5Fintf=5Fname,         &nbs= p;               /* interface name */> +                   &nbs= p;        "GetObject",         &nbs= p;                     &n= bsp; /* method name */
> +             =                &error,   &= nbsp;                    =              /* object to return error = in */
> +                 &nb= sp;          &m,         &= nbsp;                    =            /* return message on success */> +                   &nb= sp;        "s",           &nbs= p;                     &n= bsp;       /* input signature */
> +     &nb= sp;                     &= nbsp;obj=5Fpath                 &nb= sp;                   /* first= argument */
> +               &nb= sp;           );
> +
> +    = if (r < 0) {
> +        fprintf (stderr, "Fail= ed to issue method call: %s\n", error.message);
> +     &nb= sp;  goto finish;
> +    }
> +
> +   =  // Get the key, aka, the connection name
> +    sd=5F= bus=5Fmessage=5Fread (m, "a{sas}", 1, &temp=5Fbuf, 1, &intf);
&g= t; +

This seems very roundabout, could you explain this method call.= What are you
expecting to receive? From the looks of what you're using = it for wouldn't it
make more sense to just sd=5Fbus=5Fmessage=5Fread a "= s"?

> +    /*
> +     * TODO: check the= return code. Currently for no reason the message
> +     *= parsing of object mapper is always complaining about
> +   &nbs= p; * "Device or resource busy", but the result seems OK for now. Need
&g= t; +     * further checks.

If sd=5Fbus=5Fmessage=5Fread re= turns a negative value, something definitely went
wrong, I highly highly= doubt you can use the result if it turns a negative.

> +   =   * TODO: The following code is preserved in the comments so that it c= an be
> +     * resumed after the problem aforementioned is= resolved.
> +     *r =3D sd=5Fbus=5Fmessage=5Fread(m, "a{s= as}", 1, &temp=5Fbuf, 1, &intf);
> +     *if (r <= ; 0) {
> +     *    fprintf(stderr, "Failed to pa= rse response message: %s\n", strerror(-r));
> +     *  = ;  goto finish;
> +     *}

The code is in git, = it is saved there, delete it now and when someone wants to
add it back t= hey can look in the git history for what was deleted. Having said
that, = the error check should be there, if you're hitting it, you should figureout why.

> +     */
> +
> +    b= uf=5Fsize =3D strlen (temp=5Fbuf) + 1;
> +    printf ("IPMI= D connection name: %s\n", temp=5Fbuf);
> +    *buf =3D (cha= r *)malloc (buf=5Fsize);
> +
> +    if (*buf =3D=3D N= ULL) {
> +        fprintf (stderr, "Malloc failed= for get=5Fsys=5Fboot=5Foptions");
> +        r = =3D -1;
> +        goto finish;
> +   =  }
> +

I don't mind using malloc but these are strings no= ? Couldn't strndup be used,
might make life easier? Not a big concern.
> +    memcpy (*buf, temp=5Fbuf, buf=5Fsize);
> +<= br>> +finish:
> +    sd=5Fbus=5Ferror=5Ffree (&error= );
> +    sd=5Fbus=5Fmessage=5Funref (m);
> +   =  sd=5Fbus=5Funref (bus);
> +

Spaces between name and para= ms

> +    return r;
> +}
> +
> +int d= bus=5Fget=5Fproperty (char * buf)
> +{
> +    sd=5Fbu= s=5Ferror error =3D SD=5FBUS=5FERROR=5FNULL;
> +    sd=5Fbu= s=5Fmessage * m =3D NULL;
> +    sd=5Fbus * bus =3D NULL;> +    char * temp=5Fbuf =3D NULL;
> +    ui= nt8=5Ft * get=5Fvalue =3D NULL;
> +    char * connection = =3D NULL;
> +    int r, i;
> +
> +    = ;r =3D object=5Fmapper=5Fget=5Fconnection (&connection, settings=5Fobje= ct=5Fname);
> +
> +    if (r < 0) {
> + &nbs= p;      fprintf (stderr, "Failed to get connection, return v= alue: %d.\n", r);
> +        goto finish;
>= +    }
> +
> +    printf ("connection: %s\= n", connection);
> +
> +    // Open the system bus wh= ere most system services are provided.
> +    r =3D sd=5Fbu= s=5Fopen=5Fsystem (&bus);
> +
> +    if (r < 0= ) {
> +        fprintf (stderr, "Failed to connec= t to system bus: %s\n", strerror (-r));
> +       &nbs= p;goto finish;
> +    }
> +
> +    /*=
> +     * Bus, service, object path, interface and method = are provided to call
> +     * the method.
> +  =   * Signatures and input arguments are provided by the arguments at t= he
> +     * end.
> +     */
> + &nbs= p;  r =3D sd=5Fbus=5Fcall=5Fmethod (bus,
> +      = ;                     &nb= sp;connection,                 &nbs= p;               /* service to contact *= /
> +                   =          settings=5Fobject=5Fname,     &= nbsp;                 /* object pat= h */
> +                 &nbs= p;          settings=5Fintf=5Fname,     =                     /* in= terface name */
> +               =              "Get",      =                     &nbs= p;          /* method name */
> +   &nb= sp;                     &= nbsp;  &error,               &n= bsp;                     = /* object to return error in */
> +          = ;                  &m, &nb= sp;                     &= nbsp;                 /* return mes= sage on success */
> +             &nbs= p;              "ss",     &nbs= p;                     &n= bsp;           /* input signature */
> + &nb= sp;                     &= nbsp;    settings=5Fintf=5Fname,         &nbs= p;               /* first argument */> +                   &nbs= p;        "boot=5Fflags");         =                      = ;/* second argument */
> +
> +    if (r < 0) {
= > +        fprintf (stderr, "Failed to issue method = call: %s\n", error.message);
> +        goto fini= sh;
> +    }
> +
> +    /*
> + =     * The output should be parsed exactly the same as the output = formatting
> +     * specified.
> +     */<= br>> +    r =3D sd=5Fbus=5Fmessage=5Fread (m, "v", "s", &t= emp=5Fbuf);
> +

Why not just read a "s"?

> +   =  if (r < 0) {
> +        fprintf (stderr,= "Failed to parse response message: %s\n", strerror (-r));
> +  =      goto finish;
> +    }
> +
>= ; +    printf ("IPMID boot option property get: {%s}.\n", (char *= ) temp=5Fbuf);
> +
> +    /*
> +     = * To represent a hex in string, e.g., "A0A0", which represents two bytes> +     * in the hex, but requires 5 bytes to store it as str= ing, i.e., 4
> +     * characters to store the "A0A0" and 1= additional space for "\0".
> +     * Thereby we have 2 * &= lt;number of bytes> + 1 for the string buffer.
> +     *= /

So why don't you have #define NUM=5FRETURN=5FBYTES=5FOF=5FGET=5FUS= ED 11 ?

This question is out of scope of this patch but why are we p= assing around
numbers as strings? dbus handles other types...

>= ; +    memcpy (buf, temp=5Fbuf, 2 * NUM=5FRETURN=5FBYTES=5FOF=5FG= ET=5FUSED + 1);
> +
> +finish:
> +    sd=5Fbus= =5Ferror=5Ffree (&error);
> +    sd=5Fbus=5Fmessage=5Fu= nref (m);
> +    sd=5Fbus=5Funref (bus);
> +   &= nbsp;free (connection);
> +
> +    return r;
> = +}
> +
> +int dbus=5Fset=5Fproperty (const char * buf)
> = +{
> +    sd=5Fbus=5Ferror error =3D SD=5FBUS=5FERROR=5FNUL= L;
> +    sd=5Fbus=5Fmessage * m =3D NULL;
> +  =  sd=5Fbus * bus =3D NULL;
> +    char * connection = =3D NULL;
> +    int r;
> +
> +    r = =3D object=5Fmapper=5Fget=5Fconnection (&connection, settings=5Fobject= =5Fname);
> +
> +    if (r < 0) {
> +  =      fprintf (stderr, "Failed to get connection, return val= ue: %d.\n", r);
> +        goto finish;
> +=    }
> +
> +    printf ("connection: %s\n"= , connection);
> +
> +    // Open the system bus wher= e most system services are provided.
> +    r =3D sd=5Fbus= =5Fopen=5Fsystem (&bus);
> +
> +    if (r < 0)= {
> +        fprintf (stderr, "Failed to connect= to system bus: %s\n", strerror (-r));
> +        = ;goto finish;
> +    }
> +
> +    /*<= br>> +     * Bus, service, object path, interface and method a= re provided to call
> +     * the method.
> +   =   * Signatures and input arguments are provided by the arguments at th= e
> +     * end.
> +     */
> +  = ;  r =3D sd=5Fbus=5Fcall=5Fmethod (bus,
> +      =                     &nbs= p;connection,                  = ;               /* service to contact */=
> +                   &= nbsp;        settings=5Fobject=5Fname,     &n= bsp;                 /* object path= */
> +                  = ;          settings=5Fintf=5Fname,     &= nbsp;                   /* int= erface name */
> +               &= nbsp;            "Set",       =                      = ;          /* method name */
> +   &nbs= p;                     &n= bsp;  &error,               &nb= sp;                     /= * object to return error in */
> +          =                  &m, &nbs= p;                     &n= bsp;                 /* return mess= age on success */
> +              = ;              "ssv",     &nbs= p;                     &n= bsp;          /* input signature */
> + &nbs= p;                     &n= bsp;    settings=5Fintf=5Fname,          = ;               /* first argument */
= > +                    = ;        "boot=5Fflags",         &n= bsp;                     = /* second argument */
> +             &= nbsp;              "s",     &n= bsp;                     =              /* third argument */
>= ; +                     &= nbsp;      buf);             &= nbsp;                    =     /* fourth argument */
> +

Are we documenting th= ese interfaces at all, I don't understand why "ssv" and
then a hardcoded= "s", why not "sss"?

> +    if (r < 0) {
> + &= nbsp;      fprintf (stderr, "Failed to issue method call: %s= \n", error.message);
> +        goto finish;
&= gt; +    }
> +
> +    printf ("IPMID boot o= ption property set: {%s}.\n", buf);
> +
> +finish:
> + &n= bsp;  sd=5Fbus=5Ferror=5Ffree (&error);
> +    sd= =5Fbus=5Fmessage=5Funref (m);
> +    sd=5Fbus=5Funref (bus)= ;
> +    free (connection);
> +
> +   &nbs= p;return r;
> +}
> +
> +void register=5Fnetfn=5Fchassis= =5Ffunctions () =5F=5Fattribute=5F=5F ((constructor));
>  
&g= t;  struct get=5Fsys=5Fboot=5Foptions=5Ft {
>     &nbs= p;uint8=5Ft parameter;
> @@ -17,11 +241,28 @@ struct get=5Fsys=5Fboot= =5Foptions=5Ft {
>      uint8=5Ft block;
>  = ;}  =5F=5Fattribute=5F=5F ((packed));
>  
> -ipmi=5Fr= et=5Ft ipmi=5Fchassis=5Fwildcard(ipmi=5Fnetfn=5Ft netfn, ipmi=5Fcmd=5Ft cmd= ,
> -                   =            ipmi=5Frequest=5Ft request, ipmi= =5Fresponse=5Ft response,
> -           &nbs= p;                  ipmi=5Fdat= a=5Flen=5Ft data=5Flen, ipmi=5Fcontext=5Ft context)
> +struct set=5Fs= ys=5Fboot=5Foptions=5Ft {
> +    uint8=5Ft parameter;
&g= t; +    union {
> +        struct {
= > +            uint8=5Ft d1;
> + &nb= sp;          uint8=5Ft d2;
> +    =        uint8=5Ft d3;
> +       &n= bsp;    uint8=5Ft d4;
> +          = ;  uint8=5Ft d5;
> +            ui= nt8=5Ft d6;
> +            uint8=5Ft d7= ;
> +            uint8=5Ft d8;
> = +        } data8;
> +        = uint64=5Ft data64;
> +    } data;
> +}  =5F=5Fat= tribute=5F=5F ((packed));
> +
> +ipmi=5Fret=5Ft ipmi=5Fchassis= =5Fwildcard (ipmi=5Fnetfn=5Ft netfn, ipmi=5Fcmd=5Ft cmd,
> +   &= nbsp;                    =          ipmi=5Frequest=5Ft request, ipmi=5Frespo= nse=5Ft response,
> +              = ;                    ipmi= =5Fdata=5Flen=5Ft data=5Flen, ipmi=5Fcontext=5Ft context)
>  {> -    printf("Handling CHASSIS WILDCARD Netfn:[0x%X], Cmd:[= 0x%X]\n",netfn, cmd);
> +    printf ("Handling CHASSIS WILD= CARD Netfn:[0x%X], Cmd:[0x%X]\n", netfn, cmd);

Be careful with chang= es like this, you didn't change this code...

>     &nbs= p;// Status code.
>      ipmi=5Fret=5Ft rc =3D IPMI=5F= CC=5FOK;
>      *data=5Flen =3D 0;
> @@ -31,116 = +272,193 @@ ipmi=5Fret=5Ft ipmi=5Fchassis=5Fwildcard(ipmi=5Fnetfn=5Ft netfn= , ipmi=5Fcmd=5Ft cmd,
>  //-------------------------------------= -----------------------
>  // Calls into Chassis Control Dbus ob= ject to do the power off
>  //----------------------------------= --------------------------
> -int ipmi=5Fchassis=5Fpower=5Fcontrol(co= nst char *method)
> +int ipmi=5Fchassis=5Fpower=5Fcontrol (const char= * method)
>  {
> - // sd=5Fbus error
> - int rc =3D= 0;
> +    // sd=5Fbus error
> +    int rc = =3D 0;
>  
>      // SD Bus error report me= chanism.
>      sd=5Fbus=5Ferror bus=5Ferror =3D SD=5F= BUS=5FERROR=5FNULL;
>  
> - // Response from the call. Alt= hough there is no response for this call,
> - // obligated to mention= this to make compiler happy.
> - sd=5Fbus=5Fmessage *response =3D NU= LL;
> -
> - // Gets a hook onto either a SYSTEM or SESSION bus<= br>> - sd=5Fbus *bus=5Ftype =3D ipmid=5Fget=5Fsd=5Fbus=5Fconnection();> -
> - rc =3D sd=5Fbus=5Fcall=5Fmethod(bus=5Ftype,    = ;     // On the System Bus
> - chassis=5Fbus=5Fname,  =      // Service to contact
> - chassis=5Fobject=5Fnam= e,     // Object path
> - chassis=5Fintf=5Fname,   &nb= sp;   // Interface name
> - method,       // Meth= od to be called
> - &bus=5Ferror,       // object = to return error
> - &response, // Response buffer if any
> = - NULL); // No input arguments
> - if(rc < 0)
> - {
> = - fprintf(stderr,"ERROR initiating Power Off:[%s]\n",bus=5Ferror.message);<= br>> - }
> - else
> - {
> - printf("Chassis Power Off = initiated successfully\n");
> - }
> -
> -    sd= =5Fbus=5Ferror=5Ffree(&bus=5Ferror);
> -    sd=5Fbus=5F= message=5Funref(response);
> -
> - return rc;
> +   =  /*
> +     * Response from the call. Although there i= s no response for this call,
> +     * obligated to mention= this to make compiler happy.
> +     */
> +   &= nbsp;sd=5Fbus=5Fmessage * response =3D NULL;
> +
> +   &nb= sp;// Gets a hook onto either a SYSTEM or SESSION bus
> +   &nbs= p;sd=5Fbus * bus=5Ftype =3D ipmid=5Fget=5Fsd=5Fbus=5Fconnection ();
>= +
> +    rc =3D sd=5Fbus=5Fcall=5Fmethod (bus=5Ftype, &nbs= p;              /* On the System Bus*/> +                   &nb= sp;         chassis=5Fbus=5Fname,       =  /* Service to contact*/
> +           =                   chassis=5Fob= ject=5Fname,     /* Object path*/
> +       =                      = ; chassis=5Fintf=5Fname,       /* Interface name*/
> +=                     &nbs= p;       method,             &= nbsp;    /* Method to be called*/
> +       =                      = ; &bus=5Ferror,              /* obje= ct to return error*/
> +             &n= bsp;               &response,  =             /* Response buffer if any*/
&= gt; +                    =         NULL);           &nbs= p;       /* No input arguments*/
> +
> +   =  if (rc < 0) {
> +        fprintf (stderr= , "ERROR initiating Power Off:[%s]\n", bus=5Ferror.message);
> + &nbs= p;  } else {
> +        printf ("Chassis Pow= er Off initiated successfully\n");
> +    }
> +
&g= t; +    sd=5Fbus=5Ferror=5Ffree (&bus=5Ferror);
> + &nb= sp;  sd=5Fbus=5Fmessage=5Funref (response);
> +
> +  =  return rc;

Cleanups are great but please send them as a separ= ate patch, this helps people
a lot when reviewing patches, if it is just= a cleanup then people can focus on
checking that you didn't change beha= viour OR if you're sending a functional
change patch people can focus on= checking that you're achieving what you set
out to do.

> &nbs= p;}
>  
>  
>  //-------------------------= ---------------------------------------------
>  // Chassis Cont= rol commands
>  //----------------------------------------------= ------------------------
> -ipmi=5Fret=5Ft ipmi=5Fchassis=5Fcontrol(i= pmi=5Fnetfn=5Ft netfn, ipmi=5Fcmd=5Ft cmd,
> -       &= nbsp;                ipmi=5Frequest= =5Ft request, ipmi=5Fresponse=5Ft response,
> -       =                  ipmi=5Fdata= =5Flen=5Ft data=5Flen, ipmi=5Fcontext=5Ft context)
> +ipmi=5Fret=5Ft = ipmi=5Fchassis=5Fcontrol (ipmi=5Fnetfn=5Ft netfn, ipmi=5Fcmd=5Ft cmd,
&g= t; +                     =             ipmi=5Frequest=5Ft request, ipmi= =5Fresponse=5Ft response,
> +           &nbs= p;                     ip= mi=5Fdata=5Flen=5Ft data=5Flen, ipmi=5Fcontext=5Ft context)
>  {=
> - // Error from power off.
> - int rc =3D 0;
> +  = ;  // Error from power off.
> +    int rc =3D 0;
&g= t;  
> - // No response for this command.
> +    = ;// No response for this command.
>      *data=5Flen = =3D 0;
>  
> - // Catch the actual operaton by peeking int= o request buffer
> - uint8=5Ft chassis=5Fctrl=5Fcmd =3D *(uint8=5Ft *= )request;
> - printf("Chassis Control Command: Operation:[0x%X]\n",ch= assis=5Fctrl=5Fcmd);
> -
> - switch(chassis=5Fctrl=5Fcmd)
&g= t; - {
> - case CMD=5FPOWER=5FOFF:
> - rc =3D ipmi=5Fchassis=5F= power=5Fcontrol("powerOff");
> - break;
> - case CMD=5FHARD=5FR= ESET:
> - rc =3D ipmi=5Fchassis=5Fpower=5Fcontrol("reboot");
> = - break;
> - default:
> - {
> - fprintf(stderr, "Invalid = Chassis Control command:[0x%X] received\n",chassis=5Fctrl=5Fcmd);
> -= rc =3D -1;
> - }
> - }
> -
> - return ( (rc < 0= ) ? IPMI=5FCC=5FINVALID : IPMI=5FCC=5FOK);
> +    // Catch = the actual operaton by peeking into request buffer
> +    u= int8=5Ft chassis=5Fctrl=5Fcmd =3D * (uint8=5Ft *)request;
> +   =  printf ("Chassis Control Command: Operation:[0x%X]\n", chassis=5Fctrl= =5Fcmd);
> +
> +    switch (chassis=5Fctrl=5Fcmd) {> +    case CMD=5FPOWER=5FOFF:
> +      = ;  rc =3D ipmi=5Fchassis=5Fpower=5Fcontrol ("powerOff");
> + &nb= sp;      break;
> +
> +    case CMD=5F= HARD=5FRESET:
> +        rc =3D ipmi=5Fchassis=5F= power=5Fcontrol ("reboot");
> +        break;
= > +
> +    default: {
> +       &nb= sp;fprintf (stderr, "Invalid Chassis Control command:[0x%X] received\n", ch= assis=5Fctrl=5Fcmd);
> +        rc =3D -1;
>= ; +    }
> +    }
> +
> +   &nbs= p;return ((rc < 0) ? IPMI=5FCC=5FINVALID : IPMI=5FCC=5FOK);
> &nbs= p;}
>  
> -ipmi=5Fret=5Ft ipmi=5Fchassis=5Fget=5Fsys=5Fboo= t=5Foptions(ipmi=5Fnetfn=5Ft netfn, ipmi=5Fcmd=5Ft cmd,
> -   &n= bsp;                     =      ipmi=5Frequest=5Ft request, ipmi=5Fresponse=5Ft respons= e,
> -                  =            ipmi=5Fdata=5Flen=5Ft data=5Flen,= ipmi=5Fcontext=5Ft context)
> +ipmi=5Fret=5Ft ipmi=5Fchassis=5Fget= =5Fsys=5Fboot=5Foptions (ipmi=5Fnetfn=5Ft netfn, ipmi=5Fcmd=5Ft cmd,
>= ; +        ipmi=5Frequest=5Ft request, ipmi=5Fresponse= =5Ft response,
> +        ipmi=5Fdata=5Flen=5Ft d= ata=5Flen, ipmi=5Fcontext=5Ft context)
>  {
>    = ;  ipmi=5Fret=5Ft rc =3D IPMI=5FCC=5FOK;
>      *= data=5Flen =3D 0;
>  
> -    printf("IPMI GET=5F= SYS=5FBOOT=5FOPTIONS\n");
> +    printf ("IPMI GET=5FSYS=5F= BOOT=5FOPTIONS\n");
> +
> +    get=5Fsys=5Fboot=5Fopt= ions=5Ft * reqptr =3D (get=5Fsys=5Fboot=5Foptions=5Ft *) request;
> +=
> +    /*
> +     * To represent a hex in = string, e.g., "A0A0", which represents two bytes
> +     * = in the hex, but requires 5 bytes to store it as string, i.e., 4
> + &= nbsp;   * characters to store the "A0A0" and 1 additional space for "\= 0".
> +     * Thereby we have 2 * <number of bytes> += 1 for the string buffer.
> +     */
> +    = ;char buf[NUM=5FRETURN=5FBYTES=5FOF=5FGET * 2 + 1] =3D {0};

Once aga= in, why not #define NUM=5FRETURN=5FBYTES=5FOF=5FGET 11

>  > -    get=5Fsys=5Fboot=5Foptions=5Ft *reqptr =3D (get=5Fsys= =5Fboot=5Foptions=5Ft*) request;
> +    /*
> +  =   * Parameter #5 means boot flags. Please refer to 28.13 of ipmi doc.=
> +     * This is the only parameter used by petitboot.> +     */
> +    if (reqptr->parameter = =3D=3D 5) {
> +        int r =3D dbus=5Fget=5Fpro= perty (buf);
>  
> -    // TODO Return default v= alues to OPAL until dbus interface is available
> +     &nb= sp;  if (r < 0) {
> +           &nbs= p;fprintf (stderr, "Dbus get property failed for get=5Fsys=5Fboot=5Foptions= .\n");
> +            return IPMI=5FOUT= =5FOF=5FSPACE;
> +        }
>  
>= ; -    if (reqptr->parameter =3D=3D 5) // Parameter #5
>= -    {
> -        uint8=5Ft buf[] =3D = {0x1,0x5,80,0,0,0,0};
> -        *data=5Flen =3D = sizeof(buf);
> -        memcpy(response, &buf= , *data=5Flen);
> +        uint64=5Ft return=5Fva= lue;
> +        sscanf (buf, "%llX", &return= =5Fvalue);

See my comment at the top about the best tool for the job= .

> +
> +        *data=5Flen =3D NUM=5F= RETURN=5FBYTES=5FOF=5FGET;
> +        /*
> = +         * TODO: last 3 bytes
> +     =     *(NUM=5FRETURN=5FBYTES=5FOF=5FGET - NUM=5FRETURN=5FBYTES=5FOF= =5FGET=5FUSED) is meanlingless
> +         */
= > +        memcpy (response, (uint8=5Ft *) (&ret= urn=5Fvalue), *data=5Flen);

So this interests me, does whoever is go= ing to be reading response know to read
it as a uint64=5Ft on success bu= t as an array of chars on failure, also, rather
than #defineing, wouldn'= t:

*data=5Flen =3D sizeof(return=5Fvalue);

be better?

=
I've done some looking around in ipmid.C and I don't think what you've = done
will work,
https://github.com/open= bmc/phosphor-host-ipmid/blob/master/ipmid.C#L276
appears to use the = first byte of response as the completion code but you've
just written wh= atever the first byte of your uint64=5Ft is into response[0]...

Also= , 28.13 in the IPMI spec doesn't seem to match up with whats going on here,=
am I missing something?

> +    } else {
> + &= nbsp;      *data=5Flen =3D NUM=5FRETURN=5FBYTES=5FOF=5FGET;<= br>> +        // Parameter not supported
> + &= nbsp;      buf[0] =3D IPMI=5FCC=5FPARM=5FNOT=5FSUPPORTED;> +        memcpy (response, buf, *data=5Flen);
= > +        fprintf (stderr, "Unsupported parameter 0= x%x\n", reqptr->parameter);
> +        return = IPMI=5FCC=5FPARM=5FNOT=5FSUPPORTED;
>      }
> -=    else
> -    {
> -       =  fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter)= ;
> -        return IPMI=5FCC=5FPARM=5FNOT=5FSUPP= ORTED;        
> +
> +    return = rc;
> +}
> +
> +ipmi=5Fret=5Ft ipmi=5Fchassis=5Fset=5Fsys= =5Fboot=5Foptions (ipmi=5Fnetfn=5Ft netfn, ipmi=5Fcmd=5Ft cmd,
> + &n= bsp;      ipmi=5Frequest=5Ft request, ipmi=5Fresponse=5Ft re= sponse,
> +        ipmi=5Fdata=5Flen=5Ft data=5Fl= en, ipmi=5Fcontext=5Ft context)
> +{
> +    ipmi=5Fre= t=5Ft rc =3D IPMI=5FCC=5FOK;
> +
> +    printf ("IPMI= SET=5FSYS=5FBOOT=5FOPTIONS\n");
> +    printf ("IPMID set = command required return bytes: %i\n", *data=5Flen);
> +
> + &nb= sp;  set=5Fsys=5Fboot=5Foptions=5Ft * reqptr =3D (set=5Fsys=5Fboot=5Fo= ptions=5Ft *) request;
> +
> +    char output=5Fbuf[N= UM=5FRETURN=5FBYTES=5FOF=5FSET] =3D {0};
> +
> +    /= *
> +     * Parameter #5 means boot flags. Please refer to = 28.13 of ipmi doc.
> +     * This is the only parameter use= d by petitboot.
> +     */
> +    if (reqpt= r->parameter =3D=3D 5) {
> +        /*
>= +         * To represent a hex in string, e.g., "A0A0"= , which represents two bytes
> +         * in the= hex, but requires 5 bytes to store it as string, i.e., 4
> +   =       * characters to store the "A0A0" and 1 additional spac= e for "\0".
> +         * Thereby we have 2 * <= ;number of bytes> + 1 for the string buffer.
> +     &nb= sp;   */
> +        char input=5Fbuf[NUM=5FI= NPUT=5FBYTES=5FOF=5FSET * 2 + 1];
> +        spri= ntf (input=5Fbuf, "%llX", reqptr->data.data64);
> +
> + &nbs= p;      int r =3D dbus=5Fset=5Fproperty (input=5Fbuf);
&g= t; +
> +        if (r < 0) {
> +   =          fprintf (stderr, "Dbus set property faile= d for set=5Fsys=5Fboot=5Foptions.\n");
> +        = ;    return IPMI=5FOUT=5FOF=5FSPACE;
> +     &nbs= p;  }
> +
> +        *data=5Flen =3D N= UM=5FRETURN=5FBYTES=5FOF=5FSET;
> +        // Ret= urn code OK.
> +        output=5Fbuf[0] =3D IPMI= =5FOK;
> +        memcpy (response, output=5Fbuf,= *data=5Flen);
> +    } else {
> +     &nbs= p;  // Parameter not supported
> +        ou= tput=5Fbuf[0] =3D IPMI=5FCC=5FPARM=5FNOT=5FSUPPORTED;
> +   &nbs= p;    memcpy (response, output=5Fbuf, *data=5Flen);
> + &nb= sp;      fprintf (stderr, "Unsupported parameter 0x%x\n", re= qptr->parameter);
> +        return IPMI=5FCC= =5FPARM=5FNOT=5FSUPPORTED;

As far as I can see you're simply using o= utput=5Fbuf to assign the first byte of
response to either IPMI=5FOK or = IPMI=5FCC=5FPARM=5FNOT=5FSUPPORTED and you're using
memcpy.

Would= n't it be nicer to
memset(response, 0, *data=5Flen);
(or not even mem= set since NUM=5FRETURN=5FBYTES=5FOF=5FSET is 1) and
*response =3D IPMI= =5FOK;
or
*response =3D IPMI=5FCC=5FPARM=5FNOT=5FSUPPORTED;
and av= oid having output=5Fbuf at all?

Which makes me wonder why response i= s actually a void *, is this a good idea?

>      }=
>  
>      return rc;
>  }
&= gt;  
> -void register=5Fnetfn=5Fchassis=5Ffunctions()
> += void register=5Fnetfn=5Fchassis=5Ffunctions ()
>  {
> - &n= bsp;  printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN=5FCHASSIS= , IPMI=5FCMD=5FWILDCARD);
> -    ipmi=5Fregister=5Fcallback= (NETFUN=5FCHASSIS, IPMI=5FCMD=5FWILDCARD, NULL, ipmi=5Fchassis=5Fwildcard);=
> +    printf ("Registering NetFn:[0x%X], Cmd:[0x%X]\n", N= ETFUN=5FCHASSIS, IPMI=5FCMD=5FWILDCARD);
> +    ipmi=5Fregi= ster=5Fcallback (NETFUN=5FCHASSIS, IPMI=5FCMD=5FWILDCARD, NULL, ipmi=5Fchas= sis=5Fwildcard);
>  
> -    printf("Registering = NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN=5FCHASSIS, IPMI=5FCMD=5FGET=5FSYS=5FBOOT= =5FOPTIONS);
> -    ipmi=5Fregister=5Fcallback(NETFUN=5FCHA= SSIS, IPMI=5FCMD=5FGET=5FSYS=5FBOOT=5FOPTIONS, NULL, ipmi=5Fchassis=5Fget= =5Fsys=5Fboot=5Foptions);
> +    printf ("Registering NetFn= :[0x%X], Cmd:[0x%X]\n", NETFUN=5FCHASSIS, IPMI=5FCMD=5FGET=5FSYS=5FBOOT=5FO= PTIONS);
> +    ipmi=5Fregister=5Fcallback (NETFUN=5FCHASSI= S, IPMI=5FCMD=5FGET=5FSYS=5FBOOT=5FOPTIONS, NULL, ipmi=5Fchassis=5Fget=5Fsy= s=5Fboot=5Foptions);
>  
> -    printf("Register= ing NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN=5FCHASSIS, IPMI=5FCMD=5FCHASSIS=5FCO= NTROL);
> -    ipmi=5Fregister=5Fcallback(NETFUN=5FCHASSIS,= IPMI=5FCMD=5FCHASSIS=5FCONTROL, NULL, ipmi=5Fchassis=5Fcontrol);
> +=    printf ("Registering NetFn:[0x%X], Cmd:[0x%X]\n", NETFUN=5FCH= ASSIS, IPMI=5FCMD=5FSET=5FSYS=5FBOOT=5FOPTIONS);
> +    ipm= i=5Fregister=5Fcallback (NETFUN=5FCHASSIS, IPMI=5FCMD=5FSET=5FSYS=5FBOOT=5F= OPTIONS, NULL, ipmi=5Fchassis=5Fset=5Fsys=5Fboot=5Foptions);
> +
&= gt; +    printf ("Registering NetFn:[0x%X], Cmd:[0x%X]\n", NETFUN= =5FCHASSIS, IPMI=5FCMD=5FCHASSIS=5FCONTROL);
> +    ipmi=5F= register=5Fcallback (NETFUN=5FCHASSIS, IPMI=5FCMD=5FCHASSIS=5FCONTROL, NULL= , ipmi=5Fchassis=5Fcontrol);
>  }
> +
> diff --git a= /chassishandler.h b/chassishandler.h
> index 1a26411..a310741 100644<= br>> --- a/chassishandler.h
> +++ b/chassishandler.h
> @@ -3= ,21 +3,39 @@
>  
>  #include <stdint.h>
>=  
> +// TODO: Petitboot requires 8 bytes of response
> +/= / however only 5 of them are used. The remaining
> +// 3 bytes are no= t used in petitboot and the value
> +// of them are all zero.
>= +#define NUM=5FRETURN=5FBYTES=5FOF=5FGET 8
> +#define NUM=5FRETURN= =5FBYTES=5FOF=5FGET=5FUSED 5
> +#define NUM=5FRETURN=5FBYTES=5FOF=5FS= ET 1
> +#define NUM=5FINPUT=5FBYTES=5FOF=5FSET 5
> +
> &n= bsp;// IPMI commands for Chassis net functions.
>  enum ipmi=5Fn= etfn=5Fapp=5Fcmds
>  {
>   // Chassis Control
>=   IPMI=5FCMD=5FCHASSIS=5FCONTROL  =3D 0x02,
>    = ;  // Get capability bits
> +    IPMI=5FCMD=5FSET=5FSY= S=5FBOOT=5FOPTIONS =3D 0x08,
>      IPMI=5FCMD=5FGET= =5FSYS=5FBOOT=5FOPTIONS =3D 0x09,
>  };
>  
> &= nbsp;// Command specific completion codes
>  enum ipmi=5Fchassis= =5Freturn=5Fcodes
>  {
> +    IPMI=5FOK =3D 0x0,=
>      IPMI=5FCC=5FPARM=5FNOT=5FSUPPORTED =3D 0x80,>  };
>  
> +// Generic completion codes,
>= ; +// see IPMI doc section 5.2
> +enum ipmi=5Fgeneric=5Freturn=5Fcode= s
> +{
> +    IPMI=5FOUT=5FOF=5FSPACE =3D 0xC4,
&g= t; +};
> +
>  // Various Chassis operations under a single= command.
>  enum ipmi=5Fchassis=5Fcontrol=5Fcmds : uint8=5Ft>  {