From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e23smtp02.au.ibm.com (e23smtp02.au.ibm.com [202.81.31.144]) (using TLSv1 with cipher CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id B19CD1A11A7 for ; Wed, 6 Jan 2016 18:26:23 +1100 (AEDT) Received: from localhost by e23smtp02.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 6 Jan 2016 17:26:22 +1000 Received: from d23dlp01.au.ibm.com (202.81.31.203) by e23smtp02.au.ibm.com (202.81.31.208) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Wed, 6 Jan 2016 17:26:20 +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 4A3242CE8055 for ; Wed, 6 Jan 2016 18:26:20 +1100 (EST) Received: from d23av06.au.ibm.com (d23av06.au.ibm.com [9.190.235.151]) by d23relay10.au.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u067QCPZ61931598 for ; Wed, 6 Jan 2016 18:26:20 +1100 Received: from d23av06.au.ibm.com (localhost [127.0.0.1]) by d23av06.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u067PmI8020216 for ; Wed, 6 Jan 2016 18:25:48 +1100 Received: from e39.co.us.ibm.com (e39.boulder.ibm.com [9.17.249.49]) by d23av06.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id u067PjRc019318 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 6 Jan 2016 18:25:46 +1100 Message-Id: <201601060725.u067PjRc019318@d23av06.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 ; Wed, 6 Jan 2016 00:25:21 -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) Wed, 6 Jan 2016 00:25:19 -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 07:25:18 -0000 Received: from us1a3-smtp01.a3.dal06.isc4sb.com (10.106.154.100) by smtp.notes.na.collabserv.com (10.106.227.143) with smtp.notes.na.collabserv.com ESMTP; Wed, 6 Jan 2016 07:25:16 -0000 Received: from us1a3-mail141.a3.dal06.isc4sb.com ([10.146.38.85]) by us1a3-smtp01.a3.dal06.isc4sb.com with ESMTP id 2016010607255659-50803 ; Wed, 6 Jan 2016 07:25:56 +0000 Subject: Re: [PATCH phosphor-host-ipmid v4] Add get/set boot option ipmid command support with correct DBUS property handling. In-Reply-To: <20160106175701.196f6a77@camb691> From: "Peng Fei BG Gou" To: cyrilbur@gmail.com Cc: openbmc-patches@stwcx.xyz, openbmc@lists.ozlabs.org Date: Wed, 6 Jan 2016 07:25:15 +0000 Sensitivity: MIME-Version: 1.0 References: <20160106175701.196f6a77@camb691>, <20160106160934.493e446f@camb691><1451956226-19954-1-git-send-email-openbmc-patches@stwcx.xyz><1451956226-19954-2-git-send-email-openbmc-patches@stwcx.xyz><201601060613.u066Dbgg023586@d23av03.au.ibm.com> Importance: Normal X-Priority: 3 (Normal) X-Mailer: Lotus Domino Web Server Build V851SAAS_12072015_FP3 December 17, 2015 X-LLNOutbound: False X-Disclaimed: 29915 X-TNEFEvaluated: 1 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable x-cbid: 16010607-0005-0000-0000-0000030A0386 X-IBM-ISS-SpamDetectors: Score=0.4332; BY=0.041088; FL=0; FP=0; FZ=0; HX=0; KW=0; PH=0; SC=0.4332; 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.00641224; UDB=6.00288631; UTC=2016-01-06 07:25:17 x-cbparentid: 16010607-4778-0000-0000-000001767438 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 07:26:23 -0000
Hey Cyril,
 
For the whitespace introduced and the coding style issue,= I will try to propose a fix to it, hopefully with minimum coding style cha= nges.
 
For other issues, we can continue discuss it but I believ= e we can firstly accept this patch and then revisit them if there is really= concerns for them.
 
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: Peng Fei BG Gou/China/IBM@IBMCN
Cc: openbmc@li= sts.ozlabs.org, openbmc-patches@stwcx.xyz
Subject: Re: [PATCH phosphor-h= ost-ipmid v4] Add get/set boot option ipmid command support with correct DB= US property handling.
Date: Wed, Jan 6, 2016 3:12 PM
 
On Wed, 6 Jan 2016 06:13:06 +0000
"Peng Fei BG Gou" <shgoupf@cn.= ibm.com> wrote:

> Hey Cyril,
>  
> Thanks for= your detailed review on this patch.
>  

Hi Peng,

= > Just want to answer (most of) your questions in batch:
> 1) This= patch intended to add the functionality of ipmi set/get boot option, with = dbus property and object mapper used, so I believe all of the contents in t= his patch should be considered as a whole. We shouldn't split them into mul= tiple patches.

The set/get boot option is fine as one patch (or two.= .. not too fussed really) I
agree.

You do still have whitespace (= and other) changes which touch code not related
to the boot option work,= notably in ipmi=5Fchassis=5Fpower=5Fcontrol() and
ipmi=5Fchassis=5Fcont= rol()

> 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.

I'm not saying sscanf= won't work, I completely agree that it will. I do
disagree that there i= sn't a performance concern, if I'm not mistaken this code
will be runnin= g on an ASPEED2400 which is a single core ARM chip running at
400MHz. We= have breathing space and we don't need to write absolutely
everything t= o save all the possible cycles but lets not waste the little
breathing s= pace we do have.

> 3) For those sd=5Fbus method call with signatu= res ("s", "v", "s{ass}", etc.), I believe the current way I use it is valid= since I have tested them already.

Again, I'm not saying it won't wo= rk but I'm trying to understand why the
complexity in these signatures, = it doesn't seem like it's being used and
pointless complexity doesn't he= lp anyone.

However, if there is a good reason for these complicated = signatures I'm all for
it, if you could document them in
https://github.com/openbmc/docs/blob/master/dbus-interfaces.md that would be
fantastic.

Thanks.

> 4) For the c= ommented out error handling, I believe it's just 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 h= andled, I believe it is working fine.
> 6) For the coding style stuff= , such as spaces between function 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.
>  

It isn't a major issue at all but this projec= t already has enough code style
issues as it is, I don't see why patches= should be merged that introduce more
style problems.

