From mboxrd@z Thu Jan  1 00:00:00 1970
From: Arnout Vandecappelle <arnout@mind.be>
Date: Wed, 21 Aug 2013 00:24:36 +0200
Subject: [Buildroot] [PATCH 1/1] new package - generate iso with
 isolinux bootloader
In-Reply-To: <1376644934-4302-1-git-send-email-jean.sorgemoel@laposte.net>
References: <y> <1376644934-4302-1-git-send-email-jean.sorgemoel@laposte.net>
Message-ID: <5213ECA4.9030405@mind.be>
List-Id: <buildroot.busybox.net>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: buildroot@busybox.net

On 16/08/13 11:22, jean wrote:
>
> Signed-off-by: jean <jean.sorgemoel@laposte.net>

  Hi Jean,

  Thanks for your patch. We will comment on it, and hopefully you can 
send an updated patch to fix the issues we report. When you do, please 
send the updated patch with the
--in-reply-to=1376644934-4302-1-git-send-email-jean.sorgemoel at laposte.net
and
-v 2 (or --subject-prefix='PATCH v2' for older git)
options, so that we can easily follow the updated patch in patchwork.

  First a generic comment. Is there any reason not to simply replace the 
iso9660 filesystem? That one is currently using grub as a bootloader, but 
it doesn't work very well. So I think this patch is very valuable to get 
the iso9660 filesystem working properly again. But as it is now, it is 
too complex to be included in buildroot. See my comments below.

  A second generic comment is about the choice of booting with an 
initramfs. Why not boot with a (rockridge) iso9660 rootfs? Clearly it 
puts a bit more strain on the kernel config since iso9660 as well as the 
bus drivers (sata, usb) have to be linked in, but I think that would be a 
much nicer solution. This type of image containing the actual rootfs in a 
different format should really be generated by a post-image script 
instead of a filesystem target. Can the rest of the list give their opinion?

> ---
>   fs/Config.in            |    1 +
>   fs/isolinux/Config.in   |  182 ++++++++++++++++++++++++++++++++++++++
>   fs/isolinux/isolinux.mk |  226 +++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 409 insertions(+)
>   create mode 100644 fs/isolinux/Config.in
>   create mode 100644 fs/isolinux/isolinux.mk
>
> diff --git a/fs/Config.in b/fs/Config.in
> index da4c5ff..02294a9 100644
> --- a/fs/Config.in
> +++ b/fs/Config.in
> @@ -11,5 +11,6 @@ source "fs/romfs/Config.in"
>   source "fs/squashfs/Config.in"
>   source "fs/tar/Config.in"
>   source "fs/ubifs/Config.in"
> +source "fs/isolinux/Config.in"
>
>   endmenu
> diff --git a/fs/isolinux/Config.in b/fs/isolinux/Config.in
> new file mode 100644
> index 0000000..8abe409
> --- /dev/null
> +++ b/fs/isolinux/Config.in
> @@ -0,0 +1,182 @@
> +## Menu ISO image with syslinux

  This comment is redundant: the line below says exactly that.

> +menu "iso image (isolinux bootloader - with initramfs)"

  We normally don't have a menu in this case. The sub-options will be 
hidden unless the ISOLINUX option is selected, and normally you only have 
one filesystem so the total menu doesn't become too large.

  In some cases we do add a menuconfig (which is a combination of a menu 
and a boolean config), but I don't think it is warranted here.

> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX

  The name of the config symbols should correspond to the directory, i.e. 
BR2_TARGET_ROOTFS_ISOLINUX (or BR2_TARGET_ROOTFS_ISO9660 if you do decide 
to replace that one). And of course the suboptions should also be renamed 
to BR2_TARGET_ROOTFS_ISOLINUX_FOO.

> +	bool "iso image (isolinux bootloader)"
> +	depends on (BR2_i386 || BR2_x86_64)
> +	depends on BR2_LINUX_KERNEL

  If you have a dependency on the kernel, you should also add a comment 
at the file explaining that it is needed. Cfr. iso9660.

> +	select BR2_TARGET_ROOTFS_INITRAMFS
> +	select BR2_TARGET_SYSLINUX
> +	select BR2_TARGET_SYSLINUX_ISOLINUX
> +	help
> +		Build a bootable iso9660 image (with isolinux bootloader and initramfs)

  I think it would be nice if the bootable aspect would be optional. But 
that can be done in a follow-up patch.

  Help text should be indented with 1 tab + 2 spaces, and wrap at 70 
characters (= 80 columns if you have 8-char tabs).

> +		You can launch this iso with :
> +			qemu -boot d -cdrom rootfs.isolinux
> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_JOLIET
> +        bool "create iso with Joliet format"
> +        depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX

  Instead of defining a 'depends on' for every suboption, just wrap the 
whole thing in a 'if ... endif' construct.

> +        help
> +              	Create iso image with Joliet format (long file)

  long filenames

> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_ROCK_RIDGE
> +        bool "Generate Rock Ridge directory information"
> +        depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> +        help
> +                Generate Rock Ridge directory information

  That help text isn't very useful... How about 'Generate Rock Ridge 
directory information, which includes symbolic links, file mode, uid, 
gid, etc.'

> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_MENU_LINUX
> +        string "Name to start linux"

  All Config.in stuff (except help text) should be indented with 1 tab, 
not with spaces.

> +        depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> +        default "buildroot"
> +        help
> +                define message see to select linux

  I don't think generating the boot menu is a good idea. Instead, add an 
option to let the user point to the menu file, like iso9660 does it. Or 
actually, a list of files to include in the root dir.

  There should be a default 'menu' that boots immediately. You can just 
put this menu in fs/isolinux, and set the default for this option to that 
path.

> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_NAME
> +        string "Name iso"

  "Volume label" or "Volume ID"

  And maybe the config symbol should be named the same.

> +        depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> +        default "buildroot iso"

  I think the default label should be just "buildroot".

> +        help
> +                cdrom name
> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_INPUT_CHARSET
> +        string "define parameter input charset"
> +        depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> +	default "iso8859-15"
> +        help
> +                define parameter input charset
> +		(see program genisoimage : genisoimage -input-charset help)
> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_BOOT_MESSAGE
> +	string "Message boot for isolinux"

  I guess this one is part of the menu file so can be removed.

> +	depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> +	default "Buildroot isolinux boot"
> +	help
> +		define first message see in prompt boot
> +		after that, you see all options
> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_BOOT_TIMEOUT
> +	int "timeout"

  Ditto.

> +	depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> +        default "20"
> +	help
> +		define timeout (second)
> +		after this time, boot on default config
> +
> +config BR2_TARGET_ISO_ISOLINUX_TOOLS_HARDWARE_INFO
> +	bool "add tools hardware info"

  Ditto.

> +        depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> +        help
> +		isolinux menu, you can see hardware info
> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_TOOLS_REBOOT
> +        bool "add isolinux option reboot"

  Ditto.

> +        depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> +        help
> +                isolinux menu, you can reboot
> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_TOOLS_POWEROFF
> +        bool "add isolinux option poweroff"

  Ditto.

> +        depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> +        help
> +                isolinux menu, you can power off
> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_TOOLS_FIRSTDISKBOOT
> +        bool "add isolinux option to boot on first disk"

  Ditto.

> +        depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> +        help
> +                isolinux menu, you can boot on disk
> +
> +## Keyboard menu
> +menu "keyboard"

  This whole keyboard thing is way too complex. Buildroot wants to be 
_simple_. The only relevant thing would be a space-separate list of 
keyboard layouts to include in the image (cfr. the manual option, but 
with the possibility to specify several keyboards). However, I think for 
the time being it is best to only support a single keyboard (US).

[snip the 100 lines for the keyboard definition]

> diff --git a/fs/isolinux/isolinux.mk b/fs/isolinux/isolinux.mk
> new file mode 100644
> index 0000000..0d9464d
> --- /dev/null
> +++ b/fs/isolinux/isolinux.mk
> @@ -0,0 +1,226 @@
> +################################################################################
> +#
> +# Build the iso96600 with isolinux bootloader (and initramfs)
> +#
> +################################################################################
> +
> +BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_BOOT=boot

  Variables defined in a .mk file should never start with BR2_. The BR2_ 
prefix indicates that it is a Kconfig variable. Also the rest of the 
prefixes can be removed, so you would simply have ISOLINUX_ROOT_DIR_BOOT

  Assignments in .mk files should have a space before and after the = 
(like you do below).

  This variable and the three below, however, are redundant. They are not 
an abbreviation, and they are not variable. So they're not useful, and 
they make the file harder to read.

> +BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_ISOLINUX=isolinux
> +BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_TOOLS=tools
> +BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_KEYBOARD=keyboard
> +
> +BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR = $(BUILD_DIR)/isolinux

  This one could still be useful, since we have a similar variable for 
the generic packages. However, it should then be called ISOLINUX_DIR.

