[Libguestfs] [PATCH] libvirt: read disk paths from pools (RHBZ#1366049)

[Pino Toscano]

A disk of type 'volume' is stored as
  
and its real location is inside the 'volume_name', as 'pool_name': in
this case, query libvirt for the actual path of the specified volume in
the specified pool.

Adjust the code so that:
- for_each_disk gets the virConnectPtr, needed to do operations with
  libvirt
- when extracting the disk filename depending on the type, the code
  snippet doing it can directly set 'filename', without setting an XPath
  result variable

Only file-based volumes are supported for now; more types can be added
(with proper testing) later on.
---
 src/libvirt-domain.c | 131 +--
 1 file changed, 117 insertions(+), 14 deletions(-)

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index d54814f..50759e3 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -40,8 +40,9 @@
 #if defined(HAVE_LIBVIRT)
 
 static xmlDocPtr get_domain_xml (guestfs_h *g, virDomainPtr dom);
-static ssize_t for_each_disk (guestfs_h *g, xmlDocPtr doc, int (*f) (guestfs_h 
*g, const char *filename, const char *format, int readonly, const char 
*protocol, char *const *server, const char *username, void *data), void *data);
+static ssize_t for_each_disk (guestfs_h *g, virConnectPtr conn, xmlDocPtr doc, 
int (*f) (guestfs_h *g, const char *filename, const char *format, int readonly, 
const char *protocol, char *const *server, const char *username, void *data), 
void *data);
 static int libvirt_selinux_label (guestfs_h *g, xmlDocPtr doc, char 
**label_rtn, char **imagelabel_rtn);
+static char *filename_from_pool (guestfs_h *g, virConnectPtr conn, const char 
*pool_nane, const char *volume_name);
 
 static void
 ignore_errors (void *ignore, virErrorPtr ignore2)
@@ -311,7 +312,7 @@ guestfs_impl_add_libvirt_dom (guestfs_h *g, void *domvp,
* all disks are added or none are added.
*/
   ckp = guestfs_int_checkpoint_drives (g);
-  r = for_each_disk (g, doc, add_disk, );
+  r = for_each_disk (g, virDomainGetConnect (dom), doc, add_disk, );
   if (r == -1)
 guestfs_int_rollback_drives (g, ckp);
 
@@ -466,6 +467,7 @@ libvirt_selinux_label (guestfs_h *g, xmlDocPtr doc,
  */
 static ssize_t
 for_each_disk (guestfs_h *g,
+   virConnectPtr conn,
xmlDocPtr doc,
int (*f) (guestfs_h *g,
  const char *filename, const char *format,
@@ -509,6 +511,8 @@ for_each_disk (guestfs_h *g,
   CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpprotocol = NULL;
   CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xphost = NULL;
   CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpusername = NULL;
+  CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xppool = NULL;
+  CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpvolume = NULL;
   xmlAttrPtr attr;
   int readonly;
   int t;
@@ -628,22 +632,65 @@ for_each_disk (guestfs_h *g,
  * TODO: secrets: ./auth/secret/@type,
  * ./auth/secret/@usage || ./auth/secret/@uuid
  */
+  } else if (STREQ (type, "volume")) { /* type = "volume", use 
source/@volume */
+CLEANUP_FREE char *pool = NULL;
+CLEANUP_FREE char *volume = NULL;
+
+xpathCtx->node = nodes->nodeTab[i];
+
+/* Get the source pool.  Required. */
+xppool = xmlXPathEvalExpression (BAD_CAST "./source/@pool",
+ xpathCtx);
+if (xppool == NULL ||
+xppool->nodesetval == NULL ||
+xppool->nodesetval->nodeNr == 0)
+  continue;
+assert (xppool->nodesetval->nodeTab[0]);
+assert (xppool->nodesetval->nodeTab[0]->type ==
+XML_ATTRIBUTE_NODE);
+attr = (xmlAttrPtr) xppool->nodesetval->nodeTab[0];
+pool = (char *) xmlNodeListGetString (doc, attr->children, 1);
+
+/* Get the source volume.  Required. */
+xpvolume = xmlXPathEvalExpression (BAD_CAST "./source/@volume",
+   xpathCtx);
+if (xpvolume == NULL ||
+xpvolume->nodesetval == NULL ||
+xpvolume->nodesetval->nodeNr == 0)
+  continue;
+assert (xpvolume->nodesetval->nodeTab[0]);
+assert (xpvolume->nodesetval->nodeTab[0]->type ==
+XML_ATTRIBUTE_NODE);
+attr = (xmlAttrPtr) xpvolume->nodesetval->nodeTab[0];
+volume = (char *) xmlNodeListGetString (doc, attr->children, 1);
+
+debug (g, "disk[%zu]: pool: %s; volume: %s", i, pool, volume);
+
+filename = filename_from_pool (g, conn, pool, volume);
+if (filename == NULL)
+  continue; /* filename_from_pool already called error() */
   } else
 continue; /* type <> "file", "block", or "network", skip it */
 
-  assert (xpfilename);
-  assert (xpfilename->nodesetval);
-  if (xpfilename->nodesetval->nodeNr > 0) {
-assert (xpfilename->nodesetval->nodeTab[0]);
-assert (xpfilename->nodesetval->nodeTab[0]->type ==
-

[Libguestfs] [PATCH v3 1/3] New API: internal_find_block

[Matteo Cafasso]

The internal_find_block command searches all entries referring to the
given filesystem data block and returns a tsk_dirent structure
for each of them.

For filesystems such as NTFS which do not delete the block mapping
when removing files, it is possible to get multiple non-allocated
entries for the same block.

The gathered list of tsk_dirent structs is serialised into XDR format
and written to a file by the appliance.

Signed-off-by: Matteo Cafasso 
---
 daemon/tsk.c | 91 
 generator/actions.ml |  9 ++
 src/MAX_PROC_NR  |  2 +-
 3 files changed, 101 insertions(+), 1 deletion(-)

diff --git a/daemon/tsk.c b/daemon/tsk.c
index af803d7..f971840 100644
--- a/daemon/tsk.c
+++ b/daemon/tsk.c
@@ -20,6 +20,7 @@

 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -42,9 +43,16 @@ enum tsk_dirent_flags {
   DIRENT_COMPRESSED = 0x04
 };

+typedef struct {
+  bool found;
+  uint64_t block;
+} findblk_data;
+
 static int open_filesystem (const char *, TSK_IMG_INFO **, TSK_FS_INFO **);
 static TSK_WALK_RET_ENUM fswalk_callback (TSK_FS_FILE *, const char *, void *);
 static TSK_WALK_RET_ENUM findino_callback (TSK_FS_FILE *, const char *, void 
*);
+static TSK_WALK_RET_ENUM findblk_callback (TSK_FS_FILE *, const char *, void 
*);
+static TSK_WALK_RET_ENUM attrwalk_callback (TSK_FS_FILE *, TSK_OFF_T , 
TSK_DADDR_T , char *, size_t , TSK_FS_BLOCK_FLAG_ENUM , void *);
 static int send_dirent_info (TSK_FS_FILE *, const char *);
 static char file_type (TSK_FS_FILE *);
 static int file_flags (TSK_FS_FILE *fsfile);
@@ -109,6 +117,35 @@ do_internal_find_inode (const mountable_t *mountable, 
int64_t inode)
   return ret;
 }

+int
+do_internal_find_block (const mountable_t *mountable, int64_t block)
+{
+  int ret = -1;
+  TSK_FS_INFO *fs = NULL;
+  TSK_IMG_INFO *img = NULL;  /* Used internally by tsk_fs_dir_walk */
+  const 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;
+
+  ret = open_filesystem (mountable->device, , );
+  if (ret < 0)
+return ret;
+
+  reply (NULL, NULL);  /* Reply message. */
+
+  ret = tsk_fs_dir_walk (fs, fs->root_inum, flags,
+ findblk_callback, (void *) );
+  if (ret == 0)
+ret = send_file_end (0);  /* File transfer end. */
+  else
+send_file_end (1);  /* Cancel file transfer. */
+
+  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.
  */
@@ -172,6 +209,60 @@ findino_callback (TSK_FS_FILE *fsfile, const char *path, 
void *data)
   return (ret == 0) ? TSK_WALK_CONT : TSK_WALK_ERROR;
 }

+/* Find block, it gets called on every FS node.
+ *
+ * Return TSK_WALK_CONT on success, TSK_WALK_ERROR on error.
+ */
+static TSK_WALK_RET_ENUM
+findblk_callback (TSK_FS_FILE *fsfile, const char *path, void *data)
+{
+  findblk_data blkdata;
+  const TSK_FS_ATTR *fsattr = NULL;
+  int ret = 0, count = 0, index = 0;
+  const int flags = TSK_FS_FILE_WALK_FLAG_AONLY | TSK_FS_FILE_WALK_FLAG_SLACK;
+
+  if (entry_is_dot (fsfile))
+return TSK_WALK_CONT;
+
+  blkdata.found = false;
+  blkdata.block = * (uint64_t *) data;
+
+  /* Retrieve block list */
+  count = tsk_fs_file_attr_getsize (fsfile);
+
+  for (index = 0; index < count; index++) {
+fsattr = tsk_fs_file_attr_get_idx (fsfile, index);
+
+if (fsattr != NULL && fsattr->flags & TSK_FS_ATTR_NONRES)
+  tsk_fs_attr_walk (fsattr, flags, attrwalk_callback, (void *) );
+  }
+
+  if (blkdata.found)
+ret = send_dirent_info (fsfile, path);
+
+  return (ret == 0) ? TSK_WALK_CONT : TSK_WALK_ERROR;
+}
+
+/* Attribute walk, searches the given block within the FS node attributes.
+ *
+ * Return TSK_WALK_CONT on success, TSK_WALK_ERROR on error.
+ */
+static TSK_WALK_RET_ENUM
+attrwalk_callback (TSK_FS_FILE *fsfile, TSK_OFF_T offset,
+   TSK_DADDR_T blkaddr, char *buf, size_t size,
+   TSK_FS_BLOCK_FLAG_ENUM flags, void *data)
+{
+  findblk_data *blkdata = (findblk_data *) data;
+
+  if (!(flags & TSK_FS_BLOCK_FLAG_SPARSE) && blkaddr == blkdata->block) {
+blkdata->found = true;
+
+return TSK_WALK_STOP;
+  }
+
+  return TSK_WALK_CONT;
+}
+
 /* Extract the information from the entry, serialize and send it out.
  * Return 0 on success, -1 on error.
  */
diff --git a/generator/actions.ml b/generator/actions.ml
index 91a1819..b38a30f 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -13253,6 +13253,15 @@ is removed." };
 shortdesc = "search the entries associated to the given inode";
 longdesc = "Internal function for find_inode." };

+  { defaults with
+name = "internal_find_block"; added = (1, 35, 6);
+style = RErr, [Mountable "device"; Int64 "block"; FileOut "filename";], [];
+proc_nr = Some 471;
+visibility = VInternal;
+optional = Some 

[Libguestfs] [PATCH v3 2/3] New API: find_block

[Matteo Cafasso]

Library's counterpart of the daemon's internal_find_block command.

It writes the daemon's command output on a temporary file and parses it,
deserialising the XDR formatted tsk_dirent structs.

It returns to the caller the list of tsk_dirent structs generated by the
internal_find_block command.

Signed-off-by: Matteo Cafasso 
---
 generator/actions.ml | 16 
 src/tsk.c| 17 +
 2 files changed, 33 insertions(+)

diff --git a/generator/actions.ml b/generator/actions.ml
index b38a30f..8947551 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -3729,6 +3729,22 @@ Searches all the entries associated with the given inode.
 For each entry, a C structure is returned.
 See C for more information about C structures." };

+  { defaults with
+name = "find_block"; added = (1, 35, 6);
+style = RStructList ("dirents", "tsk_dirent"), [Mountable "device"; Int64 
"block";], [];
+optional = Some "libtsk";
+progress = true; cancellable = true;
+shortdesc = "search the entries referring to the given data block";
+longdesc = "\
+Searches all the entries referring to the given data block.
+
+Certain filesystems preserve the block mapping when deleting a file.
+Therefore, it is possible to see multiple deleted files referring
+to the same block.
+
+For each entry, a C structure is returned.
+See C for more information about C structures." };
+
 ]

 (* daemon_functions are any functions which cause some action
diff --git a/src/tsk.c b/src/tsk.c
index 1def9c9..7db6f71 100644
--- a/src/tsk.c
+++ b/src/tsk.c
@@ -72,6 +72,23 @@ guestfs_impl_find_inode (guestfs_h *g, const char 
*mountable, int64_t inode)
   return parse_dirent_file (g, tmpfile);  /* caller frees */
 }

+struct guestfs_tsk_dirent_list *
+guestfs_impl_find_block (guestfs_h *g, const char *mountable, int64_t block)
+{
+  int ret = 0;
+  CLEANUP_UNLINK_FREE char *tmpfile = NULL;
+
+  tmpfile = make_temp_file (g, "find_block");
+  if (tmpfile == NULL)
+return NULL;
+
+  ret = guestfs_internal_find_block (g, mountable, block, tmpfile);
+  if (ret < 0)
+return NULL;
+
+  return parse_dirent_file (g, tmpfile);  /* caller frees */
+}
+
 /* Parse the file content and return dirents list.
  * Return a list of tsk_dirent on success, NULL on error.
  */
--
2.9.3

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v3 0/3] New API - find_block

[Matteo Cafasso]

v3:

 - fixed attribute walk callback: checking against TSK_FS_BLOCK_FLAG_RAW flag 
would
   exclude compressed data blocks which are still important.
   Yet we want to exclude sparse blocks (TSK_FS_BLOCK_FLAG_SPARSE) as they are 
not stored
   on the disk.

Matteo Cafasso (3):
  New API: internal_find_block
  New API: find_block
  find_block: added API tests

 daemon/tsk.c | 91 
 generator/actions.ml | 25 
 src/MAX_PROC_NR  |  2 +-
 src/tsk.c| 17 +
 tests/tsk/Makefile.am|  1 +
 tests/tsk/test-find-block.sh | 66 
 6 files changed, 201 insertions(+), 1 deletion(-)
 create mode 100755 tests/tsk/test-find-block.sh

--
2.9.3

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH v3 3/3] find_block: added API tests

[Matteo Cafasso]

NTFS file system always has the Boot file at block 0. This reliable
information helps testing the API.

Signed-off-by: Matteo Cafasso 
---
 tests/tsk/Makefile.am|  1 +
 tests/tsk/test-find-block.sh | 66 
 2 files changed, 67 insertions(+)
 create mode 100755 tests/tsk/test-find-block.sh

diff --git a/tests/tsk/Makefile.am b/tests/tsk/Makefile.am
index 07c74f9..44a893e 100644
--- a/tests/tsk/Makefile.am
+++ b/tests/tsk/Makefile.am
@@ -21,6 +21,7 @@ TESTS = \
test-download-inode.sh \
test-download-blocks.sh \
test-filesystem-walk.sh \
+   test-find-block.sh \
test-find-inode.sh

 TESTS_ENVIRONMENT = $(top_builddir)/run --test
diff --git a/tests/tsk/test-find-block.sh b/tests/tsk/test-find-block.sh
new file mode 100755
index 000..984947d
--- /dev/null
+++ b/tests/tsk/test-find-block.sh
@@ -0,0 +1,66 @@
+#!/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 find-block command.
+
+if [ -n "$SKIP_TEST_FIND_BLOCK_SH" ]; then
+echo "$0: test skipped because environment variable is set."
+exit 77
+fi
+
+# 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/windows.img ]; then
+echo "$0: skipped because windows.img is zero-sized"
+exit 77
+fi
+
+output=$(
+guestfish --ro -a ../../test-data/phony-guests/windows.img 

[Libguestfs] [PATCH v5] v2v: linux: correctly reconfigure the initrd on Debian

[Tomáš Golembiovský]

Using update-initramfs is the native way of updating initrd on Debian
based systems.

To add some modules to the image we can list them in file
/etc/initramfs-tools/modules.

Signed-off-by: Tomáš Golembiovský 
---
 v2v/convert_linux.ml | 32 
 1 file changed, 32 insertions(+)

diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
index 08f4b2a..6b4197d 100644
--- a/v2v/convert_linux.ml
+++ b/v2v/convert_linux.ml
@@ -478,6 +478,15 @@ let rec convert ~keep_serial_console (g : G.guestfs) 
inspect source rcaps =
 ignore (g#command (Array.of_list args))
   in
 
+  let run_update_initramfs_command () =
+let args =
+  "/usr/sbin/update-initramfs"  ::
+(if verbose () then [ "-v" ] else [])
+  @ [ "-c"; "-k"; mkinitrd_kv ]
+in
+ignore (g#command (Array.of_list args))
+  in
+
   if g#is_file ~followsymlinks:true "/sbin/dracut" then
 run_dracut_command "/sbin/dracut"
   else if g#is_file ~followsymlinks:true "/usr/bin/dracut" then
@@ -491,6 +500,29 @@ let rec convert ~keep_serial_console (g : G.guestfs) 
inspect source rcaps =
"-k"; kernel.ki_vmlinuz |]
 )
   )
+  else if family = `Debian_family then (
+if not (g#is_file ~followsymlinks:true "/usr/sbin/update-initramfs") 
then
+  error (f_"unable to rebuild initrd (%s) because update-initramfs was 
not found in the guest")
+initrd;
+
+if List.length modules > 0 then (
+  (* The modules to add to initrd are defined in:
+  * /etc/initramfs-tools/modules
+  * File format is same as modules(5).
+  *)
+  let path = "/files/etc/initramfs-tools/modules" in
+  g#aug_transform "modules" "/etc/initramfs-tools/modules";
+  Linux.augeas_reload g;
+  g#aug_set (sprintf "%s/#comment[last()+1]" path)
+"The following modules were added by virt-v2v";
+  List.iter (
+fun m -> g#aug_clear (sprintf "%s/%s" path m)
+  ) modules;
+  g#aug_save ();
+);
+
+run_update_initramfs_command ()
+  )
   else if g#is_file ~followsymlinks:true "/sbin/mkinitrd" then (
 let module_args = List.map (sprintf "--with=%s") modules in
 let args =
-- 
2.9.3


___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2 1/3] New API: internal_find_block

[NoxDaFox]

2016-09-20 11:38 GMT+03:00 Pino Toscano :

> On Monday, 19 September 2016 23:26:57 CEST Matteo Cafasso wrote:
> > The internal_find_block command searches all entries referring to the
> > given filesystem data block and returns a tsk_dirent structure
> > for each of them.
> >
> > For filesystems such as NTFS which do not delete the block mapping
> > when removing files, it is possible to get multiple non-allocated
> > entries for the same block.
> >
> > The gathered list of tsk_dirent structs is serialised into XDR format
> > and written to a file by the appliance.
> >
> > Signed-off-by: Matteo Cafasso 
> > ---
>
> The idea is fine, there are few notes below.
>
> > +int
> > +do_internal_find_block (const mountable_t *mountable, int64_t block)
> > +{
> > +  int ret = -1;
> > +  TSK_FS_INFO *fs = NULL;
> > +  TSK_IMG_INFO *img = NULL;  /* Used internally by tsk_fs_dir_walk */
> > +  const 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;
> > +
> > +  ret = open_filesystem (mountable->device, , );
> > +  if (ret < 0)
> > +return ret;
> > +
> > +  reply (NULL, NULL);  /* Reply message. */
> > +
> > +  ret = tsk_fs_dir_walk (fs, fs->root_inum, flags,
> > + findblk_callback, (void *) );
> > +  if (ret == 0)
> > +ret = send_file_end (0);  /* File transfer end. */
> > +  else
> > +send_file_end (1);  /* Cancel file transfer. */
> > +
> > +  fs->close (fs);
> > +  img->close (img);
> > +
> > +  return ret;
> > +}
> > +
> > +
>
> (One extra empty line.)
>
> > +static TSK_WALK_RET_ENUM
> > +findblk_callback (TSK_FS_FILE *fsfile, const char *path, void *data)
> > +{
> > +  findblk_data blkdata;
> > +  const TSK_FS_ATTR *fsattr = NULL;
> > +  int ret = 0, count = 0, index = 0;
> > +  uint64_t *block = (uint64_t *) data;
>
> Just dereference the pointer directly, so it will not be needed later
> on:
>
>   const uint64_t block = * (uint64_t *) data;
>
> Or, even better, this can be done only when needed, ...
>
> > +  const int flags = TSK_FS_FILE_WALK_FLAG_AONLY |
> TSK_FS_FILE_WALK_FLAG_SLACK;
> > +
> > +  if (entry_is_dot (fsfile))
> > +return TSK_WALK_CONT;
> > +
> > +  blkdata.found = false;
> > +  blkdata.block = *block;
>
> ... here:
>
>   blkdata.block = * (uint64_t *) data;
>
> > +  /* Retrieve block list */
> > +  count = tsk_fs_file_attr_getsize (fsfile);
> > +
> > +  for (index = 0; index < count; index++) {
> > +fsattr = tsk_fs_file_attr_get_idx (fsfile, index);
> > +
> > +if (fsattr != NULL && fsattr->flags & TSK_FS_ATTR_NONRES)
> > +  tsk_fs_attr_walk (fsattr, flags, attrwalk_callback, (void*)
> );
>
> (Minor style nitpick: space missing between "void" and "*".)
>
> Should the return value of tsk_fs_attr_walk be checked for failures,
> and return TSK_WALK_ERROR in case of error?
>
> > +/* Attribute walk, searches the given block within the FS node
> attributes. */
>
> Even if within 80 columns, I'd split it at 70 for readability.
>
> > +static TSK_WALK_RET_ENUM
> > +attrwalk_callback (TSK_FS_FILE *fsfile, TSK_OFF_T offset,
> > +   TSK_DADDR_T blkaddr, char *buf, size_t size,
> > +   TSK_FS_BLOCK_FLAG_ENUM flags, void *data)
> > +{
> > +  findblk_data *blkdata = (findblk_data *) data;
> > +
> > +  if (blkaddr == blkdata->block) {
> > +blkdata->found = true;
>
> If I read the sleuthkit API docs, blkaddr will be meaningful only if
> flags contains TSK_FS_BLOCK_FLAG_RAW.  Should attrwalk_callback check
> for it?
>
I was thinking the same but I then took a look at its usage within the
sleuthkit tool and it seems not being checked.
https://github.com/sleuthkit/sleuthkit/blob/develop/tsk/fs/ifind_lib.c#L479

Not sure whether to trust the API docs or the code. My main concern is
ignoring some relevant blocks.

I will test it with both solutions and see.

I will also fix the rest.

Thanks for the comments.


>
> Thanks,
> --
> Pino Toscano
> ___
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
>
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH 0/2] supermin: use /etc/os-release

[Pino Toscano]

On Wednesday, 31 August 2016 15:05:34 CEST Pino Toscano wrote:
> let's make supermin use /etc/os-release as primary source instead of
> the various release files in /etc; apparently distros (e.g. openSUSE)
> are starting removing them.

Ping.

Thanks,
-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2 0/3] New API - find_block

[Pino Toscano]

On Monday, 19 September 2016 23:26:56 CEST Matteo Cafasso wrote:
> v2:
> 
> - use boolean field in struct
> - move refactoring to previous series
> 
> Matteo Cafasso (3):
>   New API: internal_find_block
>   New API: find_block
>   find_block: added API tests

Patches #2, and #3 LGTM.

-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2 1/3] New API: internal_find_block

[Pino Toscano]

On Monday, 19 September 2016 23:26:57 CEST Matteo Cafasso wrote:
> The internal_find_block command searches all entries referring to the
> given filesystem data block and returns a tsk_dirent structure
> for each of them.
> 
> For filesystems such as NTFS which do not delete the block mapping
> when removing files, it is possible to get multiple non-allocated
> entries for the same block.
> 
> The gathered list of tsk_dirent structs is serialised into XDR format
> and written to a file by the appliance.
> 
> Signed-off-by: Matteo Cafasso 
> ---

The idea is fine, there are few notes below.

> +int
> +do_internal_find_block (const mountable_t *mountable, int64_t block)
> +{
> +  int ret = -1;
> +  TSK_FS_INFO *fs = NULL;
> +  TSK_IMG_INFO *img = NULL;  /* Used internally by tsk_fs_dir_walk */
> +  const 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;
> +
> +  ret = open_filesystem (mountable->device, , );
> +  if (ret < 0)
> +return ret;
> +
> +  reply (NULL, NULL);  /* Reply message. */
> +
> +  ret = tsk_fs_dir_walk (fs, fs->root_inum, flags,
> + findblk_callback, (void *) );
> +  if (ret == 0)
> +ret = send_file_end (0);  /* File transfer end. */
> +  else
> +send_file_end (1);  /* Cancel file transfer. */
> +
> +  fs->close (fs);
> +  img->close (img);
> +
> +  return ret;
> +}
> +
> +

(One extra empty line.)

> +static TSK_WALK_RET_ENUM
> +findblk_callback (TSK_FS_FILE *fsfile, const char *path, void *data)
> +{
> +  findblk_data blkdata;
> +  const TSK_FS_ATTR *fsattr = NULL;
> +  int ret = 0, count = 0, index = 0;
> +  uint64_t *block = (uint64_t *) data;

Just dereference the pointer directly, so it will not be needed later
on:

  const uint64_t block = * (uint64_t *) data;

Or, even better, this can be done only when needed, ...

> +  const int flags = TSK_FS_FILE_WALK_FLAG_AONLY | 
> TSK_FS_FILE_WALK_FLAG_SLACK;
> +
> +  if (entry_is_dot (fsfile))
> +return TSK_WALK_CONT;
> +
> +  blkdata.found = false;
> +  blkdata.block = *block;

... here:

  blkdata.block = * (uint64_t *) data;

> +  /* Retrieve block list */
> +  count = tsk_fs_file_attr_getsize (fsfile);
> +
> +  for (index = 0; index < count; index++) {
> +fsattr = tsk_fs_file_attr_get_idx (fsfile, index);
> +
> +if (fsattr != NULL && fsattr->flags & TSK_FS_ATTR_NONRES)
> +  tsk_fs_attr_walk (fsattr, flags, attrwalk_callback, (void*) );

(Minor style nitpick: space missing between "void" and "*".)

Should the return value of tsk_fs_attr_walk be checked for failures,
and return TSK_WALK_ERROR in case of error?

> +/* Attribute walk, searches the given block within the FS node attributes. */

Even if within 80 columns, I'd split it at 70 for readability.

> +static TSK_WALK_RET_ENUM
> +attrwalk_callback (TSK_FS_FILE *fsfile, TSK_OFF_T offset,
> +   TSK_DADDR_T blkaddr, char *buf, size_t size,
> +   TSK_FS_BLOCK_FLAG_ENUM flags, void *data)
> +{
> +  findblk_data *blkdata = (findblk_data *) data;
> +
> +  if (blkaddr == blkdata->block) {
> +blkdata->found = true;

If I read the sleuthkit API docs, blkaddr will be meaningful only if
flags contains TSK_FS_BLOCK_FLAG_RAW.  Should attrwalk_callback check
for it?

Thanks,
-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs