<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">2016-04-04 12:48 GMT+03:00 Pino Toscano <span dir="ltr"><<a href="mailto:ptoscano@redhat.com" target="_blank">ptoscano@redhat.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Sunday 03 April 2016 16:30:48 Matteo Cafasso wrote:<br>
> The internal_filesystem_walk command walks<br>
> through the FS structure of a disk partition<br>
> and returns all the files or directories<br>
> which could be found.<br>
><br>
> The command is able to retrieve information<br>
> regarding deleted or unaccessible files as well<br>
> where other commands such as stat or find<br>
> would fail.<br>
><br>
> The gathered list of tsk_dirent structs<br>
> is serialised into XDR format and written<br>
> to a file by the appliance.<br>
<br>
</span>Not that it is a big issue, but you can wrap commit messages at the<br>
72 columns.<br>
<div><div class="h5"><br>
> Signed-off-by: Matteo Cafasso <<a href="mailto:noxdafox@gmail.com">noxdafox@gmail.com</a>><br>
> ---<br>
> daemon/Makefile.am | 4 +-<br>
> daemon/tsk.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++<br>
> generator/<a href="http://actions.ml" rel="noreferrer" target="_blank">actions.ml</a> | 25 ++++++<br>
> src/MAX_PROC_NR | 2 +-<br>
> 4 files changed, 254 insertions(+), 2 deletions(-)<br>
> create mode 100644 daemon/tsk.c<br>
><br>
> diff --git a/daemon/Makefile.am b/daemon/Makefile.am<br>
> index beb7962..03bf71f 100644<br>
> --- a/daemon/Makefile.am<br>
> +++ b/daemon/Makefile.am<br>
> @@ -179,6 +179,7 @@ guestfsd_SOURCES = \<br>
> sync.c \<br>
> syslinux.c \<br>
> tar.c \<br>
> + tsk.c \<br>
> truncate.c \<br>
> umask.c \<br>
> upload.c \<br>
> @@ -209,7 +210,8 @@ guestfsd_LDADD = \<br>
> $(LIB_CLOCK_GETTIME) \<br>
> $(LIBINTL) \<br>
> $(SERVENT_LIB) \<br>
> - $(PCRE_LIBS)<br>
> + $(PCRE_LIBS) \<br>
> + $(TSK_LIBS)<br>
><br>
> guestfsd_CPPFLAGS = \<br>
> -I$(top_srcdir)/gnulib/lib \<br>
> diff --git a/daemon/tsk.c b/daemon/tsk.c<br>
> new file mode 100644<br>
> index 0000000..ac44106<br>
> --- /dev/null<br>
> +++ b/daemon/tsk.c<br>
> @@ -0,0 +1,225 @@<br>
> +/* libguestfs - the guestfsd daemon<br>
> + * Copyright (C) 2016 Red Hat Inc.<br>
> + *<br>
> + * This program is free software; you can redistribute it and/or modify<br>
> + * it under the terms of the GNU General Public License as published by<br>
> + * the Free Software Foundation; either version 2 of the License, or<br>
> + * (at your option) any later version.<br>
> + *<br>
> + * This program is distributed in the hope that it will be useful,<br>
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of<br>
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the<br>
> + * GNU General Public License for more details.<br>
> + *<br>
> + * You should have received a copy of the GNU General Public License<br>
> + * along with this program; if not, write to the Free Software<br>
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.<br>
> + */<br>
> +<br>
> +#include <config.h><br>
> +<br>
> +#include <stdio.h><br>
> +#include <stdlib.h><br>
> +#include <inttypes.h><br>
> +#include <string.h><br>
> +#include <unistd.h><br>
> +#include <rpc/xdr.h><br>
> +#include <rpc/types.h><br>
> +<br>
> +#include "guestfs_protocol.h"<br>
> +#include "daemon.h"<br>
> +#include "actions.h"<br>
> +#include "optgroups.h"<br>
> +<br>
> +#ifdef HAVE_LIBTSK<br>
> +<br>
> +#include <tsk/libtsk.h><br>
> +<br>
> +/* File types map similar to dirent. */<br>
> +#define TSK_FILE_TYPE_NUM 10<br>
> +char TSK_FILE_TYPE[TSK_FILE_TYPE_NUM] = {<br>
> + 'u', 'f', 'c', 'd', 'b', 'r', 'l', 's', 'h', 'w'<br>
> +};<br>
<br>
</div></div>I see the libtsk already uses TSK_* and tsk_* prefixes for its own<br>
stuff, so I'd avoid using the same prefixes for local variables.<br>
<br>
Also, make the string as static const, since it does not need to be<br>
modified at all (and thus can then be placed into .rodata).<br></blockquote><div><br></div><div>Good spot! I'll fix it thanks!<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> +<br>
> +static int open_filesystem (const char *device,<br>
> + TSK_IMG_INFO **img, TSK_FS_INFO **fs);<br>
> +static TSK_WALK_RET_ENUM fswalk_callback (TSK_FS_FILE *fsfile,<br>
> + const char *path, void *data);<br>
<br>
</span>Single line for forward declarations.<br></blockquote><div><br></div><div>Even if they are longer than 80 chars?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> +static char file_type (TSK_FS_FILE *fsfile);<br>
> +static int send_dirent_info (guestfs_int_tsk_dirent *dirent);<br>
> +static void reply_with_tsk_error (const char *funcname);<br>
> +<br>
> +int<br>
> +do_internal_filesystem_walk (const mountable_t *mountable)<br>
> +{<br>
> + int ret = -1;<br>
> + TSK_FS_INFO *fs = NULL;<br>
> + TSK_IMG_INFO *img = NULL; /* Used internally by tsk_fs_dir_walk */<br>
> + int flags = TSK_FS_DIR_WALK_FLAG_ALLOC | TSK_FS_DIR_WALK_FLAG_UNALLOC |<br>
> + TSK_FS_DIR_WALK_FLAG_RECURSE | TSK_FS_DIR_WALK_FLAG_NOORPHAN;<br>
> +<br>
> + ret = open_filesystem (mountable->device, &img, &fs);<br>
> + if (ret < 0)<br>
> + return ret;<br>
> +<br>
> + reply (NULL, NULL); /* Reply message. */<br>
> +<br>
> + ret = tsk_fs_dir_walk (fs, fs->root_inum, flags, fswalk_callback, NULL);<br>
> + if (ret == 0)<br>
> + ret = send_file_end (0); /* File transfer end. */<br>
> + else<br>
> + send_file_end (1); /* Cancel file transfer. */<br>
> +<br>
> + fs->close (fs);<br>
> + img->close (img);<br>
> +<br>
> + return ret;<br>
> +}<br>
> +<br>
> +/* Inspect the device and initialises the img and fs structures.<br>
> + * Return 0 on success, -1 on error.<br>
> + */<br>
> +static int<br>
> +open_filesystem (const char *device, TSK_IMG_INFO **img, TSK_FS_INFO **fs)<br>
> +{<br>
> + const char *images[] = { device };<br>
> +<br>
> + *img = tsk_img_open (1, images, TSK_IMG_TYPE_DETECT , 0);<br>
> + if (*img == NULL) {<br>
> + reply_with_tsk_error ("tsk_image_open");<br>
> + return -1;<br>
> + }<br>
> +<br>
> + *fs = tsk_fs_open_img (*img, 0, TSK_FS_TYPE_DETECT);<br>
> + if (*fs == NULL) {<br>
> + reply_with_tsk_error ("tsk_fs_open_img");<br>
> + (*img)->close (*img);<br>
> + return -1;<br>
> + }<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +/* Filesystem walk callback, it gets called on every FS node.<br>
> + * Parse the node, encode it into an XDR structure and send it to the appliance.<br>
> + * Return TSK_WALK_CONT on success, TSK_WALK_ERROR on error.<br>
> + */<br>
> +static TSK_WALK_RET_ENUM<br>
> +fswalk_callback (TSK_FS_FILE *fsfile, const char *path, void *data)<br>
> +{<br>
> + int ret = 0;<br>
> + CLEANUP_FREE char *fname = NULL;<br>
> + struct guestfs_int_tsk_dirent dirent;<br>
> +<br>
> + /* Ignore ./ and ../ */<br>
> + ret = TSK_FS_ISDOT (fsfile->name->name);<br>
> + if (ret != 0)<br>
> + return TSK_WALK_CONT;<br>
> +<br>
> + /* Build the full relative path of the entry */<br>
> + ret = asprintf_nowarn (&fname, "%Q%Q", path, fsfile->name->name);<br>
<br>
</div></div>Why the quoting? We don't quote results in similar APIs (e.g. readdir).<br></blockquote><div><br></div><div>I didn't understand this one. I checked daemon/readdir.c and I found no asprintf examples there.<br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> + if (ret < 0) {<br>
> + fprintf (stderr, "asprintf: %m");<br>
> + return TSK_WALK_ERROR;<br>
> + }<br>
> +<br>
> + dirent.tsk_inode = fsfile->name->meta_addr;<br>
> + dirent.tsk_type = file_type (fsfile);<br>
> + dirent.tsk_size = (fsfile->meta != NULL) ? fsfile->meta->size : 0;<br>
<br>
</span>If 'meta' is null, then I guess the size should be -1 to indicate it<br>
was not available; otherwise, there is no difference between an empty<br>
file, and a file whose metadata could not be read.<br></blockquote><div> </div>The issue is that even if 'meta' is non-null, yet the value could be 0. In cases where the file has been deleted, TSK does its best to retrieve as much as it can and set to 0 the rest (same applies with inode for example, the inode is set to 0 instead of -1).<br><br></div><div class="gmail_quote">The command documentation reports this "issue" (or feature?).<br><br></div><div class="gmail_quote">Anyway I'll triple-check in order to be sure about it.<br></div><div class="gmail_quote"><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> + dirent.tsk_name = fname;<br>
> + dirent.tsk_allocated = !(fsfile->name->flags & TSK_FS_NAME_FLAG_UNALLOC);<br>
> +<br>
> + ret = send_dirent_info (&dirent);<br>
> + ret = (ret == 0) ? TSK_WALK_CONT : TSK_WALK_ERROR;<br>
> +<br>
> + return ret;<br>
> +}<br>
> +<br>
> +/* Inspect fsfile to identify its type. */<br>
> +static char<br>
> +file_type(TSK_FS_FILE *fsfile)<br>
> +{<br>
> + if (fsfile->name->type < TSK_FILE_TYPE_NUM)<br>
> + return TSK_FILE_TYPE[fsfile->name->type];<br>
> + else if (fsfile->meta != NULL && fsfile->meta->type < TSK_FILE_TYPE_NUM)<br>
> + return TSK_FILE_TYPE[fsfile->meta->type];<br>
> + else<br>
> + return 'u';<br>
> +}<br>
<br>
</span>I think it would be better to have a switch case on the file type: the<br>
current solution will silently break if a future (incompatible) version<br>
of libtsk changes the order/values of the elements in<br>
TSK_FS_NAME_TYPE_ENUM. Also, new values are simply not detected at<br>
all, whereas with a switch case at least they would give us compiler<br>
warnings.<br>
<br>
Also, IMHO code like:<br>
<br>
switch (type) {<br>
case TSK_FS_NAME_TYPE_UNDEF: return 'u';<br>
...<br>
<br>
would be much more readable.<br></blockquote><div><br></div><div>I'll fix it thanks!<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> +/* Serialise dirent into XDR stream and send it to the appliance.<br>
> + * Return 0 on success, -1 on error.<br>
> + */<br>
> +static int<br>
> +send_dirent_info (guestfs_int_tsk_dirent *dirent)<br>
> +{<br>
> + XDR xdr;<br>
> + size_t len = 0;<br>
> + CLEANUP_FREE char *buf;<br>
> +<br>
> + buf = malloc (GUESTFS_MAX_CHUNK_SIZE);<br>
> + if (buf == NULL) {<br>
> + fprintf (stderr, "malloc: %m");<br>
> + return -1;<br>
> + }<br>
> +<br>
> + /* Serialise tsk_dirent struct. */<br>
> + xdrmem_create (&xdr, buf, GUESTFS_MAX_CHUNK_SIZE, XDR_ENCODE);<br>
> +<br>
> + if (!xdr_uint64_t (&xdr, &dirent->tsk_inode))<br>
> + return -1;<br>
> + if (!xdr_char (&xdr, &dirent->tsk_type))<br>
> + return -1;<br>
> + if (!xdr_int64_t (&xdr, &dirent->tsk_size))<br>
> + return -1;<br>
> +<br>
> + /* Serialise filename. */<br>
> + len = strlen (dirent->tsk_name) + 1;<br>
> + if (!xdr_u_long (&xdr, &len))<br>
> + return -1;<br>
> + if (!xdr_string (&xdr, &dirent->tsk_name, len))<br>
> + return -1;<br>
> +<br>
> + if (!xdr_uint32_t (&xdr, &dirent->tsk_allocated))<br>
> + return -1;<br>
> +<br>
> + /* Resize buffer to actual length. */<br>
> + len = xdr_getpos (&xdr);<br>
> + xdr_destroy (&xdr);<br>
> + buf = realloc (buf, len);<br>
> + if (buf == NULL) {<br>
> + fprintf (stderr, "realloc: %m");<br>
> + return -1;<br>
> + }<br>
<br>
</div></div>I think this manual code is not needed, and you can use the XDR<br>
serializing functions generated in daemon/guestfs_protocol.{c,h}.<br></blockquote><div><br></div><div>I'll take a look at those.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> +<br>
> + /* Send serialised tsk_dirent out. */<br>
> + return send_file_write (buf, len);<br>
> +}<br>
> +<br>
> +/* Parse TSK error and send it to the appliance. */<br>
> +static void<br>
> +reply_with_tsk_error (const char *funcname)<br>
> +{<br>
> + int ret = 0;<br>
> + const char *buf = NULL;<br>
> +<br>
> + ret = tsk_error_get_errno ();<br>
> + if (ret != 0) {<br>
> + buf = tsk_error_get ();<br>
> + reply_with_error ("TSK function %s error: %s", funcname, buf);<br>
> + }<br>
> + else<br>
> + reply_with_error ("TSK function %s: unknown error", funcname);<br>
> +}<br>
<br>
</span>s/TSK function//<br>
<div><div class="h5"><br>
> +<br>
> +int<br>
> +optgroup_libtsk_available (void)<br>
> +{<br>
> + return 1;<br>
> +}<br>
> +<br>
> +#else /* !HAVE_LIBTSK */<br>
> +<br>
> +OPTGROUP_LIBTSK_NOT_AVAILABLE<br>
> +<br>
> +#endif /* !HAVE_LIBTSK */<br>
> diff --git a/generator/<a href="http://actions.ml" rel="noreferrer" target="_blank">actions.ml</a> b/generator/<a href="http://actions.ml" rel="noreferrer" target="_blank">actions.ml</a><br>
> index e5cb939..449ffa0 100644<br>
> --- a/generator/<a href="http://actions.ml" rel="noreferrer" target="_blank">actions.ml</a><br>
> +++ b/generator/<a href="http://actions.ml" rel="noreferrer" target="_blank">actions.ml</a><br>
> @@ -12958,6 +12958,31 @@ and save it as F<filename> on the local machine.<br>
><br>
> This allows to download deleted or inaccessible files." };<br>
><br>
> + { defaults with<br>
> + name = "internal_filesystem_walk"; added = (1, 33, 17);<br>
> + style = RErr, [Mountable "device"; FileOut "filename"], [];<br>
> + proc_nr = Some 465;<br>
> + visibility = VInternal;<br>
> + optional = Some "libtsk";<br>
> + shortdesc = "walk through the filesystem content";<br>
> + longdesc = "Walk through the internal structures of a disk partition<br>
> +(eg. F</dev/sda1>) in order to return a list of all the files<br>
> +and directories stored within.<br>
> +<br>
> +It is not necessary to mount the disk partition to run this command.<br>
> +<br>
> +All entries in the filesystem are returned, excluding C<.> and<br>
> +C<..>. This function can list deleted or unaccessible files.<br>
> +The entries are I<not> sorted.<br>
> +<br>
> +If the entry is not allocated (ex: it has been deleted),<br>
> +its inode, type or size might not be recovered correctly.<br>
> +In such case, the inode and the size will be 0 while the type<br>
> +will be unidentified 'u'.<br>
> +<br>
> +This call returns basic file type information about each<br>
> +file." };<br>
<br>
</div></div>No need to copy&paste the same description used for the public<br>
filesystem_walk -- a one-liner telling it is an internal function for<br>
filesystem_walk is enough.<br>
Internal functions will not be documented anyway, so it is not worth<br>
adding "proper" documentation text for them.<br></blockquote><div><br></div><div>Saw the same thing with some other API, I'll remove the duplicate.<br><br></div><div>Thanks for the comments.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks,<br>
<span class="HOEnZb"><font color="#888888">--<br>
Pino Toscano</font></span><br>_______________________________________________<br>
Libguestfs mailing list<br>
<a href="mailto:Libguestfs@redhat.com">Libguestfs@redhat.com</a><br>
<a href="https://www.redhat.com/mailman/listinfo/libguestfs" rel="noreferrer" target="_blank">https://www.redhat.com/mailman/listinfo/libguestfs</a><br></blockquote></div><br></div></div>