> +BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_BOOT = $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)/$(BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_BOOT)
> +BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_ISOLINUX = $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)/$(BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_ISOLINUX)
> +BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_TOOLS = $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)/$(BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_TOOLS)
> +BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_KEYBOARD = $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)/$(BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_KEYBOARD)

  These four are redundant again.

> +
> +BR2_TARGET_ISO_ISOLINUX_BOOT_MESSAGE := $(call qstrip,$(BR2_TARGET_ROOTFS_ISO_ISOLINUX_BOOT_MESSAGE))
> +BR2_TARGET_ISO_ISOLINUX_BOOT_TIMEOUT := $(call qstrip,$(BR2_TARGET_ROOTFS_ISO_ISOLINUX_BOOT_TIMEOUT))

  Redundant.

> +
> +BR2_TARGET_ISO_ISOLINUX_KEYBOARD_US = $(KBD_BUILDDIR)data/keymaps/i386/qwerty/us.map

  Redundant.

  Also, refering to KBD_BUILDDIR is not very nice. As I said before, I 
would leave the whole keyboard handling thing out for the first 
submission, and maybe add it in a follow-up patch.

> +
> +BR2_TARGET_ISO_ISOLINUX_CONFIG = ""
> +BR2_TARGET_ISO_BR2_TARGET_ISO_ISOLINUX_BOOT_MSG = ""
> +
> +define copy_files_isolinux_image_tools
> +	cp $(1) $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_TOOLS);
> +endef

  Redundant


> +
> +define copy_files_isolinux_image_keyboard
> +	$(SYSLINUX_BUILDDIR)utils/keytab-lilo \
> +	        $(BR2_TARGET_ISO_ISOLINUX_KEYBOARD_US) \
> +        	$1 \
> +	         > $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_KEYBOARD)/$(subst -,_,$(notdir $(basename $1))).ktl;
> +endef
> +
> +$(BINARIES_DIR)/rootfs.isolinux: host-cdrkit linux syslinux rootfs-initramfs

  Dependency on linux is redundant, since it is implied by the initramfs.

  You should probably collect the dependencies in a ISOLINUX_DEPENDENCIES 
variable, for consistency with the generic infrastructure.


> +	@$(call MESSAGE,"Generating root filesystem image rootfs.isolinux")
> +	@mkdir -p $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)
> +	@mkdir -p $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_BOOT)
> +	@mkdir -p $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_ISOLINUX)
> +	@mkdir -p $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_TOOLS)
> +	@mkdir -p $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_KEYBOARD)
> +
> +	@echo -e $(BR2_TARGET_ISO_BR2_TARGET_ISO_ISOLINUX_BOOT_MSG) > $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)/bootmsg.txt
> +
> +	@cp $(BINARIES_DIR)/isolinux.bin $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)/$(BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_ISOLINUX)/

  We use '$(INSTALL) -D' instead of cp, which also makes the mkdir's 
above redundant. Remember, however, that the target file name has to be 
given if the -D option is used.

> +	@cp $(LINUX_IMAGE_PATH) $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)/$(BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_BOOT)/
> +
> +	@echo -e $(BR2_TARGET_ISO_ISOLINUX_CONFIG) > $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)/isolinux.cfg
> +	
> +	@$(foreach file, $(BR2_TARGET_ISO_ISOLINUX_LIST_TOOLS), $(call copy_files_isolinux_image_tools, $(file) ) )
> +	@$(foreach file, $(BR2_TARGET_ISO_ISOLINUX_LIST_KEYBOARD), $(call copy_files_isolinux_image_keyboard, $(file) ) )
> +
> +	$(HOST_DIR)/usr/bin/genisoimage \
> +		$(BR2_TARGET_ISO_ISOLINUX_GENISOIMAGE_OPTION) \
> +		-o $(BINARIES_DIR)/rootfs.isolinux \

  This should be $@

> +		-b $(BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_ISOLINUX)/isolinux.bin \
> +		-no-emul-boot \
> +		-c $(BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_ISOLINUX)/boot.cat \
> +		-boot-load-size 4 \
> +		-boot-info-table \
> +		-input-charset $(BR2_TARGET_ROOTFS_ISO_ISOLINUX_INPUT_CHARSET) \
> +		-V $(BR2_TARGET_ROOTFS_ISO_ISOLINUX_NAME) \
> +		$(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)
> +
> +	- at rm -rf $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)
> +
> +rootfs-isolinux: $(BINARIES_DIR)/rootfs.isolinux

  I won't comment on the rest anymore since it will change too much after 
my feedback.

  I hope you're not scared off by the big change requests...

  Regards,
  Arnout

[snip]

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F