Re: [libvirt] PATCH: Improve python exception messages

2008-08-22 Thread Richard W.M. Jones
On Thu, Aug 21, 2008 at 12:00:46PM +0100, Daniel P. Berrange wrote:
 This patch was flawed - it missed out the error message info when passed
 in a domain/network/volume/pool instead of a connection object. Here is
 an updated patch which addresses that

+1.

I added the original '.. failed' messages.  Before that there was
no exception on failure at all :-)

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] PATCH: Allow specific FDs to be kept open in virExec()

2008-08-22 Thread Richard W.M. Jones
On Thu, Aug 21, 2008 at 02:58:30PM +0100, Daniel P. Berrange wrote:
 +#define VIR_EXEC_FDSET_SIZE() (sysconf(_SC_OPEN_MAX)/8)
 +#define VIR_EXEC_FDSET_CLEAR(set) memset((set), 0, VIR_EXEC_FDSET_SIZE())
 +#define VIR_EXEC_FDSET_ON(set, fd) (set[(fd)/8] |= (1  ((fd)%8)))
 +#define VIR_EXEC_FDSET_OFF(set, fd) (set[(fd)/8] = ~(1  ((fd)%8)))
 +#define VIR_EXEC_FDSET_ISON(set, fd) (set[(fd)/8]  (1  ((fd)%8)))

+1 although I'm interested to know why you didn't just use ordinary
FD_SETs for this.

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://et.redhat.com/~rjones/virt-df/

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] autobuild.sh: Fix minor shell-quoting bugs.

2008-08-22 Thread Richard W.M. Jones
On Thu, Aug 21, 2008 at 05:57:37PM +0200, Jim Meyering wrote:
 I noticed some minor quoting problems in ovirt's autobuild.sh,
 and since part of that code came from here, ...

+1

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] PATCH: Allow specific FDs to be kept open in virExec()

2008-08-22 Thread Daniel P. Berrange
On Fri, Aug 22, 2008 at 09:22:06AM +0100, Richard W.M. Jones wrote:
 On Thu, Aug 21, 2008 at 02:58:30PM +0100, Daniel P. Berrange wrote:
  +#define VIR_EXEC_FDSET_SIZE() (sysconf(_SC_OPEN_MAX)/8)
  +#define VIR_EXEC_FDSET_CLEAR(set) memset((set), 0, VIR_EXEC_FDSET_SIZE())
  +#define VIR_EXEC_FDSET_ON(set, fd) (set[(fd)/8] |= (1  ((fd)%8)))
  +#define VIR_EXEC_FDSET_OFF(set, fd) (set[(fd)/8] = ~(1  ((fd)%8)))
  +#define VIR_EXEC_FDSET_ISON(set, fd) (set[(fd)/8]  (1  ((fd)%8)))
 
 +1 although I'm interested to know why you didn't just use ordinary
 FD_SETs for this.

For some reason I have it in my head that fd_set was limited to 32
but looking at the Linux source its clearly 1024, which is the sme
as my _SC_OPEN_MAX.   The manpage says its FD_SETSIZE, but doesn't
clarify whether FD_SETSIZE is guarenteed to be large enough to store
any FD upto _SC_OPEN_MAX.  

Jim, any thoughts on this ?  I'll happily switch to fd_set if we expect
it'll be suitable

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] Check disk target collision when parsing domain xml

2008-08-22 Thread Richard W.M. Jones
On Thu, Aug 21, 2008 at 11:20:13PM -0400, Cole Robinson wrote:
 The current domain xml parsing doesn't check if disks
 are specified with duplicate targets (hda etc.). The
 attached patch adds a check for this.

+1.

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
Read my OCaml programming blog: http://camltastic.blogspot.com/
Fedora now supports 60 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] Add success output to virsh attach/detach commands

2008-08-22 Thread Daniel P. Berrange
On Fri, Aug 22, 2008 at 09:26:13AM +0100, Richard W.M. Jones wrote:
 On Thu, Aug 21, 2008 at 11:19:29PM -0400, Cole Robinson wrote:
  The virsh attach-* and detach-* commands don't say
  anything if the operation succeeds, which doesn't
  seem very polite. The attached adds some simple
  feedback.
 
 Hmmm ...  The patch is fine, but I don't really like commands which
 are verbose on anything other than errors.

Nor do I, but all our other commands print a confirmation message
too, so for sake of consistency I think Cole's patch is reasonable.
We do have a --quiet arg to make it STFU

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] Set correct device type when parsing input devices

2008-08-22 Thread Daniel P. Berrange
On Thu, Aug 21, 2008 at 11:19:47PM -0400, Cole Robinson wrote:
 In virDomainDeviceDefParse, parsing an input device
 was actually setting it's type as DEVICE_DISK. The
 attached patch fixes this.

ACK

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] Check disk target collision when parsing domain xml

2008-08-22 Thread Daniel P. Berrange
On Thu, Aug 21, 2008 at 11:20:13PM -0400, Cole Robinson wrote:
 The current domain xml parsing doesn't check if disks
 are specified with duplicate targets (hda etc.). The
 attached patch adds a check for this.
 
 Thanks,
 Cole
 

 commit 27df1653474738a2ce83c89e7bdb2c4f7327f9b6
 Author: Cole Robinson [EMAIL PROTECTED]
 Date:   Thu Aug 21 14:58:04 2008 -0400
 
 Check for duplicate disk targets when parsing domain xml.
 
 diff --git a/src/domain_conf.c b/src/domain_conf.c
 index 6b23474..ed6cc8b 100644
 --- a/src/domain_conf.c
 +++ b/src/domain_conf.c
 @@ -1955,6 +1955,12 @@ static virDomainDefPtr 
 virDomainDefParseXML(virConnectPtr conn,
  } else {
  virDomainDiskDefPtr ptr = def-disks;
  while (ptr) {
 +if (ptr-next  STREQ(disk-dst, ptr-next-dst)) {

I'm confused as to why you're comparing for dups against ptr-next-dst
instead of ptr-dst. It would seem to miss a duplication disk target
check for the very first device..

 +virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR,
 + _(duplicate disk target '%s'),
 + disk-dst);
 +goto error;
 +}
  if (!ptr-next || virDomainDiskCompare(disk, ptr-next)  0) 
 {
  disk-next = ptr-next;
  ptr-next = disk;

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] Fix cd eject segfault

2008-08-22 Thread Richard W.M. Jones
On Thu, Aug 21, 2008 at 11:20:28PM -0400, Cole Robinson wrote:
 The cdrom eject code was trying to dereference the
 NULL source of an empty cdrom. Attached patch fixes
 this.

Good catch, +1

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


[libvirt] [PATCH] enable parallel builds

2008-08-22 Thread James Morris
I'm not sure if this is the best way to do this, but it seems to work.



Enable parallel compilation of the repository when running the autobuild 
script and/or via rpmbuild.

---

 autobuild.sh|   15 ++-
 libvirt.spec.in |2 +-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/autobuild.sh b/autobuild.sh
index 7ae5d1e..1a5413b 100755
--- a/autobuild.sh
+++ b/autobuild.sh
@@ -21,7 +21,20 @@ rm -rf coverage
   --with-lxc \
   --with-xen-proxy
 
-make
+# from the gdc spec file
+# if RPM_BUILD_NCPUS unset, set it
+if [ -z $RPM_BUILD_NCPUS ] ; then
+  if [ -x /usr/bin/getconf ] ; then
+RPM_BUILD_NCPUS=$(/usr/bin/getconf _NPROCESSORS_ONLN)
+if [ $RPM_BUILD_NCPUS -eq 0 ]; then
+  RPM_BUILD_NCPUS=1
+fi
+  else
+RPM_BUILD_NCPUS=1
+  fi
+fi
+
+make -j$RPM_BUILD_NCPUS
 make install
 
 set -o pipefail
diff --git a/libvirt.spec.in b/libvirt.spec.in
index d37c0e0..cd8e937 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -169,7 +169,7 @@ of recent versions of Linux (and other OSes).
--with-init-script=redhat \
--with-qemud-pid-file=%{_localstatedir}/run/libvirt_qemud.pid \
--with-remote-file=%{_localstatedir}/run/libvirtd.pid
-make
+make %{?_smp_mflags}
 
 %install
 rm -fr %{buildroot}

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] Fix cd eject segfault

