Remove unnecessary null pointer checks?

Applied, thanks.

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Markus Elfring <[email protected]>
Date: Sun, 23 Nov 2014 19:13:56 +0100

Another update suggestion was taken into account after a patch was applied from static source code analysis.

Markus Elfring (2):
Deletion of unnecessary checks before two function calls One function call less in ion_buffer_create() after error detection

 drivers/staging/android/ion/ion.c              | 9 +++------
 drivers/staging/android/ion/ion_dummy_driver.c | 6 ++----
 drivers/staging/android/ion/tegra/tegra_ion.c  | 6 ++----
 3 files changed, 7 insertions(+), 14 deletions(-)

2.1.3

From: Markus Elfring <[email protected]>
Date: Sun, 23 Nov 2014 18:48:15 +0100

The functions ion_heap_destroy() and vfree() perform also input parameter validation. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/staging/android/ion/ion.c              | 6 ++----
 drivers/staging/android/ion/ion_dummy_driver.c | 6 ++----
 drivers/staging/android/ion/tegra/tegra_ion.c  | 6 ++----
 3 files changed, 6 insertions(+), 12 deletions(-)

index 2703609..df12cc3 100644
@@ -263,8 +263,7 @@ err:
 	heap->ops->unmap_dma(heap, buffer);
 	heap->ops->free(buffer);
 err1:
-	if (buffer->pages)
-		vfree(buffer->pages);
+	vfree(buffer->pages);
 err2:
 	kfree(buffer);
 	return ERR_PTR(ret);

@@ -276,8 +275,7 @@ void ion_buffer_destroy(struct ion_buffer *buffer)
 		buffer->heap->ops->unmap_kernel(buffer->heap, buffer);
 	buffer->heap->ops->unmap_dma(buffer->heap, buffer);
 	buffer->heap->ops->free(buffer);
-	if (buffer->pages)
-		vfree(buffer->pages);
+	vfree(buffer->pages);
 	kfree(buffer);
 }
 

index 3a45e79..d06bf52 100644
@@ -112,10 +112,8 @@ static int __init ion_dummy_init(void)
 	}
 	return 0;
 err:
-	for (i = 0; i < dummy_ion_pdata.nr; i++) {
-		if (heaps[i])
-			ion_heap_destroy(heaps[i]);
-	}
+	for (i = 0; i < dummy_ion_pdata.nr; ++i)
+		ion_heap_destroy(heaps[i]);
 	kfree(heaps);
 
 	if (carveout_ptr) {

index 11c7cce..5b8ef0e 100644
@@ -54,10 +54,8 @@ static int tegra_ion_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, idev);
 	return 0;
 err:
-	for (i = 0; i < num_heaps; i++) {
-		if (heaps[i])
-			ion_heap_destroy(heaps[i]);
-	}
+	for (i = 0; i < num_heaps; ++i)
+		ion_heap_destroy(heaps[i]);
 	return err;
 }
 

2.1.3

From: Markus Elfring <[email protected]>
Date: Sun, 23 Nov 2014 19:12:29 +0100

The jump label "err1" was used by the ion_buffer_create() function in case of a memory allocation failure just to pass a null pointer to a vfree() function call by a data structure element.
This implementation detail could be improved by the deletion of this inappropriate jump label.

---
 drivers/staging/android/ion/ion.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index df12cc3..7a26b85 100644
@@ -226,7 +226,7 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
 		buffer->pages = vmalloc(sizeof(struct page *) * num_pages);
 		if (!buffer->pages) {
 			ret = -ENOMEM;
-			goto err1;
+			goto err2;
 		}
 
 		for_each_sg(table->sgl, sg, table->nents, i) {

@@ -262,7 +262,6 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
 err:
 	heap->ops->unmap_dma(heap, buffer);
 	heap->ops->free(buffer);
-err1:
 	vfree(buffer->pages);
 err2:
 	kfree(buffer);

2.1.3

How do you want this source code to be improved? Should I introduce longer names for the affected jump labels?

Regards,
Markus

Am 27.11.2014 15:25, schrieb SF Markus Elfring:

hi markus,
the confusion arises because the errX are numbered and now on in the sequence is missing.

So far i see you can renumber them or give more descriptiv names.

hope that helps,
re,
wh

From: Markus Elfring <[email protected]>
Date: Mon, 24 Nov 2014 22:00:21 +0100

The pci_dev_put() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/platform/x86/sony-laptop.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 26ad9ff..22b173d 100644
@@ -3717,8 +3717,7 @@ static void sony_pic_detect_device_type(struct sony_pic_dev *dev)
 	dev->event_types = type2_events;
 
 out:
-	if (pcidev)
-		pci_dev_put(pcidev);
+	pci_dev_put(pcidev);
 
 	pr_info("detected Type%d model\n",
 		dev->model = SONYPI_DEVICE_TYPE1 ? 1 :

2.1.3

Thanks Markus, queued.

Darren Hart
Intel Open Source Technology Center

From: Markus Elfring <[email protected]>
Date: Mon, 24 Nov 2014 20:30:29 +0100

The backlight_device_unregister() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/platform/x86/asus-laptop.c    | 3 +--
 drivers/platform/x86/asus-wmi.c       | 3 +--
 drivers/platform/x86/eeepc-laptop.c   | 3 +--
 drivers/platform/x86/fujitsu-laptop.c | 6 ++----
 drivers/platform/x86/ideapad-laptop.c | 3 +--
 drivers/platform/x86/intel_oaktrail.c | 3 +--
 drivers/platform/x86/msi-wmi.c        | 3 +--
 drivers/platform/x86/sony-laptop.c    | 3 +--
 drivers/platform/x86/toshiba_acpi.c   | 3 +--
 9 files changed, 10 insertions(+), 20 deletions(-)

index 7f4dc6f..11fac1a 100644
@@ -843,8 +843,7 @@ static int asus_backlight_init(struct asus_laptop *asus)
 
 static void asus_backlight_exit(struct asus_laptop *asus)
 {
-	if (asus->backlight_device)
-		backlight_device_unregister(asus->backlight_device);
+	backlight_device_unregister(asus->backlight_device);
 	asus->backlight_device = NULL;
 }
 

index 21fc932..7543a56 100644
@@ -1308,8 +1308,7 @@ static int asus_wmi_backlight_init(struct asus_wmi *asus)
 
 static void asus_wmi_backlight_exit(struct asus_wmi *asus)
 {
-	if (asus->backlight_device)
-		backlight_device_unregister(asus->backlight_device);
+	backlight_device_unregister(asus->backlight_device);
 
 	asus->backlight_device = NULL;
 }

index bd533c2..9bc7eb7 100644
@@ -1158,8 +1158,7 @@ static int eeepc_backlight_init(struct eeepc_laptop *eeepc)
 
 static void eeepc_backlight_exit(struct eeepc_laptop *eeepc)
 {
-	if (eeepc->backlight_device)
-		backlight_device_unregister(eeepc->backlight_device);
+	backlight_device_unregister(eeepc->backlight_device);
 	eeepc->backlight_device = NULL;
 }
 

index 87aa28c..f1a670b 100644
@@ -1147,8 +1147,7 @@ fail_hotkey1:
 fail_hotkey:
 	platform_driver_unregister(&fujitsupf_driver);
 fail_backlight:
-	if (fujitsu->bl_device)
-		backlight_device_unregister(fujitsu->bl_device);
+	backlight_device_unregister(fujitsu->bl_device);
 fail_sysfs_group:
 	sysfs_remove_group(&fujitsu->pf_device->dev.kobj,
 			   &fujitsupf_attribute_group);

@@ -1172,8 +1171,7 @@ static void __exit fujitsu_cleanup(void)
 
 	platform_driver_unregister(&fujitsupf_driver);
 
-	if (fujitsu->bl_device)
-		backlight_device_unregister(fujitsu->bl_device);
+	backlight_device_unregister(fujitsu->bl_device);
 
 	sysfs_remove_group(&fujitsu->pf_device->dev.kobj,
 			   &fujitsupf_attribute_group);

index ed494f3..3163061 100644
@@ -729,8 +729,7 @@ static int ideapad_backlight_init(struct ideapad_private *priv)
 
 static void ideapad_backlight_exit(struct ideapad_private *priv)
 {
-	if (priv->blightdev)
-		backlight_device_unregister(priv->blightdev);
+	backlight_device_unregister(priv->blightdev);
 	priv->blightdev = NULL;
 }
 

index 4bc9604..0dd72cf 100644
@@ -271,8 +271,7 @@ static int oaktrail_backlight_init(void)
 
 static void oaktrail_backlight_exit(void)
 {
-	if (oaktrail_bl_device)
-		backlight_device_unregister(oaktrail_bl_device);
+	backlight_device_unregister(oaktrail_bl_device);
 }
 
 static int oaktrail_probe(struct platform_device *pdev)

index 70222f2..6d2bac0 100644
@@ -354,8 +354,7 @@ static void __exit msi_wmi_exit(void)
 		sparse_keymap_free(msi_wmi_input_dev);
 		input_unregister_device(msi_wmi_input_dev);
 	}
-	if (backlight)
-		backlight_device_unregister(backlight);
+	backlight_device_unregister(backlight);
 }
 
 module_init(msi_wmi_init);

