[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: ppp_2.4.4rel-5 patch

On Tuesday 09 January 2007 19:07, Eddy Petrişor wrote:
> Attached is a patch that closes a few open issues with the ppp-udeb.
> The most important are #402450 (not bringing up the interface after
> install and not saving a correspondent section in the target
> /etc/network/interfaces) and the handling of bad login information
> being typed.

Patch looks better than previous version. Still some comments.


I think these earlier comments are still valid:
! I still have some reservations regarding this patch.
! - netcfg's base-installer script already copies the /etc/network/*
!   files to the target system; what does the interfaces file in the d-i
!   environment contain if ppp-udeb is run?
!   as the netcfg base-installer script will still be run, this is
!   relevant.
! - The 50config-target-ppp script is not idempotent: it will
!   add the same section again and again if it is run more than once.

In combination these two comments are deadly: you are potentially _adding_ 
to an existing /target/etc/network/interfaces that was copied from the 
d-i environment. If you are certain that no /etc/network/interfaces is 
created in the d-i environment if ppp-udeb is used, this is a bit less of 
an issue.

> Issues still open:
> - hostname is not set (I think I can force netcfg to run the mandatory
> steps, but I didn't had the time to do it during the winter holidays)
> and does not yet blend well with netcfg

I don't think you should want to use netcfg for that in the current 
situation. ppp-udeb should be more or less self-sufficient.
IMO this issue is important.

What about the /etc/hosts file? Is that being created?

> - finish-install.d was not working at some point in the past, but the
>   current situation is uncertain

Huh? We would have installation failures all over the place if 
finish-install.d was not working...

Other comments
As you are introducing new templates, you should allow translators the 
opportunity to update their translations!

Is the lo interface now being activated for the installation itself? If 
not, that could cause installation problems. Note that the netmask for 
the lo interface should have address!
netcfg uses:
   ip link set lo up
   ip addr flush dev lo
   ip addr add dev lo

From the patch
+  * ppp-udeb: the interface is now raised in the installed system
Interfaces are not "raised", but "brought up" or maybe "activated".

+  * Added myself to Uploades with maintainer's consent.

+log-output -t ppp-udeb pppd call provider || RET=$?
+if [ 0$RET -eq 19 ]; then
Why not just initialize RET=0, then you can replace the ugly "0$RET" by
"$RET" for both tests.

Attachment: pgpgGfXNwyCOd.pgp
Description: PGP signature

Reply to: