Indeed, there was no way for this change to be 100% backwards compatible, except the one you just described. Breaking tests.py was admittedly an omission on my side to test more thoroughly.
What you suggest (change the loader's behavior) should be the option with the least effort involved and probably the best guarantee for not breaking any script. My only worry is adding unnecessary complexity to the loader. The only other solution I see is adding a --rootfs flag to run.py (useful only for overriding, e.g. when using the -e flag to run.py to override the command line) and utilizing that in the other scripts. Nevertheless, this still leaves the possibility of breaking some other use case, while it's also not a particularly robust (or elegant) solution in my eyes. Your suggestion seems to be the safest option, but if we think the other one is worth a shot, I am also willing to implement that. What do you (or anyone else who wants to chime in) think? Fotis Στις Παρασκευή, 28 Αυγούστου 2020 στις 6:05:08 π.μ. UTC+2, ο χρήστης [email protected] έγραψε: > So I played more and I think I answered my question myself. The default > new behavior (no --rootfs passed) is not backward compatible. For example > './scripts/build check fs=rofs' fails because test.py does not get/pass the > proper value of --rootfs to run.py. I wonder if we have other cases/scripts > when it breaks like that. > > So I wonder if we tweak the 3rd patch in your series like so: > - make --rootfs optional without default value of 'zfs' > - if rootfs not specified like with test.py do old behavior - try rofs and > then zfs > > I like ability to specify explicit root filesystem by I think we need to > keep default behavior if a new parameter not specified. What do you think? > > Waldek > > On Thu, Aug 27, 2020 at 1:05 AM Waldek Kozaczuk <[email protected]> > wrote: > >> I think all your patches look good (though I have not had a chance to >> test it yet). >> >> I see you add new option '--rootfs' to explicitly tell which filesystem >> to use. Now can you clarify if your changes to the mounting logic are >> backward compatible? >> >> I other words when I do not specify '--rootfs', will it try to mounts >> stuff in the same order it used to before your changes? I wonder if that >> may break any existing scripts. >> >> Waldek >> >> On Wednesday, August 19, 2020 at 5:43:05 AM UTC-4 Fotis Xenakis wrote: >> >>> Signed-off-by: Fotis Xenakis <[email protected]> >>> --- >>> fs/vfs/main.cc | 52 +++++++++++++++++++++++++++++--------------------- >>> loader.cc | 20 +++++++++---------- >>> 2 files changed, 40 insertions(+), 32 deletions(-) >>> >>> diff --git a/fs/vfs/main.cc b/fs/vfs/main.cc >>> index 3c8b327b..6cee319e 100644 >>> --- a/fs/vfs/main.cc >>> +++ b/fs/vfs/main.cc >>> @@ -1593,7 +1593,7 @@ int faccessat(int dirfd, const char *pathname, int >>> mode, int flags) >>> return error; >>> } >>> >>> -extern "C" >>> +extern "C" >>> int euidaccess(const char *pathname, int mode) >>> { >>> return access(pathname, mode); >>> @@ -2375,45 +2375,53 @@ extern "C" void unmount_devfs() >>> >>> extern "C" int mount_rofs_rootfs(bool pivot_root) >>> { >>> - int ret; >>> - >>> - if (mkdir("/rofs", 0755) < 0) >>> - kprintf("failed to create /rofs, error = %s\n", strerror(errno)); >>> + constexpr char* mp = "/rofs"; >>> >>> - ret = sys_mount("/dev/vblk0.1", "/rofs", "rofs", MNT_RDONLY, 0); >>> + if (mkdir(mp, 0755) < 0) { >>> + int ret = errno; >>> + kprintf("failed to create %s, error = %s\n", mp, strerror(errno)); >>> + return ret; >>> + } >>> >>> + int ret = sys_mount("/dev/vblk0.1", mp, "rofs", MNT_RDONLY, nullptr); >>> if (ret) { >>> - kprintf("failed to mount /rofs, error = %s\n", strerror(ret)); >>> - rmdir("/rofs"); >>> + kprintf("failed to mount %s, error = %s\n", mp, strerror(ret)); >>> + rmdir(mp); >>> return ret; >>> } >>> >>> if (pivot_root) { >>> - pivot_rootfs("/rofs"); >>> + pivot_rootfs(mp); >>> } >>> >>> return 0; >>> } >>> >>> -extern "C" void mount_zfs_rootfs(bool pivot_root, bool extra_zfs_pools) >>> +extern "C" int mount_zfs_rootfs(bool pivot_root, bool extra_zfs_pools) >>> { >>> - if (mkdir("/zfs", 0755) < 0) >>> - kprintf("failed to create /zfs, error = %s\n", strerror(errno)); >>> + constexpr char* mp = "/zfs"; >>> >>> - int ret = sys_mount("/dev/vblk0.1", "/zfs", "zfs", 0, (void >>> *)"osv/zfs"); >>> - >>> - if (ret) >>> - kprintf("failed to mount /zfs, error = %s\n", strerror(ret)); >>> - >>> - if (!pivot_root) { >>> - return; >>> + if (mkdir(mp, 0755) < 0) { >>> + int ret = errno; >>> + kprintf("failed to create %s, error = %s\n", mp, strerror(errno)); >>> + return ret; >>> } >>> >>> - pivot_rootfs("/zfs"); >>> + int ret = sys_mount("/dev/vblk0.1", mp, "zfs", 0, (void *)"osv/zfs"); >>> + if (ret) { >>> + kprintf("failed to mount %s, error = %s\n", mp, strerror(ret)); >>> + rmdir(mp); >>> + return ret; >>> + } >>> >>> - if (extra_zfs_pools) { >>> - import_extra_zfs_pools(); >>> + if (pivot_root) { >>> + pivot_rootfs(mp); >>> + if (extra_zfs_pools) { >>> + import_extra_zfs_pools(); >>> + } >>> } >>> + >>> + return 0; >>> } >>> >>> extern "C" void unmount_rootfs(void) >>> diff --git a/loader.cc b/loader.cc >>> index 66bfb52c..9d9d3173 100644 >>> --- a/loader.cc >>> +++ b/loader.cc >>> @@ -87,7 +87,7 @@ extern "C" { >>> void premain(); >>> void vfs_init(void); >>> void unmount_devfs(); >>> - void mount_zfs_rootfs(bool,bool); >>> + int mount_zfs_rootfs(bool, bool); >>> int mount_rofs_rootfs(bool); >>> void rofs_disable_cache(); >>> } >>> @@ -396,19 +396,20 @@ void* do_main_thread(void *_main_args) >>> >>> if (opt_mount) { >>> unmount_devfs(); >>> - // >>> + >>> // Try to mount rofs >>> - if(mount_rofs_rootfs(opt_pivot) != 0) { >>> - // >>> + if (mount_rofs_rootfs(opt_pivot) != 0) { >>> // Failed -> try to mount zfs >>> zfsdev::zfsdev_init(); >>> - mount_zfs_rootfs(opt_pivot, opt_extra_zfs_pools); >>> + auto error = mount_zfs_rootfs(opt_pivot, opt_extra_zfs_pools); >>> + if (error) { >>> + debug("Could not mount zfs root filesystem.\n"); >>> + } >>> bsd_shrinker_init(); >>> >>> boot_time.event("ZFS mounted"); >>> - } >>> - else { >>> - if(opt_disable_rofs_cache) { >>> + } else { >>> + if (opt_disable_rofs_cache) { >>> debug("Disabling ROFS memory cache.\n"); >>> rofs_disable_cache(); >>> } >>> @@ -491,8 +492,7 @@ void* do_main_thread(void *_main_args) >>> >>> if (opt_bootchart) { >>> boot_time.print_chart(); >>> - } >>> - else { >>> + } else { >>> boot_time.print_total_time(); >>> } >>> >>> -- >>> 2.28.0 >>> >>> -- >> You received this message because you are subscribed to the Google Groups >> "OSv Development" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to [email protected]. >> > To view this discussion on the web visit >> https://groups.google.com/d/msgid/osv-dev/c64592fe-7f80-4661-8e25-a10d51ce44f6n%40googlegroups.com >> >> <https://groups.google.com/d/msgid/osv-dev/c64592fe-7f80-4661-8e25-a10d51ce44f6n%40googlegroups.com?utm_medium=email&utm_source=footer> >> . >> > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/328c804d-989d-47c3-8bfb-67e475e999d5n%40googlegroups.com.
