Re: [libvirt] [PATCH] Fixed NULL pointer check
At 03/18/2012 08:29 AM, Martin Kletzander Wrote:
This patch fixes a NULL pointer check that was causing SegFault on
some specific configurations.
---
src/util/conf.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/util/conf.c b/src/util/conf.c
index 8ad60e0..e76362c 100644
--- a/src/util/conf.c
+++ b/src/util/conf.c
@@ -1,7 +1,7 @@
/**
* conf.c: parser for a subset of the Python encoded Xen configuration files
*
- * Copyright (C) 2006-2011 Red Hat, Inc.
+ * Copyright (C) 2006-2012 Red Hat, Inc.
*
* See COPYING.LIB for the License of this software
*
@@ -836,6 +836,9 @@ virConfGetValue(virConfPtr conf, const char *setting)
{
virConfEntryPtr cur;
+if (conf == NULL)
+return(NULL);
Please use 'return NULL;' instead of 'return(NULL);'
'return(NULL);' is old style, and we donot use it now and later.
+
cur = conf-entries;
while (cur != NULL) {
if ((cur-name != NULL)
ACK with the nit fixed.
Wen Congyang
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fixed NULL pointer check
On 03/19/2012 08:43 AM, Wen Congyang wrote:
At 03/18/2012 08:29 AM, Martin Kletzander Wrote:
This patch fixes a NULL pointer check that was causing SegFault on
some specific configurations.
---
src/util/conf.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/util/conf.c b/src/util/conf.c
index 8ad60e0..e76362c 100644
--- a/src/util/conf.c
+++ b/src/util/conf.c
@@ -1,7 +1,7 @@
/**
* conf.c: parser for a subset of the Python encoded Xen configuration files
*
- * Copyright (C) 2006-2011 Red Hat, Inc.
+ * Copyright (C) 2006-2012 Red Hat, Inc.
*
* See COPYING.LIB for the License of this software
*
@@ -836,6 +836,9 @@ virConfGetValue(virConfPtr conf, const char *setting)
{
virConfEntryPtr cur;
+if (conf == NULL)
+return(NULL);
Please use 'return NULL;' instead of 'return(NULL);'
'return(NULL);' is old style, and we donot use it now and later.
It seems weird to me too, it's just that it's almost everywhere in this
file so that's why I wanted to keep the same look.
+
cur = conf-entries;
while (cur != NULL) {
if ((cur-name != NULL)
ACK with the nit fixed.
I'll send a second version, I don't have push permissions.
Wen Congyang
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fixed NULL pointer check
On Mon, Mar 19, 2012 at 08:51:24AM +0100, Martin Kletzander wrote:
On 03/19/2012 08:43 AM, Wen Congyang wrote:
At 03/18/2012 08:29 AM, Martin Kletzander Wrote:
This patch fixes a NULL pointer check that was causing SegFault on
some specific configurations.
---
src/util/conf.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/util/conf.c b/src/util/conf.c
index 8ad60e0..e76362c 100644
--- a/src/util/conf.c
+++ b/src/util/conf.c
@@ -1,7 +1,7 @@
/**
* conf.c: parser for a subset of the Python encoded Xen configuration
files
*
- * Copyright (C) 2006-2011 Red Hat, Inc.
+ * Copyright (C) 2006-2012 Red Hat, Inc.
*
* See COPYING.LIB for the License of this software
*
@@ -836,6 +836,9 @@ virConfGetValue(virConfPtr conf, const char *setting)
{
virConfEntryPtr cur;
+if (conf == NULL)
+return(NULL);
Please use 'return NULL;' instead of 'return(NULL);'
'return(NULL);' is old style, and we donot use it now and later.
It seems weird to me too, it's just that it's almost everywhere in this
file so that's why I wanted to keep the same look.
That is a historical remant. Do feel free to send another followup patch to
cleanup all the cases of 'return(NULL)' as well.
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fixed NULL pointer check
On 03/19/2012 11:32 AM, Daniel P. Berrange wrote:
On Mon, Mar 19, 2012 at 08:51:24AM +0100, Martin Kletzander wrote:
On 03/19/2012 08:43 AM, Wen Congyang wrote:
At 03/18/2012 08:29 AM, Martin Kletzander Wrote:
This patch fixes a NULL pointer check that was causing SegFault on
some specific configurations.
---
src/util/conf.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/util/conf.c b/src/util/conf.c
index 8ad60e0..e76362c 100644
--- a/src/util/conf.c
+++ b/src/util/conf.c
@@ -1,7 +1,7 @@
/**
* conf.c: parser for a subset of the Python encoded Xen configuration
files
*
- * Copyright (C) 2006-2011 Red Hat, Inc.
+ * Copyright (C) 2006-2012 Red Hat, Inc.
*
* See COPYING.LIB for the License of this software
*
@@ -836,6 +836,9 @@ virConfGetValue(virConfPtr conf, const char *setting)
{
virConfEntryPtr cur;
+if (conf == NULL)
+return(NULL);
Please use 'return NULL;' instead of 'return(NULL);'
'return(NULL);' is old style, and we donot use it now and later.
It seems weird to me too, it's just that it's almost everywhere in this
file so that's why I wanted to keep the same look.
That is a historical remant. Do feel free to send another followup patch to
cleanup all the cases of 'return(NULL)' as well.
Daniel
Did you mean in that file or globally? Because I just tried the first
thing that came to my mind and look at the output:
libvirt $ find . -name '*.[ch]' -type f -exec grep -nH -e \
'return(.*);' {} + | wc -l
852
Not that I wouldn't do it, it just seem as a pretty big change. On the
other hand, I don't see the point in changing that in only one file.
Martin
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fixed NULL pointer check
On Mon, Mar 19, 2012 at 01:38:47PM +0100, Martin Kletzander wrote:
On 03/19/2012 11:32 AM, Daniel P. Berrange wrote:
On Mon, Mar 19, 2012 at 08:51:24AM +0100, Martin Kletzander wrote:
On 03/19/2012 08:43 AM, Wen Congyang wrote:
At 03/18/2012 08:29 AM, Martin Kletzander Wrote:
This patch fixes a NULL pointer check that was causing SegFault on
some specific configurations.
---
src/util/conf.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/util/conf.c b/src/util/conf.c
index 8ad60e0..e76362c 100644
--- a/src/util/conf.c
+++ b/src/util/conf.c
@@ -1,7 +1,7 @@
/**
* conf.c: parser for a subset of the Python encoded Xen configuration
files
*
- * Copyright (C) 2006-2011 Red Hat, Inc.
+ * Copyright (C) 2006-2012 Red Hat, Inc.
*
* See COPYING.LIB for the License of this software
*
@@ -836,6 +836,9 @@ virConfGetValue(virConfPtr conf, const char *setting)
{
virConfEntryPtr cur;
+if (conf == NULL)
+return(NULL);
Please use 'return NULL;' instead of 'return(NULL);'
'return(NULL);' is old style, and we donot use it now and later.
It seems weird to me too, it's just that it's almost everywhere in this
file so that's why I wanted to keep the same look.
That is a historical remant. Do feel free to send another followup patch to
cleanup all the cases of 'return(NULL)' as well.
Daniel
Did you mean in that file or globally? Because I just tried the first
thing that came to my mind and look at the output:
libvirt $ find . -name '*.[ch]' -type f -exec grep -nH -e \
'return(.*);' {} + | wc -l
852
Not that I wouldn't do it, it just seem as a pretty big change. On the
other hand, I don't see the point in changing that in only one file.
IMHO, we should do this to make our code consistent, and ideally add a
syntax-check rule to prevent them coming back. Anyone disagree ?
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fixed NULL pointer check
On 03/19/2012 06:43 AM, Daniel P. Berrange wrote:
That is a historical remant. Do feel free to send another followup patch to
cleanup all the cases of 'return(NULL)' as well.
Daniel
Did you mean in that file or globally? Because I just tried the first
thing that came to my mind and look at the output:
libvirt $ find . -name '*.[ch]' -type f -exec grep -nH -e \
'return(.*);' {} + | wc -l
852
Yes, it is a rather big cleanup, which is all the more reason why we'd
want to ensure that 'make syntax-check' prevents it from recurring.
Not that I wouldn't do it, it just seem as a pretty big change. On the
other hand, I don't see the point in changing that in only one file.
IMHO, we should do this to make our code consistent, and ideally add a
syntax-check rule to prevent them coming back. Anyone disagree ?
I'm in favor of such a cleanup. There are still a few places where
return needs parenthesis; for example, I'm fond of the style:
return (big_long_conditional ?
option_1 :
option_2);
since the open '(' lets the rest of the code indent nicely when using
default emacs indentation. But it's still pretty easy to recognize the
difference between complex returns and the real offenders. I think the
number of false positives and false negatives is pretty near zero if you
boil it down to detecting uses where there are no spaces between the '('
and ')'. Thus, for cfg.mk, I suggest a pattern something like:
sc_prohibit_return_as_function:
@prohibit='\return *([^ ]*)' \
halt='avoid extra () with return statements' \
$(_sc_search_regexp)
--
Eric Blake ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fixed NULL pointer check
On 03/19/2012 04:43 PM, Eric Blake wrote:
On 03/19/2012 06:43 AM, Daniel P. Berrange wrote:
[...]
since the open '(' lets the rest of the code indent nicely when using
default emacs indentation. But it's still pretty easy to recognize the
difference between complex returns and the real offenders. I think the
number of false positives and false negatives is pretty near zero if you
boil it down to detecting uses where there are no spaces between the '('
and ')'. Thus, for cfg.mk, I suggest a pattern something like:
sc_prohibit_return_as_function:
@prohibit='\return *([^ ]*)' \
halt='avoid extra () with return statements' \
$(_sc_search_regexp)
I agree with the first part. There are some places that should be kept
as is. However, the '\return *([^ ]*)' would generate some false
positives, for example 'return (buf[0] 0) | (buf[1] 8)' and few
others I've found.
Don't worry though, I've created a regexp that matches just what's
needed, composing the patch right now.
Martin
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fixed NULL pointer check
On Mon, Mar 19, 2012 at 09:43:26AM -0600, Eric Blake wrote:
On 03/19/2012 06:43 AM, Daniel P. Berrange wrote:
That is a historical remant. Do feel free to send another followup patch
to
cleanup all the cases of 'return(NULL)' as well.
Daniel
Did you mean in that file or globally? Because I just tried the first
thing that came to my mind and look at the output:
libvirt $ find . -name '*.[ch]' -type f -exec grep -nH -e \
'return(.*);' {} + | wc -l
852
Yes, it is a rather big cleanup, which is all the more reason why we'd
want to ensure that 'make syntax-check' prevents it from recurring.
Not that I wouldn't do it, it just seem as a pretty big change. On the
other hand, I don't see the point in changing that in only one file.
IMHO, we should do this to make our code consistent, and ideally add a
syntax-check rule to prevent them coming back. Anyone disagree ?
I'm in favor of such a cleanup. There are still a few places where
return needs parenthesis; for example, I'm fond of the style:
return (big_long_conditional ?
option_1 :
option_2);
Yes, that usage scenario is fine.
since the open '(' lets the rest of the code indent nicely when using
default emacs indentation. But it's still pretty easy to recognize the
difference between complex returns and the real offenders. I think the
number of false positives and false negatives is pretty near zero if you
boil it down to detecting uses where there are no spaces between the '('
and ')'. Thus, for cfg.mk, I suggest a pattern something like:
sc_prohibit_return_as_function:
@prohibit='\return *([^ ]*)' \
halt='avoid extra () with return statements' \
$(_sc_search_regexp)
Looks good.
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
