PATCH: verbose mode for smbclient tar

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

PATCH: verbose mode for smbclient tar

Samba - samba-technical mailing list
hi,

we lost a verbose tar mode a while ago, here it comes again, please review and
push if happy with it.

Björn

tar-verbose.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: verbose mode for smbclient tar

Samba - samba-technical mailing list
isdir is uninitialized for all non-directory items.
isdir should be initialized and then used the if statement.

     isdir = finfo->mode & FILE_ATTRIBUTE_DIRECTORY;
     if (isdir) {
         old_dir = talloc_strdup(ctx, initial_dir);

On 1/3/18 17:02, Björn JACKE via samba-technical wrote:

> + bool isdir;
>   int rc;
>   TALLOC_CTX *ctx = talloc_new(NULL);
>   if (ctx == NULL) {
> @@ -846,7 +863,19 @@ static NTSTATUS get_file_callback(struct cli_state *cli,
>   goto out;
>   }
>  
> - status = tar_create_skip_path(&tar_ctx, remote_name, finfo, &skip);
> + if (finfo->mode & FILE_ATTRIBUTE_DIRECTORY) {
> + isdir = true;
> + old_dir = talloc_strdup(ctx, initial_dir);
> + new_dir = talloc_asprintf(ctx, "%s\\", remote_name);
> + if ((old_dir == NULL) || (new_dir == NULL)) {
> + status = NT_STATUS_NO_MEMORY;
> + goto out;
> + }
> + }


Reply | Threaded
Open this post in threaded view
|

Re: PATCH: verbose mode for smbclient tar

Samba - samba-technical mailing list
On 2018-01-03 at 17:17 -0500 Jim Brown via samba-technical sent off:
> isdir is uninitialized for all non-directory items.
> isdir should be initialized and then used the if statement.

thanks, attached fixed!

tar-verbose.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: verbose mode for smbclient tar

Samba - samba-technical mailing list
On 2018-01-03 at 23:43 +0100 Björn JACKE via samba-technical sent off:
> On 2018-01-03 at 17:17 -0500 Jim Brown via samba-technical sent off:
> > isdir is uninitialized for all non-directory items.
> > isdir should be initialized and then used the if statement.
>
> thanks, attached fixed!

one more with a minor documentation fix on this for the new v switch.

Bjoern

tar-verbose.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PATCH: verbose mode for smbclient tar

Samba - samba-technical mailing list
On Thu, Jan 04, 2018 at 12:20:58PM +0100, Björn JACKE via samba-technical wrote:

> On 2018-01-03 at 23:43 +0100 Björn JACKE via samba-technical sent off:
> > On 2018-01-03 at 17:17 -0500 Jim Brown via samba-technical sent off:
> > > isdir is uninitialized for all non-directory items.
> > > isdir should be initialized and then used the if statement.
> >
> > thanks, attached fixed!
>
> one more with a minor documentation fix on this for the new v switch.
>
> Bjoern

Bjoern,

The "normal" status messages produced from clitar should be printed
using d_printf() instead of a DBG() (which maps internally to a
DEBUG() statement).

I know clitar.c currently messes this up by mixing printf() and DBG() -> DEBUG()
but moving forward we should try and clean this up by moving printf() to d_printf()
and ensuring that only internal status messages (which may be part of the
'verbose' stream) go out using DEBUG().

If you don't have time for this now, let me know and I'll review the
patch as-is, but eventually we need to fix this up correctly (we can
do it as a patch on top if you prefer).

Cheers,

        Jeremy.

> From 13676b75394b83045ae3a7ca87273ee5c570eefa Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Bj=C3=B6rn=20Jacke?= <[hidden email]>
> Date: Wed, 3 Jan 2018 03:42:40 +0100
> Subject: [PATCH 1/2] smbclient/tar: add verbose mode
>
> A verbose mode got lost with the introduction of libarchive support.
>
> The verbose mode is optional, default is quiet mode.
>
> The output format is close to the verbose output format of POSIX tar
> implementations and should be good parsable.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=11642
>
> Signed-off-by: Bjoern Jacke <[hidden email]>
> ---
>  source3/client/client.c |  2 +-
>  source3/client/clitar.c | 86 +++++++++++++++++++++++++++++++++++--------------
>  2 files changed, 63 insertions(+), 25 deletions(-)
>
> diff --git a/source3/client/client.c b/source3/client/client.c
> index ad10a53..d90aa3f 100644
> --- a/source3/client/client.c
> +++ b/source3/client/client.c
> @@ -6317,7 +6317,7 @@ int main(int argc,char *argv[])
>   { "stderr", 'E', POPT_ARG_NONE, NULL, 'E', "Write messages to stderr instead of stdout" },
>   { "list", 'L', POPT_ARG_STRING, NULL, 'L', "Get a list of shares available on a host", "HOST" },
>   { "max-protocol", 'm', POPT_ARG_STRING, NULL, 'm', "Set the max protocol level", "LEVEL" },
> - { "tar", 'T', POPT_ARG_STRING, NULL, 'T', "Command line tar", "<c|x>IXFqgbNan" },
> + { "tar", 'T', POPT_ARG_STRING, NULL, 'T', "Command line tar", "<c|x>IXFvgbNan" },
>   { "directory", 'D', POPT_ARG_STRING, NULL, 'D', "Start from directory", "DIR" },
>   { "command", 'c', POPT_ARG_STRING, &cmdstr, 'c', "Execute semicolon separated commands" },
>   { "send-buffer", 'b', POPT_ARG_INT, &io_bufsize, 'b', "Changes the transmit/send buffer", "BYTES" },
> diff --git a/source3/client/clitar.c b/source3/client/clitar.c
> index b8009c9..f3f60566 100644
> --- a/source3/client/clitar.c
> +++ b/source3/client/clitar.c
> @@ -213,6 +213,10 @@ static NTSTATUS is_subpath(const char *sub, const char *full,
>     bool *_subpath_match);
>  
>  /**
> + * counters
> + */
> +static int tar_numdir, tar_numfile;
> +/**
>   * tar_get_ctx - retrieve global tar context handle
>   */
>  struct tar *tar_get_ctx()
> @@ -342,7 +346,7 @@ int cmd_tar(void)
>  
>   ok = next_token_talloc(ctx, &cmd_ptr, &buf, NULL);
>   if (!ok) {
> - DBG(0, ("tar <c|x>[IXFbganN] [options] <tar file> [path list]\n"));
> + DBG(0, ("tar <c|x>[IXFbgvanN] [options] <tar file> [path list]\n"));
>   err = 1;
>   goto out;
>   }
> @@ -364,6 +368,7 @@ int cmd_tar(void)
>   err = 1;
>   goto out;
>   }
> + tar_numdir = tar_numfile = 0;
>  
>   rc = tar_process(&tar_ctx);
>   if (rc != 0) {
> @@ -517,7 +522,7 @@ int tar_parse_args(struct tar* t,
>   break;
>  
>   /* verbose */
> - case 'q':
> + case 'v':
>   t->mode.verbose = true;
>   break;
>  
> @@ -642,11 +647,14 @@ static int tar_create(struct tar* t)
>   int err = 0;
>   NTSTATUS status;
>   const char *mask;
> + struct timespec tp_start, tp_end;
>   TALLOC_CTX *ctx = talloc_new(NULL);
>   if (ctx == NULL) {
>   return 1;
>   }
>  
> + clock_gettime_mono(&tp_start);
> +
>   t->archive = archive_write_new();
>  
>   if (!t->mode.dry) {
> @@ -713,8 +721,14 @@ static int tar_create(struct tar* t)
>   }
>   }
>  
> + clock_gettime_mono(&tp_end);
> + DEBUG(0, ("tar: dumped %d files and %d directories\n",
> +         tar_numfile, tar_numdir));
> + DEBUG(0, ("Total bytes written: %"PRIu64" (%.1f MiB/s)\n",
> + t->total_size,
> + t->total_size/timespec_elapsed2(&tp_start, &tp_end)/1024/1024));
> +
>  out_close:
> - DBG(0, ("Total bytes received: %" PRIu64 "\n", t->total_size));
>  
>   if (!t->mode.dry) {
>   r = archive_write_close(t->archive);
> @@ -823,8 +837,11 @@ static NTSTATUS get_file_callback(struct cli_state *cli,
>  {
>   NTSTATUS status = NT_STATUS_OK;
>   char *remote_name;
> + char *old_dir;
> + char *new_dir;
>   const char *initial_dir = client_get_cur_dir();
>   bool skip = false;
> + bool isdir;
>   int rc;
>   TALLOC_CTX *ctx = talloc_new(NULL);
>   if (ctx == NULL) {
> @@ -846,7 +863,19 @@ static NTSTATUS get_file_callback(struct cli_state *cli,
>   goto out;
>   }
>  
> - status = tar_create_skip_path(&tar_ctx, remote_name, finfo, &skip);
> + isdir = finfo->mode & FILE_ATTRIBUTE_DIRECTORY;
> + if (isdir) {
> + old_dir = talloc_strdup(ctx, initial_dir);
> + new_dir = talloc_asprintf(ctx, "%s\\", remote_name);
> + if ((old_dir == NULL) || (new_dir == NULL)) {
> + status = NT_STATUS_NO_MEMORY;
> + goto out;
> + }
> + }
> +
> + status = tar_create_skip_path(&tar_ctx,
> +                              isdir ? new_dir : remote_name,
> +                              finfo, &skip);
>   if (!NT_STATUS_IS_OK(status)) {
>   goto out;
>   }
> @@ -857,18 +886,8 @@ static NTSTATUS get_file_callback(struct cli_state *cli,
>   goto out;
>   }
>  
> - if (finfo->mode & FILE_ATTRIBUTE_DIRECTORY) {
> - char *old_dir;
> - char *new_dir;
> + if (isdir) {
>   char *mask;
> -
> - old_dir = talloc_strdup(ctx, initial_dir);
> - new_dir = talloc_asprintf(ctx, "%s%s\\",
> -  initial_dir, finfo->name);
> - if ((old_dir == NULL) || (new_dir == NULL)) {
> - status = NT_STATUS_NO_MEMORY;
> - goto out;
> - }
>   mask = talloc_asprintf(ctx, "%s*", new_dir);
>   if (mask == NULL) {
>   status = NT_STATUS_NO_MEMORY;
> @@ -889,6 +908,7 @@ static NTSTATUS get_file_callback(struct cli_state *cli,
>   client_set_cur_dir(new_dir);
>   do_list(mask, TAR_DO_LIST_ATTR, get_file_callback, false, true);
>   client_set_cur_dir(old_dir);
> + ++tar_numdir;
>   } else {
>   rc = tar_get_file(&tar_ctx, remote_name, finfo);
>   if (rc != 0) {
> @@ -922,6 +942,7 @@ static int tar_get_file(struct tar *t,
>   int err = 0, r;
>   const bool isdir = finfo->mode & FILE_ATTRIBUTE_DIRECTORY;
>   TALLOC_CTX *ctx = talloc_new(NULL);
> +
>   if (ctx == NULL) {
>   return 1;
>   }
> @@ -969,25 +990,39 @@ static int tar_get_file(struct tar *t,
>  
>   archive_entry_set_size(entry, (int64_t)finfo->size);
>  
> - r = archive_write_header(t->archive, entry);
> - if (r != ARCHIVE_OK) {
> - DBG(0, ("Fatal: %s\n", archive_error_string(t->archive)));
> - err = 1;
> - goto out_entry;
> - }
> -
>   if (isdir) {
> + /* It's a directory just write a header */
> + r = archive_write_header(t->archive, entry);
> + if (r != ARCHIVE_OK) {
> + DBG(0, ("Fatal: %s\n", archive_error_string(t->archive)));
> + err = 1;
> + }
> + if (t->mode.verbose) {
> + DEBUG(0,("a %s\\\n", full_dos_path));
> + }
>   DBG(5, ("get_file skip dir %s\n", full_dos_path));
>   goto out_entry;
>   }
>  
>   status = cli_open(cli, full_dos_path, O_RDONLY, DENY_NONE, &remote_fd);
>   if (!NT_STATUS_IS_OK(status)) {
> - DBG(0,("%s opening remote file %s\n",
> + DEBUG(0,("%s opening remote file %s\n",
>   nt_errstr(status), full_dos_path));
>   goto out_entry;
>   }
>  
> + /* don't make tar file entry until after the file is open */
> + r = archive_write_header(t->archive, entry);
> + if (r != ARCHIVE_OK) {
> + DBG(0, ("Fatal: %s\n", archive_error_string(t->archive)));
> + err = 1;
> + goto out_entry;
> + }
> +
> + if (t->mode.verbose) {
> + DEBUG(0, ("a %s\n", full_dos_path));
> + }
> +
>   do {
>   status = cli_read(cli, remote_fd, buf, off, sizeof(buf), &len);
>   if (!NT_STATUS_IS_OK(status)) {
> @@ -1007,6 +1042,7 @@ static int tar_get_file(struct tar *t,
>   }
>  
>   } while (off < finfo->size);
> + ++tar_numfile;
>  
>  out_close:
>   cli_close(cli, remote_fd);
> @@ -1075,7 +1111,9 @@ static int tar_extract(struct tar *t)
>   continue;
>   }
>  
> - DBG(5, ("+++ %s\n", archive_entry_pathname(entry)));
> + if (t->mode.verbose) {
> + DEBUG(0, ("x %s\n", archive_entry_pathname(entry)));
> + }
>  
>   rc = tar_send_file(t, entry);
>   if (rc != 0) {
> --
> 2.7.4
>
>
> From 93f4073728b50cdfbd4faf138c24fe201d487660 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Bj=C3=B6rn=20Jacke?= <[hidden email]>
> Date: Wed, 3 Jan 2018 22:01:35 +0100
> Subject: [PATCH 2/2] docs-xml: document verbose mode of smbclient tar
>
> Signed-off-by: Bjoern Jacke <[hidden email]>
> ---
>  docs-xml/manpages/smbclient.1.xml | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/docs-xml/manpages/smbclient.1.xml b/docs-xml/manpages/smbclient.1.xml
> index a5d4808..65b029b 100644
> --- a/docs-xml/manpages/smbclient.1.xml
> +++ b/docs-xml/manpages/smbclient.1.xml
> @@ -69,7 +69,7 @@
>   <arg choice="opt">-R &lt;name resolve order&gt;</arg>
>   <arg choice="opt">-s &lt;smb config file&gt;</arg>
>   <arg choice="opt">-t &lt;per-operation timeout in seconds&gt;</arg>
> - <arg choice="opt">-T&lt;c|x&gt;IXFqgbNan</arg>
> + <arg choice="opt">-T&lt;c|x&gt;IXFvgbNan</arg>
>   <arg choice="opt">-k</arg>
>   </cmdsynopsis>
>  </refsynopsisdiv>
> @@ -406,8 +406,9 @@
>   files that have the archive bit set. Useful only with the
>   <parameter>c</parameter> flag. </para></listitem>
>  
> - <listitem><para><parameter>q</parameter> - Quiet. Keeps tar from printing
> - diagnostics as it works.  This is the same as tarmode quiet.
> + <listitem><para><parameter>v</parameter> - Verbose. Makes tar
> + print out the files being processed. By default tar is not verbose.
> + This is the same as tarmode verbose.
>   </para></listitem>
>  
>   <listitem><para><parameter>r</parameter> - Use wildcard
> @@ -1095,7 +1096,7 @@
>   </varlistentry>
>  
>   <varlistentry>
> - <term>tarmode &lt;full|inc|reset|noreset|system|nosystem|hidden|nohidden&gt;</term>
> + <term>tarmode &lt;full|inc|reset|noreset|system|nosystem|hidden|nohidden|quiet|verbose&gt;</term>
>   <listitem><para>Changes tar's behavior with regard to DOS
>   attributes. There are 4 modes which can be turned on or
>   off.</para>
> --
> 2.7.4
>


Reply | Threaded
Open this post in threaded view
|

Re: PATCH: verbose mode for smbclient tar

Samba - samba-technical mailing list
Hi Jeremy,
On 04.01.2018 21:50, Jeremy Allison wrote:
> I know clitar.c currently messes this up by mixing printf() and DBG() -> DEBUG()
> but moving forward we should try and clean this up by moving printf() to d_printf()
> and ensuring that only internal status messages (which may be part of the
> 'verbose' stream) go out using DEBUG().
>
> If you don't have time for this now, let me know and I'll review the
> patch as-is, but eventually we need to fix this up correctly (we can
> do it as a patch on top if you prefer).

here is the updated patch using d_printf at the places that this patch
touches.

Björn

tar-verbose.patch (9K) Download Attachment