Libguestfs - Mar 2016 - [PATCH 0/3] rename icat API into download_inode

The command name is a bit confusing because it's similar to "cat"
but act as "donwload". Therefore I am renaming it with a more clear
name.

At the same go I cleaned up a bit the code following the standards and improved
the API documentation.

This patch is ready for review.
Code available at:

https://github.com/noxdafox/libguestfs/tree/download_inode

Matteo Cafasso (3):
  Rename icat command in download_inode
  Improve download_inode documentation string.
  Code cleanup in daemon/sleuthkit.c

 daemon/sleuthkit.c               | 50 ++++++++++++++++++++-----------------
 generator/actions.ml             | 10 +++++---
 tests/tsk/Makefile.am            |  2 +-
 tests/tsk/test-download-inode.sh | 53 ++++++++++++++++++++++++++++++++++++++++
 tests/tsk/test-icat.sh           | 53 ----------------------------------------
 5 files changed, 88 insertions(+), 80 deletions(-)
 create mode 100755 tests/tsk/test-download-inode.sh
 delete mode 100755 tests/tsk/test-icat.sh

--
2.8.0.rc3

The "icat" name comes from the employed command line tool which might
be
replaced at any time with a different implementation.

The command name is a bit confusing because it's similar to "cat"
but
act as "donwload".

download_inode is more clear and descriptive.

Signed-off-by: Matteo Cafasso <noxdafox@gmail.com>
---
 daemon/sleuthkit.c               |  2 +-
 generator/actions.ml             |  2 +-
 tests/tsk/Makefile.am            |  2 +-
 tests/tsk/test-download-inode.sh | 53 ++++++++++++++++++++++++++++++++++++++++
 tests/tsk/test-icat.sh           | 53 ----------------------------------------
 5 files changed, 56 insertions(+), 56 deletions(-)
 create mode 100755 tests/tsk/test-download-inode.sh
 delete mode 100755 tests/tsk/test-icat.sh

diff --git a/daemon/sleuthkit.c b/daemon/sleuthkit.c
index 0fe1250..2f8c97b 100644
--- a/daemon/sleuthkit.c
+++ b/daemon/sleuthkit.c
@@ -40,7 +40,7 @@ optgroup_sleuthkit_available (void)
 }

 int
-do_icat (const mountable_t *mountable, int64_t inode)
+do_download_inode (const mountable_t *mountable, int64_t inode)
 {
   CLEANUP_FREE char *cmd = NULL;

diff --git a/generator/actions.ml b/generator/actions.ml
index ff72cfe..e5cb939 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -12946,7 +12946,7 @@ The filesystem from which to extract the file must be
unmounted,
 otherwise the call will fail." };

   { defaults with
-    name = "icat"; added = (1, 33, 14);
+    name = "download_inode"; added = (1, 33, 14);
     style = RErr, [Mountable "device"; Int64 "inode";
FileOut "filename"], [];
     proc_nr = Some 464;
     optional = Some "sleuthkit";
diff --git a/tests/tsk/Makefile.am b/tests/tsk/Makefile.am
index e060e58..0cd7c03 100644
--- a/tests/tsk/Makefile.am
+++ b/tests/tsk/Makefile.am
@@ -18,7 +18,7 @@
 include $(top_srcdir)/subdir-rules.mk

 TESTS = \
-	test-icat.sh
+	test-download-inode.sh

 TESTS_ENVIRONMENT = $(top_builddir)/run --test

diff --git a/tests/tsk/test-download-inode.sh b/tests/tsk/test-download-inode.sh
new file mode 100755
index 0000000..9c65aa9
--- /dev/null
+++ b/tests/tsk/test-download-inode.sh
@@ -0,0 +1,53 @@
+#!/bin/bash -
+# libguestfs
+# Copyright (C) 2016 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Test the download_inode command.
+
+set -e
+
+if [ -n "$SKIP_TEST_DOWNLOAD_INODE_SH" ]; then
+    echo "$0: test skipped because environment variable is set."
+    exit 77
+fi
+
+rm -f test-mft.bin
+
+# Skip if TSK is not supported by the appliance.
+if ! guestfish add /dev/null : run : available "sleuthkit"; then
+    echo "$0: skipped because TSK is not available in the appliance"
+    exit 77
+fi
+
+if [ ! -s ../../test-data/phony-guests/windows.img ]; then
+    echo "$0: skipped because windows.img is zero-sized"
+    exit 77
+fi
+
+# download Master File Table ($MFT).
+guestfish --ro -a ../../test-data/phony-guests/windows.img <<EOF
+run
+download-inode /dev/sda2 0 test-mft.bin
+EOF
+
+# test extracted file is the Master File Table
+if [ `head -c 5 test-mft.bin` != "FILE0" ]; then
+    echo "$0: wrong file extracted."
+    exit 1
+fi
+
+rm -f test-mft.bin
diff --git a/tests/tsk/test-icat.sh b/tests/tsk/test-icat.sh
deleted file mode 100755
index 3b0fca4..0000000
--- a/tests/tsk/test-icat.sh
+++ /dev/null
@@ -1,53 +0,0 @@
-#!/bin/bash -
-# libguestfs
-# Copyright (C) 2016 Red Hat Inc.
-#
-# This program is free software; you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation; either version 2 of the License, or
-# (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program; if not, write to the Free Software
-# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
-
-# Test the icat command.
-
-set -e
-
-if [ -n "$SKIP_TEST_ICAT_SH" ]; then
-    echo "$0: test skipped because environment variable is set."
-    exit 77
-fi
-
-rm -f test-mft.bin
-
-# Skip if TSK is not supported by the appliance.
-if ! guestfish add /dev/null : run : available "sleuthkit"; then
-    echo "$0: skipped because TSK is not available in the appliance"
-    exit 77
-fi
-
-if [ ! -s ../../test-data/phony-guests/windows.img ]; then
-    echo "$0: skipped because windows.img is zero-sized"
-    exit 77
-fi
-
-# download Master File Table ($MFT).
-guestfish --ro -a ../../test-data/phony-guests/windows.img <<EOF
-run
-icat /dev/sda2 0 test-mft.bin
-EOF
-
-# test extracted file is the Master File Table
-if [ `head -c 5 test-mft.bin` != "FILE0" ]; then
-    echo "$0: wrong file extracted."
-    exit 1
-fi
-
-rm -f test-mft.bin
--
2.8.0.rc3

The download_inode does not require the disk to be mounted
in order to work.

Signed-off-by: Matteo Cafasso <noxdafox@gmail.com>
---
 generator/actions.ml | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/generator/actions.ml b/generator/actions.ml
index e5cb939..0eb11fe 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -12953,10 +12953,12 @@ otherwise the call will fail." };
     progress = true; cancellable = true;
     shortdesc = "download a file to the local machine given its
inode";
     longdesc = "\
-Download a file given its inode from the disk partition (eg.
F</dev/sda1>)
-and save it as F<filename> on the local machine.
+Download a file given its inode from the disk partition
+(eg. F</dev/sda1>) and save it as F<filename> on the local machine.

-This allows to download deleted or inaccessible files." };
+It is not required to mount the disk to run this command.
+
+The command is capable of downloading deleted or inaccessible files." };

 ]