2008-08-22 Thread Daniel P. Berrange
On Thu, Aug 21, 2008 at 11:20:28PM -0400, Cole Robinson wrote:
 The cdrom eject code was trying to dereference the
 NULL source of an empty cdrom. Attached patch fixes
 this.
 @@ -2972,6 +2973,13 @@ static int qemudDomainChangeCDROM(virDomainPtr dom,
  }
  VIR_FREE(safe_path);
  
 +newsrc = strdup(newdisk-src);
 +if (!newsrc) {
 +qemudReportError(dom-conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
 + %s, _(out of memory));
 +return -1;
 +}

Rather than dup'ing the string here and having to deal with OOM...

 +
  } else if (asprintf(cmd, eject cdrom) == -1) {
  qemudReportError(dom-conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
   %s, _(out of memory));
 @@ -2982,11 +2990,17 @@ static int qemudDomainChangeCDROM(virDomainPtr dom,
  qemudReportError(dom-conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
   %s, _(cannot change cdrom media));
  VIR_FREE(cmd);
 +VIR_FREE(newsrc);
  return -1;
  }
  VIR_FREE(reply);
  VIR_FREE(cmd);
 -strcpy(olddisk-src, newdisk-src);
 +
 +VIR_FREE(olddisk-src);
 +if (newsrc) {
 +olddisk-src = newsrc;
 +newsrc = NULL;
 +}

Just do

  VIR_FREE(olddisk-src);
  olddisk-src = newdisk-src;
  newdisk-src = NULL;

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] Fix disk device ordering

2008-08-22 Thread Richard W.M. Jones
On Thu, Aug 21, 2008 at 11:21:15PM -0400, Cole Robinson wrote:
 When parsing the domain xml, disks are supposed to be
 rearranged in an alphabetic order based on bus type
 and target. The reordering code had a flaw though, in
 that it would always put the first disk it encountered
 at the head of the list, and had no way of putting
 new entries in front of it.
 
 The attached patch reworks the code to compare the new
 disk against the current disk entry (rather than the
 next entry like the existing code). Seems to pass all
 my tests.

Urgh, the perils of writing everything in C.

+1, this code looks correct to me.

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
Read my OCaml programming blog: http://camltastic.blogspot.com/
Fedora now supports 60 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] Fix disk device ordering

2008-08-22 Thread Daniel P. Berrange
On Thu, Aug 21, 2008 at 11:21:15PM -0400, Cole Robinson wrote:
 When parsing the domain xml, disks are supposed to be
 rearranged in an alphabetic order based on bus type
 and target. The reordering code had a flaw though, in
 that it would always put the first disk it encountered
 at the head of the list, and had no way of putting
 new entries in front of it.

Hmm, yes good catch.

 
 The attached patch reworks the code to compare the new
 disk against the current disk entry (rather than the
 next entry like the existing code). Seems to pass all
 my tests.

If the test suite passes that's good enough for me - we have a
test case which explicitly does lots of disks with different
buses  names for this.


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] Update domain xml after usb hotplug

2008-08-22 Thread Daniel P. Berrange
On Thu, Aug 21, 2008 at 11:21:33PM -0400, Cole Robinson wrote:
 The recently added usb hostdev and mass storage device
 hotplug code doesn't append the devices to the running
 guests xml if the hotplug succeeds. The attached patch
 fixes this.

Yes, completely forgot about this when reviewing it before. ACK

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] enable parallel builds

2008-08-22 Thread Daniel P. Berrange
On Fri, Aug 22, 2008 at 06:36:23PM +1000, James Morris wrote:
 I'm not sure if this is the best way to do this, but it seems to work.

Sure, gets my vote. 

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] anyone implementing host device enumeration?

2008-08-22 Thread Daniel P. Berrange
On Thu, Aug 21, 2008 at 03:18:57PM -0400, David Lively wrote:
 Hi -
 
   I'm about to start working on host device enumeration, along the
 (HAL-ish) lines of what was discussed back in April:
   https://www.redhat.com/archives/libvir-list/2008-April/msg5.html
 
   I know the xml details haven't been fully fleshed out, but there seems
 to be agreement that it will be a fairly direct mapping from (a subset
 of the) HAL info to the details that we need in the xml.  Doubtless it
 will take a while to figure out exactly what subset suffices (and, for
 that matter, if everything needed is available via HAL ...), but I think
 the work is well-defined for some of the obvious details (discussed in
 the above thread) on which there's broad agreement.
 
   Is anyone working on such an implementation?

No one that I'm aware of - so go for it. FYI, one complication has come
up since my initial proposal. HAL is going away, to be replaced by
DeviceKit :-( That said they will co-exist and HAL will be around for
a while and they will both expose very similar properties for devices
so it shouldn't too much trouble. I think we just need to be conservative
when deciding how many of the HAL properties we need to add in libvirt.
I'd go for a pretty minimal set at first.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] storage pool discovery

2008-08-22 Thread Daniel P. Berrange
On Thu, Aug 21, 2008 at 10:29:43AM -0400, David Lively wrote:
 Hi Folks -
   Here's my second pass at storage pool discovery.  I've taken Daniel's
 suggestion and made it return a single XML doc containing source
 elements rather than an array of pool docs (and also incorporated
 suggestions from Daniel V and Jim M).
   Note that the storage source name patch is closely related
 (without it, the source docs returned for logical pools aren't
 correct).

Excellant, I think this patch looks more or less good to commit now.

 +static char *
 +virStorageBackendFileSystemNetDiscoverPools(virConnectPtr conn,
 +const char *srcSpec,
 +unsigned int flags 
 ATTRIBUTE_UNUSED)

...

 +const char *prog[] = { SHOWMOUNT, --no-headers, --exports, NULL, 
 NULL };
 +const char *start_tag = SourceList\n;
 +const char *end_tag = /SourceList\n;

I'd prefer that to be  sources  - we avoid capitals in the
XML element names everywhere else, and in the few cases of
joining words use an underscore, but I think plural form is
OK for this.

 +static char *
 +virStorageBackendLogicalDiscoverPools(virConnectPtr conn,
 +  const char *srcSpec ATTRIBUTE_UNUSED,
 +  unsigned int flags ATTRIBUTE_UNUSED)


 +const char *prog[] = { VGS, --noheadings, -o, vg_name, NULL };
 +const char *start_tag = SourceList\n;
 +const char *end_tag = /SourceList\n;

We should #define these two tags in storage_backend.h so each driver doesn't
need to dup them


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] introducing source name (for logical storage pools)

2008-08-22 Thread Daniel P. Berrange
On Thu, Aug 21, 2008 at 10:17:49AM -0400, David Lively wrote:
 Oops - that was against an old base.  Sorry.  Here's the new one.
 Also fixed a few other issues ...

Ok, this gets my vote to commit.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] Update domain xml after usb hotplug

2008-08-22 Thread Richard W.M. Jones
On Thu, Aug 21, 2008 at 11:21:33PM -0400, Cole Robinson wrote:
 The recently added usb hostdev and mass storage device
 hotplug code doesn't append the devices to the running
 guests xml if the hotplug succeeds. The attached patch
 fixes this.

IIRC there was some question about whether it should actually do this.
I think it should -- a user would expect that when you attach a device
to a domain, the device should appear in the XML (and be persistent
too).

So I ACK this patch.

This code pattern:

 +/* Find spot in domain definition where we will put the disk */
 +ptr = vm-def-disks;
 +prev = (vm-def-disks);
 +while (ptr) {
 

appears at least twice.  What we need is a Set abstract type.
Implementing it as a red-black self-balancing tree would avoid any
unexpected surprises when someone's guest has 100s of devices
attached.

Rich.

Ob-OCaml-hack.  The Set type in OCaml, implemented as an RB-tree,
comes with a formal proof of correctness which runs to 5000 lines and
took two man-months to complete.  Along the way they found bugs in the
original implementation.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] PATCH: Allow specific FDs to be kept open in virExec()

2008-08-22 Thread Daniel P. Berrange
On Fri, Aug 22, 2008 at 10:51:54AM +0200, Jim Meyering wrote:
 Daniel P. Berrange [EMAIL PROTECTED] wrote:
  On Fri, Aug 22, 2008 at 09:22:06AM +0100, Richard W.M. Jones wrote:
  On Thu, Aug 21, 2008 at 02:58:30PM +0100, Daniel P. Berrange wrote:
   +#define VIR_EXEC_FDSET_SIZE() (sysconf(_SC_OPEN_MAX)/8)
   +#define VIR_EXEC_FDSET_CLEAR(set) memset((set), 0, 
   VIR_EXEC_FDSET_SIZE())
   +#define VIR_EXEC_FDSET_ON(set, fd) (set[(fd)/8] |= (1  ((fd)%8)))
   +#define VIR_EXEC_FDSET_OFF(set, fd) (set[(fd)/8] = ~(1  ((fd)%8)))
   +#define VIR_EXEC_FDSET_ISON(set, fd) (set[(fd)/8]  (1  ((fd)%8)))
 
  +1 although I'm interested to know why you didn't just use ordinary
  FD_SETs for this.
 
  For some reason I have it in my head that fd_set was limited to 32
  but looking at the Linux source its clearly 1024, which is the sme
  as my _SC_OPEN_MAX.   The manpage says its FD_SETSIZE, but doesn't
  clarify whether FD_SETSIZE is guarenteed to be large enough to store
  any FD upto _SC_OPEN_MAX.
 
  Jim, any thoughts on this ?  I'll happily switch to fd_set if we expect
  it'll be suitable
 
 AFAIK, FD_SET must work for any valid file descriptor.
 
 The fd_set bit-manipulation function/macros like FD_SET, FD_CLR
 are specified in http://www.opengroup.org/susv3xsh/select.html

Ok, I'll redo this patch

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] configure.in: Prepend -lpthread to LIBS if required.

2008-08-22 Thread Daniel P. Berrange
On Thu, Aug 21, 2008 at 08:23:21PM +0200, Jim Meyering wrote:
 Jun Koi [EMAIL PROTECTED] wrote:
  The latest cvs version has an error when compiling:
 
  make[3]: Entering directory `/home/jun/projects/libvirt-0821/src'
  /bin/bash ../libtool --tag=CC   --mode=link gcc   -g -O2 -Wall
 ...
  ./.libs/libvirt.so: undefined reference to `pthread_sigmask'
  collect2: ld returned 1 exit status
  make[3]: *** [virsh] Error 1
 
 Thanks for the report.
 Next time, please tell us about your system,
 because I didn't 'see any such problem on a rawhide-based build.
 However, I did reproduce it on a Debian/unstable system.

I think what's going on here is some linker fun with libtool / pkgconfig

On Fedora it appears we end up automatically linked against pthread.so
as a result of libxml2 (or some othe lib) being linked against it. I
expect that Debian isn't doing this implicit linkage for us. In any
case we should rely on Fedora behaviour, so this patch looks reasonable
to me.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] enable parallel builds

2008-08-22 Thread Jim Meyering
James Morris [EMAIL PROTECTED] wrote:
 I'm not sure if this is the best way to do this, but it seems to work.

 

 Enable parallel compilation of the repository when running the autobuild
 script and/or via rpmbuild.

 ---

  autobuild.sh|   15 ++-
  libvirt.spec.in |2 +-
  2 files changed, 15 insertions(+), 2 deletions(-)

 diff --git a/autobuild.sh b/autobuild.sh
 index 7ae5d1e..1a5413b 100755
 --- a/autobuild.sh
 +++ b/autobuild.sh
 @@ -21,7 +21,20 @@ rm -rf coverage
--with-lxc \
--with-xen-proxy

 -make
 +# from the gdc spec file
 +# if RPM_BUILD_NCPUS unset, set it
 +if [ -z $RPM_BUILD_NCPUS ] ; then
 +  if [ -x /usr/bin/getconf ] ; then
 +RPM_BUILD_NCPUS=$(/usr/bin/getconf _NPROCESSORS_ONLN)
 +if [ $RPM_BUILD_NCPUS -eq 0 ]; then
 +  RPM_BUILD_NCPUS=1
 +fi
 +  else
 +RPM_BUILD_NCPUS=1
 +  fi
 +fi

Good idea.
I'll be happy to commit it with something like the following
in place of the above:

  # If the MAKEFLAGS envvar does not yet include a -j option,
  # add -jN where N depends on the number of processors.
  case $MAKEFLAGS in
*-j*) ;;
*) n=$(getconf _NPROCESSORS_ONLN 2 /dev/null)
  test $n -gt 0 || n=1
  n=`expr $n + 1`
  MAKEFLAGS=$MAKEFLAGS -j$n
  export MAKEFLAGS
  ;;
  esac

Then you don't have to change the make invocation below,
and it won't interfere if someone has already set MAKEFLAGS.
Also, not using an absolute path to getconf lets that program
work even also when it's installed in a different location.

Finally, I prefer to use N_CPUS+1 as the -j option, here.
(personally, I use 2*$N_CPUS+1, but that's probably too aggressive)

Ok?

 +make -j$RPM_BUILD_NCPUS
  make install

  set -o pipefail

Here's the patch I tested:

diff --git a/autobuild.sh b/autobuild.sh
index ce12692..cb6101f 100755
--- a/autobuild.sh
+++ b/autobuild.sh
@@ -21,6 +21,18 @@ rm -rf coverage
   --with-lxc \
   --with-xen-proxy

+# If the MAKEFLAGS envvar does not yet include a -j option,
+# add -jN where N depends on the number of processors.
+case $MAKEFLAGS in
+  *-j*) ;;
+  *) n=$(getconf _NPROCESSORS_ONLN 2 /dev/null)
+test $n -gt 0 || n=1
+n=`expr $n + 1`
+MAKEFLAGS=$MAKEFLAGS -j$n
+export MAKEFLAGS
+;;
+esac
+
 make
 make install

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 4aff4a5..7c151ef 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -157,7 +157,7 @@ of recent versions of Linux (and other OSes).
--with-init-script=redhat \
--with-qemud-pid-file=%{_localstatedir}/run/libvirt_qemud.pid \
--with-remote-file=%{_localstatedir}/run/libvirtd.pid
-make
+make %{?_smp_mflags}

 %install
 rm -fr %{buildroot}

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


RE: [libvirt] Using Xen config files

2008-08-22 Thread Matthew Donovan
 Xen and Xen-devel are 3.1.2.

I'm using version 0.4.4 of libvirt.

-matthew

 -Original Message-
 From: Richard W.M. Jones [mailto:[EMAIL PROTECTED] 
 Sent: Friday, August 22, 2008 4:19 AM
 To: Matthew Donovan
 Cc: Daniel P. Berrange; libvir-list@redhat.com
 Subject: Re: [libvirt] Using Xen config files
 
 On Thu, Aug 21, 2008 at 02:16:55PM -0400, Matthew Donovan wrote:
  I'm still having problems with this.  I've dug around and 
 found errors in
  the qemu-dm log.
  
  When I try to start the VM with libvirt, the error I'm getting is:
  
 xs_read(/vm/414e73de-cf1e-487c-87e0-d4ebf7a23576/rtc/timeoffs
 et): read
  error
  
  I did a xenstore-ls on the /vm/414e73de-cf1e-487c-87e0-d4ebf7a23576
  directory and sure enough, rtc/timeoffset is not there.  
 When I start the VM
  with the Xen config file I used to generate the XML the 
 rtc/timeoffset is
  there (timeoffset is set to 0).
 
 Which version of xen, xen-devel, etc?
 
 Rich.
 
 -- 
 Richard Jones, Emerging Technologies, Red Hat  
 http://et.redhat.com/~rjones
 virt-df lists disk usage of guests without needing to install any
 software inside the virtual machine.  Supports Linux and Windows.
 http://et.redhat.com/~rjones/virt-df/
 

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] Add success output to virsh attach/detach commands

2008-08-22 Thread Cole Robinson
Daniel P. Berrange wrote:
 On Fri, Aug 22, 2008 at 09:26:13AM +0100, Richard W.M. Jones wrote:
   
 On Thu, Aug 21, 2008 at 11:19:29PM -0400, Cole Robinson wrote:
 
 The virsh attach-* and detach-* commands don't say
 anything if the operation succeeds, which doesn't
 seem very polite. The attached adds some simple
 feedback.
   
 Hmmm ...  The patch is fine, but I don't really like commands which
 are verbose on anything other than errors.
 

 Nor do I, but all our other commands print a confirmation message
 too, so for sake of consistency I think Cole's patch is reasonable.
 We do have a --quiet arg to make it STFU
   
That was my reasoning as well. When commands like 'start'
and 'define' report success but attach-device doesn't, I
was left wondering if it even worked.

- Cole

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] PATCH: REmove bogus call to virStateInit in openvz driver

2008-08-22 Thread Daniel Veillard
On Tue, Aug 19, 2008 at 02:12:39PM +0100, Daniel P. Berrange wrote:
 For some unknown reason there is a call to virStateInitialize() in the 
 openvz driver's open method. This is absolutely forbidden - this API is
 only intended for use by the daemon. This patch removes this bogus call
 and makes it call the openvz setup APIs directly.

  Okay, I think I understand the problem, +1

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
[EMAIL PROTECTED]  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] PATCH: Improve python exception messages

2008-08-22 Thread Daniel Veillard
On Tue, Aug 19, 2008 at 10:31:34AM +0100, Daniel P. Berrange wrote:
 Most of the libvirt python API bindings use code snippet like this when
 raising an exception:
 
 if ret is None:raise libvirtError('virConnectOpen() failed')
 
 
 THis sets the message associated with the exception to
 
 virConnectOpen() failed
 
 This contains essentially zero useful information - you can see that it
 was virConnectOpen which failed from the stack trace.
 
 Now the libvirt error object has a real message, such as
 
authentication failed
 
 Or
 
unable to connect to '/var/run/libvirt/libvirt-sock': Connection refused
 
 This patch makes sure we extract this real error message and use it to 
 set the message associated with the exception object. This is one step
 in getting better error reporting for virt-install/virt-manager, which
 is particularly needed for remote connections

  Good idea, +1,

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
[EMAIL PROTECTED]  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] PATCH: Ensure errors are guarenteed reported in virConnectOpen

2008-08-22 Thread Daniel Veillard
On Tue, Aug 19, 2008 at 11:35:18AM +0100, Daniel P. Berrange wrote:
 The virConnectOpen method is unfortuantely rather special. While there is
 a virConnect object available, the current rule is that drivers must report
 errors against the global error object, because upon failure no virConnect
 object will be returned to the caller. Unforatunately, despite frequent
 cleanups of code getting this wrong, I've just audited the remote driver
 and found another 20 or so places where its wrong. This is clearly not a
 sustainable approach to error reporting.
 
 The guarenteed correct solution is actually rather simple
 
  - Always report errors against the virConnect object, even in the driver
open method
 
  - In the cleanup patch of do_open() in libvirt.c, if no global error is
set, copy the error from the virConnect object's virError, into the 
global virError.
 
 With this in place we can change all the drivers back to always report
 against the error object, and thus cleanup some disgusting code like
 
 __virRaiseError (in_open ? NULL : conn, ...
 
 To just
 
 __virRaiseError (conn, ...

  Okay, make sense, it's better to be more flexible in one location
and simplify code everywhere else.

  +1

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
[EMAIL PROTECTED]  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] static ip address

2008-08-22 Thread Daniel Veillard
On Fri, Aug 15, 2008 at 10:43:15AM +0100, Daniel P. Berrange wrote:
 On Tue, Aug 12, 2008 at 05:24:01PM -0400, Daniel Veillard wrote:
New patch with just a pair of minor fixes which seems to work just fine 
  for
  me. The syntax is nearly the old one, just use host instead of statichost:
  
ip address=192.168.122.1 netmask=255.255.255.0
  dhcp
range start=192.168.122.4 end=192.168.122.254 /
host mac=00:16:3E:XX:XX:XX host=XXX
 
 Seems a little odd to have 'host' as the element name and attribute
 name at once. Perhaps the atribute should just be name='' ?

  Agreed, I noticed that but it's not a serious problem, still using
name feels cleaner I will change that before commiting !
 
  @@ -1103,6 +1103,8 @@
   2 + /* --listen-address 10.0.0.1 */
   1 + /* --dhcp-leasefile=path */
   (2 * network-def-nranges) + /* --dhcp-range 10.0.0.2,10.0.0.254 
  */
  +/*  --dhcp-host 01:23:45:67:89:0a,hostname,10.0.0.3 */
  +(2 * network-def-nhosts) +
 
 Using the --dhcp-host option means we can't easily update on the fly, but
 this patch doesn't support on the fly updates anyway, so not a huge deal.
 In the future we can switch to --dhcp-host-file instead.
 
 
 ACK to including this.

  okay, will push later today,

thanks !

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
[EMAIL PROTECTED]  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] anyone implementing host device enumeration?

2008-08-22 Thread Daniel Veillard
On Fri, Aug 22, 2008 at 09:44:47AM +0100, Daniel P. Berrange wrote:
 On Thu, Aug 21, 2008 at 03:18:57PM -0400, David Lively wrote:
  Hi -
  
I'm about to start working on host device enumeration, along the
  (HAL-ish) lines of what was discussed back in April:
https://www.redhat.com/archives/libvir-list/2008-April/msg5.html
  
I know the xml details haven't been fully fleshed out, but there seems
  to be agreement that it will be a fairly direct mapping from (a subset
  of the) HAL info to the details that we need in the xml.  Doubtless it
  will take a while to figure out exactly what subset suffices (and, for
  that matter, if everything needed is available via HAL ...), but I think
  the work is well-defined for some of the obvious details (discussed in
  the above thread) on which there's broad agreement.
  
Is anyone working on such an implementation?
 
 No one that I'm aware of - so go for it. FYI, one complication has come
 up since my initial proposal. HAL is going away, to be replaced by
 DeviceKit :-( That said they will co-exist and HAL will be around for
 a while and they will both expose very similar properties for devices
 so it shouldn't too much trouble. I think we just need to be conservative
 when deciding how many of the HAL properties we need to add in libvirt.
 I'd go for a pretty minimal set at first.

  And HAL based detection might work on more platforms at least
  initially like OpenSolaris,

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
[EMAIL PROTECTED]  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] storage pool discovery

2008-08-22 Thread David Lively
On Fri, 2008-08-22 at 09:50 +0100, Daniel P. Berrange wrote:
  +const char *start_tag = SourceList\n;
  +const char *end_tag = /SourceList\n;
 
 I'd prefer that to be  sources  - we avoid capitals in the
 XML element names everywhere else, and in the few cases of
 joining words use an underscore, but I think plural form is
 OK for this.

I prefer sources as well, so I'll change this.  And I'll put those
defines in storage_backend.h as well.

As long as we're on the subject of naming (and before it's too late),
it's been bothering me that we keep calling this storage pool
discovery.  To me, storage source discovery seems more accurate
(because they're not pools until we define libvirt pools based on the
sources).  So I'd prefer renaming the various *Discover[Storage]Pools*
functions (and support structs) introduced in this patch to
*Discover[Storage]Sources*.  I was just sticking with the
originally-proposed names to avoid confusion.  What do you all think?

Dave

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] storage pool discovery

2008-08-22 Thread David Lively
Here's the patch with those issues addressed (also merged with latest
upstream - avoids internal.h merge conflict) ...

Dave

On Fri, 2008-08-22 at 09:50 +0100, Daniel P. Berrange wrote:
 On Thu, Aug 21, 2008 at 10:29:43AM -0400, David Lively wrote:
  Hi Folks -
Here's my second pass at storage pool discovery.  I've taken Daniel's
  suggestion and made it return a single XML doc containing source
  elements rather than an array of pool docs (and also incorporated
  suggestions from Daniel V and Jim M).
Note that the storage source name patch is closely related
  (without it, the source docs returned for logical pools aren't
  correct).
 
 Excellant, I think this patch looks more or less good to commit now.
 
  +static char *
  +virStorageBackendFileSystemNetDiscoverPools(virConnectPtr conn,
  +const char *srcSpec,
  +unsigned int flags 
  ATTRIBUTE_UNUSED)
 
 ...
 
  +const char *prog[] = { SHOWMOUNT, --no-headers, --exports, NULL, 
  NULL };
  +const char *start_tag = SourceList\n;
  +const char *end_tag = /SourceList\n;
 
 I'd prefer that to be  sources  - we avoid capitals in the
 XML element names everywhere else, and in the few cases of
 joining words use an underscore, but I think plural form is
 OK for this.
 
  +static char *
  +virStorageBackendLogicalDiscoverPools(virConnectPtr conn,
  +  const char *srcSpec ATTRIBUTE_UNUSED,
  +  unsigned int flags ATTRIBUTE_UNUSED)
 
 
  +const char *prog[] = { VGS, --noheadings, -o, vg_name, NULL };
  +const char *start_tag = SourceList\n;
  +const char *end_tag = /SourceList\n;
 
 We should #define these two tags in storage_backend.h so each driver doesn't
 need to dup them
 
 
 Daniel
diff --git a/configure.in b/configure.in
index 9479f1c..430a097 100644
--- a/configure.in
+++ b/configure.in
@@ -671,6 +671,11 @@ if test $with_storage_fs = yes -o $with_storage_fs = check; then
   fi
 fi
 AM_CONDITIONAL([WITH_STORAGE_FS], [test $with_storage_fs = yes])
+if test $with_storage_fs = yes; then
+  AC_PATH_PROG([SHOWMOUNT], [showmount], [], [$PATH:/sbin:/usr/sbin])
+  AC_DEFINE_UNQUOTED([SHOWMOUNT], [$SHOWMOUNT],
+[Location or name of the showmount program])
+fi
 
 AC_PATH_PROG([QEMU_IMG], [qemu-img], [], [$PATH:/sbin:/usr/sbin:/bin:/usr/bin])
 if test -n $QEMU_IMG ; then
diff --git a/include/libvirt/libvirt.h b/include/libvirt/libvirt.h
index 9c3e1c2..5308fa9 100644
--- a/include/libvirt/libvirt.h
+++ b/include/libvirt/libvirt.h
@@ -890,6 +890,14 @@ int virConnectListDefinedStoragePools(virConnectPtr conn,
   int maxnames);
 
 /*
+ * Query a host for storage pools of a particular type
+ */
+char *  virConnectDiscoverStoragePools(virConnectPtr conn,
+   const char *type,
+   const char *srcSpec,
+   unsigned int flags);
+
+/*
  * Lookup pool by name or uuid
  */
 virStoragePoolPtr   virStoragePoolLookupByName  (virConnectPtr conn,
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index f077a26..3d06d76 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -890,6 +890,14 @@ int virConnectListDefinedStoragePools(virConnectPtr conn,
   int maxnames);
 
 /*
+ * Query a host for storage pools of a particular type
+ */
+char *  virConnectDiscoverStoragePools(virConnectPtr conn,
+   const char *type,
+   const char *srcSpec,
+   unsigned int flags);
+
+/*
  * Lookup pool by name or uuid
  */
 virStoragePoolPtr   virStoragePoolLookupByName  (virConnectPtr conn,
diff --git a/qemud/remote.c b/qemud/remote.c
index b5a6ec9..43b3a56 100644
--- a/qemud/remote.c
+++ b/qemud/remote.c
@@ -2958,6 +2958,27 @@ remoteDispatchListStoragePools (struct qemud_server *server ATTRIBUTE_UNUSED,
 }
 
 static int
+remoteDispatchDiscoverStoragePools (struct qemud_server *server ATTRIBUTE_UNUSED,
+struct qemud_client *client,
+remote_message_header *req,
+remote_discover_storage_pools_args *args,
+remote_discover_storage_pools_ret *ret)
+{
+CHECK_CONN(client);
+
+ret-xml =
+virConnectDiscoverStoragePools (client-conn,
+args-type,
+args-srcSpec ? *args-srcSpec : NULL,
+

Re: [libvirt] [PATCH] storage pool discovery

2008-08-22 Thread Daniel P. Berrange
On Fri, Aug 22, 2008 at 10:58:54AM -0400, David Lively wrote:
 On Fri, 2008-08-22 at 09:50 +0100, Daniel P. Berrange wrote:
   +const char *start_tag = SourceList\n;
   +const char *end_tag = /SourceList\n;
  
  I'd prefer that to be  sources  - we avoid capitals in the
  XML element names everywhere else, and in the few cases of
  joining words use an underscore, but I think plural form is
  OK for this.
 
 I prefer sources as well, so I'll change this.  And I'll put those
 defines in storage_backend.h as well.
 
 As long as we're on the subject of naming (and before it's too late),
 it's been bothering me that we keep calling this storage pool
 discovery.  To me, storage source discovery seems more accurate
 (because they're not pools until we define libvirt pools based on the
 sources).  So I'd prefer renaming the various *Discover[Storage]Pools*
 functions (and support structs) introduced in this patch to
 *Discover[Storage]Sources*.  I was just sticking with the
 originally-proposed names to avoid confusion.  What do you all think?

That sounds like a reasonable idea to me.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] Delete veth devices during lxcVMCleanup

2008-08-22 Thread Dan Smith
DB All my patches for LXC with exception of the pivot_root stuff are
DB now commited, so this is good to commit.

Done.

-- 
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: [EMAIL PROTECTED]


pgpg7jtngAkLp.pgp
Description: PGP signature
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] storage pool discovery

2008-08-22 Thread Jim Meyering
David Lively [EMAIL PROTECTED] wrote:
 Here's the patch with those issues addressed (also merged with latest
 upstream - avoids internal.h merge conflict) ...

Hi David,
Looks good.
A couple of minor suggestions and questions.

...
 diff --git a/configure.in b/configure.in
 index 9479f1c..430a097 100644
 --- a/configure.in
 +++ b/configure.in
 @@ -671,6 +671,11 @@ if test $with_storage_fs = yes -o $with_storage_fs 
 = check; then
fi
  fi
  AM_CONDITIONAL([WITH_STORAGE_FS], [test $with_storage_fs = yes])
 +if test $with_storage_fs = yes; then
 +  AC_PATH_PROG([SHOWMOUNT], [showmount], [], [$PATH:/sbin:/usr/sbin])
 +  AC_DEFINE_UNQUOTED([SHOWMOUNT], [$SHOWMOUNT],
 +[Location or name of the showmount program])
 +fi

Do we want to require the showmount package via the spec file?

...
 diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c
...
 +static int
 +virStorageBackendFileSystemNetDiscoverPoolsFunc(virConnectPtr conn 
 ATTRIBUTE_UNUSED,
 +virStoragePoolObjPtr pool 
 ATTRIBUTE_UNUSED,
 +char **const groups,
 +void *data)
 +{
 +virNetfsDiscoverState *state = data;
 +virStringList *newItem;
 +const char *name, *path;

path can be const, too.

 +
 +path = groups[0];
 +
 +name = strrchr(path, '/');
 +if (name == NULL) {
 +virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
 +  _(invalid netfs path (no slash): %s), path);
 +return -1;
 +}
 +name += 1;

It'd be good to diagnose a path that ends in a slash (*name == '\0'),
rather than emitting XML with an empty name, below.

 +/* Append new XML desc to list */
 +
 +if (VIR_ALLOC(newItem) != 0) {
 +virStorageReportError(conn, VIR_ERR_NO_MEMORY, %s, _(new xml 
 desc));
 +return -1;
 +}
 +
 +if (asprintf(newItem-val,
 + sourcehost name='%s'/dir path='%s'//source\n,
 + state-host, path) = 0) {
 +virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, %s, 
 _(asprintf failed));
 +VIR_FREE(newItem);
 +return -1;
 +}
 +
 +newItem-len = strlen(newItem-val);
 +newItem-next = state-list;
 +state-list = newItem;
 +
 +return 0;
 +}
 +
 +static char *
 +virStorageBackendFileSystemNetDiscoverPools(virConnectPtr conn,
 +const char *srcSpec,
 +unsigned int flags 
 ATTRIBUTE_UNUSED)
 +{
 +/*
 + *  # showmount --no-headers -e HOSTNAME
 + *  /tmp   *
 + *  /A dir demo1.foo.bar,demo2.foo.bar
 + *
 + * Extract directory name (including possible interior spaces ...).
 + */
 +
 +const char *regexes[] = {
 +^(/.*\\S) +\\S+$
 +};
 +int vars[] = {
 +1
 +};
 +xmlDocPtr doc = NULL;
 +xmlXPathContextPtr xpath_ctxt = NULL;
 +virNetfsDiscoverState state = { .host = NULL, .list = NULL };
 +const char *prog[] = { SHOWMOUNT, --no-headers, --exports, NULL, 
 NULL };
 +int exitstatus, len;

Please use size_t as the type for string-length
variables like len.

 +virStringList *p;
 +char *retval = NULL;
 +
 +doc = xmlReadDoc((const xmlChar *)srcSpec, srcSpec.xml, NULL,
 + XML_PARSE_NOENT | XML_PARSE_NONET |
 + XML_PARSE_NOERROR | XML_PARSE_NOWARNING);
 +if (doc == NULL) {
 +virStorageReportError(conn, VIR_ERR_XML_ERROR, %s, _(bad source 
 spec));
 +goto cleanup;
 +}
 +
 +xpath_ctxt = xmlXPathNewContext(doc);
 +if (xpath_ctxt == NULL) {
 +virStorageReportError(conn, VIR_ERR_NO_MEMORY, %s, 
 _(xpath_ctxt));
 +goto cleanup;
 +}
 +
 +state.host = virXPathString(conn, string(/source/host/@name), 
 xpath_ctxt);
 +if (!state.host || !state.host[0]) {
 +virStorageReportError(conn, VIR_ERR_XML_ERROR, %s,
 +  _(missing host in source spec));
 +goto cleanup;
 +}
 +prog[3] = state.host;
 +
 +if (virStorageBackendRunProgRegex(conn, NULL, prog, 1, regexes, vars,
 +  
 virStorageBackendFileSystemNetDiscoverPoolsFunc,
 +  state, exitstatus)  0)
 +goto cleanup;
 +
 +/* Turn list of strings into final document */
 +len = strlen(SOURCES_START_TAG) + strlen(SOURCES_END_TAG);
 +for (p = state.list; p; p = p-next)
 +len += p-len;
 +if (VIR_ALLOC_N(retval, len+1)  0) {
 +virStorageReportError(conn, VIR_ERR_NO_MEMORY, _(retval (%d 
 bytes)), len);
 +goto cleanup;
 +}
 +strcpy(retval, SOURCES_START_TAG);
 +len = strlen(SOURCES_START_TAG);
 +for (p = state.list; p; p = p-next) {
 +strcpy(retval + len, p-val);
 +len += p-len;
 +}
 +strcpy(retval + len, SOURCES_END_TAG);
 +
 + cleanup:
 +

Re: [libvirt] [PATCH] kvm: maxVCPU runtime detection

2008-08-22 Thread Jim Meyering
Daniel P. Berrange [EMAIL PROTECTED] wrote:
 On Fri, Aug 22, 2008 at 03:16:42PM +0200, Guido G?nther wrote:
 Hi,
 with recent linux kernels we can detect the maximum number of virtual
 cpus at runtime via an ioctl. Possible patch attached. It does this on
 every call to qemudGetMaxVCPUs. Would you prefer something that does
 this only once in qemudStartup()?
 Cheers,
  -- Guido


 +dnl
 +dnl check for kvm headers
 +dnl
 +AC_CHECK_HEADERS([linux/kvm.h])

 Hmm, I wonder if that's commonly installed by Fedora/Debian RPMs
 for KVM

I've just checked up to date rawhide and debian unstable systems.
Both had this:

$ grep KVM_CHECK_EXTENSION /usr/include/linux/kvm.h
#define KVM_CHECK_EXTENSION   _IO(KVMIO,   0x03)

But rawhide lacks KVM_CAP_NR_VCPUS, while Debian has it.

$ grep -r KVM_CAP_NR_VCPUS /usr/include/linux/kvm.h
#define KVM_CAP_NR_VCPUS 9   /* returns max vcpus per vm */

 It might be neccessary to just #define the IOCTL constant in
 our own header files if its not available.

Good point.
Otherwise, this patch looks fine.

 @@ -72,6 +77,9 @@
  static int qemudShutdown(void);
  #endif

 +/* device for kvm ioctls */
 +#define KVM_DEVICE /dev/kvm
 +
  /* qemudDebug statements should be changed to use this macro instead. */
  #define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt, __VA_ARGS__)
  #define DEBUG0(msg) VIR_DEBUG(__FILE__, %s, msg)
 @@ -1768,6 +1776,29 @@ static const char *qemudGetType(virConnectPtr conn 
 ATTRIBUTE_UNUSED) {
  return QEMU;
  }

 +
 +static int kvmGetMaxVCPUs(void) {
 +int maxvcpus = 1;
 +
 +#if defined(KVM_CAP_NR_VCPUS)
 +int r, fd;
 +
 +fd = open(KVM_DEVICE, O_RDONLY);
 +if (fd  0) {
 +qemudLog(QEMUD_WARN, _(Unable to open  KVM_DEVICE : %s\n), 
 strerror(errno));
 +return maxvcpus;
 +}
 +
 +r = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS);
 +if (r  0)
 +maxvcpus = r;
 +
 +close(fd);
 +#endif
 +return maxvcpus;
 +}

 This is all very cool. IIRC Cole has some code which could guess
 a reasonable maxvcpus from KVM version number which we could
 potentially fallback to

 Daniel

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] PATCH: Allow specific FDs to be kept open in virExec()

2008-08-22 Thread Jim Meyering
Daniel P. Berrange [EMAIL PROTECTED] wrote:

 With my recent patches to virExec(), all FDs except stdin/out/err are closed
 before the child is exec'd to prevent accidental leaks. Of course I forgot
 the one key place where we need to propagate FDs... The TAP devices passed
 to QEMU.

 This patch adds a 'keepfd' parameter to virExec() which allows the caller
 to specify a bitset of file descriptors to keep open, and updates the
 callers to use this as required. The QEMU driver specifies any TAP fds
 and the LXC driver specifies its PTY this way removing a previous hack.

This all looks fine, assuming you're adjusting it to use the
standard fd_set type.

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] kvm: maxVCPU runtime detection

2008-08-22 Thread Guido Günther
On Fri, Aug 22, 2008 at 07:26:12PM +0200, Jim Meyering wrote:
[..snip..] 
 I've just checked up to date rawhide and debian unstable systems.
 Both had this:
 
 $ grep KVM_CHECK_EXTENSION /usr/include/linux/kvm.h
 #define KVM_CHECK_EXTENSION   _IO(KVMIO,   0x03)
 
 But rawhide lacks KVM_CAP_NR_VCPUS, while Debian has it.
 
 $ grep -r KVM_CAP_NR_VCPUS /usr/include/linux/kvm.h
 #define KVM_CAP_NR_VCPUS 9   /* returns max vcpus per vm */

Debian has it only in unstable not in testing. That's why I added the
header check as well as the #if defined(KVM_CAP_NR_VCPUS) - the code
stays as is for old header files and uses the definition (and therefore
the ioctl) if available. 
Rawhide will get the defintion with the next libc header update I guess.
 -- Guido

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] storage pool discovery

2008-08-22 Thread David Lively
Hi Jim -
  Thanks for your comments.  I really appreciate the detailed look.
I'll implement your suggestions (including the refactoring of the code
to turn a virStringList into a single XML string), though I have a
question about one of them:

On Fri, 2008-08-22 at 19:16 +0200, Jim Meyering wrote:
  diff --git a/configure.in b/configure.in
  index 9479f1c..430a097 100644
  --- a/configure.in
  +++ b/configure.in
  @@ -671,6 +671,11 @@ if test $with_storage_fs = yes -o 
  $with_storage_fs = check; then
 fi
   fi
   AM_CONDITIONAL([WITH_STORAGE_FS], [test $with_storage_fs = yes])
  +if test $with_storage_fs = yes; then
  +  AC_PATH_PROG([SHOWMOUNT], [showmount], [], [$PATH:/sbin:/usr/sbin])
  +  AC_DEFINE_UNQUOTED([SHOWMOUNT], [$SHOWMOUNT],
  +[Location or name of the showmount program])
  +fi
 
 Do we want to require the showmount package via the spec file?

I was a little worried about this too.  It's probably better to handle
this more like iscsiadmin/--with-storage-iscsi: defaults to enabling
iscsi backend if and only if iscsiadmin is present.  Fails with
--with-storage-iscsi=yes explicitly specified and iscsiadmin not found.

But I'm not sure we want to disable the storage_fs backend just because
we can't find showmount.  So maybe there should be another config option
--with-storage-netfs-discovery to cover the only code that actually uses
showmount?  Either of these sounds reasonable to me.  Do you have a
preference?

Dave


--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] storage pool discovery

2008-08-22 Thread David Lively
On Fri, 2008-08-22 at 19:16 +0200, Jim Meyering wrote:
  +const char *name, *path;
 
 path can be const, too.

It is, at least according to my gcc (4.3.0-8, x86_64, Fedora9).

Compile the following and without FORCE_WARNING to check:

void check()
{
#ifdef FORCE_WARNING
const char *foo;
char *bar;
#else
const char *foo, *bar;
#endif

foo = I'm a const char *;
bar = foo;  // warns iff bar not const char *
}

Dave


--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] storage pool discovery

2008-08-22 Thread Jim Meyering
David Lively [EMAIL PROTECTED] wrote:
 On Fri, 2008-08-22 at 19:16 +0200, Jim Meyering wrote:
  +const char *name, *path;

 path can be const, too.

 It is, at least according to my gcc (4.3.0-8, x86_64, Fedora9).

 Compile the following and without FORCE_WARNING to check:

 void check()
 {
 #ifdef FORCE_WARNING
 const char *foo;
 char *bar;
 #else
 const char *foo, *bar;
 #endif

Oops.  Now you know why I prefer to declare them on separate lines:
one less rule to remember that way ;-)

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] storage pool discovery

2008-08-22 Thread Jim Meyering
David Lively [EMAIL PROTECTED] wrote:
...
   AM_CONDITIONAL([WITH_STORAGE_FS], [test $with_storage_fs = yes])
  +if test $with_storage_fs = yes; then
  +  AC_PATH_PROG([SHOWMOUNT], [showmount], [], [$PATH:/sbin:/usr/sbin])
  +  AC_DEFINE_UNQUOTED([SHOWMOUNT], [$SHOWMOUNT],
  +[Location or name of the showmount program])
  +fi

 Do we want to require the showmount package via the spec file?

 I was a little worried about this too.  It's probably better to handle
 this more like iscsiadmin/--with-storage-iscsi: defaults to enabling
 iscsi backend if and only if iscsiadmin is present.  Fails with
 --with-storage-iscsi=yes explicitly specified and iscsiadmin not found.

Adding Requires: showmount and BuildRequires lines to
libvirt.spec.in would be similar to the approach of requiring
the iscsi-initiator-utils package (which contains iscsiadm):

$ grep isc *.spec.in
Requires: iscsi-initiator-utils
BuildRequires: iscsi-initiator-utils

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] storage pool discovery

2008-08-22 Thread Daniel P. Berrange
On Fri, Aug 22, 2008 at 11:06:38PM +0200, Jim Meyering wrote:
 David Lively [EMAIL PROTECTED] wrote:
 ...
AM_CONDITIONAL([WITH_STORAGE_FS], [test $with_storage_fs = yes])
   +if test $with_storage_fs = yes; then
   +  AC_PATH_PROG([SHOWMOUNT], [showmount], [], [$PATH:/sbin:/usr/sbin])
   +  AC_DEFINE_UNQUOTED([SHOWMOUNT], [$SHOWMOUNT],
   +[Location or name of the showmount program])
   +fi
 
  Do we want to require the showmount package via the spec file?
 
  I was a little worried about this too.  It's probably better to handle
  this more like iscsiadmin/--with-storage-iscsi: defaults to enabling
  iscsi backend if and only if iscsiadmin is present.  Fails with
  --with-storage-iscsi=yes explicitly specified and iscsiadmin not found.
 
 Adding Requires: showmount and BuildRequires lines to
 libvirt.spec.in would be similar to the approach of requiring
 the iscsi-initiator-utils package (which contains iscsiadm):

Yes, basically the spec file should encode 'best practice' and as such
it is reasonable to add a 'Requires: showmount'.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list