Re: [libvirt] [PATCH] ALL_LINGUAS: remove no, now that it's superseded by np.po
On Sun, Oct 19, 2008 at 11:56:49AM +0200, Jim Meyering wrote: Without the first change below, make distcheck would fail due to the absence of po/no.po. But that file was deliberately removed, because np.po supersedes it. Oops, right, I forgot to update the list However, there is no need to maintain that hard-coded list of .po files. Here's a proposed patch to do away with it: diff --git a/configure.in b/configure.in index 54f1fe1..32fffb2 100644 --- a/configure.in +++ b/configure.in @@ -1011,12 +1011,7 @@ AM_CONDITIONAL([WITH_LIBVIRTD],[test x$with_libvirtd = xyes]) dnl Check for gettext AM_GNU_GETTEXT_VERSION([0.14.1]) AM_GNU_GETTEXT([external]) -if test -d po -then -ALL_LINGUAS=`(cd po /dev/null ls *.po) | sed 's+\.po$++'` -else -ALL_LINGUAS=af am ar as be bg bn_IN bn ca cs cy da de el en_GB es et eu_ES fa fi fr gl gu he hi hr hu hy id is it ja ka kn ko ku lo lt lv mk ml mr ms my nb nl nn nso or pa pl pt_BR pt ro ru si sk sl sq [EMAIL PROTECTED] sr sv ta te th tr uk ur vi zh_CN zh_TW zu -fi +ALL_LINGUAS=`(cd $srcdir/po /dev/null ls *.po) | sed 's+\.po$++'` dnl Extra link-time flags for Cygwin. dnl Copied from libxml2 configure.in, but I removed mingw changes yes, that makes sense, +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] xen+ssh
On Sun, Oct 19, 2008 at 08:14:30PM -0500, deface wrote: Trying to connect to remote host via ssh. keys work fine but get this -- *Trace - * Unable to open connection to hypervisor URI 'xen+ssh://[EMAIL PROTECTED]/': class 'libvirt.libvirtError' virConnectOpenAuth() failed socket closed unexpectedly Traceback (most recent call last): File /usr/share/virt-manager/virtManager/connection.py, line 430, in _open_thread None], flags) File /usr/lib/python2.5/site-packages/libvirt.py, line 98, in openAuth if ret is None:raise libvirtError('virConnectOpenAuth() failed') libvirtError: virConnectOpenAuth() failed socket closed unexpectedly *Remote Log - * Oct 19 20:12:09 zeus sshd[1332]: Accepted publickey for root from x.x.x.x port 52555 ssh2 Oct 19 20:12:10 zeus sshd[1336]: pam_unix(sshd:session): session opened for user root by (uid=0) not sure where to go - any way to enable verbose/debug logging? This usually means that you don't have 'nc' (aka netcat) installed on the remote host, or that the 'libvirtd' daemon is not running. 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 2/4: implement getVersion method for openvz
On Tue, Oct 14, 2008 at 04:19:06PM +0100, Daniel P. Berrange wrote: This patch implements the getVersion driver method for openvz to report the version number of vzctl. This is needed in the next patch to determine if we have builtin support for bridges Looks fine except: +static int +openvzExtractVersionInfo(const char *cmd, int *retversion) +{ [...] +enum { MAX_HELP_OUTPUT_SIZE = 8192 }; +int len = virFileReadLimFD(newstdout, MAX_HELP_OUTPUT_SIZE, help); Except that part which puzzles me. Why defining an enum for that. Also enum is one of the worse part of C, it has no clear storage size definition, and virFileReadLimFD takes an input int anyway... 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 4/4: support FS template config
On Thu, Oct 16, 2008 at 11:28:40AM +0200, Jim Meyering wrote: Daniel P. Berrange [EMAIL PROTECTED] wrote: The root filesystem for an openvz guest is defined from a template name. We support this when creating a new guest, but never include this info when dumping the XML. Thsi patch addresses this problem by reading the OSTEMPLATE config parameter diff -r e0c166ce24bd src/openvz_conf.c --- a/src/openvz_conf.c Tue Oct 14 15:46:24 2008 +0100 +++ b/src/openvz_conf.c Tue Oct 14 15:49:41 2008 +0100 @@ -308,6 +308,47 @@ error: } +static int +openvzReadFSConf(virConnectPtr conn, + virDomainDefPtr def, + int veid) { +int ret; +virDomainFSDefPtr fs; This needs to be initialized here or in the 'if' block. Otherwise, it will be freed uninitialized upon read failure: virDomainFSDefPtr fs = NULL; Thanks, will fix that and then commit 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 1/4: More generic MAC address handling
On Thu, Oct 16, 2008 at 12:37:36PM +0200, Jim Meyering wrote: Daniel P. Berrange [EMAIL PROTECTED] wrote: This patch improves the MAC address handling. Currently our XML parser auto-generates a MAC addres using the KVM vendor prefix. This isn't much use for other drivers. This patch addresses this: - Stores each driver's vendor prefix in the capability object - Changes domain parser to use the per-driver vendor prefix for generating mac addresses - Adds more utility methods to util.c for parsing/generating/formatting MAC addresses - Updates each driver to record its vendor prefix for MAC address. This all looks fine, but I have a question about the moved code that makes up the new virGenerateMacAddr function: +void virGenerateMacAddr(const unsigned char *prefix, +unsigned char *addr) +{ +addr[0] = prefix[0]; +addr[1] = prefix[1]; +addr[2] = prefix[2]; +addr[3] = 1 + (int)(256*(rand()/(RAND_MAX+1.0))); +addr[4] = 1 + (int)(256*(rand()/(RAND_MAX+1.0))); +addr[5] = 1 + (int)(256*(rand()/(RAND_MAX+1.0))); +} I presume the intent is to generated numbers in 0..255, but that code generates them in 1..256 and relies on the truncate-to-8-bit-unsigned-char to map back to 0..255. Why are those 1 + there? It doesn't seem to hurt, but doesn't make sense (to me), either. Removing them would not change the results. This was just moving code from elsewhere, not sure why it was like that in the first place. Seems sensible to just remove the '1 +' bit. Also, unless there's a guarantee that the random number state is initialized elsewhere, it should be initialized here, like it was in the now-removed xenXMAutoAssignMac function. srand((unsigned)time(NULL)); Or maybe just initialize it once at start-up? Could just add a call to srand() in virInitialize() i guess, although is that a reasonable thing for a shared library to be doing, rather than leaving it upto the application ? 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 3/4: SUpport bridge config for openvz
On Thu, Oct 16, 2008 at 07:52:42PM +0400, Evgeniy Sokolov wrote: This implements support for bridge configs in openvz following the rules set out in http://wiki.openvz.org/Virtual_Ethernet_device#Making_a_bridged_veth-device_persistent This simply requires that the admin has created /etc/vz/vznetctl.conf containing #!/bin/bash EXTERNAL_SCRIPT=/usr/sbin/vznetaddbr For openvz = 3.0.22, we have to manually re-write the NETIF line to add the bridge config parameter. It is alternative, but more flexible way. It require simple modification of vznetaddbr. Common scenario is to add VZHOSTBR=bridge if to config. For newer openvz we can simply pass the bridge name on the commnand line to --netif_add. Older openvz also requires that the admin install /usr/sbin/vznetaddbr since it is not available out of the box Daniel + +while(1) { +if (openvz_readline(fd, line, sizeof(line)) = 0) +break; + +if (!STRPREFIX(line, param)) { need to check for '='. Currently, if you will search for 'NETIF', you can find any string with such prefix. example 'NETIFOTHERPARAM' Ahh, good point, will fix that. +if (safewrite(temp_fd, line, strlen(line)) != +strlen(line)) +goto error; +} +} + /* + +if (!(opt = virBufferContentAndReset(buf))) +goto no_memory; + ADD_ARG_LIT(opt) ; Need to free opt Yes. -}else if (net-type == VIR_DOMAIN_NET_TYPE_ETHERNET +} else if (net-type == VIR_DOMAIN_NET_TYPE_ETHERNET net-data.ethernet.ipaddr != NULL) { +static int +openvzDomainSetNetworkConfig(virConnectPtr conn, + virDomainDefPtr def) +{ +unsigned int i; +virBuffer buf = VIR_BUFFER_INITIALIZER; +char *param; +struct openvz_driver *driver = (struct openvz_driver *) conn-privateData; + +for (i = 0 ; i def-nnets ; i++) { +if (driver-version VZCTL_BRIDGE_MIN_VERSION i 0) +virBufferAddLit(buf, ;); Need to check that network is bridge. In other case we will have NETIF='some params' Opps, completely forgot that check + +if (openvzDomainSetNetwork(conn, def-name, def-nets[i], buf) 0) { +openvzError(conn, VIR_ERR_INTERNAL_ERROR, +%s, _(Could not configure network)); +goto exit; +} +} + Regards, 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 4/4: support FS template config
On Tue, Oct 14, 2008 at 04:22:35PM +0100, Daniel P. Berrange wrote: The root filesystem for an openvz guest is defined from a template name. We support this when creating a new guest, but never include this info when dumping the XML. Thsi patch addresses this problem by reading the OSTEMPLATE config parameter Except the fs initialization to NULL pointed by Jim looks fine, 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 3/4: SUpport bridge config for openvz
On Fri, Oct 17, 2008 at 03:58:05PM +0200, Jim Meyering wrote: Daniel P. Berrange [EMAIL PROTECTED] wrote: ... +int +openvzWriteConfigParam(int vpsid, const char *param, const char *value) +{ +char conf_file[PATH_MAX]; +char temp_file[PATH_MAX]; +char line[PATH_MAX] ; +int fd, temp_fd; + +if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX, conf)0) +return -1; +if (openvzLocateConfFile(vpsid, temp_file, PATH_MAX, tmp)0) +return -1; + +fd = open(conf_file, O_RDONLY); +if (fd == -1) +return -1; +temp_fd = open(temp_file, O_WRONLY | O_CREAT | O_TRUNC, 0644); +if (temp_fd == -1) { +close(fd); +return -1; +} + +while(1) { +if (openvz_readline(fd, line, sizeof(line)) = 0) +break; + +if (!STRPREFIX(line, param)) { +if (safewrite(temp_fd, line, strlen(line)) != +strlen(line)) +goto error; +} +} + +if (safewrite(temp_fd, param, strlen(param)) != +strlen(param)) +goto error; +if (safewrite(temp_fd, =\, 2) != 2) +goto error; +if (safewrite(temp_fd, value, strlen(value)) != +strlen(value)) +goto error; +if (safewrite(temp_fd, \\n, 2) != 2) +goto error; How about this instead, so the reader doesn't have to manually count string lengths and verify that the VAR in strlen(VAR) on the RHS matches the one on the LHS: if (safewrite(temp_fd, param, strlen(param)) 0 || safewrite(temp_fd, =\, 2) 0 || safewrite(temp_fd, value, strlen(value)) 0 || safewrite(temp_fd, \\n, 2) 0) goto error; Yeah, that's good. +close(fd); +close(temp_fd); Officially, you should always check for failure when you've opened the file descriptor for writing. Ok, will fix. +fd = temp_fd = -1; + +if (rename(temp_file, conf_file) 0) +goto error; + +return 0; + +error: +fprintf(stderr, damn %s\n, strerror(errno)); How about mentioning the file name, too: fprintf(stderr, failed to write %s: %s\n, conf_file, strerror(errno)); That is debugging code I should have removed - we should raise a proper libvirt error if applicable. + +static int +openvzDomainSetNetworkConfig(virConnectPtr conn, + virDomainDefPtr def) +{ +unsigned int i; +virBuffer buf = VIR_BUFFER_INITIALIZER; +char *param; +struct openvz_driver *driver = (struct openvz_driver *) conn-privateData; + +for (i = 0 ; i def-nnets ; i++) { +if (driver-version VZCTL_BRIDGE_MIN_VERSION i 0) +virBufferAddLit(buf, ;); + +if (openvzDomainSetNetwork(conn, def-name, def-nets[i], buf) 0) { +openvzError(conn, VIR_ERR_INTERNAL_ERROR, +%s, _(Could not configure network)); +goto exit; +} +} + +param = virBufferContentAndReset(buf); +if (driver-version VZCTL_BRIDGE_MIN_VERSION def-nnets) { +if (openvzWriteConfigParam(strtoI(def-name), NETIF, param) 0) { +VIR_FREE(param); +openvzError(conn, VIR_ERR_INTERNAL_ERROR, +%s, _(cannot replace NETIF config)); +return -1; +} +} + +VIR_FREE(param); +return 0; param can be NULL and then dereferenced via openvzWriteConfigParam. How about this instead: if (driver-version VZCTL_BRIDGE_MIN_VERSION def-nnets) { char *param = virBufferContentAndReset(buf); if (openvzWriteConfigParam(strtoI(def-name), NETIF, param) 0) { VIR_FREE(param); openvzError(conn, VIR_ERR_INTERNAL_ERROR, %s, _(cannot replace NETIF config)); return -1; } VIR_FREE(param); } return 0; +exit: +param = virBufferContentAndReset(buf); +VIR_FREE(param); Then free the above directly. Yep, good idea. 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] Bridge network for VM using libvirt library
Hi I trying to create an application to manage virtual machines using libvirt library. Currently i am focusing with KVM hypervisor and bridge networking. I face two issues which i cant find a solution First issue: When i tried to reboot a virtual machine (using virDomain's reboot fn). I got exception message that there was no support in the hypervisor for reboot. is it true that we cant reboot a KVM based VM? Second issue: When i tried to setup a bridge network for KVM based VM. I got error message like this .. QEMU quit during console startup bind() failed I have listed below my steps which i tried setup bridge network. On Dom0 === brctl addbr br0 ifconfig eth0 0.0.0.0 brctl addif br0 eth0 ifconfig br0 192.168.1.82 netmask 255.255.255.0 up route add -net 192.168.1.0 netmask 255.255.255.0 br0 route add default gw 192.168.1.1 br0 ** dom0 IP 192.168.1.82 after that i checked the network connection of the Dom0.. it was fine. ifconfig was fine. then i created tun/tap device like given in the following link http://wiki.centos.org/HowTos/KVM#head-c02a0b33e7949b0bc3b151ac6e0bdfb91b6bbd1c sudo tunctl -b -u john sudo ifconfig tap1 up sudo brctl addif br0 tap1 export SDL_VIDEO_X11_DGAMOUSE=0 sudo iptables -I INPUT -i br0 -j ACCEPT then i started to define vm like this... domain type=kvm nametest82/name uuide5520db4-ad8a-38c3-b9d9-4e3c7a1052dc/uuid memory131072/memory vcpu1/vcpu on_poweroffdestroy/on_poweroff ostype arch=i686hvm/type boot dev=hd/ /os clock sync=localtime/ features pae/ acpi/ apic/ /features devices emulator/usr/bin/qemu-kvm/emulator disk type=file device=disk source file=/dev/FluidVM_grp/Windows_NT_2003_121560222596/ target dev=hda/ /disk interface type=bridge source bridge=br0/ /interface interface type=bridge source bridge=br0/ target dev=tap1/ mac address=00:16:3e:8f:ca:4b / /interface graphics type=vnc port=5900 listen=0.0.0.0/ /devices /domain is anything i am missing in setting up the bridge network? Thanks and Regards, -- Shan -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH: Pass 'remote_error' object to RPC handlers
On Fri, Oct 17, 2008 at 12:27:52PM +0100, Daniel P. Berrange wrote: In the libvirtd daemon, remote.c file the current RPC handlers have a return value contract saying 0 - success -1 - failure in libvirt call, no error returned -2 - failure in dispatch process, error already serialized sent While this isn't a problem for the code as it stands today, for the thread support I want to be able to avoid the dispatch handlers having to touch the 'struct qemud_client' object in normal usage. Allowing the RPC handlers to directly serialize send the error makes this impossible because it requires access to the client object. So this patch changes the way the RPC handlers deal with errors. The RPC handler API changes from typedef int (*dispatch_fn) (struct qemud_server *server, struct qemud_client *client, dispatch_args *args, dispatch_ret *ret); To typedef int (*dispatch_fn) (struct qemud_server *server, struct qemud_client *client, remote_error *err, dispatch_args *args, dispatch_ret *ret); Note, the addition of a 'remote_error *err' argument. Whenever an error occurs during the dispatch process, the RPC handler must populate this 'remote_error *err' object with the error details. This rule applies whether its a libvirt error, or a dispatch process error, so there is no longer any need to have separate -1 or -2 return values, a simple -1 or 0 return value suffices. Sounds fine, @@ -1381,7 +1380,14 @@ static void qemudDispatchClientRead(stru if (client-bufferOffset client-bufferLength) return; /* Not read enough */ -remoteDispatchClientRequest (server, client); +if ((len = remoteDispatchClientRequest (server, client)) == 0) +qemudDispatchClientFailure(server, client); Shouldn't you return here instead of continuing ? + +/* Set up the output buffer. */ +client-mode = QEMUD_MODE_TX_PACKET; +client-bufferLength = len; +client-bufferOffset = 0; + if (qemudRegisterClientEvent(server, client, 1) 0) qemudDispatchClientFailure(server, client); [...] +static void +remoteDispatchFormatError (remote_error *rerr, + const char *fmt, ...) +{ +va_list args; +char msgbuf[1024]; +char *msg = msgbuf; + +va_start (args, fmt); +vsnprintf (msgbuf, sizeof msgbuf, fmt, args); +va_end (args); + +remoteDispatchStringError (rerr, VIR_ERR_RPC, msg); +} Really no way we would get more than 1024 bytes ? The problem is that if this happens it's usually a stack being printed and the useful info can be at the end. [...] -remoteDispatchError (client, req, - _(program mismatch (actual %x, expected %x)), - req.prog, REMOTE_PROGRAM); -xdr_destroy (xdr); -return; +remoteDispatchFormatError (rerr, + _(program mismatch (actual %x, expected %x)), + req.prog, REMOTE_PROGRAM); stylistic issue, indenting to the opening ( is nice but can exceed the 80 columns rule, I usually prefer dropping the ( alignment to fit in. @@ -1395,6 +1368,7 @@ remoteDispatchDomainMigratePrepare (stru args-flags, dname, args-resource); if (r == -1) { maybe if (r 0) { as a mor generic error catching instruction VIR_FREE(uri_out); +remoteDispatchConnError(rerr, client-conn); return -1; } @@ -1439,7 +1413,10 @@ remoteDispatchDomainMigratePerform (stru args-uri, args-flags, dname, args-resource); virDomainFree (dom); -if (r == -1) return -1; +if (r == -1) { Same thing, we didn't defined virDrvDomainMigratePerform error code so maybe it's better to be generic [...] @@ -482,8 +482,12 @@ void virDomainRemoveInactive(virDomainOb memmove(doms-objs + i, doms-objs + i + 1, sizeof(*(doms-objs)) * (doms-count - (i + 1))); -if (VIR_REALLOC_N(doms-objs, doms-count - 1) 0) { -; /* Failure to reduce memory allocation isn't fatal */ +if (doms-count 1) { +if (VIR_REALLOC_N(doms-objs, doms-count - 1) 0) { +; /* Failure to reduce memory allocation isn't fatal */ +} +} else { +VIR_FREE(doms-objs); } doms-count--; Like this there is a couple case where new error handling blocks have been added, but after having reviewed the full patch I really could not find anything suspicious.
Re: [libvirt] Question about supporting other hypervisor
On Mon, Oct 20, 2008 at 06:16:14PM +0900, Atsushi SAKAI wrote: Hi, Daniel and all Thank you for various comments. These comments are very helpful for me. Daniel Veillard [EMAIL PROTECTED] wrote: Atsushi do you know if by default VMWare ESX and Hyper-V allow CIM XML access, or if some 'additional' software need to be installed to get this to work ? Also if you have feedback about the quality of the client CIM APIs implemented (completeness, interoperability ...) that's good to know too, For VMware, there has CIM tool. http://www.vmware.com/download/sdk/ For Hyper-V, there is a WMI. http://msdn.microsoft.com/en-us/library/aa490398.aspx By the way, I am not sure the quality of these CIM I/F implementation. But If there are have a possiblity, I will consider to check it. Well as Dan Smith pointed out this may get really tedious, especially the asynchronous part of CIM processing. 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] Re: [PATCH 07/12] Domain Events - remote driver
Just discovered one tiny problem here - need to check 'event' to see if the POLLHUP or POLLERR flags are set, and unregister the callback. Otherwise if you kill the server, the client will just spin on POLLHUP or ERR forever. Something like this ought todo the trick if (event (POLLERR | POLLHUP)) { virEventRemoveHandle(fd); return; } I've been looking over the rest of your changes. Generally, I agree all these suggestions are good ones...except for the code above With this code in, I run the following test 1. start libvirtd 2. begin to monitor events with event-test 3. virsh create foo.xml At this point, the event-test app encounters a HUP, or ERR, and stops monitoring for events - it will only ever get the Started event I handle this in the event-test poll loop via if ( pfd.revents POLLHUP ) { DEBUG0(Reset by peer); return -1; } Is it not a reasonable restriction to require the client app to handle a Hangup? -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PATCH 1/4: More generic MAC address handling
On Tue, Oct 14, 2008 at 04:17:57PM +0100, Daniel P. Berrange wrote: This patch improves the MAC address handling. Currently our XML parser auto-generates a MAC addres using the KVM vendor prefix. This isn't much use for other drivers. This patch addresses this: - Stores each driver's vendor prefix in the capability object - Changes domain parser to use the per-driver vendor prefix for generating mac addresses - Adds more utility methods to util.c for parsing/generating/formatting MAC addresses - Updates each driver to record its vendor prefix for MAC address. NB, for LXC driver we're 'borrowing' KVM's vendor prefix. Looks fine to me. Jim's raised an interesting point though, 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] Re: [PATCH 07/12] Domain Events - remote driver
On Fri, Oct 17, 2008 at 12:02:13PM -0400, Ben Guthro wrote: Deliver local callbacks in response to remote events remote_internal.c | 255 -- 1 file changed, 248 insertions(+), 7 deletions(-) diff --git a/src/remote_internal.c b/src/remote_internal.c index 35b7b4b..13537f7 100644 --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -34,6 +34,7 @@ +/** remoteDomainEventFired: + * + * The callback for monitoring the remote socket + * for event data + */ +void remoteDomainEventFired(int fd ATTRIBUTE_UNUSED, + int event ATTRIBUTE_UNUSED, + void *opaque) +{ +char buffer[REMOTE_MESSAGE_MAX]; +char buffer2[4]; +struct remote_message_header hdr; +XDR xdr; +int len; + +virConnectPtrconn = opaque; +struct private_data *priv = conn-privateData; + +DEBUG(%s : Event fired, __FUNCTION__); + +/* Read and deserialise length word. */ +if (really_read (conn, priv, 0, buffer2, sizeof buffer2) == -1) +return; Just discovered one tiny problem here - need to check 'event' to see if the POLLHUP or POLLERR flags are set, and unregister the callback. Otherwise if you kill the server, the client will just spin on POLLHUP or ERR forever. Something like this ought todo the trick if (event (POLLERR | POLLHUP)) { virEventRemoveHandle(fd); return; } before we try to read any data. Regards, 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] Re: [PATCH 07/12] Domain Events - remote driver
On Mon, Oct 20, 2008 at 10:48:38AM -0400, Ben Guthro wrote: Just discovered one tiny problem here - need to check 'event' to see if the POLLHUP or POLLERR flags are set, and unregister the callback. Otherwise if you kill the server, the client will just spin on POLLHUP or ERR forever. Something like this ought todo the trick if (event (POLLERR | POLLHUP)) { virEventRemoveHandle(fd); return; } I've been looking over the rest of your changes. Generally, I agree all these suggestions are good ones...except for the code above With this code in, I run the following test 1. start libvirtd 2. begin to monitor events with event-test 3. virsh create foo.xml At this point, the event-test app encounters a HUP, or ERR, and stops monitoring for events - it will only ever get the Started event That's a little odd - I'm not sure why 'event-test' would be getting a HUP/ERR when 'virsh' starts a domain. 'event-test' should only get a HUP/ERR if it looses its socket connection to the libvirtd server. Once you've lost the connection like this, the entire virConnectPtr is non-operative, and the client app needs to create a new virConnectPtr to re-connect. So removing the FD from the event loop shouldn't result in us loosing any events - we're already doomed there.to I handle this in the event-test poll loop via if ( pfd.revents POLLHUP ) { DEBUG0(Reset by peer); return -1; } The integration into the event loop though is only something the app will have general control off - eg, in the example libvirt-glib code I posted its totally opaque to the app. Is it not a reasonable restriction to require the client app to handle a Hangup? Once the socket is dead, all subsequent libvirt call made on that virConnectPtr object will throw errors, which the app should see. Though if they're only ever waiting for events, and not making any other calls, they'd not see it. Perhaps we could do with an explicit callback for connection disconnect. 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] Re: [PATCH 07/12] Domain Events - remote driver
OK - it looks like my EventImpl was passing along the wrong bits. I'll look into the token scheme suggested in an earlier email, and get this ready for re-submission. Would you prefer a new patch series, as before - or another patch that modifies the prior series? Daniel P. Berrange wrote on 10/20/2008 10:59 AM: On Mon, Oct 20, 2008 at 10:48:38AM -0400, Ben Guthro wrote: Just discovered one tiny problem here - need to check 'event' to see if the POLLHUP or POLLERR flags are set, and unregister the callback. Otherwise if you kill the server, the client will just spin on POLLHUP or ERR forever. Something like this ought todo the trick if (event (POLLERR | POLLHUP)) { virEventRemoveHandle(fd); return; } I've been looking over the rest of your changes. Generally, I agree all these suggestions are good ones...except for the code above With this code in, I run the following test 1. start libvirtd 2. begin to monitor events with event-test 3. virsh create foo.xml At this point, the event-test app encounters a HUP, or ERR, and stops monitoring for events - it will only ever get the Started event That's a little odd - I'm not sure why 'event-test' would be getting a HUP/ERR when 'virsh' starts a domain. 'event-test' should only get a HUP/ERR if it looses its socket connection to the libvirtd server. Once you've lost the connection like this, the entire virConnectPtr is non-operative, and the client app needs to create a new virConnectPtr to re-connect. So removing the FD from the event loop shouldn't result in us loosing any events - we're already doomed there.to I handle this in the event-test poll loop via if ( pfd.revents POLLHUP ) { DEBUG0(Reset by peer); return -1; } The integration into the event loop though is only something the app will have general control off - eg, in the example libvirt-glib code I posted its totally opaque to the app. Is it not a reasonable restriction to require the client app to handle a Hangup? Once the socket is dead, all subsequent libvirt call made on that virConnectPtr object will throw errors, which the app should see. Though if they're only ever waiting for events, and not making any other calls, they'd not see it. Perhaps we could do with an explicit callback for connection disconnect. Daniel -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Re: [PATCH 07/12] Domain Events - remote driver
On Mon, Oct 20, 2008 at 11:43:24AM -0400, Ben Guthro wrote: OK - it looks like my EventImpl was passing along the wrong bits. I'll look into the token scheme suggested in an earlier email, and get this ready for re-submission. Would you prefer a new patch series, as before - or another patch that modifies the prior series? Best to just re-post the whole series with changes incorporated, rather than trying to add more patches ontop of patches. Regards, 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] Re: [PATCH 04/12] Domain Events - rpc changes
On Sun, 2008-10-19 at 20:22 +0100, Daniel P. Berrange wrote: On Fri, Oct 17, 2008 at 11:58:15AM -0400, Ben Guthro wrote: Changes to the RPC protocol +struct remote_domain_event_ret { +remote_nonnull_domain dom; +int event; +unsigned long int callback; +unsigned long int user_data; +}; Using a 'unsigned long int' field to transmit the raw pointer feels a little wrong to me. Could we have the client side pass a simple integer 'token' when registering / unregistering, and have that 'token' passed back by the server in the actual event. The client could use this token to lookup the callback and user_data. Hold on. We can (and IMO should) quite easily avoid both this lookup and the passing of the callback pointer to the server: Suppose we have the same client registered for two different domain event callbacks. In the current patch, the server will send two RPCs per event, one for each callback (which the client then unmarshals, casts, and calls). But what if we sent just one RPC per event ( per client) and had the client walk its list of callbacks (which we'll need to track on the client side anyway, if we're not sending the data over the wire)? We *always* make *all* the callbacks on the list, so there's no point in making individual RPCs to fire off each callback individually. This gets rid of the need to send callback/user_data over the wire, and also doesn't require tokenization (which is all just extra overhead in this case). remote_domain_events_register/deregister_args structs will go away. remoteDispatchhDomainEventsRegister/Deregister will simply inc/dec a value, sending events only when the value is 0. While I'm not sure I've described this very well, I feel pretty strongly that it's the right way to go. If my explanation isn't clear, please get back to me ... Dave -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list