On Fri, 07 Sep 2012 21:56:48 +1000 Allan McRae <[email protected]> wrote:
> On 07/09/12 21:06, Menachem Moystoviz wrote: > > I'm not a dev, nor the original author of the patch, but... > > > > On Fri, Sep 7, 2012 at 10:29 AM, Allan McRae <[email protected]> wrote: > >> On 06/09/12 13:34, Andrew Gregory wrote: <snip> > >>> + > >>> + /* make sure pkgfile and target are of the > >>> same type */ > >>> + if(!is_dir != !(pkgfile[pkgfile_len - 1] == > >>> '/')) { > >> > >> !!! - literally.... How about: > >> > >> if (is_dir && (pkgfile[pkgfile_len - 1] != '/')) > > This isn't the same check - Andrew's check verifies that is_dir XOR > > (pkgfile's last character is not '/' (i.e. pkgfile names a file > > path)), > > is true, while your test is for AND. This way of defining XOR is quite > > confusing, though - isn't there a clearer way? (i.e. macros, inline > > functions?) > > Ah - that is confusing... obviously! > > So a clearer version of the test would be: > > if ((!is_dir && pkgfile[pkgfile_len - 1] == '/') || > (is_dir && pkgfile[pkgfile_len - 1] != '/')) > Yeah, the !!! xor can be a bit confusing if you're not familiar with it, but I dislike long multi-line ifs. I can just force is_dir to 1 or 0 and compare them directly: is_dir = S_ISDIR(buf.st_mode) ? 1 : 0; ... if(is_dir != (pkgfile[pkgfile_len - 1] == '/'))... > >> > >>> + continue; > >>> + } > >>> + > >>> + /* make sure pkgfile is long enough */ > >>> + if(pkgfile_len < pbname_len) { > >>> + continue; > >>> + } > >> > >> If I understand this correctly, we are comparing the length of the final > >> component of the passed in parameter to the entire filename length from > >> the package. I think that it would be quite uncommon to hit the > >> continue here, so that can be removed. > >> > > I think you mean to say that we replace this if(){continue;} with > > if(!){...} wrapping the rest of the loop? > > That would make sense if this branch is costly. This is a safety to make sure that we don't try to dereference a negative index in the next if. > > No. I was saying that I think this test is going to result in a very > small number of cases being skipped and so it would be more efficient to > just not perform it. > > >>> > >>> - /* avoid the costly realpath usage if the > >>> basenames don't match */ > >>> - if(strcmp(mbasename(pkgfile), bname) != 0) { > >>> + /* make sure pbname_len takes us to the > >>> start of a path component */ > >>> + if(pbname_len != pkgfile_len && > >>> pkgfile[pkgfile_len - pbname_len - 1] != '/') { > >> > >> With the above removal: > >> if (pbname_len > pkgfile_len) > >> This check must be != because it is specifically looking for pbname_len either taking us to the beginning of the string or the beginning of a path component, ie: pkgfile == "basename" or pkgfile == "foo/bar/basename" but not pkgfile == "foo/barbasename" It could be combined (not replaced) with the check in the previous if statement, but that gets a little ugly, so I left them separate. > >>> + continue; > >>> + } > >>> + > >>> + /* compare basename with bname */ > >>> + if(strncmp(pkgfile + pkgfile_len - > >>> pbname_len, bname, bname_len) != 0) { > >> > >> Why an strncmp here? strcmp will do the same comparison... > >> > > In order to make sure that we don't run into buffer overflow issues - > > http://stackoverflow.com/questions/448563/am-i-correct-that-strcmp-is-equivalent-and-safe-for-literals > > Both strings are null terminated. > strncmp is needed because if it's a directory pkgfile will end in '/' but bname will not. > >>> continue; > >>> } > >>> > >>> /* concatenate our file and the root path */ > >>> - if(rootlen + 1 + strlen(pkgfile) > > >>> PATH_MAX) { > >>> + if(rootlen + 1 + pkgfile_len > PATH_MAX) { > >>> path[rootlen] = '\0'; /* reset path > >>> for error message */ > >>> pm_printf(ALPM_LOG_ERROR, _("path > >>> too long: %s%s\n"), path, pkgfile); > >>> continue; > >>> } > >>> strcpy(path + rootlen, pkgfile); > >>> > >>> - pdname = mdirname(path); > >>> - ppath = realpath(pdname, NULL); > >>> + /* check that the file exists on disk and > >>> is the correct type */ > >>> + if(lstat(path, &buf) == -1 || !is_dir != > >>> !S_ISDIR(buf.st_mode)) { > >> > >> !!! again. How about: > >> > >> if(is_dir && lstat(path, &buf) == 0 && !S_ISDIR(buf.st_mode)) > >> > >> which is what I think is being tested here. > > Again, your fix doesn't match the original test - Andrew's test > > verifies that the file either doesn't exist > > or that (the file is a directory) XOR is_dir is true. > >> > >> But I wonder if we really need this... I believe this is to avoid > >> "strange" results when people replace directories in a package with > >> symlinks to directories in another package. The more I think about > >> this, the more I want to just remove our special case handling of this > >> in general. A directory in a package and a symlink on the filesystem > >> should just be a conflict. So unless I am missing another reason for > >> this, get rid of it. > >> > > No comment, as I do not know the codebase well enough to respond. I didn't really like this in the first place; deleted. > >>> + continue; > >>> + } > >>> + > >>> + if(is_dir) { > >>> + pdname = realpath(path, NULL); > >>> + ppath = mdirname(pdname); > >>> + } else { > >>> + pdname = mdirname(path); > >>> + ppath = realpath(pdname, NULL); > >>> + } > >>> free(pdname); > >> > >> Umm... why calculate pdname just to free it immediately? > > Since we need to use it to calculate ppath? > > Oops... I blame doing quick review before rushing to the train! > I copied this particular bit from above where I do the same thing to the target, but it's unnecessary work here. A simple strncpy will actually suffice: strncpy(path + rootlen, pkgfile, pkgfile_len - pbname_len); path[rootlen + pkgfile_len - pbname_len] = '\0'; ppath = realpath(path, NULL); > >> > >>> if(!ppath) { > >>> > >> > >> > > > > Overall, I can't really judge the quality of Andrew's patch, but I > > definitely > > do not agree with the comments given by Allan. > > > > HTH in my own way, > > > > Gesh > > > > > > > >
