[PATCH] libsmbclient: readdirplus call.

[Alexander Bokovoy]

On to, 03 touko 2018, Jeremy Allison wrote:
> On Tue, Apr 24, 2018 at 01:51:32PM -0700, Jeremy Allison wrote:
> > Now I'm back from vacation, I (finally, sorry Puran :-) got
> > the chance to code up a working test suite for the new
> > readdirplus code.
> > 
> > Readdirplus patch already tested by Puran, the original
> > author. Just the last patch (test coverage) is new.
> > 
> > Please review and push if happy !
> 
> Ping again. Can I get a second Team reviewer for this (CC:ing ab
> as he seems to be being kind at the moment :-).
:)

I have few comments where I'd like to understand whether it is by
design or a copy/paste mistake.


> 
> Cheers,
> 
> 	Jeremy.
> 
> > From d482f05e385800c6c6bf31a066e1f4f4ba399927 Mon Sep 17 00:00:00 2001
> > From: Jeremy Allison <jra at samba.org>
> > Date: Fri, 6 Apr 2018 13:35:05 -0700
> > Subject: [PATCH 1/6] s3: client: Add btime_ts to struct finfo.
> > 
> > Fill it in when available, else return it as zero.
> > 
> > Based on a patch from Puran Chand <pchand at vmware.com>.
> > 
> > Signed-off-by: Jeremy Allison <jra at samba.org>
> > ---
> >  examples/fuse/clifuse.c        |  1 +
> >  source3/include/client.h       |  1 +
> >  source3/libsmb/cli_smb2_fnum.c |  1 +
> >  source3/libsmb/clilist.c       | 19 +++++++++++++++++++
> >  4 files changed, 22 insertions(+)
> > 
> > diff --git a/examples/fuse/clifuse.c b/examples/fuse/clifuse.c
> > index da9dd4d3e82..3c7e4982475 100644
> > --- a/examples/fuse/clifuse.c
> > +++ b/examples/fuse/clifuse.c
> > @@ -484,6 +484,7 @@ static NTSTATUS parse_finfo_id_both_directory_info(uint8_t *dir_data,
> >  		return NT_STATUS_INFO_LENGTH_MISMATCH;
> >  	}
> >  
> > +	finfo->btime_ts = interpret_long_date((const char *)dir_data + 8);
This offset (8) is OK, according to MS-FSCC 2.4.8.

> >  	finfo->atime_ts = interpret_long_date((const char *)dir_data + 16);
> >  	finfo->mtime_ts = interpret_long_date((const char *)dir_data + 24);
> >  	finfo->ctime_ts = interpret_long_date((const char *)dir_data + 32);
> > diff --git a/source3/include/client.h b/source3/include/client.h
> > index 1fe3f1cb960..0cb21384f17 100644
> > --- a/source3/include/client.h
> > +++ b/source3/include/client.h
> > @@ -108,6 +108,7 @@ struct file_info {
> >  	uid_t uid;
> >  	gid_t gid;
> >  	/* these times are normally kept in GMT */
> > +	struct timespec btime_ts; /* Birth-time if supported by system */
> >  	struct timespec mtime_ts;
> >  	struct timespec atime_ts;
> >  	struct timespec ctime_ts;
> > diff --git a/source3/libsmb/cli_smb2_fnum.c b/source3/libsmb/cli_smb2_fnum.c
> > index c397b29b381..1eb1bea7a72 100644
> > --- a/source3/libsmb/cli_smb2_fnum.c
> > +++ b/source3/libsmb/cli_smb2_fnum.c
> > @@ -803,6 +803,7 @@ static NTSTATUS parse_finfo_id_both_directory_info(uint8_t *dir_data,
> >  		return NT_STATUS_INFO_LENGTH_MISMATCH;
> >  	}
> >  
> > +	finfo->btime_ts = interpret_long_date((const char *)dir_data + 8);
This offset (8) is OK, according to MS-FSCC 2.4.8.

> >  	finfo->atime_ts = interpret_long_date((const char *)dir_data + 16);
> >  	finfo->mtime_ts = interpret_long_date((const char *)dir_data + 24);
> >  	finfo->ctime_ts = interpret_long_date((const char *)dir_data + 32);
> > diff --git a/source3/libsmb/clilist.c b/source3/libsmb/clilist.c
> > index 41f585120b9..5cb1fce4338 100644
> > --- a/source3/libsmb/clilist.c
> > +++ b/source3/libsmb/clilist.c
> > @@ -77,6 +77,14 @@ static size_t interpret_long_filename(TALLOC_CTX *ctx,
> >  			if (pdata_end - base < 27) {
> >  				return pdata_end - base;
> >  			}
> > +			/*
> > +			 * What we're returning here as ctime_ts is
> > +			 * actually the server create time.
> > +			 */
> > +			finfo->btime_ts = convert_time_t_to_timespec(
> > +				make_unix_date2(p+4,
> > +					smb1cli_conn_server_time_zone(
> > +						cli->conn)));
> >  			finfo->ctime_ts = convert_time_t_to_timespec(
> >  				make_unix_date2(p+4, smb1cli_conn_server_time_zone(cli->conn)));
What about here? Is it intended to provide the same value as ctime_ts
instead of stating that we didn't get any birth time at all?
SMB_FIND_INFO_STANDARD level doesn't give you that information, unlike
SMB_FIND_FILE_BOTH_DIRECTORY_INFO.

> >  			finfo->atime_ts = convert_time_t_to_timespec(
> > @@ -128,6 +136,14 @@ static size_t interpret_long_filename(TALLOC_CTX *ctx,
> >  			if (pdata_end - base < 31) {
> >  				return pdata_end - base;
> >  			}
> > +			/*
> > +			 * What we're returning here as ctime_ts is
> > +			 * actually the server create time.
> > +			 */
> > +			finfo->btime_ts = convert_time_t_to_timespec(
> > +				make_unix_date2(p+4,
> > +					smb1cli_conn_server_time_zone(
> > +						cli->conn)));
> >  			finfo->ctime_ts = convert_time_t_to_timespec(
> >  				make_unix_date2(p+4, smb1cli_conn_server_time_zone(cli->conn)));
Same here.

> >  			finfo->atime_ts = convert_time_t_to_timespec(
> > @@ -250,6 +266,9 @@ static bool interpret_short_filename(TALLOC_CTX *ctx,
> >  
> >  	finfo->mode = CVAL(p,21);
> >  
> > +	/* We don't get birth time. */
> > +	finfo->btime_ts.tv_sec = 0;
> > +	finfo->btime_ts.tv_nsec = 0;
So here we default to 0 but in the cases above we don't default to zero.
It is confusing and I'd like you to explain why we have different
defaults.

-- 
/ Alexander Bokovoy