Skip to content

Conversation

damoclark
Copy link

Hi @rfjakob

Thanks for sharing cshatag. PR implements goroutines to perform the hashing and updating of the xattrs.

I've made the following changes:

  1. Updated the man page including the new -jobs flag for specifying the number of goroutines to execute.
  2. Implemented a channel for the walker (or the iteration of the command arguments) to pass fn paths to the goroutines to process
  3. Updated the stats struct to use uint32 so they can be incremented atomically in the goroutines
  4. Introduced a signal handler so that the process can be terminated gracefully, and ensure that either both or neither xattrs are set on a file
  5. Introduced module northbright/iocopy that allows the io.copy call to be interrupted by the signal handler using Context
  6. Updated the test.sh script to handle macOS/BSD touch command syntax

I'm not new to system programming, but I am new to Go. Please run a careful eye over my code.

If you would like to merge, please make any changes/improvements you feel necessary.

Hope this PR proves useful.

Cheers,
Damien.

Copy link
Owner

@rfjakob rfjakob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for the PR!

I don't think we need the context thing. Bang you're dead is fine.

Benchmark results would be nice.

A test would be nice.

timechange uint32
outdated uint32
newfile uint32
ok uint32
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use 64-bit values, and use the new type atomic.Uint64 as ( https://pkg.go.dev/sync/atomic#Uint64 )

Comment on lines +42 to +48
type Queue chan string

var queue Queue

var ctx context.Context
var cancel context.CancelFunc

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new globals, please

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I don't think we need the context at all

../cshatag foo.txt > /dev/null
echo asd > foo.txt
touch --date="2023-04-16 20:56:16.585798400+02:00" foo.txt
touch $touch_date_switch "$timestamp2" foo.txt
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make this a separate commit that explains the problem you are solving here

Comment on lines +124 to +136
ctx, cancel = context.WithCancel(context.Background())
defer cancel()

// Set up signal handler
sigChan := make(chan os.Signal, 1)
signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM)

// Start signal watcher goroutine
go func() {
sig := <-sigChan
fmt.Fprintf(os.Stderr, "\nReceived signal: %s\n", sig)
cancel()
}()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need any of this. Bang you're dead works just fine for cshatag. Nothing bad happens if we write only one xattr.

@@ -31,16 +36,29 @@ var args struct {
qq bool
dryrun bool
fix bool
jobs int
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uint?

queue = make(chan string, args.jobs)
var wg sync.WaitGroup

for i := 1; i <= args.jobs; i++ {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please handle jobs=0. You can just exit with an error like "make -j0" does:

$ make -j0
make: the '-j' option requires a positive integer argument


2012-12-05
* C source code & man page published on Github
([commit](https://github.com/rfjakob/cshatag/commit/5ce7674ea3210fd0bb6b06a81ca8823e0664761a))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't delete the readme :)

cancel()
}()

queue = make(chan string, args.jobs)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the queue depth here. Does it get faster with a deeper queue?

Otherwise I would just do

queue = make(chan string)

Copy link
Owner

@rfjakob rfjakob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, there's a race condition somewhere. I did not immediately spot where.

With

fmt.Printf("stats.total=%d\n", stats.total)

before exit and running against the extracted https://cdn.kernel.org/pub/linux/kernel/v3.0/linux-3.0.tar.gz :

6 jakob@brikett:~/code/cshatag$ ./cshatag -recursive -qq -jobs 2 /tmp/linux-3.0
stats.total=36768
6 jakob@brikett:~/code/cshatag$ 
6 jakob@brikett:~/code/cshatag$ ./cshatag -recursive -qq -jobs 2 /tmp/linux-3.0
stats.total=36774
6 jakob@brikett:~/code/cshatag$ ./cshatag -recursive -qq -jobs 2 /tmp/linux-3.0
stats.total=36775
6 jakob@brikett:~/code/cshatag$ ./cshatag -recursive -qq -jobs 1 /tmp/linux-3.0
stats.total=36782
0 jakob@brikett:~/code/cshatag$ ./cshatag -recursive -qq -jobs 1 /tmp/linux-3.0
stats.total=36782
0 jakob@brikett:~/code/cshatag$ ./cshatag -recursive -qq -jobs 2 /tmp/linux-3.0
stats.total=36764
6 jakob@brikett:~/code/cshatag$ ./cshatag -recursive -qq -jobs 2 /tmp/linux-3.0
stats.total=36765
6 jakob@brikett:~/code/cshatag$ ./cshatag -recursive -qq -jobs 2 /tmp/linux-3.0
stats.total=36765
6 jakob@brikett:~/code/cshatag$ ./cshatag -recursive -qq -jobs 2 /tmp/linux-3.0
stats.total=36767
6 jakob@brikett:~/code/cshatag$ ./cshatag -recursive -qq -jobs 8 /tmp/linux-3.0
stats.total=36768
6 jakob@brikett:~/code/cshatag$ ./cshatag -recursive -qq -jobs 8 /tmp/linux-3.0
stats.total=36765

@rfjakob
Copy link
Owner

rfjakob commented Jul 6, 2025

Oh, you did not change stats.total++ (and similar) in check.go to use atomics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants