Re: Proposal to add patches to netcfg (#682737)
Hello,
On Tue, Nov 20, 2012 at 1:42 PM, Philipp Kern <pkern@debian.org> wrote:
> Hi,
>
> so your patches looked very good, and as Michael already mailed, they
> have been merged (but not yet uploaded). I need to try yet another
> installation with the desktop task activated, which takes a fair while.
> Afterwards it should be uploaded, hit the dailies and hopefully not
> be the cause of much breakage. ;-)
Nice to hear this, thanks, I also do hope that they won't cause much breakage :D
> I did tweak a couple of things:
>
> * e946cee nm-conf: Use Linux's random/uuid proc entry if available.
> * ba5ee42 Write out network-manager configuration files with mode 0600.
> * 46208b2 Reword target_network_config template; do not ask the question.
> * d2dd5a1 nm-conf: use ESSID as connection name
> * 83e9215 test: reactivate test_nc_v6_interface_configured_suite
> * 4e1dbbd finish-install: Do not set /etc/network/interfaces to mode 0600.
> * 5bfce24 nm-conf: Allow both IPv4 and IPv6 to be activated post-installation.
> * 7f9f1d2 nm-conf: Only write out MAC addresses on static configurations.
> * c1e4697 Add a changelog entry for the n-m write config changes.
>
> So we need libuuid only on non-Linux platforms (I hope it still compiled
> there). As discussed the mode of both n-m config files and /e/n/i was
> adjusted to be private to root. The ESSID connection name change is done
> because Michael said that n-m's default changed, hence do the same in
> netcfg. I longly discussed the MAC write out with Michael and I guess we
> reached a sane conclusion here that still allows e.g. images be produced
> with one hardware (say KVM) and reused on another one while keeping the
> settings and not creating a new connection while still dealing with
> multiple NICs. The IPv4/IPv6 change is IPv6 enablement: if IPv6 gets
> activated on the network at some point later post-installation the
> machine should still take advantage of it instead of ignoring it forever.
>
> Two tiny notes about the coding style:
>
> * You aligned struct members horizontally. That causes diffs to be more
> noisy, makes them more likely to conflict and more lines need to be
> changed when a new member is added.
I didn't think of diffs when I did so :) Maybe it would be better to
have one single commit aligning them to the left and after that
everything will be good.
> * You passed the structs in the nm_write_* function by value. Is there
> a reason for this? I presume it's partly due to the fact that
> nm_config_info inlines the structs. Still the structure's parts
> should not be copied when passed to another function, hence pointers
> should be used.
I think the reason I did that in the beginning was that no values
needs to be modified during writing so passing the address wouldn't be
necessary. Since there are only few calls for each function, I didn't
believe that copying the value of each structure is a problem, but I
agree that it would be best not to, I'll write a patch for this.
> I did not refactor this when basing my changes upon your branch, though.
>
> Lastly thanks for following up on this and your great work. :-)
Thanks,
Sorina
Reply to: