On Tue, Feb 20, 2018 at 11:18:28AM -0700, Dave Jiang wrote:
> Adding firmware output of firmware information when ndctl list -D -F is used.
> Components displayed are current firmware version, next firmware version,
> and if a powercycle is required (firmware updated).
>
> Signed-off-by: Dave Jiang <[email protected]>
> Tested-by: Jeff Moyer <[email protected]>
<>
> +struct json_object *util_dimm_firmware_to_json(struct ndctl_dimm *dimm,
> + unsigned long flags)
> +{
> + struct json_object *jfirmware = json_object_new_object();
> + struct json_object *jobj;
> + struct ndctl_cmd *cmd;
> + int rc;
> + uint64_t run, next;
> + bool reboot = false;
> +
> + if (!jfirmware)
> + return NULL;
> +
> + cmd = ndctl_dimm_cmd_new_fw_get_info(dimm);
> + if (!cmd)
> + goto err;
> +
> + rc = ndctl_cmd_submit(cmd);
> + if (rc || ndctl_cmd_fw_xlat_firmware_status(cmd) != FW_SUCCESS) {
> + int run_err = -1;
Just a few nits. Looks good otherwise.
This new version defines three different local variables with the value -1
(run_err, run_err, next_err) and uses each one only once. Can we just use -1
instead of a variable in util_json_object_hex(), or maybe just have one
"error_val" variable that is defined for the whole function?
> +
> + jobj = util_json_object_hex(run_err, flags);
> + if (jobj)
> + json_object_object_add(jfirmware, "current_version",
> + jobj);
> + goto out;
> + }
> +
> + run = ndctl_cmd_fw_info_get_run_version(cmd);
> + if (run == ULLONG_MAX) {
> + int run_err = -1;
> +
> + jobj = util_json_object_hex(run_err, flags);
> + if (jobj)
> + json_object_object_add(jfirmware, "current_version",
> + jobj);
> + goto out;
> + }
> +
> + jobj = util_json_object_hex(run, flags);
> + if (jobj)
> + json_object_object_add(jfirmware, "current_version", jobj);
> +
> + next = ndctl_cmd_fw_info_get_next_version(cmd);
> + if (next == ULLONG_MAX) {
> + int next_err = -1;
> +
> + jobj = util_json_object_hex(next_err, flags);
> + if (jobj)
> + json_object_object_add(jfirmware, "next_version",
> + jobj);
> + goto out;
> + }
> +
> + if (next != 0) {
> + reboot = true;
> + jobj = util_json_object_hex(next, flags);
> + if (jobj)
> + json_object_object_add(jfirmware,
> + "next_version", jobj);
> + }
> +
> + if (reboot) {
> + jobj = json_object_new_boolean(reboot);
> + if (jobj)
> + json_object_object_add(jfirmware,
> + "need_powercycle", jobj);
> + }
You don't need this 'if (reboot) block' because 'reboot == (next != 0)' - you
can just combine this with the above conditional:
if (next != 0) {
jobj = util_json_object_hex(next, flags);
if (jobj)
json_object_object_add(jfirmware,
"next_version", jobj);
jobj = json_object_new_boolean(true);
if (jobj)
json_object_object_add(jfirmware,
"need_powercycle", jobj);
}
and you can get rid of the variable 'reboot' entirely.
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
