| Message ID | 20220717133759.8479-1-khalid.masum.92@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Bluetooth: hci_core: Use ERR_PTR instead of NULL | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
Hi Khalid, Khalid Masum <khalid.masum.92@gmail.com> says: > Failure of kzalloc to allocate memory is not reported. Return Error > pointer to ENOMEM if memory allocation fails. This will increase > readability and will make the function easier to use in future. > > Signed-off-by: Khalid Masum <khalid.masum.92@gmail.com> > --- [snip] > index a0f99baafd35..ea50767e02bf 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -2419,7 +2419,7 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv) > > hdev = kzalloc(alloc_size, GFP_KERNEL); > if (!hdev) > - return NULL; > + return ERR_PTR(-ENOMEM); > This will break all callers of hci_alloc_dev(). All callers expect NULL in case of an error, so you will leave them with wrong pointer. Also, allocation functionS return an error only in case of ENOMEM, so initial code is fine, IMO Thanks, --Pavel Skripkin
On Sun, Jul 17, 2022 at 10:17 PM Pavel Skripkin <paskripkin@gmail.com> wrote: > > Hi Khalid, > > Khalid Masum <khalid.masum.92@gmail.com> says: > > Failure of kzalloc to allocate memory is not reported. Return Error > > pointer to ENOMEM if memory allocation fails. This will increase > > readability and will make the function easier to use in future. > > > > Signed-off-by: Khalid Masum <khalid.masum.92@gmail.com> > > --- > > [snip] > > > index a0f99baafd35..ea50767e02bf 100644 > > --- a/net/bluetooth/hci_core.c > > +++ b/net/bluetooth/hci_core.c > > @@ -2419,7 +2419,7 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv) > > > > hdev = kzalloc(alloc_size, GFP_KERNEL); > > if (!hdev) > > - return NULL; > > + return ERR_PTR(-ENOMEM); > > > > This will break all callers of hci_alloc_dev(). All callers expect NULL > in case of an error, so you will leave them with wrong pointer. You are right. All callers of hci_alloc_dev() need to be able to handle the error pointer. I shall send a V2 with all the callers of hci_alloc_dev handling the ERR_PTR. > Also, allocation functionS return an error only in case of ENOMEM, so > initial code is fine, IMO > I think it makes the memory allocation error handling look to be a bit different from what we usually do while allocating memory which is, returning an error or an error pointer. Here we are returning a NULL without any context, making it a bit unreadable. So I think returning an error pointer is better. If I am not mistaken, this also complies with the return convention: https://www.kernel.org/doc/htmldocs/kernel-hacking/convention-returns.html > > Thanks, > --Pavel Skripkin Thanks, -- Khalid Masum
Hi Khalid, On Sun, Jul 17, 2022 at 11:34 AM Khalid Masum <khalid.masum.92@gmail.com> wrote: > > On Sun, Jul 17, 2022 at 10:17 PM Pavel Skripkin <paskripkin@gmail.com> wrote: > > > > Hi Khalid, > > > > Khalid Masum <khalid.masum.92@gmail.com> says: > > > Failure of kzalloc to allocate memory is not reported. Return Error > > > pointer to ENOMEM if memory allocation fails. This will increase > > > readability and will make the function easier to use in future. > > > > > > Signed-off-by: Khalid Masum <khalid.masum.92@gmail.com> > > > --- > > > > [snip] > > > > > index a0f99baafd35..ea50767e02bf 100644 > > > --- a/net/bluetooth/hci_core.c > > > +++ b/net/bluetooth/hci_core.c > > > @@ -2419,7 +2419,7 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv) > > > > > > hdev = kzalloc(alloc_size, GFP_KERNEL); > > > if (!hdev) > > > - return NULL; > > > + return ERR_PTR(-ENOMEM); > > > > > > > This will break all callers of hci_alloc_dev(). All callers expect NULL > > in case of an error, so you will leave them with wrong pointer. > > You are right. All callers of hci_alloc_dev() need to be able to handle > the error pointer. I shall send a V2 with all the callers of hci_alloc_dev > handling the ERR_PTR. > > > Also, allocation functionS return an error only in case of ENOMEM, so > > initial code is fine, IMO > > If there just a single error like ENOMEM then Id say this is fine, just as it is fine for kzalloc. > I think it makes the memory allocation error handling look to be a bit > different from what we usually do while allocating memory which is, > returning an error or an error pointer. Here we are returning a NULL > without any context, making it a bit unreadable. So I think returning > an error pointer is better. If I am not mistaken, this also complies with > the return convention: > https://www.kernel.org/doc/htmldocs/kernel-hacking/convention-returns.html Not sure if that would apply to code that is basically a wrapper of kzalloc. > > > > Thanks, > > --Pavel Skripkin > > > Thanks, > -- Khalid Masum
On Tue, Jul 19, 2022 at 5:04 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Khalid, > > On Sun, Jul 17, 2022 at 11:34 AM Khalid Masum <khalid.masum.92@gmail.com> wrote: > > > > On Sun, Jul 17, 2022 at 10:17 PM Pavel Skripkin <paskripkin@gmail.com> wrote: > > > > > > Hi Khalid, > > > > > > Khalid Masum <khalid.masum.92@gmail.com> says: > > > > Failure of kzalloc to allocate memory is not reported. Return Error > > > > pointer to ENOMEM if memory allocation fails. This will increase > > > > readability and will make the function easier to use in future. > > > > > > > > Signed-off-by: Khalid Masum <khalid.masum.92@gmail.com> > > > > --- > > > > > > [snip] > > > > > > > index a0f99baafd35..ea50767e02bf 100644 > > > > --- a/net/bluetooth/hci_core.c > > > > +++ b/net/bluetooth/hci_core.c > > > > @@ -2419,7 +2419,7 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv) > > > > > > > > hdev = kzalloc(alloc_size, GFP_KERNEL); > > > > if (!hdev) > > > > - return NULL; > > > > + return ERR_PTR(-ENOMEM); > > > > > > > > > > This will break all callers of hci_alloc_dev(). All callers expect NULL > > > in case of an error, so you will leave them with wrong pointer. > > > > You are right. All callers of hci_alloc_dev() need to be able to handle > > the error pointer. I shall send a V2 with all the callers of hci_alloc_dev > > handling the ERR_PTR. > > > > > Also, allocation functionS return an error only in case of ENOMEM, so > > > initial code is fine, IMO > > > > > If there just a single error like ENOMEM then Id say this is fine, > just as it is fine for kzalloc. > > > I think it makes the memory allocation error handling look to be a bit > > different from what we usually do while allocating memory which is, > > returning an error or an error pointer. Here we are returning a NULL > > without any context, making it a bit unreadable. So I think returning > > an error pointer is better. If I am not mistaken, this also complies with > > the return convention: > > https://www.kernel.org/doc/htmldocs/kernel-hacking/convention-returns.html > > Not sure if that would apply to code that is basically a wrapper of kzalloc. I got you. > > > Thanks, > > > --Pavel Skripkin > > > > > > Thanks, > > -- Khalid Masum > > > > -- > Luiz Augusto von Dentz Thanks, -- Khalid Masum
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index e25fcd49db70..3407762b3b15 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -3692,8 +3692,8 @@ static int btusb_probe(struct usb_interface *intf, data->recv_acl = hci_recv_frame; hdev = hci_alloc_dev_priv(priv_size); - if (!hdev) - return -ENOMEM; + if (IS_ERR(hdev)) + return PTR_ERR(hdev); hdev->bus = HCI_USB; hci_set_drvdata(hdev, data); diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index a0f99baafd35..ea50767e02bf 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -2419,7 +2419,7 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv) hdev = kzalloc(alloc_size, GFP_KERNEL); if (!hdev) - return NULL; + return ERR_PTR(-ENOMEM); hdev->pkt_type = (HCI_DM1 | HCI_DH1 | HCI_HV1); hdev->esco_type = (ESCO_HV1);
Failure of kzalloc to allocate memory is not reported. Return Error pointer to ENOMEM if memory allocation fails. This will increase readability and will make the function easier to use in future. Signed-off-by: Khalid Masum <khalid.masum.92@gmail.com> --- drivers/bluetooth/btusb.c | 4 ++-- net/bluetooth/hci_core.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)