The fcntl call fails in the actual scene and it is really hard to happen. But according to a good coding style, I think there should be a error handling for a system call.
+ if (((unsigned int) flags & O_ACCMODE) == O_RDONLY) { The flags is a int type. According to strict programming specifications, it should be converted to unsigned type before doing bitwise operator. I am doing this just to avoid codex warnings. If you think it is not necessary to do so, I can remove it. -----邮件原件----- 发件人: Eric Blake [mailto:ebl...@redhat.com] 发送时间: 2019年3月22日 10:01 收件人: lizhengui; Paolo Bonzini; stefa...@redhat.com; mre...@redhat.com; kw...@redhat.com 抄送: wangjie (P); qemu-devel@nongnu.org; qemu-bl...@nongnu.org; Fangyi (C) 主题: Re: [Qemu-block] [PATCH] qemu-pr-helper: check the return value of fcntl in do_pr_out [top-posting is harder to read on technical lists; I'm reordering your message before replying inline] > On 20/03/19 15:07, Zhengui li wrote: >> The function fcntl maybe return -1, which is not a unsigned type. >> Unsigned type or Negative values should not do bitwise operator with >> O_ACCMODE. > > Did you actually find a case in which the fcntl can fail? On 3/21/19 8:50 PM, lizhengui wrote: > If the fd is invalid or interrupted by signal. If the fd is invalid, we have a coding bug on our hand - we should not be calling do_pr_out with an invalid fd. Do you have a backtrace where that actually happened? As for being interrupted by a signal, that's not possible. fcntl() can only be interrupted by signal forF_SETLKW, F_OFD_SETLKW, F_GETLK, F_SETLK, F_OFD_GETLK, or F_OFD_SETLK. I agree that your fix avoids a bug if it can actually happen - but I also want to know if it happened in practice or whether it is just plugging a theoretical hole (it may determine whether your patch must go into 4.0, or can slip to 4.1). >> - if ((fcntl(fd, F_GETFL) & O_ACCMODE) == O_RDONLY) { >> + flags = fcntl(fd, F_GETFL); >> + if (flags < 0) { >> + return -1; >> + } This addition is fine. >> + >> + if (((unsigned int) flags & O_ACCMODE) == O_RDONLY) { This cast is not. You already guaranteed that flags is non-negative by the code added above, and therefore the bitwise-and on the signed type is well-defined, without the need to muddy things up with a cast. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org