On Wed, Jun 20, 2018 at 09:41:35PM -0400, Pavel Tatashin wrote: > > I don't think __try_online_node() will ever return a value greater than > > zero. I assume what was meant was > > Hi Andrew and Oscar, > > Actually, the new __try_online_node() returns: > 1 -> a new node was allocated > 0 -> node is already online > -error -> an error encountered. > > The function simply missing the return comment at the beginning. > > Oscar, please check it via ./scripts/checkpatch.pl > > Add comment explaining the return values. > > And change: > ret = __try_online_node (nid, start, false); > new_node = !!(ret > 0); > if (ret < 0) > goto error; > To: > ret = __try_online_node (nid, start, false); > if (ret < 0) > goto error; > new_node = ret; > > Other than that the patch looks good to me, it simplifies the code. > So, if the above is addressed: > > Reviewed-by: Pavel Tatashin <pasha.tatashin@xxxxxxxxxx> Hi Pavel, I'll do so, thanks! > > Thank you, > Pavel > > > > > new_node = !!(ret >= 0); > > > > which may as well be > > > > new_node = (ret >= 0); > > > > since both sides have bool type. > > > > The fact that testing didn't detect this is worrisome.... > > > > > + if (ret < 0) > > > + goto error; > > > + > > > > > > /* call arch's memory hotadd */ > > > ret = arch_add_memory(nid, start, size, NULL, true); > > > - > > > if (ret < 0) > > > goto error; > > > > > > > > > ... > > > > > > -- Oscar Salvador SUSE L3
