[Pkg-uml-devel] Re: [Pkg-uml-commit] r81 - in trunk/src/rootstrap: . debian modules

[Mattia Dongili]

Some comments:

On Sun, May 21, 2006 at 10:38:54PM +0000, Stefano Melchior wrote:
> Author: stex-guest
> Date: 2006-05-21 22:38:53 +0000 (Sun, 21 May 2006)
[...]
> Log:

please, pretty please, write relevant changes in the SVN log.
A Good Approach (tm) is to have small changesets (eg: committing only
related portions of you work together) and useful logs, main advantages
being:
- if you need to revert a change you don't loose all your changes
- easy to track changes by other people
- ... more
I know I sometimes commit big blobs too [shame on me], however we should
try to work in an more appropriate team-based-fashion. :)

[...]
> Modified: trunk/src/rootstrap/debian/changelog
> ===================================================================
> --- trunk/src/rootstrap/debian/changelog	2006-05-21 20:49:14 UTC (rev 80)
> +++ trunk/src/rootstrap/debian/changelog	2006-05-21 22:38:53 UTC (rev 81)
> @@ -1,3 +1,15 @@
> +rootstrap (0.3.21-2) unstable; urgency=low

If we're taking over roostrap completely (are we?) I'd release a new
shiny 0.3.22, agree?

> +
> +  * fixed issue for 2.6 kernel for ubd device (Closes: #272914), (closes: #282914)

which issue? the changelog should better describe changes without
requiring people to look at the bugreport to understand what changed.
What about bug 272914??? where does it come from? :)

> +  * fixed Standards depedance changes
> +  * fixed user-mode-linux dependancy (Closes: #275410)
> +  * fixed udb missing (Closes: #342694)

"fixed" doesn't tell much if one doesn't know what's broken, I'd use the
following wording (just an example):
 * bumped Standards-Version to 3.7.2
 * removed version[1] in user-mode-linux dependency (closes...)
 * creating ubd device nodes if missing (closes...)

[1]: by the way are we really sure it works with old (2.4 series) UMLs?

> +  * uml-utilities fix assures network to work with u-m-l 2.6 serie (Closes: #304461)

if this was really an uml-utilites bug it should be closed differently
(not by a roostrap upload at least).

> +  * fixed issue with more recent version of u-m-l (Closes: #320410)

oh, #320410 seems to be unrelated to the description (and not fixed
too).

> +  * add reference of the hostfs need on man page (Closes: #300221)

Actually in the manpage you don't say that host_fs needs to be built-in
and not as a module in the guest kernel that was the culprit of the
bugreport :)

> Modified: trunk/src/rootstrap/modules/uml
> ===================================================================
> --- trunk/src/rootstrap/modules/uml	2006-05-21 20:49:14 UTC (rev 80)
> +++ trunk/src/rootstrap/modules/uml	2006-05-21 22:38:53 UTC (rev 81)
> @@ -2,9 +2,11 @@
>  
>  # Add ubd devices on host (if not present)
>  
> -if [ ! -d /dev/ubd || -e /dev/ubd$i ]; then
> +if [ ! -d /dev/ubd ]; then
>  	for i in 0 1 2 3 4 5 6 7; do
> -		mknod $TARGET/dev/ubd$i b 98 $[$i * 16]; 
> +		if [ -e /dev/ubd$i ]; then
                   ^^^^
if [ ! -e /dev/ubd$i ] ; then ?

> +                       mknod $TARGET/dev/ubd$i b 98 $[$i * 16];

and as said Sven, you're still testing for /dev/udb[01234567] and
creating $TARGET/dev/ubd[01234567]

> +               fi
>	done
> fi

no "else" here? What happens if /dev/ubd exists and is a directory?
The rest of the file also re-does:

for i in 0 1 2 3 4 5 6 7; do mknod $TARGET/dev/ubd$i b 98 $[$i * 16]; done
...
chroot $TARGET /bin/sh -c 'cd /dev && ./MAKEDEV ubd'

Isn't it a bit redundant? :)

bye
-- 
mattia
:wq!