~alpine/devel

3 2

Re: [alpine-devel] Understanding alpine-setup

Details
Message ID
<1330880212.26962.140661044634665@webmail.messagingengine.com>
Sender timestamp
1330880212
DKIM signature
missing
Download raw message
Thanks Jeff for your reply.

> > 2. The "-a" option is bewildering. If I supply it, we create a directory
> > under /tmp and save its name in ROOT. However we set a trap on exit to
> > remove this directory, and never cancel the trap. From what I've seen so
> > far, that ROOT directory is never copied anywhere else, right? So this
> > functionality doesn't yet work? Also, the ROOT variable isn't exported,
> > so I don't see how the subroutine scripts like
> > setup-{hostname,interfaces,keymap} expect to have it populated. Is all
> > of this just under construction?
> 
> The idea with that option is to create an apkovl that can be copied to
> boot media.  On boot, those settings would be copied into tmpfs along
> with the packages that are specified in that overlay file.  However, I
> haven't done much with that option in the past.

Yes, I figured it was supposed to do something like that. But I don't
see how it could succeed in doing so, in the scripts as currently
written (on the 2.3.6 ISO). Or is your last sentence acknowledging that
this functionality is still being developed?

I realize many of these scripts are most likely works in progress, and
maybe no one's even aimed to fully document them yet. Nonetheless I
thought it might still be useful for me to point out these gaps as I see
them. And once I get myself better set up on Alpine, I will post patches
or enter bug reports directly. At the moment, I'm still just getting
into the water.



On Sun, Mar 4, 2012, at 05:19 AM, lists+alpine-devel@jimpryor.net wrote:

> About to dive into setup-bootable and setup-disk now. It looks like
> setup-bootable (like setup-cryptswap and some others) is only invoked
> manually, correct?

Things I notice with setup-bootable:

* undocumented that the script respects MNT when SOURCE is a ISO
* The usage message neglects to say that SOURCE can also be a url.
* undocumented that the script respects WGET when SOURCE is a url
* The usage message says that DEST/$2 can be a mounted directory or a
device. This may be misleading: (a) If I mount /dev/foo to /mnt/foo, and
then there exists a /mnt/foo/subdir, does the latter count as a "mounted
directory"? I wouldn't be sure it doesn't; however, if I understand the
logic of setup-bootable correctly, I can't provide /mnt/foo/subdir as a
value for $2. Perhaps it would be clearer to say "a directory
mountpoint" instead of "a mounted directory"? That would also help with
the second misleading aspect: (b) DEST need not be mounted;
setup-bootable will try to mount it and I guess if it's listed in the
fstab will succeed.
* When setting srcdir, an echo ... | sed ... construct is used to strip
any trailing "/". Is there a specific reason for doing it that way,
instead of just with srcdir=${src%/}, as is done several times elsewhere
in the same script?


I started to make a wiki page to document the setup scripts better.
Based solely on my skimming of the code. If some of this is also
documented elsewhere in the wiki, such as in
http://wiki.alpinelinux.org/wiki/Alpine_Linux_Handbook, I expect I'll
notice eventually and will try to integrate the notes then. Or someone
else who notices this is welcome to point it out.

Anyone should of course feel free to correct inaccuracies or
misunderstandings I express on that page.

Reading setup-disk now...
-- 
Jim Pryor
jim@jimpryor.net



---
Unsubscribe:  alpine-devel+unsubscribe@lists.alpinelinux.org
Help:         alpine-devel+help@lists.alpinelinux.org
---

Re: [alpine-devel] Understanding alpine-setup

Details
Message ID
<1330897651.26242.140661044650293@webmail.messagingengine.com>
In-Reply-To
<1330880212.26962.140661044634665@webmail.messagingengine.com> (view parent)
Sender timestamp
1330897651
DKIM signature
missing
Download raw message
On Sun, Mar 4, 2012, at 11:56 AM, lists+alpine-devel@jimpryor.net wrote:
> Reading setup-disk now...

Ok, I have a variety of questions here. I don't really know a way to do
this efficiently. I'm going to include portions of the /sbin/setup-disk
file with numbered lines, and intersperse my questions / observations.

     6       MBR=${MBR:-"/usr/share/syslinux/mbr.bin"}
     7       ROOTFS=${ROOTFS:-ext4}
     8       BOOTFS=${BOOTFS:-ext4}

Mightn't it also be useful to have a VARFS, instead of always just using
ext4?


    20      all_in_list() {
    21              local needle="$1"
    22              local i
    23              [ -z "$needle" ] && return 1

I see that the way this function is used later in the script requires an
empty $needle to fail; however, since this goes counter to what one
expects from the name of the function (if $needle is empty, all elements
of $needle are in the list), perhaps it's worth a comment here so that
it never gets accidentally broken in the future.


    54      # generate an fstab from a given mountpoint. Convert to UUID
    if possible
    55      enumerate_fstab() {
    56              local mnt="$1"
    57              local fs_spec= fs_file= fs_vfstype= fs_mntops=
    fs_freq= fs_passno=
    58              [ -z "$mnt" ] && return
    59              local escaped_mnt=$(echo $mnt | sed 's:/:\\/:g')
    60              awk "\$2 ~ /^$escaped_mnt/ {print \$0}" /proc/mounts
    | \

Notice that $escaped_mnt is only anchored at the beginning, not at the
end. Mightn't this break if one of my mountpoints is a prefix of
another?


    76      # return true (0) if given device is lvm
    77      is_lvm() {
    78              lvs "$1" >/dev/null 2>&1
    79      }

Perhaps the comment could be updated to say specifically that the $1 is
expected to be a lvs, not a vg. This had me scratching my head for a
while, since supplying a bare vg name as $1 will also work here; however
the code that uses this function will always be supplying putative lvs,
not vgs.


   163     install_mounted_root() {
   ...
   183     
   184             bootdev=$(find_mount_dev "$mnt"/boot)
   185             if [ -z "$bootdev" ]; then
   186                     bootdev=$rootdev
   187             else
   188                     mnt_boot="$mnt"/boot
   189                     bootdev=$(find_mount_dev "$mnt_boot")

Isn't this line redundant? bootdev is already what we want it to be on
this codepath, no?



   194             if [ -e "/sys/block/${bootdev#/dev/}/md" ]; then
   195                     raidopt="--raid"
   196             fi
   197     
   198             for dev in $rootdev $pvs; do
   199                     [ -e "/sys/block/${dev#/dev/}/md" ] ||
   continue
   200     
   201                     local md=${dev#/dev/}
   202                     initfs_features="$(echo $initfs_features |
   sed 's/raid//') raid"
   203                     local level=$(cat /sys/block/$md/md/level)
   204                     case "$level" in
   205                             raid1) raidmod="$raidmod,$level";;
   206                             raid[456])
   raidmod="$raidmod,raid456";;
   207                     esac
   208             done

Might be nice to add a comment explaining that $raidopt records the
raid-status of the bootdev, and is used only if is_xen, in the
invocation of extlinux, whereas raidmod records the raid-status of the
rootvol[s], and is used in the /etc/mkinitfs and
/etc/update-extlinux.conf configuration. This can be extracted by
grepping the source, but it would be friendly to just explain it where
these variables are initialized. Also, if some of the behavior described
here is unintended / leftover cruft, this is a good chance to fix it.
:-)


   201                     local md=${dev#/dev/}
   202                     initfs_features="$(echo $initfs_features |
   sed 's/raid//') raid"
   ...
   211             case $rootdev in
   212                     /dev/cciss/*)
   213                             initfs_features="$(echo
   $initfs_features | sed 's/raid//') raid"
   214                             ;;

Note to myself: Why are echo... | sed... necessary here? Wouldn't shell
manipulation suffice? Answer: BusyBox ash does let you use
${FOO//orig/repl} constructs, but we should steer clear of non-POSIX sh
capabilities, and this isn't one. However, if the desired replacements
here were anchored (^raid or raid$), then shell manipulation would
suffice.


   286             is_xen || extlinux $raidopt --install "$mnt"/boot
   
Why do we use extlinux here and syslinux everywhere else? At one point I
understood the difference between them, but now I've forgotten.


   289     unmount_partitions() {
   290             local mnt="$1"
   291     
   292             # unmount the partitions
   293             umount $(awk '{print $2}' /proc/mounts | grep ^"$mnt"
   | sort -r)
   294     }

Notice that as on line 60, the regexp pattern here is only anchored at
the beginning. Mightn't this break? Shouldn't we anchor it on both
sides?


   411     # setup disk dev in $1 for LVM usage.
   412     # usage: setup_partitions <diskdev> size1,type1 [size2,type2
   ...] 
   413     setup_partitions() {
   
That comment is misleading; this function is also called on codepaths
that don't use LVM.


   436             ) | sfdisk -q -L -uM $diskdev >>/tmp/sfdisk.out ||
   return 1
   
Who uses /tmp/sfdisk.out? It doesn't show up when I grep all the other
setup-* scripts.

   454     # set up boot device. We only use raid1 for boot devices if
   any raid
   455     setup_boot_dev() {

That comment seems to be truncated or otherwise ungrammatical.



   492     setup_non_boot_raid_dev() {
   ...
   497             local numdevs=$#

This seems to be cruft; so far as I can see this variable is never again
referenced.

   563     # set up /var on given device
   564     setup_var() {
   565             local var_dev="$1"
   566             local varfs=ext4

See comment to line 8 about possibly introducing a VARFS env variable.


   596     data_only_disk_install_lvm() {
   ...
   601             local raid_part_type="fd"
   ...
   608             if [ "$USE_RAID" ]; then
   609                     part_type=$raid_part_type
   610                     stop_all_raid
   611             fi

Is raid_part_type necessary? It doesn't seem to used anywhere else, and
at several other parallel places in this same script, we just say
part_type="fd". See also comment to line 684, below.

   661     # setup 
   662     setup_root() {
   663             local root_dev="$1" boot_dev="$2"
   664             mkfs.$ROOTFS -q "$root_dev"
   665             mkdir -p /mnt
   666             mount -t $ROOTFS $root_dev /mnt || return 1
   667             if [ -n "$boot_dev" ]; then
   668                     mkdir -p /mnt/boot
   669                     mount -t $BOOTFS $boot_dev /mnt/boot ||
   return 1
   670             fi
   671     
   672             setup_mdadm_conf
   673             install_mounted_root /mnt || return 1
   674             unmount_partitions /mnt
   675             swapoff -a
   676     
   677             echo ""
   678             echo "Installation is complete. Please reboot."
   679     }

setup-bootable uses ${MNT:-/mnt}, instead of just mandating /mnt. Should
we do the same here?


   681     native_disk_install_lvm() {
   ...
   684             local raid_part_type="fd"

This variable doesn't seem to be anywhere referenced.

   690             init_progs syslinux || return 1
   ...
   721             init_progs syslinux || return 1

syslinux is already included in the packages init_progs installs by
default.


   817     usage() {
   818             cat <<__EOF__
   819     usage: setup-disk [-hqr] [-k kernelflavor] [-m MODE] [-o
   apkovl] [-s SWAPSIZE]
   820                       [MOUNTPOINT | DISKDEV...]

Shouldn't we s/-hqr/-hqrLv/ ? See below.

   ...
   831     options:
   832      -h  Show this help
   833      -m  Use disk for MODE without asking, where MODE is either
   'data' or 'root'

Shouldn't we s/root/sys/ ? Perhaps also permit specifying "-m none"?
Alternatively, see comment to line 895, below.
   ...
   836      -L  Use LVM to manage partitions
   ...
   839      -s  Use SWAPSIZE MB instead of $SWAP_SIZE MB for swap (Use 0
   to disable swap)

It looks like, when usage() would normally be run, $SWAP_SIZE won't yet
have been set?

   840      -v  Be more verbose about what is happening
   841     __EOF__



   895     if [ $# -gt 0 ]; then

Mightn't it be useful to permit, and check whether, $* = "none"?

   896             # check that they are 
   897             for i in "$@"; do
   898                     j=$(readlink -f "$i" | sed 's:^/dev/::;
   s:/:!:g')
   899                     if ! [ -e "/sys/block/$j/device" ]; then
   900                             echo "$i is not a suitable for
   partitioning"
   901                             exit 1
   902                     fi
   903                     diskdevs="$diskdevs /dev/$j"
   904             done
   905     else
   906             ask_disk "Which disk(s) would you like to use? (or
   '?' for help or 'none')" \
   907                     diskselect_help $disks
   908             if [ "$answer" != none ]; then
   909                     for i in $answer; do
   910                             diskdevs="$diskdevs /dev/$i"
   911                     done
   912             else
   913                     DISK_MODE="none"
   914             fi
   915     fi


-- 
Jim Pryor
jim@jimpryor.net



---
Unsubscribe:  alpine-devel+unsubscribe@lists.alpinelinux.org
Help:         alpine-devel+help@lists.alpinelinux.org
---

Re: [alpine-devel] Understanding alpine-setup

Natanael Copa <ncopa@alpinelinux.org>
Details
Message ID
<20120305101632.17dda3f4@ncopa-desktop.nor.wtbts.net>
In-Reply-To
<1330880212.26962.140661044634665@webmail.messagingengine.com> (view parent)
Sender timestamp
1330938992
DKIM signature
missing
Download raw message
On Sun, 04 Mar 2012 11:56:52 -0500
lists+alpine-devel@jimpryor.net wrote:

...
> > The idea with that option is to create an apkovl that can be copied
> > to boot media.  On boot, those settings would be copied into tmpfs
> > along with the packages that are specified in that overlay file.
> > However, I haven't done much with that option in the past.
> 
> Yes, I figured it was supposed to do something like that. But I don't
> see how it could succeed in doing so, in the scripts as currently
> written (on the 2.3.6 ISO). Or is your last sentence acknowledging
> that this functionality is still being developed?

Development started but died long time ago. We should complete it or
remove it.

> I realize many of these scripts are most likely works in progress, and
> maybe no one's even aimed to fully document them yet. Nonetheless I
> thought it might still be useful for me to point out these gaps as I
> see them. And once I get myself better set up on Alpine, I will post
> patches or enter bug reports directly. At the moment, I'm still just
> getting into the water.

Yes. Thanks for your feedback!

-nc


---
Unsubscribe:  alpine-devel+unsubscribe@lists.alpinelinux.org
Help:         alpine-devel+help@lists.alpinelinux.org
---

Re: [alpine-devel] Understanding alpine-setup

Natanael Copa <ncopa@alpinelinux.org>
Details
Message ID
<20120305141254.484661d1@ncopa-desktop.nor.wtbts.net>
In-Reply-To
<1330897651.26242.140661044650293@webmail.messagingengine.com> (view parent)
Sender timestamp
1330953174
DKIM signature
missing
Download raw message
On Sun, 04 Mar 2012 16:47:31 -0500
Jim Pryor <lists+alpine-devel@jimpryor.net> wrote:

> 
> On Sun, Mar 4, 2012, at 11:56 AM, lists+alpine-devel@jimpryor.net
> wrote:
> > Reading setup-disk now...
> 
> Ok, I have a variety of questions here. I don't really know a way to
> do this efficiently. I'm going to include portions of
> the /sbin/setup-disk file with numbered lines, and intersperse my
> questions / observations.
> 
>      6       MBR=${MBR:-"/usr/share/syslinux/mbr.bin"}
>      7       ROOTFS=${ROOTFS:-ext4}
>      8       BOOTFS=${BOOTFS:-ext4}
> 
> Mightn't it also be useful to have a VARFS, instead of always just
> using ext4?

You are of course right about that.

Fixed. Thanks!


> 
> 
>     20      all_in_list() {
>     21              local needle="$1"
>     22              local i
>     23              [ -z "$needle" ] && return 1
> 
> I see that the way this function is used later in the script requires
> an empty $needle to fail; however, since this goes counter to what one
> expects from the name of the function (if $needle is empty, all
> elements of $needle are in the list), perhaps it's worth a comment
> here so that it never gets accidentally broken in the future.

or change the function name to "non_empty_all_in_list"?

> 
>     54      # generate an fstab from a given mountpoint. Convert to
> UUID if possible
>     55      enumerate_fstab() {
>     56              local mnt="$1"
>     57              local fs_spec= fs_file= fs_vfstype= fs_mntops=
>     fs_freq= fs_passno=
>     58              [ -z "$mnt" ] && return
>     59              local escaped_mnt=$(echo $mnt | sed 's:/:\\/:g')
>     60              awk "\$2 ~ /^$escaped_mnt/ {print
> \$0}" /proc/mounts | \
> 
> Notice that $escaped_mnt is only anchored at the beginning, not at the
> end. Mightn't this break if one of my mountpoints is a prefix of
> another?

I think you are right. I wonder if there are any practical cases where
this might happen. Maybe if you mount your dir as /d/ (which would
include /dev/)

Yup. tested here. It ends up creating various /ev/* entries in fstab.

Good catch!

How about this fix:
--- a/setup-disk.in
+++ b/setup-disk.in
@@ -57,8 +57,8 @@ enumerate_fstab() {
        local mnt="$1"
        local fs_spec= fs_file= fs_vfstype= fs_mntops= fs_freq= fs_passno=
        [ -z "$mnt" ] && return
-       local escaped_mnt=$(echo $mnt | sed 's:/:\\/:g')
-       awk "\$2 ~ /^$escaped_mnt/ {print \$0}" /proc/mounts | \
+       local escaped_mnt=$(echo $mnt | sed -e 's:/*$::' -e 's:/:\\/:g')
+       awk "\$2 ~ /^$escaped_mnt(\/|\$)/ {print \$0}" /proc/mounts | \
                sed "s:$mnt:/:g; s: :\t:g" | sed 's:/\+:/:g' | \
                while read fs_spec fs_file fs_vfstype fs_mntops fs_freq fs_passno; do
                        echo -e "$(uuid_or_device $fs_spec)\t${fs_file}\t${fs_vfstype}\t${fs_mntops} ${fs_freq} ${fs_passno}"


> 
> 
>     76      # return true (0) if given device is lvm
>     77      is_lvm() {
>     78              lvs "$1" >/dev/null 2>&1
>     79      }
> 
> Perhaps the comment could be updated to say specifically that the $1
> is expected to be a lvs, not a vg. This had me scratching my head for
> a while, since supplying a bare vg name as $1 will also work here;
> however the code that uses this function will always be supplying
> putative lvs, not vgs.

Do you have a better test that will return true on lvs but false on vgs?

If not I suppose a comment will do.

 
>    163     install_mounted_root() {
>    ...
>    183     
>    184             bootdev=$(find_mount_dev "$mnt"/boot)
>    185             if [ -z "$bootdev" ]; then
>    186                     bootdev=$rootdev
>    187             else
>    188                     mnt_boot="$mnt"/boot
>    189                     bootdev=$(find_mount_dev "$mnt_boot")
> 
> Isn't this line redundant? bootdev is already what we want it to be on
> this codepath, no?

Indeed. Also, it is slightly confusing that we depend on the earlier initialized
mnt_boot. Lets move it to same if .. else .. clause to make it clearer
whats going on.

 
> 
> 
>    194             if [ -e "/sys/block/${bootdev#/dev/}/md" ]; then
>    195                     raidopt="--raid"
>    196             fi
>    197     
>    198             for dev in $rootdev $pvs; do
>    199                     [ -e "/sys/block/${dev#/dev/}/md" ] ||
>    continue
>    200     
>    201                     local md=${dev#/dev/}
>    202                     initfs_features="$(echo $initfs_features |
>    sed 's/raid//') raid"
>    203                     local level=$(cat /sys/block/$md/md/level)
>    204                     case "$level" in
>    205                             raid1) raidmod="$raidmod,$level";;
>    206                             raid[456])
>    raidmod="$raidmod,raid456";;
>    207                     esac
>    208             done
> 
> Might be nice to add a comment explaining that $raidopt records the
> raid-status of the bootdev, and is used only if is_xen, in the
> invocation of extlinux,

It is actually the opposite. The $raidopt is only used if not xen.

> whereas raidmod records the raid-status of the
> rootvol[s], and is used in the /etc/mkinitfs and
> /etc/update-extlinux.conf configuration. This can be extracted by
> grepping the source, but it would be friendly to just explain it where
> these variables are initialized. Also, if some of the behavior
> described here is unintended / leftover cruft, this is a good chance
> to fix it. :-)

Could any xen user exmplain why we don't want run extlinux when xen is
detected? Do we still want this behaviour?


>    201                     local md=${dev#/dev/}
>    202                     initfs_features="$(echo $initfs_features |
>    sed 's/raid//') raid"
>    ...
>    211             case $rootdev in
>    212                     /dev/cciss/*)
>    213                             initfs_features="$(echo
>    $initfs_features | sed 's/raid//') raid"
>    214                             ;;
> 
> Note to myself: Why are echo... | sed... necessary here? Wouldn't
> shell manipulation suffice? Answer: BusyBox ash does let you use
> ${FOO//orig/repl} constructs, but we should steer clear of non-POSIX
> sh capabilities, and this isn't one. However, if the desired
> replacements here were anchored (^raid or raid$), then shell
> manipulation would suffice.

I think the shell manipulation would suffice in this case. The
initfs_features is a local var which we have full control over and the
code only makes sure we don't add the "raid" multiple times.

Looks like we should do the same with raidmod btw. Seems like there is
a chance that raidmod gets added multiple times if the root filesystem
is on a lvm with multiple raids as pvs.

> 
>    286             is_xen || extlinux $raidopt --install "$mnt"/boot
>    
> Why do we use extlinux here and syslinux everywhere else? At one
> point I understood the difference between them, but now I've
> forgotten.

syslinux: works only only on FAT. for booting CF or USB and run from tmpfs.

isolinux: for booting from cdrom

extlinux: is a more general prupose boot loader (like lilo and grub).
          for booting on a 'sys' installed disk.

http://www.syslinux.org/wiki/index.php/EXTLINUX

> 
> 
>    289     unmount_partitions() {
>    290             local mnt="$1"
>    291     
>    292             # unmount the partitions
>    293             umount $(awk '{print $2}' /proc/mounts | grep
> ^"$mnt" | sort -r)
>    294     }
> 
> Notice that as on line 60, the regexp pattern here is only anchored at
> the beginning. Mightn't this break? Shouldn't we anchor it on both
> sides?

Yes. I think you are right.

> 
> 
>    411     # setup disk dev in $1 for LVM usage.
>    412     # usage: setup_partitions <diskdev> size1,type1
> [size2,type2 ...] 
>    413     setup_partitions() {
>    
> That comment is misleading; this function is also called on codepaths
> that don't use LVM.

Fixed. Thanks!

> 
>    436             ) | sfdisk -q -L -uM $diskdev >>/tmp/sfdisk.out ||
>    return 1
>    
> Who uses /tmp/sfdisk.out? It doesn't show up when I grep all the other
> setup-* scripts.

I would guess it was for debugging purposes. Probably never used.

> 
>    454     # set up boot device. We only use raid1 for boot devices if
>    any raid
>    455     setup_boot_dev() {
> 
> That comment seems to be truncated or otherwise ungrammatical.

Fixed. Thanks!

> 
> 
> 
>    492     setup_non_boot_raid_dev() {
>    ...
>    497             local numdevs=$#
> 
> This seems to be cruft; so far as I can see this variable is never
> again referenced.

Yup. You are right. I have removed this. Thanks!

> 
>    563     # set up /var on given device
>    564     setup_var() {
>    565             local var_dev="$1"
>    566             local varfs=ext4
> 
> See comment to line 8 about possibly introducing a VARFS env variable.

Fixed. Thanks!

> 
>    596     data_only_disk_install_lvm() {
>    ...
>    601             local raid_part_type="fd"
>    ...
>    608             if [ "$USE_RAID" ]; then
>    609                     part_type=$raid_part_type
>    610                     stop_all_raid
>    611             fi
> 
> Is raid_part_type necessary? It doesn't seem to used anywhere else,
> and at several other parallel places in this same script, we just say
> part_type="fd". See also comment to line 684, below.

I think i did that for remembering that "fd" is the partition type for
raid. But you are right of course.

Fixed. Thanks!

>    661     # setup 
>    662     setup_root() {
>    663             local root_dev="$1" boot_dev="$2"
>    664             mkfs.$ROOTFS -q "$root_dev"
>    665             mkdir -p /mnt
>    666             mount -t $ROOTFS $root_dev /mnt || return 1
>    667             if [ -n "$boot_dev" ]; then
>    668                     mkdir -p /mnt/boot
>    669                     mount -t $BOOTFS $boot_dev /mnt/boot ||
>    return 1
>    670             fi
>    671     
>    672             setup_mdadm_conf
>    673             install_mounted_root /mnt || return 1
>    674             unmount_partitions /mnt
>    675             swapoff -a
>    676     
>    677             echo ""
>    678             echo "Installation is complete. Please reboot."
>    679     }
> 
> setup-bootable uses ${MNT:-/mnt}, instead of just mandating /mnt.
> Should we do the same here?

Good idea. I wonder if we should use $SYSROOT instead of $MNT though.

>    681     native_disk_install_lvm() {
>    ...
>    684             local raid_part_type="fd"
> 
> This variable doesn't seem to be anywhere referenced.

Fixed. Thanks!

> 
>    690             init_progs syslinux || return 1
>    ...
>    721             init_progs syslinux || return 1
> 
> syslinux is already included in the packages init_progs installs by
> default.

Fixed. Thanks!

> 
>    817     usage() {
>    818             cat <<__EOF__
>    819     usage: setup-disk [-hqr] [-k kernelflavor] [-m MODE] [-o
>    apkovl] [-s SWAPSIZE]
>    820                       [MOUNTPOINT | DISKDEV...]
> 
> Shouldn't we s/-hqr/-hqrLv/ ? See below.

Yes.
 
>    ...
>    831     options:
>    832      -h  Show this help
>    833      -m  Use disk for MODE without asking, where MODE is either
>    'data' or 'root'
> 
> Shouldn't we s/root/sys/ ?

Yes.

> Perhaps also permit specifying "-m none"?

I think in that case you wouldn't call setup-disk in first place.

> Alternatively, see comment to line 895, below.
>    ...
>    836      -L  Use LVM to manage partitions
>    ...
>    839      -s  Use SWAPSIZE MB instead of $SWAP_SIZE MB for swap
> (Use 0 to disable swap)
> 
> It looks like, when usage() would normally be run, $SWAP_SIZE won't
> yet have been set?

You are right. I'll change that to "instead of autodetecting..."
> 
>    840      -v  Be more verbose about what is happening
>    841     __EOF__
> 
> 
> 
>    895     if [ $# -gt 0 ]; then
> 
> Mightn't it be useful to permit, and check whether, $* = "none"?

I think we wouldn't run setup-disk at all in that case.

>    896             # check that they are 
>    897             for i in "$@"; do
>    898                     j=$(readlink -f "$i" | sed 's:^/dev/::;
>    s:/:!:g')
>    899                     if ! [ -e "/sys/block/$j/device" ]; then
>    900                             echo "$i is not a suitable for
>    partitioning"
>    901                             exit 1
>    902                     fi
>    903                     diskdevs="$diskdevs /dev/$j"
>    904             done
>    905     else
>    906             ask_disk "Which disk(s) would you like to use? (or
>    '?' for help or 'none')" \
>    907                     diskselect_help $disks
>    908             if [ "$answer" != none ]; then
>    909                     for i in $answer; do
>    910                             diskdevs="$diskdevs /dev/$i"
>    911                     done
>    912             else
>    913                     DISK_MODE="none"
>    914             fi
>    915     fi
> 
> 


Thanks a lot for your review of my code and for very useful feedback :)

I have done most of the changes in git:

http://git.alpinelinux.org/cgit/alpine-conf/log/

Now it needs some testing. There have been various nasty corner cases
along the way.

-nc


---
Unsubscribe:  alpine-devel+unsubscribe@lists.alpinelinux.org
Help:         alpine-devel+help@lists.alpinelinux.org
---
Reply to thread Export thread (mbox)