On 07/28/2011 11:04 AM, Denys Vlasenko wrote:
On Thu, 2011-07-28 at 10:14 +0200, Jiri Moskovcak wrote:
> This patch reinstates some functions we removed from python bindings -
> as it turned out, we don't have /etc/*release when anaconda is running
> so we need to obtain "os_release" from anaconda and we should avoid
> reading /etc/*-release, because it shows an error message on tty1:
>
> Can't open /etc/system-release: No such file or directory
>
> which from users POV can be considered as a bug. So this patch tries to
> get the os_release from anaconda then from /etc and the function
> dd_create_basic_files() now skips creating os_release if it already
> exists (this check shouldn't generate any error msg if the file doesn't
> exist). It also modifies create_dump_dir_from_problem_data() moving the
> dd_create_basic_files() to the end when all fields from problem_data are
> saved in dump_dir, so the mentioned check for "os_release" can happen.
>
> Please review carefully, I can help with anaconda images if someone
> would like to test it (I did of course ;)).
- ####version = getVersion_fromRPM()
- ####if version:
- #### return version
+ version = getVersion_fromRPM()
+ if version:
+ return version
Anaconda does not need this, right? Therefore
I propose to leave getVersion_fromRPM() commented out.
(Same for getProduct_fromRPM()).
My point, let's have only minimal compat code we need:
getVersion_fromPRODUCT() and getProduct_fromPRODUCT()
- try:
+ if os.path.exists(each_path):
file = open(each_path, "r")
content = file.read()
if content.startswith("Red Hat Enterprise Linux"):
return "Red Hat Enterprise Linux"
+
if content.startswith("Fedora"):
return "Fedora"
+
i = content.find(" release")
if i> -1:
return content[0:i]
- except:
- pass
It is conceptually wrong to do
if (file_exists(filename)) { f = open(filename); ... }
- it's racy. What if open will nevertheless fail (say, EACCES)?
That's right: you'll get an exception.
The correct way (IMO) is
f = open(filename);
if (f) { ... }
In this particular case it's very unlikely (/etc/system-release
is world-readable), but as a matter of principle, it is wrong.
The rest of the patch look good to me.
-ok I'll add a few IFs and the exception block back, but I'm thinking we
can remove getVersion_fromFILE because as you said
getVersion_fromPRODUCT is all we need, if the /etc/*-release exists,
then we handle it later in dd_add_basic_files(), so this is just a code
duplication...
J.