<font face="Default Sans Serif,Verdana,Arial,Helvetica,sans-serif" size="2">Hi Cyril,<br><br>Thank you for the comments. Please see my reply bellow:<br><br><font color="#990099">-----"openbmc" <<a target="_blank" href="mailto:openbmc-bounces+shliyi=cn.ibm.com@lists.ozlabs.org">openbmc-bounces+shliyi=cn.ibm.com@lists.ozlabs.org</a>> wrote: -----</font><br><br>>To: OpenBMC Patches <<a target="_blank" href="mailto:openbmc-patches@stwcx.xyz">openbmc-patches@stwcx.xyz</a>><br>>From: Cyril Bur <br>>Sent by: "openbmc" <br>>Date: 01/20/2016 07:58AM<br>>Cc: <a target="_blank" href="mailto:openbmc@lists.ozlabs.org">openbmc@lists.ozlabs.org</a>, Nan KX Li/China/IBM@IBMCN<br>>Subject: Re: [PATCH skeleton] skeleton: Add BMC coldReset() method to<br>>org.openbmc.control.Bmc dbus interface<br>><br>>On Mon, 18 Jan 2016 22:40:36 -0600<br>>OpenBMC Patches <<a target="_blank" href="mailto:openbmc-patches@stwcx.xyz">openbmc-patches@stwcx.xyz</a>> wrote:<br>><br>>> From: William <<a target="_blank" href="mailto:bjlinan@cn.ibm.com">bjlinan@cn.ibm.com</a>><br>>> <br>><br>>Hi William,<br>><br>>Is there a reason you're using glib? I was under the impression that<br>>the<br>>preferred dbus library was sd_bus. I suppose this is in<br>>openbmc/skeleton so it<br>>is destined to be rewritten in which case I don't have too much of<br>>and issue<br>>with glib for now.<br><br>Current skeleton dbus interface is writing using glib. I also heard from Chris we will shift to sd_bus.<br>If we got details, we can rewrite it using sd_bus.<br><br>><br>>I do have one concern below.<br>><br><br>>> +static gboolean<br>>> +on_cold_reset (ControlBmc        *bmc,<br>>> +         GDBusMethodInvocation        *invocation,<br>>> +         gpointer                user_data)<br>>> +{<br>>> +        GError *err = NULL;<br>>> +        /* Wait a while before reboot, so the caller can be responded.<br>>> +         * Note that g_spawn_command_line_async() cannot parse ';' as<br>>> +         * a command separator. Need to use 'sh -c' to let shell parse<br>>it.<br>>> +         */<br>>> +        gchar *reboot_command = "/bin/sh -c 'sleep 3;reboot --force'";<br>><br>>This doesn't seem like a good idea, I'm not exactly sure how<br>>g_spawn_command_line_async() works but calling out to the shell to<br>>reboot(2)<br>>doesn't seem like the best way of going about things.<br>><br><br>The main idea is to reboot the system, while leave a few seconds for the coldReset() caller (e.g, REST client) get responded firstly.<br>The REST server can have some time to send respond the caller.<br><br>We have been thinking of using libc function "int reboot(cmd)". Since we need to wait before<br>reboot, using shell command is more straightforward.<br>Linux reboot man page says:<br>"       When called with --force or when in runlevel 0 or 6, this tool  invokes<br>       the  reboot(2)  system call itself (with REBOOTCOMMAND argument passed)<br>       and directly reboots the system."<br><br>Also "g_spawn_command_line_async()" execute the reboot command *in background*. The coldReset() method can return<br>firstly. Then after 3 seconds, BMC reboots. 3 second is just a magic, it can be other proper value.<br><br>>Also, sleep 3; is probably fine all of the time but it doesn't<br>>actually provide<br>>any guarantee that on_cold_reset() will ever return, it looks like<br>>there's a<br>>(however unlikely) race here.<br>><br><br>"g_spawn_command_line_async()" can help on that.<br><br>>Again, I consider just ignoring these issues for now since I believe<br>>this code<br>>is going to be replaced, however, this is starting to differ greatly<br>>from what<br>>the finished product will be.<br>><br>>If this code is being put here to exercise these codepathes and not<br>>just for the<br>>bare requirement of bringing up a machine, it would be good if it<br>>looked more<br>>like the finished product.<br>><br><br>We will refine the code based on feedback.<br><br>Thanks,<br>-Yi<br></font><BR>