Skip to contentSkip to conversation

Unhandled return values in acpi/video.c

PATCH In review ville palo 2 participants 3 messages Aug 21, 2006 – Aug 22, 2006

Subsystems (2)
Kernel janitors ACPI
lore
Share & tools
lore

ville palo

author
#0 lore raw

PATCH 1/1 Unhandled return values in acpi/video.c

Is there any particular reason not to care about return values of the acpi_remove_notify_handler()? If there is some reason, status variable is declared for nothing. If there aren't any reasons, this patch should solve the issue.

Signed-off-by Ville Palo <[email protected]>
drivers/acpi/video.c +2 -2
index 56666a9..c55123a 100644
@@ -1569,7 +1569,7 @@ static int acpi_video_bus_put_one_device
                                            ACPI_DEVICE_NOTIFY,
                                            acpi_video_device_notify);
 
-       return 0;
+       return status;
 }
 
 static int acpi_video_bus_put_devices(struct acpi_video_bus *video)

@@ -1771,7 +1771,7 @@ static int acpi_video_bus_remove(struct 
        kfree(video->attached_array);
        kfree(video);
 
-       return 0;
+       return status;
 }
 
 static int

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#1 lore raw
Quoted 4 lines · ville palo

Is there any particular reason not to care about return values of the acpi_remove_notify_handler()? If there is some reason, status variable is declared for nothing. If there aren't any reasons, this patch should solve the issue.

The thing is that all of this kinda pointless because mentioned return values will be forgotten on upper layers.

drivers/acpi/video.c static int acpi_video_bus_put_one_device · line 1573

Quoted 9 lines
drivers/acpi/video.c +1 -1
@@ -1569,7 +1569,7 @@ static int acpi_video_bus_put_one_device
                                            ACPI_DEVICE_NOTIFY,
                                            acpi_video_device_notify);
 
-       return 0;
+       return status;
 }

Assume, this happened. Then type of acpi_video_bus_put_one_device() will be changed to acpi_status and check at the beginning dropped. What a single caller of it do with return value? It spams logs and forgetting it. Assume we return status even from acpi_video_bus_put_devices() (and change return type). It will again be forgotten since both callers don't bother with it.

The real question is at which point return values should be dropped?

drivers/acpi/video.c static int acpi_video_bus_remove(struct · line 1775

Quoted 8 lines
drivers/acpi/video.c +1 -1
 static int acpi_video_bus_put_devices(struct acpi_video_bus *video)

@@ -1771,7 +1771,7 @@ static int acpi_video_bus_remove(struct
        kfree(video->attached_array);
        kfree(video);
 
-       return 0;
+       return status;
 }

Ditto: acpi_video_bus_remove() is ->remove method which is called (and probably should be called) without checking at all.

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

author
#2 lore raw
In reply to ville palo
Quoted 15 lines · Alexey Dobriyan
Quoted 4 lines · ville palo

Is there any particular reason not to care about return values of the acpi_remove_notify_handler()? If there is some reason, status variable is declared for nothing. If there aren't any reasons, this patch should solve the issue.

The thing is that all of this kinda pointless because mentioned return values will be forgotten on upper layers.

Quoted 6 lines
drivers/acpi/video.c +0 -0
@@ -1569,7 +1569,7 @@ static int acpi_video_bus_put_one_device
                                            ACPI_DEVICE_NOTIFY,
                                            acpi_video_device_notify);
Quoted 11 lines

drivers/acpi/video.c static int acpi_video_bus_put_one_device · line 1573

Quoted 3 lines
drivers/acpi/video.c +1 -1
-       return 0;
+       return status;
 }

Assume, this happened. Then type of acpi_video_bus_put_one_device() will be changed to acpi_status and check at the beginning dropped. What a single caller of it do with return value? It spams logs and forgetting it. Assume we return status even from acpi_video_bus_put_devices() (and change return type). It will again be forgotten since both callers don't bother with it.

Could it be helpfull for debugging purposes to know that there was or wasn't an error in acpi_remove_notify_handler() function or in acpi_video_bus_put_one_device()? (sorry for changing the return type :/)

Quoted 7 lines

The real question is at which point return values should be dropped?

Quoted 5 lines
drivers/acpi/video.c +0 -0
 static int acpi_video_bus_put_devices(struct acpi_video_bus *video)

@@ -1771,7 +1771,7 @@ static int acpi_video_bus_remove(struct
        kfree(video->attached_array);
        kfree(video);
Quoted 6 lines

drivers/acpi/video.c static int acpi_video_bus_put_one_device · line 1573

Quoted 3 lines
drivers/acpi/video.c +1 -1
-       return 0;
+       return status;
 }

Ditto: acpi_video_bus_remove() is ->remove method which is called (and probably should be called) without checking at all.

Well, why to declare status variable at all then :) Wouldn't it be good the get log messages in case of an error here as well?

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors