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

[Endi Sukma Dewata]

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.

3. Is there a reliable way to test this?

4. The comment for adding SELinux contexts incorrectly says:

   # A maximum of 10 tries to delete the SELinux contexts

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?

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

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.

8. Some trailing whitespaces.

-- 
Endi S. Dewata