Re: [libvirt] [PATCH] create() and destroy() support for Power Hypervisor

2009-10-30 Thread Eduardo Otubo 

Hello Matthias,

First of all, I would like to thank you for all the comments you made. I 
tried to fix all those things up. But I still have some points to say:


Matthias Bolte wrote:
  +virCapsPtr

+phypCapsInit(void)
+{
+struct utsname utsname;
+virCapsPtr caps;
+virCapsGuestPtr guest;
+
+uname(utsname);
+
+if ((caps = virCapabilitiesNew(utsname.machine, 0, 0)) == NULL)
+goto no_memory;
+
+/* Some machines have problematic NUMA toplogy causing
+ * unexpected failures. We don't want to break the QEMU
+ * driver in this scenario, so log errors  carry on
+ */
+if (nodeCapsInitNUMA(caps)  0) {
+virCapabilitiesFreeNUMAInfo(caps);
+VIR_WARN0
+(Failed to query host NUMA topology, disabling NUMA 
capabilities);
+}
+
+/* XXX shouldn't 'borrow' KVM's prefix */
+virCapabilitiesSetMacPrefix(caps, (unsigned char[]) {
+0x52, 0x54, 0x00});
+
+if ((guest = virCapabilitiesAddGuest(caps,
+ linux,
+ utsname.machine,
+ sizeof(int) == 4 ? 32 : 8,
+ NULL, NULL, 0, NULL)) == NULL)
+goto no_memory;
+
+if (virCapabilitiesAddGuestDomain(guest,
+  phyp, NULL, NULL, 0, NULL) == NULL)
+goto no_memory;
+
+return caps;
+
+  no_memory:
+virCapabilitiesFree(caps);
+return NULL;
+}


As I understand the mechanics of this driver, the driver is designed
to operator from a client machine. The caps should describe the
capabilities of the hypervisor machine, but this functions initializes
the caps based on the machine where the driver is running.


Yep, you're right. I really need to think a better way to gather remote 
information about hypervisor. I missunderstood this concept, but I 
surely will have a fix for next the patch.



+
+int
+phypUUIDTable_ReadFile(virConnectPtr conn)
+{
+phyp_driverPtr phyp_driver = conn-privateData;
+uuid_tablePtr uuid_table = phyp_driver-uuid_table;
+unsigned int i = 0;
+int fd = -1;
+char *local_file;
+int rc = 0;
+char buffer[1024];
+
+if (virAsprintf(local_file, ./uuid_table)  0) {
+virReportOOMError(conn);
+goto err;
+}


virAsprintf for a fixed string is a bit of overkill, just use const
char *local_file = ./uuid_table.

In addition IMHO using a file in the current working directory for
temporary purpose isn't a good idea. Use a temporary path returned by
mktemp(), or even better try to solve the problem without using a
temporary file.


I agree with you. But this was the fastest way I found to get this part 
functional. I'll come up with a better solution in the next patch.



+int
+phypUUIDTable_WriteFile(virConnectPtr conn)
+{
+phyp_driverPtr phyp_driver = conn-privateData;
+uuid_tablePtr uuid_table = phyp_driver-uuid_table;
+unsigned int i = 0;
+int fd = -1;
+char *local_file;
+
+if (virAsprintf(local_file, ./uuid_table)  0) {
+virReportOOMError(conn);
+goto err;
+}


See comment in phypUUIDTable_ReadFile() about virAsprintf for a static
string etc.


+if ((fd = creat(local_file, 0755)) == -1)
+goto err;
+
+for (i = 0; i  uuid_table-nlpars; i++) {
+if (write(fd, uuid_table-lpars[i]-id, 
sizeof(uuid_table-lpars[i]-id)) == -1)
+VIR_WARN(%s, Unable to write information to local file.);
+
+if (write(fd, uuid_table-lpars[i]-uuid, VIR_UUID_BUFLEN) == -1)
+VIR_WARN(%s, Unable to write information to local file.);


Failed or incomplete writes shouldn't just warnings, they must be
error, because they corrupt the file.

Your binary file format is based on offsets. At offset 0 the first ID
is located, at offset 4 the first UUID, at offset 20 the next ID etc.
If you miss to write the appropriate number of bytes per item you'll
fail to read the data back in correctly.


+}
+close(fd);
+goto exit;
+
+  exit:
+VIR_FREE(local_file);
+return 0;
+
+  err:
+VIR_FREE(local_file);
+return -1;
+}


It seems you just write the UUID table to a file to read it back in
again in phypUUIDTable_Push(). I would suggest to remove the detour
using the temporary file, but instead calculate the total size of the
data (uuid_table-nlpars * (sizeof (int) + VIR_UUID_BUFLEN)) for
libssh2_scp_send() and write the data directly via
libssh2_channel_write(). Basically merge phypUUIDTable_WriteFile()
into phypUUIDTable_Push() and phypUUIDTable_ReadFile() into
phypUUIDTable_Pull().


This also applies to above comment, I'll come up with a better way to 
handle this.


Change log:
 * Now we have CPU management! Now I assume the user will know how to 
operate a HMC/IVM IBM system. This saves a lot of (useless) coding time 
for me :)

 * I solved all the other issues Matthias pointed.

Next steps:
 * Find a better way to

Re: [libvirt] Restore after restart libvirtd

2009-10-30 Thread Chris Lalancette 
Diego Woitasen wrote:
 Hi,
  I 'm playing with save/restore domains. It's working, but if I
 restart libvirtd after 'save domain file' and then try to restore it I
 get:
 
 error: Failed to restore domain from /tmp/domain.out
 error: operation failed: domain 'domainname' is already defined with
 uuid 17c77406-b6e3-621a-1fae-420affce8f48
 
 If I move domainname.xml out of libvirt config. directory it works,
 but I think this should not be necessary.

No, that's definitely not necessary, so something is wrong.  I just tried the
same thing on the head of libvirt git, and it worked just fine:

# ./daemon/libvirtd --verbose --listen
# ./tools/virsh save guest /tmp/foo.save
# kill libvirtd
# ./daemon/libvirtd --verbose --listen
# ./tools/virsh restore /tmp/foo.save

What version of libvirt are you using?  What are the exact steps you are taking
to try this out?

-- 
Chris Lalancette

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

[libvirt] Re: [virt-tools-list] Remote management over SSH fails to connect

2009-10-30 Thread Richard W.M. Jones 
On Mon, Oct 26, 2009 at 06:37:01PM +0100, Jon Nordby wrote:
 For the archives: the openbsd netcat implementation provides the -U option,
 where as gnu netcat does not.

Indeed - there are two separate and incompatible implementations of
the netcat program!  Only the one we use in Fedora supports the -U
option.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.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] QEMU driver thread safety rules

2009-10-30 Thread Daniel P. Berrange 
On Thu, Oct 29, 2009 at 06:04:29PM +0100, Daniel Veillard wrote:
  All other API calls making changes get safely queued up at
  step 1, but  API calls which simply wish to query information
  can run without being blocked at all. This fixes the major
  concurrency problem with running monitor commands. The use
  of a condition variable at the start of step 1, also allows
  us to time out API calls, if some other thread get stuck in
  the monitor for too long.  I think this also makes the use of
  a RWLock on the QEMU driver unneccessary, since no code will
  ever be holding a mutex in any place that sleeps/wait. Only
  the condition variable will be held during sleeps/waits.
  
  Since we'll now effectively have 3 locks, and 1 condition
  variable this is getting kind of complex. So the rest of this
  mail is a file I propose to put in src/qemu/THREADS.txt
  describing what is going on, and showing the recommended 
  design patterns to use.
 
   I have just one remark, this separation between APIs might
 be done one level up, i.e. at the library entry point level
 we should know what may induce a state change and those could
 be flagged more formally. This may help other drivers where
 libvirt needs to keep the state instead of asking the hypervisor.

It can't be done at the library entry level, since the locking needs
to be done against objects that are private to the driver.

  Basic locking primitives
  
  
  There are a number of locks on various objects
  
* struct qemud_driver: RWLock

Opps, this should have said 'Mutex' rather than RWLock 

  
  This is the top level lock on the entire driver. Every API call in
  the QEMU driver is blocked while this is held, though some internal
  callbacks may still run asynchronously. This lock must never be held
  for anything which sleeps/waits (ie monitor commands)
  
  When obtaining the driver lock, under *NO* circumstances must
  any lock be held on a virDomainObjPtr. This *WILL* result in
  deadlock.
 
   Any chance to enforce that at the code level ? Since we have
 primitives for both, we could once the RW lock is taken set a flag in
 the driver, and the DomainObj locking/unlocking routine could raise an
 error if this happen.

That is not possible todo safely. If you add a flag in the driver to
indicate whether it is locked or not, then you need to add another
mutex to protect reads/write to that flag, otherwise you've got a 
clear race condition in checking it.

* qemuMonitorPtr:  Mutex
  
  Lock to be used when invoking any monitor command to ensure safety
  wrt any asynchronous events that may be dispatched from the monitor.
  It should be acquired before running a command.
  
  The job condition *MUST* be held before acquiring the monitor lock
  
  The virDomainObjPtr lock *MUST* be held before acquiring the monitor
  lock.
  
  The virDomainObjPtr lock *MUST* then be released when invoking the
  monitor command.
  
  The driver lock *MUST* be released when invoking the monitor commands.
  
  This ensures that the virDomainObjPtr  driver are both unlocked while
  sleeping/waiting for the monitor response.
 
   I had to read this twice and I'm not sure I managed to fully map
 mentally the full set of constraints. 

Essentially there's a hierarchy of objects

   Driver - virDomainObjPtr  - qemuMonitorPtr

You have to acquire the locks in that order, and once you've acquired
the final qemuMonitorPtr lock, you must release the other locks
before running the actual monitor command.

  To acquire the job condition variable (int jobActive)
  
qemuDomainObjBeginJob()   (if driver is unlocked)
  - Increments ref count on virDomainObjPtr
  - Wait qemuDomainObjPrivate condition 'jobActive != 0' using 
  virDomainObjPtr mutex
  - Sets jobActive to 1
  
qemuDomainObjBeginJobWithDriver() (if driver needs to be locked)
  - Unlocks driver
  - Increments ref count on virDomainObjPtr
  - Wait qemuDomainObjPrivate condition 'jobActive != 0' using 
  virDomainObjPtr mutex
  - Sets jobActive to 1
  - Unlocks virDomainObjPtr
  - Locks driver
  - Locks virDomainObjPtr
  
 NB: this variant is required in order to comply with lock ordering rules
 for virDomainObjPtr vs driver
  
  
qemuDomainObjEndJob()
  - Set jobActive to 0
  - Signal on qemuDomainObjPrivate condition
  - Decrements ref count on virDomainObjPtr
  
  
  
  To acquire the QEMU monitor lock
  
qemuDomainObjEnterMonitor()
  - Acquires the qemuMonitorObjPtr lock
  - Releases the virDomainObjPtr lock
  
qemuDomainObjExitMonitor()
  - Acquires the virDomainObjPtr lock
  - Releases the qemuMonitorObjPtr lock
  
NB: caller must take care to drop the driver lock if neccessary
  
 
   It would be good if a maximum number of the constraints lested above
   could also be checked at runtime. Sure we could try

Re: [libvirt] remote clients are non-interruptible

2009-10-30 Thread Daniel Veillard 
On Sun, Oct 25, 2009 at 05:07:12PM +, Richard W.M. Jones wrote:
 On Sun, Oct 25, 2009 at 01:00:58PM -0400, John Levon wrote:
  
  remoteIOEventLoop() has this:
  
  6974 repoll:
  6975 ret = poll(fds, ARRAY_CARDINALITY(fds), -1);
  6976 if (ret  0  errno == EINTR)
  6977 goto repoll;
  
  with the result that all clients using the remote driver cannot be
  control-C'd. Dan, you added this code it seems, why?
 
 I'd agree with John here that probably you want to test only for errno
 == EAGAIN, not EINTR in this (client) case.

  Looking just at the Linux man page, EAGAIN is not in the list
of potential error returns for poll(2), but when looking at one of
the OpenGroup pages on the function
  http://www.opengroup.org/onlinepubs/009695399/functions/poll.html
it allows EAGAIN, so I just changed EINTR to EAGAIN, in this function
and pushed the change,

  thanks !

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | 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] QEMU driver thread safety rules

2009-10-30 Thread Daniel Veillard 
On Fri, Oct 30, 2009 at 11:05:03AM +, Daniel P. Berrange wrote:
 On Thu, Oct 29, 2009 at 06:04:29PM +0100, Daniel Veillard wrote:
   All other API calls making changes get safely queued up at
   step 1, but  API calls which simply wish to query information
   can run without being blocked at all. This fixes the major
   concurrency problem with running monitor commands. The use
   of a condition variable at the start of step 1, also allows
   us to time out API calls, if some other thread get stuck in
   the monitor for too long.  I think this also makes the use of
   a RWLock on the QEMU driver unneccessary, since no code will
   ever be holding a mutex in any place that sleeps/wait. Only
   the condition variable will be held during sleeps/waits.
   
   Since we'll now effectively have 3 locks, and 1 condition
   variable this is getting kind of complex. So the rest of this
   mail is a file I propose to put in src/qemu/THREADS.txt
   describing what is going on, and showing the recommended 
   design patterns to use.
  
I have just one remark, this separation between APIs might
  be done one level up, i.e. at the library entry point level
  we should know what may induce a state change and those could
  be flagged more formally. This may help other drivers where
  libvirt needs to keep the state instead of asking the hypervisor.
 
 It can't be done at the library entry level, since the locking needs
 to be done against objects that are private to the driver.

  hum, I'm not suggesting to do the locking one level up, but
to flag in some way entry points which may induce a state change.

   Basic locking primitives
   
   
   There are a number of locks on various objects
   
 * struct qemud_driver: RWLock
 
 Opps, this should have said 'Mutex' rather than RWLock 

  Ah right, since you said you dropped the idea I was a bit surprized...

   
   This is the top level lock on the entire driver. Every API call in
   the QEMU driver is blocked while this is held, though some internal
   callbacks may still run asynchronously. This lock must never be held
   for anything which sleeps/waits (ie monitor commands)
   
   When obtaining the driver lock, under *NO* circumstances must
   any lock be held on a virDomainObjPtr. This *WILL* result in
   deadlock.
  
Any chance to enforce that at the code level ? Since we have
  primitives for both, we could once the RW lock is taken set a flag in
  the driver, and the DomainObj locking/unlocking routine could raise an
  error if this happen.
 
 That is not possible todo safely. If you add a flag in the driver to
 indicate whether it is locked or not, then you need to add another
 mutex to protect reads/write to that flag, otherwise you've got a 
 clear race condition in checking it.

  Well you can't protect at 100% but that state change could be modified
after having taken the lock and just before releasing it. It's not a
protection against reentrancy, it's about detecting a problem at
runtime. You may not be able to detect it for a new nanosecs after
having taken the lock or before releasing it, but that will allow better
runtime error reporting in general than just a hung driver which was the
outcome when we introduced locking and chased locking.
  The goal is to be able to log (in general but not a garanteed 100%)
when we are doing something which may lead to a lock, report this in the
log files and allow users to send them.

 * qemuMonitorPtr:  Mutex
   
   Lock to be used when invoking any monitor command to ensure safety
   wrt any asynchronous events that may be dispatched from the monitor.
   It should be acquired before running a command.
   
   The job condition *MUST* be held before acquiring the monitor lock
   
   The virDomainObjPtr lock *MUST* be held before acquiring the monitor
   lock.
   
   The virDomainObjPtr lock *MUST* then be released when invoking the
   monitor command.
   
   The driver lock *MUST* be released when invoking the monitor commands.
   
   This ensures that the virDomainObjPtr  driver are both unlocked while
   sleeping/waiting for the monitor response.
  
I had to read this twice and I'm not sure I managed to fully map
  mentally the full set of constraints. 
 
 Essentially there's a hierarchy of objects
 
Driver - virDomainObjPtr  - qemuMonitorPtr
 
 You have to acquire the locks in that order, and once you've acquired
 the final qemuMonitorPtr lock, you must release the other locks
 before running the actual monitor command.

  okay

It would be good if a maximum number of the constraints lested above
could also be checked at runtime. Sure we could try to make new
checking rules like we did for previous locking checks but it's hard
for someone doing a patch to really run those. And I doubt the extra
burden of checking a few conditions in locking routines would really
impact

Re: [libvirt] [PATCH] Priority of vzctl create should be lower

2009-10-30 Thread Daniel Veillard 
On Mon, Oct 26, 2009 at 01:15:17PM +0100, Chris Lalancette wrote:
 Yuji NISHIDA wrote:
  Hi all
  
  About OpenVZ, it uses vzctl command that produces tar/gzip commands.
  I think we should lower these commands for running other procedures  
  prior to them.
  This is the patch to achieve it with nice/ionice commands.
 
 I'm not sure that this is such a good idea, as it is encoding policy into
 libvirt.  There have been other requests for being able to set the priorities 
 of
 certain domains, so it might make sense to have a more generic priority
 concept that could be set by the user.  Then the user would specify how
 important a particular task is, and we wouldn't be putting any policy into
 libvirt itself.

  Agreed hardcoding a policy like this at the lowest level doesn't
sound the right approach,

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | 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] Restore after restart libvirtd

2009-10-30 Thread Diego Woitasen 
2009/10/30 Chris Lalancette clala...@redhat.com:
 Diego Woitasen wrote:
 Hi,
  I 'm playing with save/restore domains. It's working, but if I
 restart libvirtd after 'save domain file' and then try to restore it I
 get:

 error: Failed to restore domain from /tmp/domain.out
 error: operation failed: domain 'domainname' is already defined with
 uuid 17c77406-b6e3-621a-1fae-420affce8f48

 If I move domainname.xml out of libvirt config. directory it works,
 but I think this should not be necessary.

 No, that's definitely not necessary, so something is wrong.  I just tried the
 same thing on the head of libvirt git, and it worked just fine:

 # ./daemon/libvirtd --verbose --listen
 # ./tools/virsh save guest /tmp/foo.save
 # kill libvirtd
 # ./daemon/libvirtd --verbose --listen
 # ./tools/virsh restore /tmp/foo.save

 What version of libvirt are you using?  What are the exact steps you are 
 taking
 to try this out?

 --
 Chris Lalancette



# /usr/local/sbin/libvirtd --version
/usr/local/sbin/libvirtd (libvirt) 0.7.2
# /usr/local/sbin/libvirtd -d
# virsh start MyDomain
Domain MyDomain started

# virsh save MyDomain /tmp/MyDomain.state
Domain MyDomain saved to /tmp/MyDomain.state

# killall libvirtd
# /usr/local/sbin/libvirtd -d
# virsh restore /tmp/MyDomain.state
error: Failed to restore domain from /tmp/MyDomain.state
error: operation failed: domain 'MyDomain' is already defined with
uuid 88709ecf-c60f-f57f-385d-3f3bd09ba040


-- 
Diego Woitasen

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

Re: [libvirt] 0.7.1 compile fails on opensuse 11.1

2009-10-30 Thread Daniel Veillard 
On Mon, Oct 26, 2009 at 09:37:48AM -0600, Jim Fehlig wrote:
 Jim Fehlig wrote:
  Pritesh Kothari wrote:

  it seems openSuSE 11.1 does not come with a pkgconfig for the
  device-mapper-devel package. I created a bug [1] for opensuse but was
  also told to mention it here so configure.in could be patched once the
  bug was fixed.
  

  Hi All,
 
  Just fixed this for ubuntu (should work now for suse as well) with some 
  help 
  from danpb and DV.
 
  The patch is attached below.

  
 
  ACK to this patch btw, it does solve the problem in SuSE distros where
  device-mapper-devel is missing pkgconfig file.  But the problem is
  really a bug in the devel package, so will this workaround be committed
  to libvirt configure script?

 
 Hi Daniel,
 
 Any plans to apply this patch?  I can keep it locally for environments
 with broken dev-mapper-devel package if need be.

  Ah right ! I had forgotten about it, it's pushed !

   thanks for the reminder :-)

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | 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] Restore after restart libvirtd

2009-10-30 Thread Diego Woitasen 
2009/10/30 Diego Woitasen di...@woitasen.com.ar:
 2009/10/30 Chris Lalancette clala...@redhat.com:
 Diego Woitasen wrote:
 Hi,
  I 'm playing with save/restore domains. It's working, but if I
 restart libvirtd after 'save domain file' and then try to restore it I
 get:

 error: Failed to restore domain from /tmp/domain.out
 error: operation failed: domain 'domainname' is already defined with
 uuid 17c77406-b6e3-621a-1fae-420affce8f48

 If I move domainname.xml out of libvirt config. directory it works,
 but I think this should not be necessary.

 No, that's definitely not necessary, so something is wrong.  I just tried the
 same thing on the head of libvirt git, and it worked just fine:

 # ./daemon/libvirtd --verbose --listen
 # ./tools/virsh save guest /tmp/foo.save
 # kill libvirtd
 # ./daemon/libvirtd --verbose --listen
 # ./tools/virsh restore /tmp/foo.save

 What version of libvirt are you using?  What are the exact steps you are 
 taking
 to try this out?

 --
 Chris Lalancette



 # /usr/local/sbin/libvirtd --version
 /usr/local/sbin/libvirtd (libvirt) 0.7.2
 # /usr/local/sbin/libvirtd -d
 # virsh start MyDomain
 Domain MyDomain started

 # virsh save MyDomain /tmp/MyDomain.state
 Domain MyDomain saved to /tmp/MyDomain.state

 # killall libvirtd
 # /usr/local/sbin/libvirtd -d
 # virsh restore /tmp/MyDomain.state
 error: Failed to restore domain from /tmp/MyDomain.state
 error: operation failed: domain 'MyDomain' is already defined with
 uuid 88709ecf-c60f-f57f-385d-3f3bd09ba040


 --
 Diego Woitasen



I tried that steps with git snapshot and I got the same result. Restore failed.

-- 
Diego Woitasen

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

Re: [libvirt] [PATCH 3/3] (REVISED) Support for IPv6 / multiple addresses per interface in virInterface

2009-10-30 Thread Laine Stump 

On 10/29/2009 02:53 AM, Laine Stump wrote:

On 10/28/2009 07:06 AM, Daniel Veillard wrote:

On Fri, Oct 23, 2009 at 01:31:19PM -0400, Laine Stump wrote:




 Hum, we don't check there that an ipv6/4 protocol is not defined
 multiple time. Maybe this could be slightly refactored to search
for 1 protocol node with type IPv4 and the one protocol node for type
Ipv6 something like
v4 = virXPathNode(conn, ./protoc...@family = 'ipv4'], ctxt)
v6 = virXPathNode(conn, ./protoc...@family = 'ipv6'], ctxt)


Yeah, I can do that.



Now that I'm actually doing it, I'm having doubts about this. Making 
this change leads to many other changes (mostly making the code simpler, 
but still changed lines are changed lines, and could lead to new bugs). 
More important, though, I notice that if we do it this way we lose the 
ability to report an error if someone botches up the family of a 
protocol - with this new method of parsing it will just be ignored; with 
the current setup in my patch, an error will be reported if there's a 
protocol that is missing family, or has family set to anything other 
than ipv4 or ipv6.


Would it maybe be better to still cycle through all the protocol nodes 
in the document with a loop as currently, but report an error if a 
particular protocol family is encountered a 2nd time?


If so, can/should that be submitted as a separate patch, rather than 
trying to get it into the patch that's already working? (If that's the 
case, I'll clean up and resend the patches sooner rather than later).



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

Re: [libvirt] [PATCH] Fix up a bogus TLS message.

2009-10-30 Thread Daniel P. Berrange 
On Fri, Oct 23, 2009 at 01:01:32PM +0200, Chris Lalancette wrote:
 When working with the TLS transport, I noticed that every single time
 a remote stream was closed, I would get a message like:
 
 09:09:40.793: error : remoteIOReadBuffer:7328 : failed to read from TLS 
 socket A TLS packet with unexpected length was received.
 libvir: QEMU error : failed to read from TLS socket A TLS packet with 
 unexpected length was received.
 
 in the logs.  This happens because of a race in libvirtd; one thread
 handles the doRemoteClose(), which calls gnutls_bye() followed by close()
 while another thread is poll()'ing on the same fd.  Once the close()
 happens, the poll returns with revents set to POLLIN, and we would poll
 one more time for data from the now-closed fd.  Fix this situation by
 setting poll-session to NULL when we clean up, and check for that in
 remoteIOHandleInput().

I'm not sure that this is correct. If the connection is being closed
while another thread is still using it, that sounds like a bug that
should be fixed, because whatever just called doRemoteClose() has
also free'd 'priv', and so whatever other thread is in remoteIOReadBuffer()
is now accessing free'd memory. 

 
 Signed-off-by: Chris Lalancette clala...@redhat.com
 ---
  src/remote/remote_driver.c |7 +++
  1 files changed, 7 insertions(+), 0 deletions(-)
 
 diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
 index bf001eb..335f44b 100644
 --- a/src/remote/remote_driver.c
 +++ b/src/remote/remote_driver.c
 @@ -1389,6 +1389,7 @@ doRemoteClose (virConnectPtr conn, struct private_data 
 *priv)
  if (priv-uses_tls  priv-session) {
  gnutls_bye (priv-session, GNUTLS_SHUT_RDWR);
  gnutls_deinit (priv-session);
 +priv-session = NULL;
  }
  #if HAVE_SASL
  if (priv-saslconn)
 @@ -7223,6 +7224,12 @@ remoteIOReadBuffer(virConnectPtr conn,
  int ret;
  
  if (priv-uses_tls) {
 +if (!priv-session) {
 +/* we may have reached here one more time after gnutls_bye()
 + * was called, so just return here
 + */
 +return 0;
 +}

If gnutls_bye() was just called, then 'priv' has also just been 
freed. So this must surely be accessing freed memory.

  tls_resend:
  ret = gnutls_record_recv (priv-session, bytes, len);
  if (ret == GNUTLS_E_INTERRUPTED)
 -- 
 1.6.0.6

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] network utilities

2009-10-30 Thread Daniel Veillard 
On Thu, Oct 22, 2009 at 04:46:51PM +0200, Daniel Veillard wrote:
  Revamping my previous patches to parse IPv4/6 numeric addresses,
 adding range computation and netmask checking entries too.
 
 As suggested I used an union for the socket address type:
 
 typedef union {
 struct sockaddr_storage stor;
 struct sockaddr_in inet4;
 struct sockaddr_in6 inet6;
 } virSocketAddr;
 typedef virSocketAddr *virSocketAddrPtr;
 
   But I wonder if it's not better to make it a struct and add
 the lenght of the address as this is usually needed when passing
 one of the struct sockaddr_* and that lenght is returned as part
 of parsing, keeping both together is probably more useful.
   A macro to get the type AF_INET/AF_INET6 out of a virSocketAddr
 might be useful instead of having the users manually guess that
 -stor.ss_family is teh filed to look at.
 
   Also I wonder how useful the Ipv6 code for netmask and ranges
 really is, it seems that IPv6 mostly go with prefix and never use
 netmasks, maybe that's just dead code, maybe that's just wrong,
 anyway here it is.
 
   I named the header and file network.[ch] as I think other
 network related utilities like validation or generation of
 MAC addresses could well be moved in that file too.
[..]
 commit d5ab523c72242d76184838d8215aa0ebe720c08e
 Author: Daniel Veillard veill...@redhat.com
 Date:   Thu Oct 22 16:34:43 2009 +0200
 
 Set of new network related utilities
 
 * src/util/network.h src/util/network.c: utilities to parse network
   addresses, check netmask and compute ranges

  Okay I have pushed this, I will need to update the internal symbols
list and adapt my patch 2 and 3 to solve the original bug on dnsmasq
that I was aiming at,

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | 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] Fix up a bogus TLS message.

2009-10-30 Thread Daniel P. Berrange 
On Fri, Oct 30, 2009 at 03:36:33PM +0100, Daniel Veillard wrote:
 On Fri, Oct 23, 2009 at 01:01:32PM +0200, Chris Lalancette wrote:
  When working with the TLS transport, I noticed that every single time
  a remote stream was closed, I would get a message like:
  
  09:09:40.793: error : remoteIOReadBuffer:7328 : failed to read from TLS 
  socket A TLS packet with unexpected length was received.
  libvir: QEMU error : failed to read from TLS socket A TLS packet with 
  unexpected length was received.
  
  in the logs.  This happens because of a race in libvirtd; one thread
  handles the doRemoteClose(), which calls gnutls_bye() followed by close()
  while another thread is poll()'ing on the same fd.  Once the close()
  happens, the poll returns with revents set to POLLIN, and we would poll
  one more time for data from the now-closed fd.  Fix this situation by
  setting poll-session to NULL when we clean up, and check for that in
  remoteIOHandleInput().
 
   ACK, thanks for following though !

I'd like to NACK this unless we understand more about why the conenction
is being closed while it is still in use.

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

[libvirt] [PATCH 1/2] Add internal symbols for util/network.c

2009-10-30 Thread Matthew Booth 
* src/libvirt_private.syms: Add symbols added in 24c8fc5d
---
 src/libvirt_private.syms |   10 ++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3997704..96c2b3b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -285,6 +285,16 @@ virReallocN;
 virFree;
 
 
+# network.h
+virSocketParseAddr;
+virSocketParseIpv4Addr;
+virSocketParseIpv6Addr;
+virSocketAddrInNetwork;
+virSocketGetRange;
+virSocketAddrIsNetmask;
+virSocketCheckNetmask;
+
+
 # network_conf.h
 virNetworkAssignDef;
 virNetworkConfigFile;
-- 
1.6.2.5

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

[libvirt] [PATCH 2/2] src/util/network.c: Fix typos in comments

2009-10-30 Thread Matthew Booth 
---
 src/util/network.c |8 
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/util/network.c b/src/util/network.c
index 8581cdc..abd866c 100644
--- a/src/util/network.c
+++ b/src/util/network.c
@@ -91,11 +91,11 @@ virSocketParseAddr(const char *val, virSocketAddrPtr addr, 
int hint) {
 /*
  * virSocketParseIpv4Addr:
  * @val: an IPv4 numeric address
- * @addr: the loacation to store the result
+ * @addr: the location to store the result
  *
  * Extract the address storage from an IPv4 numeric address
  *
- * Returns the lenght of the network address or -1 in case of error.
+ * Returns the length of the network address or -1 in case of error.
  */
 int
 virSocketParseIpv4Addr(const char *val, virSocketAddrPtr addr) {
@@ -105,11 +105,11 @@ virSocketParseIpv4Addr(const char *val, virSocketAddrPtr 
addr) {
 /*
  * virSocketParseIpv6Addr:
  * @val: an IPv6 numeric address
- * @addr: the loacation to store the result
+ * @addr: the location to store the result
  *
  * Extract the address storage from an IPv6 numeric address
  *
- * Returns the lenght of the network address or -1 in case of error.
+ * Returns the length of the network address or -1 in case of error.
  */
 int
 virSocketParseIpv6Addr(const char *val, virSocketAddrPtr addr) {
-- 
1.6.2.5

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

Re: [libvirt] [PATCH 3/3] (REVISED) Support for IPv6 / multiple addresses per interface in virInterface

2009-10-30 Thread Daniel Veillard 
On Fri, Oct 30, 2009 at 10:34:03AM -0400, Laine Stump wrote:
 On 10/29/2009 02:53 AM, Laine Stump wrote:
 On 10/28/2009 07:06 AM, Daniel Veillard wrote:
 On Fri, Oct 23, 2009 at 01:31:19PM -0400, Laine Stump wrote:


  Hum, we don't check there that an ipv6/4 protocol is not defined
  multiple time. Maybe this could be slightly refactored to search
 for 1 protocol node with type IPv4 and the one protocol node for type
 Ipv6 something like
 v4 = virXPathNode(conn, ./protoc...@family = 'ipv4'], ctxt)
 v6 = virXPathNode(conn, ./protoc...@family = 'ipv6'], ctxt)

 Yeah, I can do that.


 Now that I'm actually doing it, I'm having doubts about this. Making  
 this change leads to many other changes (mostly making the code simpler,  
 but still changed lines are changed lines, and could lead to new bugs).  
 More important, though, I notice that if we do it this way we lose the  
 ability to report an error if someone botches up the family of a  
 protocol - with this new method of parsing it will just be ignored; with  
 the current setup in my patch, an error will be reported if there's a  
 protocol that is missing family, or has family set to anything other  
 than ipv4 or ipv6.

  Ah right, I didn't though about that side of the checking

 Would it maybe be better to still cycle through all the protocol nodes  
 in the document with a loop as currently, but report an error if a  
 particular protocol family is encountered a 2nd time?

  yup,

 If so, can/should that be submitted as a separate patch, rather than  
 trying to get it into the patch that's already working? (If that's the  
 case, I'll clean up and resend the patches sooner rather than later).

  Let's just make a cleanup patch on top of the current one !

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | 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 1/2] Add internal symbols for util/network.c

2009-10-30 Thread Daniel Veillard 
On Fri, Oct 30, 2009 at 03:24:27PM +, Matthew Booth wrote:
 * src/libvirt_private.syms: Add symbols added in 24c8fc5d
 ---
  src/libvirt_private.syms |   10 ++
  1 files changed, 10 insertions(+), 0 deletions(-)
 
 diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
 index 3997704..96c2b3b 100644
 --- a/src/libvirt_private.syms
 +++ b/src/libvirt_private.syms
 @@ -285,6 +285,16 @@ virReallocN;
  virFree;
  
  
 +# network.h
 +virSocketParseAddr;
 +virSocketParseIpv4Addr;
 +virSocketParseIpv6Addr;
 +virSocketAddrInNetwork;
 +virSocketGetRange;
 +virSocketAddrIsNetmask;
 +virSocketCheckNetmask;
 +
 +
  # network_conf.h
  virNetworkAssignDef;
  virNetworkConfigFile;

  Ah, I had that in my tree, sounds like an ACK :-)
I just prefer mine because it's sorted alphabetically ;-)

  thanks !

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | 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 2/2] src/util/network.c: Fix typos in comments

2009-10-30 Thread Daniel Veillard 
On Fri, Oct 30, 2009 at 03:24:28PM +, Matthew Booth wrote:
 ---
  src/util/network.c |8 
  1 files changed, 4 insertions(+), 4 deletions(-)
 
 diff --git a/src/util/network.c b/src/util/network.c

  Ah, yup, nice :-) applied !

thanks !

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | 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

[libvirt] Don't use private struct member names of in6_addr

2009-10-30 Thread Matthias Bolte 
__in6_u.__u6_addr16 is the private name for this struct member,
s6_addr16 is the public one. Our buildserver revealed this problem,
because for some reason the internal names are missing the __ prefix
there.

Matthias
diff --git a/src/util/network.c b/src/util/network.c
index abd866c..5e3cb3a 100644
--- a/src/util/network.c
+++ b/src/util/network.c
@@ -46,7 +46,7 @@ static int getIPv6Addr(virSocketAddrPtr addr, virIPv6AddrPtr tab) {
 if ((addr == NULL) || (tab == NULL) || (addr-stor.ss_family != AF_INET6))
 return(-1);
 
-val = (virIPv6AddrPtr) (addr-inet6.sin6_addr.__in6_u.__u6_addr16);
+val = (virIPv6AddrPtr) (addr-inet6.sin6_addr.s6_addr16);
 
 for (i = 0;i  8;i++) {
 (*tab)[i] = ntohs((*val)[i]);
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] create() and destroy() support for Power Hypervisor

2009-10-30 Thread Matthias Bolte 
2009/10/30 Eduardo Otubo ot...@linux.vnet.ibm.com:
 Hello Matthias,

 First of all, I would like to thank you for all the comments you made. I
 tried to fix all those things up. But I still have some points to say:

 Matthias Bolte wrote:
   +virCapsPtr

 +phypCapsInit(void)
 +{
 +    struct utsname utsname;
 +    virCapsPtr caps;
 +    virCapsGuestPtr guest;
 +
 +    uname(utsname);
 +
 +    if ((caps = virCapabilitiesNew(utsname.machine, 0, 0)) == NULL)
 +        goto no_memory;
 +
 +    /* Some machines have problematic NUMA toplogy causing
 +     * unexpected failures. We don't want to break the QEMU
 +     * driver in this scenario, so log errors  carry on
 +     */
 +    if (nodeCapsInitNUMA(caps)  0) {
 +        virCapabilitiesFreeNUMAInfo(caps);
 +        VIR_WARN0
 +            (Failed to query host NUMA topology, disabling NUMA
 capabilities);
 +    }
 +
 +    /* XXX shouldn't 'borrow' KVM's prefix */
 +    virCapabilitiesSetMacPrefix(caps, (unsigned char[]) {
 +                                0x52, 0x54, 0x00});
 +
 +    if ((guest = virCapabilitiesAddGuest(caps,
 +                                         linux,
 +                                         utsname.machine,
 +                                         sizeof(int) == 4 ? 32 : 8,
 +                                         NULL, NULL, 0, NULL)) == NULL)
 +        goto no_memory;
 +
 +    if (virCapabilitiesAddGuestDomain(guest,
 +                                      phyp, NULL, NULL, 0, NULL) ==
 NULL)
 +        goto no_memory;
 +
 +    return caps;
 +
 +  no_memory:
 +    virCapabilitiesFree(caps);
 +    return NULL;
 +}

 As I understand the mechanics of this driver, the driver is designed
 to operator from a client machine. The caps should describe the
 capabilities of the hypervisor machine, but this functions initializes
 the caps based on the machine where the driver is running.

 Yep, you're right. I really need to think a better way to gather remote
 information about hypervisor. I missunderstood this concept, but I surely
 will have a fix for next the patch.

Okay.

 +
 +int
 +phypUUIDTable_ReadFile(virConnectPtr conn)
 +{
 +    phyp_driverPtr phyp_driver = conn-privateData;
 +    uuid_tablePtr uuid_table = phyp_driver-uuid_table;
 +    unsigned int i = 0;
 +    int fd = -1;
 +    char *local_file;
 +    int rc = 0;
 +    char buffer[1024];
 +
 +    if (virAsprintf(local_file, ./uuid_table)  0) {
 +        virReportOOMError(conn);
 +        goto err;
 +    }

 virAsprintf for a fixed string is a bit of overkill, just use const
 char *local_file = ./uuid_table.

 In addition IMHO using a file in the current working directory for
 temporary purpose isn't a good idea. Use a temporary path returned by
 mktemp(), or even better try to solve the problem without using a
 temporary file.

 I agree with you. But this was the fastest way I found to get this part
 functional. I'll come up with a better solution in the next patch.

Okay.

 +int
 +phypUUIDTable_WriteFile(virConnectPtr conn)
 +{
 +    phyp_driverPtr phyp_driver = conn-privateData;
 +    uuid_tablePtr uuid_table = phyp_driver-uuid_table;
 +    unsigned int i = 0;
 +    int fd = -1;
 +    char *local_file;
 +
 +    if (virAsprintf(local_file, ./uuid_table)  0) {
 +        virReportOOMError(conn);
 +        goto err;
 +    }

 See comment in phypUUIDTable_ReadFile() about virAsprintf for a static
 string etc.

 +    if ((fd = creat(local_file, 0755)) == -1)
 +        goto err;
 +
 +    for (i = 0; i  uuid_table-nlpars; i++) {
 +        if (write(fd, uuid_table-lpars[i]-id,
 sizeof(uuid_table-lpars[i]-id)) == -1)
 +            VIR_WARN(%s, Unable to write information to local
 file.);
 +
 +        if (write(fd, uuid_table-lpars[i]-uuid, VIR_UUID_BUFLEN) ==
 -1)
 +            VIR_WARN(%s, Unable to write information to local
 file.);

 Failed or incomplete writes shouldn't just warnings, they must be
 error, because they corrupt the file.

 Your binary file format is based on offsets. At offset 0 the first ID
 is located, at offset 4 the first UUID, at offset 20 the next ID etc.
 If you miss to write the appropriate number of bytes per item you'll
 fail to read the data back in correctly.

 +    }
 +    close(fd);
 +    goto exit;
 +
 +  exit:
 +    VIR_FREE(local_file);
 +    return 0;
 +
 +  err:
 +    VIR_FREE(local_file);
 +    return -1;
 +}

 It seems you just write the UUID table to a file to read it back in
 again in phypUUIDTable_Push(). I would suggest to remove the detour
 using the temporary file, but instead calculate the total size of the
 data (uuid_table-nlpars * (sizeof (int) + VIR_UUID_BUFLEN)) for
 libssh2_scp_send() and write the data directly via
 libssh2_channel_write(). Basically merge phypUUIDTable_WriteFile()
 into phypUUIDTable_Push() and phypUUIDTable_ReadFile() into
 phypUUIDTable_Pull().

 This also applies to above comment, I'll come up with a better way to handle
 this.

 Change log:
  * Now we have CPU management! Now I assume the user will know