| Message ID | 54C4DC4E.3080306@sunrus.com.cn |
|---|---|
| State | New |
| Headers | show |
On 25 January 2015 at 12:06, Chen Gang S <gang.chen@sunrus.com.cn> wrote: > lock_user_struct() and unlock_user_struct() need always be paired with > each other, or will cause resource leak. > > Also remove redundant check for 'target_mb' in abi_long do_msgrcv(). > > Also match the coding styles found by "./scripts/checkpatch.pl". > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Are you claiming that you've reviewed *all* the code in this file for mismatched lock/unlock calls? If so, it would be nice to say so explicitly in the commit message. If not, it would be nice if the commit message was clearer about what areas of the code it applied to. The code changes are correct, though. thanks -- PMM
On 1/25/15 20:49, Peter Maydell wrote: > On 25 January 2015 at 12:06, Chen Gang S <gang.chen@sunrus.com.cn> wrote: >> lock_user_struct() and unlock_user_struct() need always be paired with >> each other, or will cause resource leak. >> >> Also remove redundant check for 'target_mb' in abi_long do_msgrcv(). >> >> Also match the coding styles found by "./scripts/checkpatch.pl". >> >> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > Thank you for your work about the several patches. > Are you claiming that you've reviewed *all* the code in this > file for mismatched lock/unlock calls? If so, it would be nice > to say so explicitly in the commit message. If not, it would be > nice if the commit message was clearer about what areas of the > code it applied to. The code changes are correct, though. > At present, I finished all lock_user_struct() and unlock_user_struct() in "linux-user/syscall.c". For me, after this patch, they are all OK. But for all lock/unlock in "linux-user/syscall.c", for me, I am doubting several areas, but I did not send patch for them: - I need check them carefully again to be sure they are really issue: Read the related code again and again, if I really treat it as an issue, I shall make related patch (and pass compiling, at least). - I have no enough time resources on it: In week end, I have to spend some time resource on my son (check his homework, play with him). I also need spend time resources on Linux kernel and gcc/binutils. In week day, I may work overtime sometimes, also I need spend 4 hours on subway between my home and office per day (come and go). - So it is really necessary for me to make a schedule (the tasks/steps, the time point, and the risks), and also need consider about: How to use the time resources efficiently: e.g. fully use the time resources on the subway between home and office, in work day; fully use the time resources on holidays (e.g. Chinese Spring Festival). How to fully use the human resources: e.g. try my best to always keep well communication with all related open source members (which will give much valuable help for the related tasks). Welcome any members' ideas, suggestions, and completions, also welcome any members to help check "linux-user/syscall.c" (it will save my time resource: now for qemu, my highest priority task is for tile qemu). Thanks.
On 25 January 2015 at 21:59, Chen Gang S <gang.chen@sunrus.com.cn> wrote: > On 1/25/15 20:49, Peter Maydell wrote: >> Are you claiming that you've reviewed *all* the code in this >> file for mismatched lock/unlock calls? If so, it would be nice >> to say so explicitly in the commit message. If not, it would be >> nice if the commit message was clearer about what areas of the >> code it applied to. The code changes are correct, though. >> > > At present, I finished all lock_user_struct() and unlock_user_struct() > in "linux-user/syscall.c". For me, after this patch, they are all OK. > > But for all lock/unlock in "linux-user/syscall.c", for me, I am doubting > several areas, but I did not send patch for them: > > - I need check them carefully again to be sure they are really issue: > > Read the related code again and again, if I really treat it as an > issue, I shall make related patch (and pass compiling, at least). > > - I have no enough time resources on it: That's fine. I'm definitely not asking you to do this work. I would just like the commit message to be clear about the scope of the work the patch covers. If the patch is just "Fix mismatched lock/unlock calls in IPC struct conversion functions" then that's fine, but the commit message should say that. At the moment the commit message is very vague. thanks -- PMM
On 1/26/15 06:10, Peter Maydell wrote: > On 25 January 2015 at 21:59, Chen Gang S <gang.chen@sunrus.com.cn> wrote: >> On 1/25/15 20:49, Peter Maydell wrote: >>> Are you claiming that you've reviewed *all* the code in this >>> file for mismatched lock/unlock calls? If so, it would be nice >>> to say so explicitly in the commit message. If not, it would be >>> nice if the commit message was clearer about what areas of the >>> code it applied to. The code changes are correct, though. >>> >> >> At present, I finished all lock_user_struct() and unlock_user_struct() >> in "linux-user/syscall.c". For me, after this patch, they are all OK. >> >> But for all lock/unlock in "linux-user/syscall.c", for me, I am doubting >> several areas, but I did not send patch for them: >> >> - I need check them carefully again to be sure they are really issue: >> >> Read the related code again and again, if I really treat it as an >> issue, I shall make related patch (and pass compiling, at least). >> >> - I have no enough time resources on it: > > That's fine. I'm definitely not asking you to do this work. OK, thanks. :-) > I would just like the commit message to be clear about the > scope of the work the patch covers. If the patch is just "Fix > mismatched lock/unlock calls in IPC struct conversion functions" > then that's fine, but the commit message should say that. At the > moment the commit message is very vague. > OK, thanks. I am not quite familiar with this file, so I describe the modification by function name, e.g. lock_user_struct() and unlick_user_struct() in the patch subject. Welcome to help improve the patch comments. If necessary to send patch v2, please let me know, I shall try. Thanks.
On 26 January 2015 at 14:59, Chen Gang S <gang.chen@sunrus.com.cn> wrote: > On 1/26/15 06:10, Peter Maydell wrote: >> I would just like the commit message to be clear about the >> scope of the work the patch covers. If the patch is just "Fix >> mismatched lock/unlock calls in IPC struct conversion functions" >> then that's fine, but the commit message should say that. At the >> moment the commit message is very vague. >> > > OK, thanks. > > I am not quite familiar with this file, so I describe the modification > by function name, e.g. lock_user_struct() and unlick_user_struct() in > the patch subject. In a big file I think it's often more useful to describe the functions which are being changed. My suggested subject would be: "Fix mismatched lock/unlock calls in IPC struct conversion functions" Riku can decide if he wants a v2 or will just fix it up as he applies it to his linux-user tree. -- PMM
On 1/26/15 23:01, Peter Maydell wrote: > On 26 January 2015 at 14:59, Chen Gang S <gang.chen@sunrus.com.cn> wrote: >> On 1/26/15 06:10, Peter Maydell wrote: >>> I would just like the commit message to be clear about the >>> scope of the work the patch covers. If the patch is just "Fix >>> mismatched lock/unlock calls in IPC struct conversion functions" >>> then that's fine, but the commit message should say that. At the >>> moment the commit message is very vague. >>> >> >> OK, thanks. >> >> I am not quite familiar with this file, so I describe the modification >> by function name, e.g. lock_user_struct() and unlick_user_struct() in >> the patch subject. > > In a big file I think it's often more useful to describe the > functions which are being changed. My suggested subject would be: > > "Fix mismatched lock/unlock calls in IPC struct conversion functions" > What you said above sounds reasonable to me. > Riku can decide if he wants a v2 or will just fix it up as he > applies it to his linux-user tree. > OK, thanks.
Hi, First of all, thanks Chen for taking time to improve the linux-user codebase in qemu! On Mon, Jan 26, 2015 at 03:01:52PM +0000, Peter Maydell wrote: > On 26 January 2015 at 14:59, Chen Gang S <gang.chen@sunrus.com.cn> wrote: > > On 1/26/15 06:10, Peter Maydell wrote: > >> I would just like the commit message to be clear about the > >> scope of the work the patch covers. If the patch is just "Fix > >> mismatched lock/unlock calls in IPC struct conversion functions" > >> then that's fine, but the commit message should say that. At the > >> moment the commit message is very vague. > >> > > > > OK, thanks. > > > > I am not quite familiar with this file, so I describe the modification > > by function name, e.g. lock_user_struct() and unlick_user_struct() in > > the patch subject. > In a big file I think it's often more useful to describe the > functions which are being changed. My suggested subject would be: > "Fix mismatched lock/unlock calls in IPC struct conversion functions" > Riku can decide if he wants a v2 or will just fix it up as he > applies it to his linux-user tree. No need for v2, I've change the title in my tree. Riku
Thank you for your work, too. Next month, I shall start tile qemu, I guess, for coding, I shall start from linux-user (which is mainly for cpu emulation). - For each patch, I should make at least a valuable change, and pass related test. - Each month, I should make 3 patches at least. - Hope I can finish tile for linux-user within 2015-06-30: finish all tile cpu common instructions emulation, and pass all related test. Welcome any ideas, suggestions, and completions, e.g. - Is what I said above really correct (e.g. is linux-user really mainly for cpu emulation)?. - For each patch, are there additional information, requirements or suggestions which I should notice about? - If I can finish 3 patches per month, is it possible to finish tile cpu common instructions emulation for linux-user within 2015-06-30? Thanks. On 1/28/15 22:27, Riku Voipio wrote: > Hi, > > First of all, thanks Chen for taking time to improve the linux-user > codebase in qemu! > > On Mon, Jan 26, 2015 at 03:01:52PM +0000, Peter Maydell wrote: >> On 26 January 2015 at 14:59, Chen Gang S <gang.chen@sunrus.com.cn> wrote: >>> On 1/26/15 06:10, Peter Maydell wrote: >>>> I would just like the commit message to be clear about the >>>> scope of the work the patch covers. If the patch is just "Fix >>>> mismatched lock/unlock calls in IPC struct conversion functions" >>>> then that's fine, but the commit message should say that. At the >>>> moment the commit message is very vague. >>>> >>> >>> OK, thanks. >>> >>> I am not quite familiar with this file, so I describe the modification >>> by function name, e.g. lock_user_struct() and unlick_user_struct() in >>> the patch subject. > >> In a big file I think it's often more useful to describe the >> functions which are being changed. My suggested subject would be: > >> "Fix mismatched lock/unlock calls in IPC struct conversion functions" > >> Riku can decide if he wants a v2 or will just fix it up as he >> applies it to his linux-user tree. > > No need for v2, I've change the title in my tree. > > Riku > >
On 28 January 2015 at 22:09, Chen Gang S <gang.chen@sunrus.com.cn> wrote: > - Is what I said above really correct (e.g. is linux-user really mainly > for cpu emulation)?. Not really. linux-user is mainly for running single Linux binaries. It has a secondary use for running gcc test binaries which think they are "bare metal" but actually use some kind of semihosting API. (You should check whether tile has one of those.) As well as linux-user mode, QEMU has system emulation mode, where we emulate a complete machine. Both modes need CPU emulation. -- PMM
On 1/29/15 06:36, Peter Maydell wrote: > On 28 January 2015 at 22:09, Chen Gang S <gang.chen@sunrus.com.cn> wrote: >> - Is what I said above really correct (e.g. is linux-user really mainly >> for cpu emulation)?. > > Not really. linux-user is mainly for running single Linux binaries. > It has a secondary use for running gcc test binaries which think > they are "bare metal" but actually use some kind of semihosting API. > (You should check whether tile has one of those.) > > As well as linux-user mode, QEMU has system emulation mode, where > we emulate a complete machine. > > Both modes need CPU emulation. > OK, thanks. For coding and test, is linux-user a good starting position for me? (I guess it is). Thanks.
On 1/29/15 09:37, Chen Gang S wrote: > On 1/29/15 06:36, Peter Maydell wrote: >> On 28 January 2015 at 22:09, Chen Gang S <gang.chen@sunrus.com.cn> wrote: >>> - Is what I said above really correct (e.g. is linux-user really mainly >>> for cpu emulation)?. >> >> Not really. linux-user is mainly for running single Linux binaries. >> It has a secondary use for running gcc test binaries which think >> they are "bare metal" but actually use some kind of semihosting API. >> (You should check whether tile has one of those.) >> >> As well as linux-user mode, QEMU has system emulation mode, where >> we emulate a complete machine. >> >> Both modes need CPU emulation. >> > > OK, thanks. > > For coding and test, is linux-user a good starting position for me? (I > guess it is). > At present, I make a static program which will print "Hello world" for microblaze architecture, and can excute successfully. [root@localhost qemu]# cat test.c #include <stdio.h> int main() { printf("Hello world!\n"); return 0; } [root@localhost qemu]# /upstream/release/bin/microblaze-gchen-linux-gcc -Wall -O2 -static -o test.mb test.c [root@localhost qemu]# file ./test.mb ./test.mb: ELF 32-bit MSB executable, Xilinx MicroBlaze 32-bit RISC, version 1 (SYSV), statically linked, for GNU/Linux 2.6.32, not stripped [root@localhost qemu]# ls -l ./test.mb -rwxr-xr-x. 1 root root 3224540 Feb 5 06:39 ./test.mb [root@localhost qemu]# ./microblaze-linux-user/qemu-microblaze ./test.mb Hello world! And I also generate tile "Hello world" static program: [root@localhost qemu]# /upstream/release-tile/bin/tilegx-gchen-linux-gcc -Wall -O2 -static -o test.tgx test.c [root@localhost qemu]# file ./test.tgx ./test.tgx: ELF 64-bit LSB executable, Tilera TILE-Gx, version 1 (SYSV), statically linked, for GNU/Linux 2.6.32, not stripped [root@localhost qemu]# ls -l ./test.tgx -rwxr-xr-x. 1 root root 3611376 Feb 5 06:42 ./test.tgx I shall try to let tile "Hello world" static program run successfully within this month: - 1st patch: can run an empty elf64 tile executable in linux-user. (try to finish within 2015-02-15). - 2nd patch: can run "Hello world" elf64 tile program in linux-user. (try to finish within 2015-02-25, it seems hard to finish in time). - 3rd patch: for completion, or fixing issues, or documentations. (try to finish within 2015-02-28). Welcome any ideas, suggestions, and completions. Thanks.
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index ec9e4fc..b2da432 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -2518,8 +2518,10 @@ static inline abi_long target_to_host_semid_ds(struct semid_ds *host_sd, if (!lock_user_struct(VERIFY_READ, target_sd, target_addr, 1)) return -TARGET_EFAULT; - if (target_to_host_ipc_perm(&(host_sd->sem_perm),target_addr)) + if (target_to_host_ipc_perm(&(host_sd->sem_perm), target_addr)) { + unlock_user_struct(target_sd, target_addr, 0); return -TARGET_EFAULT; + } host_sd->sem_nsems = tswapal(target_sd->sem_nsems); host_sd->sem_otime = tswapal(target_sd->sem_otime); host_sd->sem_ctime = tswapal(target_sd->sem_ctime); @@ -2534,8 +2536,10 @@ static inline abi_long host_to_target_semid_ds(abi_ulong target_addr, if (!lock_user_struct(VERIFY_WRITE, target_sd, target_addr, 0)) return -TARGET_EFAULT; - if (host_to_target_ipc_perm(target_addr,&(host_sd->sem_perm))) + if (host_to_target_ipc_perm(target_addr, &(host_sd->sem_perm))) { + unlock_user_struct(target_sd, target_addr, 0); return -TARGET_EFAULT; + } target_sd->sem_nsems = tswapal(host_sd->sem_nsems); target_sd->sem_otime = tswapal(host_sd->sem_otime); target_sd->sem_ctime = tswapal(host_sd->sem_ctime); @@ -2796,8 +2800,10 @@ static inline abi_long target_to_host_msqid_ds(struct msqid_ds *host_md, if (!lock_user_struct(VERIFY_READ, target_md, target_addr, 1)) return -TARGET_EFAULT; - if (target_to_host_ipc_perm(&(host_md->msg_perm),target_addr)) + if (target_to_host_ipc_perm(&(host_md->msg_perm), target_addr)) { + unlock_user_struct(target_md, target_addr, 0); return -TARGET_EFAULT; + } host_md->msg_stime = tswapal(target_md->msg_stime); host_md->msg_rtime = tswapal(target_md->msg_rtime); host_md->msg_ctime = tswapal(target_md->msg_ctime); @@ -2817,8 +2823,10 @@ static inline abi_long host_to_target_msqid_ds(abi_ulong target_addr, if (!lock_user_struct(VERIFY_WRITE, target_md, target_addr, 0)) return -TARGET_EFAULT; - if (host_to_target_ipc_perm(target_addr,&(host_md->msg_perm))) + if (host_to_target_ipc_perm(target_addr, &(host_md->msg_perm))) { + unlock_user_struct(target_md, target_addr, 0); return -TARGET_EFAULT; + } target_md->msg_stime = tswapal(host_md->msg_stime); target_md->msg_rtime = tswapal(host_md->msg_rtime); target_md->msg_ctime = tswapal(host_md->msg_ctime); @@ -2953,8 +2961,7 @@ static inline abi_long do_msgrcv(int msqid, abi_long msgp, target_mb->mtype = tswapal(host_mb->mtype); end: - if (target_mb) - unlock_user_struct(target_mb, msgp, 1); + unlock_user_struct(target_mb, msgp, 1); g_free(host_mb); return ret; } @@ -2966,8 +2973,10 @@ static inline abi_long target_to_host_shmid_ds(struct shmid_ds *host_sd, if (!lock_user_struct(VERIFY_READ, target_sd, target_addr, 1)) return -TARGET_EFAULT; - if (target_to_host_ipc_perm(&(host_sd->shm_perm), target_addr)) + if (target_to_host_ipc_perm(&(host_sd->shm_perm), target_addr)) { + unlock_user_struct(target_sd, target_addr, 0); return -TARGET_EFAULT; + } __get_user(host_sd->shm_segsz, &target_sd->shm_segsz); __get_user(host_sd->shm_atime, &target_sd->shm_atime); __get_user(host_sd->shm_dtime, &target_sd->shm_dtime); @@ -2986,8 +2995,10 @@ static inline abi_long host_to_target_shmid_ds(abi_ulong target_addr, if (!lock_user_struct(VERIFY_WRITE, target_sd, target_addr, 0)) return -TARGET_EFAULT; - if (host_to_target_ipc_perm(target_addr, &(host_sd->shm_perm))) + if (host_to_target_ipc_perm(target_addr, &(host_sd->shm_perm))) { + unlock_user_struct(target_sd, target_addr, 0); return -TARGET_EFAULT; + } __put_user(host_sd->shm_segsz, &target_sd->shm_segsz); __put_user(host_sd->shm_atime, &target_sd->shm_atime); __put_user(host_sd->shm_dtime, &target_sd->shm_dtime);
lock_user_struct() and unlock_user_struct() need always be paired with each other, or will cause resource leak. Also remove redundant check for 'target_mb' in abi_long do_msgrcv(). Also match the coding styles found by "./scripts/checkpatch.pl". Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> --- linux-user/syscall.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-)