Re: [Libguestfs] [PATCH 0/3] New API: find_inode

[NoxDaFox]

2016-08-25 16:12 GMT+03:00 Pino Toscano :

> On Thursday, 25 August 2016 16:05:47 CEST NoxDaFox wrote:
> > 2016-08-25 14:09 GMT+03:00 Pino Toscano :
> >
> > > 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@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/3] New API: find_inode

[Pino Toscano]

On Thursday, 25 August 2016 16:05:47 CEST NoxDaFox wrote:
> 2016-08-25 14:09 GMT+03:00 Pino Toscano :
> 
> > 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

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 0/3] New API: find_inode

[NoxDaFox]

2016-08-25 14:09 GMT+03:00 Pino Toscano :

> 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@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/3] New API: find_inode

[Pino Toscano]

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

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

[Libguestfs] [PATCH 0/3] New API: find_inode

[Matteo Cafasso]

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

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