Bug 220185 - Review Request: kvm - Kernel Based Virtual Machine
Review Request: kvm - Kernel Based Virtual Machine
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: David Cantrell
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-12-19 11:11 EST by Jeremy Katz
Modified: 2013-01-09 20:34 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-01-05 14:09:24 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Jeremy Katz 2006-12-19 11:11:14 EST
Spec URL: http://people.redhat.com/~katzj/kvm/kvm.spec
SRPM URL: http://people.redhat.com/~katzj/kvm/kvm-7-1.src.rpm
Description: This is the userspace side of the kvm virtualization that is being included in 2.6.20.

Since it's largely qemu + some patches, I've packaged it so that it a) coexists with the existing qemu package and b) requires the qemu package for some of the stuff they share (keymaps, etc).  

Currently, the -devel package only has a static lib as that's all that is included upstream as it's very new and so there's no semblance of an ABI but for people wanting to start to try to play, it's somewhat important to have.  Alternately, we just drop the devel subpackage altogether.
Comment 1 Parag AN(पराग) 2006-12-19 12:07:53 EST
I love packages with kernel word.

Anyway i tried to build this package in mock build environment but build failed with
make[1]: Entering directory `/builddir/build/BUILD/kvm-7/qemu'
gcc34 -DQEMU_TOOL -I /builddir/build/BUILD/kvm-7/qemu/../user -Wall -O2 -g
-fno-strict-aliasing -I. -g -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64
-D_LARGEFILE_SOURCE -o qemu-img qemu-img.c block.c block-cow.c block-qcow.c
aes.c block-vmdk.c block-cloop.c block-dmg.c block-bochs.c block-vpc.c
block-vvfat.c -lz -luuid
block-vmdk.c:25:23: uuid/uuid.h: No such file or directory
block-vmdk.c: In function `vmdk_snapshot_create':
block-vmdk.c:194: error: `uuid_t' undeclared (first use in this function)
block-vmdk.c:194: error: (Each undeclared identifier is reported only once
block-vmdk.c:194: error: for each function it appears in.)
block-vmdk.c:194: error: syntax error before "name"
block-vmdk.c:211: warning: implicit declaration of function `uuid_generate'
block-vmdk.c:211: error: `name' undeclared (first use in this function)
block-vmdk.c:212: warning: implicit declaration of function `uuid_unparse'
block-vmdk.c: In function `vmdk_open':
block-vmdk.c:420: warning: passing arg 1 of `dirname' discards qualifiers from
pointer target type
make[1]: *** [qemu-img] Error 1
make[1]: Leaving directory `/builddir/build/BUILD/kvm-7/qemu'
make: *** [qemu] Error 2
error: Bad exit status from /var/tmp/rpm-tmp.21186 (%build)

Looks to me you need to add e2fsprogs-devel as BuildRequires.
Comment 2 Jeremy Katz 2006-12-19 13:25:50 EST
Indeed -- that'll teach me to trust based on inspection for deps.  Build
actually tested in mock now (after setting up my new workstation with a sane
config) and it builds fine on x86_64 in mock with that buildreq.  Updated spec
file in the same location, and
http://people.redhat.com/~katzj/kvm/kvm-7-2.src.rpm is there
Comment 3 Parag AN(पराग) 2006-12-20 02:12:22 EST
I will review this.
mock build for i386 successfull but rpmlint is not silent

You SHOULD:
 - Add disttag {?dist} to release tag
 - Add direct download link of any mirror of sf.net
  say http://jaist.dl.sourceforge.net/sourceforge/kvm/kvm-7.tar.gz
 - Add COPYRIGHT file under %doc 
 - You may add test directory under %doc

Kindly check against rpmlint command both RPM and SRPM and resubmit package.
Comment 4 Thorsten Leemhuis 2006-12-20 02:41:11 EST
(In reply to comment #3)
>  - Add disttag {?dist} to release tag

Disttag ist still optional

>  - Add direct download link of any mirror of sf.net
>   say http://jaist.dl.sourceforge.net/sourceforge/kvm/kvm-7.tar.gz

Using download.sourceforge.net is prefered:
http://download.sourceforge.net/sourceforge/kvm/kvm-7.tar.gz 
That will redirect to a mirror.
Comment 5 Parag AN(पराग) 2006-12-20 05:07:07 EST
(In reply to comment #4)
> (In reply to comment #3)
> >  - Add disttag {?dist} to release tag
> 
> Disttag ist still optional
   Ohh sorry, i thought asking reporter for adding disttag is essential. Will
take care from next time for not to ask anyone to add disttag.
> 
> >  - Add direct download link of any mirror of sf.net
> >   say http://jaist.dl.sourceforge.net/sourceforge/kvm/kvm-7.tar.gz
> 
> Using download.sourceforge.net is prefered:
> http://download.sourceforge.net/sourceforge/kvm/kvm-7.tar.gz 
> That will redirect to a mirror.
  Ohh sorry, actually i was asked same for my own v4l2-tool package review, so
similarly i asked here.


Comment 6 Parag AN(पराग) 2006-12-20 05:33:04 EST
Information given on kvm.sf.net said
"KVM (for Kernel-based Virtual Machine) is a full virtualization solution for
Linux on x86 hardware. It consists of a loadable kernel module (kvm.ko) and a
userspace component."
IMHO, Either you need to submit KMOD package for kvm.ko for kernel versions
lesser than 2.6.20 if this module supports existing FC6/devel kernels or  this
package needs dependency to be set for kernel's > 2.6.20

Kindly ignore my comment for disttag and Source URL and use 
http://download.sourceforge.net/sourceforge/kvm/kvm-7.tar.gz as Source URL


Also static libraries need FESCO approval. So I request you to ask FESCO for
permission to include .a files ot drop -devel package.


Comment 7 Jeremy Katz 2006-12-20 10:36:38 EST
(In reply to comment #3)
> mock build for i386 successfull but rpmlint is not silent

rpmlint errors are bogus --
E: kvm configure-without-libdir-spec
   The configure script isn't a standard autoconf configure and so doesn't take
a libdir argument
W: kvm no-documentation
   Upstream doesn't really include any documentation in the tarball 
 
> You SHOULD:
>  - Add disttag {?dist} to release tag

disttag isn't required.

>  - Add direct download link of any mirror of sf.net
>   say http://jaist.dl.sourceforge.net/sourceforge/kvm/kvm-7.tar.gz

Fixed, but with the link from the other comment

>  - Add COPYRIGHT file under %doc 

There's not an explicit COPYRIGHT for the top-level and adding more copies of
the GPL to the distro doesn't help anyone.

>  - You may add test directory under %doc

This doesn't make any sense at all; tests aren't documentation.

(In reply to comment #6)
> IMHO, Either you need to submit KMOD package for kvm.ko for kernel versions
> lesser than 2.6.20 if this module supports existing FC6/devel kernels or  this
> package needs dependency to be set for kernel's > 2.6.20

The package is only going to be submitted for devel (where rawhide has
2.6.20-rc).  And a requires on a "new" kernel isn't the answer as a user can
trivially build the module if they really don't want to bump versions.  Also,
the requires doesn't mean that the user is actually running said kernel.

The more right answer is that as tools get built up, they need to check and make
sure the kernel provides the functionality, much like tools for xen need to
ensure that they're running on a Xen dom0.

> Also static libraries need FESCO approval. So I request you to ask FESCO for
> permission to include .a files ot drop -devel package.

I don't think that's actually been fully passed yet, but I don't care enough to
keep the library around for now.  Nuking -devel.


http://people.redhat.com/~katzj/kvm/kvm-7-3.src.rpm and
http://people.redhat.com/~katzj/kvm/kvm.spec updated
Comment 8 Avi Kivity 2006-12-26 01:03:18 EST
Suggest calling the binary 'kvm' instead of qemu-kvm, and adding a softlink to 
the qemu manual page.

Alternatively, merge it to the qemu package.  That's perhaps a little 
premature at this moment.
Comment 9 Parag AN(पराग) 2006-12-28 05:03:20 EST
Mock build failed.
Comment 10 Jeremy Katz 2007-01-03 13:31:42 EST
(In reply to comment #8)
> Suggest calling the binary 'kvm' instead of qemu-kvm, and adding a softlink to 
> the qemu manual page.

Not calling the binary kvm due to potential confusion with the little kvm
wrapper script in the tarball; although I'm somewhat interested in getting the
script a little bit more suitable so that I can just include it.  But I'll do
that via kvm-devel
 
> Alternatively, merge it to the qemu package.  That's perhaps a little 
> premature at this moment.

Yeah, seems a little early still.  Once everything is upstream, I'll be more
than glad to have qemu obsolete this (although if there are some useful tools
around using qemu+kvm, then maybe this just becomes the tools package :-)
Comment 11 Jeremy Katz 2007-01-03 13:33:13 EST
(In reply to comment #9)
> Mock build failed.

Apparently, I can't type/save files when I'm actually done with them...

http://people.redhat.com/~katzj/kvm/kvm-7-4.src.rpm and
http://people.redhat.com/~katzj/kvm/kvm.spec updated.

For real this time!
Comment 12 Jesse Keating 2007-01-05 13:18:37 EST
This now builds and looks good, I would approve it.
Comment 13 Jesse Keating 2007-01-05 13:42:52 EST
Ok, approving, sorry Parag but we want this in soon.
Comment 14 Jeremy Katz 2007-01-05 14:09:24 EST
Thanks Parag and Jesse.

Built for extras-development
Comment 15 Parag AN(पराग) 2007-01-06 00:06:03 EST
Jesse and Jeremy,
 Sorry i could not get time to approve this package.
I don't mind Jesse that you approved this package :)

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