This is the mail archive of the cygwin-patches mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] cygcheck: follow symbolic links


On Jul 21 20:52, Igor Peshansky wrote:
> In any case, here's the latest incarnation, with get_word and get_dword
> folded into path.cc, and display_error returned to cygcheck.cc, where it
> belongs.  Tested reasonably well (with symlinks pointing to symlinks,
> etc).  I'll let you judge the neatness of the ChangeLog entry.  If I'm
> lucky, this might just get into 1.5.21[*].
> 	Igor
> [*] Corinna, I'm guessing this is sufficiently different that you can't
> accept it without "the fax" -- I'll keep pinging the guy who's holding
> this up, but this message is also supposed to confirm that there is a
> working patch, and the delay is simply bureaucratic.  Oh, the
> frustration...  If you judge the changes from the previous incarnation
> to not be significant, just go ahead and apply this, given the previous
> approval.

The latest fax was about this change, so I think this should still be
covered, shouldn't it?  Ping the guy nevertheless.  We should stay on
the safe side in legal questions.

I'd be happy to apply the patch, but it would be nice if you could tweak
the formatting somewhat:

> +  if (GetLastError () != NO_ERROR) display_error ("get_dword");

The display_error call should be on its own line, as usual.  This
happens multiple times in your patch.

> +  if (is_exe (fh))
> +    dll_info (path, fh, lvl, 1);
> +  else if (is_symlink (fh))
> +    display_error ("track_down: Huh?  Got a symlink!");

Is that really the supposed message here?

> +      printf (" - Not a DLL: magic number %x (%d) '%s'\n", magic, magic, (char *)&magic);

Please split the printf so that it's not longer than 80 chars.

> +      /* TODO: check for invalid path contents (see ../cygwin/path.cc:3313 */

Since source code lines are most volatile, I'd not refer to a line number
in another source code.  Just mention the function name. */

> +      if (got != sizeof (buf) || memcmp (buf, SYMLINK_COOKIE, sizeof (buf)) != 0)

Split the line, please.

> +      if (SetFilePointer (fh, 0x4c + offset + 4, 0, FILE_BEGIN) == INVALID_SET_FILE_POINTER

Same here.

> +	{
> +	  return false;
> +	}

I'd rather not have these one liners in curly brackets.  It's a bit
irritating since sometimes you put them in curly brackets, sometimes you
don't.


The code looks ok, otherwise.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]