Bug 517488

MUST items:
*** rpmlint reports errors shown above [1]
Package is named according to the package naming guidelines
spec file name matches the base package name
Package meets the packaging guidelines
Package is licensed with a Fedora approved license
License field in spec file matches actual license
Spec file is written in American English
Spec file is legible
*** Upstream sources are not available [2]
Package successfully builds on i686 and x86_64
*** Package uses ExclusiveArch [3]
All build dependencies are listed in BuildRequires
Upstream does not contain localisation
ldconfig is called in %post and %postun for vm-dump-metrics
Package does not bundle copies of system libraries
Package is not relocatable
Package owns all directories that it creates
No files are listed more than once in %files
Permissions on files are set properly
Package has a %clean section containing rm -rf $RPM_BUILD_ROOT
Package uses macros consistently
Package contains only code
Package does not include 'large documentation files'
%doc files are not required for correct function
Header files are in -devel package
Static libraries are not built
Package does not include pkgconfig files (but autoconf still uses pkgconfig)
Bare .so file is in -devel pacakge
-devel subpackage requires parent subpackage
Package does not contain .la libtool archives
Package does not contain a gui application
Package does not own files or directories owned by other packages
%install runs rm -rf $RPM_BUILD_ROOT
All filenames are valid UTF-8

SHOULD items:
Source package includes license text
*** Description and summary sections do not contain translations (not blocker)
Package builds in mock (see koji build in original submission)
Package compiles and builds for supported architectures (but see [3])
*** Package caused system to become unstable requiring reboot [4]
Scriptlets are sane
Subpackage does not need to require base package in this case
Package does not contain pkgconfig files
Package does not have file dependencies

Misc:
Did you consider any of the other approaches to removing the use of rpath in
the spec file?

Notes:
[1] If the 2 DTDs are genuinely not configuration files, they should be
removed. I note that they're already in %doc, which seems appropriate.

[2] It's not clear how the tarball was obtained or can be reproduced exactly
from source. I'm not able to verify this.

[3] I'm taking ExclusiveArch here to be semantically equivalent to
ExcludeArch. I note that xen (which is a dependency), also has an
ExclusiveArch. libvirt has a solution to this which involves not building
against xen where it is not available. Is this possible in this package?

[4] Starting vhostmd on my F-11 host flooded /var/log/messages with:
Oct  7 14:51:06 mbooth libvirtd: 14:51:06.616: error :
qemudDomainLookupByID:3028 : Domain not found: no domain with matching id 0
Oct  7 14:51:06 mbooth libvirtd: 14:51:06.619: error :
qemudDomainLookupByName:3080 : Domain not found: no domain with matching name
'0'
Oct  7 14:53:51 mbooth vhostmd: Error retrieving metric UsedMem
Oct  7 14:53:53 mbooth vhostmd: Failed to collect vm metrics during update

and the following 2 AVC messages:
Oct  7 14:42:33 mbooth kernel: type=1400 audit(1254922953.004:17101): avc:
denied  { read write } for  pid=32590 comm="virsh"
path="/dev/shm/vhostmd/disk0"
dev=tmpfs ino=2946820 scontext=unconfined_u:system_r:xm_t:s0
tcontext=unconfined_u:object_r:tmpfs_t:s0 tclass=file
Oct  7 14:42:33 mbooth kernel: type=1400 audit(1254922953.012:17102): avc:
denied  { getsched } for  pid=32590 comm="virsh"
scontext=unconfined_u:system_r:xm_t:s0 tcontext=unconfined_u:system_r:xm_t:s0
tclass=process

The system became sluggish after only a few seconds, and unresponsive
requiring a reboot shortly after that. This may be a problem with SELinux
policy or the default configuration file. Either way, I'd be uncomfortable
shipping the package with this as the default behaviour. Is this still a
problem on rawhide? I didn't test there.

-----

Rich,

Can you respond to Misc and 4 Notes above first?

Matt

rpaths: No, I just used chrpath.  Fedora recommends passing
"--disable-rpath" to configure, but AFAIK that is not a standard
configure option, and in any case I checked and it's definitely
not supported by vhostmd's upstream configure.ac.

Note [4]:

The out of memory issue was a memory leak in libvirtd (bug 528162).

"Domain not found: no domain with matching id 0" is a warning with
the default configuration file supplied by upstream.  I'll supply
an alternative configuration which should work better with KVM.

"Error retrieving metric UsedMem" - idem.

AVCs: We need an selinux policy update to cope with this, but we
can't do that until the package is in Fedora.

rpaths: OK.

Note 4: I'll take your word that this is fixed :) There must be a way to engage the SELinux folks early on this, but I don't see that this should be a blocker.

Any response to notes 1-3?

Thanks for looking at this.

[1] DTDs: These aren't configuration files, but vhostmd.dtd at
least is required and has to stay at that location because it is
used to validate the XML configuration file.  Stupid stupid stupid.

[2] Tarball is now packaged upstream, but the URL is still
unusable.  This is because gitorious doesn't really know how
to host tarballs properly.  Upstream is looking for alternate
hosting.

[3] Change made as suggested.

Spec URL: http://www.annexia.org/tmp/vhostmd.spec
SRPM URL: http://www.annexia.org/tmp/vhostmd-0.3-3.fc11.src.rpm

* Mon Oct 12 2009 Richard W.M. Jones <rjones> - 0.3-3
- Remove metric.dtd file from /etc (fixes rpmlint warning), but
  vhostmd.dtd has to remain because it is needed to validate the
  XML configuration file.
- Remove ExclusiveArch, instead conditionally depend on xen-devel.
- Use a better, less noisy, more minimal configuration file which
  doesn't depend on Xen.

