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<[email protected]
<mailto:[email protected]>> 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-----
--
upstart-devel mailing list
[email protected]
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/upstart-devel