index 26ad9ff..a48cdc7 100644
@@ -3141,8 +3141,7 @@ static void sony_nc_backlight_setup(void)
 
 static void sony_nc_backlight_cleanup(void)
 {
-	if (sony_bl_props.dev)
-		backlight_device_unregister(sony_bl_props.dev);
+	backlight_device_unregister(sony_bl_props.dev);
 }
 
 static int sony_nc_add(struct acpi_device *device)

index d0dce73..703a68a 100644
@@ -1647,8 +1647,7 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
 		rfkill_destroy(dev->bt_rfk);
 	}
 
-	if (dev->backlight_dev)
-		backlight_device_unregister(dev->backlight_dev);
+	backlight_device_unregister(dev->backlight_dev);
 
 	if (dev->illumination_supported)
 		led_classdev_unregister(&dev->led_dev);

2.1.3

Thanks Markus, queued.

Darren Hart
Intel Open Source Technology Center

Hi Markus,

What script was used ? Is it in scripts/coccinelle ?

A semantic patch approach which I published on the mailing lists in March is in action on my software development system for a while.

Not yet.

I hope that the involved update suggestions got sufficient positive feedback to integrate five scripts there.

Regards,
Markus

The current scripts are very complicated, involving the interaction between multiple scripts and a database, and I think they are not very suitable for make coccicheck. Also, the idea of removing the null checks is not appropriate in all contexts. Perhaps it could be possible to add a script to the Linux kernel that handles a number of common cases for which removing the null test is widely considered to be desirable.

julia

Julia, do you have any particular objection to this specific patch? I didn't see a reason to prevent it going in.

Darren Hart
Intel Open Source Technology Center

Sorry if I was unclear. If there is no problem with the current patch, I have no objection to it. I don't think that the semantic patch that caused this patch to be generated is suitable for inclusion in the Linux kernel.

julia

Thanks for your interest around this concrete update suggestion.

* Would you like to distinguish the consequences a bit more for results from the application of the semantic patch language?

* Where do you prefer to store special extensions for Coccinelle's script collection

Regards,
Markus

I hate these patches...

We're saying "these functions have sanity checks so let's pass nonsense values to them, it's fine." It makes the code harder to understand. There is no way for a human being to remember the complete list of functions with sanity checks and which don't have sanity checks.

Markus has introduced quite a few bugs as well (people have so far managed to catch his bugs before they were committed). He so far has resisted any suggestion that he should manually review his patches before sending them.

Btw do have a scripts/coccinelle/free/ifnullfree.cocci which removes checks before kfree, debugfs_remove, debugfs_remove_recursive, and usb_free_urb.

regards,
dan carpenter

I understand also this software development challenge.

I have only found other opinions about specific update suggestions. Which of such "bugs" are real mistakes?
Are you looking for a better consensus?

Would you like to refer to any concrete software developers?

I am performing source code review to some degree. My approach does partly not fit to your expectations.

Regards,
Markus

From: Markus Elfring <[email protected]>
Date: Mon, 24 Nov 2014 22:32:30 +0100

The rc_unregister_device() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/media/common/siano/smsir.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 273043e..35d0e88 100644
@@ -107,8 +107,7 @@ int sms_ir_init(struct smscore_device_t *coredev)
 
 void sms_ir_exit(struct smscore_device_t *coredev)
 {
-	if (coredev->ir.dev)
-		rc_unregister_device(coredev->ir.dev);
+	rc_unregister_device(coredev->ir.dev);
 
 	sms_log("");
 }

2.1.3

From: Markus Elfring <[email protected]>
Date: Mon, 24 Nov 2014 22:58:48 +0100

The backlight_device_unregister() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/staging/olpc_dcon/olpc_dcon.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index eb83b28..6a9a881 100644
@@ -682,8 +682,7 @@ static int dcon_remove(struct i2c_client *client)
 
 	free_irq(DCON_IRQ, dcon);
 
-	if (dcon->bl_dev)
-		backlight_device_unregister(dcon->bl_dev);
+	backlight_device_unregister(dcon->bl_dev);
 
 	if (dcon_device != NULL)
 		platform_device_unregister(dcon_device);

2.1.3

From: Markus Elfring <[email protected]>
Date: Tue, 25 Nov 2014 13:44:20 +0100

The release_firmware() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index 655ce5b..757052c 100644
@@ -404,9 +404,7 @@ void adreno_gpu_cleanup(struct adreno_gpu *gpu)
 			msm_gem_put_iova(gpu->memptrs_bo, gpu->base.id);
 		drm_gem_object_unreference(gpu->memptrs_bo);
 	}
-	if (gpu->pm4)
-		release_firmware(gpu->pm4);
-	if (gpu->pfp)
-		release_firmware(gpu->pfp);
+	release_firmware(gpu->pm4);
+	release_firmware(gpu->pfp);
 	msm_gpu_cleanup(&gpu->base);
 }

2.1.3

You can probably get rid of this if you configure your emailer properly.

This is odd. I've never seen git send-email generate these.

I didn't notice with your earlier patches, but the subject prefix is wrong. Please use something more consistent with existing patches, in this case: "drm/msm: ..." or "drm/msm/adreno: ..."

From: Markus Elfring <[email protected]>
Date: Tue, 25 Nov 2014 14:30:28 +0100

The functions framebuffer_release() and vunmap() perform also input parameter validation. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/gpu/drm/msm/msm_fbdev.c | 3 +--
 drivers/gpu/drm/msm/msm_gem.c   | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

index ab5bfd2..fd5a6f3 100644
@@ -193,8 +193,7 @@ fail_unlock:
 fail:
 
 	if (ret) {
-		if (fbi)
-			framebuffer_release(fbi);
+		framebuffer_release(fbi);
 		if (fb) {
 			drm_framebuffer_unregister_private(fb);
 			drm_framebuffer_remove(fb);

index 4b1b82a..157cf21 100644
@@ -543,8 +543,7 @@ void msm_gem_free_object(struct drm_gem_object *obj)
 			drm_free_large(msm_obj->pages);
 
 	} else {
-		if (msm_obj->vaddr)
-			vunmap(msm_obj->vaddr);
+		vunmap(msm_obj->vaddr);
 		put_pages(obj);
 	}
 

2.1.3

This needs the same fix for the subject prefix that I mentioned for your other patch, otherwise:

Perhaps a good idea would be to send all of these patches with the subject prefix fixed up as a second version and threaded in a series. That makes it easier for people to pick them up (assuming Dave will take them directly).

Thierry

no worries, I'll fix up the subject lines when I apply..

BR,
-R

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

btw, I have these two queued up on msm-next, thanks

BR,
-R

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

From: Markus Elfring <[email protected]>
Date: Tue, 25 Nov 2014 16:06:19 +0100

The drm_fbdev_cma_restore_mode() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/staging/imx-drm/imx-drm-core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 6b22106..0a1ecef 100644
@@ -64,8 +64,7 @@ static void imx_drm_driver_lastclose(struct drm_device *drm)
 #if IS_ENABLED(CONFIG_DRM_IMX_FB_HELPER)
 	struct imx_drm_device *imxdrm = drm->dev_private;
 
-	if (imxdrm->fbhelper)
-		drm_fbdev_cma_restore_mode(imxdrm->fbhelper);
+	drm_fbdev_cma_restore_mode(imxdrm->fbhelper);
 #endif
 }
 

2.1.3

From: Markus Elfring <[email protected]>
Date: Tue, 25 Nov 2014 16:51:08 +0100

The oz_free_urb_link() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/staging/ozwpan/ozhcd.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index 30bd928..c9b6d98 100644
@@ -411,8 +411,7 @@ static void oz_complete_urb(struct usb_hcd *hcd, struct urb *urb,
 	}
 	spin_lock(&g_tasklet_lock);
 	spin_unlock_irqrestore(&g_tasklet_lock, irq_state);
-	if (cancel_urbl)
-		oz_free_urb_link(cancel_urbl);
+	oz_free_urb_link(cancel_urbl);
 }
 
 /*

@@ -581,8 +580,7 @@ static int oz_dequeue_ep_urb(struct oz_port *port, u8 ep_addr, int in_dir,
 		}
 	}
 	spin_unlock_bh(&port->ozhcd->hcd_lock);
-	if (urbl)
-		oz_free_urb_link(urbl);
+	oz_free_urb_link(urbl);
 	return urbl ? 0 : -EIDRM;
 }
 

2.1.3

From: Markus Elfring <[email protected]>
Date: Sat, 29 Nov 2014 14:34:59 +0100

The vfree() function performs also input parameter validation. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/net/ethernet/sun/cassini.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 37f87ff..d745808 100644
@@ -5179,8 +5179,7 @@ static void cas_remove_one(struct pci_dev *pdev)
 	cp = netdev_priv(dev);
 	unregister_netdev(dev);
 
-	if (cp->fw_data)
-		vfree(cp->fw_data);
+	vfree(cp->fw_data);
 
 	mutex_lock(&cp->pm_mutex);
 	cancel_work_sync(&cp->reset_task);

2.1.3

From: Markus Elfring <[email protected]>
Date: Sat, 29 Nov 2014 14:34:59 +0100

The vfree() function performs also input parameter validation. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/net/ethernet/sun/cassini.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 37f87ff..d745808 100644
@@ -5179,8 +5179,7 @@ static void cas_remove_one(struct pci_dev *pdev)
 	cp = netdev_priv(dev);
 	unregister_netdev(dev);
 
-	if (cp->fw_data)
-		vfree(cp->fw_data);
+	vfree(cp->fw_data);
 
 	mutex_lock(&cp->pm_mutex);
 	cancel_work_sync(&cp->reset_task);

2.1.3

From: SF Markus Elfring <[email protected]>
Date: Sat, 29 Nov 2014 15:05:33 +0100

Applied.

From: Markus Elfring <[email protected]>
Date: Sat, 29 Nov 2014 15:16:01 +0100

The input_free_device() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/hid/wacom_sys.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index eb55316..21ced00 100644
@@ -1099,10 +1099,8 @@ static void wacom_free_inputs(struct wacom *wacom)
 {
 	struct wacom_wac *wacom_wac = &(wacom->wacom_wac);
 
-	if (wacom_wac->input)
-		input_free_device(wacom_wac->input);
-	if (wacom_wac->pad_input)
-		input_free_device(wacom_wac->pad_input);
+	input_free_device(wacom_wac->input);
+	input_free_device(wacom_wac->pad_input);
 	wacom_wac->input = NULL;
 	wacom_wac->pad_input = NULL;
 }

2.1.3

From: Markus Elfring <[email protected]>
Date: Thu, 9 Jul 2015 08:00:10 +0200

The input_free_device() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/hid/wacom_sys.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

index 4c0ffca..936ad77 100644
@@ -1149,12 +1149,9 @@ static void wacom_free_inputs(struct wacom *wacom)
 {
 	struct wacom_wac *wacom_wac = &(wacom->wacom_wac);
 
-	if (wacom_wac->pen_input)
-		input_free_device(wacom_wac->pen_input);
-	if (wacom_wac->touch_input)
-		input_free_device(wacom_wac->touch_input);
-	if (wacom_wac->pad_input)
-		input_free_device(wacom_wac->pad_input);
+	input_free_device(wacom_wac->pen_input);
+	input_free_device(wacom_wac->touch_input);
+	input_free_device(wacom_wac->pad_input);
 	wacom_wac->pen_input = NULL;
 	wacom_wac->touch_input = NULL;
 	wacom_wac->pad_input = NULL;

2.4.5

This is really on a "is it worth it?" borderline, but anyway ... applied to for-4.3/upstream.

Thanks,

Jiri Kosina
SUSE Labs

From: Markus Elfring <[email protected]>
Date: Sat, 29 Nov 2014 16:23:20 +0100

The free_percpu() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/net/ipvlan/ipvlan_main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 96b71b0..feb1853 100644
@@ -125,8 +125,7 @@ static void ipvlan_uninit(struct net_device *dev)
 	struct ipvl_dev *ipvlan = netdev_priv(dev);
 	struct ipvl_port *port = ipvlan->port;
 
-	if (ipvlan->pcpu_stats)
-		free_percpu(ipvlan->pcpu_stats);
+	free_percpu(ipvlan->pcpu_stats);
 
 	port->count -= 1;
 	if (!port->count)

2.1.3

From: SF Markus Elfring <[email protected]>
Date: Sat, 29 Nov 2014 16:30:27 +0100

Applied.

From: Markus Elfring <[email protected]>
Date: Sat, 29 Nov 2014 16:42:50 +0100

The iput() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 fs/notify/inode_mark.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 3daf513..d8157a6 100644
@@ -231,8 +231,7 @@ void fsnotify_unmount_inodes(struct list_head *list)
 		 */
 		spin_unlock(&inode_sb_list_lock);
 
-		if (need_iput_tmp)
-			iput(need_iput_tmp);
+		iput(need_iput_tmp);
 
 		/* for each watch, send FS_UNMOUNT and then remove it */
 		fsnotify(inode, FS_UNMOUNT, inode, FSNOTIFY_EVENT_INODE, NULL, 0);

2.1.3

From: Markus Elfring <[email protected]>
Date: Sat, 29 Nov 2014 17:00:21 +0100

The iput() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 fs/namei.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index ca81416..ce69074 100644
@@ -3759,8 +3759,7 @@ exit2:
 		dput(dentry);
 	}
 	mutex_unlock(&nd.path.dentry->d_inode->i_mutex);
