On Thu, Mar 28, 2024 at 03:55 PM +0100, Ján Tomko <jtomko@redhat.com> wrote:
On a Wednesday in 2024, Marc Hartmayer wrote:
Make sure the old value in `scsi_target->wwpn` is free'd before replacing it. While at it, simplify the code.
"While at it" usually means it should have been a separate commit, especially if the simplification changes the behavior of the code.
:) […snip…]
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 48140b17baa5..c146a9f0c881 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2894,9 +2894,9 @@ int virNodeDeviceGetSCSITargetCaps(const char *sysfsPath, virNodeDevCapSCSITarget *scsi_target) { - int ret = -1; g_autofree char *dir = NULL; g_autofree char *rport = NULL; + g_autofree char *wwpn = NULL;
VIR_DEBUG("Checking if '%s' is an FC remote port", scsi_target->name);
@@ -2906,28 +2906,21 @@ virNodeDeviceGetSCSITargetCaps(const char *sysfsPath, rport = g_path_get_basename(dir);
if (!virFCIsCapableRport(rport)) - goto cleanup; + return -1; + + if (virFCReadRportValue(rport, "port_name", + &wwpn) < 0) { + VIR_WARN("Failed to read port_name for '%s'", rport); + return -1; + }
VIR_FREE(scsi_target->rport); scsi_target->rport = g_steal_pointer(&rport);
- if (virFCReadRportValue(scsi_target->rport, "port_name", - &scsi_target->wwpn) < 0) { - VIR_WARN("Failed to read port_name for '%s'", scsi_target->rport); - goto cleanup; - } - + VIR_FREE(scsi_target->wwpn); + scsi_target->wwpn = g_steal_pointer(&wwpn); scsi_target->flags |= VIR_NODE_DEV_CAP_FLAG_FC_RPORT; - ret = 0; - - cleanup: - if (ret < 0) { - VIR_FREE(scsi_target->rport); - VIR_FREE(scsi_target->wwpn); - scsi_target->flags &= ~VIR_NODE_DEV_CAP_FLAG_FC_RPORT;
Before, we cleared out the info if we were not able to refresh it.
Yep, that was the reason why have looked at other functions like `virNodeDeviceGetSCSIHostCaps` to understand if that “clear-out” is by intention or just a side-effect of the “cleanup” pattern here. IMO, it’s an unwanted side-effect of the cleanup pattern, but yes, I agree, we have to double check if that’s really the case.
Is there a possibility that would lead to stale information?
With this change: If the function fails with -1 no values are changed @scsi_target and that is what I would expect… But if it’s a valid use case to clear out previous information in the cleanup path and to return -1 hmm… why do we then not return 0 if it’s no error case but something expected? Thanks for the feedback.
Jano
- } - - return ret; + return 0; }
-- 2.34.1 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
-- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294