3:59 p.m.
The find_inode API allows the User to search all the entries referring
to a given inode and returns a tsk_dirent structure for each of them.
As I didn't want to change unrelated code, there is a little bit
of code duplication at the moment. Plan is to refactor the logic
in a dedicated set of patches.
Matteo Cafasso (3):
New API: internal_find_inode
New API: find_inode
find_inode: added API tests
daemon/tsk.c | 75 ++++++++++++++++++++++++++++++++++++++++++++
generator/actions.ml | 21 +++++++++++++
src/MAX_PROC_NR | 2 +-
src/tsk.c | 26 +++++++++++++++
tests/tsk/Makefile.am | 3 +-
tests/tsk/test-find-inode.sh | 66 ++++++++++++++++++++++++++++++++++++++
6 files changed, 191 insertions(+), 2 deletions(-)
create mode 100755 tests/tsk/test-find-inode.sh
--
2.9.3
3:59 p.m.
New subject: [PATCH 1/3] New API: internal_find_inode
The internal_find_inode command searches all entries referring to the
given inode and returns a tsk_dirent structure for each of them.
The command is able to retrieve information regarding deleted
or unaccessible files where other commands such as stat or find
would fail.
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 <noxdafox(a)gmail.com>
---
daemon/tsk.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++
generator/actions.ml | 9 +++++++
src/MAX_PROC_NR | 2 +-
3 files changed, 85 insertions(+), 1 deletion(-)
diff --git a/daemon/tsk.c b/daemon/tsk.c
index dd368d7..beb92a3 100644
--- a/daemon/tsk.c
+++ b/daemon/tsk.c
@@ -44,6 +44,7 @@ enum tsk_dirent_flags {
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 ifind_callback (TSK_FS_FILE *, const char *, void *);
static char file_type (TSK_FS_FILE *);
static int file_flags (TSK_FS_FILE *fsfile);
static void file_metadata (TSK_FS_META *, guestfs_int_tsk_dirent *);
@@ -78,6 +79,35 @@ do_internal_filesystem_walk (const mountable_t *mountable)
return ret;
}
+int
+do_internal_find_inode (const mountable_t *mountable, int64_t inode)
+{
+ 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, &img, &fs);
+ if (ret < 0)
+ return ret;
+
+ reply (NULL, NULL); /* Reply message. */
+
+ ret = tsk_fs_dir_walk (fs, fs->root_inum, flags, ifind_callback,
+ (void *) &inode);
+ 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.
*/
@@ -141,6 +171,51 @@ fswalk_callback (TSK_FS_FILE *fsfile, const char *path, void *data)
return ret;
}
+/* Find inode callback, it gets called on every FS node.
+ * Parse the node, encode it into an XDR structure and send it to the appliance.
+ * Return TSK_WALK_CONT on success, TSK_WALK_ERROR on error.
+ */
+static TSK_WALK_RET_ENUM
+ifind_callback (TSK_FS_FILE *fsfile, const char *path, void *data)
+{
+ int ret = 0;
+ CLEANUP_FREE char *fname = NULL;
+ uint64_t *inode = (uint64_t *) data;
+ struct guestfs_int_tsk_dirent dirent;
+
+ if (*inode != fsfile->name->meta_addr)
+ return TSK_WALK_CONT;
+
+ /* Ignore ./ and ../ */
+ if (TSK_FS_ISDOT (fsfile->name->name)) {
+ /* Root is represented as . */
+ if (fsfile->fs_info->root_inum != fsfile->name->meta_addr ||
+ strcmp (fsfile->name->name, "."))
+ return TSK_WALK_CONT;
+ }
+
+ /* Build the full relative path of the entry */
+ ret = asprintf (&fname, "%s%s", path, fsfile->name->name);
+ if (ret < 0) {
+ perror ("asprintf");
+ return TSK_WALK_ERROR;
+ }
+
+ /* Set dirent fields */
+ memset (&dirent, 0, sizeof dirent);
+
+ dirent.tsk_inode = fsfile->name->meta_addr;
+ dirent.tsk_type = file_type (fsfile);
+ dirent.tsk_name = fname;
+ dirent.tsk_flags = file_flags (fsfile);
+
+ file_metadata (fsfile->meta, &dirent);
+
+ ret = send_dirent_info (&dirent);
+
+ return (ret == 0) ? TSK_WALK_CONT : TSK_WALK_ERROR;;
+}
+
/* Inspect fsfile to identify its type. */
static char
file_type (TSK_FS_FILE *fsfile)
diff --git a/generator/actions.ml b/generator/actions.ml
index eccef00..449443f 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -13205,6 +13205,15 @@ only the unallocated blocks will be extracted.
This is useful to detect hidden data or to retrieve deleted files
which data units have not been overwritten yet." };
+ { defaults with
+ name = "internal_find_inode"; added = (1, 35, 2);
+ style = RErr, [Mountable "device"; Int64 "inode"; FileOut
"filename";], [];
+ proc_nr = Some 469;
+ visibility = VInternal;
+ optional = Some "libtsk";
+ shortdesc = "search the entries associated to the given inode";
+ longdesc = "Internal function for find_inode." };
+
]
(* Non-API meta-commands available only in guestfish.
diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR
index 1023289..5ef9d24 100644
--- a/src/MAX_PROC_NR
+++ b/src/MAX_PROC_NR
@@ -1 +1 @@
-468
+469
--
2.9.3
6:19 a.m.
New subject: [PATCH 1/3] New API: internal_find_inode
On Wednesday, 24 August 2016 23:59:54 CEST Matteo Cafasso wrote:
The internal_find_inode command searches all entries referring to
the
given inode and returns a tsk_dirent structure for each of them.
The command is able to retrieve information regarding deleted
or unaccessible files where other commands such as stat or find
would fail.
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 <noxdafox(a)gmail.com>
---
daemon/tsk.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++
generator/actions.ml | 9 +++++++
src/MAX_PROC_NR | 2 +-
3 files changed, 85 insertions(+), 1 deletion(-)
diff --git a/daemon/tsk.c b/daemon/tsk.c
index dd368d7..beb92a3 100644
--- a/daemon/tsk.c
+++ b/daemon/tsk.c
@@ -44,6 +44,7 @@ enum tsk_dirent_flags {
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 ifind_callback (TSK_FS_FILE *, const char *, void *);
static char file_type (TSK_FS_FILE *);
static int file_flags (TSK_FS_FILE *fsfile);
static void file_metadata (TSK_FS_META *, guestfs_int_tsk_dirent *);
@@ -78,6 +79,35 @@ do_internal_filesystem_walk (const mountable_t *mountable)
return ret;
}
+int
+do_internal_find_inode (const mountable_t *mountable, int64_t inode)
+{
+ 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, &img, &fs);
+ if (ret < 0)
+ return ret;
+
+ reply (NULL, NULL); /* Reply message. */
+
+ ret = tsk_fs_dir_walk (fs, fs->root_inum, flags, ifind_callback,
+ (void *) &inode);
Here 'inode' is int64_t ...
+ 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.
*/
@@ -141,6 +171,51 @@ fswalk_callback (TSK_FS_FILE *fsfile, const char *path, void *data)
return ret;
}
+/* Find inode callback, it gets called on every FS node.
+ * Parse the node, encode it into an XDR structure and send it to the appliance.
+ * Return TSK_WALK_CONT on success, TSK_WALK_ERROR on error.
+ */
+static TSK_WALK_RET_ENUM
+ifind_callback (TSK_FS_FILE *fsfile, const char *path, void *data)
+{
+ int ret = 0;
+ CLEANUP_FREE char *fname = NULL;
+ uint64_t *inode = (uint64_t *) data;
... but then here you cast it as uint64_t. I don't see an immediate
issue with it, but please keep in mind that this kind of things in C
(i.e. cast a typed pointer to void*, and cast it back as some other
type) is dangerous, and should be avoided as much as possible.
I guess the proper solution would be adding a new UInt64 type (and UInt
as well) to the generator...
Thanks,
--
Pino Toscano
8:13 a.m.
New subject: [PATCH 1/3] New API: internal_find_inode
2016-08-25 14:19 GMT+03:00 Pino Toscano <ptoscano(a)redhat.com>:
On Wednesday, 24 August 2016 23:59:54 CEST Matteo Cafasso wrote:
> The internal_find_inode command searches all entries referring to the
> given inode and returns a tsk_dirent structure for each of them.
>
> The command is able to retrieve information regarding deleted
> or unaccessible files where other commands such as stat or find
> would fail.
>
> 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 <noxdafox(a)gmail.com>
> ---
> daemon/tsk.c | 75 ++++++++++++++++++++++++++++++
++++++++++++++++++++++
> generator/actions.ml | 9 +++++++
> src/MAX_PROC_NR | 2 +-
> 3 files changed, 85 insertions(+), 1 deletion(-)
>
> diff --git a/daemon/tsk.c b/daemon/tsk.c
> index dd368d7..beb92a3 100644
> --- a/daemon/tsk.c
> +++ b/daemon/tsk.c
> @@ -44,6 +44,7 @@ enum tsk_dirent_flags {
>
> 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 ifind_callback (TSK_FS_FILE *, const char *,
void *);
> static char file_type (TSK_FS_FILE *);
> static int file_flags (TSK_FS_FILE *fsfile);
> static void file_metadata (TSK_FS_META *, guestfs_int_tsk_dirent *);
> @@ -78,6 +79,35 @@ do_internal_filesystem_walk (const mountable_t
*mountable)
> return ret;
> }
>
> +int
> +do_internal_find_inode (const mountable_t *mountable, int64_t inode)
> +{
> + 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, &img, &fs);
> + if (ret < 0)
> + return ret;
> +
> + reply (NULL, NULL); /* Reply message. */
> +
> + ret = tsk_fs_dir_walk (fs, fs->root_inum, flags, ifind_callback,
> + (void *) &inode);
Here 'inode' is int64_t ...
> + 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.
> */
> @@ -141,6 +171,51 @@ fswalk_callback (TSK_FS_FILE *fsfile, const char
*path, void *data)
> return ret;
> }
>
> +/* Find inode callback, it gets called on every FS node.
> + * Parse the node, encode it into an XDR structure and send it to the
appliance.
> + * Return TSK_WALK_CONT on success, TSK_WALK_ERROR on error.
> + */
> +static TSK_WALK_RET_ENUM
> +ifind_callback (TSK_FS_FILE *fsfile, const char *path, void *data)
> +{
> + int ret = 0;
> + CLEANUP_FREE char *fname = NULL;
> + uint64_t *inode = (uint64_t *) data;
... but then here you cast it as uint64_t. I don't see an immediate
issue with it, but please keep in mind that this kind of things in C
(i.e. cast a typed pointer to void*, and cast it back as some other
type) is dangerous, and should be avoided as much as possible.
I do agree with your point. Reason behind the cast is that the comparison
few lines below is done against a uint64_t.
Moreover, other related APIs parameters are int64_t (download_inode and
download_blocks). This would make the APIs inconsistent between each other.
If I picture myself as a user I would wonder why download_inode() wants a
int64_t and find_inode() wants a uint64_t.
I guess the proper solution would be adding a new UInt64 type (and UInt
as well) to the generator...
I am not familiar with the generator architecture. What would adding a type
to it imply from the practical point of view?
Thanks,
--
Pino Toscano
_______________________________________________
Libguestfs mailing list
Libguestfs(a)redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
3:59 p.m.
New subject: [PATCH 2/3] New API: find_inode
Library's counterpart of the daemon's internal_find_inode 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_inode command.
Signed-off-by: Matteo Cafasso <noxdafox(a)gmail.com>
---
generator/actions.ml | 12 ++++++++++++
src/tsk.c | 26 ++++++++++++++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/generator/actions.ml b/generator/actions.ml
index 449443f..7e19fa0 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -3702,6 +3702,18 @@ Unknown file type
=back" };
+ { defaults with
+ name = "find_inode"; added = (1, 35, 2);
+ style = RStructList ("dirents", "tsk_dirent"), [Mountable
"device"; Int64 "inode";], [];
+ optional = Some "libtsk";
+ progress = true; cancellable = true;
+ shortdesc = "search the entries associated to the given inode";
+ longdesc = "\
+Searches all the entries associated with the given inode.
+
+For each entry, a C<tsk_dirent> structure is returned.
+See C<filesystem_walk> for more information about C<tsk_dirent>
structures." };
+
]
(* daemon_functions are any functions which cause some action
diff --git a/src/tsk.c b/src/tsk.c
index 90177ab..f7b854e 100644
--- a/src/tsk.c
+++ b/src/tsk.c
@@ -63,6 +63,32 @@ guestfs_impl_filesystem_walk (guestfs_h *g, const char *mountable)
return parse_filesystem_walk (g, fp); /* caller frees */
}
+struct guestfs_tsk_dirent_list *
+guestfs_impl_find_inode (guestfs_h *g, const char *mountable, int64_t inode)
+{
+ int ret = 0;
+ CLEANUP_FCLOSE FILE *fp = NULL;
+ CLEANUP_UNLINK_FREE char *tmpfile = NULL;
+
+ ret = guestfs_int_lazy_make_tmpdir (g);
+ if (ret < 0)
+ return NULL;
+
+ tmpfile = safe_asprintf (g, "%s/find_inode%d", g->tmpdir,
++g->unique);
+
+ ret = guestfs_internal_find_inode (g, mountable, inode, tmpfile);
+ if (ret < 0)
+ return NULL;
+
+ fp = fopen (tmpfile, "r");
+ if (fp == NULL) {
+ perrorf (g, "fopen: %s", tmpfile);
+ return NULL;
+ }
+
+ return parse_filesystem_walk (g, fp); /* caller frees */
+}
+
/* Parse the file content and return dirents list.
* Return a list of tsk_dirent on success, NULL on error.
*/
--
2.9.3
3:59 p.m.
New subject: [PATCH 3/3] find_inode: added API tests
NTFS file system always has the MFT file at inode 0. This reliable
information helps testing the API.
Signed-off-by: Matteo Cafasso <noxdafox(a)gmail.com>
---
tests/tsk/Makefile.am | 3 +-
tests/tsk/test-find-inode.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 68 insertions(+), 1 deletion(-)
create mode 100755 tests/tsk/test-find-inode.sh
diff --git a/tests/tsk/Makefile.am b/tests/tsk/Makefile.am
index 0b50839..07c74f9 100644
--- a/tests/tsk/Makefile.am
+++ b/tests/tsk/Makefile.am
@@ -20,7 +20,8 @@ include $(top_srcdir)/subdir-rules.mk
TESTS = \
test-download-inode.sh \
test-download-blocks.sh \
- test-filesystem-walk.sh
+ test-filesystem-walk.sh \
+ test-find-inode.sh
TESTS_ENVIRONMENT = $(top_builddir)/run --test
diff --git a/tests/tsk/test-find-inode.sh b/tests/tsk/test-find-inode.sh
new file mode 100755
index 0000000..f69fcf6
--- /dev/null
+++ b/tests/tsk/test-find-inode.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-inode command.
+
+if [ -n "$SKIP_TEST_FIND_INODE_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 <<EOF
+run
+find-inode /dev/sda2 0
+EOF
+)
+
+# test $MFT is in the list
+echo $output | grep -zq '{ tsk_inode: 0
+tsk_type: r
+tsk_size: .*
+tsk_name: \$MFT
+tsk_flags: 1
+tsk_atime_sec: .*
+tsk_atime_nsec: .*
+tsk_mtime_sec: .*
+tsk_mtime_nsec: .*
+tsk_ctime_sec: .*
+tsk_ctime_nsec: .*
+tsk_crtime_sec: .*
+tsk_crtime_nsec: .*
+tsk_nlink: 1
+tsk_link:
+tsk_spare1: 0 }'
+if [ $? != 0 ]; then
+ echo "$0: \$MFT not found in files list."
+ echo "File list:"
+ echo $output
+ exit 1
+fi
--
2.9.3
6:09 a.m.
On Wednesday, 24 August 2016 23:59:53 CEST Matteo Cafasso wrote:
The find_inode API allows the User to search all the entries
referring
to a given inode and returns a tsk_dirent structure for each of them.
As I didn't want to change unrelated code, there is a little bit
of code duplication at the moment. Plan is to refactor the logic
in a dedicated set of patches.
The general idea looks ok, but I'd rather see the duplication dealt
with sooner than later.
Thanks,
--
Pino Toscano
8:05 a.m.
2016-08-25 14:09 GMT+03:00 Pino Toscano <ptoscano(a)redhat.com>:
On Wednesday, 24 August 2016 23:59:53 CEST Matteo Cafasso wrote:
> The find_inode API allows the User to search all the entries referring
> to a given inode and returns a tsk_dirent structure for each of them.
>
> As I didn't want to change unrelated code, there is a little bit
> of code duplication at the moment. Plan is to refactor the logic
> in a dedicated set of patches.
The general idea looks ok, but I'd rather see the duplication dealt
with sooner than later.
In the previous submissions, non related changes were rejected therefore I
thought that was the custom.
Moreover I'll add another API find_block (block_number --> tsk_dirents
referring to it) and I think is easier to refactor the code once all the
use cases are in place as the picture gets more clear.
Thanks,
--
Pino Toscano
_______________________________________________
Libguestfs mailing list
Libguestfs(a)redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
8:12 a.m.
On Thursday, 25 August 2016 16:05:47 CEST NoxDaFox wrote:
2016-08-25 14:09 GMT+03:00 Pino Toscano <ptoscano(a)redhat.com>:
> On Wednesday, 24 August 2016 23:59:53 CEST Matteo Cafasso wrote:
> > The find_inode API allows the User to search all the entries referring
> > to a given inode and returns a tsk_dirent structure for each of them.
> >
> > As I didn't want to change unrelated code, there is a little bit
> > of code duplication at the moment. Plan is to refactor the logic
> > in a dedicated set of patches.
>
> The general idea looks ok, but I'd rather see the duplication dealt
> with sooner than later.
>
In the previous submissions, non related changes were rejected therefore I
thought that was the custom.
I don't see how the two cases are the same: what was "rejected" was a
single patch contaning both additions documented in the commit message,
and unrelated changes such as formatting fixes. I remember to have said
that it's a no-go as *single* patch, but they can be sent (and
integrated) as different commits.
In this case, refactoring and de-duplication of code should be done in
different commits as well.
Moreover I'll add another API find_block (block_number -->
tsk_dirents
referring to it) and I think is easier to refactor the code once all the
use cases are in place as the picture gets more clear.
More reasons to refactor *before*: since you already planned more APIs,
it's easier to just refactor one in advance with all the common code
needed, and use it later. Also, it eases a lot the review of the
patches, since it will be less code added.
--
Pino Toscano
8:15 a.m.
2016-08-25 16:12 GMT+03:00 Pino Toscano <ptoscano(a)redhat.com>:
On Thursday, 25 August 2016 16:05:47 CEST NoxDaFox wrote:
> 2016-08-25 14:09 GMT+03:00 Pino Toscano <ptoscano(a)redhat.com>:
>
> > On Wednesday, 24 August 2016 23:59:53 CEST Matteo Cafasso wrote:
> > > The find_inode API allows the User to search all the entries
referring
> > > to a given inode and returns a tsk_dirent structure for each of them.
> > >
> > > As I didn't want to change unrelated code, there is a little bit
> > > of code duplication at the moment. Plan is to refactor the logic
> > > in a dedicated set of patches.
> >
> > The general idea looks ok, but I'd rather see the duplication dealt
> > with sooner than later.
> >
> In the previous submissions, non related changes were rejected therefore
I
> thought that was the custom.
I don't see how the two cases are the same: what was "rejected" was a
single patch contaning both additions documented in the commit message,
and unrelated changes such as formatting fixes. I remember to have said
that it's a no-go as *single* patch, but they can be sent (and
integrated) as different commits.
Then I totally misunderstood.
In this case, refactoring and de-duplication of code should be done in
different commits as well.
> Moreover I'll add another API find_block (block_number
--> tsk_dirents
> referring to it) and I think is easier to refactor the code once all the
> use cases are in place as the picture gets more clear.
More reasons to refactor *before*: since you already planned more APIs,
it's easier to just refactor one in advance with all the common code
needed, and use it later. Also, it eases a lot the review of the
patches, since it will be less code added.
Seems reasonable, I'll proceed and re-submit then.
--
Pino Toscano
_______________________________________________
Libguestfs mailing list
Libguestfs(a)redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
3057
days inactive
3058
days old
9 comments
3 participants
participants (3)
-
Matteo Cafasso -
NoxDaFox
-
Pino Toscano