----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3583/#review8591 -----------------------------------------------------------
Hi Nicolas, thanks for the contribution. Couple of things before you commit this. src/sim/syscall_emul.hh (line 308) <http://reviews.gem5.org/r/3583/#comment7479> alignment should be under the 'S' in "SyscallDesc" src/sim/syscall_emul.hh (line 312) <http://reviews.gem5.org/r/3583/#comment7480> alignment src/sim/syscall_emul.cc (line 355) <http://reviews.gem5.org/r/3583/#comment7467> Please rename fd to sim_fd. It gets confusing over time if the distinction isn't explicit especially in more complicated functions. I've seen at least one bug that was caused by this type of naming. src/sim/syscall_emul.cc (line 356) <http://reviews.gem5.org/r/3583/#comment7468> This should not be an assert here. I think the correct thing to do here is instead "if(fd < 0) {return -EBADF;}". The point is that the application is free to pass in bad file descriptors to the kernel. The kernel's job is to reject the file descriptor and return an errno to the application. It's not the kernel's job to kill the process for passing in a bad file descriptor. src/sim/syscall_emul.cc (line 357) <http://reviews.gem5.org/r/3583/#comment7469> Local variables are supposed to be lower case and seperated by underscores (instead of camel case). http://www.m5sim.org/Coding_Style src/sim/syscall_emul.cc (line 359) <http://reviews.gem5.org/r/3583/#comment7470> variable name src/sim/syscall_emul.cc (line 361) <http://reviews.gem5.org/r/3583/#comment7478> save errno to a local variable after the system call src/sim/syscall_emul.cc (line 363) <http://reviews.gem5.org/r/3583/#comment7471> space between if keyword and paren src/sim/syscall_emul.cc (line 372) <http://reviews.gem5.org/r/3583/#comment7481> add a conditional return here if the sim_fd is bad src/sim/syscall_emul.cc (line 373) <http://reviews.gem5.org/r/3583/#comment7477> rename fd to sim_fd src/sim/syscall_emul.cc (line 374) <http://reviews.gem5.org/r/3583/#comment7472> name src/sim/syscall_emul.cc (line 376) <http://reviews.gem5.org/r/3583/#comment7473> name src/sim/syscall_emul.cc (line 378) <http://reviews.gem5.org/r/3583/#comment7475> You should save the errno value into a local variable immediately after a real system call if you have intervening code between the system call and the code. Otherwise, the errno value might get clobbered before you return it. src/sim/syscall_emul.cc (line 380) <http://reviews.gem5.org/r/3583/#comment7474> space - Brandon Potter On Aug. 8, 2016, 9:18 a.m., Nicolas Derumigny wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3583/ > ----------------------------------------------------------- > > (Updated Aug. 8, 2016, 9:18 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > syscall_emul: Added getdents and getdents64 syscalls > > > Diffs > ----- > > src/arch/x86/linux/process.cc ba45735a726a > src/sim/syscall_emul.hh ba45735a726a > src/sim/syscall_emul.cc ba45735a726a > > Diff: http://reviews.gem5.org/r/3583/diff/ > > > Testing > ------- > > Works with readdir() wrapper, tested with CERE codelets > (https://github.com/benchmark-subsetting/cere). > > > Thanks, > > Nicolas Derumigny > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
