Taku Izumi wrote:
> This patch creates the symlink to the hotplug driver at the hotplug slot
> directory (/sys/bus/pci/slots/<slot_name>). As a result, we can know easily
> what driver manages the hotplug slot.
>
> Signed-off-by: Taku Izumi <izumi.taku@xxxxxxxxxxxxxx>
>
> ---
> drivers/pci/hotplug/pci_hotplug_core.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> Index: linux-next.29rc6/drivers/pci/hotplug/pci_hotplug_core.c
> ===================================================================
> --- linux-next.29rc6.orig/drivers/pci/hotplug/pci_hotplug_core.c
> +++ linux-next.29rc6/drivers/pci/hotplug/pci_hotplug_core.c
> @@ -420,9 +420,20 @@ static int has_test_file(struct pci_slot
> return -ENOENT;
> }
>
> +static int has_owner_file(struct pci_slot *pci_slot)
> +{
> + struct hotplug_slot *slot = pci_slot->hotplug;
> + if ((!slot) || (!slot->ops))
> + return -ENODEV;
> + if (slot->ops->owner)
> + return 0;
> + return -ENOENT;
> +}
Given the name of the function I would expect to return it either 0 (no slot
there) or 1 (yes, there is one). On things like slot being NULL I would also
expect an error code. Now with the given name you would do something like:
if (!has_owner_file(foo))
which is very unintuitive. From just reading over that code it would mean
exactly the opposite of what is expected.
I see that the other functions around have the same interface so you are not
to blame for this, but ... well, understanding this expression needs a look
into the implementation of the has_foo() function. It also looks as the error
code is never checked for, so this function might really return either 0 or 1
or bool.
Eike
Attachment:
signature.asc
Description: This is a digitally signed message part.
