Re: history and val-tags locks.

[Derek Price]

Mark D. Baushke wrote:

> Derek Price <derek@ximbiot.com> writes:
>
> >I'm getting ready to make two changes, possibly on stable.
>
> >The first would be to add file locking for the CVSROOT/history and
> >CVSROOT/val-tags files.  I have some reports of massively corrupted
> >history files in large repositories, and I don't see any other likely
> >cause.  Similarly, I expect locking will also be necessary on val-tags
> >in such a large repository and the additional code will be minimal after
> >I add the history locking stuff.
>
> >I was thinking that I would make totally separate lock dirs for this
> >(#cvs.history.lock and #cvs.val-tags.lock or somesuch) to allow
> >concurrent write access to other CVSROOT files, history, and val-tags.
> >I don't think deadlock will be an issue - despite the fact that any
> >given process writing to the history file or val-tags might hold any
> >number of other locks, the history or val-tags lock will always be
> >"last" in the chain and only held for the duration of the write of a
> >single line to the file.  Does anyone see any problems with that design?
>
> >Since file corruption is the current consequence of not having these
> >locks, I thought this would be justified on stable.  Any objections?
>
>
> I have no objections. (I have seen such corrptions and as a result have
> reduced the amount of history being logged in favor of custom hacks
> using syslog().)


Hrm.  That's interesting in some respects...  syslog would already
handle a bunch of the functionality I've been considering adding, but I
think it would be harder to hook into the `cvs history' command.  :(

> >The other set of changes I was considering would be to enable a number
> >of new keywords for the config file to allow CVS to search for config
> >files in new locations.  I would not be changing the default locations,
> >but the new setup would enable something like the following to be
> >specified in CVSROOT/config:
>
> >  CheckoutListFile  $CVSROOT/CVSROOT/admin/checkoutlist
> >  CommitInfoFile    $CVSROOT/CVSROOT/triggers/commitinfo
> >  CVSIgnoreFile     $CVSROOT/CVSROOT/cvsignore
> >  CVSWrappersFile   $CVSROOT/CVSROOT/admin/cvswrappers
> >  EditInfoFile      $CVSROOT/CVSROOT/triggers/editinfo
> >  HistoryFile       $CVSROOT/CVSROOT/logs/history/%Y%m%d
> >  HistorySearch     $CVSROOT/CVSROOT/logs/history/*
> >  LogInfoFile       $CVSROOT/CVSROOT/triggers/loginfo
> >  ModulesFile       $CVSROOT/CVSROOT/modules
> >  NotifyFile        $CVSROOT/CVSROOT/triggers/notify
> >  PasswdFile        $CVSROOT/CVSROOT/admin/passwd
> >  RCSInfoFile       $CVSROOT/CVSROOT/triggers/rcsinfo
> >  ReadersFile       $CVSROOT/CVSROOT/admin/readers
> >  TagInfoFile       $CVSROOT/CVSROOT/triggers/taginfo
> >  ValTagsFile       $CVSROOT/CVSROOT/admin/val-tags
> >  VerifyMsgFile     $CVSROOT/CVSROOT/triggers/verifymsg
> >  WritersFile       $CVSROOT/CVSROOT/admin/writers
>
>
> Please do not break the assumption of <keyword>=<value>
> in the CVSROOT/config file. So, the above would need to
> look closer to this:


I won't.  My apologies for cutting and pasting the list above out of
context from an email I received.

> CheckoutListFile=$CVSROOT/CVSROOT/admin/checkoutlist
> CommitInfoFile=$CVSROOT/CVSROOT/triggers/commitinfo
> CVSIgnoreFile=$CVSROOT/CVSROOT/cvsignore
> CVSWrappersFile=$CVSROOT/CVSROOT/admin/cvswrappers
> EditInfoFile=$CVSROOT/CVSROOT/triggers/editinfo
> HistoryFile=$CVSROOT/CVSROOT/logs/history/%Y%m%d
> HistorySearch=$CVSROOT/CVSROOT/logs/history/*
> LogInfoFile=$CVSROOT/CVSROOT/triggers/loginfo
> ModulesFile=$CVSROOT/CVSROOT/modules
> NotifyFile=$CVSROOT/CVSROOT/triggers/notify
> PasswdFile=$CVSROOT/CVSROOT/admin/passwd
> RCSInfoFile=$CVSROOT/CVSROOT/triggers/rcsinfo
> ReadersFile=$CVSROOT/CVSROOT/admin/readers
> TagInfoFile=$CVSROOT/CVSROOT/triggers/taginfo
> ValTagsFile=$CVSROOT/CVSROOT/admin/val-tags
> VerifyMsgFile=$CVSROOT/CVSROOT/triggers/verifymsg
> WritersFile=$CVSROOT/CVSROOT/admin/writers
>
> Am I correct in assuming that this change also assumes handling
> expansions of internal CVS variables like $CVSROOT is being added to to
> CVSROOT/config processing and that those are NOT general purpose
> environment variables being added?


Yes.  I'll probably hook into the same function used to do this by the
trigger scripts, expand_path().  This should enable the same config file
to be used in multiple CVS roots as well.  An associated change I was
putting off talking about was adding a global `-c <config_file>' option
to cause CVS to look elsewhere for its configuration file.

> >Note that HistoryFile has an argument that would basically be run
> >through strftime, to enable log rotation.  Also see the HistorySearch,
> >which would be used as a file glob to locate history files to be read
> >for executions of the `cvs history' command.
>
>
> Hmmm.... I do not like the BSD glob(3) function and its introduction in
> CVS would not make sense given we already have POSIX fnmatch()
> available... It would be better to specify looking for history files in
> terms of fnmatch() semantics if that is what you intend to use to do the
> resolution.


That is what I meant.  I had thought that a "file glob" was not precise
and referred to a whole class of path expansion, using patterns like
"name.??.* ", and implemented by various functions including fnmatch.  I
use the term in the same way I might specify "regex" matching when I
don't see a need to be more precise (e.g. "basic regex", "extended
regex", "perl regex", etc.).  I completely intended and continue to
intend to use the POSIX fnmatch() we've already imported from GNULIB to
implement this matching and any similar matching in the future.

Regards,

Derek

signature.asc
Description: OpenPGP digital signature