* Thu Oct  8 2009 Richard W.M. Jones <rjones> - 0.3-1
- New upstream version 0.3.

[1] DTD: OK.

[2] Tarball: I appreciate the upstream hosting issue. It still looks odd, though. The srpm contains vhostmd-vhostmd-0.3.tar.gz (note the repeated vhostmd-). make dist on the source creates the expected vhostmd-0.3.tar.gz. Is this intentional?

Also, there are no tags in the git repo, but make dist on what appears to be the release commit creates a tarball with a different md5, at least on my box.

[3] Unfortunately this now fails to build on ppc:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1742110

You may need some additional configure magic when xen-devel isn't available.

(In reply to comment #5)
> [1] DTD: OK.
> 
> [2] Tarball: I appreciate the upstream hosting issue. It still looks odd,
> though. The srpm contains vhostmd-vhostmd-0.3.tar.gz (note the repeated
> vhostmd-). make dist on the source creates the expected vhostmd-0.3.tar.gz. Is
> this intentional?

Yeah, this is all fscked up.  The upstream issue is being
discussed here:

http://lists.opensuse.org/vhostmd/2009-10/msg00007.html

> Also, there are no tags in the git repo, but make dist on what appears to be
> the release commit creates a tarball with a different md5, at least on my box.

Ditto above.  It generates a new tarball each time :-(

> [3] Unfortunately this now fails to build on ppc:
> 
> http://koji.fedoraproject.org/koji/taskinfo?taskID=1742110
> 
> You may need some additional configure magic when xen-devel isn't available.  

OK I get this now.  It's actually a feature of vhostmd that
I wasn't aware of up to now.  As well as using the metrics
disk to export data, it can also export it through xenstore.

Unfortunately the current code doesn't allow that to be
configured out at compile time (and thus always requires
the Xenstore client library to be available).

I think the best thing here is to add back the
ExclusiveArch temporarily until I can get it fixed
upstream.

(In reply to comment #6)
> > [2] Tarball: I appreciate the upstream hosting issue. It still looks odd,
> > though. The srpm contains vhostmd-vhostmd-0.3.tar.gz (note the repeated
> > vhostmd-). make dist on the source creates the expected vhostmd-0.3.tar.gz. Is
> > this intentional?
> 
> Yeah, this is all fscked up.  The upstream issue is being
> discussed here:
> 
> http://lists.opensuse.org/vhostmd/2009-10/msg00007.html

Ok, it doesn't sound like the gitorious solution will fly. Can you do a 'make dist' on the source yourself and add a comment to the spec identifying the commit the dist is based on (presumably a92f35045ab8c7b88c714a44623219ffac3caea1 in this case). This should at least be verifiable and reproducible. Of course, this would be easier if upstream would add a tag to this commit, then you could just document the name of the tag.

> > [3] Unfortunately this now fails to build on ppc:
> > 
> > http://koji.fedoraproject.org/koji/taskinfo?taskID=1742110
> > 
> > You may need some additional configure magic when xen-devel isn't available.  
> 
> OK I get this now.  It's actually a feature of vhostmd that
> I wasn't aware of up to now.  As well as using the metrics
> disk to export data, it can also export it through xenstore.
> 
> Unfortunately the current code doesn't allow that to be
> configured out at compile time (and thus always requires
> the Xenstore client library to be available).
> 
> I think the best thing here is to add back the
> ExclusiveArch temporarily until I can get it fixed
> upstream.  

Agreed.

So what I've done is to move to a pre-release of 0.4,
which also includes the changes to remove the dependency
on xenstore on non-Xen platforms.

Spec URL: http://www.annexia.org/tmp/vhostmd.spec
SRPM URL: http://www.annexia.org/tmp/vhostmd-0.4-0.1.git326f0012172.fc11.src.rpm

* Tue Oct 13 2009 Richard W.M. Jones <rjones> - 0.4-0.1.git326f0012172
- Move to pre-release of 0.4, self-built tarball.
- Disable xenstore on non-x86 platforms.
- Add patch to fix --without-xenstore option.

Koji scratch build here:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1743279

Created attachment 364576 [details]
%define have_xen to abstract architecture ugliness

Not a blocker. Would you consider the attached patch to the spec file which I think reads slightly better? It puts the architecture stuff at the top with a brief comment about why it's there.

The only remaining issue is the lack of a released tarball. However, this is an upstream issue and I'm happy that the package does the best currently possible.

ACK.

Yes, I'll add that patch, thanks.

(Of course using %global, not %define !)

New Package CVS Request
=======================
Package Name: vhostmd
Short Description: Virtualization host metrics daemon
Owners: 
Branches: F-12 EL-5
InitialCC:

I assume you meant rjones as owner? :) 

cvs done.

vhostmd-0.4-0.1.git326f0012172.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/vhostmd-0.4-0.1.git326f0012172.fc12

Why are we letting in packages that break when SELinux is enforcing mode?  The apps should be fixed or work out with the policy maintainers the fixes before the package gets accepted.  Any package that forces the user of Fedora to be put in to permissive/disabled mode should not be accepted.

Dan,

I agree entirely (and made that point!). However, fixing it does seem to be a chicken/egg problem. What's the correct process here?

Sorry, someone just requested that this is built for F11 too :-(

Package Change Request
======================
Package Name: vhostmd
New Branches: F-11
Owners:

Matthew, I think you did the right thing in contacting me.  I have asked Miroslav to begin writing policy for vhostmd.  And I will put it in F12 and F11 when it is available.

I will talk to the Fedora team about making this a more formal part of the review.

cvs done.

I noticed this bug should be closed, since vhostmd
has been added to Fedora.