virtualization.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
* [patch] vhost: NULL vs ERR_PTR bug
@ 2015-07-15 11:16 Dan Carpenter
  2015-07-15 11:28 ` Michael S. Tsirkin
       [not found] ` <20150715142420-mutt-send-email-mst@redhat.com>
  0 siblings, 2 replies; 6+ messages in thread
From: Dan Carpenter @ 2015-07-15 11:16 UTC (permalink / raw)
  To: Michael S. Tsirkin, Igor Mammedov; +Cc: kernel-janitors, kvm, virtualization

There is only one caller for vhost_kvzalloc() and it expects NULL on
allocation failure.  Most people would probably expect that so let's
change ERR_PTR(-ENOMEM) to NULL.

Fixes: 4de7255f7d2b ('vhost: extend memory regions allocation to vmalloc')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index a9fe859..99d613b 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -686,7 +686,7 @@ static void *vhost_kvzalloc(unsigned long size)
 	if (!n) {
 		n = vzalloc(size);
 		if (!n)
-			return ERR_PTR(-ENOMEM);
+			return NULL;
 	}
 	return n;
 }

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [patch] vhost: NULL vs ERR_PTR bug
  2015-07-15 11:16 [patch] vhost: NULL vs ERR_PTR bug Dan Carpenter
@ 2015-07-15 11:28 ` Michael S. Tsirkin
       [not found] ` <20150715142420-mutt-send-email-mst@redhat.com>
  1 sibling, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2015-07-15 11:28 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: Igor Mammedov, kernel-janitors, kvm, virtualization

On Wed, Jul 15, 2015 at 02:16:59PM +0300, Dan Carpenter wrote:
> There is only one caller for vhost_kvzalloc() and it expects NULL on
> allocation failure.  Most people would probably expect that so let's
> change ERR_PTR(-ENOMEM) to NULL.
> 
> Fixes: 4de7255f7d2b ('vhost: extend memory regions allocation to vmalloc')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Ouch. Thanks a lot for noticing this, and it's a good thing
you did before Linus merged it.
I'll squash this into Igor's patch and redo my pull request.

> 
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index a9fe859..99d613b 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -686,7 +686,7 @@ static void *vhost_kvzalloc(unsigned long size)
>  	if (!n) {
>  		n = vzalloc(size);
>  		if (!n)
> -			return ERR_PTR(-ENOMEM);
> +			return NULL;
>  	}
>  	return n;
>  }

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [patch] vhost: NULL vs ERR_PTR bug
       [not found] ` <20150715142420-mutt-send-email-mst@redhat.com>
@ 2015-07-15 11:35   ` walter harms
       [not found]   ` <55A64574.1090907@bfs.de>
  1 sibling, 0 replies; 6+ messages in thread
From: walter harms @ 2015-07-15 11:35 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Igor Mammedov, kernel-janitors, kvm, Dan Carpenter,
	virtualization



Am 15.07.2015 13:28, schrieb Michael S. Tsirkin:
> On Wed, Jul 15, 2015 at 02:16:59PM +0300, Dan Carpenter wrote:
>> There is only one caller for vhost_kvzalloc() and it expects NULL on
>> allocation failure.  Most people would probably expect that so let's
>> change ERR_PTR(-ENOMEM) to NULL.
>>
>> Fixes: 4de7255f7d2b ('vhost: extend memory regions allocation to vmalloc')
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Ouch. Thanks a lot for noticing this, and it's a good thing
> you did before Linus merged it.
> I'll squash this into Igor's patch and redo my pull request.
> 

Is this function needed at all ?

e.g. I followed vzalloc() to __vmalloc_node_range()

a check for size==0 seems to be done there.

re,
 wh


>>
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index a9fe859..99d613b 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -686,7 +686,7 @@ static void *vhost_kvzalloc(unsigned long size)
>>  	if (!n) {
>>  		n = vzalloc(size);
>>  		if (!n)
>> -			return ERR_PTR(-ENOMEM);
>> +			return NULL;
>>  	}
>>  	return n;
>>  }
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [patch] vhost: NULL vs ERR_PTR bug
       [not found]   ` <55A64574.1090907@bfs.de>
