| Message ID | 1403356646-29443-1-git-send-email-rickard_strandqvist@spectrumdigital.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
(Adding Geert, who probably wrote most of this and likely might have forgotten all of it) On Sat, 2014-06-21 at 15:17 +0200, Rickard Strandqvist wrote: > Adding missing code for managing a memory allocation error that may occur. > > This was partly found using a static code analysis program called cppcheck. skeletonfb.c is not meant to be compiled. It's a sample driver template. Those /* goto error path */ lines are for driver writers that use this to figure out what to do. The second return is not correct as it would not free the first alloc'd block. > diff --git a/drivers/video/fbdev/skeletonfb.c b/drivers/video/fbdev/skeletonfb.c [] > @@ -692,6 +692,7 @@ static int xxxfb_probe(struct pci_dev *dev, const struct pci_device_id *ent) > > if (!info) { > /* goto error path */ > + return -ENOMEM; > } > > par = info->par; > @@ -746,6 +747,7 @@ static int xxxfb_probe(struct pci_dev *dev, const struct pci_device_id *ent) > info->pixmap.addr = kmalloc(PIXMAP_SIZE, GFP_KERNEL); > if (!info->pixmap.addr) { > /* goto error */ > + return -ENOMEM; > } > > info->pixmap.size = PIXMAP_SIZE; > -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jun 21, 2014 at 3:58 PM, Joe Perches <joe@perches.com> wrote: > (Adding Geert, who probably wrote most of this > and likely might have forgotten all of it) > > On Sat, 2014-06-21 at 15:17 +0200, Rickard Strandqvist wrote: >> Adding missing code for managing a memory allocation error that may occur. >> >> This was partly found using a static code analysis program called cppcheck. > > skeletonfb.c is not meant to be compiled. > It's a sample driver template. Yep. > Those /* goto error path */ lines are for > driver writers that use this to figure out > what to do. Still, it's nice if a sample driver has correct error handling. Just comments are not good enough as guidance for some people ;-) > The second return is not correct as it would > not free the first alloc'd block. Indeed. Better do it right. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sounds like a good idea to have better error handling although it is an example, even more important there. At the first return nothing has been allocated, so it's ok I guess. At the second I reflect about adding some deallocation, there are comments on some form of goto error part which would appropriately handle a deallocation. But all the other places in the code is just do a return, that was why I chose to do it at the second return to. Kind regards Rickard Strandqvist 2014-06-21 22:48 GMT+02:00 Geert Uytterhoeven <geert@linux-m68k.org>: > On Sat, Jun 21, 2014 at 3:58 PM, Joe Perches <joe@perches.com> wrote: >> (Adding Geert, who probably wrote most of this >> and likely might have forgotten all of it) >> >> On Sat, 2014-06-21 at 15:17 +0200, Rickard Strandqvist wrote: >>> Adding missing code for managing a memory allocation error that may occur. >>> >>> This was partly found using a static code analysis program called cppcheck. >> >> skeletonfb.c is not meant to be compiled. >> It's a sample driver template. > > Yep. > >> Those /* goto error path */ lines are for >> driver writers that use this to figure out >> what to do. > > Still, it's nice if a sample driver has correct error handling. > Just comments are not good enough as guidance for some people ;-) > >> The second return is not correct as it would >> not free the first alloc'd block. > > Indeed. Better do it right. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/video/fbdev/skeletonfb.c b/drivers/video/fbdev/skeletonfb.c index fefde7c..ee6dc7e 100644 --- a/drivers/video/fbdev/skeletonfb.c +++ b/drivers/video/fbdev/skeletonfb.c @@ -692,6 +692,7 @@ static int xxxfb_probe(struct pci_dev *dev, const struct pci_device_id *ent) if (!info) { /* goto error path */ + return -ENOMEM; } par = info->par; @@ -746,6 +747,7 @@ static int xxxfb_probe(struct pci_dev *dev, const struct pci_device_id *ent) info->pixmap.addr = kmalloc(PIXMAP_SIZE, GFP_KERNEL); if (!info->pixmap.addr) { /* goto error */ + return -ENOMEM; } info->pixmap.size = PIXMAP_SIZE;
Adding missing code for managing a memory allocation error that may occur. This was partly found using a static code analysis program called cppcheck. Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> --- drivers/video/fbdev/skeletonfb.c | 2 ++ 1 file changed, 2 insertions(+)