[libvirt] sysctl variables, FreeBSD
Is there a method that can be used to remove the sysctl tuning for FreeBSD in the pipeline? Here are the current patches I have, but would like to remove: http://www.freebsd.org/cgi/cvsweb.cgi/ports/devel/libvirt/files/ Thanks! Jason -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4] python: add virDomainGetCPUStats python binding API
dom.getCPUStats(True, 0) [{'cpu_time': 24699446159L, 'system_time': 1087000L, 'user_time': 95000L}] dom.getCPUStats(False, 0) [{'cpu_time': 8535292289L}, {'cpu_time': 1005395355L}, {'cpu_time': 9351766377L}, {'cpu_time': 5813545649L}] *generator.py Add a new naming rule *libvirt-override-api.xml The API function description *libvirt-override.c Implement it. --- python/generator.py |5 +- python/libvirt-override-api.xml | 13 python/libvirt-override.c | 147 +++ 3 files changed, 164 insertions(+), 1 deletions(-) diff --git a/python/generator.py b/python/generator.py index 98072f0..4f95cc9 100755 --- a/python/generator.py +++ b/python/generator.py @@ -423,7 +423,7 @@ skip_impl = ( 'virDomainGetBlockIoTune', 'virDomainSetInterfaceParameters', 'virDomainGetInterfaceParameters', -'virDomainGetCPUStats', # not implemented now. +'virDomainGetCPUStats', 'virDomainGetDiskErrors', ) @@ -966,6 +966,9 @@ def nameFixup(name, classe, type, file): elif name[0:19] == "virStorageVolLookup": func = name[3:] func = string.lower(func[0:1]) + func[1:] +elif name[0:20] == "virDomainGetCPUStats": +func = name[9:] +func = string.lower(func[0:1]) + func[1:] elif name[0:12] == "virDomainGet": func = name[12:] func = string.lower(func[0:1]) + func[1:] diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml index ab8f33a..26d9902 100644 --- a/python/libvirt-override-api.xml +++ b/python/libvirt-override-api.xml @@ -149,6 +149,19 @@ + + Extracts CPU statistics for a running domain. On success it will + return a list of data of dictionary type. If boolean total is False, the + first element of the list refers to CPU0 on the host, second element is + CPU1, and so on. The format of data struct is as follows: + [{cpu_time:xxx}, {cpu_time:xxx}, ...] + If it is True, it returns total domain CPU statistics in the format of + [{cpu_time:xxx, user_time:xxx, system_time:xxx}] + + + + + Extracts interface device statistics for a domain diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 792cfa3..5920456 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -378,6 +378,152 @@ cleanup: } static PyObject * +libvirt_virDomainGetCPUStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) +{ +virDomainPtr domain; +PyObject *pyobj_domain, *totalbool; +PyObject *cpu, *total; +PyObject *ret = NULL; +PyObject *error = NULL; +int ncpus = -1, start_cpu = 0; +int sumparams = 0, nparams = -1; +int i, i_retval; +unsigned int flags, totalflag; +virTypedParameterPtr params = NULL, cpuparams; + +if (!PyArg_ParseTuple(args, (char *)"OOi:virDomainGetCPUStats", + &pyobj_domain, &totalbool, &flags)) +return NULL; +domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); + +if (!PyBool_Check(totalbool)) { +PyErr_Format(PyExc_TypeError, +"The \"total\" attribute must be bool"); +return NULL; +} else { +/* Hack - Python's definition of Py_True breaks strict + * aliasing rules, so can't directly compare + */ +PyObject *hacktrue = PyBool_FromLong(1); +totalflag = hacktrue == totalbool ? 1 : 0; +Py_DECREF(hacktrue); +} + +if ((ret = PyList_New(0)) == NULL) +return NULL; + +if (!totalflag) { +LIBVIRT_BEGIN_ALLOW_THREADS; +ncpus = virDomainGetCPUStats(domain, NULL, 0, 0, 0, flags); +LIBVIRT_END_ALLOW_THREADS; + +if (ncpus < 0) { +error = VIR_PY_NONE; +goto error; +} + +LIBVIRT_BEGIN_ALLOW_THREADS; +nparams = virDomainGetCPUStats(domain, NULL, 0, 0, 1, flags); +LIBVIRT_END_ALLOW_THREADS; + +if (nparams < 0) { +error = VIR_PY_NONE; +goto error; +} + +sumparams = nparams * MIN(ncpus, 128); + +if (VIR_ALLOC_N(params, sumparams) < 0) { +error = PyErr_NoMemory(); +goto error; +} + +while (ncpus) { +int queried_ncpus = MIN(ncpus, 128); +if (nparams) { + +LIBVIRT_BEGIN_ALLOW_THREADS; +i_retval = virDomainGetCPUStats(domain, params, +nparams, start_cpu, queried_ncpus, flags); +LIBVIRT_END_ALLOW_THREADS; + +if (i_retval < 0) { +error = VIR_PY_NONE; +goto error; +} +} else { +i_retval = 0; +} + +for (i = 0; i < queried_ncpus; i++) { +cpuparams = ¶ms[i * nparams]; +if ((cpu = getPyVirTypedPa
Re: [libvirt] [PATCH 0/11 v2] Add supports for three new QMP events
ping. On 03/14/2012 11:26 PM, Osier Yang wrote: v1 ~ v2: * Two more patches, [5/11] to prohibit tray='open' for block type disk. And [9/11] to introduce a new domain state 'pmsuspended' * Definition (including name) for DEVICE_TRAY_MOVED event is changed into: typedef void (*virConnectDomainEventTrayChangeCallback)(virConnectPtr conn, virDomainPtr dom, const char *devAlias, int reason, void *opaque); * Definition for both SUSPEND and WAKEUP event are changed, one more parameter 'int reason' for future extension. * While SUSPEND event is emitted, the running domain will be transfered into the new domain state "pmsuspended", instead of "paused". And while WAKEUP is emitted, it will transfer the domain state to running only if the domain state is "pmsuspended". This patch series adds support for 3 new QMP events: WAKEUP, SUSPEND, and DEVICE_TRAY_MOVED, and related changes on domain's conf and status. [1/11] Add support for tray moved event [2/11] ~ [6/11]: New attribute "tray" is added to disk target, it indicates the tray status of removable disk, i.e. CDROM and Floppy disks, its value could be either of "open" or "closed", defaults to "closed", and a removable disk with tray == "open" won't have the source when domain is started. The value of "tray" will be updated while tray moved event is emitted from guest. tray status 'open' is prohibited for block type disk. Prior to these patches, if the user ejected the medium of removable disk from guest side, and then do migration or save/restoring, the guest will still starts the medium source ,and thus the medium will still exists in guest, which is strange. These patches fix it. [7/11] + [11/11]: Add support for wakeup event, and update the domain status to running if the domain is in pmsuspended state, while the wakeup event is emitted. [9/11] Introduce new domain state "pmsuspended", which indicates the domain has been suspended by guest power management, e.g, entered into s3 state. [8/11] + [10/11]: Add support for suspend event, and update the domain status to pmsuspended if the domain was running, while the suspend event is emitted. Osier Yang(11) Add support for event tray moved of removable disks docs: Add documentation for new attribute tray of disk target conf: Parse and for the tray attribute qemu: Do not start with source for removable disks if tray is open qemu: Prohibit setting tray status as open for block type disk qemu: Update tray status while tray moved event is emitted Add support for the wakeup event Add support for the suspend event New domain state pmsuspended qemu: Update domain state to pmsuspended while suspend event occurs qemu: Update domain status to running while wakeup event is emitted daemon/remote.c| 79 ++ docs/formatdomain.html.in | 13 ++- docs/schemas/domaincommon.rng |8 + examples/domain-events/events-c/event-test.c | 66 +- examples/domain-events/events-python/event-test.py | 12 ++ include/libvirt/libvirt.h.in | 98 - python/libvirt-override-virConnect.py | 27 python/libvirt-override.c | 150 src/conf/domain_conf.c | 71 -- src/conf/domain_conf.h |9 ++ src/conf/domain_event.c| 115 +++ src/conf/domain_event.h| 10 ++ src/libvirt_private.syms |6 + src/qemu/qemu_command.c| 31 - src/qemu/qemu_monitor.c| 33 + src/qemu/qemu_monitor.h| 16 ++- src/qemu/qemu_monitor_json.c | 45 ++ src/qemu/qemu_process.c| 121 src/remote/remote_driver.c | 95 src/remote/remote_protocol.x | 19 +++- src/remote_protocol-structs| 14 ++ tests/qemuxml2argvdata/qemuxml2argv-boot-cdrom.xml |2 +- .../qemuxml2argv-boot-complex-bootindex.xml|6 +- .../qemuxml2argvdata/qemuxml2argv-boot-complex.xml |6 +- .../qemuxml2argvdata/qemuxml2argv-boot-floppy.xml |2 +- ...uxml2argv-boot-menu-disable-drive-bootindex.xml |2 +- .../qemuxml2argv-boot-menu-disable-drive.xml |2 +- .../qemuxml2argv-boot-menu-disable.xml |2 +- .../qemuxml2argv-boot-menu-enable.xml |2 +- tests/qemuxml2ar
Re: [libvirt] [PATCH] conf: forbid use of multicast mac addresses
On 03/19/2012 01:46 PM, Eric Blake wrote: > On 03/19/2012 11:32 AM, Laine Stump wrote: >> A few times libvirt users manually setting mac addresses have >> complained of networking failure that ends up being due to a multicast >> mac address being used for a guest interface. This patch prevents that >> by loggin an error and failing if a multicast mac address is > s/loggin/logging/ > >> encountered in each of the three following cases: >> >> 1) domain xml mac address. >> 2) network xml bridge mac address. >> 3) network xml dhcp/host mac address. >> >> There are several other places where a mac address cna be input that > s/cna/can/ > >> aren't controlled in this manner because failure to do so has no >> consequences (e.g., if the address will be used to search through >> existing interfaces for a match). >> >> The RNG has been updated to add multiMacAddr and uniMacAddr along with >> the existing macAddr, and macAddr was switched to uniMacAddr where >> appropriate. >> --- >> docs/schemas/basictypes.rng | 17 - >> docs/schemas/domaincommon.rng |6 +++--- >> docs/schemas/network.rng |4 ++-- >> src/conf/domain_conf.c|8 +++- >> src/conf/network_conf.c | 33 - >> src/libvirt_private.syms |2 ++ >> src/util/virmacaddr.c | 13 + >> src/util/virmacaddr.h |3 ++- > Is there any way to add a test of an expected error, where we prove that > a multicast mac fails to validate? But since the existing tests use > unicast addrs, and those still validate, you've at least tested for no > regressions on sane usage. > >> + >> + >> + >> + >> + >> + >> + >> + > name="pattern">[a-fA-F0-9][02468aAcCeE](:[a-fA-F0-9]{2}){5} >> + >> + >> + >> + >> + > name="pattern">[a-fA-F0-9][13579bBdDfF](:[a-fA-F0-9]{2}){5} >> + >> + >> >> >> - ([a-fA-F0-9]{2}:){5}[a-fA-F0-9]{2} >> + [a-fA-F0-9]{2}(:[a-fA-F0-9]{2}){5} > Changed to match the earlier patterns. Nice patterns. > > ACK. If you do figure out how to add a negative test for expected > validation failure, I'm okay if you submit it as a separate followup patch. > Pushed (I agree that negative tests are in general a weak spot of the xml2argv and xml2xml tests, just can't afford the distraction to deal with it now). -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Minor docs fix
On 03/19/2012 05:52 PM, Martin Kletzander wrote: > End tag for "host" element was missing in example configuration > --- > docs/formatnetwork.html.in |1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in > index 907c046..7e8e991 100644 > --- a/docs/formatnetwork.html.in > +++ b/docs/formatnetwork.html.in > @@ -363,6 +363,7 @@ >> > >myhost >myhostalias > +> ACK and pushed. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] conf: return immediately on error in dhcp host element
On 03/19/2012 03:49 PM, Eric Blake wrote: > On 03/19/2012 12:46 PM, Laine Stump wrote: >> If an error was encountered parsing a dhcp host entry mac address or >> name, parsing would continue and log a less descriptive error that >> might make it more difficult to notice the true nature of the problem. >> >> This patch returns immediately on logging the first error. >> --- >> src/conf/network_conf.c |3 +++ >> 1 files changed, 3 insertions(+), 0 deletions(-) >> >> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c >> index 0333141..4341f11 100644 >> --- a/src/conf/network_conf.c >> +++ b/src/conf/network_conf.c >> @@ -430,13 +430,16 @@ virNetworkDHCPRangeDefParseXML(const char *networkName, >>_("Cannot parse MAC address '%s' in >> network '%s'"), >>mac, networkName); >> VIR_FREE(mac); >> +return -1; >> } >> name = virXMLPropString(cur, "name"); >> if ((name != NULL) && (!c_isalpha(name[0]))) { >> virNetworkReportError(VIR_ERR_INTERNAL_ERROR, >>_("Cannot use name address '%s' in >> network '%s'"), >>name, networkName); >> +VIR_FREE(mac); >> VIR_FREE(name); >> +return -1; > ACK. I was debating if it would have been any cleaner to have a > cleanup: label, but then you start having to do major refactoring to > take advantage of it, at which point this patch would no longer be quite > so small. > Yeah, I go through the same thought process every time I see cleanup code like this. It's so much more difficult to introduce leaks when there is a single exit from the function and everything is always cleaned up. Pushed. Thanks! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Minor docs fix
End tag for "host" element was missing in example configuration --- docs/formatnetwork.html.in |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 907c046..7e8e991 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -363,6 +363,7 @@myhost myhostalias +-- 1.7.3.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 4/3] snapshot: make offline qemu snapshots atomic
On 03/19/2012 07:40 AM, Peter Krempa wrote: > On 03/17/2012 05:33 PM, Eric Blake wrote: >> Offline internal snapshots can be rolled back with just a little >> bit of refactoring, meaning that we are now automatically atomic. >> >> * src/qemu/qemu_domain.c (qemuDomainSnapshotForEachQcow2): Move >> guts... >> (qemuDomainSnapshotForEachQcow2Raw): ...to new helper, to allow >> rollbacks. >> * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Offline >> snapshots are now atomic. >> --- > You rollback changes to disks if other disks are not snapshotable, but later > on, > when the actual qemu-img command is run and fails the rollback is not > performed. Good catch. > > I's suggest squashing in: > > if (virRun(qemuimgarg, NULL) < 0) { > if (try_all) { > VIR_WARN("skipping snapshot action on %s", > def->disks[i]->dst); > skipped = true; > continue; > +} else if (STREQ(op, "-c") && i) { > +/* We must roll back partial creation by deleting > + * all earlier snapshots. */ > +qemuDomainSnapshotForEachQcow2Raw(driver, def, name, > + "-d", false, i); > } Yep, that looks right. Thanks for the test case. > > Otherwise looks good. ACK with that suggested change. Same story as for 3/3 - I'll wait to push this until I've run a few more tests for the rest of my pending series, in case I find any more last-minute issues. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 3/3] snapshot: rudimentary qemu support for atomic disk snapshot
On 03/19/2012 06:20 AM, Peter Krempa wrote: > On 03/17/2012 05:33 PM, Eric Blake wrote: >> Taking an external snapshot of just one disk is atomic, without having >> to pause and resume the VM. This also paves the way for later patches >> to interact with the new qemu 'transaction' monitor command. >> >> +++ b/src/qemu/qemu_driver.c >> @@ -10279,6 +10295,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, >> &vm, snap, flags)< 0) >> goto cleanup; >> } else if (!virDomainObjIsActive(vm)) { >> +if (flags& VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) { >> +qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> +_("atomic snapshots of inactive domains >> not " >> + "implemented yet")); >> +goto cleanup; > > I wonder if we shouldn't add a error code for unimplemented > functionality to differentiate it from unsupported configurations. > (Well, but using a configuration that uses unimplemented functionality > makes it an unsupported configuration basically, so I don't really mind) This exact same error message is removed in 4/3, so I don't think it quite matters _what_ we put here. I guess I could rebase the series to swap 3/4 and 4/4, to avoid the churn. > > >> +} >> if (qemuDomainSnapshotCreateInactive(driver, vm, snap)< 0) >> goto cleanup; >> } else { > > ACK, Thanks for the reviews. I may wait another day or two before actually pushing, since I'm still in the middle of testing other snapshot-related patches, just to make sure my tests don't turn up any other needed last-minute changes to the ones I've posted so far. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] python: add virDomainGetCPUStats python binding API
On 03/19/2012 12:27 AM, Guannan Ren wrote: > dom.getCPUStats(True, 0) > [{'cpu_time': 24699446159L, 'system_time': 1087000L, 'user_time': > 95000L}] > dom.getCPUStats(False, 0) > [{'cpu_time': 8535292289L}, {'cpu_time': 1005395355L}, {'cpu_time': > 9351766377L}, {'cpu_time': 5813545649L}] > > *generator.py Add a new naming rule > *libvirt-override-api.xml The API function description > *libvirt-override.c Implement it. > --- > python/generator.py |5 +- > python/libvirt-override-api.xml | 13 > python/libvirt-override.c | 147 > +++ > 3 files changed, 164 insertions(+), 1 deletions(-) ACK once you fix one bug: > + > +for (i = 0; i < queried_ncpus; i++) { > +cpuparams = ¶ms[i * i_retval]; s/i_retval/nparams/ Your testing worked because you happened to have a situation where i_retval==nparams, but the API allows for i_retval < nparams (that is, the number of filled entries within a stride can be less than the number of slots reserved by the stride). -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] conf: return immediately on error in dhcp host element
On 03/19/2012 12:46 PM, Laine Stump wrote: > If an error was encountered parsing a dhcp host entry mac address or > name, parsing would continue and log a less descriptive error that > might make it more difficult to notice the true nature of the problem. > > This patch returns immediately on logging the first error. > --- > src/conf/network_conf.c |3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c > index 0333141..4341f11 100644 > --- a/src/conf/network_conf.c > +++ b/src/conf/network_conf.c > @@ -430,13 +430,16 @@ virNetworkDHCPRangeDefParseXML(const char *networkName, >_("Cannot parse MAC address '%s' in > network '%s'"), >mac, networkName); > VIR_FREE(mac); > +return -1; > } > name = virXMLPropString(cur, "name"); > if ((name != NULL) && (!c_isalpha(name[0]))) { > virNetworkReportError(VIR_ERR_INTERNAL_ERROR, >_("Cannot use name address '%s' in > network '%s'"), >name, networkName); > +VIR_FREE(mac); > VIR_FREE(name); > +return -1; ACK. I was debating if it would have been any cleaner to have a cleanup: label, but then you start having to do major refactoring to take advantage of it, at which point this patch would no longer be quite so small. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2] conf: return immediately on error in dhcp host element
If an error was encountered parsing a dhcp host entry mac address or name, parsing would continue and log a less descriptive error that might make it more difficult to notice the true nature of the problem. This patch returns immediately on logging the first error. --- src/conf/network_conf.c |3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 0333141..4341f11 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -430,13 +430,16 @@ virNetworkDHCPRangeDefParseXML(const char *networkName, _("Cannot parse MAC address '%s' in network '%s'"), mac, networkName); VIR_FREE(mac); +return -1; } name = virXMLPropString(cur, "name"); if ((name != NULL) && (!c_isalpha(name[0]))) { virNetworkReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot use name address '%s' in network '%s'"), name, networkName); +VIR_FREE(mac); VIR_FREE(name); +return -1; } /* * You need at least one MAC address or one host name -- 1.7.7.6 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Cpu mapping cleanup
On 03/19/2012 06:10 PM, Eric Blake wrote: > On 03/19/2012 06:29 AM, Martin Kletzander wrote: >> Using inheritance, this patch cleans up the cpu_map.xml file and also >> sorts all CPU features according to the feature and registry >> values. Model features are sorted the same way as foeatures in the > > s/foeatures/features/ > >> specification. >> Also few models that are related were organized together and parts of >> the XML are marked with comments >> --- >> src/cpu/cpu_map.xml | 455 >> +- >> 1 files changed, 82 insertions(+), 373 deletions(-) > > The inheritance is definitely an improvement for reducing lines of code. > The patch was a bit hard to read (splitting things into three patches: > reordering options, reordering CPU models into groups, then taking > advantage of inheritance; rather than doing all three actions in one > patch, might have been easier), but seems correct after the few minutes > I've spent on it, and isn't worth the time to respin the series just to > split the patch. I was thinking how to split the patch and this variant didn't cross my mind. Definitely next time =) > > ACK. > Could you also push it, please (I don't have permissions for that)? Thanks -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] python: make virDomainSetSchedulerParameters accpet integer argument
On 03/19/2012 12:03 PM, Eric Blake wrote: > > Why are you doing this for just VIR_TYPED_PARAM_ULLONG? I argue that we > should be doing it for all of the integral conversions. In fact, I'd argue that we want new helper functions in typewrappers.c, as counterparts to our libvirt_intWrap() and friends. Perhaps: /* Return 0 if obj could be unwrapped into the corresponding C type, -1 with python exception set otherwise. */ int libvirt_intUnwrap(PyObject *obj, int *val); int libvirt_uintUnwrap(PyObject *obj, unsigned int *val); int libvirt_longlongUnwrap(PyObject *obj, long long *val); int libvirt_ulonglongUnwrap(PyObject *obj, unsigned long long *val); Then the code in question simplifies to: case VIR_TYPED_PARAM_ULLONG: { unsigned long long ullong_val; if (libvirt_ulonglongUnwrap(value, &ullong_val) < 0) goto cleanup; temp->value.ul = ullong_val; -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] python: make virDomainSetSchedulerParameters accpet integer argument
On 03/19/2012 11:45 AM, Guannan Ren wrote: > When sending a Python integer as an argument to > PyLong_AsUnsignedLong or PyLong_AsUnsignedLongLong, > the following error occurs > > SystemError: Objects/longobject.c:980: > bad argument to internal function > > +++ b/python/libvirt-override.c > @@ -233,7 +233,14 @@ setPyVirTypedParameter(PyObject *info, > break; > case VIR_TYPED_PARAM_ULLONG: > { > -unsigned long long ullong_val = PyLong_AsUnsignedLongLong(value); > +unsigned long long ullong_val; Pre-initialize this to -1; otherwise... > +if (PyInt_Check(value)) > +ullong_val = (unsigned long long)PyInt_AsLong(value); Cast is unnecessary. > +else if (PyLong_Check(value)) > +ullong_val = PyLong_AsUnsignedLongLong(value); > +else > +PyErr_SetString(PyExc_TypeError, "an integer is required"); > + > if ((ullong_val == -1) && PyErr_Occurred()) ...if I don't pass in PyInt or PyLong, then this error detection will be based on uninitialized memory. Why are you doing this for just VIR_TYPED_PARAM_ULLONG? I argue that we should be doing it for all of the integral conversions. Needs a v2. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: forbid use of multicast mac addresses
On 03/19/2012 11:32 AM, Laine Stump wrote: > A few times libvirt users manually setting mac addresses have > complained of networking failure that ends up being due to a multicast > mac address being used for a guest interface. This patch prevents that > by loggin an error and failing if a multicast mac address is s/loggin/logging/ > encountered in each of the three following cases: > > 1) domain xml mac address. > 2) network xml bridge mac address. > 3) network xml dhcp/host mac address. > > There are several other places where a mac address cna be input that s/cna/can/ > aren't controlled in this manner because failure to do so has no > consequences (e.g., if the address will be used to search through > existing interfaces for a match). > > The RNG has been updated to add multiMacAddr and uniMacAddr along with > the existing macAddr, and macAddr was switched to uniMacAddr where > appropriate. > --- > docs/schemas/basictypes.rng | 17 - > docs/schemas/domaincommon.rng |6 +++--- > docs/schemas/network.rng |4 ++-- > src/conf/domain_conf.c|8 +++- > src/conf/network_conf.c | 33 - > src/libvirt_private.syms |2 ++ > src/util/virmacaddr.c | 13 + > src/util/virmacaddr.h |3 ++- Is there any way to add a test of an expected error, where we prove that a multicast mac fails to validate? But since the existing tests use unicast addrs, and those still validate, you've at least tested for no regressions on sane usage. > + > + > + > + > + > + > + > + name="pattern">[a-fA-F0-9][02468aAcCeE](:[a-fA-F0-9]{2}){5} > + > + > + > + > + name="pattern">[a-fA-F0-9][13579bBdDfF](:[a-fA-F0-9]{2}){5} > + > + > > > - ([a-fA-F0-9]{2}:){5}[a-fA-F0-9]{2} > + [a-fA-F0-9]{2}(:[a-fA-F0-9]{2}){5} Changed to match the earlier patterns. Nice patterns. ACK. If you do figure out how to add a negative test for expected validation failure, I'm okay if you submit it as a separate followup patch. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] python: make virDomainSetSchedulerParameters accpet integer argument
When sending a Python integer as an argument to PyLong_AsUnsignedLong or PyLong_AsUnsignedLongLong, the following error occurs SystemError: Objects/longobject.c:980: bad argument to internal function This error comes from the fact that PyLong_AsUnsignedLong and PyLong_AsUnsignedLongLong, unlike PyLong_AsLong or PyLong_AsLongLong, does not check to see if the number is a long or integer, but only a long. The regression is introduced by 9c8466daac19379c41be39ec8f18db36c9573c02 >>> dom.setSchedulerParameters({'cpu_shares': 1024}) 0 dom.schedulerParameters() {'cpu_shares': 1024L} >>> dom.setSchedulerParameters({'cpu_shares': 1024L}) 0 >>> dom.schedulerParameters() {'cpu_shares': 1024L} --- python/libvirt-override.c |9 - 1 files changed, 8 insertions(+), 1 deletions(-) diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 792cfa3..aec8f5a 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -233,7 +233,14 @@ setPyVirTypedParameter(PyObject *info, break; case VIR_TYPED_PARAM_ULLONG: { -unsigned long long ullong_val = PyLong_AsUnsignedLongLong(value); +unsigned long long ullong_val; +if (PyInt_Check(value)) +ullong_val = (unsigned long long)PyInt_AsLong(value); +else if (PyLong_Check(value)) +ullong_val = PyLong_AsUnsignedLongLong(value); +else +PyErr_SetString(PyExc_TypeError, "an integer is required"); + if ((ullong_val == -1) && PyErr_Occurred()) goto cleanup; temp->value.ul = ullong_val; -- 1.7.7.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: return immediately on error in dhcp host element
On 03/19/2012 11:32 AM, Laine Stump wrote: > If and error was encountered parsing a dhcp host entry mac address or > name, parsing would continue and log a less descriptive error that > might make it more difficult to notice the true nature of the problem. > > This patch returns immediately on logging the first error. > --- > src/conf/network_conf.c |2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c > index 0333141..0fa58f8 100644 > --- a/src/conf/network_conf.c > +++ b/src/conf/network_conf.c > @@ -430,6 +430,7 @@ virNetworkDHCPRangeDefParseXML(const char *networkName, >_("Cannot parse MAC address '%s' in > network '%s'"), >mac, networkName); > VIR_FREE(mac); > +return -1; > } > name = virXMLPropString(cur, "name"); > if ((name != NULL) && (!c_isalpha(name[0]))) { > @@ -437,6 +438,7 @@ virNetworkDHCPRangeDefParseXML(const char *networkName, >_("Cannot use name address '%s' in > network '%s'"), >name, networkName); > VIR_FREE(name); > +return -1; Memory leak - you just leaked mac. Needs a v2. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] python: add virDomainGetCPUStats python binding API
On 03/18/2012 04:41 AM, Guannan Ren wrote: >>> +if (totalbool == Py_False) { >> Per other code in libvirt-override.c, you can't compare totalbool (type >> PyObject) with Py_False, at least not on all compilers. You need >> something like this instead: >> >> /* Hack - Python's definition of Py_True breaks strict >> * aliasing rules, so can't directly compare >> */ >> if (PyBool_Check(value)) { >> PyObject *hacktrue = PyBool_FromLong(1); >> temp->value.b = hacktrue == value ? 1 : 0; >> Py_DECREF(hacktrue); > > Yes, it did report warning in compiling as follows due to the case > from PyIntObject* to PyObject* > warning :dereferencing type-punned pointer might break > strict-aliasing rules [-Wstrict-aliasing] And that would trip up a -Werror compilation, so I'm glad to see you changed it in v3. > > GCC command line to reproduce the error: > gcc -Wstrict-aliasing=1 -O2 cpythonexample.c > > Actually PyObject_IsTrue() is a more light-weight approach to do > the checking instead of > creating a intermediate PyObject * for the compare. Is PyObject_IsTrue() available in the version of python present on RHEL 5? If so, I'd be in favor of a followup cleanup patch that removes all our hacks in favor of the python glue code that does the same thing. And even if not, we should write a decent wrapper in our own typewrappers.c, so that the rest of our code doesn't have to look so ugly with so much copy-and-paste. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] conf: forbid use of multicast mac addresses
A few times libvirt users manually setting mac addresses have complained of networking failure that ends up being due to a multicast mac address being used for a guest interface. This patch prevents that by loggin an error and failing if a multicast mac address is encountered in each of the three following cases: 1) domain xml mac address. 2) network xml bridge mac address. 3) network xml dhcp/host mac address. There are several other places where a mac address cna be input that aren't controlled in this manner because failure to do so has no consequences (e.g., if the address will be used to search through existing interfaces for a match). The RNG has been updated to add multiMacAddr and uniMacAddr along with the existing macAddr, and macAddr was switched to uniMacAddr where appropriate. --- docs/schemas/basictypes.rng | 17 - docs/schemas/domaincommon.rng |6 +++--- docs/schemas/network.rng |4 ++-- src/conf/domain_conf.c|8 +++- src/conf/network_conf.c | 33 - src/libvirt_private.syms |2 ++ src/util/virmacaddr.c | 13 + src/util/virmacaddr.h |3 ++- 8 files changed, 69 insertions(+), 17 deletions(-) diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 7d7911d..9dbda4a 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -55,9 +55,24 @@ + + + + + + + + [a-fA-F0-9][02468aAcCeE](:[a-fA-F0-9]{2}){5} + + + + + [a-fA-F0-9][13579bBdDfF](:[a-fA-F0-9]{2}){5} + + - ([a-fA-F0-9]{2}:){5}[a-fA-F0-9]{2} + [a-fA-F0-9]{2}(:[a-fA-F0-9]{2}){5} diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 5b3e5fa..f629691 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1391,7 +1391,7 @@ - + @@ -1417,7 +1417,7 @@ - + @@ -1498,7 +1498,7 @@ - + diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 6e1002f..2ae879e 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -57,7 +57,7 @@ - + @@ -218,7 +218,7 @@ - + diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e6d0f4b..d5def1c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4416,11 +4416,17 @@ virDomainNetDefParseXML(virCapsPtr caps, if (macaddr) { if (virMacAddrParse((const char *)macaddr, def->mac) < 0) { -virDomainReportError(VIR_ERR_INTERNAL_ERROR, +virDomainReportError(VIR_ERR_XML_ERROR, _("unable to parse mac address '%s'"), (const char *)macaddr); goto error; } +if (virMacAddrIsMulticast(def->mac)) { +virDomainReportError(VIR_ERR_XML_ERROR, + _("expected unicast mac address, found multicast '%s'"), + (const char *)macaddr); +goto error; +} } else { virCapabilitiesGenerateMac(caps, def->mac); } diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 0fa58f8..87796b5 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -419,22 +419,30 @@ virNetworkDHCPRangeDefParseXML(const char *networkName, def->nranges++; } else if (cur->type == XML_ELEMENT_NODE && xmlStrEqual(cur->name, BAD_CAST "host")) { -char *mac, *name, *ip; +char *mac = NULL, *name = NULL, *ip; unsigned char addr[6]; virSocketAddr inaddr; mac = virXMLPropString(cur, "mac"); -if ((mac != NULL) && -(virMacAddrParse(mac, &addr[0]) != 0)) { -virNetworkReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot parse MAC address '%s' in network '%s'"), - mac, networkName); -VIR_FREE(mac); -return -1; +if (mac != NULL) { +if (virMacAddrParse(mac, &addr[0]) < 0) { +virNetworkReportError(VIR_ERR_XML_ERROR, + _("Cannot parse MAC address '%s' in network '%s'"), + mac, networkName); +
[libvirt] [PATCH] conf: return immediately on error in dhcp host element
If and error was encountered parsing a dhcp host entry mac address or name, parsing would continue and log a less descriptive error that might make it more difficult to notice the true nature of the problem. This patch returns immediately on logging the first error. --- src/conf/network_conf.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 0333141..0fa58f8 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -430,6 +430,7 @@ virNetworkDHCPRangeDefParseXML(const char *networkName, _("Cannot parse MAC address '%s' in network '%s'"), mac, networkName); VIR_FREE(mac); +return -1; } name = virXMLPropString(cur, "name"); if ((name != NULL) && (!c_isalpha(name[0]))) { @@ -437,6 +438,7 @@ virNetworkDHCPRangeDefParseXML(const char *networkName, _("Cannot use name address '%s' in network '%s'"), name, networkName); VIR_FREE(name); +return -1; } /* * You need at least one MAC address or one host name -- 1.7.7.6 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] snapshot: make quiesce a bit safer
On 03/19/2012 04:04 AM, Michal Privoznik wrote: > On 16.03.2012 21:49, Eric Blake wrote: >> If a guest is paused, we were silently ignoring the quiesce flag, >> which results in unclean snapshots, contrary to the intent of the >> flag. Since we can't quiesce without guest agent support, we should >> instead fail if the guest is not running. >> >> Meanwhile, if we attempt a quiesce command, but the guest agent >> doesn't respond, and we time out, we may have left the command >> pending on the guest's queue, and when the guest resumes parsing >> commands, it will freeze even though our command is no longer >> around to issue a thaw. To be safe, we must _always_ pair every >> quiesce call with a counterpart thaw, even if the quiesce call >> failed due to a timeout, so that if a guest wakes up and starts >> processing a command backlog, it will not get stuck in a frozen >> state. >> >> * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive): >> Always issue thaw after a quiesce, even if quiesce failed. >> (qemuDomainSnapshotFSThaw): Add a parameter. >> --- >> >> See also: https://bugzilla.redhat.com/show_bug.cgi?id=804210 >> https://www.redhat.com/archives/libvir-list/2012-March/msg00708.html >> >> src/qemu/qemu_driver.c | 51 >> +-- >> 1 files changed, 36 insertions(+), 15 deletions(-) >> > > ACK this and the follow up patch as well. Thanks; pushed. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Cpu mapping cleanup
On 03/19/2012 06:29 AM, Martin Kletzander wrote: > Using inheritance, this patch cleans up the cpu_map.xml file and also > sorts all CPU features according to the feature and registry > values. Model features are sorted the same way as foeatures in the s/foeatures/features/ > specification. > Also few models that are related were organized together and parts of > the XML are marked with comments > --- > src/cpu/cpu_map.xml | 455 +- > 1 files changed, 82 insertions(+), 373 deletions(-) The inheritance is definitely an improvement for reducing lines of code. The patch was a bit hard to read (splitting things into three patches: reordering options, reordering CPU models into groups, then taking advantage of inheritance; rather than doing all three actions in one patch, might have been easier), but seems correct after the few minutes I've spent on it, and isn't worth the time to respin the series just to split the patch. ACK. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3] python: Avoid memory leaks on libvirt_virNodeGetMemoryStats
On 02/17/2012 01:15 AM, a...@redhat.com wrote: > From: Alex Jia > > Detected by valgrind. Leaks are introduced in commit 17c7795. > > * python/libvirt-override.c (libvirt_virNodeGetMemoryStats): fix memory leaks > and improve codes return value. > > For details, please see the following link: > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=770944 > > Signed-off-by: Alex Jia > --- > python/libvirt-override.c | 46 ++-- > 1 files changed, 31 insertions(+), 15 deletions(-) > > > LIBVIRT_BEGIN_ALLOW_THREADS; > c_retval = virNodeGetMemoryStats(conn, cellNum, NULL, &nparams, flags); > LIBVIRT_END_ALLOW_THREADS; > if (c_retval < 0) > -return VIR_PY_NONE; > +return ret; Wrong. At this point, ret is still NULL, but we really do want to return VIR_PY_NONE (we have a valid libvirt error, and not a python exception). > > if (nparams) { > if (VIR_ALLOC_N(stats, nparams) < 0) > -return VIR_PY_NONE; > +return PyErr_NoMemory(); Correct (here we have a python exception, not a libvirt error). > > LIBVIRT_BEGIN_ALLOW_THREADS; > c_retval = virNodeGetMemoryStats(conn, cellNum, stats, &nparams, > flags); > LIBVIRT_END_ALLOW_THREADS; > -if (c_retval < 0) { > -VIR_FREE(stats); > -return VIR_PY_NONE; > -} > -} > -if (!(ret = PyDict_New())) { > -VIR_FREE(stats); > -return VIR_PY_NONE; > +if (c_retval < 0) > +goto error; Wrong - here's another place where we want to return VIR_PY_NONE. > } > + > +if (!(ret = PyDict_New())) > +goto error; Good - here we have a python exception, and ret is still NULL. > + > for (i = 0; i < nparams; i++) { > -PyDict_SetItem(ret, > - libvirt_constcharPtrWrap(stats[i].field), > - libvirt_ulonglongWrap(stats[i].value)); > +key = libvirt_constcharPtrWrap(stats[i].field); > +val = libvirt_ulonglongWrap(stats[i].value); > + > +if (!key || !val) > +goto error; Hmm. Here, ret is allocated, so we mistakenly end up returning a python object when we really want to return NULL to cover the python error we just encountered. > + > +if (PyDict_SetItem(ret, key, val) < 0) { > +Py_DECREF(ret); > +goto error; Even worse - we are allowing ret to be garbage collected, but still returning its address. If PyDict_SetItem fails, you want to return NULL, not the just-allocated dictionary object. > +} > + > +Py_DECREF(key); > +Py_DECREF(val); > } > > VIR_FREE(stats); > return ret; > + > +error: > +VIR_FREE(stats); > +Py_XDECREF(key); > +Py_XDECREF(val); > +return ret; I think we still need a v4 to fix things properly. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/5] qemu: Add connection close callbacks
Add support for registering arbitrary callback to be called for a domain when a connection gets closed. --- src/qemu/qemu_conf.c | 172 src/qemu/qemu_conf.h | 27 src/qemu/qemu_driver.c |5 ++ 3 files changed, 204 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index a709cbf..88a04bc 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -56,6 +56,11 @@ #define VIR_FROM_THIS VIR_FROM_QEMU +struct _qemuDriverCloseDef { +virConnectPtr conn; +qemuDriverCloseCallback cb; +}; + void qemuDriverLock(struct qemud_driver *driver) { virMutexLock(&driver->lock); @@ -490,3 +495,170 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, virConfFree (conf); return 0; } + +static void +qemuDriverCloseCallbackFree(void *payload, +const void *name ATTRIBUTE_UNUSED) +{ +VIR_FREE(payload); +} + +int +qemuDriverCloseCallbackInit(struct qemud_driver *driver) +{ +driver->closeCallbacks = virHashCreate(5, qemuDriverCloseCallbackFree); +if (!driver->closeCallbacks) +return -1; + +return 0; +} + +void +qemuDriverCloseCallbackShutdown(struct qemud_driver *driver) +{ +virHashFree(driver->closeCallbacks); +} + +int +qemuDriverCloseCallbackSet(struct qemud_driver *driver, + virDomainObjPtr vm, + virConnectPtr conn, + qemuDriverCloseCallback cb) +{ +char uuidstr[VIR_UUID_STRING_BUFLEN]; +qemuDriverCloseDefPtr closeDef; + +virUUIDFormat(vm->def->uuid, uuidstr); +VIR_DEBUG("vm=%s, uuid=%s, conn=%p, cb=%p", + vm->def->name, uuidstr, conn, cb); + +closeDef = virHashLookup(driver->closeCallbacks, uuidstr); +if (closeDef) { +if (closeDef->conn != conn) { +qemuReportError(VIR_ERR_INTERNAL_ERROR, +_("Close callback for domain %s already registered" + " with another connection %p"), +vm->def->name, closeDef->conn); +return -1; +} +if (closeDef->cb && closeDef->cb != cb) { +qemuReportError(VIR_ERR_INTERNAL_ERROR, +_("Another close callback is already defined for" + " domain %s"), vm->def->name); +return -1; +} + +closeDef->cb = cb; +} else { +if (VIR_ALLOC(closeDef) < 0) { +virReportOOMError(); +return -1; +} + +closeDef->conn = conn; +closeDef->cb = cb; +if (virHashAddEntry(driver->closeCallbacks, uuidstr, closeDef) < 0) { +VIR_FREE(closeDef); +return -1; +} +} +return 0; +} + +int +qemuDriverCloseCallbackUnset(struct qemud_driver *driver, + virDomainObjPtr vm, + qemuDriverCloseCallback cb) +{ +char uuidstr[VIR_UUID_STRING_BUFLEN]; +qemuDriverCloseDefPtr closeDef; + +virUUIDFormat(vm->def->uuid, uuidstr); +VIR_DEBUG("vm=%s, uuid=%s, cb=%p", + vm->def->name, uuidstr, cb); + +closeDef = virHashLookup(driver->closeCallbacks, uuidstr); +if (!closeDef) +return -1; + +if (closeDef->cb && closeDef->cb != cb) { +qemuReportError(VIR_ERR_INTERNAL_ERROR, +_("Trying to remove mismatching close callback for" + " domain %s"), vm->def->name); +return -1; +} + +return virHashRemoveEntry(driver->closeCallbacks, uuidstr); +} + +qemuDriverCloseCallback +qemuDriverCloseCallbackGet(struct qemud_driver *driver, + virDomainObjPtr vm, + virConnectPtr conn) +{ +char uuidstr[VIR_UUID_STRING_BUFLEN]; +qemuDriverCloseDefPtr closeDef; +qemuDriverCloseCallback cb = NULL; + +virUUIDFormat(vm->def->uuid, uuidstr); +VIR_DEBUG("vm=%s, uuid=%s, conn=%p", + vm->def->name, uuidstr, conn); + +closeDef = virHashLookup(driver->closeCallbacks, uuidstr); +if (closeDef && (!conn || closeDef->conn == conn)) +cb = closeDef->cb; + +VIR_DEBUG("cb=%p", cb); +return cb; +} + +struct qemuDriverCloseCallbackData { +struct qemud_driver *driver; +virConnectPtr conn; +}; + +static void +qemuDriverCloseCallbackRun(void *payload, + const void *name, + void *opaque) +{ +struct qemuDriverCloseCallbackData *data = opaque; +qemuDriverCloseDefPtr closeDef = payload; +const char *uuidstr = name; +unsigned char uuid[VIR_UUID_BUFLEN]; +virDomainObjPtr dom; + +VIR_DEBUG("conn=%p, thisconn=%p, uuid=%s, cb=%p", + closeDef->conn, data->conn, uuidstr, closeDef->cb); + +if (data->conn != closeDef->conn || !closeDef->cb) +return; + +if (virUUIDParse(uuidstr, uuid) < 0) { +
[libvirt] [PATCH 5/5] qemu: Avoid dangling migration-out job when client dies
When a client which started non-p2p migration dies in a bad time, the source libvirtd never clears the migration job and almost nothing can be done with the domain without restarting the daemon. This patch makes use of connection close callbacks and ensures that migration job is properly discarded when the client disconnects. --- src/qemu/qemu_driver.c|4 +++ src/qemu/qemu_migration.c | 66 + src/qemu/qemu_migration.h |4 +++ 3 files changed, 74 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3a5ef09..c2622d3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8838,6 +8838,9 @@ qemuDomainMigrateBegin3(virDomainPtr domain, * This prevents any other APIs being invoked while migration is taking * place. */ +if (qemuDriverCloseCallbackSet(driver, vm, domain->conn, + qemuMigrationCleanup) < 0) +goto endjob; if (qemuMigrationJobContinue(vm) == 0) { vm = NULL; qemuReportError(VIR_ERR_OPERATION_FAILED, @@ -9069,6 +9072,7 @@ qemuDomainMigrateConfirm3(virDomainPtr domain, phase = QEMU_MIGRATION_PHASE_CONFIRM3; qemuMigrationJobStartPhase(driver, vm, phase); +qemuDriverCloseCallbackUnset(driver, vm, qemuMigrationCleanup); ret = qemuMigrationConfirm(driver, domain->conn, vm, cookiein, cookieinlen, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 4eb3bf4..e02ba86 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1037,6 +1037,67 @@ qemuDomainMigrateGraphicsRelocate(struct qemud_driver *driver, } +/* This is called for outgoing non-p2p migrations when a connection to the + * client which initiated the migration was closed and we are waiting for it + * to follow up with the next phase, that is, in between + * virDomainMigrateBegin3 and qemuDomainMigratePerform3 or + * qemuDomainMigratePerform3 and qemuDomainMigrateConfirm3. + */ +virDomainObjPtr +qemuMigrationCleanup(struct qemud_driver *driver, + virDomainObjPtr vm, + virConnectPtr conn) +{ +qemuDomainObjPrivatePtr priv = vm->privateData; + +VIR_DEBUG("vm=%s, conn=%p, asyncJob=%s, phase=%s", + vm->def->name, conn, + qemuDomainAsyncJobTypeToString(priv->job.asyncJob), + qemuDomainAsyncJobPhaseToString(priv->job.asyncJob, + priv->job.phase)); + +if (!qemuMigrationJobIsActive(vm, QEMU_ASYNC_JOB_MIGRATION_OUT)) +goto cleanup; + +VIR_DEBUG("The connection which started outgoing migration of domain %s" + " was closed; cancelling the migration", + vm->def->name); + +switch ((enum qemuMigrationJobPhase) priv->job.phase) { +case QEMU_MIGRATION_PHASE_BEGIN3: +/* just forget we were about to migrate */ +qemuDomainObjDiscardAsyncJob(driver, vm); +break; + +case QEMU_MIGRATION_PHASE_PERFORM3_DONE: +VIR_WARN("Migration of domain %s finished but we don't know if the" + " domain was successfully started on destination or not", + vm->def->name); +/* clear the job and let higher levels decide what to do */ +qemuDomainObjDiscardAsyncJob(driver, vm); +break; + +case QEMU_MIGRATION_PHASE_PERFORM3: +/* cannot be seen without an active migration API; unreachable */ +case QEMU_MIGRATION_PHASE_CONFIRM3: +case QEMU_MIGRATION_PHASE_CONFIRM3_CANCELLED: +/* all done; unreachable */ +case QEMU_MIGRATION_PHASE_PREPARE: +case QEMU_MIGRATION_PHASE_FINISH2: +case QEMU_MIGRATION_PHASE_FINISH3: +/* incoming migration; unreachable */ +case QEMU_MIGRATION_PHASE_PERFORM2: +/* single phase outgoing migration; unreachable */ +case QEMU_MIGRATION_PHASE_NONE: +case QEMU_MIGRATION_PHASE_LAST: +/* unreachable */ +; +} + +cleanup: +return vm; +} + /* The caller is supposed to lock the vm and start a migration job. */ char *qemuMigrationBegin(struct qemud_driver *driver, virDomainObjPtr vm, @@ -2547,6 +2608,7 @@ qemuMigrationPerformPhase(struct qemud_driver *driver, } qemuMigrationJobStartPhase(driver, vm, QEMU_MIGRATION_PHASE_PERFORM3); +qemuDriverCloseCallbackUnset(driver, vm, qemuMigrationCleanup); resume = virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING; ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen, @@ -2577,6 +2639,10 @@ qemuMigrationPerformPhase(struct qemud_driver *driver, qemuMigrationJobSetPhase(driver, vm, QEMU_MIGRATION_PHASE_PERFORM3_DONE); +if (qemuDriverCloseCallbackSet(driver, vm, conn, + qemuMigrationCleanup) < 0) +goto endjob; + endjob: if (ret < 0)
[libvirt] [PATCH 0/5] Make migration more robust when client dies
Libvirt daemon was not completely ready to lose a connection to the client (may even be source libvirtd) which is controling the migration. We might end up with dangling migration jobs on both source and destination daemons. More details about the scenarios this might happen can be found in patches 2/5 and 5/5 which fix the issue using infrastructure set up by the other patches in this series. Jiri Denemark (5): qemu: Add support for domain cleanup callbacks qemu: Avoid dangling migration-in job on shutoff domains qemu: Add connection close callbacks qemu: Make autodestroy utilize connection close callbacks qemu: Avoid dangling migration-out job when client dies src/qemu/qemu_conf.c | 172 + src/qemu/qemu_conf.h | 30 +++- src/qemu/qemu_domain.c| 75 +++- src/qemu/qemu_domain.h| 17 + src/qemu/qemu_driver.c| 10 ++- src/qemu/qemu_migration.c | 88 +++ src/qemu/qemu_migration.h |4 + src/qemu/qemu_process.c | 109 +++-- 8 files changed, 413 insertions(+), 92 deletions(-) -- 1.7.8.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/5] qemu: Add support for domain cleanup callbacks
Add support for registering cleanup callbacks to be run when a domain transitions to shutoff state. --- src/qemu/qemu_domain.c | 73 +++ src/qemu/qemu_domain.h | 15 + src/qemu/qemu_process.c |2 + 3 files changed, 90 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 625c595..a9469cf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -232,6 +232,7 @@ static void qemuDomainObjPrivateFree(void *data) VIR_ERROR(_("Unexpected QEMU agent still active during domain deletion")); qemuAgentClose(priv->agent); } +VIR_FREE(priv->cleanupCallbacks); VIR_FREE(priv); } @@ -1769,3 +1770,75 @@ qemuDomainCheckDiskPresence(struct qemud_driver *driver, cleanup: return ret; } + +/* + * The vm must be locked when any of the following cleanup functions is + * called. + */ +int +qemuDomainCleanupAdd(virDomainObjPtr vm, + qemuDomainCleanupCallback cb) +{ +qemuDomainObjPrivatePtr priv = vm->privateData; +int i; + +VIR_DEBUG("vm=%s, cb=%p", vm->def->name, cb); + +for (i = 0; i < priv->ncleanupCallbacks; i++) { +if (priv->cleanupCallbacks[i] == cb) +return 0; +} + +if (VIR_RESIZE_N(priv->cleanupCallbacks, + priv->ncleanupCallbacks_max, + priv->ncleanupCallbacks, 1) < 0) { +virReportOOMError(); +return -1; +} + +priv->cleanupCallbacks[priv->ncleanupCallbacks++] = cb; +return 0; +} + +void +qemuDomainCleanupRemove(virDomainObjPtr vm, +qemuDomainCleanupCallback cb) +{ +qemuDomainObjPrivatePtr priv = vm->privateData; +int i; + +VIR_DEBUG("vm=%s, cb=%p", vm->def->name, cb); + +for (i = 0; i < priv->ncleanupCallbacks; i++) { +if (priv->cleanupCallbacks[i] == cb) { +memmove(priv->cleanupCallbacks + i, +priv->cleanupCallbacks + i + 1, +priv->ncleanupCallbacks - i - 1); +priv->ncleanupCallbacks--; +} +} + +VIR_SHRINK_N(priv->cleanupCallbacks, + priv->ncleanupCallbacks_max, + priv->ncleanupCallbacks_max - priv->ncleanupCallbacks); +} + +void +qemuDomainCleanupRun(struct qemud_driver *driver, + virDomainObjPtr vm) +{ +qemuDomainObjPrivatePtr priv = vm->privateData; +int i; + +VIR_DEBUG("driver=%p, vm=%s", driver, vm->def->name); + +/* run cleanup callbacks in reverse order */ +for (i = priv->ncleanupCallbacks - 1; i >= 0; i--) { +if (priv->cleanupCallbacks[i]) +priv->cleanupCallbacks[i](driver, vm); +} + +VIR_FREE(priv->cleanupCallbacks); +priv->ncleanupCallbacks = 0; +priv->ncleanupCallbacks_max = 0; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index f8e943f..af83c0e 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -94,6 +94,9 @@ struct qemuDomainJobObj { typedef struct _qemuDomainPCIAddressSet qemuDomainPCIAddressSet; typedef qemuDomainPCIAddressSet *qemuDomainPCIAddressSetPtr; +typedef void (*qemuDomainCleanupCallback)(struct qemud_driver *driver, + virDomainObjPtr vm); + typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate; typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr; struct _qemuDomainObjPrivate { @@ -130,6 +133,10 @@ struct _qemuDomainObjPrivate { char *origname; virConsolesPtr cons; + +qemuDomainCleanupCallback *cleanupCallbacks; +size_t ncleanupCallbacks; +size_t ncleanupCallbacks_max; }; struct qemuDomainWatchdogEvent @@ -307,4 +314,12 @@ bool qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, int qemuDomainCheckDiskPresence(struct qemud_driver *driver, virDomainObjPtr vm, bool start_with_state); + +int qemuDomainCleanupAdd(virDomainObjPtr vm, + qemuDomainCleanupCallback cb); +void qemuDomainCleanupRemove(virDomainObjPtr vm, + qemuDomainCleanupCallback cb); +void qemuDomainCleanupRun(struct qemud_driver *driver, + virDomainObjPtr vm); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0af3751..1945864 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3803,6 +3803,8 @@ void qemuProcessStop(struct qemud_driver *driver, /* shut it off for sure */ ignore_value(qemuProcessKill(driver, vm, VIR_QEMU_PROCESS_KILL_FORCE)); +qemuDomainCleanupRun(driver, vm); + /* Stop autodestroy in case guest is restarted */ qemuProcessAutoDestroyRemove(driver, vm); -- 1.7.8.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/5] qemu: Make autodestroy utilize connection close callbacks
--- src/qemu/qemu_conf.h|5 -- src/qemu/qemu_driver.c |5 -- src/qemu/qemu_process.c | 107 ++ 3 files changed, 24 insertions(+), 93 deletions(-) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index a22ce0c..3306014 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -141,11 +141,6 @@ struct qemud_driver { virLockManagerPluginPtr lockManager; -/* Mapping of 'char *uuidstr' -> virConnectPtr - * of guests which will be automatically killed - * when the virConnectPtr is closed*/ -virHashTablePtr autodestroy; - /* Mapping of 'char *uuidstr' -> qemuDriverCloseDefPtr of domains * which want a specific cleanup to be done when a connection is * closed. Such cleanup may be to automatically destroy the diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ce82535..3a5ef09 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -659,9 +659,6 @@ qemudStartup(int privileged) { qemu_driver->hugepage_path = mempath; } -if (qemuProcessAutoDestroyInit(qemu_driver) < 0) -goto error; - if (qemuDriverCloseCallbackInit(qemu_driver) < 0) goto error; @@ -803,7 +800,6 @@ qemudShutdown(void) { virSysinfoDefFree(qemu_driver->hostsysinfo); -qemuProcessAutoDestroyShutdown(qemu_driver); qemuDriverCloseCallbackShutdown(qemu_driver); VIR_FREE(qemu_driver->configDir); @@ -925,7 +921,6 @@ static int qemudClose(virConnectPtr conn) { qemuDriverLock(driver); virDomainEventStateDeregisterConn(conn, driver->domainEventState); -qemuProcessAutoDestroyRun(driver, conn); qemuDriverCloseCallbackRunAll(driver, conn); qemuDriverUnlock(driver); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1945864..8915a9a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4120,124 +4120,65 @@ cleanup: } -int qemuProcessAutoDestroyInit(struct qemud_driver *driver) +static virDomainObjPtr +qemuProcessAutoDestroy(struct qemud_driver *driver, + virDomainObjPtr dom, + virConnectPtr conn) { -if (!(driver->autodestroy = virHashCreate(5, NULL))) -return -1; - -return 0; -} - -struct qemuProcessAutoDestroyData { -struct qemud_driver *driver; -virConnectPtr conn; -}; - -static void qemuProcessAutoDestroyDom(void *payload, - const void *name, - void *opaque) -{ -struct qemuProcessAutoDestroyData *data = opaque; -virConnectPtr conn = payload; -const char *uuidstr = name; -unsigned char uuid[VIR_UUID_BUFLEN]; -virDomainObjPtr dom; -qemuDomainObjPrivatePtr priv; +qemuDomainObjPrivatePtr priv = dom->privateData; virDomainEventPtr event = NULL; -VIR_DEBUG("conn=%p uuidstr=%s thisconn=%p", conn, uuidstr, data->conn); +VIR_DEBUG("vm=%s, conn=%p", dom->def->name, conn); -if (data->conn != conn) -return; - -if (virUUIDParse(uuidstr, uuid) < 0) { -VIR_WARN("Failed to parse %s", uuidstr); -return; -} - -if (!(dom = virDomainFindByUUID(&data->driver->domains, -uuid))) { -VIR_DEBUG("No domain object to kill"); -return; -} - -priv = dom->privateData; if (priv->job.asyncJob) { VIR_DEBUG("vm=%s has long-term job active, cancelling", dom->def->name); -qemuDomainObjDiscardAsyncJob(data->driver, dom); +qemuDomainObjDiscardAsyncJob(driver, dom); } -if (qemuDomainObjBeginJobWithDriver(data->driver, dom, +if (qemuDomainObjBeginJobWithDriver(driver, dom, QEMU_JOB_DESTROY) < 0) goto cleanup; VIR_DEBUG("Killing domain"); -qemuProcessStop(data->driver, dom, 1, VIR_DOMAIN_SHUTOFF_DESTROYED); +qemuProcessStop(driver, dom, 1, VIR_DOMAIN_SHUTOFF_DESTROYED); virDomainAuditStop(dom, "destroyed"); event = virDomainEventNewFromObj(dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); -if (qemuDomainObjEndJob(data->driver, dom) == 0) + +if (qemuDomainObjEndJob(driver, dom) == 0) dom = NULL; if (dom && !dom->persistent) -qemuDomainRemoveInactive(data->driver, dom); - -cleanup: -if (dom) -virDomainObjUnlock(dom); +qemuDomainRemoveInactive(driver, dom); if (event) -qemuDomainEventQueue(data->driver, event); -virHashRemoveEntry(data->driver->autodestroy, uuidstr); -} - -/* - * Precondition: driver is locked - */ -void qemuProcessAutoDestroyRun(struct qemud_driver *driver, virConnectPtr conn) -{ -struct qemuProcessAutoDestroyData data = { -driver, conn -}; -VIR_DEBUG("conn=%p", conn); -virHashFo
[libvirt] [PATCH 2/5] qemu: Avoid dangling migration-in job on shutoff domains
Destination daemon should not rely on the client or source daemon (depending on the type of migration) to call Finish when migration fails, because the client may crash before it can do so. The domain prepared for incoming migration is set to be destroyed (and migration job cleaned up) when connection with the client closes but this is not enough. If the associated qemu process crashes after Prepare step and the domain is cleaned up before the connection gets closed, autodestroy is not called for the domain and migration jobs remains set. In case the domain is defined on destination host (i.e., it is not completely removed once destroyed) we keep the job set for ever. To fix this, we register a cleanup callback which is responsible to clean migration-in job when a domain dies anywhere between Prepare and Finish steps. Note that we can't blindly clean any job when spotting EOF on monitor since normally an API is running at that time. --- src/qemu/qemu_domain.c|2 -- src/qemu/qemu_domain.h|2 ++ src/qemu/qemu_migration.c | 22 ++ 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a9469cf..41ffd6a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -51,7 +51,6 @@ (VIR_DOMAIN_XML_SECURE |\ VIR_DOMAIN_XML_UPDATE_CPU) -VIR_ENUM_DECL(qemuDomainJob) VIR_ENUM_IMPL(qemuDomainJob, QEMU_JOB_LAST, "none", "query", @@ -64,7 +63,6 @@ VIR_ENUM_IMPL(qemuDomainJob, QEMU_JOB_LAST, "async nested", ); -VIR_ENUM_DECL(qemuDomainAsyncJob) VIR_ENUM_IMPL(qemuDomainAsyncJob, QEMU_ASYNC_JOB_LAST, "none", "migration out", diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index af83c0e..d79ff1d 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -64,6 +64,7 @@ enum qemuDomainJob { QEMU_JOB_LAST }; +VIR_ENUM_DECL(qemuDomainJob) /* Async job consists of a series of jobs that may change state. Independent * jobs that do not change state (and possibly others if explicitly allowed by @@ -78,6 +79,7 @@ enum qemuDomainAsyncJob { QEMU_ASYNC_JOB_LAST }; +VIR_ENUM_DECL(qemuDomainAsyncJob) struct qemuDomainJobObj { virCond cond; /* Use to coordinate jobs */ diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 81b2d5b..4eb3bf4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1107,6 +1107,23 @@ cleanup: /* Prepare is the first step, and it runs on the destination host. */ +static void +qemuMigrationPrepareCleanup(struct qemud_driver *driver, +virDomainObjPtr vm) +{ +qemuDomainObjPrivatePtr priv = vm->privateData; + +VIR_DEBUG("driver=%p, vm=%s, job=%s, asyncJob=%s", + driver, + vm->def->name, + qemuDomainJobTypeToString(priv->job.active), + qemuDomainAsyncJobTypeToString(priv->job.asyncJob)); + +if (!qemuMigrationJobIsActive(vm, QEMU_ASYNC_JOB_MIGRATION_IN)) +return; +qemuDomainObjDiscardAsyncJob(driver, vm); +} + static int qemuMigrationPrepareAny(struct qemud_driver *driver, virConnectPtr dconn, @@ -1264,6 +1281,9 @@ qemuMigrationPrepareAny(struct qemud_driver *driver, VIR_WARN("Unable to encode migration cookie"); } +if (qemuDomainCleanupAdd(vm, qemuMigrationPrepareCleanup) < 0) +goto endjob; + virDomainAuditStart(vm, "migrated", true); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, @@ -2703,6 +2723,8 @@ qemuMigrationFinish(struct qemud_driver *driver, v3proto ? QEMU_MIGRATION_PHASE_FINISH3 : QEMU_MIGRATION_PHASE_FINISH2); +qemuDomainCleanupRemove(vm, qemuMigrationPrepareCleanup); + if (flags & VIR_MIGRATE_PERSIST_DEST) cookie_flags |= QEMU_MIGRATION_COOKIE_PERSISTENT; -- 1.7.8.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3] python: Avoid memory leaks on libvirt_virNodeGetMemoryStats
On 17.02.2012 09:15, a...@redhat.com wrote: > From: Alex Jia > > Detected by valgrind. Leaks are introduced in commit 17c7795. > > * python/libvirt-override.c (libvirt_virNodeGetMemoryStats): fix memory leaks > and improve codes return value. > > For details, please see the following link: > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=770944 > > Signed-off-by: Alex Jia > --- > python/libvirt-override.c | 46 ++-- > 1 files changed, 31 insertions(+), 15 deletions(-) > ACK Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fixed NULL pointer check
On Mon, Mar 19, 2012 at 09:43:26AM -0600, Eric Blake wrote: > On 03/19/2012 06:43 AM, Daniel P. Berrange wrote: > >>> That is a historical remant. Do feel free to send another followup patch > >>> to > >>> cleanup all the cases of 'return(NULL)' as well. > >>> > >>> Daniel > >> > >> Did you mean in that file or globally? Because I just tried the first > >> thing that came to my mind and look at the output: > >> > >> libvirt $ find . -name '*.[ch]' -type f -exec grep -nH -e \ > >> 'return(.*);' {} + | wc -l > >> 852 > > Yes, it is a rather big cleanup, which is all the more reason why we'd > want to ensure that 'make syntax-check' prevents it from recurring. > > >> > >> Not that I wouldn't do it, it just seem as a pretty big change. On the > >> other hand, I don't see the point in changing that in only one file. > > > > IMHO, we should do this to make our code consistent, and ideally add a > > syntax-check rule to prevent them coming back. Anyone disagree ? > > I'm in favor of such a cleanup. There are still a few places where > return needs parenthesis; for example, I'm fond of the style: > > return (big_long_conditional ? > option_1 : > option_2); Yes, that usage scenario is fine. > since the open '(' lets the rest of the code indent nicely when using > default emacs indentation. But it's still pretty easy to recognize the > difference between complex returns and the real offenders. I think the > number of false positives and false negatives is pretty near zero if you > boil it down to detecting uses where there are no spaces between the '(' > and ')'. Thus, for cfg.mk, I suggest a pattern something like: > > sc_prohibit_return_as_function: > @prohibit='\ halt='avoid extra () with return statements' \ > $(_sc_search_regexp) Looks good. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fixed NULL pointer check
On 03/19/2012 04:43 PM, Eric Blake wrote: > On 03/19/2012 06:43 AM, Daniel P. Berrange wrote: [...] > since the open '(' lets the rest of the code indent nicely when using > default emacs indentation. But it's still pretty easy to recognize the > difference between complex returns and the real offenders. I think the > number of false positives and false negatives is pretty near zero if you > boil it down to detecting uses where there are no spaces between the '(' > and ')'. Thus, for cfg.mk, I suggest a pattern something like: > > sc_prohibit_return_as_function: > @prohibit='\ halt='avoid extra () with return statements' \ > $(_sc_search_regexp) I agree with the first part. There are some places that should be kept as is. However, the '\https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fixed NULL pointer check
On 03/19/2012 06:43 AM, Daniel P. Berrange wrote: >>> That is a historical remant. Do feel free to send another followup patch to >>> cleanup all the cases of 'return(NULL)' as well. >>> >>> Daniel >> >> Did you mean in that file or globally? Because I just tried the first >> thing that came to my mind and look at the output: >> >> libvirt $ find . -name '*.[ch]' -type f -exec grep -nH -e \ >> 'return(.*);' {} + | wc -l >> 852 Yes, it is a rather big cleanup, which is all the more reason why we'd want to ensure that 'make syntax-check' prevents it from recurring. >> >> Not that I wouldn't do it, it just seem as a pretty big change. On the >> other hand, I don't see the point in changing that in only one file. > > IMHO, we should do this to make our code consistent, and ideally add a > syntax-check rule to prevent them coming back. Anyone disagree ? I'm in favor of such a cleanup. There are still a few places where return needs parenthesis; for example, I'm fond of the style: return (big_long_conditional ? option_1 : option_2); since the open '(' lets the rest of the code indent nicely when using default emacs indentation. But it's still pretty easy to recognize the difference between complex returns and the real offenders. I think the number of false positives and false negatives is pretty near zero if you boil it down to detecting uses where there are no spaces between the '(' and ')'. Thus, for cfg.mk, I suggest a pattern something like: sc_prohibit_return_as_function: @prohibit='\http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 4/3] snapshot: make offline qemu snapshots atomic
On 03/17/2012 05:33 PM, Eric Blake wrote: > Offline internal snapshots can be rolled back with just a little > bit of refactoring, meaning that we are now automatically atomic. > > * src/qemu/qemu_domain.c (qemuDomainSnapshotForEachQcow2): Move > guts... > (qemuDomainSnapshotForEachQcow2Raw): ...to new helper, to allow > rollbacks. > * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Offline > snapshots are now atomic. > --- You rollback changes to disks if other disks are not snapshotable, but later on, when the actual qemu-img command is run and fails the rollback is not performed. I's suggest squashing in: diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a845480..8dde3c9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1512,70 +1512,75 @@ qemuDomainSnapshotForEachQcow2Raw(struct qemud_driver *driver, for (i = 0; i < ndisks; i++) { /* FIXME: we also need to handle LVM here */ if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { if (!def->disks[i]->driverType || STRNEQ(def->disks[i]->driverType, "qcow2")) { if (try_all) { /* Continue on even in the face of error, since other * disks in this VM may have the same snapshot name. */ VIR_WARN("skipping snapshot action on %s", def->disks[i]->dst); skipped = true; continue; } else if (STREQ(op, "-c") && i) { /* We must roll back partial creation by deleting * all earlier snapshots. */ qemuDomainSnapshotForEachQcow2Raw(driver, def, name, "-d", false, i); } qemuReportError(VIR_ERR_OPERATION_INVALID, _("Disk device '%s' does not support" " snapshotting"), def->disks[i]->dst); return -1; } qemuimgarg[4] = def->disks[i]->src; if (virRun(qemuimgarg, NULL) < 0) { if (try_all) { VIR_WARN("skipping snapshot action on %s", def->disks[i]->dst); skipped = true; continue; +} else if (STREQ(op, "-c") && i) { +/* We must roll back partial creation by deleting + * all earlier snapshots. */ +qemuDomainSnapshotForEachQcow2Raw(driver, def, name, + "-d", false, i); } return -1; } } } return skipped ? 1 : 0; } If you don't add this change the result is following: Try to make a snapshot of a domain that has one of images missing: virsh # snapshot-create-as Bare2 test1 test --atomic error: internal error Child process (/usr/bin/qemu-img snapshot -c test1 /var/lib/libvirt/images/d2.qcow2) status unexpected: exit status 1 But the first image is still modified: # qemu-img snapshot -l d.qcow2 Snapshot list: IDTAG VM SIZEDATE VM CLOCK 1 test1 0 2012-03-19 14:26:50 00:00:00.000 Otherwise looks good. ACK with that suggested change. Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fixed NULL pointer check
On Mon, Mar 19, 2012 at 01:38:47PM +0100, Martin Kletzander wrote: > On 03/19/2012 11:32 AM, Daniel P. Berrange wrote: > > On Mon, Mar 19, 2012 at 08:51:24AM +0100, Martin Kletzander wrote: > >> On 03/19/2012 08:43 AM, Wen Congyang wrote: > >>> At 03/18/2012 08:29 AM, Martin Kletzander Wrote: > This patch fixes a NULL pointer check that was causing SegFault on > some specific configurations. > --- > src/util/conf.c |5 - > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/src/util/conf.c b/src/util/conf.c > index 8ad60e0..e76362c 100644 > --- a/src/util/conf.c > +++ b/src/util/conf.c > @@ -1,7 +1,7 @@ > /** > * conf.c: parser for a subset of the Python encoded Xen configuration > files > * > - * Copyright (C) 2006-2011 Red Hat, Inc. > + * Copyright (C) 2006-2012 Red Hat, Inc. > * > * See COPYING.LIB for the License of this software > * > @@ -836,6 +836,9 @@ virConfGetValue(virConfPtr conf, const char *setting) > { > virConfEntryPtr cur; > > +if (conf == NULL) > +return(NULL); > >>> > >>> Please use 'return NULL;' instead of 'return(NULL);' > >>> > >>> 'return(NULL);' is old style, and we donot use it now and later. > >>> > >> > >> It seems weird to me too, it's just that it's almost everywhere in this > >> file so that's why I wanted to keep the same look. > > > > > > That is a historical remant. Do feel free to send another followup patch to > > cleanup all the cases of 'return(NULL)' as well. > > > > Daniel > > Did you mean in that file or globally? Because I just tried the first > thing that came to my mind and look at the output: > > libvirt $ find . -name '*.[ch]' -type f -exec grep -nH -e \ > 'return(.*);' {} + | wc -l > 852 > > Not that I wouldn't do it, it just seem as a pretty big change. On the > other hand, I don't see the point in changing that in only one file. IMHO, we should do this to make our code consistent, and ideally add a syntax-check rule to prevent them coming back. Anyone disagree ? Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fixed NULL pointer check
On 03/19/2012 11:32 AM, Daniel P. Berrange wrote: > On Mon, Mar 19, 2012 at 08:51:24AM +0100, Martin Kletzander wrote: >> On 03/19/2012 08:43 AM, Wen Congyang wrote: >>> At 03/18/2012 08:29 AM, Martin Kletzander Wrote: This patch fixes a NULL pointer check that was causing SegFault on some specific configurations. --- src/util/conf.c |5 - 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/src/util/conf.c b/src/util/conf.c index 8ad60e0..e76362c 100644 --- a/src/util/conf.c +++ b/src/util/conf.c @@ -1,7 +1,7 @@ /** * conf.c: parser for a subset of the Python encoded Xen configuration files * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -836,6 +836,9 @@ virConfGetValue(virConfPtr conf, const char *setting) { virConfEntryPtr cur; +if (conf == NULL) +return(NULL); >>> >>> Please use 'return NULL;' instead of 'return(NULL);' >>> >>> 'return(NULL);' is old style, and we donot use it now and later. >>> >> >> It seems weird to me too, it's just that it's almost everywhere in this >> file so that's why I wanted to keep the same look. > > > That is a historical remant. Do feel free to send another followup patch to > cleanup all the cases of 'return(NULL)' as well. > > Daniel Did you mean in that file or globally? Because I just tried the first thing that came to my mind and look at the output: libvirt $ find . -name '*.[ch]' -type f -exec grep -nH -e \ 'return(.*);' {} + | wc -l 852 Not that I wouldn't do it, it just seem as a pretty big change. On the other hand, I don't see the point in changing that in only one file. Martin -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Cpu mapping cleanup
Using inheritance, this patch cleans up the cpu_map.xml file and also sorts all CPU features according to the feature and registry values. Model features are sorted the same way as foeatures in the specification. Also few models that are related were organized together and parts of the XML are marked with comments --- src/cpu/cpu_map.xml | 455 +- 1 files changed, 82 insertions(+), 373 deletions(-) diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index 6af9c40..9a89e2e 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -268,6 +268,7 @@ + @@ -302,482 +303,190 @@ + + + - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + - + - - - - - - - - - - - - - + + + - - - - - + + - - - - + - + + - - - - - - - - - - - - - + + - - - - + + + + + - - - - - + - - - - - - - - - + + + + - - - - - - - - - - + + + + + + + + + + + - - - - - - - - - - - - - - - - - - + - + - - - - - - - - - - - - - - - - - - - - - - - + - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + - - - - - - - - - - - - - - - - - - - - - - - - - - + - - - - - - - - - - - - - - - + + + - + + + - - - - - - - - - - - - - - - - - + - - - - - - + + + - + + - - - - - - - - - - - - - - - - - - - - - - + + + + + + + + + + + + + + + - - - - + - - - - - - - - - - - - - - - - - - - - - - - - + - - - - - - + - - - - - - - - - - - - - - - - - - - - - - - - + + - - - - - - - -- 1.7.3.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 3/3] snapshot: rudimentary qemu support for atomic disk snapshot
On 03/17/2012 05:33 PM, Eric Blake wrote: Taking an external snapshot of just one disk is atomic, without having to pause and resume the VM. This also paves the way for later patches to interact with the new qemu 'transaction' monitor command. The various scenarios when requesting atomic are: online, 1 disk, old qemu - safe, allowed by this patch online, more than 1 disk, old qemu - failure, this patch offline snapshot - safe, once a future patch implements offline disk snapshot online, 1 or more disks, new qemu - safe, once future patch uses transaction Taking an online system checkpoint snapshot is atomic, since it is done via a single 'savevm' monitor command. Taking an offline system checkpoint snapshot currently uses multiple qemu-img invocations with no provision for cleanup of partial failure, so for now we mark it unsupported. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Support new flag for single-disk setups. (qemuDomainSnapshotDiskPrepare): Check for atomic here. (qemuDomainSnapshotCreateDiskActive): Skip pausing the VM when atomic supported. (qemuDomainSnapshotIsAllowed): Use bool instead of int. --- Patches 1/3 and 2/3 unchanged. v2: consider interactions of atomic with non-disk-snapshots src/qemu/qemu_driver.c | 56 +-- 1 files changed, 39 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 85f8cf7..9e62738 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10279,6 +10295,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, &vm, snap, flags)< 0) goto cleanup; } else if (!virDomainObjIsActive(vm)) { +if (flags& VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) { +qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", +_("atomic snapshots of inactive domains not " + "implemented yet")); +goto cleanup; I wonder if we shouldn't add a error code for unimplemented functionality to differentiate it from unsupported configurations. (Well, but using a configuration that uses unimplemented functionality makes it an unsupported configuration basically, so I don't really mind) +} if (qemuDomainSnapshotCreateInactive(driver, vm, snap)< 0) goto cleanup; } else { ACK, Peter. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] snapshot: add atomic create flag
On 03/16/2012 11:05 PM, Eric Blake wrote: Right now, it is appallingly easy to cause qemu disk snapshots to alter a domain then fail; for example, by requesting a two-disk snapshot where the second disk name resides on read-only storage. In this failure scenario, libvirt reports failure, but modifies the live domain XML in-place to record that the first disk snapshot was taken; and places a difficult burden on the management app to grab the XML and reparse it to see which disks, if any, were altered by the partial snapshot. This patch adds a new flag where implementations can request that the hypervisor make snapshots atomically; either no changes to XML occur, or all disks were altered as a group. If you request the flag, you either get outright failure up front, or you take advantage of hypervisor abilities to make an atomic snapshot. Of course, drivers should prefer the atomic means even without the flag explicitly requested. There's no way to make snapshots 100% bulletproof - even if the hypervisor does it perfectly atomic, we could run out of memory during the followup tasks of updating our in-memory XML, and report a failure. However, these sorts of catastrophic failures are rare and unlikely, and it is still nicer to know that either all snapshots happened or none of them, as that is an easier state to recover from. * include/libvirt/libvirt.h.in (VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC): New flag. * src/libvirt.c (virDomainSnapshotCreateXML): Document it. * tools/virsh.c (cmdSnapshotCreate, cmdSnapshotCreateAs): Expose it. * tools/virsh.pod (snapshot-create, snapshot-create-as): Document it. --- ... + * that it is still possible to fail after disks have changed, but only + * in the much rarer cases of running out of memory or disk space). (sometimes it's not that rare (but then you've also got some other problems) :D) ACK, Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] snapshot: add qemu capability for 'transaction' command
On 03/16/2012 11:05 PM, Eric Blake wrote: We need a capability bit to gracefully error out if some of the additions in future patches can't be implemented by the running qemu. * src/qemu/qemu_capabilities.h (QEMU_CAPS_TRANSACTION): New cap. * src/qemu/qemu_capabilities.c (qemuCaps): Name it. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONCheckCommands): Set it. --- ACK, Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] Fixed NULL pointer check
On 19.03.2012 11:05, Martin Kletzander wrote: > This patch fixes a NULL pointer check that was causing SegFault on > some specific configurations. It also reverts commit 59d0c9801c1ab > that was checking for this value in one place. > --- > v3: > - added revert of 59d0c9801c1ab that's not needed anymore > > - (comment) ATTRIBUTE_RETURN_CHECK not added as it is not used with other >functions defined in this file and the commit could be confusing modifying >all of them together. However, there should be one patch to fix all these >at once if possible > > v2: > - removed parenthesis around return value > > src/libvirt.c |3 +-- > src/util/conf.c |5 - > 2 files changed, 5 insertions(+), 3 deletions(-) > I've tweaked the patch subject (prepended with 'virConfGetValue: ' for easier git-log) and pushed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] Fixed NULL pointer check
On Mon, Mar 19, 2012 at 11:05:30AM +0100, Martin Kletzander wrote: > This patch fixes a NULL pointer check that was causing SegFault on > some specific configurations. It also reverts commit 59d0c9801c1ab > that was checking for this value in one place. > --- > v3: > - added revert of 59d0c9801c1ab that's not needed anymore > > - (comment) ATTRIBUTE_RETURN_CHECK not added as it is not used with other >functions defined in this file and the commit could be confusing modifying >all of them together. However, there should be one patch to fix all these >at once if possible > > v2: > - removed parenthesis around return value > > src/libvirt.c |3 +-- > src/util/conf.c |5 - > 2 files changed, 5 insertions(+), 3 deletions(-) ACK Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fixed NULL pointer check
On Mon, Mar 19, 2012 at 08:51:24AM +0100, Martin Kletzander wrote: > On 03/19/2012 08:43 AM, Wen Congyang wrote: > > At 03/18/2012 08:29 AM, Martin Kletzander Wrote: > >> This patch fixes a NULL pointer check that was causing SegFault on > >> some specific configurations. > >> --- > >> src/util/conf.c |5 - > >> 1 files changed, 4 insertions(+), 1 deletions(-) > >> > >> diff --git a/src/util/conf.c b/src/util/conf.c > >> index 8ad60e0..e76362c 100644 > >> --- a/src/util/conf.c > >> +++ b/src/util/conf.c > >> @@ -1,7 +1,7 @@ > >> /** > >> * conf.c: parser for a subset of the Python encoded Xen configuration > >> files > >> * > >> - * Copyright (C) 2006-2011 Red Hat, Inc. > >> + * Copyright (C) 2006-2012 Red Hat, Inc. > >> * > >> * See COPYING.LIB for the License of this software > >> * > >> @@ -836,6 +836,9 @@ virConfGetValue(virConfPtr conf, const char *setting) > >> { > >> virConfEntryPtr cur; > >> > >> +if (conf == NULL) > >> +return(NULL); > > > > Please use 'return NULL;' instead of 'return(NULL);' > > > > 'return(NULL);' is old style, and we donot use it now and later. > > > > It seems weird to me too, it's just that it's almost everywhere in this > file so that's why I wanted to keep the same look. That is a historical remant. Do feel free to send another followup patch to cleanup all the cases of 'return(NULL)' as well. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] heisenbug in command.c
On Fri, Mar 16, 2012 at 04:42:42PM -0500, Serge Hallyn wrote: > On 03/16/2012 03:52 PM, Jim Paris wrote: > >Serge Hallyn wrote: > >>On 03/16/2012 11:50 AM, Eric Blake wrote: > >>>On 03/16/2012 10:36 AM, Serge Hallyn wrote: > Hi, > > It seems I've run into quite the heisenbug, reported at > https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/922628 > > It manifests itself as virPidWait returning status=4 for iptables (which > should never exit with status=4). > >>> > >>>Maybe iptables isn't documented as exiting with $? of 4, but that's what > >>>is happening. The libvirt code in question is quite clear that it > >>>grabbed an accurate exit status from the child process. > >>> > >> > >>Well, yes. I figured that either (1) iptables actually got -EINTR > >>from the kernel and passed that along as its exit code, or (2) > >>something went wrong with memory being overwritten in libvirt, > >>however unlikely. Stranger things have happened. If (1), I was > >>wondering if it was being ignored on purpose. > > > >Why do you bring up EINTR at all? Just because EINTR is 4? > > Yup. > > > That > >seems very much unrelated. > > I didn't think so, but looks like you're right :) > > >This is from iptables: > > > >enum xtables_exittype { > > OTHER_PROBLEM = 1, > > PARAMETER_PROBLEM, > > VERSION_PROBLEM, > > RESOURCE_PROBLEM, > > XTF_ONLY_ONCE, > > XTF_NO_INVERT, > > XTF_BAD_VALUE, > > XTF_ONE_ACTION, > >}; > > > >So it looks like iptables is returning RESOURCE_PROBLEM (which could > >explain why it's intermittent). > > That makes a lot more sense then, yes. When scripting a bunch of > parallel iptables rules adds (followed by waits), I do once in > awhile get > > iptables: Resource temporarily unavailable. > > (and sometimes a -EINVAL message) > > though 'wait' still says status was 0. I've never gotten 4. The > next run then succeeds. > > This still however sounds like src/util/iptables.c might ought to > re-run the command if it gets 4. Or do we need some serialization of commands at a higher level perhaps ? Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Add migration APIs for libxl driver
On Fri, Mar 09, 2012 at 06:55:55PM +0800, Chunyan Liu wrote: > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c > index d5fa64a..5dc29a0 100644 > --- a/src/libxl/libxl_driver.c > +++ b/src/libxl/libxl_driver.c > +static int doParseURI(const char *uri, char **p_hostname, int *p_port) > +{ > +char *p, *hostname; > +int port_nr = 0; > + > +if (uri == NULL) > +return -1; > + > +/* URI passed is a string "hostname[:port]" */ > +if ((p = strrchr(uri, ':')) != NULL) { /* "hostname:port" */ > +int n; > + > +if (virStrToLong_i(p+1, NULL, 10, &port_nr) < 0) { > +libxlError(VIR_ERR_INVALID_ARG, > +_("Invalid port number")); > +return -1; > +} > + > +/* Get the hostname. */ > +n = p - uri; /* n = Length of hostname in bytes. */ > +if (n <= 0) { > +libxlError(VIR_ERR_INVALID_ARG, > + _("Hostname must be specified in the URI")); > +return -1; > +} > + > +if (virAsprintf(&hostname, "%s", uri) < 0) { > +virReportOOMError(); > +return -1; > +} > + > +hostname[n] = '\0'; > +} > +else {/* "hostname" (or IP address) */ > +if (virAsprintf(&hostname, "%s", uri) < 0) { > +virReportOOMError(); > +return -1; > +} > +} > +*p_hostname = hostname; > +*p_port = port_nr; > +return 0; > +} Lets not re-invent URI parsing code with wierd special cases for base hostnames. Please just use virURIParse, since there is no compatibility issue here. > +static void doMigrateReceive(void *opaque) > +{ > +migrate_receive_args *data = opaque; > +virConnectPtr conn = data->conn; > +int sockfd = data->sockfd; > +virDomainObjPtr vm = data->vm; > +libxlDriverPrivatePtr driver = conn->privateData; > +int recv_fd; > +struct sockaddr_in new_addr; > +socklen_t socklen = sizeof(new_addr); > +int len; > + > +do { > +recv_fd = accept(sockfd, (struct sockaddr *)&new_addr, &socklen); > +} while(recv_fd < 0 && errno == EINTR); [snip] > +sockfd = socket(AF_INET, SOCK_STREAM, 0); > +if (sockfd == -1) { > +libxlError(VIR_ERR_OPERATION_FAILED, > + _("Failed to create socket for incoming migration")); > +goto cleanup; > +} > + > +memset(&addr, 0, sizeof(addr)); > +addr.sin_family = AF_INET; > +addr.sin_port = htons(port); > +addr.sin_addr.s_addr = htonl(INADDR_ANY); > + > +if (bind(sockfd, (struct sockaddr *)&addr, sizeof(addr)) < 0) { > +libxlError(VIR_ERR_OPERATION_FAILED, > + _("Fail to bind port for incoming migration")); > +goto cleanup; > +} > + > +if (listen(sockfd, MAXCONN_NUM) < 0){ > +libxlError(VIR_ERR_OPERATION_FAILED, > + _("Fail to listen to incoming migration")); > +goto cleanup; > +} > + > +if (VIR_ALLOC(args) < 0) { > +virReportOOMError(); > +goto cleanup; > +} [snip] > +memset(&hints, 0, sizeof(hints)); > +hints.ai_family = AF_INET; > +hints.ai_socktype = SOCK_STREAM; > +if (getaddrinfo(hostname, servname, &hints, &res) || res == NULL) { > +libxlError(VIR_ERR_OPERATION_FAILED, > + _("IP address lookup failed")); > +goto cleanup; > +} > + > +if (connect(sockfd, res->ai_addr, res->ai_addrlen) < 0) { > +libxlError(VIR_ERR_OPERATION_FAILED, > + _("Connect error")); > +goto cleanup; > +} I know the QEMU code doesn't follow what I'm about to say, but I would prefer it if all this socket code was re-written to use the internal virNetSocketPtr APIs. In particular this would result in correct usage of getaddrinfo() which is lacking here. > diff --git a/src/libxl/libxl_driver.h b/src/libxl/libxl_driver.h > index 4632d33..ad8df1f 100644 > --- a/src/libxl/libxl_driver.h > +++ b/src/libxl/libxl_driver.h > @@ -21,9 +21,25 @@ > > /*---*/ > > #ifndef LIBXL_DRIVER_H > -# define LIBXL_DRIVER_H > +#define LIBXL_DRIVER_H > > -# include > +#include > + > +#define LIBXL_MIGRATION_FLAGS \ > +(VIR_MIGRATE_LIVE | \ > + VIR_MIGRATE_UNDEFINE_SOURCE | \ > + VIR_MIGRATE_PAUSED) > + > +#define MAXCONN_NUM 10 > +#define LIBXL_MIGRATION_MIN_PORT 49512 > +#define LIBXL_MIGRATION_NUM_PORTS 64 > +#define LIBXL_MIGRATION_MAX_PORT\ > +(LIBXL_MIGRATION_MIN_PORT + LIBXL_MIGRATION_NUM_PORTS) > + > +static const char migrate_receiver_banner[]= > +"xl migration receiver ready, send binary domain data"; > +static const char migrate_receiver_ready[]= > +"domain received, ready to unpause"; It is better if these were in the .c file, or if they need to be shared across multiple driver files, use the l
[libvirt] [PATCH v3] Fixed NULL pointer check
This patch fixes a NULL pointer check that was causing SegFault on some specific configurations. It also reverts commit 59d0c9801c1ab that was checking for this value in one place. --- v3: - added revert of 59d0c9801c1ab that's not needed anymore - (comment) ATTRIBUTE_RETURN_CHECK not added as it is not used with other functions defined in this file and the commit could be confusing modifying all of them together. However, there should be one patch to fix all these at once if possible v2: - removed parenthesis around return value src/libvirt.c |3 +-- src/util/conf.c |5 - 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 7f8d42c..99b263e 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -1085,8 +1085,7 @@ virConnectOpenResolveURIAlias(virConfPtr conf, *uri = NULL; -if (conf && -(value = virConfGetValue(conf, "uri_aliases"))) +if ((value = virConfGetValue(conf, "uri_aliases"))) ret = virConnectOpenFindURIAliasMatch(value, alias, uri); else ret = 0; diff --git a/src/util/conf.c b/src/util/conf.c index 8ad60e0..3370337 100644 --- a/src/util/conf.c +++ b/src/util/conf.c @@ -1,7 +1,7 @@ /** * conf.c: parser for a subset of the Python encoded Xen configuration files * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -836,6 +836,9 @@ virConfGetValue(virConfPtr conf, const char *setting) { virConfEntryPtr cur; +if (conf == NULL) +return NULL; + cur = conf->entries; while (cur != NULL) { if ((cur->name != NULL) && -- 1.7.3.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] snapshot: make quiesce a bit safer
On 16.03.2012 21:49, Eric Blake wrote: > If a guest is paused, we were silently ignoring the quiesce flag, > which results in unclean snapshots, contrary to the intent of the > flag. Since we can't quiesce without guest agent support, we should > instead fail if the guest is not running. > > Meanwhile, if we attempt a quiesce command, but the guest agent > doesn't respond, and we time out, we may have left the command > pending on the guest's queue, and when the guest resumes parsing > commands, it will freeze even though our command is no longer > around to issue a thaw. To be safe, we must _always_ pair every > quiesce call with a counterpart thaw, even if the quiesce call > failed due to a timeout, so that if a guest wakes up and starts > processing a command backlog, it will not get stuck in a frozen > state. > > * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive): > Always issue thaw after a quiesce, even if quiesce failed. > (qemuDomainSnapshotFSThaw): Add a parameter. > --- > > See also: https://bugzilla.redhat.com/show_bug.cgi?id=804210 > https://www.redhat.com/archives/libvir-list/2012-March/msg00708.html > > src/qemu/qemu_driver.c | 51 +-- > 1 files changed, 36 insertions(+), 15 deletions(-) > ACK this and the follow up patch as well. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] Fixed NULL pointer check
At 03/19/2012 05:38 PM, Michal Privoznik Wrote: > On 19.03.2012 08:55, Martin Kletzander wrote: >> This patch fixes a NULL pointer check that was causing SegFault on >> some specific configurations. >> --- >> src/util/conf.c |5 - >> 1 files changed, 4 insertions(+), 1 deletions(-) >> >> diff --git a/src/util/conf.c b/src/util/conf.c >> index 8ad60e0..3370337 100644 >> --- a/src/util/conf.c >> +++ b/src/util/conf.c >> @@ -1,7 +1,7 @@ >> /** >> * conf.c: parser for a subset of the Python encoded Xen configuration files >> * >> - * Copyright (C) 2006-2011 Red Hat, Inc. >> + * Copyright (C) 2006-2012 Red Hat, Inc. >> * >> * See COPYING.LIB for the License of this software >> * >> @@ -836,6 +836,9 @@ virConfGetValue(virConfPtr conf, const char *setting) >> { >> virConfEntryPtr cur; >> >> +if (conf == NULL) >> +return NULL; >> + >> cur = conf->entries; >> while (cur != NULL) { >> if ((cur->name != NULL) && > > Looking good, but I think we should revert 59d0c9801c1ab then. I agree with reverting this commit. It does not fix all space. > In addition - maybe we can set ATTRIBUTE_RETURN_CHECK to this function. Agree with it. Thanks Wen Congyang > > Michal > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] Fixed NULL pointer check
On 03/19/2012 10:38 AM, Michal Privoznik wrote: On 19.03.2012 08:55, Martin Kletzander wrote: This patch fixes a NULL pointer check that was causing SegFault on some specific configurations. --- src/util/conf.c |5 - 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/src/util/conf.c b/src/util/conf.c index 8ad60e0..3370337 100644 --- a/src/util/conf.c +++ b/src/util/conf.c @@ -836,6 +836,9 @@ virConfGetValue(virConfPtr conf, const char *setting) { virConfEntryPtr cur; +if (conf == NULL) +return NULL; + cur = conf->entries; while (cur != NULL) { if ((cur->name != NULL)&& Looking good, but I think we should revert 59d0c9801c1ab then. In addition - maybe we can set ATTRIBUTE_RETURN_CHECK to this function. Yes, with this change the check added in that commit is redundant. Peter Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] Fixed NULL pointer check
On 19.03.2012 08:55, Martin Kletzander wrote: > This patch fixes a NULL pointer check that was causing SegFault on > some specific configurations. > --- > src/util/conf.c |5 - > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/src/util/conf.c b/src/util/conf.c > index 8ad60e0..3370337 100644 > --- a/src/util/conf.c > +++ b/src/util/conf.c > @@ -1,7 +1,7 @@ > /** > * conf.c: parser for a subset of the Python encoded Xen configuration files > * > - * Copyright (C) 2006-2011 Red Hat, Inc. > + * Copyright (C) 2006-2012 Red Hat, Inc. > * > * See COPYING.LIB for the License of this software > * > @@ -836,6 +836,9 @@ virConfGetValue(virConfPtr conf, const char *setting) > { > virConfEntryPtr cur; > > +if (conf == NULL) > +return NULL; > + > cur = conf->entries; > while (cur != NULL) { > if ((cur->name != NULL) && Looking good, but I think we should revert 59d0c9801c1ab then. In addition - maybe we can set ATTRIBUTE_RETURN_CHECK to this function. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] Fixed NULL pointer check
This patch fixes a NULL pointer check that was causing SegFault on some specific configurations. --- src/util/conf.c |5 - 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/src/util/conf.c b/src/util/conf.c index 8ad60e0..3370337 100644 --- a/src/util/conf.c +++ b/src/util/conf.c @@ -1,7 +1,7 @@ /** * conf.c: parser for a subset of the Python encoded Xen configuration files * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -836,6 +836,9 @@ virConfGetValue(virConfPtr conf, const char *setting) { virConfEntryPtr cur; +if (conf == NULL) +return NULL; + cur = conf->entries; while (cur != NULL) { if ((cur->name != NULL) && -- 1.7.3.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fixed NULL pointer check
On 03/19/2012 08:43 AM, Wen Congyang wrote: > At 03/18/2012 08:29 AM, Martin Kletzander Wrote: >> This patch fixes a NULL pointer check that was causing SegFault on >> some specific configurations. >> --- >> src/util/conf.c |5 - >> 1 files changed, 4 insertions(+), 1 deletions(-) >> >> diff --git a/src/util/conf.c b/src/util/conf.c >> index 8ad60e0..e76362c 100644 >> --- a/src/util/conf.c >> +++ b/src/util/conf.c >> @@ -1,7 +1,7 @@ >> /** >> * conf.c: parser for a subset of the Python encoded Xen configuration files >> * >> - * Copyright (C) 2006-2011 Red Hat, Inc. >> + * Copyright (C) 2006-2012 Red Hat, Inc. >> * >> * See COPYING.LIB for the License of this software >> * >> @@ -836,6 +836,9 @@ virConfGetValue(virConfPtr conf, const char *setting) >> { >> virConfEntryPtr cur; >> >> +if (conf == NULL) >> +return(NULL); > > Please use 'return NULL;' instead of 'return(NULL);' > > 'return(NULL);' is old style, and we donot use it now and later. > It seems weird to me too, it's just that it's almost everywhere in this file so that's why I wanted to keep the same look. >> + >> cur = conf->entries; >> while (cur != NULL) { >> if ((cur->name != NULL) && > > ACK with the nit fixed. > I'll send a second version, I don't have push permissions. > Wen Congyang > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fixed NULL pointer check
At 03/18/2012 08:29 AM, Martin Kletzander Wrote: > This patch fixes a NULL pointer check that was causing SegFault on > some specific configurations. > --- > src/util/conf.c |5 - > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/src/util/conf.c b/src/util/conf.c > index 8ad60e0..e76362c 100644 > --- a/src/util/conf.c > +++ b/src/util/conf.c > @@ -1,7 +1,7 @@ > /** > * conf.c: parser for a subset of the Python encoded Xen configuration files > * > - * Copyright (C) 2006-2011 Red Hat, Inc. > + * Copyright (C) 2006-2012 Red Hat, Inc. > * > * See COPYING.LIB for the License of this software > * > @@ -836,6 +836,9 @@ virConfGetValue(virConfPtr conf, const char *setting) > { > virConfEntryPtr cur; > > +if (conf == NULL) > +return(NULL); Please use 'return NULL;' instead of 'return(NULL);' 'return(NULL);' is old style, and we donot use it now and later. > + > cur = conf->entries; > while (cur != NULL) { > if ((cur->name != NULL) && ACK with the nit fixed. Wen Congyang -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list