On Thu, Mar 28, 2024 at 03:55 PM +0100, Ján Tomko <jtomko@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx
>>To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
--
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
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
