> -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Thursday, July 21, 2016 4:36 PM > To: Jastrzebski, MichalX K <michalx.k.jastrzebski at intel.com> > Cc: dev at dpdk.org; Richardson, Bruce <bruce.richardson at intel.com>; > Kobylinski, MichalX <michalx.kobylinski at intel.com>; Gonzalez Monroy, > Sergio <sergio.gonzalez.monroy at intel.com>; david.marchand at 6wind.com > Subject: Re: [dpdk-dev] [PATCH] eal: fix check number of bytes from read > function > > Hi, > > 2016-07-20 16:24, Michal Jastrzebski: > > - if (read(fd, &page, sizeof(uint64_t)) < 0) { > > + > > + retval = read(fd, &page, sizeof(uint64_t)); > > + if (retval < 0) { > > RTE_LOG(ERR, EAL, "%s(): cannot read /proc/self/pagemap: > %s\n", > > __func__, strerror(errno)); > > close(fd); > > return RTE_BAD_PHYS_ADDR; > > + } else if (retval >= 0 && retval < (int)sizeof(uint64_t)) { >
Hi Thomas, > I have 4 comments about the above line: That's too much for one line. I should improve next time:) > - the check retval >= 0 is not needed because implied by else > - why not checking retval != sizeof(uint64_t) as it is the exact expected > value? Yes, it is better solution, > - (int)sizeof(uint64_t) can be replaced by 8 but it's shorter ;) I didn't want to change all invokes of read() function here. I can use some macro: #define PFN_MASK_SIZE 8 How do You think? > - only 1 space is required between } and else > > > + RTE_LOG(ERR, EAL, "%s(): read %d bytes from > /proc/self/pagemap " > > + "but expected %d: %s\n", > > + __func__, retval, (int)sizeof(uint64_t), > strerror(errno)); > > Are you sure errno is meaningful here? I think it is not. Will send v2. > > > + close(fd); > > + return RTE_BAD_PHYS_ADDR; > > } Thanks for a review Michal.
