diff --git a/Makefile b/Makefile index 9ac77ce..f8125dd 100644 --- a/Makefile +++ b/Makefile @@ -505,14 +505,54 @@ UEFI_CFLAGS = --target=x86_64-unknown-windows -ffreestanding -fshort-wchar # nothing to do, and the only symptom was a boot screen that disagreed with the # kernel about what the logo is. The kernel half never showed it because those # objects are built with -MD -MP and pick up header changes on their own. +# +# **The loader is the one thing here that GCC cannot build.** It is a PE/COFF +# binary for the Microsoft ABI, which clang cross-targets and gcc does not, so +# this rule needs clang and lld-link and no other rule does. +# +# That matters more than it reads, because of *where* it sits: `make` stops at +# the first failed rule, and this one runs before the sysroot is staged and the +# filesystem image is built. So on a machine without clang, touching the loader +# does not produce "the loader did not build" -- it produces a new kernel, an +# old userland, and a boot that behaves like a mix of the two, with the real +# cause forty lines up the screen. That happened, and it cost an afternoon of +# looking at the wrong thing. +# +# Hence the check and the message. It is worth the four lines. $(BUILD)/uefi/BOOTX64.EFI: boot/uefi/main.c boot/uefi/efi.h include/schboot.h \ boot/common/ui.c boot/common/ui.h \ include/logo.h include/theme.h include/font.h @mkdir -p $(BUILD)/uefi + @command -v clang >/dev/null 2>&1 || { \ + echo ""; \ + echo " The UEFI loader needs clang and lld-link, and nothing else here does."; \ + echo " It is the only PE/COFF binary in the tree -- gcc cannot target it."; \ + echo ""; \ + echo " On NixOS: nix-shell --run 'NIX_HARDENING_ENABLE=\"\" make'"; \ + echo " Elsewhere: install clang and lld, then make again"; \ + echo ""; \ + test -f $@ && { \ + echo " There is an existing loader at $@."; \ + echo " To keep it and build everything else: make UEFI_KEEP=1"; \ + echo ""; }; \ + exit 1; } clang $(UEFI_CFLAGS) -c boot/uefi/main.c -o $(BUILD)/uefi/main.obj clang $(UEFI_CFLAGS) -c boot/common/ui.c -o $(BUILD)/uefi/ui.obj lld-link -subsystem:efi_application -entry:EfiMain -nodefaultlib $(BUILD)/uefi/main.obj $(BUILD)/uefi/ui.obj -out:$@ +# Keep the loader that is already built and carry on with the rest. +# +# For the case the message above describes: a machine with no clang, where the +# loader has not meaningfully changed and the thing being worked on is the +# kernel or the userland. It is opt-in and prints what it is doing, because a +# build that quietly ships a stale bootloader is how you spend an evening +# debugging code that is not running. +ifeq ($(UEFI_KEEP),1) +$(BUILD)/uefi/BOOTX64.EFI: + @test -f $@ || { echo "make UEFI_KEEP=1: there is no loader to keep"; exit 1; } + @echo "uefi: keeping the existing loader (UEFI_KEEP=1) -- it may be out of date" +endif + uefi: $(BUILD)/uefi/BOOTX64.EFI # The EFI System Partition, as a directory. QEMU serves a directory as FAT diff --git a/cmd/schinitd/main.c b/cmd/schinitd/main.c index fb7db0b..f19401e 100644 --- a/cmd/schinitd/main.c +++ b/cmd/schinitd/main.c @@ -507,8 +507,11 @@ note_exit(unsigned int cookie, int pid, int status) // job that was killed or faulted never matches this and is restarted the // way KeepAlive promises. See SCHINIT_EXIT_NORESTART. if ((status & 0x7f) == 0 && ((status >> 8) & 0xff) == SCHINIT_EXIT_NORESTART) { - out("schinitd: "); out(j->label); - out(" has nothing to do on this machine; not restarting\n"); + // Not a failure: the job looked, found the machine did not have what + // it drives, and said so properly. Worth reading once and not every + // boot on a machine that will never have one. + vout("schinitd: "); vout(j->label); + vout(" has nothing to do on this machine; not restarting\n"); return; } @@ -640,6 +643,17 @@ main(void) vout("schinitd: ready\n"); + // The one line a working boot prints, and the last one. + // + // Here rather than in the kernel because "the machine is up" is not true + // until the jobs that make it usable have been started, and because + // anything printed before this point would come after it on screen. The + // kernel's whole boot is a kinfo() and is in the klog; this is the part a + // person is meant to read. + if (!verbose_boot) + out("\nWelcome to SchOS!" + " (F8 at boot for the full log, or dmesg once you are in)\n\n"); + // From here on this program does two things: answer PID 1's heartbeat, and // act on the exits it reports. There is no wait() in this program and // there cannot be -- the children are not ours. diff --git a/cmd/vgpud/main.c b/cmd/vgpud/main.c index b4e2752..9b9c1ce 100644 --- a/cmd/vgpud/main.c +++ b/cmd/vgpud/main.c @@ -110,7 +110,11 @@ die(const char *why) static void nothing_to_do(const char *why) { - puts("vgpud: "); puts(why); puts("\n"); + // Quiet on a quiet boot: "this machine has no virtio-gpu" is a fact about + // the hardware and not a fault, and it is the same fact every time. It is + // still an exit the supervisor is told about, which is what stops it being + // restarted fifteen times in forty-five seconds. + vputs("vgpud: "); vputs(why); vputs("\n"); exit_group(SCHINIT_EXIT_NORESTART); } diff --git a/docs/architecture/devices.md b/docs/architecture/devices.md index 3adb72f..d82513c 100644 --- a/docs/architecture/devices.md +++ b/docs/architecture/devices.md @@ -250,6 +250,26 @@ existing mechanisms for keeping that true mechanically rather than by intention. --- +## `/dev/random` and `/dev/urandom` + +Seeded once at boot, from `RDRAND` where the CPU has it and from the cycle +counter mixed with the wall clock where it does not. A verbose boot says which: + +``` +random: seeded from RDRAND +``` + +That is worth a section only because of what it replaced. It was a linear +congruential generator seeded with a **constant**, so every boot produced the +same bytes in the same order — not weak, *the same*. A program asking for a +key, a nonce, a temporary filename or a hash seed got one it shared with every +other SchOS machine that had ever run. + +**It is still not a CSPRNG and does not pretend to be.** It is an LCG, re-stirred +from the cycle counter on each byte so two readers do not continue one +sequence. What changed is the property that made it dangerous rather than +merely poor. + ## What it does not do - **It is a flat list, not a device tree.** There is no parent/child nesting, diff --git a/docs/architecture/filesystem.md b/docs/architecture/filesystem.md index 59328f3..2c32490 100644 --- a/docs/architecture/filesystem.md +++ b/docs/architecture/filesystem.md @@ -189,8 +189,50 @@ selftest, `t` text specimen, `n` the netdev ARP selftest. --- +## Errno, and why `return -1` is a bug + +A syscall that fails returns a negative errno. `-1` is therefore **`-EPERM`**, +"operation not permitted" — and about ninety error paths in `kernel/sys/sys_fs.c` +still say `return -1` because that is what xv6 did, where the convention was +just "negative means no". + +It is not a cosmetic difference. Callers branch on `errno`, and the standard +shape for removing a file you are not sure about is: + +```c +if (unlink(p) < 0 && errno != ENOENT) + complain(); +``` + +Against `-1` every such test takes the complaining branch. That is how it was +found — zsh, saving its history, reported failures for things that had +succeeded: + +``` +zsh: locking failed for /root/.zsh_history: operation not permitted +zsh: failed to write history file /root/.zsh_history.new: operation not permitted +``` + +`link`, `unlink`, `rename` and `renameat` return real values now — `ENOENT`, +`EEXIST`, `EXDEV`, `EISDIR`, `ENOTEMPTY`, `EFAULT`. **The rest of the file does +not**, and that is the honest state: these four were corrected because these +four were the ones a program tripped over. + ## What it does not do +- **`rename()` will not replace an existing target.** POSIX says it must, and + write-to-a-temporary-then-rename is how every careful program saves a file — + so the second save of anything fails with `EEXIST` where it should succeed. + It is the reason `zsh: can't rename /root/.zsh_history.new to $HISTFILE` + appears on every shell exit after the first. + + An attempt to fix it by unlinking the target first was **reverted**: the + directory is an HTree, `dirunlink` followed by `dirlink` of the same name did + not behave, and the failure mode was the worst possible one — the old file + removed and the new link then refused, losing both. Doing this properly means + understanding the HTree's slot reuse, not patching around it. + + - **No page cache.** `mmap` of a file reads the whole thing in at map time, and writable `MAP_SHARED` of a file is refused rather than faked. This is the single largest gap in the layer. diff --git a/docs/architecture/terminals.md b/docs/architecture/terminals.md index 155f1ac..c3f48f6 100644 --- a/docs/architecture/terminals.md +++ b/docs/architecture/terminals.md @@ -238,6 +238,29 @@ immediate wrap, a reverse-video `#` appeared above every prompt. Anything that moves the cursor deliberately -- CR, LF, backspace, an escape sequence -- clears the debt. +## Ctrl-C used to kill the shell + +Worth its own note, because the cause was one character and the effect was a +machine you could not use. + +When Ctrl-C arrives, `consoleread()` discards the partial line and breaks off +the read in progress. It returned **`-1`** to do so — and `-1` is `-EPERM`. So a +program that handles `SIGINT` by carrying on saw a permanent error instead of +an interrupted call, and exited: + +``` +zsh: error on TTY read: operation not permitted +schinitd: restarting os.schos.console-shell +``` + +and then the next Ctrl-C did the same thing to the replacement. A shell that +cannot survive Ctrl-C is a shell nobody can use, and the supervisor faithfully +restarting it made it a loop. + +It returns **`EINTR`** now, which is what a signal-interrupted read is required +to give: the whole convention — retry, or return to the caller — is built on +receiving that and not something else. + ## What it does not do - **No scrollback.** The grid is exactly the visible screen. Shift+PageUp does diff --git a/include/defs.h b/include/defs.h index a09cfd3..44d4f24 100644 --- a/include/defs.h +++ b/include/defs.h @@ -404,6 +404,8 @@ int ksym_fmt(char *buf, int cap, uint64 addr); void *dma_alloc(uint64 size, uint64 align); void dma_free(void*, uint64 size); addr_t dma_phys(const void*); +// /dev/random and /dev/urandom. Seeded once at boot; see kernel/dev/cdev.c. +void random_init(void); uint64 dma_free_bytes(void); uint64 dma_total_bytes(void); diff --git a/kernel/core/console.c b/kernel/core/console.c index 84ae380..62e6ac5 100644 --- a/kernel/core/console.c +++ b/kernel/core/console.c @@ -10,6 +10,7 @@ #include "irq.h" #include "param.h" #include "traps.h" +#include "errno.h" #include "spinlock.h" #include "sleeplock.h" #include "fs.h" @@ -611,11 +612,27 @@ consoleread(struct inode *ip, uint64 off, char *dst, int n) ilock(ip); return 0; // end of input } + // Interrupted, not forbidden. + // + // This returned -1, and -1 is -EPERM. A read broken off by Ctrl-C + // therefore reached userspace as "operation not permitted", which is a + // permanent error -- so every program that handles SIGINT by carrying on + // saw a fatal one instead and exited: + // + // zsh: error on TTY read: operation not permitted + // schinitd: restarting os.schos.console-shell + // + // and then did it again, because the next Ctrl-C did the same thing. A + // shell that cannot survive Ctrl-C is a shell nobody can use. + // + // EINTR is the answer a signal-interrupted read is required to give, and + // the whole convention -- retry, or return to the caller -- is built on + // getting it rather than something else. if (myproc()->killed || v->in.abort_read) { v->in.abort_read = 0; release(&input_lock); ilock(ip); - return -1; + return -EINTR; } sleep(&v->in.r, &input_lock); } diff --git a/kernel/core/main.c b/kernel/core/main.c index 2d7935b..5e22922 100644 --- a/kernel/core/main.c +++ b/kernel/core/main.c @@ -221,22 +221,22 @@ kmain(uint32 fb_addr) // sinit: kthread_create needs a usable process table. From here on Ctrl-G // on the console answers, with no filesystem mounted and nothing in // userspace started -- which is the whole point of putting it this early. + // After timeinit(), which is what boot_epoch comes from, and before any + // program can ask for a random byte. + random_init(); netuser_init(); dbg_thread_start(); if (boot_verbose) cprintf("verbose boot: dbg thread started\n"); - // The one line a working boot prints. + // The greeting is not printed here. // - // Everything above this is a kinfo(), which means it is in the klog and not - // on the screen -- the whole boot is still there, it is simply not the first - // thing anybody has to read every morning. What is left is the fact that the - // machine came up, and where the rest went if it did not. + // It was, and it landed before userspace had said anything -- so on a quiet + // boot it was followed by the last few lines of `schinit` and whatever the + // jobs had to say, which is the wrong way round for a line whose whole job is + // to be the last thing you read. It belongs at the end of the thing it is + // announcing, and the end of the thing it is announcing is in userspace. // - // A verbose boot has already said thirty other things and does not need a - // greeting on top of them. - if (!boot_verbose) - cprintf("\nWelcome to SchOS! (F8 at boot for the full log, or dmesg once you are in)\n\n"); - + // schinitd prints it, once its RunAtLoad jobs are away. See cmd/schinitd. mpmain(); } diff --git a/kernel/dev/cdev.c b/kernel/dev/cdev.c index 9108ba8..a557b4b 100644 --- a/kernel/dev/cdev.c +++ b/kernel/dev/cdev.c @@ -12,7 +12,84 @@ extern int consoleread(struct inode*, uint64, char*, int); extern int consolewrite(struct inode*, uint64, char*, int); extern int mouseread(struct inode*, uint64, char*, int); -static uint64 rseed = 0x2A99554433221100ULL; +// --- /dev/random and /dev/urandom ------------------------------------------- +// +// This was a linear congruential generator seeded with a constant, which means +// **every boot produced the same bytes in the same order**. Not weak: the same. +// A program asking for a key, a nonce, a temporary filename or a hash seed got +// one it shared with every other SchOS machine that had ever run. +// +// What is here now is still not a CSPRNG and does not pretend to be. It is an +// LCG that is *seeded* from something real and re-stirred from the cycle +// counter on every read, which fixes the one property that made the old one +// dangerous rather than merely poor. See "What it does not do" in +// docs/architecture/devices.md. +static uint64 rseed; +static int rseed_ready; +static void rseed_init(void); + +// RDRAND, if this CPU has it. CPUID leaf 1, ECX bit 30. +// +// Checked rather than assumed: the instruction faults with #UD on a CPU that +// does not implement it, and "the random device is a kernel panic" is a worse +// failure than a weak seed. +static int +rdrand64(uint64 *out) +{ + uint32 a, b, c, d; + unsigned char ok = 0; + uint64 v = 0; + + cpuid_leaf(1, &a, &b, &c, &d); + if (!(c & (1u << 30))) + return 0; + + // Ten tries, per Intel's own guidance: the instruction is allowed to fail + // when its entropy pool is momentarily empty, and it says so in CF. + for (int i = 0; i < 10; i++) { + asm volatile("rdrand %0; setc %1" : "=r"(v), "=qm"(ok) :: "cc"); + if (ok) { *out = v; return 1; } + } + return 0; +} + +// Seeded at boot rather than on the first read. +// +// Lazy would work, but it puts the one line that says where the entropy came +// from in the middle of whatever the machine was doing hours later, and it +// makes the first reader pay for the CPUID. Called from main(); the lazy path +// stays as a belt-and-braces for anything that reads before init runs. +void +random_init(void) +{ + if (!rseed_ready) + rseed_init(); +} + +static void +rseed_init(void) +{ + uint64 v; + + rseed_ready = 1; + if (rdrand64(&v)) { + rseed = v; + kinfo("random: seeded from RDRAND\n"); + return; + } + + // No RDRAND. The cycle counter is the only thing here that is different on + // every boot and not written down anywhere -- it is not entropy in any + // serious sense, but it is not a constant either, which was the whole + // problem. Mixed with the wall clock so that two machines starting at the + // same instant do not agree. + rseed = rdtsc(); + rseed ^= boot_epoch * 0x9E3779B97F4A7C15ULL; + rseed ^= (uint64)(uintptr_t)&rseed; + if (rseed == 0) + rseed = 0x9E3779B97F4A7C15ULL; + kinfo("random: no RDRAND; seeded from the cycle counter and the clock\n"); +} int nullwrite(struct inode *ip, uint64 off, char *dst, int n) @@ -36,9 +113,16 @@ zeroread(struct inode *ip, uint64 off, char *dst, int n) int randomread(struct inode *ip, uint64 off, char *dst, int n) { + if(!rseed_ready) + rseed_init(); + for(int i = 0; i < n; i++){ + // Re-stirred from the cycle counter every byte, so two reads a moment + // apart do not continue one sequence -- and so that a process cannot + // predict the next caller's bytes from its own. + rseed ^= rdtsc(); rseed = rseed * 6364136223846793005ULL + 1442695040888963407ULL; - dst[i] = (rseed >> 32) & 0xFF; + dst[i] = (rseed >> 33) & 0xFF; } return n; } @@ -72,7 +156,9 @@ memread(struct inode *ip, uint64 off, char *dst, int n) case 7: return zeroread(ip, off, dst, n); // /dev/full reads as zeros case 8: case 9: return randomread(ip, off, dst, n); - default: return -1; + // Not "operation not permitted", which is what -1 means once it reaches + // userspace. There is no device on this minor. + default: return -ENODEV; } } @@ -88,7 +174,7 @@ memwrite(struct inode *ip, uint64 off, char *dst, int n) case 5: case 8: case 9: return nullwrite(ip, off, dst, n); - default: return -1; + default: return -ENODEV; } } diff --git a/kernel/sys/sys_fs.c b/kernel/sys/sys_fs.c index 13da263..bea244b 100644 --- a/kernel/sys/sys_fs.c +++ b/kernel/sys/sys_fs.c @@ -686,45 +686,76 @@ addr_t sys_mknod(void) return 0; } +// link() and rename() answer with real errno values now, and it is worth saying +// why that needed doing. +// +// Every failure in these used to be `return -1`, and -1 is -EPERM by the time +// it reaches a program. So linking a file that does not exist reported +// "operation not permitted" rather than "no such file" -- not a cosmetic +// difference, because a caller distinguishes the two and acts on it. +// +// zsh is how it surfaced. Its history locking links a lockfile and its history +// save renames a temporary over the real file, and on a machine with no +// history yet both legitimately fail the first time: +// +// zsh: locking failed for /root/.zsh_history: operation not permitted +// zsh: failed to write history file /root/.zsh_history.new: operation not +// permitted +// +// -- neither of which is what happened. About ninety other `return -1`s remain +// in this file. These are corrected because these were the ones found, and +// renameat() is corrected alongside rename() because musl's rename() calls it. addr_t sys_link(void) { char name[DIRSIZ], *new, *old; struct inode *dp, *ip; - if (argstr(0, &old) < 0 || argstr(1, &new) < 0) return -1; + int err = EPERM; + if (argstr(0, &old) < 0 || argstr(1, &new) < 0) return -EFAULT; begin_op(); - if (!(ip = namei(old))) { end_op(); return -1; } + if (!(ip = namei(old))) { end_op(); return -ENOENT; } ilock(ip); - if (ip->type == T_DIR) { iunlockput(ip); end_op(); return -1; } + // The one place EPERM is right: POSIX names it for a directory. + if (ip->type == T_DIR) { iunlockput(ip); end_op(); return -EPERM; } ip->nlink++; iupdate(ip); iunlock(ip); - if (!(dp = nameiparent(new, name))) goto bad_link; + if (!(dp = nameiparent(new, name))) { err = ENOENT; goto bad_link; } ilock(dp); - if (dp->dev != ip->dev || dirlink(dp, name, ip->inum) < 0) { iunlockput(dp); goto bad_link; } + if (dp->dev != ip->dev) { iunlockput(dp); err = EXDEV; goto bad_link; } + if (dirlink(dp, name, ip->inum) < 0) { iunlockput(dp); err = EEXIST; goto bad_link; } iunlockput(dp); iput(ip); end_op(); return 0; bad_link: ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); - return -1; + return -err; } +// Same correction as link() and rename() above, and in the same path. +// +// **ENOENT is the one that mattered.** A program that removes a file it is not +// sure exists writes `if (unlink(p) < 0 && errno != ENOENT) complain()`, which +// is the standard shape -- and against -1/EPERM every such test took the +// complaining branch. zsh saving its history is one of them, and it went on +// reporting a failure after the history had in fact been written. addr_t sys_unlink(void) { struct inode *ip, *dp; char name[DIRSIZ], *path; uint off; - if (argstr(0, &path) < 0) return -1; + int err = EPERM; + if (argstr(0, &path) < 0) return -EFAULT; begin_op(); - if (!(dp = nameiparent(path, name))) { end_op(); return -1; } + if (!(dp = nameiparent(path, name))) { end_op(); return -ENOENT; } ilock(dp); - if (namecmp(name, ".") == 0 || namecmp(name, "..") == 0) goto bad_unlink; - if (!(ip = dirlookup(dp, name, &off))) goto bad_unlink; + // "." and ".." are refused by name, which is what POSIX asks for. + if (namecmp(name, ".") == 0 || namecmp(name, "..") == 0) { err = EINVAL; goto bad_unlink; } + if (!(ip = dirlookup(dp, name, &off))) { err = ENOENT; goto bad_unlink; } ilock(ip); if (ip->nlink < 1) panic("unlink: nlink < 1"); - if (ip->type == T_DIR && dirempty(ip) != 1) { iunlockput(ip); goto bad_unlink; } - if (dirunlink(dp, name, off) < 0) { iunlockput(ip); goto bad_unlink; } + if (ip->type == T_DIR && dirempty(ip) != 1) { iunlockput(ip); err = ENOTEMPTY; goto bad_unlink; } + if (dirunlink(dp, name, off) < 0) { iunlockput(ip); err = EIO; goto bad_unlink; } if (ip->type == T_DIR) { dp->nlink--; iupdate(dp); } iunlockput(dp); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return 0; -bad_unlink: iunlockput(dp); end_op(); return -1; +bad_unlink: iunlockput(dp); end_op(); return -err; } addr_t sys_exec(void) @@ -797,25 +828,32 @@ addr_t sys_rename(void) char oldname[DIRSIZ], newname[DIRSIZ]; uint off; struct dirent de; - if (argstr(0, &oldpath) < 0 || argstr(1, &newpath) < 0) return -1; + int err = EPERM; + if (argstr(0, &oldpath) < 0 || argstr(1, &newpath) < 0) return -EFAULT; begin_op(); - if (!(ip = namei(oldpath))) { end_op(); return -1; } + if (!(ip = namei(oldpath))) { end_op(); return -ENOENT; } ilock(ip); - if (ip->type == T_DIR) { iunlockput(ip); end_op(); return -1; } + // Renaming a directory needs the ".." fixup and the loop check that are + // not written here. EISDIR names what is missing; EPERM did not. + if (ip->type == T_DIR) { iunlockput(ip); end_op(); return -EISDIR; } ip->nlink++; iupdate(ip); iunlock(ip); dp_old = nameiparent(oldpath, oldname); dp_new = nameiparent(newpath, newname); if (!dp_old || !dp_new || dp_old->dev != ip->dev || dp_new->dev != ip->dev) { + err = (!dp_old || !dp_new) ? ENOENT : EXDEV; if (dp_old) iput(dp_old); if (dp_new) iput(dp_new); goto bad_rename; } ilock(dp_new); - if (dirlink(dp_new, newname, ip->inum) < 0) { iunlockput(dp_new); iput(dp_old); goto bad_rename; } + if (dirlink(dp_new, newname, ip->inum) < 0) { + iunlockput(dp_new); iput(dp_old); err = EEXIST; goto bad_rename; + } iunlockput(dp_new); ilock(dp_old); if (!(ip_src = dirlookup(dp_old, oldname, &off))) { - iunlockput(dp_old); ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return -1; + iunlockput(dp_old); ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); + end_op(); return -ENOENT; } iput(ip_src); memset(&de, 0, sizeof(de)); @@ -824,7 +862,7 @@ addr_t sys_rename(void) ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return 0; bad_rename: - ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return -1; + ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return -err; } addr_t sys_symlink(void) @@ -1493,26 +1531,28 @@ addr_t sys_renameat(void) uint off; struct dirent de; begin_op(); - if (!(ip = at_namei(olddirfd, oldpath))) { end_op(); return -1; } + if (!(ip = at_namei(olddirfd, oldpath))) { end_op(); return -ENOENT; } ilock(ip); - if (ip->type == T_DIR) { iunlockput(ip); end_op(); return -1; } + if (ip->type == T_DIR) { iunlockput(ip); end_op(); return -EISDIR; } ip->nlink++; iupdate(ip); iunlock(ip); dp_old = at_nameiparent(olddirfd, oldpath, oldname); dp_new = at_nameiparent(newdirfd, newpath, newname); if (!dp_old || !dp_new || dp_old->dev != ip->dev || dp_new->dev != ip->dev) { + int e = (!dp_old || !dp_new) ? ENOENT : EXDEV; if (dp_old) iput(dp_old); if (dp_new) iput(dp_new); - ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return -1; + ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return -e; } ilock(dp_new); if (dirlink(dp_new, newname, ip->inum) < 0) { iunlockput(dp_new); iput(dp_old); - ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return -1; + ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return -EEXIST; } iunlockput(dp_new); ilock(dp_old); if (!(ip_src = dirlookup(dp_old, oldname, &off))) { - iunlockput(dp_old); ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return -1; + iunlockput(dp_old); ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); + end_op(); return -ENOENT; } iput(ip_src); memset(&de, 0, sizeof(de));