Re: [PATCH 3/3] oeqa/utils/ftools: Checks before appending/reading files

[Leonardo Sandoval <leonardo.sandoval.gonzalez@linux.intel.com>]

Hi Paul,

On 10/19/2015 08:41 AM, Paul Eggleton wrote:
> Hi Leo,
>
> On Monday 19 October 2015 05:24:44 leonardo.sandoval.gonzalez@linux.intel.com
> wrote:
>> From: Leonardo Sandoval <leonardo.sandoval.gonzalez@linux.intel.com>
>>
>> Before trying to append/read a file, check if file exists.
>>
>> Signed-off-by: Leonardo Sandoval
>> <leonardo.sandoval.gonzalez@linux.intel.com> ---
>>   meta/lib/oeqa/utils/ftools.py | 24 ++++++++++++++----------
>>   1 file changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/meta/lib/oeqa/utils/ftools.py b/meta/lib/oeqa/utils/ftools.py
>> index 64ebe3d..70a55b8 100644
>> --- a/meta/lib/oeqa/utils/ftools.py
>> +++ b/meta/lib/oeqa/utils/ftools.py
>> @@ -8,20 +8,24 @@ def write_file(path, data):
>>
>>   def append_file(path, data):
>>       wdata = data.rstrip() + "\n"
>> -    with open(path, "a") as f:
>> +    if os.path.isfile(path):
>> +        with open(path, "a") as f:
>>               f.write(wdata)
>
> Hang on - opening a nonexistent file with mode 'a' is perfectly fine, it'll just
> get created - why do we need this check?

You are right, there is no need.

>
>
>>   def read_file(path):
>>       data = None
>> -    with open(path) as f:
>> -        data = f.read()
>> +    if os.path.isfile(path):
>> +        with open(path) as f:
>> +            data = f.read()
>>       return data
>>
>>   def remove_from_file(path, data):
>> -    lines = read_file(path).splitlines()
>> -    rmdata = data.strip().splitlines()
>> -    for l in rmdata:
>> -        for c in range(0, lines.count(l)):
>> -            i = lines.index(l)
>> -            del(lines[i])
>> -    write_file(path, "\n".join(lines))
>> +    rawdata = read_file(path)
>> +    if rawdata:
>> +        lines = rawdata.splitlines()
>> +        rmdata = data.strip().splitlines()
>> +        for l in rmdata:
>> +            for c in range(0, lines.count(l)):
>> +                i = lines.index(l)
>> +                del(lines[i])
>> +        write_file(path, "\n".join(lines))
>
> Checking a file exists before opening it isn't good practice. It's much better
> to try opening it and if the open fails with IOError of errno.ENOENT then
> ignore it (or rather, if e.errno  != errno.ENOENT then re-raise the
 > exception).

After applying patch into master

http://lists.openembedded.org/pipermail/openembedded-core/2015-October/111704.html

I saw this error:

Traceback (most recent call last):
   File 
"/srv/ab/yocto-autobuilder/yocto-worker/nightly-oe-selftest/build/meta/lib/oeqa/selftest/base.py", 
line 110, in remove_config
     ftools.remove_from_file(self.testinc_path, data)
   File 
"/srv/ab/yocto-autobuilder/yocto-worker/nightly-oe-selftest/build/meta/lib/oeqa/utils/ftools.py", 
line 21, in remove_from_file
     lines = read_file(path).splitlines()
   File 
"/srv/ab/yocto-autobuilder/yocto-worker/nightly-oe-selftest/build/meta/lib/oeqa/utils/ftools.py", 
line 16, in read_file
     with open(path) as f:
IOError: [Errno 2] No such file or directory: 
'/srv/ab/yocto-autobuilder/yocto-worker/nightly-oe-selftest/build/build/conf/selftest.inc'


That is why I created the latter check. I will change it to use IOError, 
safer and cleaner. Sending V2 today.

Thanks for your comments.

>
> Cheers,
> Paul
>