We are communicating with nm via ifcfg files heavily (for configuring
we do it exclusively), and we started to use nm-c-e which does the same,
we can pass parameters to nm-c-e only via ifcfg files, so we need
backend class for NetworkDevice object to be able to keep it synced
with ifcfg files reasonably.
---
simpleconfig.py | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ ++--
1 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/simpleconfig.py b/simpleconfig.py
index b3e6c90..1f90356 100644
--- a/simpleconfig.py
+++ b/simpleconfig.py
@@ -16,6 +16,8 @@

import string
import os
+import tempfile
+import shutil

# use our own ASCII only uppercase function to avoid locale issues
# not going to be fast but not important
@@ -80,9 +82,68 @@ class SimpleConfigFile:

def get (self, key):
key = uppercase_ASCII_string(key)
- if self.info.has_key (key):
- return self.info[key]
+ return self.info.get(key, "")
+
+
+class IfcfgFile(SimpleConfigFile):
+
+ def __init__(self, dir, iface):
+ SimpleConfigFile.__init__(self)
+ self.iface = iface
+ self.dir = dir
+
+ @property
+ def path(self):
+ return os.path.join(self.dir, "ifcfg-%s" % self.iface)
+
+ def clear(self):
+ self.info = {}
+
+ def read(self):
+ """Reads values from ifcfg file.
+
+ returns: number of values read
+ """
+ f = open(self.path, "r")
+ lines = f.readlines()
+ f.close()
+
+ for line in lines:
+ line = line.strip()
+ if line.startswith("#") or line == ':
+ continue
+ fields = line.strip().split('=', 2)
+ if len(fields) < 2:
+ continue
+ key = uppercase_ASCII_string(fields[0])
+ value = fields[1]
+ # XXX hack
+ value = value.replace('"', ')
+ value = value.replace("'", ')
+ self.info[key] = value
+
+ return len(self.info)
+
+ # This method has to write file in a particular
+ # way so that ifcfg-rh's inotify mechanism triggeres
+ # TODORV: check that it is still true.
+ def write(self, dir=None):
+ """Writes values into ifcfg file."""
+
+ if not dir:
+ path = self.path
else:
- return ""
+ path = os.path.join(dir, os.path.basename(self.path))
+
+ fd, newifcfg = tempfile.mkstemp(prefix="ifcfg-%s" % self.iface, text=False)
+ os.write(fd, self.__str__())
+ os.close(fd)

+ os.chmod(newifcfg, 0644)
+ try:
+ os.remove(path)
+ except OSError, e:
+ if e.errno != 2:
+ raise
+ shutil.move(newifcfg, path)

--
1.6.0.6

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/27/2010 01:49 AM, Radek Vykydal wrote:

> + def read(self):
> + """Reads values from ifcfg file.
> +
> + returns: number of values read
> + """

if not os.access(self.path, os.R_OK):
return None

> + f = open(self.path, "r")
> + lines = f.readlines()
> + f.close()
> +
> + for line in lines:
> + line = line.strip()
> + if line.startswith("#") or line == ':
> + continue
> + fields = line.strip().split('=', 2)

line is already stripped, so make this:

fields = line.split('=', 2)

> + # XXX hack
> + value = value.replace('"', ')
> + value = value.replace("'", ')

value = value.translate(None, "'"")

> + fd, newifcfg = tempfile.mkstemp(prefix="ifcfg-%s" % self.iface, text=False)

I think it is possible for this to fail. You may want to wrap it in a
try: except: check


- --
Brian C. Lane <bcl@redhat.com>
Red Hat / Port Orchard, WA
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iQEVAwUBS9cnZRF+jBaO/jp/AQIhhQf+N5E3zq8x4Vehhi12W0Q5eeTSH6/q4/Sd
RL+zEVd3Eq005wMnHixx+yqh11e0fOBv/iaAw/B7eP81LkBmMV51jq+hG5zZAMvn
umUHeY12x/F72iJHr5Fnc9wiR3LfTI+KJ6dXwGi5men88KYBqVwiJtiJOOzt 94dl
bMKOk+YaUKrXhd1IDogEvN1XOi4NFv4ozlilJtblS/a48STOmh/hXoC6uf7Voett
Qk/bJ1gjB8//FV6xyaLZ6X7z7JrTyhePd5q0zB4UlJ6lCoi12x0Jc21SrFT7zv 3V
rt1vSjXt57zXgXcqDUtXc/aSnWH632nqvxW/EG5818/wWf+GWvfEFA==
=OyEI
-----END PGP SIGNATURE-----

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Brian C. Lane wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/27/2010 01:49 AM, Radek Vykydal wrote:



+ def read(self):
+ """Reads values from ifcfg file.
+
+ returns: number of values read
+ """



if not os.access(self.path, os.R_OK):
return None




Throwing an exception on open instead was intentional,
as it is really bad if we don't have access to the real
file (that's what the class is all about) - there is an assumption
that is quite related on in the stage 2 code that we have all ifcfg files
for network devices post stage1. Besides, it prevents from forgetting
to check return value of read. But maybe I'll reconsider it.


+ f = open(self.path, "r")
+ lines = f.readlines()
+ f.close()
+
+ for line in lines:
+ line = line.strip()
+ if line.startswith("#") or line == ':
+ continue
+ fields = line.strip().split('=', 2)



line is already stripped, so make this:

fields = line.split('=', 2)



right, thanks



+ # XXX hack
+ value = value.replace('"', ')
+ value = value.replace("'", ')



value = value.translate(None, "'"")



yeah, that's better. I just copy and pasted this from method
used to read ifcfg files from another place (I remove it in later
patch) and didn't change it much for safety reasons.



+ fd, newifcfg = tempfile.mkstemp(prefix="ifcfg-%s" % self.iface, text=False)



I think it is possible for this to fail. You may want to wrap it in a
try: except: check




Maybe, but the fail is quite fatal, and I am a bit afraid of catching
exceptions sooner than it proves necessary. Additionally, the mkstemp
call is not caught at other places in anaconda.


Thanks for the review.

Radek




_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/28/2010 12:49 AM, Radek Vykydal wrote:

> Thanks for the review.
>

Thanks for the explanations.

Is there a better way to do this than to do a series of patches that
modify earlier patches? I know part of my difficulty is unfamiliarity
with the code, but I expect that it makes reviewing things, especially
ones as big as this, hard for everyone.

I actually ran out of steam about half way through the patchset.

- --
Brian C. Lane <bcl@redhat.com>
Red Hat / Port Orchard, WA

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iQEVAwUBS9iF0hF+jBaO/jp/AQLgnwgAil5528qtKEjzHYN+Cx9+mdZsg5uSxYoK
Yzsb268j23x6t9+Ow3q+ytr6ze134S3jiowFYiNBdZVYbWpe5R LWlBgTmgxz1SyB
wmdPfIai027BlfAQeZcHxbt+umR6DJlu6clpjr5RFZeh4ReFMi 3EzdEqym+vmedV
kklFzbni0zQxaByaKdfIL6YKbUeRhzwVtLQWuFG02OU+UXF3cF zJrsBjtSQFNtR9
fHvEp2Q9oSupDvTHWfH5dQELzwRjelv9EvqDeJv3rXC+o29A18 rVOVoSf+RCwajN
aT7/UwiQamNTCgiJkL6Pw21WkUkjieMwBdCunQPGv7T9oZXbDxiJZQ ==
=oON6
-----END PGP SIGNATURE-----

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Wed, 28 Apr 2010, Brian C. Lane wrote:


On 04/28/2010 12:49 AM, Radek Vykydal wrote:


Thanks for the review.



Thanks for the explanations.

Is there a better way to do this than to do a series of patches that
modify earlier patches? I know part of my difficulty is unfamiliarity
with the code, but I expect that it makes reviewing things, especially
ones as big as this, hard for everyone.

I actually ran out of steam about half way through the patchset.


ReviewBoard is supposed to make this process easier, so once we can really
start using that more, I think posting revisions will be easier.

For email threads, I suggest people make the change(s) suggested and then
commit it to their local branch. Do an interactive rebase and squash the
change back in to the original patch you sent. You can then send that patch
to the list and note that it's a revision. Not very elegant, but eliminates
the "patches for patches" problem.

- --
David Cantrell <dcantrell@redhat.com>

Red Hat / Honolulu, HI

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iEYEARECAAYFAkvYiPIACgkQ5hsjjIy1VkmPRgCgpHFGz6QwvF LLC2+MrrAVFrZg
Ek4AoMOROL1VIUqtvnTHXLvM9eudjAG/
=xuc3
-----END PGP SIGNATURE-----

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Brian C. Lane wrote:


Is there a better way to do this than to do a series of patches that
modify earlier patches? I know part of my difficulty is unfamiliarity
with the code, but I expect that it makes reviewing things, especially
ones as big as this, hard for everyone.

I actually ran out of steam about half way through the patchset.




I tried really hard to split the whole change to patches
of successive steps I sent (squashing of lots of partial patches,
fixes, changes, etc... and then using add -i). This is the best
I was able to come up with, I hoped that it was the way that
would make understanding of the changes easier.
So generally the patches are not (and perhaps can't be)
independent and should be read successively.

Can you be more specific about patches (or hunks) that
modify earlier patches? I am not sure that I get what you mean.
Perhaps patches 12-17? For example enableNetwork and
some other functions are updated in successive patches
- they are adding features (12, 14, 16, 17) for which I am
asking ack in each patch. Some (13, 15) are simple fixes
that should have been squashed into earlier patch.
Some are fixes I am not sure about so I want ack
(18, 19, 20, 21, 22)

I really appreciate your review, and I am ready to answer
all your questions or go into detail.

Radek






_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

David Cantrell wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Wed, 28 Apr 2010, Brian C. Lane wrote:



Is there a better way to do this than to do a series of patches that
modify earlier patches? I know part of my difficulty is unfamiliarity
with the code, but I expect that it makes reviewing things, especially
ones as big as this, hard for everyone.

I actually ran out of steam about half way through the patchset.


ReviewBoard is supposed to make this process easier, so once we can
really

start using that more, I think posting revisions will be easier.

For email threads, I suggest people make the change(s) suggested and then
commit it to their local branch. Do an interactive rebase and squash the
change back in to the original patch you sent. You can then send that
patch
to the list and note that it's a revision. Not very elegant, but
eliminates

the "patches for patches" problem.


Yes, I'd definitely send a revision, not a patch on top of first take patch.
I think Brian meant something different (the series is just the first take)
- he was talking about the way the change is splitted into patches.

Radek

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Nothing beyond what bcl already said. Ack from me. I do question the use of
shutil.move() because, in my experience, that function has been prone to
failure pretty easy. Almost easier to copy and then remove. But maybe the
function is a little safer these days.

On Tue, 27 Apr 2010, Radek Vykydal wrote:


We are communicating with nm via ifcfg files heavily (for configuring
we do it exclusively), and we started to use nm-c-e which does the same,
we can pass parameters to nm-c-e only via ifcfg files, so we need
backend class for NetworkDevice object to be able to keep it synced
with ifcfg files reasonably.
---
simpleconfig.py | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ ++--
1 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/simpleconfig.py b/simpleconfig.py
index b3e6c90..1f90356 100644
--- a/simpleconfig.py
+++ b/simpleconfig.py
@@ -16,6 +16,8 @@

import string
import os
+import tempfile
+import shutil

# use our own ASCII only uppercase function to avoid locale issues
# not going to be fast but not important
@@ -80,9 +82,68 @@ class SimpleConfigFile:

def get (self, key):
key = uppercase_ASCII_string(key)
- if self.info.has_key (key):
- return self.info[key]
+ return self.info.get(key, "")
+
+
+class IfcfgFile(SimpleConfigFile):
+
+ def __init__(self, dir, iface):
+ SimpleConfigFile.__init__(self)
+ self.iface = iface
+ self.dir = dir
+
+ @property
+ def path(self):
+ return os.path.join(self.dir, "ifcfg-%s" % self.iface)
+
+ def clear(self):
+ self.info = {}
+
+ def read(self):
+ """Reads values from ifcfg file.
+
+ returns: number of values read
+ """
+ f = open(self.path, "r")
+ lines = f.readlines()
+ f.close()
+
+ for line in lines:
+ line = line.strip()
+ if line.startswith("#") or line == ':
+ continue
+ fields = line.strip().split('=', 2)
+ if len(fields) < 2:
+ continue
+ key = uppercase_ASCII_string(fields[0])
+ value = fields[1]
+ # XXX hack
+ value = value.replace('"', ')
+ value = value.replace("'", ')
+ self.info[key] = value
+
+ return len(self.info)
+
+ # This method has to write file in a particular
+ # way so that ifcfg-rh's inotify mechanism triggeres
+ # TODORV: check that it is still true.
+ def write(self, dir=None):
+ """Writes values into ifcfg file."""
+
+ if not dir:
+ path = self.path
else:
- return ""
+ path = os.path.join(dir, os.path.basename(self.path))
+
+ fd, newifcfg = tempfile.mkstemp(prefix="ifcfg-%s" % self.iface, text=False)
+ os.write(fd, self.__str__())
+ os.close(fd)

+ os.chmod(newifcfg, 0644)
+ try:
+ os.remove(path)
+ except OSError, e:
+ if e.errno != 2:
+ raise
+ shutil.move(newifcfg, path)




- --
David Cantrell <dcantrell@redhat.com>

Red Hat / Honolulu, HI

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iEYEARECAAYFAkvfiC0ACgkQ5hsjjIy1Vknl0ACgpwN0l8ilvI 2NrYoedsHc+vbj
FTgAoIif4BmRaYKBf9uGoIRdlBOu7NbW
=TCxz
-----END PGP SIGNATURE-----

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list