[Pki-devel] [PATCH] 49 Retry setting selinux contexts incase of concurrent pkispawn/pkidestroy execution on a machine - Ticket 470

[Abhishek Koneru]

On discussing with Endi on IRC, done these changes before pushing to
master.

- Changed the while counter < max_tries to while True.
- Error checked as error_message.strip == "Could not start semanage
transaction" rather than doing an 'in' operation.

- Changed the condition counter == max_tries in except block to counter
>= max_tries.

--Abhishek

On Tue, 2013-04-09 at 11:51 -0400, Abhishek Koneru wrote:
> Please review the patch with fixes for comments given by Endi.
> 
> --Abhishek
> On Mon, 2013-04-08 at 13:26 -0500, Endi Sukma Dewata wrote:
> > On 4/8/2013 9:22 AM, Abhishek Koneru wrote:
> > > Please review the patch which adds a retry mechanism if a semanage
> > > transaction lock could not be acquired by a pkispawn/pkidestroy
> > > execution. Normally, if a process does not get SELinux transaction lock
> > > it throws an error and quits.
> > >
> > > This patch allows pkispawn/pkidestroy to retry 10 times with a 5 second
> > > interval between each try.
> > 
> > Some comments:
> > 
> > 1. Is there any document describing that the SELinux transaction would 
> > throw an exception instead of blocking? Or did you already confirm this 
> > with someone?
> > 
> > 2. Do you have the link of the ticket that handles this issue in DS? 
> > Please put it in ticket #470 as a reference.
> 
> -- Added the reference in the comments section of #470
> > 
> > 3. Is there a reliable way to test this?
> 
> Tested the scenario using the following script.
> #! /usr/bin/python
> 
> import selinux
> if selinux.is_selinux_enabled():
>         print 'SELinux is enabled'
>         import seobject
> 
> try:
>         trans = seobject.semanageRecords("targeted")
>         trans.start()
>         portRecords = seobject.portRecords()
>         portRecords.add('8492', "tcp", "s0", 'http_port_t')
>         trans.finish()
> except ValueError as e:
>         s = str(e)
>         if s.find('Could not start the semanage transaction') != 1:
>                 print (s)
> 
> Executed the same script simultaneously in two terminals. Only one
> script completed the transaction, and the other failed throwing a
> ValueError.
> libsemanage.semanage_get_lock: Could not get direct transaction lock
> at /etc/selinux/targeted/modules/semanage.trans.LOCK. (Resource
> temporarily unavailable).
> ValueError: Could not start semanage transaction 
> > 
> > 
> > 4. The comment for adding SELinux contexts incorrectly says:
> > 
> >    # A maximum of 10 tries to delete the SELinux contexts
> > 
> -- Rectified
> > 5. The code checks the type of error based on error message:
> > 
> >    if error_message.find("Could not start semanage transaction")
> > 
> > The problem is that the error message might change or be translated so 
> > it would not match. Can we check using the exception class or error code?
> > 
> The methods in classes in seobject throw just the ValueErrors if there
> is an exception during execution. No error codes returned. Since the
> retry has to be done only when the transaction could not begin without
> getting the lock, a check on the error message is done.
> > 6. The timeOut variable is used as a counter for number of tries. It 
> > might be better to use the following variable names for better clarity:
> > 
> >    counter = 1
> >    max_tries = 10
> > 
> 
> -- Changed the variable names
> > 7. There's a bug in the patch. Suppose it fails to start transaction 
> > when timeOut is 9, it will enter the exception handling code, then 
> > timeOut is incremented to 10. Since timeOut is not bigger than 10 it 
> > doesn't throw an exception:
> > 
> >    if timeOut > 10:
> >        raise
> > 
> > Then it goes back to the loop, but since timeOut is not less than 10 the 
> > loop now will terminate:
> > 
> >    while timeOut < 10:
> > 
> > So the code will continue without throwing an error. I think it would be 
> > better to check the timeOut in just one location instead of in two 
> > places to avoid bugs like this.
> 
> -- Fixed. Modified the check in catch clause to, counter == max_tries
> > 
> > 8. Some trailing whitespaces.
> > 
> Fixed.
> 
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel