On 11/23/2015 01:50 PM, Jan Cholasta wrote: > On 23.11.2015 13:40, Tomas Babej wrote: >> >> >> On 11/23/2015 01:31 PM, Jan Cholasta wrote: >>> On 23.11.2015 13:28, Tomas Babej wrote: >>>> >>>> >>>> On 11/23/2015 01:11 PM, Jan Cholasta wrote: >>>>> On 23.11.2015 12:53, Tomas Babej wrote: >>>>>> Hi, >>>>>> >>>>>> If the code within the private_ccache contextmanager does not >>>>>> set/removes the KRB5CCNAME, the pop method will raise KeyError, which >>>>>> will cause unnecessary termination of the code flow. >>>>>> >>>>>> Make sure the KRB5CCNAME is popped out of os.environ only if present. >>>>>> >>>>>> Tomas >>>>> >>>>> NACK, use os.environ.pop('KRB5CCNAME', None) to safely remove it. >>>> >>>> Yeah, well, both ways are equivalent, I found the if statement more >>>> explicit. We can go with the suggested version, if it's more pleasing >>>> though - patch is attached. >>>> >>>>> >>>>> Also I don't think the comment is necessary, it's quite obvious >>>>> what the >>>>> code does. >>>>> >>>> >>>> I don't think an explanatory comment can hurt, ever. Worst thing that >>>> happens is that somebody is assured that he understands the code >>>> correctly. >>> >>> Actually the worst thing is that someone changes the code without >>> changing the comment. If the comment does not add any real value, it's >>> only a maintenance burden. >>> >> >> Yep, that's a real issue in our code base (i.e. I recently removed such >> a stale comment in f05846e26787da5ef6c996abf823fcc4a7f65e0f). Making >> sure the comments describe the implementation properly is on the >> author/reviewer though. >> >> What's "added value" highly depends on your skill set, and familiarity >> with the code base. Particularly the familiarity with the code base can >> diminish over time even for the author, and those are the times where >> comments can come to the rescue. > > Let's take a look at the code: > > if original_value is not None: > os.environ['KRB5CCNAME'] = original_value > else: > os.environ.pop('KRB5CCNAME', None) > > Can you tell me what in there couldn't be obvious to a person with even > the most basic skill set? > > IMHO a docstring for private_cccache would add some real value, but this > comment alone does not. > >> >> For a newcomer to the project, even a trivial comment (from the point of >> view of the experienced developer) can be valuable. > > Following this logic, there should be a comment for every line of our > code, which is ridiculous. >
Here's the version of the patch with the comment removed. Tomas
From f87ae19e673d4fe2ae1330fb2b1e1a1dbaa55630 Mon Sep 17 00:00:00 2001 From: Tomas Babej <tba...@redhat.com> Date: Mon, 23 Nov 2015 12:47:56 +0100 Subject: [PATCH] private_ccache: Harden the removal of KRB5CCNAME env variable If the code within the private_ccache contextmanager does not set/removes the KRB5CCNAME, the pop method will raise KeyError, which will cause unnecessary termination of the code flow. Make sure the KRB5CCNAME is popped out of os.environ only if present. --- ipapython/ipautil.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py index 4acdd1a98818bf311a8fef103e7219cc62a28ec1..a5545688d61354716745c7814b6bbf1ae316c13d 100644 --- a/ipapython/ipautil.py +++ b/ipapython/ipautil.py @@ -1367,7 +1367,7 @@ def private_ccache(path=None): if original_value is not None: os.environ['KRB5CCNAME'] = original_value else: - os.environ.pop('KRB5CCNAME') + os.environ.pop('KRB5CCNAME', None) if os.path.exists(path): os.remove(path) -- 2.5.0
-- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code