Libguestfs - Mar 2016 - [PATCH 0/2] added filesystem_walk0 low level API

The filesystem_walk0 API parses the FS internals of a partition and returns a
list of all the files and directories contained within. It list deleted files
and directories as well.

For each node, it reports its relative path, its inode and its allocation
status.

The output is serialised in XDR format and written to the given file.

The command is similar to The Sleuth Kit "fls -rp <disk_image>"
except from the fact that it does not report the node type (file or directory).

Matteo Cafasso (2):
  added filesystem_walk0 API
  added filesystem_walk0 API tests

 daemon/Makefile.am                 |   3 +-
 daemon/tsk.c                       | 186 +++++++++++++++++++++++++++++++++++++
 generator/actions.ml               |  16 ++++
 m4/guestfs_daemon.m4               |   8 ++
 src/MAX_PROC_NR                    |   2 +-
 tests/tsk/Makefile.am              |   3 +-
 tests/tsk/test-filesystem-walk0.sh |  54 +++++++++++
 7 files changed, 269 insertions(+), 3 deletions(-)
 create mode 100755 tests/tsk/test-filesystem-walk0.sh

--
2.8.0.rc3

Signed-off-by: Matteo Cafasso <noxdafox@gmail.com>
---
 daemon/Makefile.am   |   3 +-
 daemon/tsk.c         | 186 +++++++++++++++++++++++++++++++++++++++++++++++++++
 generator/actions.ml |  16 +++++
 m4/guestfs_daemon.m4 |   8 +++
 src/MAX_PROC_NR      |   2 +-
 5 files changed, 213 insertions(+), 2 deletions(-)

diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index 4e2051b..036def9 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -209,7 +209,8 @@ guestfsd_LDADD = \
 	$(LIB_CLOCK_GETTIME) \
 	$(LIBINTL) \
 	$(SERVENT_LIB) \
-	$(PCRE_LIBS)
+	$(PCRE_LIBS) \
+	$(TSK_LIBS)

 guestfsd_CPPFLAGS = \
 	-I$(top_srcdir)/gnulib/lib \
diff --git a/daemon/tsk.c b/daemon/tsk.c
index b84dfae..d72868e 100644
--- a/daemon/tsk.c
+++ b/daemon/tsk.c
@@ -29,6 +29,26 @@
 #include "actions.h"
 #include "optgroups.h"

+#ifdef HAVE_LIBTSK
+
+#include <tsk/libtsk.h>
+#include <rpc/xdr.h>
+#include <rpc/types.h>
+
+static int
+open_filesystem(const char *device, TSK_IMG_INFO **img, TSK_FS_INFO **fs);
+static TSK_WALK_RET_ENUM
+fswalk_callback(TSK_FS_FILE *fsfile, const char *path, void *data);
+static char *join_path(const char *path, const char *name);
+static int inode_out(guestfs_int_tsk_node *node_info);
+static void reply_with_tsk_error(void);
+
+#else
+
+OPTGROUP_LIBTSK_NOT_AVAILABLE
+
+#endif
+
 static int file_out (const char *cmd);
 static guestfs_int_tsk_node* parse_ffind (const char *out, int64_t inode);

@@ -226,3 +246,169 @@ file_out (const char *cmd)

   return 0;
 }
+
+#ifdef HAVE_LIBTSK
+
+
+int optgroup_libtsk_available(void)
+{
+  return 1;
+}
+
+int do_filesystem_walk0(const mountable_t *mountable)
+{
+  int ret = 0;
+  TSK_FS_INFO *fs = NULL;
+  TSK_IMG_INFO *img = NULL;
+  int flags = TSK_FS_DIR_WALK_FLAG_ALLOC | TSK_FS_DIR_WALK_FLAG_UNALLOC |
+    TSK_FS_DIR_WALK_FLAG_RECURSE | TSK_FS_DIR_WALK_FLAG_NOORPHAN;
+
+  if (open_filesystem(mountable->device, &img, &fs) < 0)
+    return -1;
+
+  reply(NULL, NULL);  /* Reply message. */
+
+  if (tsk_fs_dir_walk(fs, fs->root_inum, flags, fswalk_callback, &ret)
!= 0) {
+    send_file_end(1);	/* Cancel file transfer. */
+    reply_with_tsk_error();
+
+    ret = -1;
+  }
+  else {
+    if (send_file_end(0))  /* Normal end of file. */
+      ret = -1;
+
+    ret = 0;
+  }
+
+  fs->close(fs);
+  img->close(img);
+
+  return ret;
+}
+
+/* Inspect the device and initialises the img and fs structures.
+ * Return 0 on success, -1 on error.
+ */
+static int
+open_filesystem(const char *device, TSK_IMG_INFO **img, TSK_FS_INFO **fs)
+{
+  const char *images[] = { device };
+
+  if ((*img = tsk_img_open(1, images, TSK_IMG_TYPE_DETECT , 0)) == NULL) {
+    reply_with_tsk_error();
+
+    return -1;
+  }
+
+  if ((*fs = tsk_fs_open_img(*img, 0, TSK_FS_TYPE_DETECT)) == NULL) {
+    reply_with_tsk_error();
+
+    (*img)->close(*img);
+
+    return -1;
+  }
+
+  return 0;
+}
+
+/* Filesystem walk callback, it gets called on every FS node.
+ * Parse the node, encode it into an XDR structure and send it to the
appliance.
+ * Return 0 on success, -1 on error.
+ */
+static TSK_WALK_RET_ENUM
+fswalk_callback(TSK_FS_FILE *fsfile, const char *path, void *data)
+{
+  CLEANUP_FREE char *file_name = NULL;
+  struct guestfs_int_tsk_node node_info;
+
+  /* Ignore ./ and ../ */
+  if (TSK_FS_ISDOT(fsfile->name->name))
+    return 0;
+
+  if ((file_name = join_path(path, fsfile->name->name)) == NULL)
+    return -1;
+
+  node_info.tsk_name = file_name;
+  node_info.tsk_inode = fsfile->name->meta_addr;
+  if (fsfile->name->flags & TSK_FS_NAME_FLAG_UNALLOC)
+    node_info.tsk_allocated = 0;
+  else
+    node_info.tsk_allocated = 1;
+
+  return inode_out(&node_info);
+}
+
+/* Joins the file name and file path.
+ * Return the joined path on success, NULL on failure.
+ */
+static char *join_path(const char *path, const char *name)
+{
+  char *buf;
+  size_t len;
+
+  path = (path != NULL) ? path : "";
+  len = strlen(path) + strlen(name) + 1;
+
+  if (!(buf = malloc(len))) {
+    reply_with_perror("malloc");
+    return NULL;
+  }
+
+  snprintf(buf, len, "%s%s", path, name);
+
+  return buf;
+}
+
+/* Serialise node_info into XDR stream and send it to the appliance.
+ * Return 0 on success, -1 on error.
+ */
+static int inode_out(guestfs_int_tsk_node *node_info)
+{
+  XDR xdr;
+  size_t len;
+  CLEANUP_FREE char *buf;
+
+  if ((buf = malloc(GUESTFS_MAX_CHUNK_SIZE)) == NULL) {
+    reply_with_perror ("malloc");
+    return -1;
+  }
+
+  /* Serialise tsk_node struct. */
+  len = strlen(node_info->tsk_name) + 1;
+
+  xdrmem_create(&xdr, buf, GUESTFS_MAX_CHUNK_SIZE, XDR_ENCODE);
+  if (!xdr_u_long(&xdr, &len))
+    return -1;
+  if (!xdr_string(&xdr, &node_info->tsk_name, len))
+    return -1;
+  if (!xdr_uint64_t(&xdr, &node_info->tsk_inode))
+    return -1;
+  if (!xdr_uint32_t(&xdr, &node_info->tsk_allocated))
+    return -1;
+
+  /* Resize buffer to actual length. */
+  len = xdr_getpos(&xdr);
+  xdr_destroy(&xdr);
+  if ((buf = realloc(buf, len)) == NULL)
+    return -1;
+
+  /* Send serialised tsk_node out. */
+  if (send_file_write(buf, len) < 0)
+    return -1;
+
+  return 0;
+}
+
+/* Parse TSK error and send it to the appliance. */
+static void reply_with_tsk_error(void)
+{
+  const char *buf = NULL;
+
+  if (tsk_error_get_errno() != 0) {
+    buf = tsk_error_get();
+    reply_with_error("TSK error: %s", buf);
+  }
+}
+
+#endif /* !HAVE_LIBTSK */
diff --git a/generator/actions.ml b/generator/actions.ml
index 2d291fb..e33655a 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -13015,6 +13015,22 @@ given its inode.

 On some filesystem, it can find deleted files." };

+  { defaults with
+    name = "filesystem_walk0"; added = (1, 33, 16);
+    style = RErr, [Mountable "device"; FileOut
"filename";], [];
+    proc_nr = Some 468;
+    optional = Some "libtsk";
+    progress = true; cancellable = true;
+    shortdesc = "walks through the filesystem content";
+    longdesc = "\
+This specialised command is used to walk through the internal structures
+of a disk partition (eg. F</dev/sda1>) in order to return a list of all
+the files and directories stored within.
+
+For each file or directory found, the function reports its path, its inode and
whether
+it is allocated or not.
+
+The output is stored in F<filename> on the host encoded in XDR
format." };
 ]

 (* Non-API meta-commands available only in guestfish.
diff --git a/m4/guestfs_daemon.m4 b/m4/guestfs_daemon.m4
index 88936b2..09cfecd 100644
--- a/m4/guestfs_daemon.m4
+++ b/m4/guestfs_daemon.m4
@@ -118,3 +118,11 @@ PKG_CHECK_MODULES([SD_JOURNAL], [libsystemd],[
         AC_MSG_WARN([systemd journal library not found, some features will be
disabled])
     ])
 ])
+
+dnl libtsk sleuthkit library (optional)
+AC_CHECK_LIB([tsk],[tsk_version_print],[
+    AC_CHECK_HEADER([tsk/libtsk.h],[
+        AC_SUBST([TSK_LIBS], [-ltsk])
+        AC_DEFINE([HAVE_LIBTSK], [1], [Define to 1 if The Sleuth Kit  library
(libtsk) is available.])
+    ], [])
+],[AC_MSG_WARN([The Sleuth Kit library (libtsk) not found])])
diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR
index 5873851..1023289 100644
--- a/src/MAX_PROC_NR
+++ b/src/MAX_PROC_NR
@@ -1 +1 @@
-467
+468
--
2.8.0.rc3

Signed-off-by: Matteo Cafasso <noxdafox@gmail.com>
---
 tests/tsk/Makefile.am              |  3 ++-
 tests/tsk/test-filesystem-walk0.sh | 54 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)
 create mode 100755 tests/tsk/test-filesystem-walk0.sh

diff --git a/tests/tsk/Makefile.am b/tests/tsk/Makefile.am
index 80e8793..c7eac09 100644
--- a/tests/tsk/Makefile.am
+++ b/tests/tsk/Makefile.am
@@ -20,7 +20,8 @@ include $(top_srcdir)/subdir-rules.mk
 TESTS = \
 	test-icat.sh \
 	test-blkcat.sh \
-	test-blkls.sh
+	test-blkls.sh \
+	test-filesystem-walk0.sh

 TESTS_ENVIRONMENT = $(top_builddir)/run --test

diff --git a/tests/tsk/test-filesystem-walk0.sh
b/tests/tsk/test-filesystem-walk0.sh
new file mode 100755
index 0000000..c30eac2
--- /dev/null
+++ b/tests/tsk/test-filesystem-walk0.sh
@@ -0,0 +1,54 @@
+#!/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 filesystem-walk0 command.
+
+set -e
+
+if [ -n "$SKIP_TEST_FILESYSTEM_WALK0_SH" ]; then
+    echo "$0: test skipped because environment variable is set."
+    exit 77
+fi
+
+rm -f test-filesystem-walk0.bin
+
+# Skip if TSK is not supported by the appliance.
+if ! guestfish add /dev/null : run : available "libtsk"; then
+    echo "$0: skipped because TSK is not available in the appliance"
+    exit 77
+fi
+
+if [ ! -s ../../test-data/phony-guests/ubuntu.img ]; then
+    echo "$0: skipped because ubuntu.img is zero-sized"
+    exit 77
+fi
+
+# list content of disk image and store it into a file
+guestfish --ro -a ../../test-data/phony-guests/ubuntu.img <<EOF
+run
+filesystem-walk0 /dev/sda2 test-filesystem-walk0.bin
+EOF
+
+# test bin directory is in the list
+grep -q "bin" test-filesystem-walk0.bin
+if [ $? neq 0 ]; then
+    echo "$0: /bin not found in files list."
+    exit 1
+fi
+
+rm -f test-filesystem-walk0.bin
--
2.8.0.rc3

Hi,

while reviewing the new series of tsk-related changes, I'm noticing
common mistakes in all of them: I'll list them below, not
copy/paste-ing them every review.

