Bug 495564

Summary: Review Request: libguestfs - Access and modify virtual machine disk images
Product: [Fedora] Fedora Reporter: Richard W.M. Jones <rjones>
Component: Package ReviewAssignee: Jim Meyering <meyering>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, hbrock, meyering, notting
Target Milestone: ---Flags: meyering: fedora‑review+
kevin: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-05-11 14:16:50 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On: 495563    
Bug Blocks:    

Description Richard W.M. Jones 2009-04-13 16:03:31 EDT
Spec URL: http://www.annexia.org/tmp/libguestfs.spec
SRPM URL: http://et.redhat.com/~rjones/libguestfs/files/libguestfs-0.9-1.fc11.src.rpm
Description: 
Libguestfs is a library for accessing and modifying guest disk images.
Amongst the things this is good for: making batch configuration
changes to guests, getting disk used/free statistics (see also:
virt-df), migrating between virtualization systems (see also:
virt-p2v), performing partial backups, performing partial guest
clones, cloning guests and changing registry/UUID/hostname info, and
much else besides.

Libguestfs uses Linux kernel and qemu code, and can access any type of
guest filesystem that Linux and qemu can, including but not limited
to: ext2/3/4, btrfs, FAT and NTFS, LVM, many different disk partition
schemes, qcow, qcow2, vmdk.

Libguestfs provides ways to enumerate guest storage (eg. partitions,
LVs, what filesystem is in each LV, etc.).  It can also run commands
in the context of the guest.  Also you can access filesystems over FTP.

Libguestfs is a library that can be linked with C and C++ management
programs.

See also the 'guestfish' package for shell scripting and command line
access.

For Perl bindings, see 'libguestfs-perl'.

For OCaml bindings, see 'libguestfs-ocaml-devel'.

For Python bindings, see 'libguestfs-python'.
Comment 1 Richard W.M. Jones 2009-04-14 17:57:27 EDT
Now ready for review, but still dependent on bug 495563.
Comment 4 Richard W.M. Jones 2009-04-22 05:45:13 EDT
Version 1.0.6 released, which includes Java bindings, hence more
subpackages.

There is some bug with qemu on PPC.  It builds OK but does not run on
PPC, so I have to disable PPC for the time being.  This is unfortunate, but
as usual because I don't have a PPC machine to run this on, there's not
a lot I can do to look into the bug at this time.

Spec URL: http://www.annexia.org/tmp/libguestfs.spec
SRPM URL: http://www.annexia.org/tmp/libguestfs-1.0.6-1.fc11.src.rpm

Scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1314307
Comment 6 Richard W.M. Jones 2009-04-24 10:20:13 EDT
rpmlint says:

libguestfs-java.x86_64: E: library-without-ldconfig-postin /usr/lib64/libguestfs_jni.so.1.0.12
libguestfs-java.x86_64: E: library-without-ldconfig-postun /usr/lib64/libguestfs_jni.so.1.0.12

[These libraries are used/loaded internally by the JVM.  I'm sure
they don't require ldconfig]

perl-libguestfs.x86_64: E: non-standard-executable-perm /usr/lib64/perl5/vendor_perl/5.10.0/x86_64-linux-thread-multi/auto/Sys/Guestfs/Guestfs.so 0555

[This is how Perl installs the file]

perl-libguestfs.x86_64: W: spurious-executable-perm /usr/share/doc/perl-libguestfs-1.0.12/examples/lvs.pl
perl-libguestfs.x86_64: W: doc-file-dependency /usr/share/doc/perl-libguestfs-1.0.12/examples/lvs.pl /usr/bin/perl

[rpmlint being fussy - I want that demo script to be executable
so that people can run it.  It doesn't do anything dangerous]

python-libguestfs.x86_64: W: devel-file-in-non-devel-package /usr/lib64/python2.6/site-packages/libguestfsmod.so

[Seems to be OK, packaged as per the Python guidelines]

13 packages and 0 specfiles checked; 3 errors, 3 warnings.
Comment 7 Jim Meyering 2009-04-24 12:03:46 EDT
I'll review it.
Comment 9 Richard W.M. Jones 2009-04-28 17:49:03 EDT
In the spec file in comment 8 I disabled the %check section
because it took a long time to run.  This build has the %check
tests reenabled, and it works, although it does take a pretty
long time to complete (almost 1 hour):

http://koji.fedoraproject.org/koji/taskinfo?taskID=1327042
Comment 10 Jim Meyering 2009-05-06 02:40:05 EDT
hi Rich,

I just noticed this in output from a from-git build on F10:

chmod 644 blib/arch/auto/Sys/Guestfs/Guestfs.bs
cp lib/Sys/Guestfs.pm blib/lib/Sys/Guestfs.pm
Please specify prototyping behavior for Guestfs.xs (see perlxs manual)

Is it worth addressing the "please specify..." part?
Comment 11 Jim Meyering 2009-05-06 08:12:14 EDT
Hi Rich,

In src/guestfs.c, you probably want to use a read wrapper
so you don't have to worry about EINTR and EAGAIN
(the code should retry, not fail in those cases).
It looks like there are a few others that would benefit.

  while (!cancel && (r = read (fd, buf, sizeof buf)) > 0) {
    err = send_file_data_sync (g, buf, r);
    if (err < 0) {
      if (err == -2)		/* daemon sent cancellation */
	send_file_cancellation_sync (g);
      return err;
    }
  }
Comment 12 Jim Meyering 2009-05-06 08:13:12 EDT
Please use an unsigned type for length-only variables like "len" here:
This is partly stylistic, and partly to keep reviewers from wondering
if they can be negative.  Added bonus, use a wider type like
size_t and you don't have to worry about overflow if there's
ever an input of 2^32 bytes or longer.

guestfs__receive_file_sync (guestfs_h *g, const char *filename)
{
  void *buf;
  int fd, r, len;

  fd = open (filename, O_WRONLY|O_CREAT|O_TRUNC|O_NOCTTY, 0666);
  if (fd == -1) {
    perrorf (g, "open: %s", filename);
    goto cancel;
  }
Comment 13 Jim Meyering 2009-05-06 08:13:39 EDT
This realloc (from guestfsd.c) leaks upon failure:

      if (r > 0 && stdoutput) {
	so_size += r;
	*stdoutput = realloc (*stdoutput, so_size);
	if (*stdoutput == NULL) {
	  perror ("realloc");
	  *stdoutput = NULL;
	  continue;
	}
	memcpy (*stdoutput + so_size - r, buf, r);
      }

Also, this stmt is unnecessary:

	  *stdoutput = NULL;

Same thing with unnecessary code below:

    if (*stdoutput == NULL) {
      perror ("realloc");
      *stdoutput = NULL;
    } else

and with stderror:
 
    if (*stderror == NULL) {
      perror ("realloc");
      *stderror = NULL;

More importantly, it looks like this function can return
0 even when it has set *stdoutput to NULL, and that would
make the following from file.c deference out==NULL:

    r = command (&out, &err, "file", "-bsL", buf, NULL);
    if (freeit) free (buf);

    if (r == -1) {
      free (out);
      reply_with_error ("file: %s: %s", path, err);
      free (err);
      return NULL;
    }
    free (err);

    /* We need to remove the trailing \n from output of file(1). */
    len = strlen (out);
    if (out[len-1] == '\n')
      out[len-1] = '\0';

Also, so_size and se_size become invalid upon failed realloc.
You should change it so that they are increased only if realloc
succeeds.
Comment 14 Jim Meyering 2009-05-06 08:14:44 EDT
I've built the RPM from a git clone on F10:

  make dist && rpmbuild -ta libguestfs-1.0.18.tar.gz

It satisfies all MUST requirements and almost all SHOULDs.
The only missing SHOULD is gettext support.

However, I'm still getting the "make check" failures
we've been looking at for some time.  Probably not
worth worrying about if I'm the only one.
Comment 15 Jim Meyering 2009-05-06 08:16:52 EDT
Since "make check" doesn't pass for me, I haven't yet tried running things under valgrind.  Have you?
Comment 16 Jim Meyering 2009-05-06 08:49:27 EDT
oops. re #14, there's no SHOULD-use-gettext.  I didn't read carefully enough the one that mentioned translations.
Comment 17 Richard W.M. Jones 2009-05-07 16:26:40 EDT
(In reply to comment #10)
> Please specify prototyping behavior for Guestfs.xs (see perlxs manual)
> 
> Is it worth addressing the "please specify..." part?  

This is fixed by this commit:
http://git.et.redhat.com/?p=libguestfs.git;a=commitdiff;h=672c4ba257924c3e64836c08fb3b55bc4a6b2aba

(In reply to comment #11)
> In src/guestfs.c, you probably want to use a read wrapper
> so you don't have to worry about EINTR and EAGAIN
> (the code should retry, not fail in those cases).
> It looks like there are a few others that would benefit.

This should fix it:
http://git.et.redhat.com/?p=libguestfs.git;a=commitdiff;h=bb349b05333aa5bf87a3882f15458d8f7341d807

(In reply to comment #12)
> Please use an unsigned type for length-only variables like "len" here:

This change isn't exhaustive by any means, but it fixes some
of these problems:

http://git.et.redhat.com/?p=libguestfs.git;a=commitdiff;h=dd8b152da0e899104fec305159640d08d7d6cdd9

(In reply to comment #13)
> This realloc (from guestfsd.c) leaks upon failure:

http://git.et.redhat.com/?p=libguestfs.git;a=commitdiff;h=427b5f079fd344919ecf568bab2084825aacf606

(In reply to comment #15)
> Since "make check" doesn't pass for me, I haven't yet tried running things
> under valgrind.  Have you?

No I haven't run it under valgrind, but obviously I should!

I've not seen the 'make check' error that we discussed on IRC, and
as you know I've tried to reproduce your setup quite closely.  I've
now built libguestfs on RHEL 5 too, which has quite a few differences
from Fedora 11.  Again, not seen that problem in 'make check' ...
Comment 18 Richard W.M. Jones 2009-05-07 16:55:07 EDT
New upstream version:

Spec URL: http://www.annexia.org/tmp/libguestfs.spec
SRPM URL: http://www.annexia.org/tmp/libguestfs-1.0.20-1.fc11.src.rpm

Koji scratch build of the above:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1341246
Comment 19 Richard W.M. Jones 2009-05-07 18:03:13 EDT
This Koji scratch build enables the tests:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1341318
(still building as I write ...)
Comment 20 Jim Meyering 2009-05-08 04:29:19 EDT
Hi Rich,

Those changes all look fine.  Thanks.
I've switched "fedora-review" to "+".

BTW, there are a couple of nits in
http://koji.fedoraproject.org/koji/getfile?taskID=1341247&name=build.log:

Makefile:1097: warning: overriding commands for target `make-initramfs.sh'
Makefile:400: warning: ignoring old commands for target `make-initramfs.sh'
...
mv initramfs.fedora-12.x86_64.img initramfs.fedora-12.x86_64.img.bak
...
mv: cannot stat `initramfs.fedora-12.x86_64.img': No such file or directory
make[2]: [initramfs/fakeroot.log] Error 1 (ignored)
mv vmlinuz.fedora-12.x86_64 vmlinuz.fedora-12.x86_64.bak
mv: cannot stat `vmlinuz.fedora-12.x86_64': No such file or directory
...
Comment 21 Richard W.M. Jones 2009-05-08 04:33:30 EDT
New Package CVS Request
=======================
Package Name: libguestfs
Short Description: Access and modify virtual machine disk images
Owners: rjones
Branches: F-10 F-11 EL-5
InitialCC:
Comment 22 Jim Meyering 2009-05-08 04:52:04 EDT
I've just fedora-review to + again. third time, now, after two mid-air-collisions.
Comment 23 Kevin Fenzi 2009-05-09 16:51:52 EDT
cvs done.
Comment 24 Richard W.M. Jones 2009-05-09 18:48:49 EDT
Thank you everyone.

Now built in F-10, F-11, F-12.

Still waiting on EL-5.
Comment 25 Richard W.M. Jones 2009-05-11 14:16:50 EDT
Now built in EPEL:
http://buildsys.fedoraproject.org/logs/fedora-5-epel/2268-libguestfs-1.0.23-8.el5/
Comment 26 Richard W.M. Jones 2009-05-15 04:35:44 EDT
Package Change Request
======================
Package Name: libguestfs
REMOVE Branch: F-10

Sorry about this - I spent some time trying to backport
the necessary enhancement to qemu into the version of qemu
in F-10, but it's a lot more complicated than I expected.
So I'd like to remove the F-10 branch from this package.
Comment 27 Kevin Fenzi 2009-05-15 19:50:45 EDT
Just follow: 
http://fedoraproject.org/wiki/How_to_remove_a_package_at_end_of_life
for the f10 branch.