--
2.8.0.rc3

Adhere to coding standards.

Signed-off-by: Matteo Cafasso <noxdafox@gmail.com>
---
 daemon/sleuthkit.c | 48 +++++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/daemon/sleuthkit.c b/daemon/sleuthkit.c
index 2f8c97b..1262b68 100644
--- a/daemon/sleuthkit.c
+++ b/daemon/sleuthkit.c
@@ -29,19 +29,14 @@
 #include "actions.h"
 #include "optgroups.h"

-static int file_out (const char *cmd);
+static int send_command_output (const char *cmd);

-GUESTFSD_EXT_CMD(str_sleuthkit_probe, icat);
-
-int
-optgroup_sleuthkit_available (void)
-{
-  return prog_exists (str_sleuthkit_probe);
-}
+GUESTFSD_EXT_CMD(str_icat, icat);

 int
 do_download_inode (const mountable_t *mountable, int64_t inode)
 {
+  int ret;
   CLEANUP_FREE char *cmd = NULL;

   /* Inode must be greater than 0 */
@@ -51,18 +46,22 @@ do_download_inode (const mountable_t *mountable, int64_t
inode)
   }

   /* Construct the command. */
-  if (asprintf (&cmd, "icat -r %s %" PRIi64,
mountable->device, inode) == -1) {
+  ret = asprintf(&cmd, "%s -r %s %" PRIi64, str_icat,
mountable->device, inode);
+  if (ret < 0) {
     reply_with_perror ("asprintf");
     return -1;
   }

-  return file_out (cmd);
+  return send_command_output (cmd);
 }

+/* Run the given command, collect the output and send it to the appliance.
+ * Return 0 on success, -1 on error.
+ */
 static int
-file_out (const char *cmd)
+send_command_output (const char *cmd)
 {
-  int r;
+  int ret;
   FILE *fp;
   CLEANUP_FREE char *buffer = NULL;

@@ -81,34 +80,41 @@ file_out (const char *cmd)
     return -1;
   }

-  /* Now we must send the reply message, before the file contents.  After
-   * this there is no opportunity in the protocol to send any error
-   * message back.  Instead we can only cancel the transfer.
-   */
+  /* Send reply message before the file content. */
   reply (NULL, NULL);

-  while ((r = fread (buffer, 1, sizeof buffer, fp)) > 0) {
-    if (send_file_write (buffer, r) < 0) {
+  while ((ret = fread (buffer, 1, sizeof buffer, fp)) > 0) {
+    ret = send_file_write (buffer, ret);
+    if (ret < 0) {
       pclose (fp);
       return -1;
     }
   }

-  if (ferror (fp)) {
+  ret = ferror (fp);
+  if (ret != 0) {
     fprintf (stderr, "fread: %m");
     send_file_end (1);		/* Cancel. */
     pclose (fp);
     return -1;
   }

-  if (pclose (fp) != 0) {
+  ret = pclose (fp);
+  if (ret != 0) {
     fprintf (stderr, "pclose: %m");
     send_file_end (1);		/* Cancel. */
     return -1;
   }

-  if (send_file_end (0))	/* Normal end of file. */
+  ret = send_file_end (0);      /* Normal end of file. */
+  if (ret != 0)
     return -1;

   return 0;
 }
+
+int
+optgroup_sleuthkit_available (void)
+{
+  return prog_exists (str_icat);
+}
--
2.8.0.rc3

On Wednesday 30 March 2016 22:18:08 Matteo Cafasso
wrote:
> The "icat" name comes from the employed command line tool which
might be
> replaced at any time with a different implementation.
> 
> The command name is a bit confusing because it's similar to
"cat" but
> act as "donwload".
small typo..^
> download_inode is more clear and descriptive.
> 
> Signed-off-by: Matteo Cafasso <noxdafox@gmail.com>
> ---
LGTM -- fixed the typo in the commit message directly while applying
it.

Thanks,
-- 
Pino Toscano