[PATCH] ELAPI: Fix dispatcher structure initialization

[Jakub Hrozek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

The memset size_t parameter was wrong and the async_ctx param was not
manually zeroed resulting in random segfaults when the structure was freed.

	Jakub
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqw/38ACgkQHsardTLnvCUEdACgs8kFkJH+sSCHuYY5eDCGWciF
> iIQAoJTRz078L7cWzqucckE85g15gIXu
> =BR9P
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/16/2009 11:08 AM, Jakub Hrozek wrote:
>  The memset size_t parameter was wrong and the async_ctx param was
> not
>  manually zeroed resulting in random segfaults when the structure was freed.
>  
>  	Jakub 

If you're memsetting handle, then you don't need to also explicitly set
handle->async_ctx (or any of the other pointers) to NULL.

If Valgrind is later complaining that testing for NULL is an unitialized
jump or move, that's a bug in Valgrind.

- ------------------------------------------------------------------------

_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel


- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqxAUcACgkQeiVVYja6o6PMjgCfX3C1vllI5mvLJvyN6Pt8Xui+
> 9ckAoKMcUEzY5B0+lLeMTo/3y/EXcgBb
> =LT+z
> -----END PGP SIGNATURE-----

[Jakub Hrozek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/16/2009 05:16 PM, Stephen Gallagher wrote:
>  If you're memsetting handle, then you don't need to also
> explicitly set
>  handle->async_ctx (or any of the other pointers) to NULL.
>  
>  If Valgrind is later complaining that testing for NULL is an unitialized
>  jump or move, that's a bug in Valgrind.
>   
I know and my personal preference would be to just use calloc or memset
and not enumerate the structure members. However, I'm fixing existing
code which I'm not author of, so I felt I'd better adhere to the
standards in the existing code.

	Jakub
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqxAqYACgkQHsardTLnvCXfmACfTUhCeTauneX+BcQH79+nWJjC
> /UkAoJnQyGKi7z4YPwGGPf7bP8xUyx2w
> =CiWa
> -----END PGP SIGNATURE-----

[Martin Nagy]

Stephen Gallagher wrote:
>  -----BEGIN PGP SIGNED MESSAGE-----
>  Hash: SHA1
>  
>  On 09/16/2009 11:08 AM, Jakub Hrozek wrote:
>  > The memset size_t parameter was wrong and the async_ctx param was not
>  > manually zeroed resulting in random segfaults when the structure was freed.
>  > 
>  > 	Jakub
>  
>  
>  If you're memsetting handle, then you don't need to also explicitly set
>  handle->async_ctx (or any of the other pointers) to NULL.
>  
>  If Valgrind is later complaining that testing for NULL is an unitialized
>  jump or move, that's a bug in Valgrind. 
Obviously, valgrind was producing warnings precisely because of the bug
that Jakub's patch fixes. Therefore it was right to complain. I think
the right thing to do right now is to remove all the explicit
initializations and the comment about valgrind complaints.

Martin

[Jakub Hrozek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/16/2009 05:35 PM, Martin Nagy wrote:
>  Obviously, valgrind was producing warnings precisely because of the
> bug
>  that Jakub's patch fixes. Therefore it was right to complain. I think
>  the right thing to do right now is to remove all the explicit
>  initializations and the comment about valgrind complaints. 
That is absolutely correct, I even verified this with valgrind. New
patch attached.

	Jakub
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqxCbwACgkQHsardTLnvCXYrgCg4zzTo839fJZ9XoGuBxjV1J40
> i4gAoOLVYFaWlYWf//vT9KR2kxK/CSzC
> =GqOy
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/16/2009 11:52 AM, Jakub Hrozek wrote:
>  On 09/16/2009 05:35 PM, Martin Nagy wrote:
> > Obviously, valgrind was producing warnings precisely because of the bug
> > that Jakub's patch fixes. Therefore it was right to complain. I think
> > the right thing to do right now is to remove all the explicit
> > initializations and the comment about valgrind complaints.
>  
>  That is absolutely correct, I even verified this with valgrind. New
>  patch attached.
>  
>  	Jakub 
Ack

- ------------------------------------------------------------------------

_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel


- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqxChIACgkQeiVVYja6o6NxMACeMZT/Rn5YY8f2BXazYzhz4w/j
> 0XgAn2y95IT+WBNLQOrMw5ABlI24t9eQ
> =Qixi
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/16/2009 11:53 AM, Stephen Gallagher wrote:
>  On 09/16/2009 11:52 AM, Jakub Hrozek wrote:
> > On 09/16/2009 05:35 PM, Martin Nagy wrote:
> >> Obviously, valgrind was producing warnings precisely because of the bug
> >> that Jakub's patch fixes. Therefore it was right to complain. I think
> >> the right thing to do right now is to remove all the explicit
> >> initializations and the comment about valgrind complaints.
>  
> > That is absolutely correct, I even verified this with valgrind. New
> > patch attached.
>  
> > 	Jakub
>  
>  Ack
>  
>  ------------------------------------------------------------------------
>  
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/sssd-devel
>  
>   
Pushed to master.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel

- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqxIJ0ACgkQeiVVYja6o6OxXwCgkqc+iYSf87M+nVpTp8UAu7cH
> eSwAnjYX48bPPjiUgsM/gDYbzVUOEOpF
> =sFUw
> -----END PGP SIGNATURE-----

[Dmitri Pal]

Guys,

This is great, thank you for finding it.
But this will really mess up my merges.
Can it be undone?
Also I have a ticket open to explicitly return to this part of code and
sort things out with valgrind.
Thanks for finding the answer and cause of the problem, it will make the
fixes simple
but would it possible next time to ask me first or at least wait for my
comment
or ack before pushing?
I have not fell off the planet at least yet...

Thanks
Dmitri

Stephen Gallagher wrote:
>  On 09/16/2009 11:53 AM, Stephen Gallagher wrote:
>  > On 09/16/2009 11:52 AM, Jakub Hrozek wrote:
>  >> On 09/16/2009 05:35 PM, Martin Nagy wrote:
>  >>> Obviously, valgrind was producing warnings precisely because of
>  the bug
>  >>> that Jakub's patch fixes. Therefore it was right to complain. I think
>  >>> the right thing to do right now is to remove all the explicit
>  >>> initializations and the comment about valgrind complaints.
>  >> That is absolutely correct, I even verified this with valgrind. New
>  >> patch attached.
>  >>     Jakub
>  > Ack
>
>  > ------------------------------------------------------------------------
>
>  > _______________________________________________
>  > sssd-devel mailing list
>  > sssd-devel(a)lists.fedorahosted.org
>  > https://fedorahosted.org/mailman/listinfo/sssd-devel
>
>
>
>  Pushed to master.
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/sssd-devel
>  
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel

-- 
Thank you,
Dmitri Pal

Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Dmitri, it's a very simple change that will be very easy to resolve with
'git rebase' and 'git mergetool'.

There's no reason that a complete fix should be held back for the
convenience of any one developer.

Also, no. The upstream SSSD repository is one-way. To remove it, we
would have to apply a negative diff and commit that. We're not going to
remove a solid fix because it might inconvenience you.

On 09/16/2009 02:28 PM, Dmitri Pal wrote:
>  Guys,
>  
>  This is great, thank you for finding it.
>  But this will really mess up my merges.
>  Can it be undone?
>  Also I have a ticket open to explicitly return to this part of code and
>  sort things out with valgrind.
>  Thanks for finding the answer and cause of the problem, it will make the
>  fixes simple
>  but would it possible next time to ask me first or at least wait for my
>  comment
>  or ack before pushing?
>  I have not fell off the planet at least yet...
>  
>  Thanks
>  Dmitri
>  
>  Stephen Gallagher wrote:
> > On 09/16/2009 11:53 AM, Stephen Gallagher wrote:
> >> On 09/16/2009 11:52 AM, Jakub Hrozek wrote:
> >>> On 09/16/2009 05:35 PM, Martin Nagy wrote:
> >>>> Obviously, valgrind was producing warnings precisely because of
> > the bug
> >>>> that Jakub's patch fixes. Therefore it was right to complain. I
> think
> >>>> the right thing to do right now is to remove all the explicit
> >>>> initializations and the comment about valgrind complaints.
> >>> That is absolutely correct, I even verified this with valgrind. New
> >>> patch attached.
> >>>     Jakub
> >> Ack
> >
> >> ------------------------------------------------------------------------
> >
> >> _______________________________________________
> >> sssd-devel mailing list
> >> sssd-devel(a)lists.fedorahosted.org
> >> https://fedorahosted.org/mailman/listinfo/sssd-devel
> >
> >
> >
> > Pushed to master.
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel(a)lists.fedorahosted.org
> > https://fedorahosted.org/mailman/listinfo/sssd-devel
> >
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/sssd-devel
>   

- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqxL1wACgkQeiVVYja6o6Oh/wCfTu/odxK6iJ0DAX2uoz+jTzA2
> 73wAoK0yAisMuGWOqiZhkjWTAsfsKFOD
> =s0Ct
> -----END PGP SIGNATURE-----

[Simo Sorce]

On Wed, 2009-09-16 at 14:33 -0400, Stephen Gallagher wrote:
>  Dmitri, it's a very simple change that will be very easy to
> resolve
>  with
>  'git rebase' and 'git mergetool'.
>  
>  There's no reason that a complete fix should be held back for the
>  convenience of any one developer.
>  
>  Also, no. The upstream SSSD repository is one-way. To remove it, we
>  would have to apply a negative diff and commit that. We're not going
>  to
>  remove a solid fix because it might inconvenience you. 
Steve,
it doesn't make sense to revert now, but in general I agree with Dmitri
that it is at least polite to wait and have a say from the main
maintainer of the code before pushing a patch.
Unless it is blocking other developers or is urgent for any other
reasonable motive.


Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

[Dmitri Pal]

Simo Sorce wrote:
>  On Wed, 2009-09-16 at 14:33 -0400, Stephen Gallagher wrote:
>    
> > Dmitri, it's a very simple change that will be very easy to resolve
> > with
> > 'git rebase' and 'git mergetool'.
> >
> > There's no reason that a complete fix should be held back for the
> > convenience of any one developer.
> >
> > Also, no. The upstream SSSD repository is one-way. To remove it, we
> > would have to apply a negative diff and commit that. We're not going
> > to
> > remove a solid fix because it might inconvenience you.
> >     
>
>  Steve,
>  it doesn't make sense to revert now, but in general I agree with Dmitri
>  that it is at least polite to wait and have a say from the main
>  maintainer of the code before pushing a patch.
>  Unless it is blocking other developers or is urgent for any other
>  reasonable motive.
>
>
>  Simo.
>
>     
Thank you Simo for support.
A rare occasion when we are in agreement!
The main point is (thought it will make my merge more complex, I can
deal with it using merge tools as you said),
but this or similar change has to be done in  5-6 other places so rather
than push a separate patch that addresses
just one problem it should be addressed by fixing a corresponding
ticket. https://fedorahosted.org/sssd/ticket/161.
This all could have been discussed before the push and best approach
could have been sorted out.

I will send the patches later today if I have time...

-- 
Thank you,
Dmitri Pal

Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

[Simo Sorce]

On Wed, 2009-09-16 at 22:04 -0400, Dmitri Pal wrote:
>  The main point is (thought it will make my merge more complex, I can
>  deal with it using merge tools as you said), 
Just to make it clear, raising the card "I have to rebase/merge and that
is annoying", is not a valid reason to not accept a patch :-)

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

[Dmitri Pal]

Simo Sorce wrote:
>  On Wed, 2009-09-16 at 22:04 -0400, Dmitri Pal wrote:
>    
> > The main point is (thought it will make my merge more complex, I can
> > deal with it using merge tools as you said),
> >     
>
>  Just to make it clear, raising the card "I have to rebase/merge and that
>  is annoying", is not a valid reason to not accept a patch :-)
>
>  Simo.
>
>     
True but it seems logical in such situation for the person actively
working on the same module
to do the change based on this patch before it is pushed to master.

Well, another dead horse I suppose...

-- 
Thank you,
Dmitri Pal

Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/