[RFC] new initctl command

[James Hunt]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Surbhi,

A few more observations:

* init/pivot.h:
  - Out of interest when would MS_MOVE, etc not be defined? If there is
    a legitimate reason for including these redefs, I think we should
document why.
* init/pivot.c:
  - get_complete_path(): Needs asserts for dirname and dirent_name?
  - are_we_on_ramfs: compile error:
      pivot.c:186:17: error: comparison between signed and unsigned integer expressions
  - get_dir():
    - Use nih_dir_walk_scan() rather than the readdir() loop.
  - pivot_emit_failed_event(): Need to document return value.
  - is_rootfs_ok():
    - Needs assert for rootfs.
    - Return statement should start "Returns ...".
  - get_init_args():
    - Needs assert for "root" and "init".
    - Use nih_file_read() rather than fopen() and fgets().
    - "line" is hard-coded to 4k, but it should seemingly be set to the
      page size (sysconf(PAGE_SIZE) / sysconf(PAGESIZE) / getpagesize()).
    - Use PROC_CMDLINE rather than hard-coding string "/proc/cmdline".
    - Use nih_string_split() rather than strtok().
  - is_init_ok():
    - Needs assert for "init".
    - Return statement should start "Returns ...".
    - Calls to nih_dbus_error_raise_printf() could log name of requested
      init too?
  - del_dir(): Remove "end of while" comment.
  - move_mounts(): Missing space after "if" in "if(errno == ENOMEM)".
* init/events.h: PIVOT_FAILED_EVENT should be "pivot-failed" for
  consistency with other events (see upstart-events(7) on Ubuntu).
* util/initctl.c:
  - pivot_action(): If the "nih_assert (ret < 0)" fails, you will never
    get beyond that line, so the nih_error() will never be called.
  - Suggest you change...

    "INIT is the new init process"

    ... to:

    "INIT is the full path to the binary".


Regarding the compile error, can you make sure that you test by configuring like this:

./configure --disable-silent-rules --enable-compiler-warnings --disable-compiler-optimisations
- --disable-linker-optimisations ...  && make

I'll add this to the cookbook...

Regards,

James.


On 15/06/11 17:47, Scott James Remnant wrote:
> A few notes:
> 
>  - the D-Bus command needs to issue a method return to allow the initctl command to exit ;-)
> 
>  - I would say you should move /proc, /sys & /dev over simply because Upstart happens to use two of
> those, and it makes everyone's lives easier
> 
> 
> On Wed, Jun 15, 2011 at 8:06 AM, Surbhi Palande <surbhi.palande at canonical.com
> <mailto:surbhi.palande at canonical.com>> wrote:
> 
>     Hi Scott,
> 
>     I have uploaded a new diff for the new initctl "pivot" command at:
> 
>     https://code.launchpad.net/~csurbhi/upstart/upstart-add-pivot-handling
> 
>     Background:
>     --------------
>     The pivot command is used for changing the root filesystem in the initramfs from the memory
>     based "/" to the disk based real root filesystem. This command can be issued as follows:
> 
>     initctl pivot <ROOTFS> <INIT>
> 
>     where ROOTFS is the root filesystem that we want to move to while in the initramfs. INIT is the
>     first program that we wish to execute once this move to the real root filesystem is made.
> 
>     This command is intended to be used when upstart is executed in initramfs for making the
>     initramfs event driven.
> 
>     It is assumed that a user can specify a different ROOTFS, INIT or arguments to this new INIT at
>     the grub command prompt. The console used for logging the messages is /dev/console and is a not
>     a boot argument which can be changed.
> 
>     This command has no effect when it is executed from a non memory based root filesystem.
>     ---------------
> 
>     The uploaded diff has the following changes:
> 
>     1. Added the pivot related code in pivot.c, pivot.h.
>     2. Added support for compiling pivot.c
>     3. Made the code in pivot.c modular.
>     4. Added the handling of moving the virtual filesystem from / to the requested new rootfs.
>     5. The default console used is /dev/console. No other console can be specified. However, if we
>     want to make the console device a boot parameter or a command line argument for the pivot
>     command then this can be added. Would like to know views on this. Currently at least Ubuntu's
>     initramfs does not change the console when it executes run_init.
> 
>     I would also love to know any views on whether the pivot command is the right place to handle
>     the moving of the virtual filesystems (/dev, /proc, /sys to @rootfs/). Personally, I think that
>     this virtual filesystem movement is needed _only_ for the correct execution of the pivot command
>     and so it should be handled by the pivot command.
> 
>     Please do let me know your views on this diff. Thanks a lot!
> 
>     Warm Regards,
>     Surbhi.
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 

- --
James Hunt
____________________________________
Ubuntu Foundations Team, Canonical.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk352zAACgkQYBWEaHcQG9dhfwCfRUvF9yip1ztxKECQiOfUZO6b
TiMAmweAxkbfsjqoumGyHp3SuSyXllOt
=BFIB
-----END PGP SIGNATURE-----