-	if (inode)
-		iput(inode);	/* truncate the inode here */
+	iput(inode);	/* truncate the inode here */
 	inode = NULL;
 	if (delegated_inode) {
 		error = break_deleg_wait(&delegated_inode);

2.1.3

From: Markus Elfring <[email protected]>
Date: Sat, 29 Nov 2014 17:40:22 +0100

The functions xfs_blkdev_put() and xfs_qm_dqrele() test whether their argument is NULL and then return immediately.
Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 fs/xfs/xfs_qm.c    | 12 +++++-------
 fs/xfs/xfs_super.c |  3 +--
 2 files changed, 6 insertions(+), 9 deletions(-)

index d68f230..9a4b50a 100644
@@ -1749,23 +1749,21 @@ xfs_qm_vop_dqalloc(
 	xfs_iunlock(ip, lockflags);
 	if (O_udqpp)
 		*O_udqpp = uq;
-	else if (uq)
+	else
 		xfs_qm_dqrele(uq);
 	if (O_gdqpp)
 		*O_gdqpp = gq;
-	else if (gq)
+	else
 		xfs_qm_dqrele(gq);
 	if (O_pdqpp)
 		*O_pdqpp = pq;
-	else if (pq)
+	else
 		xfs_qm_dqrele(pq);
 	return 0;
 
 error_rele:
-	if (gq)
-		xfs_qm_dqrele(gq);
-	if (uq)
-		xfs_qm_dqrele(uq);
+	xfs_qm_dqrele(gq);
+	xfs_qm_dqrele(uq);
 	return error;
 }
 

index 206b97f..97eaa25 100644
@@ -796,8 +796,7 @@ xfs_open_devices(
  out_free_ddev_targ:
 	xfs_free_buftarg(mp, mp->m_ddev_targp);
  out_close_rtdev:
-	if (rtdev)
-		xfs_blkdev_put(rtdev);
+	xfs_blkdev_put(rtdev);
  out_close_logdev:
 	if (logdev && logdev != ddev)
 		xfs_blkdev_put(logdev);

2.1.3

Looks fine.

Dave Chinner
[email protected]

From: Markus Elfring <[email protected]>
Date: Fri, 26 Jun 2015 11:05:41 +0200

The xfs_qm_dqrele() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 fs/xfs/xfs_qm_syscalls.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 3640c6e..6deee1e 100644
@@ -735,15 +735,15 @@ xfs_dqrele_inode(
 	}
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	if ((flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
+	if (flags & XFS_UQUOTA_ACCT) {
 		xfs_qm_dqrele(ip->i_udquot);
 		ip->i_udquot = NULL;
 	}
-	if ((flags & XFS_GQUOTA_ACCT) && ip->i_gdquot) {
+	if (flags & XFS_GQUOTA_ACCT) {
 		xfs_qm_dqrele(ip->i_gdquot);
 		ip->i_gdquot = NULL;
 	}
-	if ((flags & XFS_PQUOTA_ACCT) && ip->i_pdquot) {
+	if (flags & XFS_PQUOTA_ACCT) {
 		xfs_qm_dqrele(ip->i_pdquot);
 		ip->i_pdquot = NULL;
 	}

2.4.4

True.

But wrong.

xfs_dqrele_inode() gets called on every inode in the inode cache, and this change results in a cacheline in every inode being dirtied even if they don't have dquots attached. Given the inode cache can hold tens to hundreds of millions of inodes on large machines, we don't want to dirty any cachelines we don't need to while walking the inode cache and releasing dquots...

Cheers,

Dave.

Dave Chinner
[email protected]

Would it make sense to annotate checks before such function calls as "LIKELY"?

Regards,
Markus

No - it will be random as to whether the inodes have dquots attached or not and so a static hint is always going to be wrong for someone....

Cheers,

Dave.

Dave Chinner
[email protected]

From: Markus Elfring <[email protected]>
Date: Sat, 29 Nov 2014 17:55:39 +0100

The kfree() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 fs/dlm/lockspace.c | 3 +--
 fs/dlm/memory.c    | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

index f3e7278..b660b93 100644
@@ -674,8 +674,7 @@ static int new_lockspace(const char *name, const char *cluster,
  out_lkbidr:
 	idr_destroy(&ls->ls_lkbidr);
 	for (i = 0; i < DLM_REMOVE_NAMES_MAX; i++) {
-		if (ls->ls_remove_names[i])
-			kfree(ls->ls_remove_names[i]);
+		kfree(ls->ls_remove_names[i]);
 	}
  out_rsbtbl:
 	vfree(ls->ls_rsbtbl);

index 7cd24bc..2d2eaa0 100644
@@ -86,8 +86,7 @@ void dlm_free_lkb(struct dlm_lkb *lkb)
 		struct dlm_user_args *ua;
 		ua = lkb->lkb_ua;
 		if (ua) {
-			if (ua->lksb.sb_lvbptr)
-				kfree(ua->lksb.sb_lvbptr);
+			kfree(ua->lksb.sb_lvbptr);
 			kfree(ua);
 		}
 	}

2.1.3

From: Markus Elfring <[email protected]>
Date: Fri, 26 Jun 2015 14:00:06 +0200

The kfree() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 fs/dlm/lockspace.c | 6 ++----
 fs/dlm/memory.c    | 3 +--
 2 files changed, 3 insertions(+), 6 deletions(-)

index f3e7278..41c53b3 100644
@@ -673,10 +673,8 @@ static int new_lockspace(const char *name, const char *cluster,
 	kfree(ls->ls_recover_buf);
  out_lkbidr:
 	idr_destroy(&ls->ls_lkbidr);
-	for (i = 0; i < DLM_REMOVE_NAMES_MAX; i++) {
-		if (ls->ls_remove_names[i])
-			kfree(ls->ls_remove_names[i]);
-	}
+	for (i = 0; i < DLM_REMOVE_NAMES_MAX; i++)
+		kfree(ls->ls_remove_names[i]);
  out_rsbtbl:
 	vfree(ls->ls_rsbtbl);
  out_lsfree:

index 7cd24bc..2d2eaa0 100644
@@ -86,8 +86,7 @@ void dlm_free_lkb(struct dlm_lkb *lkb)
 		struct dlm_user_args *ua;
 		ua = lkb->lkb_ua;
 		if (ua) {
-			if (ua->lksb.sb_lvbptr)
-				kfree(ua->lksb.sb_lvbptr);
+			kfree(ua->lksb.sb_lvbptr);
 			kfree(ua);
 		}
 	}

2.4.4

From: Markus Elfring <[email protected]>
Date: Sat, 29 Nov 2014 18:34:20 +0100

The clk_disable() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/spi/spi-txx9.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 0da7c67..9190124 100644
@@ -402,8 +402,7 @@ exit_busy:
 exit:
 	if (c->workqueue)
 		destroy_workqueue(c->workqueue);
-	if (c->clk)
-		clk_disable(c->clk);
+	clk_disable(c->clk);
 	spi_master_put(master);
 	return ret;
 }

2.1.3

Applied, but please do try to use subject lines matching the style for the subsystem.

From: Markus Elfring <[email protected]>
Date: Sat, 29 Nov 2014 18:55:40 +0100

The pci_dev_put() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/net/ethernet/pasemi/pasemi_mac.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index 30d934d..44e8d7d 100644
@@ -1837,10 +1837,8 @@ pasemi_mac_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	return err;
 
 out:
-	if (mac->iob_pdev)
-		pci_dev_put(mac->iob_pdev);
-	if (mac->dma_pdev)
-		pci_dev_put(mac->dma_pdev);
+	pci_dev_put(mac->iob_pdev);
+	pci_dev_put(mac->dma_pdev);
 
 	free_netdev(dev);
 out_disable_device:

2.1.3

I have heard of at least one case where the problem you are raising happened in practice...

julia

If one case is known then there are probably a lot more that have not been discovered yet.
Maybe this topic should be clarified somewhere (e.g. in "CodingStyle")? On the other hand i always found it obvious that its the callers responsibility to only pass sane parameters to the called functions...

Regards,
Lino

Can you imagine that any more source code places which would benefit from check adjustments because of defensive programming?

Regards,
Markus

I am not sure if i understand your question correctly. But i would not call sanity checks for function parameters "defensive programming". I would rather call it not being totally careless. So to me the question if those checks should be done or not is different from the question whether there are code parts that would benefit from an adjustment to defensive programming.

Regards,
Lino

I guess that the Coccinelle software can also help you in this use case. How do you think about to shield against "unwanted" or unexpected collateral evolutions with additional inline functions?

I assume that a few backporters can tell you more about their corresponding software development experiences.
http://www.do-not-panic.com/2014/04/automatic-linux-kernel-backporting-with-coccinelle.html

Regards,
Markus

In such cases we just provide an appropriate wrapper and replace callers of the original function by callers of the wrapper, typically with a #define.

So this kind of evolution is no problem for the (automated) backports using the backports project - although it can be difficult to detect such a thing is needed.

johannes

That is exactly the problem...

julia

I'm not convinced though that it should stop such progress in mainline.

johannes

Is it progress? These patches match the code look simpler by passing hiding the NULL check inside a function call. Calling pci_dev_put(NULL) doesn't make sense. Just because a sanity check exists doesn't mean we should do insane things.

It's easy enough to store which functions have a sanity check in a database, but to rememember all that as a human being trying to read the code is impossible.

If we really wanted to make this code cleaner we would introduce more error labels with better names.

regards,
dan carpenter

I like to think of progress as using tools to help fix code where we know it can be made simpler with a small ammendment: if you can extend the tools to also vet for safety for backports to avoid crashes even better.

So its a small evolution but we can do better, which is the point you and Julia are making.

It'd crash the system if the function call didn't have the check in place but having the code in question call pci_dev_put(NULL) is also ludicrious. Either way in this case I think we shouldn't go beyond analyzing the function call and if the error check was present before as it is a real case that has introduced crashes before which Julia wanted to flag.

This is easy but it adds complexities which I'd prefer to keep on some other people's workstations. For the developer I think we should strive to only have: a) git b) Coccinelle c) smatch.

Agreed. The problem statement presented by Julia is part of the effort of addressing the "how do we evolve faster" problem on Linux kernel development, what you describe adds to the mix of the complexities, and while Oleg does note that part of this is academic there are those of us who are making things which are academic immediately practical and a reality for Linux. This is also how we evolve faster :)

Can you describe a bit more what you mean here? If we had a label *in code* on the caller, perhaps a comment, I can see tool-wise how it'd remove the requirement for a database for immediate analysis for safety here, ie, we hunt for a label on the code; but other than that its unclear what you mean here.

If you folks agree with my simplication tool analsysi for safety can we devise a tag for whitelisting this check for a series of routines? Where would we put it, in the kernel or a tools package? If in the kernel we could end up sharing it, so I think that's be better. Perhaps scripts/safety/ ? Maybe use a header that describes the safety check that is vetted by the rule present, followed by a list of routines vetted?

Then the Cocci file can preload this and a rule that wants this paranoid check can include this db file for safety ?

The safety here would require vetting thirough history in git that the routine has a check in place throughout the routines's history up to a certain point. I propose we only care up to what kernels are listed on kernel.org as supported.

Luis

I believe this case requires a bit more information explained as to why it was explained. The "form" of change this patch has is of the type that can crash systems if the NULL pointer check on the caller implementation was only added later. We might be able to grammatically check for this situation in the future if we had a white list / black list / kernel revision where the NULL check was added but for now we don't have that and as such care is just required on the developer in consideration for backports.

It should be up to the maintainer to appreciate the gains of doing something differently to make it easier for backporting. I obviously think its a good thing to consider, its extra work though, so only if the maintainer has some appreciation for backporting would this make sense.

In this particular case I've reviewed Julia's concern and I've determined that the patch is safe up to at least v2.6.12-rc2 (which is where our git history begins on Linus' tree), this is because the check for NULL has been there since then:

