For next - 3 patches to review #15
Conversation
The function prototype of `calloc` is: void *calloc (size_t __nmemb, size_t __size) Fix the argument order to eliminate the build warning. Reported-by: Steve French <[email protected]> Signed-off-by: ChenXiaoSong <[email protected]>
When hurrying and scanning the documentation, my eye was drawn to the examples ``user%password`` or ``workgroup/user`` and ``workgroup/user%password``, especially because they were rendered in bold in my terminal. I didn't read them in the context of them being deprecated and experienced a non-zero amount of frustration when they didn't work. Given that these no longer work at all, remove them and add clarity. Signed-off-by: Martin Schwenke <[email protected]> Signed-off-by: Steve French <[email protected]>
Add `notify` subcommand to query a directory for change notifications. Example: ./smbinfo notify /mnt/dir # Then create a new file `/server/export/dir/file` on SMB server Notify completed, returned data_len is 20 00000000: 00 00 00 00 01 00 00 00 08 00 00 00 66 00 69 00 ............f.i. 00000010: 6c 00 65 00 l.e. Link: https://lore.kernel.org/linux-cifs/CAH2r5msHiZWzP5hdtPgb+wV3DL3J31RtgQRLQeuhCa_ULt3PfA@mail.gmail.com/ Suggested-by: Steve French <[email protected]> Signed-off-by: ChenXiaoSong <[email protected]> Signed-off-by: Steve French <[email protected]>
This reverts commit b06ae25. Newer version of patch now available
Add `notify` subcommand to query a directory for change notifications. Example: ./smbinfo notify /mnt/dir # Then create a new file `/server/export/dir/file` on SMB server Notify completed, returned data_len is 20 00000000: 00 00 00 00 01 00 00 00 08 00 00 00 66 00 69 00 ............f.i. 00000010: 6c 00 65 00 l.e. # Call `ioctl()` again Press `Ctrl+C` to exit `smbinfo`. Link: https://lore.kernel.org/linux-cifs/CAH2r5msHiZWzP5hdtPgb+wV3DL3J31RtgQRLQeuhCa_ULt3PfA@mail.gmail.com/ Suggested-by: Steve French <[email protected]> Signed-off-by: ChenXiaoSong <[email protected]> Signed-off-by: Steve French <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR introduces three patches to the cifs-utils suite: a new notification monitoring command for smbinfo, documentation improvements for mount.cifs, and a bug fix in cifs.upcall.
- Added notify command to smbinfo for monitoring directory change notifications
- Simplified and clarified mount.cifs documentation by removing deprecated username format information
- Fixed calloc parameter ordering in cifs.upcall.c
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| smbinfo.rst | Adds documentation for the new notify command |
| smbinfo | Implements the notify command with threading support for monitoring directory changes via CIFS_IOC_NOTIFY_INFO ioctl |
| mount.cifs.rst | Simplifies username option documentation by removing deprecated format information |
| cifs.upcall.c | Corrects calloc parameter order from (size, count) to (count, size) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
smbinfo
Outdated
| # padding | ||
| pad_len = BYTES_PER_LINE - len(chunk) | ||
| pad = " " * pad_len | ||
| if (pad_len >= 8): |
There was a problem hiding this comment.
The padding calculation has unnecessary parentheses around the condition. While not incorrect, the parentheses around 'pad_len >= 8' are redundant and reduce readability. Consider removing them for consistency with Python style.
| if (pad_len >= 8): | |
| if pad_len >= 8: |
smbinfo
Outdated
| try: | ||
| fd = os.open(args.file, os.O_RDONLY) | ||
| except Exception as e: | ||
| print("syscall failed: %s"%e) | ||
| return False |
There was a problem hiding this comment.
The notify command is documented as querying a directory, but there's no validation to ensure args.file is actually a directory before attempting the operation. Consider adding a check using os.stat and stat.S_ISDIR to provide a clearer error message if a non-directory is provided, similar to how other commands handle file type requirements.
smbinfo
Outdated
| try: | ||
| fd = os.open(args.file, os.O_RDONLY) | ||
| except Exception as e: | ||
| print("syscall failed: %s"%e) | ||
| return False |
There was a problem hiding this comment.
The file descriptor opened on line 931 is never closed. This creates a resource leak since the notify_thread function runs in an infinite loop (line 936) and only exits on error. The file descriptor should be closed before returning on error, and there should also be a mechanism to properly clean up the fd when the thread is interrupted.
| def cmd_notify(args): | ||
| thread = threading.Thread(target=notify_thread, args=(args,)) | ||
| thread.start() | ||
|
|
||
| try: | ||
| thread.join() | ||
| except KeyboardInterrupt: | ||
| return False |
There was a problem hiding this comment.
When KeyboardInterrupt is caught, the main thread returns but the notify_thread continues running indefinitely since it's blocked on the ioctl call. The thread should be created as a daemon thread (daemon=True) or use a proper signaling mechanism to ensure clean shutdown. Otherwise, the program may not exit cleanly and will leave the thread running.
This reverts commit b63b57d.
Add `notify` subcommand to query a directory for change notifications. Example: ./smbinfo notify /mnt/dir # Then create a new file `/server/export/dir/file` on SMB server Notify completed, returned data_len is 20 00000000: 00 00 00 00 01 00 00 00 08 00 00 00 66 00 69 00 ............f.i. 00000010: 6c 00 65 00 l.e. # Call `ioctl()` again Press `Ctrl+C` to exit `smbinfo`. Link: https://lore.kernel.org/linux-cifs/CAH2r5msHiZWzP5hdtPgb+wV3DL3J31RtgQRLQeuhCa_ULt3PfA@mail.gmail.com/ Suggested-by: Steve French <[email protected]> Signed-off-by: ChenXiaoSong <[email protected]> Signed-off-by: Steve French <[email protected]>
3 cifs-utils patches