@ 2015-07-15 11:41     ` Dan Carpenter
  2015-07-15 14:21     ` Julia Lawall
       [not found]     ` <alpine.DEB.2.10.1507151020460.2919@hadrien>
  2 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2015-07-15 11:41 UTC (permalink / raw)
  To: walter harms
  Cc: Igor Mammedov, virtualization, kernel-janitors, kvm,
	Michael S. Tsirkin

On Wed, Jul 15, 2015 at 01:35:16PM +0200, walter harms wrote:
> Is this function needed at all ?
> 

It tries to kmalloc() memory and if it can't then it tries to vmalloc()
it.  There are a bunch of these functions.  Eventually someone should
put one in a common header.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [patch] vhost: NULL vs ERR_PTR bug
       [not found]   ` <55A64574.1090907@bfs.de>
  2015-07-15 11:41     ` Dan Carpenter
@ 2015-07-15 14:21     ` Julia Lawall
       [not found]     ` <alpine.DEB.2.10.1507151020460.2919@hadrien>
  2 siblings, 0 replies; 6+ messages in thread
From: Julia Lawall @ 2015-07-15 14:21 UTC (permalink / raw)
  To: walter harms
  Cc: kvm, Michael S. Tsirkin, kernel-janitors, virtualization,
	Igor Mammedov, Dan Carpenter

On Wed, 15 Jul 2015, walter harms wrote:

>
>
> Am 15.07.2015 13:28, schrieb Michael S. Tsirkin:
> > On Wed, Jul 15, 2015 at 02:16:59PM +0300, Dan Carpenter wrote:
> >> There is only one caller for vhost_kvzalloc() and it expects NULL on
> >> allocation failure.  Most people would probably expect that so let's
> >> change ERR_PTR(-ENOMEM) to NULL.
> >>
> >> Fixes: 4de7255f7d2b ('vhost: extend memory regions allocation to vmalloc')
> >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > Ouch. Thanks a lot for noticing this, and it's a good thing
> > you did before Linus merged it.
> > I'll squash this into Igor's patch and redo my pull request.
> >
>
> Is this function needed at all ?
>
> e.g. I followed vzalloc() to __vmalloc_node_range()
>
> a check for size==0 seems to be done there.

At least it looks like one could return the result of vzalloc directly?

julia

>
> re,
>  wh
>
>
> >>
> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >> index a9fe859..99d613b 100644
> >> --- a/drivers/vhost/vhost.c
> >> +++ b/drivers/vhost/vhost.c
> >> @@ -686,7 +686,7 @@ static void *vhost_kvzalloc(unsigned long size)
> >>  	if (!n) {
> >>  		n = vzalloc(size);
> >>  		if (!n)
> >> -			return ERR_PTR(-ENOMEM);
> >> +			return NULL;
> >>  	}
> >>  	return n;
> >>  }
> > --
> > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [patch] vhost: NULL vs ERR_PTR bug
       [not found]     ` <alpine.DEB.2.10.1507151020460.2919@hadrien>
@ 2015-07-15 14:40       ` Dan Carpenter
  0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2015-07-15 14:40 UTC (permalink / raw)
  To: Julia Lawall
  Cc: kvm, Michael S. Tsirkin, kernel-janitors, virtualization,
	Igor Mammedov, walter harms

On Wed, Jul 15, 2015 at 10:21:12AM -0400, Julia Lawall wrote:
> At least it looks like one could return the result of vzalloc directly?
> 
> julia

That's a true...  I was tempted to re-write it a bit.  I think one of
the Lustre people are eventually going to export this function and we
will delete this one.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2015-07-15 14:40 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-15 11:16 [patch] vhost: NULL vs ERR_PTR bug Dan Carpenter
2015-07-15 11:28 ` Michael S. Tsirkin
     [not found] ` <20150715142420-mutt-send-email-mst@redhat.com>
2015-07-15 11:35   ` walter harms
     [not found]   ` <55A64574.1090907@bfs.de>
2015-07-15 11:41     ` Dan Carpenter
2015-07-15 14:21     ` Julia Lawall
     [not found]     ` <alpine.DEB.2.10.1507151020460.2919@hadrien>
2015-07-15 14:40       ` Dan Carpenter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).