Perhaps you= should run your auto style tool on your patch(es) before submission?
> In a sum, thanks for your effort on reviewing this, but I believe mo= st 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 <cyrilbur@= gmail.com>
> To: OpenBMC Patches <openbmc-patches@stwcx.xyz>= , Peng Fei BG Gou/China/IBM@IBMCN
> Cc: openbmc@lists.ozlabs.org
&= gt; Subject: Re: [PATCH phosphor-host-ipmid v4] Add get/set boot option ipm= id command support with correct DBUS property handling.
> Date: Wed, = Jan 6, 2016 1:10 PM
>  
> On Mon,  4 Jan 2016 19:10:2= 6 -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 option from the dbus
> >     property.
> = >     c) IPMI set boot option coomand set boot option to the d= bus
> >     property.
> > 2) Two methods to han= dle the dbus property set and get:
> >     a) dbus=5Fset= =5Fproperty()
> >     b) dbus=5Fget=5Fproperty()
>= > 3) The property is stored as a 10 character strings which representsd= 5-byte information.
> > 4) ipmid set method is registered and imp= lemented since petitboot will use it to clear the boot options.
> >= ; 5) Get service name via object mapper
> >     a) The c= onnection name is got via objectmapper.
> >     b) The m= ethod used to get the connection name is object=5Fmapper=5Fget=5Fconnection= ().
> >     c) dbus=5Fget=5Fproperty/dbus=5Fset=5Fproper= ty will get the connection name via the above method instead of hard coding= .
> > 6) Error code are properly handled.
> > 7) Use spri= nf/sscanf for int/string conversion.
> >     a) Instead = of reinventing the wheel by defining methods converting int to string, use = sprintf/sscanf should be a more clean and robust way.
>
> Plus = 1 for not reinventing the wheel but perhaps consider using the best tool> for the job, sscanf can and does do what you want however atoi, atol,= atoll,
> strtol, strtoll, strtoq functions are all designed for this= exact purpose. They
> will perform much better than sscanf, also the= str* versions give great control
> and error detection capabilities.=
>
>
> > 8) Fix issues addressed by the community.
= > >     a) change malloc heap to stack local variable.
&= gt; >     b) Fix multi line comment style from "//" to "/**/".=
> >     c) Add defines for return codes.
> > &= nbsp;   d) Add more comments.
>
> It looks like you're add= ing more and more into this same patch. A single patch
> should be on= e clearly defined piece of work. So you're implementing some new
> fu= nctionality, that 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 sho= uld also be their own
> patch.
>
> >  chassishand= ler.C | 492 +++++++++++++++++++++++++++++++++++++++++++++----------
>= >  chassishandler.h |  18 ++
> >  2 files chang= ed, 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>
> >  
> >  // OpenBMC Chassis Manager dbu= s framework
> > -const char  *chassis=5Fbus=5Fname   &nb= sp;  =3D  "org.openbmc.control.Chassis";
> > -const char=  *chassis=5Fobject=5Fname   =3D  "/org/openbmc/control/chas= sis0";
> > -const char  *chassis=5Fintf=5Fname     = =3D  "org.openbmc.control.Chassis";
> > +const char * chassis= =5Fbus=5Fname      =3D  "org.openbmc.control.Chassis";<= br>> > +const char * chassis=5Fobject=5Fname   =3D  "/org/o= penbmc/control/chassis0";
> > +const char * chassis=5Fintf=5Fname =     =3D  "org.openbmc.control.Chassis";
> >  <= br>>
> Not necessary, if anything you should remove the double spa= ce but what you've
> done doesn't look good.
>
> > -vo= id register=5Fnetfn=5Fchassis=5Ffunctions() =5F=5Fattribute=5F=5F((construc= tor));
> > +// Host settings in dbus
> > +// Service name= should be referenced by connection name got via object mapper
> >= +const char * settings=5Fobject=5Fname  =3D  "/org/openbmc/setti= ngs/host0";
> > +const char * settings=5Fintf=5Fname    = =3D  "org.freedesktop.DBus.Properties";
> > +
> > +c= onst char * objmapper=5Fservice=5Fname =3D  "org.openbmc.objectmapper"= ;
> > +const char * objmapper=5Fobject=5Fname  =3D  "/or= g/openbmc/objectmapper/objectmapper";
> > +const char * objmapper= =5Fintf=5Fname    =3D  "org.openbmc.objectmapper.ObjectMappe= r";
> > +
>
> We probably want to try to keep a consis= tent style. I looked at ipmid.C and
> that file isn't consistent but = it seems to be mostly "type *name;" and that's
> the most common way = everywhere else with C code.
>
> > +int object=5Fmapper=5Fge= t=5Fconnection (char ** buf, const char * obj=5Fpath)
> > +{
&g= t; > +    sd=5Fbus=5Ferror error =3D SD=5FBUS=5FERROR=5FNULL;<= br>> > +    sd=5Fbus=5Fmessage * m =3D NULL;
> > +=    sd=5Fbus * bus =3D NULL;
> > +    char * t= emp=5Fbuf =3D NULL, *intf =3D NULL;
> > +    size=5Ft bu= f=5Fsize =3D 0;
> > +    int r;
> > +
> &= gt; +    // Open the system bus where most system services are pr= ovided.
> > +    r =3D sd=5Fbus=5Fopen=5Fsystem (&bu= s);
> > +
>
> As an added stylistic note, again for co= nsistency, ipmid.C, you should avoid
> having a space between the fun= ction name and the parameters
>
> > +    if (r <= ; 0) {
> > +        fprintf (stderr, "Failed t= o connect to system bus: %s\n", strerror (-r));
> > +    = ;    goto finish;
> > +    }
> > +> > +    /*
> > +     * Bus, service, o= bject path, interface and method are provided to call
> > +  =   * the method.
> > +     * Signatures and input a= rguments are provided by the arguments at the
> > +     = * end.
> > +     */
> > +    r =3D sd= =5Fbus=5Fcall=5Fmethod (bus,
> > +         &nb= sp;                  objmapper= =5Fservice=5Fname,                 =      /* service to contact */
> > +     &= nbsp;                    =  objmapper=5Fobject=5Fname,            =           /* object path */
> > +  =                     &nbs= p;    objmapper=5Fintf=5Fname,          =               /* interface name */
&= gt; > +                   &= nbsp;        "GetObject",         &= nbsp;                    =   /* method name */
> > +          =                  &error, =                      = ;                /* object to retur= n error in */
> > +             &nbs= p;              &m,     &n= bsp;                     =                /* return message on= success */
> > +              =              "s",       =                      = ;             /* input signature */
> &= gt; +                    =        obj=5Fpath           &= nbsp;                    =     /* first argument */
> > +       &nb= sp;                   );
&g= t; > +
> > +    if (r < 0) {
> > +  =      fprintf (stderr, "Failed to issue method call: %s\n", = error.message);
> > +        goto finish;
&= gt; > +    }
> > +
> > +    // Get= the key, aka, the connection name
> > +    sd=5Fbus=5Fm= essage=5Fread (m, "a{sas}", 1, &temp=5Fbuf, 1, &intf);
> >= +
>
> This seems very roundabout, could you explain this metho= d call. What are you
> expecting to receive? From the looks of what y= ou're using it for wouldn't it
> make more sense to just sd=5Fbus=5Fm= essage=5Fread a "s"?
>
> > +    /*
> > + =     * TODO: check the return code. Currently for no reason the me= ssage
> > +     * parsing of object mapper is always com= plaining about
> > +     * "Device or resource busy", bu= t the result seems OK for now. Need
> > +     * further = checks.
>
> If sd=5Fbus=5Fmessage=5Fread returns a negative val= ue, 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 can be<= br>> > +     * resumed after the problem aforementioned is = resolved.
> > +     *r =3D sd=5Fbus=5Fmessage=5Fread(m, = "a{sas}", 1, &temp=5Fbuf, 1, &intf);
> > +     *= if (r < 0) {
> > +     *    fprintf(stderr,= "Failed to parse response message: %s\n", strerror(-r));
> > + &n= bsp;   *    goto finish;
> > +     *}
= >
> The code is in git, it is saved there, delete it now and when = someone wants to
> add it back they can look in the git history for w= hat was deleted. Having said
> that, the error check should be there,= if you're hitting it, you should figure
> out why.
>
> &= gt; +     */
> > +
> > +    buf=5Fsiz= e =3D strlen (temp=5Fbuf) + 1;
> > +    printf ("IPMID c= onnection name: %s\n", temp=5Fbuf);
> > +    *buf =3D (c= har *)malloc (buf=5Fsize);
> > +
> > +    if (*= buf =3D=3D NULL) {
> > +        fprintf (stder= r, "Malloc failed for get=5Fsys=5Fboot=5Foptions");
> > +   &= nbsp;    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.
>
> > + &nb= sp;  memcpy (*buf, temp=5Fbuf, buf=5Fsize);
> > +
> >= ; +finish:
> > +    sd=5Fbus=5Ferror=5Ffree (&error)= ;
> > +    sd=5Fbus=5Fmessage=5Funref (m);
> > = +    sd=5Fbus=5Funref (bus);
> > +
>
> Space= s between name and params
>
> > +    return r;
= > > +}
> > +
> > +int dbus=5Fget=5Fproperty (char *= buf)
> > +{
> > +    sd=5Fbus=5Ferror error = =3D SD=5FBUS=5FERROR=5FNULL;
> > +    sd=5Fbus=5Fmessage= * m =3D NULL;
> > +    sd=5Fbus * bus =3D NULL;
>= > +    char * temp=5Fbuf =3D NULL;
> > +   &nbs= p;uint8=5Ft * get=5Fvalue =3D NULL;
> > +    char * conn= ection =3D NULL;
> > +    int r, i;
> > +
&g= t; > +    r =3D object=5Fmapper=5Fget=5Fconnection (&conne= ction, settings=5Fobject=5Fname);
> > +
> > +   &nbs= p;if (r < 0) {
> > +        fprintf (stderr= , "Failed to get connection, return value: %d.\n", r);
> > +  = ;      goto finish;
> > +    }
> &g= t; +
> > +    printf ("connection: %s\n", connection);> > +
> > +    // Open the system bus where most= system services are provided.
> > +    r =3D sd=5Fbus= =5Fopen=5Fsystem (&bus);
> > +
> > +    if = (r < 0) {
> > +        fprintf (stderr, "Fa= iled to connect to system bus: %s\n", strerror (-r));
> > +  =      goto finish;
> > +    }
> >= ; +
> > +    /*
> > +     * Bus, serv= ice, object path, interface and method are provided to call
> > + =     * the method.
> > +     * Signatures and i= nput arguments are provided by the arguments at the
> > +   &= nbsp; * end.
> > +     */
> > +    r = =3D sd=5Fbus=5Fcall=5Fmethod (bus,
> > +       &nbs= p;                    con= nection,                   &nb= sp;             /* service to contact */
&= gt; > +                   &= nbsp;        settings=5Fobject=5Fname,     &n= bsp;                 /* object path= */
> > +                 =            settings=5Fintf=5Fname,   &nb= sp;                     /= * interface name */
> > +            = ;                "Get",   &nbs= p;                     &n= bsp;            /* method name */
> >= ; +                     &= nbsp;      &error,           &n= bsp;                     =     /* object to return error in */
> > +     =                      = ;  &m,                 &nb= sp;                     &= nbsp; /* return message on success */
> > +       &= nbsp;                    = "ss",                    =                   /* input si= gnature */
> > +               =              settings=5Fintf=5Fname, &nb= sp;                     &= nbsp; /* first argument */
> > +          = ;                  "boot=5Ffla= gs");                    =          /* second argument */
> > +
= > > +    if (r < 0) {
> > +      = ;  fprintf (stderr, "Failed to issue method call: %s\n", error.message= );
> > +        goto finish;
> > + &n= bsp;  }
> > +
> > +    /*
> > + &= nbsp;   * The output should be parsed exactly the same as the output f= ormatting
> > +     * specified.
> > +   &= nbsp; */
> > +    r =3D sd=5Fbus=5Fmessage=5Fread (m, "v= ", "s", &temp=5Fbuf);
> > +
>
> Why not just read = a "s"?
>
> > +    if (r < 0) {
> > + &= nbsp;      fprintf (stderr, "Failed to parse response messag= e: %s\n", strerror (-r));
> > +        goto fi= nish;
> > +    }
> > +
> > +   &n= bsp;printf ("IPMID boot option property get: {%s}.\n", (char *) temp=5Fbuf)= ;
> > +
> > +    /*
> > +    = ; * To represent a hex in string, e.g., "A0A0", which represents two bytes<= br>> > +     * in the hex, but requires 5 bytes to store it= as string, i.e., 4
> > +     * characters to store the = "A0A0" and 1 additional space for "\0".
> > +     * Ther= eby we have 2 * <number of bytes> + 1 for the string buffer.
> = > +     */
>
> So why don't you have #define NUM= =5FRETURN=5FBYTES=5FOF=5FGET=5FUSED 11 ?
>
> This question is o= ut of scope of this patch but why are we passing around
> numbers as = strings? dbus handles other types...
>
> > +    me= mcpy (buf, temp=5Fbuf, 2 * NUM=5FRETURN=5FBYTES=5FOF=5FGET=5FUSED + 1);
= > > +
> > +finish:
> > +    sd=5Fbus=5Fer= ror=5Ffree (&error);
> > +    sd=5Fbus=5Fmessage=5Fu= nref (m);
> > +    sd=5Fbus=5Funref (bus);
> > = +    free (connection);
> > +
> > +   &nbs= p;return r;
> > +}
> > +
> > +int dbus=5Fset=5Fp= roperty (const char * buf)
> > +{
> > +    sd= =5Fbus=5Ferror error =3D SD=5FBUS=5FERROR=5FNULL;
> > +   &nb= sp;sd=5Fbus=5Fmessage * m =3D NULL;
> > +    sd=5Fbus * = bus =3D NULL;
> > +    char * connection =3D NULL;
&g= t; > +    int r;
> > +
> > +    r = =3D object=5Fmapper=5Fget=5Fconnection (&connection, settings=5Fobject= =5Fname);
> > +
> > +    if (r < 0) {
>= ; > +        fprintf (stderr, "Failed to get connect= ion, return value: %d.\n", r);
> > +        go= to finish;
> > +    }
> > +
> > + &nbs= p;  printf ("connection: %s\n", connection);
> > +
> &g= t; +    // Open the system bus where most system services are pro= vided.
> > +    r =3D sd=5Fbus=5Fopen=5Fsystem (&bus= );
> > +
> > +    if (r < 0) {
> > = +        fprintf (stderr, "Failed to connect to system = bus: %s\n", strerror (-r));
> > +        goto = finish;
> > +    }
> > +
> > +   =  /*
> > +     * Bus, service, object path, interfac= e and method are provided to call
> > +     * the method= .
> > +     * Signatures and input arguments are provide= d by the arguments at the
> > +     * end.
> > = +     */
> > +    r =3D sd=5Fbus=5Fcall=5Fmeth= od (bus,
> > +               &n= bsp;            connection,     &nb= sp;                     &= nbsp;     /* service to contact */
> > +     &= nbsp;                    =  settings=5Fobject=5Fname,             =           /* object path */
> > +   =                      = ;    settings=5Fintf=5Fname,           &= nbsp;             /* interface name */
>= ; > +                   &nb= sp;        "Set",           &n= bsp;                     =      /* method name */
> > +       &= nbsp;                    = &error,                   =                   /* object to= return error in */
> > +            = ;                &m,   &nb= sp;                     &= nbsp;               /* return message on= success */
> > +              =              "ssv",      = ;                     &nb= sp;          /* input signature */
> > + =                      = ;      settings=5Fintf=5Fname,         &= nbsp;               /* first argument */=
> > +                 &nb= sp;          "boot=5Fflags",       =                      = ;   /* second argument */
> > +         &= nbsp;                  "s", &n= bsp;                     =                  /* third argu= ment */
> > +               &nb= sp;            buf);       &nb= sp;                     &= nbsp;         /* fourth argument */
> > +
&= gt;
> Are we documenting these interfaces at all, I don't understand = why "ssv" and
> then a hardcoded "s", why not "sss"?
>
> = > +    if (r < 0) {
> > +       &nb= sp;fprintf (stderr, "Failed to issue method call: %s\n", error.message);> > +        goto finish;
> > +   =  }
> > +
> > +    printf ("IPMID boot opti= on property set: {%s}.\n", buf);
> > +
> > +finish:
&g= t; > +    sd=5Fbus=5Ferror=5Ffree (&error);
> > +=    sd=5Fbus=5Fmessage=5Funref (m);
> > +    s= d=5Fbus=5Funref (bus);
> > +    free (connection);
&g= t; > +
> > + &nbsp;  return r;
> > +}
>= > +
> > +void register=5Fnetfn=5Fchassis=5Ffunctions () =5F=5F= attribute=5F=5F ((constructor));
> >  
> >  str= uct get=5Fsys=5Fboot=5Foptions=5Ft {
> >      uint8= =5Ft parameter;
> > @@ -17,11 +241,28 @@ struct get=5Fsys=5Fboot= =5Foptions=5Ft {
> >      uint8=5Ft block;
> = >  }  =5F=5Fattribute=5F=5F ((packed));
> >  > > -ipmi=5Fret=5Ft ipmi=5Fchassis=5Fwildcard(ipmi=5Fnetfn=5Ft netfn= , ipmi=5Fcmd=5Ft cmd,
> > -           &nb= sp;                  ipmi=5Fre= quest=5Ft request, ipmi=5Fresponse=5Ft response,
> > -   &nbs= p;                     &n= bsp;    ipmi=5Fdata=5Flen=5Ft data=5Flen, ipmi=5Fcontext=5Ft cont= ext)
> > +struct set=5Fsys=5Fboot=5Foptions=5Ft {
> > + &= nbsp;  uint8=5Ft parameter;
> > +    union {
>= ; > +        struct {
> > +     &= nbsp;      uint8=5Ft d1;
> > +      =      uint8=5Ft d2;
> > +       &nbs= p;    uint8=5Ft d3;
> > +         &n= bsp;  uint8=5Ft d4;
> > +           =  uint8=5Ft d5;
> > +            = ;uint8=5Ft d6;
> > +            uint= 8=5Ft d7;
> > +            uint8=5Ft= d8;
> > +        } data8;
> > + &nbs= p;      uint64=5Ft data64;
> > +    } dat= a;
> > +}  =5F=5Fattribute=5F=5F ((packed));
> > +> > +ipmi=5Fret=5Ft ipmi=5Fchassis=5Fwildcard (ipmi=5Fnetfn=5Ft net= fn, ipmi=5Fcmd=5Ft cmd,
> > +           &= nbsp;                    =  ipmi=5Frequest=5Ft request, ipmi=5Fresponse=5Ft response,
> &g= t; +                     =              ipmi=5Fdata=5Flen=5Ft data= =5Flen, ipmi=5Fcontext=5Ft context)
> >  {
> > - &nb= sp;  printf("Handling CHASSIS WILDCARD Netfn:[0x%X], Cmd:[0x%X]\n",net= fn, cmd);
> > +    printf ("Handling CHASSIS WILDCARD Ne= tfn:[0x%X], Cmd:[0x%X]\n", netfn, cmd);
>
> Be careful with cha= nges like this, you didn't change this code...
>
> >   =    // Status code.
> >      ipmi=5Fret=5F= t rc =3D IPMI=5FCC=5FOK;
> >      *data=5Flen =3D 0= ;
> > @@ -31,116 +272,193 @@ ipmi=5Fret=5Ft ipmi=5Fchassis=5Fwildc= ard(ipmi=5Fnetfn=5Ft netfn, ipmi=5Fcmd=5Ft cmd,
> >  //------= ------------------------------------------------------
> >  /= / Calls into Chassis Control Dbus object to do the power off
> > &= nbsp;//------------------------------------------------------------
>= > -int ipmi=5Fchassis=5Fpower=5Fcontrol(const char *method)
> >= ; +int ipmi=5Fchassis=5Fpower=5Fcontrol (const char * method)
> > =  {
> > - // sd=5Fbus error
> > - int rc =3D 0;
&g= t; > +    // sd=5Fbus error
> > +    int rc= =3D 0;
> >  
> >      // SD Bus erro= r report mechanism.
> >      sd=5Fbus=5Ferror bus= =5Ferror =3D SD=5FBUS=5FERROR=5FNULL;
> >  
> > - //= Response from the call. Although there is no response for this call,
&g= t; > - // obligated to mention this to make compiler happy.
> >= - sd=5Fbus=5Fmessage *response =3D NULL;
> > -
> > - // = Gets a hook onto either a SYSTEM or SESSION bus
> > - sd=5Fbus *bu= s=5Ftype =3D ipmid=5Fget=5Fsd=5Fbus=5Fconnection();
> > -
> = > - rc =3D sd=5Fbus=5Fcall=5Fmethod(bus=5Ftype,       &nb= sp; // On the System Bus
> > - chassis=5Fbus=5Fname,    =    // Service to contact
> > - chassis=5Fobject=5Fname,=     // Object path
> > - chassis=5Fintf=5Fname,   =     // Interface name
> > - method,      = // Method to be called
> > - &bus=5Ferror,     &nbs= p; // object to return error
> > - &response, // Response buff= er if any
> > - NULL); // No input arguments
> > - if(rc = < 0)
> > - {
> > - fprintf(stderr,"ERROR initiating Po= wer Off:[%s]\n",bus=5Ferror.message);
> > - }
> > - else<= br>> > - {
> > - printf("Chassis Power Off initiated success= fully\n");
> > - }
> > -
> > -    sd= =5Fbus=5Ferror=5Ffree(&bus=5Ferror);
> > -    sd=5Fb= us=5Fmessage=5Funref(response);
> > -
> > - return rc;> > +    /*
> > +     * Response from t= he call. Although there is no response for this call,
> > +  =   * obligated to mention this to make compiler happy.
> > + =     */
> > +    sd=5Fbus=5Fmessage * response = =3D NULL;
> > +
> > +    // Gets a hook onto ei= ther a SYSTEM or SESSION bus
> > +    sd=5Fbus * bus=5Ft= ype =3D ipmid=5Fget=5Fsd=5Fbus=5Fconnection ();
> > +
> >= +    rc =3D sd=5Fbus=5Fcall=5Fmethod (bus=5Ftype,     =            /* On the System Bus*/
> >= ; +                     &= nbsp;       chassis=5Fbus=5Fname,        = ;/* Service to contact*/
> > +           =                   chassis=5Fob= ject=5Fname,     /* Object path*/
> > +     &n= bsp;                     =   chassis=5Fintf=5Fname,       /* Interface name*/
&= gt; > +                   &= nbsp;         method,          = ;        /* Method to be called*/
> > +  =                     &nbs= p;     &bus=5Ferror,            = ;  /* object to return error*/
> > +       &nb= sp;                     &= amp;response,               /* Response = buffer if any*/
> > +             &n= bsp;               NULL);     =               /* No input arguments*/> > +
> > +    if (rc < 0) {
> > + &n= bsp;      fprintf (stderr, "ERROR initiating Power Off:[%s]\= n", bus=5Ferror.message);
> > +    } else {
> >= +        printf ("Chassis Power Off initiated successf= ully\n");
> > +    }
> > +
> > +  = ;  sd=5Fbus=5Ferror=5Ffree (&bus=5Ferror);
> > +   &= nbsp;sd=5Fbus=5Fmessage=5Funref (response);
> > +
> > + &= nbsp;  return rc;
>
> Cleanups are great but please send t= hem as a separate patch, this helps people
> a lot when reviewing pat= ches, if it is just a cleanup then people can focus on
> checking tha= t you didn't change behaviour OR if you're sending a functional
> cha= nge patch people can focus on checking that you're achieving what you set> out to do.
>
> >  }
> >  
> = >  
> >  //------------------------------------------= ----------------------------
> >  // Chassis Control commands=
> >  //-----------------------------------------------------= -----------------
> > -ipmi=5Fret=5Ft ipmi=5Fchassis=5Fcontrol(ipm= i=5Fnetfn=5Ft netfn, ipmi=5Fcmd=5Ft cmd,
> > -      = ;                  ipmi=5Frequ= est=5Ft request, ipmi=5Fresponse=5Ft response,
> > -    =                    ipmi= =5Fdata=5Flen=5Ft data=5Flen, ipmi=5Fcontext=5Ft context)
> > +ipm= i=5Fret=5Ft ipmi=5Fchassis=5Fcontrol (ipmi=5Fnetfn=5Ft netfn, ipmi=5Fcmd=5F= t cmd,
> > +               &nbs= p;                 ipmi=5Frequest= =5Ft request, ipmi=5Fresponse=5Ft response,
> > +     &n= bsp;               &nbsp;   &nb= sp;       ipmi=5Fdata=5Flen=5Ft data=5Flen, ipmi=5Fcontext= =5Ft context)
> >  {
> > - // Error from power off.<= br>> > - int rc =3D 0;
> > +    // Error from powe= r off.
> > +    int rc =3D 0;
> >  
>= ; > - // No response for this command.
> > +    // No= response for this command.
> >      *data=5Flen = =3D 0;
> >  
> > - // Catch the actual operaton by p= eeking into request buffer
> > - uint8=5Ft chassis=5Fctrl=5Fcmd = =3D *(uint8=5Ft *)request;
> > - printf("Chassis Control Command: = Operation:[0x%X]\n",chassis=5Fctrl=5Fcmd);
> > -
> > - sw= itch(chassis=5Fctrl=5Fcmd)
> > - {
> > - case CMD=5FPOWER= =5FOFF:
> > - rc =3D ipmi=5Fchassis=5Fpower=5Fcontrol("powerOff");=
> > - break;
> > - case CMD=5FHARD=5FRESET:
> >= - rc =3D ipmi=5Fchassis=5Fpower=5Fcontrol("reboot");
> > - break;=
> > - default:
> > - {
> > - fprintf(stderr, "I= nvalid Chassis Control command:[0x%X] received\n",chassis=5Fctrl=5Fcmd);> > - rc =3D -1;
> > - }
> > - }
> > -> > - return ( (rc < 0) ? IPMI=5FCC=5FINVALID : IPMI=5FCC=5FOK);<= br>> > +    // Catch the actual operaton by peeking into re= quest buffer
> > +    uint8=5Ft chassis=5Fctrl=5Fcmd =3D= * (uint8=5Ft *)request;
> > +    printf ("Chassis Contr= ol Command: Operation:[0x%X]\n", chassis=5Fctrl=5Fcmd);
> > +
&= gt; > +    switch (chassis=5Fctrl=5Fcmd) {
> > + &nbs= p;  case CMD=5FPOWER=5FOFF:
> > +        = rc =3D ipmi=5Fchassis=5Fpower=5Fcontrol ("powerOff");
> > +  =      break;
> > +
> > +    case= CMD=5FHARD=5FRESET:
> > +        rc =3D ipmi= =5Fchassis=5Fpower=5Fcontrol ("reboot");
> > +      = ;  break;
> > +
> > +    default: {
>= ; > +        fprintf (stderr, "Invalid Chassis Contr= ol command:[0x%X] received\n", chassis=5Fctrl=5Fcmd);
> > +  =      rc =3D -1;
> > +    }
> > = +    }
> > +
> > +    return ((rc <= ; 0) ? IPMI=5FCC=5FINVALID : IPMI=5FCC=5FOK);
> >  }
> = >  
> > -ipmi=5Fret=5Ft ipmi=5Fchassis=5Fget=5Fsys=5Fboot= =5Foptions(ipmi=5Fnetfn=5Ft netfn, ipmi=5Fcmd=5Ft cmd,
> > -  = ;                     &nb= sp;      ipmi=5Frequest=5Ft request, ipmi=5Fresponse=5Ft res= ponse,
> > -               &nbs= p;              ipmi=5Fdata=5Flen=5Ft da= ta=5Flen, ipmi=5Fcontext=5Ft context)
> > +ipmi=5Fret=5Ft ipmi=5Fc= hassis=5Fget=5Fsys=5Fboot=5Foptions (ipmi=5Fnetfn=5Ft netfn, ipmi=5Fcmd=5Ft= cmd,
> > +        ipmi=5Frequest=5Ft request,= ipmi=5Fresponse=5Ft response,
> > +        ip= mi=5Fdata=5Flen=5Ft data=5Flen, ipmi=5Fcontext=5Ft context)
> > &n= bsp;{
> >      ipmi=5Fret=5Ft rc =3D IPMI=5FCC=5FOK= ;
> >      *data=5Flen =3D 0;
> >  > > -    printf("IPMI GET=5FSYS=5FBOOT=5FOPTIONS\n");
= > > +    printf ("IPMI GET=5FSYS=5FBOOT=5FOPTIONS\n");
&= gt; > +
> > +    get=5Fsys=5Fboot=5Foptions=5Ft * req= ptr =3D (get=5Fsys=5Fboot=5Foptions=5Ft *) request;
> > +
> = > +    /*
> > +     * To represent a hex in= string, e.g., "A0A0", which represents two bytes
> > +   &nb= sp; * in the hex, but requires 5 bytes to store it as string, i.e., 4
&g= t; > +     * characters to store the "A0A0" and 1 additional s= pace for "\0".
> > +     * Thereby we have 2 * <numbe= r of bytes> + 1 for the string buffer.
> > +     */> > +    char buf[NUM=5FRETURN=5FBYTES=5FOF=5FGET * 2 + 1= ] =3D {0};
>
> Once again, why not #define NUM=5FRETURN=5FBYTES= =5FOF=5FGET 11
>
> >  
> > -    get= =5Fsys=5Fboot=5Foptions=5Ft *reqptr =3D (get=5Fsys=5Fboot=5Foptions=5Ft*) r= equest;
> > +    /*
> > +     * Param= eter #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= =5Fproperty (buf);
> >  
> > -    // TODO = Return default values to OPAL until dbus interface is available
> >= ; +        if (r < 0) {
> > +    =        fprintf (stderr, "Dbus get property failed for = get=5Fsys=5Fboot=5Foptions.\n");
> > +        =    return IPMI=5FOUT=5FOF=5FSPACE;
> > +     =    }
> >  
> > -    if (reqptr-&= gt;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=5Fvalu= e;
> > +        sscanf (buf, "%llX", &retu= rn=5Fvalue);
>
> See my comment at the top about the best tool = for the job.
>
> > +
> > +       &nb= sp;*data=5Flen =3D NUM=5FRETURN=5FBYTES=5FOF=5FGET;
> > +   &= nbsp;    /*
> > +         * TODO: la= st 3 bytes
> > +         *(NUM=5FRETURN=5FBYTE= S=5FOF=5FGET - NUM=5FRETURN=5FBYTES=5FOF=5FGET=5FUSED) is meanlingless
&= gt; > +         */
> > +     &nbs= p;  memcpy (response, (uint8=5Ft *) (&return=5Fvalue), *data=5Flen= );
>
> So this interests me, does whoever is going to be readin= g response know to read
> it as a uint64=5Ft on success but as an arr= ay of chars on failure, also, rather
> than #defineing, wouldn't:
= >
> *data=5Flen =3D sizeof(return=5Fvalue);
>
> be bet= ter?
>
>
> I've done some looking around in ipmid.C and I= don't think what you've done
> will work,
> https://github.com/openbmc/phosphor-host-ipmid/blob/master/ipmi= d.C#L276
> appears to use the first byte of response as the compl= etion code but you've
> just written whatever the first byte of your = uint64=5Ft is into response[0]...
>
> Also, 28.13 in the IPMI s= pec doesn't seem to match up with whats going on here,
> am I missing= something?
>
> > +    } else {
> > + &nb= sp;      *data=5Flen =3D NUM=5FRETURN=5FBYTES=5FOF=5FGET;> > +        // Parameter not supported
> = > +        buf[0] =3D IPMI=5FCC=5FPARM=5FNOT=5FSUPPO= RTED;
> > +        memcpy (response, buf, *dat= a=5Flen);
> > +        fprintf (stderr, "Unsup= ported parameter 0x%x\n", reqptr->parameter);
> > +   &nbs= p;    return IPMI=5FCC=5FPARM=5FNOT=5FSUPPORTED;
> > &nb= sp;    }
> > -    else
> > -   &= nbsp;{
> > -        fprintf(stderr, "Unsupport= ed parameter 0x%x\n", reqptr->parameter);
> > -     &= nbsp;  return IPMI=5FCC=5FPARM=5FNOT=5FSUPPORTED;      =  
> > +
> > +    return rc;
> > = +}
> > +
> > +ipmi=5Fret=5Ft ipmi=5Fchassis=5Fset=5Fsys= =5Fboot=5Foptions (ipmi=5Fnetfn=5Ft netfn, ipmi=5Fcmd=5Ft cmd,
> >= +        ipmi=5Frequest=5Ft request, ipmi=5Fresponse= =5Ft response,
> > +        ipmi=5Fdata=5Flen= =5Ft data=5Flen, ipmi=5Fcontext=5Ft context)
> > +{
> > +=    ipmi=5Fret=5Ft rc =3D IPMI=5FCC=5FOK;
> > +
> = > +    printf ("IPMI SET=5FSYS=5FBOOT=5FOPTIONS\n");
> &= gt; +    printf ("IPMID set command required return bytes: %i\n",= *data=5Flen);
> > +
> > +    set=5Fsys=5Fboot= =5Foptions=5Ft * reqptr =3D (set=5Fsys=5Fboot=5Foptions=5Ft *) request;
= > > +
> > +    char output=5Fbuf[NUM=5FRETURN=5FBY= TES=5FOF=5FSET] =3D {0};
> > +
> > +    /*
&= gt; > +     * Parameter #5 means boot flags. Please refer to 2= 8.13 of ipmi doc.
> > +     * This is the only parameter= used by petitboot.
> > +     */
> > +   &= nbsp;if (reqptr->parameter =3D=3D 5) {
> > +     &nbs= p;  /*
> > +         * To represent a hex= in string, e.g., "A0A0", which represents two bytes
> > +   =       * in the hex, but requires 5 bytes to store it as stri= ng, i.e., 4
> > +         * characters to stor= e the "A0A0" and 1 additional space for "\0".
> > +     =     * Thereby we have 2 * <number of bytes> + 1 for the str= ing buffer.
> > +         */
> > + &n= bsp;      char input=5Fbuf[NUM=5FINPUT=5FBYTES=5FOF=5FSET * = 2 + 1];
> > +        sprintf (input=5Fbuf, "%l= lX", reqptr->data.data64);
> > +
> > +     &= nbsp;  int r =3D dbus=5Fset=5Fproperty (input=5Fbuf);
> > +> > +        if (r < 0) {
> > + &nb= sp;          fprintf (stderr, "Dbus set property f= ailed for set=5Fsys=5Fboot=5Foptions.\n");
> > +     &nb= sp;      return IPMI=5FOUT=5FOF=5FSPACE;
> > + &nbs= p;      }
> > +
> > +      =  *data=5Flen =3D NUM=5FRETURN=5FBYTES=5FOF=5FSET;
> > + &nbs= p;      // Return code OK.
> > +     &nbs= p;  output=5Fbuf[0] =3D IPMI=5FOK;
> > +      =  memcpy (response, output=5Fbuf, *data=5Flen);
> > +   =  } else {
> > +        // Parameter not s= upported
> > +        output=5Fbuf[0] =3D IPMI= =5FCC=5FPARM=5FNOT=5FSUPPORTED;
> > +        m= emcpy (response, output=5Fbuf, *data=5Flen);
> > +     &= nbsp;  fprintf (stderr, "Unsupported parameter 0x%x\n", reqptr->par= ameter);
> > +        return IPMI=5FCC=5FPARM= =5FNOT=5FSUPPORTED;
>
> As far as I can see you're simply using= output=5Fbuf to assign the first byte of
> response to either IPMI= =5FOK or IPMI=5FCC=5FPARM=5FNOT=5FSUPPORTED and you're using
> memcpy= .
>
> Wouldn't it be nicer to
> memset(response, 0, *data= =5Flen);
> (or not even memset since NUM=5FRETURN=5FBYTES=5FOF=5FSET = is 1) and
> *response =3D IPMI=5FOK;
> or
> *response =3D= IPMI=5FCC=5FPARM=5FNOT=5FSUPPORTED;
> and avoid having output=5Fbuf = at all?
>
> Which makes me wonder why response is actually a vo= id *, is this a good idea?
>
> >      }
&g= t; >  
> >      return rc;
> > &nb= sp;}
> >  
> > -void register=5Fnetfn=5Fchassis=5Ffu= nctions()
> > +void register=5Fnetfn=5Fchassis=5Ffunctions ()
&= gt; >  {
> > -    printf("Registering NetFn:[0x%= X], Cmd:[0x%X]\n",NETFUN=5FCHASSIS, IPMI=5FCMD=5FWILDCARD);
> > - =    ipmi=5Fregister=5Fcallback(NETFUN=5FCHASSIS, IPMI=5FCMD=5FWILD= CARD, NULL, ipmi=5Fchassis=5Fwildcard);
> > +    printf = ("Registering NetFn:[0x%X], Cmd:[0x%X]\n", NETFUN=5FCHASSIS, IPMI=5FCMD=5FW= ILDCARD);
> > +    ipmi=5Fregister=5Fcallback (NETFUN=5F= CHASSIS, IPMI=5FCMD=5FWILDCARD, NULL, ipmi=5Fchassis=5Fwildcard);
> &= gt;  
> > -    printf("Registering NetFn:[0x%X], Cm= d:[0x%X]\n",NETFUN=5FCHASSIS, IPMI=5FCMD=5FGET=5FSYS=5FBOOT=5FOPTIONS);
= > > -    ipmi=5Fregister=5Fcallback(NETFUN=5FCHASSIS, IPMI= =5FCMD=5FGET=5FSYS=5FBOOT=5FOPTIONS, NULL, ipmi=5Fchassis=5Fget=5Fsys=5Fboo= t=5Foptions);
> > +    printf ("Registering NetFn:[0x%X]= , Cmd:[0x%X]\n", NETFUN=5FCHASSIS, IPMI=5FCMD=5FGET=5FSYS=5FBOOT=5FOPTIONS)= ;
> > +    ipmi=5Fregister=5Fcallback (NETFUN=5FCHASSIS,= 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=5FCHA= SSIS=5FCONTROL);
> > -    ipmi=5Fregister=5Fcallback(NET= FUN=5FCHASSIS, IPMI=5FCMD=5FCHASSIS=5FCONTROL, NULL, ipmi=5Fchassis=5Fcontr= ol);
> > +    printf ("Registering NetFn:[0x%X], Cmd:[0x= %X]\n", NETFUN=5FCHASSIS, IPMI=5FCMD=5FSET=5FSYS=5FBOOT=5FOPTIONS);
>= > +    ipmi=5Fregister=5Fcallback (NETFUN=5FCHASSIS, IPMI=5FC= MD=5FSET=5FSYS=5FBOOT=5FOPTIONS, NULL, ipmi=5Fchassis=5Fset=5Fsys=5Fboot=5F= options);
> > +
> > +    printf ("Registering N= etFn:[0x%X], Cmd:[0x%X]\n", NETFUN=5FCHASSIS, IPMI=5FCMD=5FCHASSIS=5FCONTRO= L);
> > +    ipmi=5Fregister=5Fcallback (NETFUN=5FCHASSI= S, IPMI=5FCMD=5FCHASSIS=5FCONTROL, NULL, ipmi=5Fchassis=5Fcontrol);
>= >  }
> > +
> > diff --git a/chassishandler.h b/c= hassishandler.h
> > index 1a26411..a310741 100644
> > ---= 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 not 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=5FSET 1
> > +#define = NUM=5FINPUT=5FBYTES=5FOF=5FSET 5
> > +
> >  // IPMI = commands for Chassis net functions.
> >  enum ipmi=5Fnetfn=5F= app=5Fcmds
> >  {
> >   // Chassis Control
&= gt; >   IPMI=5FCMD=5FCHASSIS=5FCONTROL  =3D 0x02,
> >=      // Get capability bits
> > +    IPM= I=5FCMD=5FSET=5FSYS=5FBOOT=5FOPTIONS =3D 0x08,
> >     &= nbsp;IPMI=5FCMD=5FGET=5FSYS=5FBOOT=5FOPTIONS =3D 0x09,
> >  }= ;
> >  
> >  // Command specific completion cod= es
> >  enum ipmi=5Fchassis=5Freturn=5Fcodes
> > &nb= sp;{
> > +    IPMI=5FOK =3D 0x0,
> >   &nb= sp;  IPMI=5FCC=5FPARM=5FNOT=5FSUPPORTED =3D 0x80,
> >  }= ;
> >  
> > +// Generic completion codes,
> &g= t; +// see IPMI doc section 5.2
> > +enum ipmi=5Fgeneric=5Freturn= =5Fcodes
> > +{
> > +    IPMI=5FOUT=5FOF=5FSPAC= E =3D 0xC4,
> > +};
> > +
> >  // Various C= hassis operations under a single command.
> >  enum ipmi=5Fch= assis=5Fcontrol=5Fcmds : uint8=5Ft
> >  {
>  
&= gt;  
>
>