Re: [PATCH][SMB3 client]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/15/2022 2:32 AM, Steve French wrote:
Change notification is a commonly supported feature by most servers,
and often used on other operating systems to detect when a file or
directory has changed and why,  but the current ioctl to request
notification when a directory is changed does not return the
information about what changed (even though it is returned by
the server in the SMB3 change notify response), it simply returns
when there is a change.

It's a useful enhancement to the API to see the details of the
notification. What Linux applications might use it, was this
requested by soneone?

I do have one general comment on the implementation. I am pasting
some snippets from the patch you attached.

The new structure adds two fields:

+struct smb3_notify_info {
+	__u32	completion_filter;
+	bool	watch_tree;
+	__u32   data_len; /* size of notify data below */
+	__u8	notify_data[];
+} __packed;
+

however the vector adds a single boolean:

 	int (*notify)(const unsigned int xid, struct file *pfile,
-			     void __user *pbuf);
+			     void __user *pbuf, bool return_changes);

Why a boolean? Wouldn't it be more logical, and general, to pass
the length instead? If zero, it's the existing behavior, and if
set to a length, then that many bytes max are requested?

It would also be logical to return the actual number of bytes returned,
so perhaps passing the length by reference.

 	int (*notify)(const unsigned int xid, struct file *pfile,
-			     void __user *pbuf);
+			     void __user *pbuf, int *length);

Finally, both the existing smb3_notify and the new smb3_notify_info
use a strange combination of native and size-specific types, packed
into a struct as if they were required to be wire-encoded. The __u32
packed with a bool, in both cases, and the __u32 length in the new
one. The bool could be anything, how does the "packed" attribute
actually work? Any why the inflexible __u32's?

struct smb3_notify {
	__u32	completion_filter;
	bool	watch_tree;
} __packed;


It seems to me that these structs should be defined in base
types and marshaled/unmarshaled in the smb3_notify processing.
Of course, the existing smb3_notify maybe needs to remain for
binary compatibility, but the new structure should not copy and
extend it.

Tom.

This ioctl improves upon CIFS_IOC_NOTIFY by returning the notify
information structure which includes the name of the file(s) that
changed and why. See MS-SMB2 2.2.35 and MS-FSCC 2.7.1 for details
on the individual filter flags and the file_notify_information
structure returned.

To use this simply pass in the following (with enough space
to fit at least one file_notify_information structure)

struct __attribute__((__packed__)) smb3_notify {
        uint32_t completion_filter;
        bool     watch_tree;
        uint32_t data_len;
        uint8_t  data[];
} __packed;

using CIFS_IOC_NOTIFY_INFO 0xc009cf0b
  or equivalently _IOWR(CIFS_IOCTL_MAGIC, 11, struct smb3_notify_info)

The ioctl will block until the server detects a change to that
directory or its subdirectories (if watch_tree is set). See attached.

Also attached is a simple sample program to demonstrate its use.  As an example
if you ran:
      # ./new-inotify-ioc-test /mnt-on-client1/subdir
it will block until a change occurs. If you then do a
      # touch /mnt-on-client2/subdir/newfile
you will see the following printed (in our sample program) showing the
notify information struct returned to the user
indicating the name of the file that was changed and what kind of change
    notify completed. returned data size is 28
    00000000:  00 00 00 00 03 00 00 00  0e 00 00 00 6e 00 65 00 ............n.e.
    00000010:  77 00 66 00 69 00 6c 00  65 00 00 00             w.f.i.l.e...





[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux