On 03/28/2013 11:46 AM, Ján Tomko wrote:
> Both virIsCapableFCHost and virIsCapableVport return 0 when the
> respective sysfs path is accessible.
> ---
> src/node_device/node_device_linux_sysfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c
> index a1c3637..fd91430 100644
> --- a/src/node_device/node_device_linux_sysfs.c
> +++ b/src/node_device/node_device_linux_sysfs.c
> @@ -46,7 +46,7 @@ detect_scsi_host_caps_linux(union _virNodeDevCapData *d)
>
> VIR_DEBUG("Checking if host%d is an FC HBA", d->scsi_host.host);
>
> - if (virIsCapableFCHost(NULL, d->scsi_host.host)) {
> + if (virIsCapableFCHost(NULL, d->scsi_host.host) == 0) {
> d->scsi_host.flags |= VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST;
>
> if (virReadFCHost(NULL,
Before anything else - I think that a function with a name like
"virIsBlorf" should return true if it is a Blorf, and false if it isn't.
If we also need the ability to return failure, then I think a different
name would be appropriate (specifically to avoid the type of confusion
that led to this mistake in the first place). (I'll stop short of
suggesting a different name, because one doesn't easily come to mind :-)
Beyond that, since virIsCapableFCHost() can return an error, you should
be checking for it here; either do "switch (virIsCapableFCHost(....))
..." or store the return value and compare it multiple times.
So, I guess ACK that this patch fixes the bug you intended it to fix,
but either this patch should be extended to at least check for failure
case, or you should promise a followup patch to do so. As for the name
of the function, since I can't think of an alternative name, I can't
really require you to either :-)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list