(a) please check the coding style, in particular
- the lack of space between a function name and its following opening
  parenthesis; e.g. do_something(param) -> do_something (param)
- the style of functions, e.g.:
  | int do_something (...)
  needs to be
  | int
  | do_something (...)
- style of function prototypes (everything in a single line)

(b) please avoid if statements with side effects, as it makes code
reading and refactoring harder

(c) please put explanations and message in the messages of the actual
commits, instead of in the cover letters sent to the mailing list:
otherwise, browsing the git history in the future will not give many
clues about the newly added APIs

(d) please wrap descriptions for actions at 80 columns max.

Thanks,
-- 
Pino Toscano

On Tuesday 29 March 2016 20:35:52 Matteo Cafasso wrote:
> Signed-off-by: Matteo Cafasso <noxdafox@gmail.com>
> ---
See general notes sent as
https://www.redhat.com/archives/libguestfs/2016-March/msg00269.html
>  daemon/Makefile.am   |   3 +-
>  daemon/tsk.c         | 186
+++++++++++++++++++++++++++++++++++++++++++++++++++
>  generator/actions.ml |  16 +++++
>  m4/guestfs_daemon.m4 |   8 +++
>  src/MAX_PROC_NR      |   2 +-
>  5 files changed, 213 insertions(+), 2 deletions(-)
> 
> diff --git a/daemon/Makefile.am b/daemon/Makefile.am
> index 4e2051b..036def9 100644
> --- a/daemon/Makefile.am
> +++ b/daemon/Makefile.am
> @@ -209,7 +209,8 @@ guestfsd_LDADD = \
>  	$(LIB_CLOCK_GETTIME) \
>  	$(LIBINTL) \
>  	$(SERVENT_LIB) \
> -	$(PCRE_LIBS)
> +	$(PCRE_LIBS) \
> +	$(TSK_LIBS)
> 
>  guestfsd_CPPFLAGS = \
>  	-I$(top_srcdir)/gnulib/lib \
> diff --git a/daemon/tsk.c b/daemon/tsk.c
> index b84dfae..d72868e 100644
> --- a/daemon/tsk.c
> +++ b/daemon/tsk.c
> @@ -29,6 +29,26 @@
>  #include "actions.h"
>  #include "optgroups.h"
> 
> +#ifdef HAVE_LIBTSK
> +
> +#include <tsk/libtsk.h>
> +#include <rpc/xdr.h>
> +#include <rpc/types.h>
> +
> +static int
> +open_filesystem(const char *device, TSK_IMG_INFO **img, TSK_FS_INFO **fs);
> +static TSK_WALK_RET_ENUM
> +fswalk_callback(TSK_FS_FILE *fsfile, const char *path, void *data);
> +static char *join_path(const char *path, const char *name);
> +static int inode_out(guestfs_int_tsk_node *node_info);
> +static void reply_with_tsk_error(void);
> +
> +#else
> +
> +OPTGROUP_LIBTSK_NOT_AVAILABLE
> +
> +#endif
> +
>  static int file_out (const char *cmd);
>  static guestfs_int_tsk_node* parse_ffind (const char *out, int64_t inode);
> 
> @@ -226,3 +246,169 @@ file_out (const char *cmd)
> 
>    return 0;
>  }
> +
> +#ifdef HAVE_LIBTSK
> +
> +
> +int optgroup_libtsk_available(void)
> +{
> +  return 1;
> +}
> +
> +int do_filesystem_walk0(const mountable_t *mountable)
> +{
> +  int ret = 0;
> +  TSK_FS_INFO *fs = NULL;
> +  TSK_IMG_INFO *img = NULL;
> +  int flags = TSK_FS_DIR_WALK_FLAG_ALLOC | TSK_FS_DIR_WALK_FLAG_UNALLOC |
> +    TSK_FS_DIR_WALK_FLAG_RECURSE | TSK_FS_DIR_WALK_FLAG_NOORPHAN;
> +
> +  if (open_filesystem(mountable->device, &img, &fs) < 0)
> +    return -1;
> +
> +  reply(NULL, NULL);  /* Reply message. */
> +
> +  if (tsk_fs_dir_walk(fs, fs->root_inum, flags, fswalk_callback,
&ret) != 0) {
> +    send_file_end(1);	/* Cancel file transfer. */
> +    reply_with_tsk_error();
> +
> +    ret = -1;
> +  }
> +  else {
> +    if (send_file_end(0))  /* Normal end of file. */
> +      ret = -1;
> +
> +    ret = 0;
This ret = 0 will always override the ret = -1 done two lines above.
> +  }
> +
> +  fs->close(fs);
> +  img->close(img);
> +
> +  return ret;
> +}
> +
> +/* Inspect the device and initialises the img and fs structures.
> + * Return 0 on success, -1 on error.
> + */
> +static int
> +open_filesystem(const char *device, TSK_IMG_INFO **img, TSK_FS_INFO **fs)
> +{
> +  const char *images[] = { device };
> +
> +  if ((*img = tsk_img_open(1, images, TSK_IMG_TYPE_DETECT , 0)) == NULL) {
> +    reply_with_tsk_error();
> +
> +    return -1;
> +  }
> +
> +  if ((*fs = tsk_fs_open_img(*img, 0, TSK_FS_TYPE_DETECT)) == NULL) {
> +    reply_with_tsk_error();
> +
> +    (*img)->close(*img);
> +
> +    return -1;
> +  }
> +
> +  return 0;
> +}
> +
> +/* Filesystem walk callback, it gets called on every FS node.
> + * Parse the node, encode it into an XDR structure and send it to the
appliance.
> + * Return 0 on success, -1 on error.
> + */
> +static TSK_WALK_RET_ENUM
> +fswalk_callback(TSK_FS_FILE *fsfile, const char *path, void *data)
> +{
> +  CLEANUP_FREE char *file_name = NULL;
> +  struct guestfs_int_tsk_node node_info;
> +
> +  /* Ignore ./ and ../ */
> +  if (TSK_FS_ISDOT(fsfile->name->name))
> +    return 0;
Considering the function returns a TSK_WALK_RET_ENUM, then better use
the enum values explicitly (TSK_WALK_CONT, etc), instead of numbers.
Other than making it clear what the return value means, it also
protects us in the eventual case the API breaks and the enum values
are changed.
> +
> +  if ((file_name = join_path(path, fsfile->name->name)) == NULL)
> +    return -1;
The whole join_path seems superfluous, a simple asprintf here does
the same.
> +  node_info.tsk_name = file_name;
> +  node_info.tsk_inode = fsfile->name->meta_addr;
> +  if (fsfile->name->flags & TSK_FS_NAME_FLAG_UNALLOC)
> +    node_info.tsk_allocated = 0;
> +  else
> +    node_info.tsk_allocated = 1;
Not a real issue, but IMHO
  node_info.tsk_allocated = fsfile->name->flags &
TSK_FS_NAME_FLAG_UNALLOC;
would be preferable.  YMMV.
> +  return inode_out(&node_info);
inode_out returns -1 on error, which is not a value of the enum
TSK_WALK_RET_ENUM.
> +}
> +
> +/* Joins the file name and file path.
> + * Return the joined path on success, NULL on failure.
> + */
> +static char *join_path(const char *path, const char *name)
> +{
> +  char *buf;
> +  size_t len;
> +
> +  path = (path != NULL) ? path : "";
> +  len = strlen(path) + strlen(name) + 1;
> +
> +  if (!(buf = malloc(len))) {
> +    reply_with_perror("malloc");
> +    return NULL;
> +  }
> +
> +  snprintf(buf, len, "%s%s", path, name);
> +
> +  return buf;
> +}
> +
> +/* Serialise node_info into XDR stream and send it to the appliance.
> + * Return 0 on success, -1 on error.
> + */
> +static int inode_out(guestfs_int_tsk_node *node_info)
> +{
> +  XDR xdr;
> +  size_t len;
> +  CLEANUP_FREE char *buf;
> +
> +  if ((buf = malloc(GUESTFS_MAX_CHUNK_SIZE)) == NULL) {
> +    reply_with_perror ("malloc");
> +    return -1;
> +  }
> +
> +  /* Serialise tsk_node struct. */
> +  len = strlen(node_info->tsk_name) + 1;
> +
> +  xdrmem_create(&xdr, buf, GUESTFS_MAX_CHUNK_SIZE, XDR_ENCODE);
> +  if (!xdr_u_long(&xdr, &len))
> +    return -1;
> +  if (!xdr_string(&xdr, &node_info->tsk_name, len))
> +    return -1;
> +  if (!xdr_uint64_t(&xdr, &node_info->tsk_inode))
> +    return -1;
> +  if (!xdr_uint32_t(&xdr, &node_info->tsk_allocated))
> +    return -1;
> +
> +  /* Resize buffer to actual length. */
> +  len = xdr_getpos(&xdr);
> +  xdr_destroy(&xdr);
> +  if ((buf = realloc(buf, len)) == NULL)
> +    return -1;
> +
> +  /* Send serialised tsk_node out. */
> +  if (send_file_write(buf, len) < 0)
> +    return -1;
> +
> +  return 0;
> +}
> +
> +/* Parse TSK error and send it to the appliance. */
> +static void reply_with_tsk_error(void)
> +{
> +  const char *buf = NULL;
> +
> +  if (tsk_error_get_errno() != 0) {
> +    buf = tsk_error_get();
> +    reply_with_error("TSK error: %s", buf);
> +  }
reply_with_tsk_error is called when some tsk function fails, so it must
always report an error; makes sure to send something like
"$function failed, unknown errno" or so in case tsk_error_get_errno
returns 0.  Hint: pass the name of the failed function as parameter to
this helper, so the error message shows what failed.
> diff --git a/generator/actions.ml b/generator/actions.ml
> index 2d291fb..e33655a 100644
> --- a/generator/actions.ml
> +++ b/generator/actions.ml
> @@ -13015,6 +13015,22 @@ given its inode.
> 
>  On some filesystem, it can find deleted files." };
> 
> +  { defaults with
> +    name = "filesystem_walk0"; added = (1, 33, 16);
The next version now is 1.33.17.
> +    style = RErr, [Mountable "device"; FileOut
"filename";], [];
> +    proc_nr = Some 468;
It should be 465...
> +    optional = Some "libtsk";
> +    progress = true; cancellable = true;
> +    shortdesc = "walks through the filesystem content";
> +    longdesc = "\
> +This specialised command is used to walk through the internal structures
> +of a disk partition (eg. F</dev/sda1>) in order to return a list of
all
> +the files and directories stored within.
> +
> +For each file or directory found, the function reports its path, its inode
and whether
> +it is allocated or not.
> +
> +The output is stored in F<filename> on the host encoded in XDR
format." };
The XDR encoding of the resulting file is basically meaningless to users
of the libguestfs API.  Considering this function seems to be only an
helper for filesystem_walk, then I'd say it needs to be internal (see
internal_feature_available for an example).

Thanks,
-- 
Pino Toscano

On Wednesday 30 March 2016 13:45:15 NoxDaFox wrote:
> Thanks for the comments, just few notes and questions.
Just remember to keep the list in the loop.
> 2016-03-30 12:24 GMT+03:00 Pino Toscano <ptoscano@redhat.com>:
> 
> > Hi,
> >
> > while reviewing the new series of tsk-related changes, I'm
noticing
> > common mistakes in all of them: I'll list them below, not
> > copy/paste-ing them every review.
> >
> > (a) please check the coding style, in particular
> > - the lack of space between a function name and its following opening
> >   parenthesis; e.g. do_something(param) -> do_something (param)
> >
> 
> I was complying with what said here:
> http://libguestfs.org/guestfs-hacking.1.html
> 
> It refers to K&R style which suggests functions defined as
> do_something(param).
> 
> I guess the documentation at the link could mention this detail.
Indeed it is something that should be changed, since the rest of the
code definitely has spaces between functions and opening parenthesis.
> - the style of functions, e.g.:
> >   | int do_something (...)
> >   needs to be
> >   | int
> >   | do_something (...)
> >
> 
> Same as above.
What I mentioned is the style used all around the code.
> - style of function prototypes (everything in a single line)
> >
> 
> I'm confused here: should they be all on a single line or should they
be
> split as in the note above?
Prototypes need a single line, while the actual function definitions
indented as above.

In general, a golden rule that applies whenever patching any software
is "follow the existing style", in case there is a single one (or at
least one followed in the majority of the codebase).
> > (b) please avoid if statements with side effects, as it makes code
> > reading and refactoring harder
> >
> 
> Will re-factor the logic.
> 
> 
> > (c) please put explanations and message in the messages of the actual
> > commits, instead of in the cover letters sent to the mailing list:
> > otherwise, browsing the git history in the future will not give many
> > clues about the newly added APIs
> >
> 
> I tried to stick to the behaviour I saw in the mailing list. Will fix this
> as well.
Usually cover letters provide a general overview of what a series of
patches does, while the actual commits still provide explanations of
what each does.
> > (d) please wrap descriptions for actions at 80 columns max.
> >
> 
> Going to fix this as well.
-- 
Pino Toscano