On 9/3/17 7:22 PM, Darrick J. Wong wrote:
> On Sun, Sep 03, 2017 at 12:24:33AM -0700, Christoph Hellwig wrote:
>> Can you splits this up a bit to be more self explanatory, e.g.
>> one patch per warning class or logical change?
>>
>>> if (be32_to_cpu(free->hdr.nvalid) > maxent ||
>>> - be32_to_cpu(free->hdr.nvalid) < 0 ||
>>> be32_to_cpu(free->hdr.nused) > maxent ||
>>> - be32_to_cpu(free->hdr.nused) < 0 ||
>>> be32_to_cpu(free->hdr.nused) >
>>> be32_to_cpu(free->hdr.nvalid)) {
>>
>> be32_to_cpu returns uint, so this makese sense.
>>
>>> - (void)getcwd(curdir,MAXPATHLEN);
>>> + if (!getcwd(curdir, MAXPATHLEN)) {
>>> + perror("getcwd");
>>> + strcpy(curdir, ".");
>>> + }
>>
>> All this chdir magic will need a lot more explanation. To me it
>> seems like most of this is a left over from IRIX days where we
>> could have device names relative to a volume. The proper fix
>> probably is to just remove that whole thing - if we'd ever
>> need to bring it back we should use openat relative to a dirfd
>> instead.
>
> Yeah, AFAICT all this getcwd/chdir stuff seems to exist so that
> check_open can change the working directory (it doesn't on Linux) and we
> can change back to continue to use relative paths. Nothing ever changes
> the directory (at least not in the call paths of libxfs_init) so I think
> we can just rip it out. As a separate patch.
>
> In the meantime, do you want me to resend with just the uncontroversial
> bits?
The rest is all fine with me, sorry, I should have said that as well.
I agree with Christoph that splitting into logically grouped changes
makes sense; I was going to let that slide, but it always has been
and still is best practice, of course.
As for moving that ASSERT - yeah, at one point we said that if a null
ptr deref is going to happen immediately after, the ASSERT is really
of no value.
-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
