Re: [systemd-devel] [PATCH 2/2] sysv-generator: remove NULL pointer dereference

2015-04-27 Thread Lennart Poettering 
On Sun, 26.04.15 21:04, Thomas H.P. Andersen (pho...@gmail.com) wrote:

 On Sun, Apr 26, 2015 at 8:31 PM, Thomas H.P. Andersen pho...@gmail.com 
 wrote:
  On Sun, Apr 26, 2015 at 8:23 PM, Shawn Landden sh...@churchofgit.com 
  wrote:
  Actually you missed that free_sysvstub_hashmap does not tolerate NULL 
  pointers.
  Indeed. I will commit that.
 
 Wait. free_sysvstub_hashmapp does tolerate NULL pointers.
 
 hashmap_steal_first will return NULL if the hashmap is NULL. And
 hashmap_free is fine with NULL too. Your patch makes it more obvious
 that free_sysvstub_hashmapp does tolerate NULL but destructors should
 tolerate NULL as per the coding style. So I guess it should just be
 assumed? I will leave it up to the others to decide what the best
 style is here.

Thomas, your are right.

The intention with hashmaps is that a NULL hashmap is considered
equivalent to an empty hashmap thus saving us tons of explicit
allocations while keeping the code reasable.

So yes, hashmap_steal_first() handles an empty hashmap correctly and
makes it a NOP retuning NULL, and so does hashmal_free().

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/2] sysv-generator: remove NULL pointer dereference

2015-04-26 Thread Thomas H.P. Andersen 
Hi Shawn,

I fixed this a few hours ago. I also updated the status in coverity.
Is there something else I can do to avoid duplicated work?

On Sun, Apr 26, 2015 at 7:58 PM, Shawn Landden sh...@churchofgit.com wrote:
 (coverity)
 ---
  src/sysv-generator/sysv-generator.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/src/sysv-generator/sysv-generator.c 
 b/src/sysv-generator/sysv-generator.c
 index 5ecd750..714ce8f 100644
 --- a/src/sysv-generator/sysv-generator.c
 +++ b/src/sysv-generator/sysv-generator.c
 @@ -922,7 +922,7 @@ finish:
  int main(int argc, char *argv[]) {
  int r, q;
  _cleanup_lookup_paths_free_ LookupPaths lp = {};
 -_cleanup_(free_sysvstub_hashmapp) Hashmap *all_services;
 +_cleanup_(free_sysvstub_hashmapp) Hashmap *all_services = NULL;
  SysvStub *service;
  Iterator j;

 --
 2.2.1.209.g41e5f3a

 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/2] sysv-generator: remove NULL pointer dereference

2015-04-26 Thread Shawn Landden 
Actually you missed that free_sysvstub_hashmap does not tolerate NULL pointers.

On Sun, Apr 26, 2015 at 11:21 AM, Shawn Landden sh...@churchofgit.com wrote:
 On Sun, Apr 26, 2015 at 11:15 AM, Thomas H.P. Andersen pho...@gmail.com 
 wrote:
 Hi Shawn,

 I fixed this a few hours ago. I also updated the status in coverity.
 Is there something else I can do to avoid duplicated work?
 I wasn't checking coverity, just reading the emails, so the duplicated
 work in on my end.

 On Sun, Apr 26, 2015 at 7:58 PM, Shawn Landden sh...@churchofgit.com wrote:
 (coverity)
 ---
  src/sysv-generator/sysv-generator.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/src/sysv-generator/sysv-generator.c 
 b/src/sysv-generator/sysv-generator.c
 index 5ecd750..714ce8f 100644
 --- a/src/sysv-generator/sysv-generator.c
 +++ b/src/sysv-generator/sysv-generator.c
 @@ -922,7 +922,7 @@ finish:
  int main(int argc, char *argv[]) {
  int r, q;
  _cleanup_lookup_paths_free_ LookupPaths lp = {};
 -_cleanup_(free_sysvstub_hashmapp) Hashmap *all_services;
 +_cleanup_(free_sysvstub_hashmapp) Hashmap *all_services = NULL;
  SysvStub *service;
  Iterator j;

 --
 2.2.1.209.g41e5f3a

 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel
 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel



 --
 Shawn Landden



-- 
Shawn Landden
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/2] sysv-generator: remove NULL pointer dereference

2015-04-26 Thread Thomas H.P. Andersen 
On Sun, Apr 26, 2015 at 8:23 PM, Shawn Landden sh...@churchofgit.com wrote:
 Actually you missed that free_sysvstub_hashmap does not tolerate NULL 
 pointers.
Indeed. I will commit that.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/2] sysv-generator: remove NULL pointer dereference

2015-04-26 Thread Shawn Landden 
 static void free_sysvstub_hashmapp(Hashmap **h) {
 while ((stub = hashmap_steal_first(*h)))

_cleanup_ sends a pointer to the pointer. and then this dereferences
that, which is kinda confusing, but yeah the code is correct, it would
be clearer with DEFINE_TRIVIAL_CLEANUP_FUNC()

On Sun, Apr 26, 2015 at 12:04 PM, Thomas H.P. Andersen pho...@gmail.com wrote:
 On Sun, Apr 26, 2015 at 8:31 PM, Thomas H.P. Andersen pho...@gmail.com 
 wrote:
 On Sun, Apr 26, 2015 at 8:23 PM, Shawn Landden sh...@churchofgit.com wrote:
 Actually you missed that free_sysvstub_hashmap does not tolerate NULL 
 pointers.
 Indeed. I will commit that.

 Wait. free_sysvstub_hashmapp does tolerate NULL pointers.

 hashmap_steal_first will return NULL if the hashmap is NULL. And
 hashmap_free is fine with NULL too. Your patch makes it more obvious
 that free_sysvstub_hashmapp does tolerate NULL but destructors should
 tolerate NULL as per the coding style. So I guess it should just be
 assumed? I will leave it up to the others to decide what the best
 style is here.
 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel



-- 
Shawn Landden
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH 2/2] sysv-generator: remove NULL pointer dereference

2015-04-26 Thread Shawn Landden 
(coverity)
---
 src/sysv-generator/sysv-generator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/sysv-generator/sysv-generator.c 
b/src/sysv-generator/sysv-generator.c
index 5ecd750..714ce8f 100644
--- a/src/sysv-generator/sysv-generator.c
+++ b/src/sysv-generator/sysv-generator.c
@@ -922,7 +922,7 @@ finish:
 int main(int argc, char *argv[]) {
 int r, q;
 _cleanup_lookup_paths_free_ LookupPaths lp = {};
-_cleanup_(free_sysvstub_hashmapp) Hashmap *all_services;
+_cleanup_(free_sysvstub_hashmapp) Hashmap *all_services = NULL;
 SysvStub *service;
 Iterator j;
 
-- 
2.2.1.209.g41e5f3a

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/2] sysv-generator: remove NULL pointer dereference

2015-04-26 Thread Thomas H.P. Andersen 
On Sun, Apr 26, 2015 at 8:31 PM, Thomas H.P. Andersen pho...@gmail.com wrote:
 On Sun, Apr 26, 2015 at 8:23 PM, Shawn Landden sh...@churchofgit.com wrote:
 Actually you missed that free_sysvstub_hashmap does not tolerate NULL 
 pointers.
 Indeed. I will commit that.

Wait. free_sysvstub_hashmapp does tolerate NULL pointers.

hashmap_steal_first will return NULL if the hashmap is NULL. And
hashmap_free is fine with NULL too. Your patch makes it more obvious
that free_sysvstub_hashmapp does tolerate NULL but destructors should
tolerate NULL as per the coding style. So I guess it should just be
assumed? I will leave it up to the others to decide what the best
style is here.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/2] sysv-generator: remove NULL pointer dereference

2015-04-26 Thread Shawn Landden 
On Sun, Apr 26, 2015 at 11:15 AM, Thomas H.P. Andersen pho...@gmail.com wrote:
 Hi Shawn,

 I fixed this a few hours ago. I also updated the status in coverity.
 Is there something else I can do to avoid duplicated work?
I wasn't checking coverity, just reading the emails, so the duplicated
work in on my end.

 On Sun, Apr 26, 2015 at 7:58 PM, Shawn Landden sh...@churchofgit.com wrote:
 (coverity)
 ---
  src/sysv-generator/sysv-generator.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/src/sysv-generator/sysv-generator.c 
 b/src/sysv-generator/sysv-generator.c
 index 5ecd750..714ce8f 100644
 --- a/src/sysv-generator/sysv-generator.c
 +++ b/src/sysv-generator/sysv-generator.c
 @@ -922,7 +922,7 @@ finish:
  int main(int argc, char *argv[]) {
  int r, q;
  _cleanup_lookup_paths_free_ LookupPaths lp = {};
 -_cleanup_(free_sysvstub_hashmapp) Hashmap *all_services;
 +_cleanup_(free_sysvstub_hashmapp) Hashmap *all_services = NULL;
  SysvStub *service;
  Iterator j;

 --
 2.2.1.209.g41e5f3a

 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel
 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel



-- 
Shawn Landden
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel