[RFC] new initctl command

[Surbhi Palande]

Hi James,

Thanks a lot for the review and the suggestions.

I have incorporated most of the changes you suggested and pushed them to:
https://code.launchpad.net/~csurbhi/upstart/upstart-add-pivot-handling/

I have not made two changes - please see the following inline comments:
> 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.

nih_dir_walk_scan () is a static function in file.c. The non static 
function which calls nih_dir_walk_scan is nih_dir_walk (). AFAIU, this 
is not very suitable for deleting the contents of a directory, but 
rather for reading them. The implementation of nih_dir_walk() 
particularly uses "stat" rather than "lstat" and so deletion of links is 
difficult using this function.

>    - 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().

nih_file_read () uses stat () to find the size of the file and then uses 
this size to read the contents of the file. stat on /proc/cmdline 
indicates that its an empty file. Hence nih_file_read () returns an 
empty string and a size of 0 bytes on trying it with /proc/cmdline. 
Thus, this is not suitable for reading /proc/cmdline.


>      - "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"

I have not yet added the man page. However shall do that soon (once the 
code is accepted on ml)

I have also added the code for sending a reply back to initctl without 
returning to the dbus middle ware. I had misinterpreted the dbus 
interface previously, due to which I thought that a ret was needed apart 
from sending a reply.

Please do let me know any other changes that are necessary. As before, I 
have tested this in Ubuntu's initramfs on kvm. Thanks!

Warm Regards,
Surbhi.

>
>
> 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-----