Bug 167034 - Review Request: foremost -- Recover files by "carving" them from a disk image
Summary: Review Request: foremost -- Recover files by "carving" them from a disk image
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Michael A. Peters
QA Contact: David Lawrence
URL: http://foremost.sf.net/
Whiteboard:
: 204227 (view as bug list)
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2005-08-29 18:12 UTC by Toshio Kuratomi
Modified: 2007-11-30 22:11 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2005-10-03 17:31:01 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
Patch to correct path for default config file (473 bytes, patch)
2005-09-29 02:09 UTC, Paul W. Frields
no flags Details | Diff

Description Toshio Kuratomi 2005-08-29 18:12:36 UTC
Spec Name or Url: http://www.tiki-lounge.com/~toshio/fedora/foremost.spec
SRPM Name or Url: http://www.tiki-lounge.com/~toshio/fedora/foremost-1.0-1.src.rpm
Description:
Foremost recovers files based on their headers, footers, and internal
data structures. This process is commonly referred to as data carving.
Foremost can work on a raw disk drive or image file generated by dd.  The
headers and footers can be specified by a configuration file or you can use
command line switches to specify built-in file types. These built-in types
look at the data structures of a given file format allowing for a more
reliable and faster recovery.

Comment 1 Toshio Kuratomi 2005-08-29 18:54:09 UTC
Note: foremost is licensed as Public Domain on its web page.  (Due to being
created using US tax dollars).  However, it seems to use at least two files
which are licensed under the GPL -- api.c seems to have been ported from the
chicago project.  extract.c may have been created later and is licensed under
the GPL.  I don't see any issue with licensing the package as a whole as GPL in
this case so I've done that for this package.

I've also contacted the upstream mintainer to see if he has thoughts on the issue.

Comment 2 Toshio Kuratomi 2005-08-30 17:37:10 UTC
From Nick Mikus -- foremost maintainer:
> Thanks for the patches I had been meaning to clean up my lazy coding but have 
> not gotten around to it.  I will post a new version tomorrow.
 
> In regards to the licesing I releasing it under GPL is fine.  The old version 
> was released as public domain but my modifications were done at an academic 
> institution and was intended to be released under GPL.  Unother examply of lazy 
> documentation, seeing a pattern here?

Comment 3 Toshio Kuratomi 2005-09-13 15:34:35 UTC
Spec Name or Url: http://www.tiki-lounge.com/~toshio/fedora/foremost.spec
SRPM Name or Url: http://www.tiki-lounge.com/~toshio/fedora/foremost-1.0-2.src.rpm

New packages for an upstream update.

If there's movement on a LiveCD for data recovery, foremost would be a good
candidate.

License has been clarified with the FSF.  Because of the chicago code
(http://chicago.sf.net) the program as a whole is GPL.  However, the bulk of the
code is Public Domain (due to the US government paying employees to create the
program.)

Comment 4 Michael A. Peters 2005-09-28 23:24:29 UTC
md5sum matches upstream
specfile looks good
rpmlint says its clean on both src and binary rpm
package builds cleanly in mock with just a couple of warnings.
installs and removes cleanly

I did not test the binary yet, I need to read the man page thoroughly before I
do that. But otherwise, it looks good from a packaging perspective.

Comment 5 Michael A. Peters 2005-09-28 23:47:06 UTC
OK - tested binary.
It complains with:

foremost: /usr/local/etc/foremost.conf: No such file or directory bu runs.
When I copied sample file to /etc - still complained.
When I copied sample file to /usr/local/etc - no complaint.

-=-
tested on a .bin file (without the matching .cue file)
It extracted two pdfs from the file, but Adobe Acroread was unable to read them,
stating they were damaged - tried to repair but result contained errors.

tested on an iso image (that is same contents as the .bin file) and it
succesfully extracted a bunch of pdf files, every one I tested was readable.

I did not test on a dd created image or on a hard disk.

-=-
My suggestion -
put the sample conf file in /etc as %config(noreplace) and patch the project to
look in /etc for default conf file instead of /usr/local/etc

Comment 6 Paul W. Frields 2005-09-29 02:09:54 UTC
Created attachment 119397 [details]
Patch to correct path for default config file

Unfortunately, that path is hardcoded into the config.c file (lazy, tsk, tsk!).
 This patch will correct it, but I think it will be between the maintainer and
the FE approver to decide whether to fix it this way, or ask upstream to do
something more rational.

Comment 7 Michael A. Peters 2005-09-29 04:03:34 UTC
Given that the Makefile supports a configuration file path as an option,
upstream should honer that option when make is run.

It should be filed as a bug with upstream.
The patch is fine with me, but a cleaner solution might be to use sed within
%prep to fix it - so that it can be replaced with the %_sysconfdir and thus
build on systems that have a custom %_sysconfdir (like /opt/etc or whatever)

cp config.c config.c.org
sed -e s?"/usr/local/etc"?"%{_sysconfdir}"? < config.c.orig > config.c

(or use sed -i - as he already does in one spot)

Whatever Toshio wants to do until upstream fixes it would be fine with me.

Comment 8 Paul W. Frields 2005-09-29 12:50:06 UTC
Whoops, obviously I didn't spend any time reading the Makefile. ;-)  Why not
just do in %build:

BIN=%{_bindir} MAN=%{_mandir}/man1 CONF=%{_sysconfdir} make -e

Then the package probably need not wait for upstream, right?

Comment 9 Paul W. Frields 2005-09-29 12:52:43 UTC
Er, forgot to add, need to do similarly in %install of course.

Comment 10 Toshio Kuratomi 2005-09-29 15:12:09 UTC
config.c still needs to be modified.  So sed will still be needed.  Give me a
few hours to see if there's an easy patch to send upstream before implementing
that, though.

Comment 11 Toshio Kuratomi 2005-09-29 17:57:31 UTC
Spec Name or Url: http://www.tiki-lounge.com/~toshio/fedora/foremost.spec
SRPM Name or Url: http://www.tiki-lounge.com/~toshio/fedora/foremost-1.0-3.src.rpm

Okay -- Here's a new version.  I modified the Makefile to do the sed
substitution on config.c and then fed make the proper values of CONF in the
%build and %install.  It's basically what an autoconf'd configure would do so it
should be easy to adapt should upstream ever convert to autoconf.

Sending patch upstream as well.

Comment 12 Michael A. Peters 2005-09-29 20:36:24 UTC
OK - as far as I can see, this package meets everything specified in the review
process at http://fedoraproject.org/wiki/PackageReviewGuidelines

I tested build on both i386 and ppc flavors of Fedora Core 4.

* Package installs and removes cleanly (no leftover directories etc.)
* Permissions are proper, %files section is good
* %clean section is proper
* consistent use of macros
* Includes license file
* Builds clean in mock, builds on x86 and ppc
* -- x86_64 not tested
* Program works, does what it says
* source md5sum matches upstream

-=-
I've not yet been approved as a review, but I believe this to be worthy of
approval for Fedora Extras according to the guidelines at
http://fedoraproject.org/wiki/PackageReviewGuidelines

Comment 13 Michael A. Peters 2005-09-29 20:39:07 UTC
One edit - attribute on /etc/foremost.conf might need to be changed to 644 from 444

Comment 14 Toshio Kuratomi 2005-09-29 21:25:26 UTC
Spec Name or Url: http://www.tiki-lounge.com/~toshio/fedora/foremost.spec
SRPM Name or Url: http://www.tiki-lounge.com/~toshio/fedora/foremost-1.0-4.src.rpm

Changes:
- chmod 0644 foremost.conf file.



Comment 15 Michael A. Peters 2005-09-30 02:21:14 UTC
Approved

Comment 16 Toshio Kuratomi 2005-10-03 17:31:01 UTC
Imported to cvs; owners.list updated; and built for devel.

Comment 17 Till Maas 2006-09-19 14:33:52 UTC
*** Bug 204227 has been marked as a duplicate of this bug. ***


Note You need to log in before you can comment on or make changes to this bug.