git show 1da177e drivers/pci/pci-driver.c

+void pci_dev_put(struct pci_dev *dev)
+{
+ if (dev)
+ put_device(&dev->dev);
+}

So this type of wide collateral evolution should not cause panics. Because of this:

But note -- I still think its only good for us to vet these, if we can't why not? If the maintainer doesn't give a shit that's different, but if there are folks out there willing to help with vetting then well, why not :)

PS. Including something like historical vetting as I did above on the commit log should help folks.

Luis

For this particular case:

Note that the "this might cause problems with backports" case is mostly academic in the scope of _this particular driver_. It's still a very valid discussion and issue though.

So I'll be happy to give the ack on this driver, but the larger problem needs consideration still.

-Olof

From: SF Markus Elfring <[email protected]>
Date: Sat, 29 Nov 2014 19:00:17 +0100

Applied.

From: Markus Elfring <[email protected]>
Date: Sun, 30 Nov 2014 10:59:10 +0100

The pci_dev_put() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/acpi/ioapic.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 9a631a6..35963d3 100644
@@ -185,8 +185,7 @@ exit_disable:
 	if (dev)
 		pci_disable_device(dev);
 exit_put:
-	if (dev)
-		pci_dev_put(dev);
+	pci_dev_put(dev);
 exit_free:
 	kfree(ioapic);
 exit:

2.1.3

From: Markus Elfring <[email protected]>
Date: Sun, 30 Nov 2014 17:25:40 +0100

Further update suggestions were taken into account after a patch was applied from static source code analysis.

Markus Elfring (3):
Deletion of unnecessary checks before the function call "kfree" Less function calls in mppe_alloc() after error detection Delete an unnecessary assignment

 drivers/net/ppp/ppp_mppe.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

2.1.3

From: Markus Elfring <[email protected]>
Date: Sun, 30 Nov 2014 17:02:07 +0100

The kfree() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/net/ppp/ppp_mppe.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

index 911b216..7e44212 100644
@@ -238,8 +238,7 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 	return (void *)state;
 
 	out_free:
-	    if (state->sha1_digest)
-		kfree(state->sha1_digest);
+	kfree(state->sha1_digest);
 	    if (state->sha1)
 		crypto_free_hash(state->sha1);
 	    if (state->arc4)

@@ -256,13 +255,12 @@ static void mppe_free(void *arg)
 {
 	struct ppp_mppe_state *state = (struct ppp_mppe_state *) arg;
 	if (state) {
-	    if (state->sha1_digest)
 		kfree(state->sha1_digest);
-	    if (state->sha1)
-		crypto_free_hash(state->sha1);
-	    if (state->arc4)
-		crypto_free_blkcipher(state->arc4);
-	    kfree(state);
+		if (state->sha1)
+			crypto_free_hash(state->sha1);
+		if (state->arc4)
+			crypto_free_blkcipher(state->arc4);
+		kfree(state);
 	}
 }
 

2.1.3

Hello.

Can it be that the previous source code contained unwanted space characters at this place?
Do you want indentation fixes as a separate update step?

Regards,
Markus

Yes, it seems so.

Yes, that would be better to keep it separate.

WBR, Sergei

From: Markus Elfring <[email protected]>
Date: Thu, 4 Dec 2014 22:50:28 +0100

Further update suggestions were taken into account before and after a patch was applied from static source code analysis.

Markus Elfring (6):
Replacement of a printk() call by pr_warn() in mppe_rekey() Fix indentation
Deletion of unnecessary checks before the function call "kfree" Less function calls in mppe_alloc() after error detection Delete an unnecessary assignment in mppe_alloc() Delete another unnecessary assignment in mppe_alloc()

 drivers/net/ppp/ppp_mppe.c | 49 +++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 29 deletions(-)

2.1.3

From: Markus Elfring <[email protected]>
Date: Thu, 4 Dec 2014 18:28:52 +0100

The mppe_rekey() function contained a few update candidates.
* Curly brackets were still used around a single function call "printk".
* Unwanted space characters

Let us improve these implementation details according to the current Linux coding style convention.

---
 drivers/net/ppp/ppp_mppe.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index 911b216..84b7bce 100644
@@ -172,9 +172,8 @@ static void mppe_rekey(struct ppp_mppe_state * state, int initial_key)
 		setup_sg(sg_in, state->sha1_digest, state->keylen);
 		setup_sg(sg_out, state->session_key, state->keylen);
 		if (crypto_blkcipher_encrypt(&desc, sg_out, sg_in,
-					     state->keylen) != 0) {
-    		    printk(KERN_WARNING "mppe_rekey: cipher_encrypt failed\n");
-		}
+					     state->keylen) != 0)
+			pr_warn("mppe_rekey: cipher_encrypt failed\n");
 	} else {
 		memcpy(state->session_key, state->sha1_digest, state->keylen);
 	}

2.1.3

trivia:

			pr_warn("%s: cipher_encrypt failed\n", __func__);

Do you want that I send a third patch series for the fine-tuning of these parameters?

Regards,
Martkus

If you want.

I just wanted you to be aware of it for future patches.

Markus, are you sure that you cannot use netdev_warn in this case?

julia

No. - I did not become familiar enough with the software infrastructure around the mppe_rekey() function.

I do not see so far how I could determine a pointer for the first parameter there. The data structure "ppp_mppe_state" (which is referenced by the input variable "state") does not contain corresponding context information, does it?

Regards,
Markus

Indeed, I also don't see anything promising.

julia

Would "a committer" fix such a small source code adjustment also without a resend of a patch series?

Thanks for your tip.

Does it make sense to express such implementation details in the Linux coding style documentation more explicitly (besides the fact that this update suggestion was also triggered by a warning from the script "checkpatch.pl").

Regards,
Markus

Depends on the committer. Some might, most wouldn't.

drivers/net/ppp doesn't have a specific maintainer.

The networking maintainer generally asks for resends of patches that don't suit his taste, but lots of non-perfect patches still get applied there.

It's a process, and it's not immediate. Wait to see if these get applied as-is. If the embedded function name use, which is trivial, bothers you, send another patch later on that changes it.

Probably not.

Overly formalized coding style rules are perhaps more of a barrier to entry than most want.

Thanks for your constructive feedback.

Not really at the moment ...

I guess that I would prefer a general development of another semantic patch approach according to your request with the topic "Finding embedded function names?" a moment ago.
https://systeme.lip6.fr/pipermail/cocci/2014-December/001517.html
http://article.gmane.org/gmane.comp.version-control.coccinelle/4399

Regards,
Markus

... ciao:

: on "12-4-2014" "Joe Perches" writ:
: > Does it make sense to express such implementation details in the Linux : > coding style documentation more explicitly (besides the fact that this : > update suggestion was also triggered by a warning from the script : > "checkpatch.pl".
:
: Probably not.
:
: Overly formalized coding style rules are perhaps
: more of a barrier to entry than most want.

funny you should mention that. as nothing more than a casual observer,
i'm noticing a "TIRED" sensation reading this thread. i have "0" confidence a "SERIOUS" participant's enthusiasm would remain untested.

however, the "checkpatch.pl" warning suggests an assumed 'custom'. i
can't tell if this a 'serious' issue, or "pickin' fly shit out of pepper".

but from my reading of it, the "CODE" , and the "logic" driving it, is
not the problem.

season's best ...

... it's not what you see ,
    but in stead , notice ...

Doing so may potentially allow some strings to be shared, thus saving a little space. Perhaps not in this case, though.

julia

It's not necessarily a code size savings in any case.

It can be, but the real benefits are stylistic consistency and lack of mismatch between function name and message.

If the code is refactored or copy/pasted into another function, a moderately common defect is not modifying the embedded function name in the message.

There may be some smallish savings if ever these __func__ uses were converted to use %pf via some internal standardized mechanism.

A negative to that approach is inlined functions would take the function name of the parent not keep the inlined function name.

I tried the following:

#include <stdio.h>

inline int foo() {
printf("%s %x\n",__func__,0x12345);
}

int main () {
foo();
}

The assembly code generated for main is:

0000000000400470 <main>:

  400470:       b9 45 23 01 00          mov    $0x12345,%ecx
  400475:       ba 4b 06 40 00          mov    $0x40064b,%edx
  40047a:       be 44 06 40 00          mov    $0x400644,%esi
  40047f:       bf 01 00 00 00          mov    $0x1,%edi
  400484:       31 c0                   xor    %eax,%eax
  400486:       e9 d5 ff ff ff          jmpq   400460 <__printf_chk@plt>

That is, the call to foo seems tom be inlined.

But the output is:

foo 12345

So it seems that __func__ is determined before inlining.

julia

True, and that's what I intended to describe.

If you did that with a kernel module and replaced "%s, __func__" with "%pf, __builtin_return_address(0)" when built with kallsyms you should get:

"modname 12345" when most would expect "foo 12345"

when built without kallsyms, that output should be "<address> 12345"

but the object code should be smaller.

OK. But the semantic patch is only using __func__ and only in cases where the string wanted is similar to the name of the current function, so I think it should be OK?

julia

Yes, it'd be a good thing.

From: Markus Elfring <[email protected]>
Date: Thu, 4 Dec 2014 22:15:20 +0100

The implementations of the functions "mppe_alloc" and "mppe_free" contained unwanted space characters.

Let us improve the indentation according to the Linux coding style convention.

---
 drivers/net/ppp/ppp_mppe.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

index 84b7bce..b80af29 100644
@@ -236,15 +236,15 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 
 	return (void *)state;
 
-	out_free:
-	    if (state->sha1_digest)
+out_free:
+	if (state->sha1_digest)
 		kfree(state->sha1_digest);
-	    if (state->sha1)
+	if (state->sha1)
 		crypto_free_hash(state->sha1);
-	    if (state->arc4)
+	if (state->arc4)
 		crypto_free_blkcipher(state->arc4);
-	    kfree(state);
-	out:
+	kfree(state);
+out:
 	return NULL;
 }
 

@@ -255,13 +255,13 @@ static void mppe_free(void *arg)
 {
 	struct ppp_mppe_state *state = (struct ppp_mppe_state *) arg;
 	if (state) {
-	    if (state->sha1_digest)
-		kfree(state->sha1_digest);
-	    if (state->sha1)
-		crypto_free_hash(state->sha1);
-	    if (state->arc4)
-		crypto_free_blkcipher(state->arc4);
-	    kfree(state);
+		if (state->sha1_digest)
+			kfree(state->sha1_digest);
+		if (state->sha1)
+			crypto_free_hash(state->sha1);
+		if (state->arc4)
+			crypto_free_blkcipher(state->arc4);
+		kfree(state);
 	}
 }
 

2.1.3

From: Markus Elfring <[email protected]>
Date: Thu, 4 Dec 2014 22:22:23 +0100

The kfree() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/net/ppp/ppp_mppe.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index b80af29..94ff216 100644
@@ -237,8 +237,7 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 	return (void *)state;
 
 out_free:
-	if (state->sha1_digest)
-		kfree(state->sha1_digest);
+	kfree(state->sha1_digest);
 	if (state->sha1)
 		crypto_free_hash(state->sha1);
 	if (state->arc4)

@@ -255,8 +254,7 @@ static void mppe_free(void *arg)
 {
 	struct ppp_mppe_state *state = (struct ppp_mppe_state *) arg;
 	if (state) {
-		if (state->sha1_digest)
-			kfree(state->sha1_digest);
+		kfree(state->sha1_digest);
 		if (state->sha1)
 			crypto_free_hash(state->sha1);
 		if (state->arc4)

2.1.3

From: Markus Elfring <[email protected]>
Date: Thu, 4 Dec 2014 22:30:20 +0100

The functions crypto_free_blkcipher((), crypto_free_hash() and kfree() could be called in some cases by the mppe_alloc() function during error handling even if the passed data structure element contained still a null pointer.

This implementation detail could be improved by adjustments for jump labels.

---
 drivers/net/ppp/ppp_mppe.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

index 94ff216..c82198f 100644
@@ -196,11 +196,11 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 
 	if (optlen != CILEN_MPPE + sizeof(state->master_key) ||
 	    options[0] != CI_MPPE || options[1] != CILEN_MPPE)
-		goto out;
+		return NULL;
 
 	state = kzalloc(sizeof(*state), GFP_KERNEL);
 	if (state = NULL)
-		goto out;
+		return NULL;
 
 
 	state->arc4 = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);

@@ -212,16 +212,16 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 	state->sha1 = crypto_alloc_hash("sha1", 0, CRYPTO_ALG_ASYNC);
 	if (IS_ERR(state->sha1)) {
 		state->sha1 = NULL;
-		goto out_free;
+		goto out_free_blkcipher;
 	}
 
 	digestsize = crypto_hash_digestsize(state->sha1);
 	if (digestsize < MPPE_MAX_KEY_LEN)
-		goto out_free;
+		goto out_free_hash;
 
 	state->sha1_digest = kmalloc(digestsize, GFP_KERNEL);
 	if (!state->sha1_digest)
-		goto out_free;
+		goto out_free_hash;
 
 	/* Save keys. */
 	memcpy(state->master_key, &options[CILEN_MPPE],

@@ -236,14 +236,12 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 
 	return (void *)state;
 
+out_free_hash:
+	crypto_free_hash(state->sha1);
+out_free_blkcipher:
+	crypto_free_blkcipher(state->arc4);
 out_free:
-	kfree(state->sha1_digest);
-	if (state->sha1)
-		crypto_free_hash(state->sha1);
-	if (state->arc4)
-		crypto_free_blkcipher(state->arc4);
 	kfree(state);
-out:
 	return NULL;
 }
 

2.1.3

From: Markus Elfring <[email protected]>
Date: Thu, 4 Dec 2014 22:33:34 +0100

The data structure element "arc4" was assigned a null pointer by the mppe_alloc() function if a previous function call "crypto_alloc_blkcipher" failed. This assignment became unnecessary with previous source code adjustments.

Let us delete it from the affected implementation because the element "arc4" will not be accessible outside the function after the detected allocation failure.

---
 drivers/net/ppp/ppp_mppe.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index c82198f..b7db4b1 100644
@@ -204,10 +204,8 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 
 
 	state->arc4 = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);
-	if (IS_ERR(state->arc4)) {
-		state->arc4 = NULL;
+	if (IS_ERR(state->arc4))
 		goto out_free;
-	}
 
 	state->sha1 = crypto_alloc_hash("sha1", 0, CRYPTO_ALG_ASYNC);
 	if (IS_ERR(state->sha1)) {

2.1.3

No. crypto_alloc_blkcipher() returns error pointers and not NULL.

This patch creates a bug.

regards,
dan carpenter

That is true.

Please explain: How?

Did you notice that the data structure element "arc4" was reset to a null pointer if a failure was detected for the function call "crypto_alloc_blkcipher"?

Do you find this specific assignment still necessary for exception handling in the implementation of mppe_alloc() function?

Regards,
Markus

Oh. In that case, I misunderstood what you wrote. Looking at it now, this patch is actually ok.

regards,
dan carpenter

I find it a bit interesting how this misunderstanding could happen here somehow.

Does that mean that you would like to add any tag like "Acked-by" or "Reviewed-by" to any of the proposed six update steps?

Regards,
Markus

From: Markus Elfring <[email protected]>
Date: Thu, 4 Dec 2014 22:42:30 +0100

The data structure element "sha1" was assigned a null pointer by the mppe_alloc() after a function call "crypto_alloc_hash" failed. It was determined that this element was not accessed by the implementation of the crypto_free_blkcipher() function.

Let us delete it from the affected implementation because the element "sha1" will not be accessible outside the function after the detected allocation failure.

---
 drivers/net/ppp/ppp_mppe.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index b7db4b1..32cb054 100644
@@ -208,10 +208,8 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 		goto out_free;
 
 	state->sha1 = crypto_alloc_hash("sha1", 0, CRYPTO_ALG_ASYNC);
-	if (IS_ERR(state->sha1)) {
-		state->sha1 = NULL;
+	if (IS_ERR(state->sha1))
 		goto out_free_blkcipher;
-	}
 
 	digestsize = crypto_hash_digestsize(state->sha1);
 	if (digestsize < MPPE_MAX_KEY_LEN)

2.1.3

This patch is also buggy.

regards,
dan carpenter

Do you really want to keep a variable reset after a detected failure?

Regards,
Markus

Actually it's ok. I was just confused by the changelog.

Sorry about that, Markus.

regards,
dan carpenter

From: SF Markus Elfring <[email protected]>
Date: Thu, 04 Dec 2014 23:03:30 +0100

Generally speaking, it is advisable to not leave error pointers in data structures, even if they are about to be free'd up in an error path anyways.

Therefore I do not like some of the patches in this series.

Sorry.

Can you give any more concrete feedback here?

Regards,
Markus

From: SF Markus Elfring <[email protected]>
Date: Fri, 12 Dec 2014 08:01:54 +0100

I gave you very concrete feedback, I said exactly that I don't want error pointers left in data structure members.

I cannot describe my requirements any more precisely than that.

I find that your critique affects the proposed update steps four to six, doesn't it?
Are the other steps acceptable in principle?

I hope that a bit more constructive suggestions will be contributed by involved
software developers around the affected source code. Now it seems that a small code clean-up becomes a more challenging development task.

How do you prefer to redesign corresponding data structures eventually?

Regards,
Markus

You are asking me to invest a lot of time for a very trivial set of changes.

Why don't you just integrate the feedback you were given and resubmit your patch series, just like any other developer would?

I find the proposed six update steps also trivial so far.

I find the requested redesign and reorganisation of involved data structures not so easy and therefore more challenging at the moment.
Where should "the error pointers" be stored instead?
Is such a software improvement even a kind of add-on to my patch series?

Regards,
Markus

From: SF Markus Elfring <[email protected]>
Date: Fri, 12 Dec 2014 17:56:36 +0100

A local variable, before you assign it into the datastructure.

Will it be acceptable for you that anyone (or even me) will introduce such a change with a seventh (and eventually eighth) update step here? Do you want any other sequence for source code preparation of the requested software improvement?

Regards,
Markus

The thing is : We are in the merge window, tracking bugs added in latest dev cycle.

Having to deal with patches like yours is adding pressure on the maintainer (and other developers) at the wrong time.

I am also curious on the software evolution about how many improvements will arrive in the next Linux versions.

You can relax a bit eventually. More merge windows will follow, won't they?

It will be nice if a bunch of recent code clean-ups which were also triggered
by static source code analysis will be integrated into Linux 3.19 already. More update suggestions will be considered later again as usual.

Regards,
Markus

From: SF Markus Elfring <[email protected]>
Date: Fri, 12 Dec 2014 18:22:48 +0100

I'd like to honestly ask why you are being so difficult?

Everyone gets their code reviewed, everyone has to modify their changes to adhere to the subsystem maintainer's wishes. You are not being treated specially, and quite frankly nobody is asking anything unreasonable of you.

There are several factors which contribute to your perception of difficulty here.

1. I try to extract from every feedback the information about the amount of acceptance or rejection for a specific update suggestion. A terse feedback (like yours for this issue) makes it occasionally
harder to see the next useful steps. So another constructive discussion is evolving around the clarification of some implementation details.

2. I prefer also different communication styles at some points.

3. I reached a point where the desired software updates were not immediately obvious for me while other contributors might have achieved a better understanding for the affected issues already.

4. I am on the way at the moment to get my Linux software development system running again.

https://forums.opensuse.org/showthread.php/503327-System-startup-does-not-continue-after-hard-disk-detection

That is fine as usual.

That is also true as the software development process will be continued.

Regards,
Markus

I have looked at the affected software infrastructure once more. Now I find still that your data reorgansisation wish can not be resolved in a simple way.

I imagine that your update suggestion would mean that the corresponding pointers will be passed around by function parameters instead, wouldn't it?

Two pointers were stored as the members "arc4" and "sha1" of the data structure "ppp_mppe_state" for a specific reason. A pointer to this structure is passed to the ppp_register_compressor() function.
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/include/linux/ppp-comp.h?id`7ca46e97a1b6594b29647d98a32d545c24bdff#n32

The data structure "compressor" manages some function pointers. I assume that this interface should not be changed at the moment, should it?

Are further ideas needed here?

Regards,
Markus

From: SF Markus Elfring <[email protected]>
Date: Thu, 18 Dec 2014 18:23:08 +0100

I'm saying to leave the code alone.

If it goes:

	var = foo_that_returns_ptr_err()
	if (IS_ERR(var))
		return PTR_ERR(var);

	p->bar = var;

or whatever, simply keep it that way!

I'm not engaging in this conversation any further, you have already consumed way too much of my limited time on this incredibly trivial matter.

It seems that there might be a misunderstanding between us.

A simple return was not used by the mppe_alloc() function so far because a bit of memory clean-up will also be useful after error detection, won't it?

It can occasionally happen that a safe clarification of specific implementation details will need more efforts than you would like to invest at the moment.

Regards,
Markus

Do I need to try another interpretation out for your feedback?

Do you want to express here that a data structure member should only be set after a previous function call succeeded?

I hope that you will find a bit time and patience again to clarify affected implementation details in a safer and unambiguous way.

Regards,
Markus

Hi Markus,

I think what David said was pretty clear: If you see code like the above there is no need to refactor it. That does not mean that this is the _preferred_ way of error handling. Its just good enough to be left alone.

Regards,
Lino

I can understand this view in principle.

Can your feedback help in the clarification of suggestions around my update steps one to six for this Linux software module?

Regards,
Markus

From: SF Markus Elfring <[email protected]>
Date: Sat, 20 Dec 2014 15:45:32 +0100

Sorry, another developer will have to hold your hand, as I said I already invested too much time into this.

This is often the case. Doing something half way is not useful.

julia

... ciao:

: on "12-12-2014" "SF Markus Elfring" writ:

:> I don't want error pointers left in data structure members. :> I cannot describe my requirements any more precisely than that.

to me, "error pointers 'left' in data structure members", seems a
little cavalier. objection to their existence tends to suggest that not a function of the design, or logic involved.

: I hope that a bit more constructive suggestions will be contributed by : involved software developers around the affected source code.

so, this 'error pointers' issue fails to meet your 'constructive'
suggestion threshold. to my mind, what i choose to ignore, imposes a dangerous minimum. what are you willing to ignore.

: Now it seems that a small code clean-up becomes a more challenging : development task.

first, if you've EVER dealt with imperfect code, that should be a
given. but more importantly to me, i never knew "code clean-up", wasn't "REQUIRED" ...

... it's not what you see ,
    but in stead , notice ...

From 06c1a0fff81142dfa6d933479e17bb1b45ab9dc7 Mon Sep 17 00:00:00 2001
From: Markus Elfring <[email protected]>
Date: Sun, 30 Nov 2014 17:07:34 +0100

The functions crypto_free_blkcipher((), crypto_free_hash() and kfree() could be called in some cases by the mppe_alloc() function during error handling even if the passed data structure element contained still a null pointer. This implementation detail could be improved by adjustments for jump labels.

---
 drivers/net/ppp/ppp_mppe.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

index 7e44212..962c1a0 100644
@@ -197,11 +197,11 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 
 	if (optlen != CILEN_MPPE + sizeof(state->master_key) ||
 	    options[0] != CI_MPPE || options[1] != CILEN_MPPE)
-		goto out;
+		return NULL;
 
 	state = kzalloc(sizeof(*state), GFP_KERNEL);
 	if (state = NULL)
-		goto out;
+		return NULL;
 
 
 	state->arc4 = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);

@@ -213,16 +213,16 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 	state->sha1 = crypto_alloc_hash("sha1", 0, CRYPTO_ALG_ASYNC);
 	if (IS_ERR(state->sha1)) {
 		state->sha1 = NULL;
-		goto out_free;
+		goto out_free_blkcipher;
 	}
 
 	digestsize = crypto_hash_digestsize(state->sha1);
 	if (digestsize < MPPE_MAX_KEY_LEN)
-		goto out_free;
+		goto out_free_hash;
 
 	state->sha1_digest = kmalloc(digestsize, GFP_KERNEL);
 	if (!state->sha1_digest)
-		goto out_free;
+		goto out_free_hash;
 
 	/* Save keys. */
 	memcpy(state->master_key, &options[CILEN_MPPE],

@@ -237,14 +237,12 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 
 	return (void *)state;
 
-	out_free:
-	kfree(state->sha1_digest);
-	    if (state->sha1)
-		crypto_free_hash(state->sha1);
-	    if (state->arc4)
-		crypto_free_blkcipher(state->arc4);
-	    kfree(state);
-	out:
+out_free_hash:
+	crypto_free_hash(state->sha1);
+out_free_blkcipher:
+	crypto_free_blkcipher(state->arc4);
+out_free:
+	kfree(state);
 	return NULL;
 }
 

2.1.3

From: Markus Elfring <[email protected]>
Date: Sun, 30 Nov 2014 17:17:36 +0100

The data structure element "arc4" was assigned a null pointer by the mppe_alloc() function if a previous function call "crypto_alloc_blkcipher" failed. This assignment became unnecessary with previous source code adjustments.
Let us delete it from the affected implementation because the element "arc4" will not be accessible outside the function after the detected allocation failure.

---
 drivers/net/ppp/ppp_mppe.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index 962c1a0..d913bc9 100644
@@ -205,10 +205,8 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 
 
 	state->arc4 = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);
-	if (IS_ERR(state->arc4)) {
-		state->arc4 = NULL;
+	if (IS_ERR(state->arc4))
 		goto out_free;
-	}
 
 	state->sha1 = crypto_alloc_hash("sha1", 0, CRYPTO_ALG_ASYNC);
 	if (IS_ERR(state->sha1)) {

2.1.3

I did not fiddle with the data structure element "sha1" because I assumed that it might be still relevant for the call of a function like crypto_free_blkcipher().

I have got different preferences for change distribution over several patches here.
I find it safer to tackle an assignment clean-up after adjustments for jump labels.

Do you want that I resend my update suggestion?

Regards,
Markus

From: Markus Elfring <[email protected]>
Date: Sun, 30 Nov 2014 20:50:15 +0100

Another update suggestion was taken into account after a patch was applied from static source code analysis.

Markus Elfring (2):
Deletion of unnecessary checks before the function call "release_firmware" One function call less in si2157_init() after error detection

 drivers/media/tuners/si2157.c | 9 ++++-----
 drivers/media/tuners/xc5000.c | 3 +--
 2 files changed, 5 insertions(+), 7 deletions(-)

2.1.3

From: Markus Elfring <[email protected]>
Date: Sun, 30 Nov 2014 19:05:48 +0100

The release_firmware() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/media/tuners/si2157.c | 3 +--
 drivers/media/tuners/xc5000.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

index 8e57696..e17ab1f 100644
@@ -196,8 +196,7 @@ warm:
 	return 0;
 
 err:
-	if (fw)
-		release_firmware(fw);
+	release_firmware(fw);
 
 	dev_dbg(&s->client->dev, "failed=%d\n", ret);
 	return ret;

index 705c258..2a039de 100644
@@ -1336,8 +1336,7 @@ static int xc5000_release(struct dvb_frontend *fe)
 
 	if (priv) {
 		cancel_delayed_work(&priv->timer_sleep);
-		if (priv->firmware)
-			release_firmware(priv->firmware);
+		release_firmware(priv->firmware);
 		hybrid_tuner_release_state(priv);
 	}
 

2.1.3

From: Markus Elfring <[email protected]>
Date: Sun, 30 Nov 2014 20:48:24 +0100

The release_firmware() function was called in some cases by the si2157_init() function during error handling even if the passed variable contained still a null pointer. This implementation detail could be improved
by the introduction of another jump label.

---
 drivers/media/tuners/si2157.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index e17ab1f..2180de9 100644
@@ -157,7 +157,7 @@ static int si2157_init(struct dvb_frontend *fe)
 		dev_err(&s->client->dev, "firmware file '%s' is invalid\n",
 				fw_file);
 		ret = -EINVAL;
-		goto err;
+		goto fw_release_exit;
 	}
 
 	dev_info(&s->client->dev, "downloading firmware from file '%s'\n",

@@ -173,7 +173,7 @@ static int si2157_init(struct dvb_frontend *fe)
 			dev_err(&s->client->dev,
 					"firmware download failed=%d\n",
 					ret);
-			goto err;
+			goto fw_release_exit;
 		}
 	}
 

@@ -195,9 +195,9 @@ warm:
 	s->active = true;
 	return 0;
 
-err:
+fw_release_exit:
 	release_firmware(fw);
-
+err:
 	dev_dbg(&s->client->dev, "failed=%d\n", ret);
 	return ret;
 }

2.1.3

From: Markus Elfring <[email protected]>
Date: Sun, 30 Nov 2014 22:50:20 +0100

The dvb_unregister_device() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/media/pci/ddbridge/ddbridge-core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index c82e855..9e3492e 100644
@@ -1118,8 +1118,7 @@ static void ddb_ports_detach(struct ddb *dev)
 			dvb_input_detach(port->input[1]);
 			break;
 		case DDB_PORT_CI:
-			if (port->output->dev)
-				dvb_unregister_device(port->output->dev);
+			dvb_unregister_device(port->output->dev);
 			if (port->en) {
 				ddb_input_stop(port->input[0]);
 				ddb_output_stop(port->output);

2.1.3

From: Markus Elfring <[email protected]>
Date: Sun, 30 Nov 2014 23:10:51 +0100

The vb2_put_vma() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/media/v4l2-core/videobuf2-vmalloc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 3966b12..fba944e 100644
@@ -154,8 +154,7 @@ static void vb2_vmalloc_put_userptr(void *buf_priv)
 		}
 		kfree(buf->pages);
 	} else {
-		if (buf->vma)
-			vb2_put_vma(buf->vma);
+		vb2_put_vma(buf->vma);
 		iounmap(buf->vaddr);
 	}
 	kfree(buf);

2.1.3

Hello,

Marek Szyprowski, PhD
Samsung R&D Institute Poland

From: Markus Elfring <[email protected]>
Date: Mon, 1 Dec 2014 05:45:42 +0100

The thermal_zone_device_unregister() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/thermal/int340x_thermal/int3403_thermal.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 6e9fb62..790b8f6 100644
@@ -293,8 +293,7 @@ static int int3403_sensor_add(struct int3403_priv *priv)
 	return 0;
 
  err_free_obj:
-	if (obj->tzone)
-		thermal_zone_device_unregister(obj->tzone);
+	thermal_zone_device_unregister(obj->tzone);
 	return result;
 }
 

2.1.3

applied.

thanks,
rui

From: Markus Elfring <[email protected]>
Date: Mon, 1 Dec 2014 06:06:57 +0100

The free_percpu() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/dma/dmaengine.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 24bfaf0..e057935 100644
@@ -330,8 +330,7 @@ static int __init dma_channel_table_init(void)
 	if (err) {
 		pr_err("initialization failure\n");
 		for_each_dma_cap_mask(cap, dma_cap_mask_all)
-			if (channel_table[cap])
-				free_percpu(channel_table[cap]);
+			free_percpu(channel_table[cap]);
 	}
 
 	return err;

2.1.3

Please make sure you use the right subsystem name

Applied, thanks

~Vinod

From: Markus Elfring <[email protected]>
Date: Mon, 1 Dec 2014 11:18:21 +0100

The ceph_put_snap_context() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/block/rbd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 27b71a0..3c34ab5 100644
@@ -3405,8 +3405,7 @@ err_rq:
 	if (result)
 		rbd_warn(rbd_dev, "%s %llx at %llx result %d",
 			 obj_op_name(op_type), length, offset, result);
-	if (snapc)
-		ceph_put_snap_context(snapc);
+	ceph_put_snap_context(snapc);
 	blk_end_request_all(rq, result);
 }
 

2.1.3