Re: [libvirt] [PATCH] Revert "daemon: use socket activation with systemd"
On Tue, Apr 19, 2016 at 11:41:58AM -0400, Cole Robinson wrote: ping. Martin you had suggested removing the socket file in one of the bugs, are you cool with this? Not only that, but I didn't like that I had to add it in, so removing it is great. Maybe we can also remove the logic inside the daemon as we might've stopped using that for the session daemon as well. ACK from me. signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/7] docs: Document the new XML elements
On 04/18/2016 01:44 PM, Andrea Bolognani wrote: > --- > docs/formatdomain.html.in | 3 ++- > docs/formatdomaincaps.html.in | 43 > ++- > 2 files changed, 44 insertions(+), 2 deletions(-) ACK - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/7] qemu: Cache GIC capabilities
On 04/18/2016 01:44 PM, Andrea Bolognani wrote:
> Implement support for saving GIC capabilities in the cache and
> read them back.
> ---
> src/qemu/qemu_capabilities.c | 87
>
> 1 file changed, 87 insertions(+)
>
I was surprised by lack of test cases, but it seems a common problem in this
area of the code, so nothing to handle for this patch series...
One comment below
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index ee80be2..be3e420 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -2926,6 +2926,77 @@ virQEMUCapsLoadCache(virQEMUCapsPtr qemuCaps, const
> char *filename,
> }
> VIR_FREE(nodes);
>
> +if ((n = virXPathNodeSet("./gic", ctxt, &nodes)) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to parse qemu capabilities gic"));
> +goto cleanup;
> +}
> +if (n > 0) {
> +unsigned int uintValue;
> +bool boolValue;
> +
> +qemuCaps->ngicCapabilities = n;
> +if (VIR_ALLOC_N(qemuCaps->gicCapabilities, n) < 0)
> +goto cleanup;
> +
> +for (i = 0; i < n; i++) {
> +virGICCapabilityPtr cap = &qemuCaps->gicCapabilities[i];
> +
> +if (!(str = virXMLPropString(nodes[i], "version"))) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing GIC version "
> + "in QEMU capabilities cache"));
> +goto cleanup;
> +}
> +if (str &&
> +virStrToLong_ui(str, NULL, 10, &uintValue) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("malformed GIC version "
> + "in QEMU capabilities cache"));
> +goto cleanup;
> +}
> +cap->version = uintValue;
> +VIR_FREE(str);
> +
> +if (!(str = virXMLPropString(nodes[i], "kernel"))) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing in-kernel GIC information "
> + "in QEMU capabilities cache"));
> +goto cleanup;
> +}
> +if (str &&
> +!(boolValue = STREQ(str, "true")) &&
> +STRNEQ(str, "false")) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("malformed in-kernel GIC information "
> + "in QEMU capabilities cache"));
> +goto cleanup;
> +}
> +if (boolValue)
> +cap->implementation |= VIR_GIC_IMPLEMENTATION_KERNEL;
> +VIR_FREE(str);
> +
> +if (!(str = virXMLPropString(nodes[i], "emulated"))) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing emulated GIC information "
> + "in QEMU capabilities cache"));
> +goto cleanup;
> +}
> +if (str &&
> +!(boolValue = STREQ(str, "true")) &&
> +STRNEQ(str, "false")) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("malformed emulated GIC information "
> + "in QEMU capabilities cache"));
> +goto cleanup;
> +}
> +if (boolValue)
> +cap->implementation |= VIR_GIC_IMPLEMENTATION_EMULATED;
> +VIR_FREE(str);
> +}
> +}
> +VIR_FREE(nodes);
> +
> ret = 0;
> cleanup:
> VIR_FREE(str);
> @@ -2992,6 +3063,22 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const
> char *filename)
>qemuCaps->machineMaxCpus[i]);
> }
>
> +for (i = 0; i < qemuCaps->ngicCapabilities; i++) {
> +virGICCapabilityPtr cap;
> +bool kernel;
> +bool emulated;
> +
> +cap = &qemuCaps->gicCapabilities[i];
> +kernel = (cap->implementation & VIR_GIC_IMPLEMENTATION_KERNEL);
> +emulated = (cap->implementation & VIR_GIC_IMPLEMENTATION_EMULATED);
> +
> +virBufferAsprintf(&buf,
> + "\n",
> + cap->version,
> + kernel ? "true" : "false",
> + emulated ? "true" : "false");
> +}
> +
> virBufferAdjustIndent(&buf, -2);
> virBufferAddLit(&buf, "\n");
>
>
Use of literal 'true'/'false' isn't common in our XML formats. This isn't user
facing, but still probably better to use 'yes'/'no', if only so that some
future cleanup can streamline things here :)
ACK otherwise
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/7] qemu: Fill in GIC capabilities
On 04/18/2016 01:44 PM, Andrea Bolognani wrote: > Take the GIC capabilities stored in a virQEMUCaps instance and > update a virDomainCaps instance appropriately. > --- > src/qemu/qemu_capabilities.c | 57 > +++- > 1 file changed, 56 insertions(+), 1 deletion(-) ACK - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/7] conf: Expose GIC capabilities
On 04/18/2016 01:44 PM, Andrea Bolognani wrote: > Add information about GIC capabilities to virDomainCaps and update > the formatter to include them in the XML output. > --- > src/conf/domain_capabilities.c | 36 > ++ > src/conf/domain_capabilities.h | 10 ++ > tests/domaincapsschemadata/domaincaps-basic.xml| 3 ++ > tests/domaincapsschemadata/domaincaps-full.xml | 3 ++ > .../domaincaps-qemu_1.6.50-1.xml | 3 ++ > 5 files changed, 55 insertions(+) > ACK - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/7] schema: Validate GIC capabilities
On 04/18/2016 01:43 PM, Andrea Bolognani wrote: > We need to expose GIC capabilities in the domain capabilities > XML: update the schema to validate documents that contain the > new information. > --- > docs/schemas/domaincaps.rng | 18 ++ > 1 file changed, 18 insertions(+) > ACK - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/7] qemu: Probe GIC capabilities
On 04/18/2016 01:43 PM, Andrea Bolognani wrote: > QEMU introduced the query-gic-capabilities QMP command > with commit 4468d4e0f383: use the command, if available, > to probe available GIC capabilities. > > The information obtained is stored in a virQEMUCaps > instance, and will be later used to fill in a > virDomainCaps instance. > --- > Changes from RFC: > > * Free qemuCaps->gicCapabilities when needed > > * Always return NULL when no capabilities have been found > > * Don't allocate (n+1) elements to store (n) capabilities > > * Check for ARM consistently with the rest of the code > > * Reference QEMU commit > > * Leave two empty lines between functions > > * Document all functions > > src/qemu/qemu_capabilities.c | 42 > src/qemu/qemu_monitor.c | 17 +++ > src/qemu/qemu_monitor.h | 4 ++ > src/qemu/qemu_monitor_json.c | 115 > +++ > src/qemu/qemu_monitor_json.h | 4 ++ > src/util/virgic.h| 13 + > 6 files changed, 195 insertions(+) ACK - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/7] conf: Get rid of virDomainCapsDevice
On 04/18/2016 01:43 PM, Andrea Bolognani wrote:
> The struct contains a single boolean field, 'supported':
> the meaning of this field is too generic to be limited to
> devices only, and in fact it's already being used for
> other things like loaders and OSs.
>
> Instead of trying to come up with a more generic name just
> get rid of the struct altogether.
> ---
> Changes from RFC:
>
> * Improve commit message
>
> src/conf/domain_capabilities.c | 6 +++---
> src/conf/domain_capabilities.h | 14 --
> src/qemu/qemu_capabilities.c | 8
> tests/domaincapstest.c | 8
> 4 files changed, 15 insertions(+), 21 deletions(-)
>
> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
> index 0e32f52..466c0c6 100644
> --- a/src/conf/domain_capabilities.c
> +++ b/src/conf/domain_capabilities.c
> @@ -187,9 +187,9 @@ virDomainCapsStringValuesFormat(virBufferPtr buf,
> #define FORMAT_PROLOGUE(item) \
> do {\
> virBufferAsprintf(buf, "<" #item " supported='%s'%s\n", \
> - item->device.supported ? "yes" : "no",\
> - item->device.supported ? ">" : "/>"); \
> -if (!item->device.supported)\
> + item->supported ? "yes" : "no", \
> + item->supported ? ">" : "/>");\
> +if (!item->supported) \
> return; \
> virBufferAdjustIndent(buf, 2); \
> } while (0)
> diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
> index 597ac75..3eacb35 100644
> --- a/src/conf/domain_capabilities.h
> +++ b/src/conf/domain_capabilities.h
> @@ -44,16 +44,10 @@ struct _virDomainCapsStringValues {
> size_t nvalues; /* number of strings */
> };
>
> -typedef struct _virDomainCapsDevice virDomainCapsDevice;
> -typedef virDomainCapsDevice *virDomainCapsDevicePtr;
> -struct _virDomainCapsDevice {
> -bool supported; /* true if is supported by hypervisor */
> -};
> -
> typedef struct _virDomainCapsLoader virDomainCapsLoader;
> typedef virDomainCapsLoader *virDomainCapsLoaderPtr;
> struct _virDomainCapsLoader {
> -virDomainCapsDevice device;
> +bool supported;
> virDomainCapsStringValues values; /* Info about values for the element
> */
> virDomainCapsEnum type; /* Info about virDomainLoader */
> virDomainCapsEnum readonly; /* Info about readonly:virTristateBool */
> @@ -62,14 +56,14 @@ struct _virDomainCapsLoader {
> typedef struct _virDomainCapsOS virDomainCapsOS;
> typedef virDomainCapsOS *virDomainCapsOSPtr;
> struct _virDomainCapsOS {
> -virDomainCapsDevice device;
> +bool supported;
> virDomainCapsLoader loader; /* Info about virDomainLoaderDef */
> };
>
> typedef struct _virDomainCapsDeviceDisk virDomainCapsDeviceDisk;
> typedef virDomainCapsDeviceDisk *virDomainCapsDeviceDiskPtr;
> struct _virDomainCapsDeviceDisk {
> -virDomainCapsDevice device;
> +bool supported;
> virDomainCapsEnum diskDevice; /* Info about virDomainDiskDevice enum
> values */
> virDomainCapsEnum bus; /* Info about virDomainDiskBus enum
> values */
> /* add new fields here */
> @@ -78,7 +72,7 @@ struct _virDomainCapsDeviceDisk {
> typedef struct _virDomainCapsDeviceHostdev virDomainCapsDeviceHostdev;
> typedef virDomainCapsDeviceHostdev *virDomainCapsDeviceHostdevPtr;
> struct _virDomainCapsDeviceHostdev {
> -virDomainCapsDevice device;
> +bool supported;
> virDomainCapsEnum mode; /* Info about virDomainHostdevMode */
> virDomainCapsEnum startupPolicy;/* Info about virDomainStartupPolicy
> */
> virDomainCapsEnum subsysType; /* Info about
> virDomainHostdevSubsysType */
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index f46dcad..9ae7b27 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -3908,7 +3908,7 @@ virQEMUCapsFillDomainLoaderCaps(virQEMUCapsPtr qemuCaps,
> {
> size_t i;
>
> -capsLoader->device.supported = true;
> +capsLoader->supported = true;
>
> if (VIR_ALLOC_N(capsLoader->values.values, nloader) < 0)
> return -1;
> @@ -3950,7 +3950,7 @@ virQEMUCapsFillDomainOSCaps(virQEMUCapsPtr qemuCaps,
> {
> virDomainCapsLoaderPtr capsLoader = &os->loader;
>
> -os->device.supported = true;
> +os->supported = true;
> if (virQEMUCapsFillDomainLoaderCaps(qemuCaps, capsLoader,
> loader, nloader) < 0)
> return -1;
> @@ -3963,7 +3963,7 @@ virQEMUCapsFillDomainDeviceDiskCaps(virQEMUCapsPtr
> qemuCaps,
> const char *mac
Re: [libvirt] [PATCH v2 1/2] Explicitly error on uri=qemu://system
On 04/19/2016 12:46 PM, Maxim Nestratov wrote: > 19.04.2016 19:22, Cole Robinson пишет: > >> It's a fairly common error that a user tries to connect to a URI >> like qemu://system or qemu://session (missing a slash). This errors >> like: >> >> $ virsh --connect qemu://session >> error: failed to connect to the hypervisor >> error: Unable to resolve address 'session' service '16514': No address >> associated with hostname >> >> If you already know that the standard qemu URI has 3 slashes, that >> error will make it obvious enough. But new user's may not get it. >> There's even a RHEL support page explicitly mentioning it!: >> >> https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/7/html/Virtualization_Deployment_and_Administration_Guide/sect-Troubleshooting-Common_libvirt_errors_and_troubleshooting.html >> >> >> Catch this error early in libvirt.c virConnectOpen for qemu (and vbox >> which has similar rules >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1038304 >> --- >> v2: >> Use conventional function naming >> Improve a comment >> Handle 'vz' driver >> >> src/libvirt.c | 35 +++ >> 1 file changed, 35 insertions(+) > > ACK > Thanks, pushed patch this now - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nwfilter: fix lock order deadlock
On 04/09/2016 12:14 PM, Maxim Nestratov wrote:
> Below is backtraces of two deadlocked threads:
>
> thread #1:
> virDomainConfVMNWFilterTeardown
>virNWFilterTeardownFilter
>lock updateMutex <
>_virNWFilterTeardownFilter
> try to lock interface <--
>
> thread #2:
> learnIPAddressThread
> lock interface <---
> virNWFilterInstantiateFilterLate
> try to lock updateMutex <--
>
> The problem is fixed by unlocking interface before calling
> virNWFilterInstantiateFilterLate to avoid updateMutex and interface ordering
> deadlocks. Otherwise we are going to instantiate the filter while holding
> interface lock, which will try to lock updateMutex, and if some other thread
> instantiating a filter in parallel is holding updateMutex and is trying to
> lock interface, both will deadlock.
> Also it is safe to unlock interface before virNWFilterInstantiateFilterLate
> because learnIPAddressThread stopped capturing packets and applied necessary
> rules on the interface, while instantiating a new filter doesn't require a
> locked interface.
>
> Signed-off-by: Maxim Nestratov
> ---
> src/nwfilter/nwfilter_learnipaddr.c | 12 +++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/src/nwfilter/nwfilter_learnipaddr.c
> b/src/nwfilter/nwfilter_learnipaddr.c
> index 1adbadb..cfd92d9 100644
> --- a/src/nwfilter/nwfilter_learnipaddr.c
> +++ b/src/nwfilter/nwfilter_learnipaddr.c
> @@ -611,6 +611,16 @@ learnIPAddressThread(void *arg)
> sa.data.inet4.sin_addr.s_addr = vmaddr;
> char *inetaddr;
>
> +/* It is necessary to unlock interface here to avoid updateMutex and
> + * interface ordering deadlocks. Otherwise we are going to
> + * instantiate the filter, which will try to lock updateMutex, and
> + * some other thread instantiating a filter in parallel is holding
> + * updateMutex and is trying to lock interface, both will deadlock.
> + * Also it is safe to unlock interface here because we stopped
> + * capturing and applied necessary rules on the interface, while
> + * instantiating a new filter doesn't require a locked interface.*/
> +virNWFilterUnlockIface(req->ifname);
> +
> if ((inetaddr = virSocketAddrFormat(&sa)) != NULL) {
> if (virNWFilterIPAddrMapAddIPAddr(req->ifname, inetaddr) < 0) {
> VIR_ERROR(_("Failed to add IP address %s to IP address "
> @@ -636,11 +646,11 @@ learnIPAddressThread(void *arg)
> req->ifname, req->ifindex);
>
> techdriver->applyDropAllRules(req->ifname);
> +virNWFilterUnlockIface(req->ifname);
> }
>
> VIR_DEBUG("pcap thread terminating for interface %s", req->ifname);
>
> -virNWFilterUnlockIface(req->ifname);
>
> err_no_lock:
> virNWFilterDeregisterLearnReq(req->ifindex);
>
This looks sensible to me... the virNWFilterInstantiateFilterLate call tree
certainly expects the iface to be unlocked at a certain point. This patch just
moves the unlock a bit earlier.
ACK, but maybe wait another day to see if anyone else wants to jump in, I'm
not really familiar with this code (then again I doubt anyone watching the
list is deeply familiar with it either)
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] man: Clarify virsh vol-clone works within a single pool
virsh vol-clone is expected to clone a volume within a single
pool; it doesn't work for cloning across pools. Clarify the docs
https://bugzilla.redhat.com/show_bug.cgi?id=1103714
---
tools/virsh-volume.c | 2 +-
tools/virsh.pod | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
index 36dd0ed..9cc8e52 100644
--- a/tools/virsh-volume.c
+++ b/tools/virsh-volume.c
@@ -546,7 +546,7 @@ static const vshCmdInfo info_vol_clone[] = {
.data = N_("clone a volume.")
},
{.name = "desc",
- .data = N_("Clone an existing volume.")
+ .data = N_("Clone an existing volume within the parent pool.")
},
{.name = NULL}
};
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 6c9d4ec..e1c0d8e 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -3530,10 +3530,10 @@ only slightly higher initial disk space usage.
=item B [I<--pool> I] I
I [I<--prealloc-metadata>] [I<--reflink>]
-Clone an existing volume. Less powerful, but easier to type, version of
-B.
+Clone an existing volume within the parent pool. Less powerful,
+but easier to type, version of B.
I<--pool> I is the name or UUID of the storage pool to create
-the volume in.
+that contains the source volume, and will contain the new volume.
I is the name or key or path of the source volume.
I is the name of the new volume.
[I<--prealloc-metadata>] preallocate metadata (for qcow2 images which don't
--
2.7.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] network: Don't use ERR_NO_SUPPORT for invalid net-update requests
VIR_ERR_NO_SUPPORT maps to the error string
this function is not supported by the connection driver
and is largely only used for when a driver doesn't have any
implementation for a public API. So its usage with invalid
net-update requests is a bit out of place. Instead use
VIR_ERR_OPERATION_UNSUPPORTED which maps to:
Operation not supported
And is what qemu's hotplug routines use in similar scenarios
---
src/conf/network_conf.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 4fb2e2a..eabaccd 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -3376,14 +3376,14 @@ void virNetworkSetBridgeMacAddr(virNetworkDefPtr def)
static void
virNetworkDefUpdateNoSupport(virNetworkDefPtr def, const char *section)
{
-virReportError(VIR_ERR_NO_SUPPORT,
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("can't update '%s' section of network '%s'"),
section, def->name);
}
static void
virNetworkDefUpdateUnknownCommand(unsigned int command)
{
-virReportError(VIR_ERR_NO_SUPPORT,
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("unrecognized network update command code %d"), command);
}
@@ -3666,7 +3666,7 @@ virNetworkDefUpdateIPDHCPRange(virNetworkDefPtr def,
/* parse the xml into a virSocketAddrRange */
if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) {
-virReportError(VIR_ERR_NO_SUPPORT, "%s",
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("dhcp ranges cannot be modified, "
"only added or deleted"));
goto cleanup;
@@ -3772,7 +3772,7 @@ virNetworkDefUpdateForwardInterface(virNetworkDefPtr def,
goto cleanup;
if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) {
-virReportError(VIR_ERR_NO_SUPPORT, "%s",
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("forward interface entries cannot be modified, "
"only added or deleted"));
goto cleanup;
@@ -3973,7 +3973,7 @@ virNetworkDefUpdateDNSHost(virNetworkDefPtr def,
memset(&host, 0, sizeof(host));
if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) {
-virReportError(VIR_ERR_NO_SUPPORT, "%s",
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("DNS HOST records cannot be modified, "
"only added or deleted"));
goto cleanup;
@@ -4067,7 +4067,7 @@ virNetworkDefUpdateDNSSrv(virNetworkDefPtr def,
memset(&srv, 0, sizeof(srv));
if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) {
-virReportError(VIR_ERR_NO_SUPPORT, "%s",
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("DNS SRV records cannot be modified, "
"only added or deleted"));
goto cleanup;
@@ -4151,7 +4151,7 @@ virNetworkDefUpdateDNSTxt(virNetworkDefPtr def,
memset(&txt, 0, sizeof(txt));
if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) {
-virReportError(VIR_ERR_NO_SUPPORT, "%s",
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("DNS TXT records cannot be modified, "
"only added or deleted"));
goto cleanup;
@@ -4268,7 +4268,7 @@ virNetworkDefUpdateSection(virNetworkDefPtr def,
ret = virNetworkDefUpdateDNSSrv(def, command, parentIndex, ctxt,
flags);
break;
default:
-virReportError(VIR_ERR_NO_SUPPORT, "%s",
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("can't update unrecognized section of network"));
break;
}
--
2.7.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nwfilter: fix lock order deadlock
09.04.2016 19:14, Maxim Nestratov пишет: Below is backtraces of two deadlocked threads: thread #1: virDomainConfVMNWFilterTeardown virNWFilterTeardownFilter lock updateMutex < _virNWFilterTeardownFilter try to lock interface <-- thread #2: learnIPAddressThread lock interface <--- virNWFilterInstantiateFilterLate try to lock updateMutex <-- The problem is fixed by unlocking interface before calling virNWFilterInstantiateFilterLate to avoid updateMutex and interface ordering deadlocks. Otherwise we are going to instantiate the filter while holding interface lock, which will try to lock updateMutex, and if some other thread instantiating a filter in parallel is holding updateMutex and is trying to lock interface, both will deadlock. Also it is safe to unlock interface before virNWFilterInstantiateFilterLate because learnIPAddressThread stopped capturing packets and applied necessary rules on the interface, while instantiating a new filter doesn't require a locked interface. Signed-off-by: Maxim Nestratov --- src/nwfilter/nwfilter_learnipaddr.c | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) No one interested? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] RFC backup API
08.04.2016 23:47, Maxim Nestratov пишет: Hello all, Here is the first more detailed view on the list of the backup API functions that look reasonable to have. Though they are presented with parameters and short description all these are the subject to discuss and I kindly ask you to take a look and comment. Your ideas and considerations are very welcome. Any opinion on that? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt-php][PATCH 0/2] Fix some build issues
While trying to make 'distcheck' work again, this is where I got so far. Unfortunately, distcheck is still not working. Michal Privoznik (2): tools: Compile generate-api-docs into builddir configure.ac: Slightly rework configure.ac | 11 ++- tools/Makefile.am | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) -- 2.7.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt-php][PATCH 2/2] configure.ac: Slightly rework
Firstly, AC_INIT() macro accepts more arguments than we currently pass. Then, it's considered a good practice to specify AC_CONFIG_SRCDIR() too. The rest is just some reorganization so that common things are close to each other rather than distributed randomly in the file. Signed-off-by: Michal Privoznik --- configure.ac | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/configure.ac b/configure.ac index 51a2cfe..2549b55 100644 --- a/configure.ac +++ b/configure.ac @@ -1,6 +1,7 @@ -AC_INIT([libvirt-php], [0.5.1], [http://libvirt.org]) +AC_INIT([libvirt-php], [0.5.1], [libvir-list@redhat.com], [], [http://libvirt.org]) +AC_CONFIG_SRCDIR([src/libvirt-php.c]) +AC_CONFIG_HEADERS([config.h]) AM_INIT_AUTOMAKE([-Wall -Werror -Wno-portability]) -AC_CONFIG_FILES([Makefile tools/Makefile src/Makefile tests/Makefile docs/Makefile]) AM_MAINTAINER_MODE([enable]) m4_ifndef([LT_INIT], [ @@ -157,10 +158,10 @@ if test "$OS" = "Darwin"; then else SHLIB_FLAGS="-shared -rdynamic" fi -AC_SUBST([SHLIB_FLAGS]) +AC_SUBST([SHLIB_FLAGS]) AC_SUBST([PHPIZE]) AC_SUBST([PHPCONFIG]) -AC_CONFIG_HEADERS([config.h]) -AC_CONFIG_FILES([libvirt-php.spec]) +AC_CONFIG_FILES([Makefile tools/Makefile src/Makefile \ + tests/Makefile docs/Makefile libvirt-php.spec]) AC_OUTPUT -- 2.7.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt-php][PATCH 1/2] tools: Compile generate-api-docs into builddir
Since we are building this file every single time, there's no need for us requiring it to be in srcdir and thus part of a .tar.gz. Signed-off-by: Michal Privoznik --- tools/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/Makefile.am b/tools/Makefile.am index 6d3f6bc..73386ef 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -12,7 +12,7 @@ doc_generated_files = \ $(doc_generated_files): $(APIBUILD_STAMP) -$(APIBUILD_STAMP): $(srcdir)/generate-api-docs +$(APIBUILD_STAMP): generate-api-docs ./generate-api-docs $(top_srcdir)/src/libvirt-php.c ../docs/api-reference.html.in ./generate-api-docs --private $(top_srcdir)/src/libvirt-php.c ../docs/dev-api-reference.html.in touch $@ -- 2.7.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] vz: handle sourceless cdroms
14.04.2016 18:00, Nikolay Shirokovskiy пишет: From: Mikhail Feoktistov libvirt handles empty source as NULL, while vz sdk as "" thus we need a bit of conversion. Signed-off-by: Nikolay Shirokovskiy --- src/vz/vz_sdk.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) changes from v1: 1. simplify empty string check in prlsdkGetDiskInfo 2. call setting "" explicitly in prlsdkAddDisk. point 2 is the actual reason for releasing new version. I need this semantics on my future work of updating disks. diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 2e9544b..9b783af 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -565,7 +565,7 @@ prlsdkGetDiskInfo(vzDriverPtr driver, if (!(buf = prlsdkGetStringParamVar(PrlVmDev_GetFriendlyName, prldisk))) goto cleanup; -if (virDomainDiskSetSource(disk, buf) < 0) +if (*buf != '\0' && virDomainDiskSetSource(disk, buf) < 0) goto cleanup; if (prlsdkGetDiskId(prldisk, isCt, &disk->bus, &disk->dst) < 0) @@ -3183,6 +3183,7 @@ static int prlsdkAddDisk(vzDriverPtr driver, PRL_DEVICE_TYPE devType; PRL_CLUSTERED_DEVICE_SUBTYPE scsiModel; char *dst = NULL; +const char *path = disk->src->path ? : ""; if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) devType = PDE_HARD_DISK; @@ -3206,10 +3207,10 @@ static int prlsdkAddDisk(vzDriverPtr driver, pret = PrlVmDev_SetEmulatedType(sdkdisk, emutype); prlsdkCheckRetGoto(pret, cleanup); -pret = PrlVmDev_SetSysName(sdkdisk, disk->src->path); +pret = PrlVmDev_SetSysName(sdkdisk, path); prlsdkCheckRetGoto(pret, cleanup); -pret = PrlVmDev_SetFriendlyName(sdkdisk, disk->src->path); +pret = PrlVmDev_SetFriendlyName(sdkdisk, path); prlsdkCheckRetGoto(pret, cleanup); drive = &disk->info.addr.drive; ACK. Pushed, thanks. Maxim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REBASE 0/4] vz: use disk bus and target name as disk id
14.04.2016 13:45, Nikolay Shirokovskiy пишет: This series is a preparation to implement device update functionality. In case of disk updates disk bus and disk target name pair is considered disk id. We already have places in vz driver code where we use disk source as id. These places are either not critical (see boot patch) or correct (see detach patch) so we need not to fix them immediately to use more rubust id. But as this id will be introduced anyway for disk updates let's move to new id in existing code. This way we have single id for disks. Nikolay Shirokovskiy (4): vz: introduce vzsdk disk id function vz: fix detach disk to use new disk id vz: fix boot check to use new disk id vz: cleanup: remove trivial function src/vz/vz_sdk.c | 251 1 file changed, 126 insertions(+), 125 deletions(-) Pushed the series. Thanks. Maxim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REBASE 4/4] vz: cleanup: remove trivial function
14.04.2016 13:45, Nikolay Shirokovskiy пишет:
---
src/vz/vz_sdk.c | 37 +
1 file changed, 13 insertions(+), 24 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 4df2ca0..2e9544b 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -3169,25 +3169,6 @@ int prlsdkDetachNet(vzDriverPtr driver,
return ret;
}
-static int prlsdkDelDisk(PRL_HANDLE sdkdom, int idx)
-{
-int ret = -1;
-PRL_RESULT pret;
-PRL_HANDLE sdkdisk = PRL_INVALID_HANDLE;
-
-pret = PrlVmCfg_GetHardDisk(sdkdom, idx, &sdkdisk);
-prlsdkCheckRetGoto(pret, cleanup);
-
-pret = PrlVmDev_Remove(sdkdisk);
-prlsdkCheckRetGoto(pret, cleanup);
-
-ret = 0;
-
- cleanup:
-PrlHandle_Free(sdkdisk);
-return ret;
-}
-
static int prlsdkAddDisk(vzDriverPtr driver,
PRL_HANDLE sdkdom,
virDomainDiskDefPtr disk)
@@ -3809,6 +3790,7 @@ prlsdkDetachDomainHardDisks(PRL_HANDLE sdkdom)
PRL_UINT32 hddCount;
PRL_UINT32 i;
PRL_HANDLE job;
+PRL_HANDLE sdkdisk = PRL_INVALID_HANDLE;
job = PrlVm_BeginEdit(sdkdom);
if (PRL_FAILED(waitJob(job)))
@@ -3818,17 +3800,24 @@ prlsdkDetachDomainHardDisks(PRL_HANDLE sdkdom)
prlsdkCheckRetGoto(pret, cleanup);
for (i = 0; i < hddCount; ++i) {
-ret = prlsdkDelDisk(sdkdom, i);
-if (ret)
-goto cleanup;
+pret = PrlVmCfg_GetHardDisk(sdkdom, i, &sdkdisk);
+prlsdkCheckRetGoto(pret, cleanup);
+
+pret = PrlVmDev_Remove(sdkdisk);
+prlsdkCheckRetGoto(pret, cleanup);
+
+PrlHandle_Free(sdkdisk);
+sdkdisk = PRL_INVALID_HANDLE;
}
job = PrlVm_CommitEx(sdkdom, PVCF_DETACH_HDD_BUNDLE);
if (PRL_FAILED(waitJob(job)))
-ret = -1;
+goto cleanup;
- cleanup:
+ret = 0;
+ cleanup:
+PrlHandle_Free(sdkdisk);
return ret;
}
ACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REBASE 3/4] vz: fix boot check to use new disk id
14.04.2016 13:45, Nikolay Shirokovskiy пишет: Current implementation does not detects all incompatible configurations. For example if we have in vzsdk bootorder "cdrom1, cdrom0" (that is "hdb, hda" in case of ide cdroms) and cdroms do not have disk images inserted. In this case boot order check code fail to distiguish them at all as for both PrlVmDev_GetFriendlyName gives "". Well the consequences are only missing warnings but as we just have introduced all the necessary tools to face the problem - let's fix it. Signed-off-by: Nikolay Shirokovskiy --- src/vz/vz_sdk.c | 19 ++- 1 file changed, 10 insertions(+), 9 deletions(-) ACK -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/2] Explicitly error on uri=qemu://system
19.04.2016 19:22, Cole Robinson пишет: It's a fairly common error that a user tries to connect to a URI like qemu://system or qemu://session (missing a slash). This errors like: $ virsh --connect qemu://session error: failed to connect to the hypervisor error: Unable to resolve address 'session' service '16514': No address associated with hostname If you already know that the standard qemu URI has 3 slashes, that error will make it obvious enough. But new user's may not get it. There's even a RHEL support page explicitly mentioning it!: https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/7/html/Virtualization_Deployment_and_Administration_Guide/sect-Troubleshooting-Common_libvirt_errors_and_troubleshooting.html Catch this error early in libvirt.c virConnectOpen for qemu (and vbox which has similar rules https://bugzilla.redhat.com/show_bug.cgi?id=1038304 --- v2: Use conventional function naming Improve a comment Handle 'vz' driver src/libvirt.c | 35 +++ 1 file changed, 35 insertions(+) ACK -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/10] secret: Move and rename secretLoadAllConfigs
On 04/19/2016 12:30 PM, John Ferlan wrote: > > > On 04/19/2016 11:10 AM, Cole Robinson wrote: >> On 04/19/2016 08:01 AM, John Ferlan wrote: >>> >>> >>> On 04/18/2016 05:48 PM, Cole Robinson wrote: On 03/02/2016 01:55 PM, John Ferlan wrote: > Move to secret_conf.c and rename to virSecretLoadAllConfigs. Also includes > moving/renaming the supporting virSecretLoad, virSecretLoadValue, and > virSecretLoadValidateUUID. > > Signed-off-by: John Ferlan > --- > src/conf/secret_conf.c | 175 > + > src/conf/secret_conf.h | 3 + > src/libvirt_private.syms | 1 + > src/secret/secret_driver.c | 174 > +--- > 4 files changed, 181 insertions(+), 172 deletions(-) ACK, mirrors network_conf.c layout. (though honestly I'd rather we have separate files for XML handling and object handling... the existing conf.c files are too large anyways. I put an entry on the LibvirtFirstBugs wiki page about that type of code reorg though it's probably over an initial contributors head) >>> >>> I could go the route of a "virsecretobj.c" (to partially mimic the >>> virdomainobjlist.c) instead of cramming everything into secret_conf - >>> there's about 1000 new lines in secret_conf just for this series (with >>> the reduction of lines in secret_driver). >>> >>> It would mean a complete v2 of this series - although considering most >>> things have been ACK'd the second review should be easier. I think >>> there's only a couple of issues/questions to resolve from reviews. >>> >> >> Oh I wasn't trying to imply making it a requirement of this series. >> Definitely >> an additive thing, and I'm not even trying to pin you with the work, just >> floating the general idea >> > > Understood - I'd rather do it now than take a hit at some unknown future > point in time to essentially do the same thing. In the long run it's the > "right thing" to do. > Yeah but I hate to see self contained changes get held up just to accommodate an additional cleanup. Reviewer bandwidth is the bottleneck here - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/10] secret: Move and rename secretLoadAllConfigs
On 04/19/2016 11:10 AM, Cole Robinson wrote: > On 04/19/2016 08:01 AM, John Ferlan wrote: >> >> >> On 04/18/2016 05:48 PM, Cole Robinson wrote: >>> On 03/02/2016 01:55 PM, John Ferlan wrote: Move to secret_conf.c and rename to virSecretLoadAllConfigs. Also includes moving/renaming the supporting virSecretLoad, virSecretLoadValue, and virSecretLoadValidateUUID. Signed-off-by: John Ferlan --- src/conf/secret_conf.c | 175 + src/conf/secret_conf.h | 3 + src/libvirt_private.syms | 1 + src/secret/secret_driver.c | 174 +--- 4 files changed, 181 insertions(+), 172 deletions(-) >>> >>> ACK, mirrors network_conf.c layout. >>> >>> (though honestly I'd rather we have separate files for XML handling and >>> object >>> handling... the existing conf.c files are too large anyways. I put an entry >>> on >>> the LibvirtFirstBugs wiki page about that type of code reorg though it's >>> probably over an initial contributors head) >>> >> >> I could go the route of a "virsecretobj.c" (to partially mimic the >> virdomainobjlist.c) instead of cramming everything into secret_conf - >> there's about 1000 new lines in secret_conf just for this series (with >> the reduction of lines in secret_driver). >> >> It would mean a complete v2 of this series - although considering most >> things have been ACK'd the second review should be easier. I think >> there's only a couple of issues/questions to resolve from reviews. >> > > Oh I wasn't trying to imply making it a requirement of this series. Definitely > an additive thing, and I'm not even trying to pin you with the work, just > floating the general idea > Understood - I'd rather do it now than take a hit at some unknown future point in time to essentially do the same thing. In the long run it's the "right thing" to do. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] qemu: Clarify usage of DomainObjIsActive for SetTime
On 04/19/2016 10:30 AM, Ján Tomko wrote:
> On Fri, Apr 15, 2016 at 10:00:14AM -0400, Cole Robinson wrote:
>> This converts SetTime to the common obj->agent->isactive pattern
>> (which adds a _third_ IsActive check), and documents the extra
>> checks
>
> I would not expect a commit with the summary 'Clarify usage' to change
> job handling.
>
>
>> ---
>> src/qemu/qemu_driver.c | 16 +---
>> 1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index f8dfa27..43f22f1 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -18725,9 +18725,9 @@ qemuDomainSetTime(virDomainPtr dom,
>>
>> priv = vm->privateData;
>>
>> -if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
>> -goto cleanup;
>> -
>
> This will let us report the capability error without even getting the
> job.
>
>> +/* We also perform this check further down after grabbing the
>> + job lock, but do it here too so we can throw an error for
>> + an invalid config, before trying to talk to the guest agent */
>
> s/trying to talk to the guest agent/acquiring the job/
>
>> if (!virDomainObjIsActive(vm)) {
>> virReportError(VIR_ERR_OPERATION_INVALID,
>> "%s", _("domain is not running"));
>> @@ -18746,9 +18746,18 @@ qemuDomainSetTime(virDomainPtr dom,
>> goto endjob;
>
> This goto would end the job before it was even started.
>
>> }
>>
>> +if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
>> +goto cleanup;
>> +
>> if (!qemuDomainAgentAvailable(vm, true))
>> goto endjob;
>>
>> +if (!virDomainObjIsActive(vm)) {
>> +virReportError(VIR_ERR_OPERATION_INVALID,
>> + "%s", _("domain is not running"));
>> +goto endjob;
>> +}
>> +
>> qemuDomainObjEnterAgent(vm);
>> rv = qemuAgentSetTime(priv->agent, seconds, nseconds, rtcSync);
>> qemuDomainObjExitAgent(vm);
>> @@ -18756,6 +18765,7 @@ qemuDomainSetTime(virDomainPtr dom,
>> if (rv < 0)
>> goto endjob;
>>
>> +/* The VM may have shut down inbetween, check state again */
>
> This pattern is so common that commenting it is pointless.
> Maybe the IsActive check could be moved inside qemuDomainObjExitAgent
> as I've done for qemuDomainObjExitMonitor.
>
Thanks for your comments, I'll look into that
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/2] Explicitly error on uri=qemu://system
It's a fairly common error that a user tries to connect to a URI
like qemu://system or qemu://session (missing a slash). This errors
like:
$ virsh --connect qemu://session
error: failed to connect to the hypervisor
error: Unable to resolve address 'session' service '16514': No address
associated with hostname
If you already know that the standard qemu URI has 3 slashes, that
error will make it obvious enough. But new user's may not get it.
There's even a RHEL support page explicitly mentioning it!:
https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/7/html/Virtualization_Deployment_and_Administration_Guide/sect-Troubleshooting-Common_libvirt_errors_and_troubleshooting.html
Catch this error early in libvirt.c virConnectOpen for qemu (and vbox
which has similar rules
https://bugzilla.redhat.com/show_bug.cgi?id=1038304
---
v2:
Use conventional function naming
Improve a comment
Handle 'vz' driver
src/libvirt.c | 35 +++
1 file changed, 35 insertions(+)
diff --git a/src/libvirt.c b/src/libvirt.c
index a21d00e..919c9cb 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -928,6 +928,35 @@ virConnectGetDefaultURI(virConfPtr conf,
}
+/*
+ * Check to see if an invalid URI like qemu://system (missing /) was passed,
+ * offer the suggested fix.
+ */
+static int
+virConnectCheckURIMissingSlash(const char *uristr, virURIPtr uri)
+{
+/* To avoid false positives, only check drivers that mandate
+ a path component in the URI, like /system or /session */
+if (STRNEQ(uri->scheme, "qemu") &&
+STRNEQ(uri->scheme, "vbox") &&
+STRNEQ(uri->scheme, "vz"))
+return 0;
+
+if (uri->path != NULL)
+return 0;
+
+if (STREQ(uri->server, "session") ||
+STREQ(uri->server, "system")) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("invalid URI %s (maybe you want %s:///%s)"),
+ uristr, uri->scheme, uri->server);
+return -1;
+}
+
+return 0;
+}
+
+
static virConnectPtr
do_open(const char *name,
virConnectAuthPtr auth,
@@ -995,6 +1024,12 @@ do_open(const char *name,
NULLSTR(ret->uri->user), ret->uri->port,
NULLSTR(ret->uri->path));
+if (virConnectCheckURIMissingSlash(alias ? alias : name,
+ ret->uri) < 0) {
+VIR_FREE(alias);
+goto failed;
+}
+
VIR_FREE(alias);
} else {
VIR_DEBUG("no name, allowing driver auto-select");
--
2.7.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] libvirt: Use conventional function names
do_open and winsock_init don't follow the naming pattern of other
functions in this file. Rename them to match
---
src/libvirt.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c
index 919c9cb..749089b 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -257,7 +257,7 @@ virConnectAuthPtr virConnectAuthPtrDefault =
&virConnectAuthDefault;
#if HAVE_WINSOCK2_H
static int
-winsock_init(void)
+virWinsockInit(void)
{
WORD winsock_version, err;
WSADATA winsock_data;
@@ -386,7 +386,7 @@ virGlobalInit(void)
VIR_DEBUG("register drivers");
#if HAVE_WINSOCK2_H
-if (winsock_init() == -1)
+if (virWinsockInit() == -1)
goto error;
#endif
@@ -958,9 +958,9 @@ virConnectCheckURIMissingSlash(const char *uristr,
virURIPtr uri)
static virConnectPtr
-do_open(const char *name,
-virConnectAuthPtr auth,
-unsigned int flags)
+virConnectOpenInternal(const char *name,
+ virConnectAuthPtr auth,
+ unsigned int flags)
{
size_t i;
int res;
@@ -1161,7 +1161,7 @@ virConnectOpen(const char *name)
VIR_DEBUG("name=%s", NULLSTR(name));
virResetLastError();
-ret = do_open(name, NULL, 0);
+ret = virConnectOpenInternal(name, NULL, 0);
if (!ret)
goto error;
return ret;
@@ -1197,7 +1197,7 @@ virConnectOpenReadOnly(const char *name)
VIR_DEBUG("name=%s", NULLSTR(name));
virResetLastError();
-ret = do_open(name, NULL, VIR_CONNECT_RO);
+ret = virConnectOpenInternal(name, NULL, VIR_CONNECT_RO);
if (!ret)
goto error;
return ret;
@@ -1237,7 +1237,7 @@ virConnectOpenAuth(const char *name,
VIR_DEBUG("name=%s, auth=%p, flags=%x", NULLSTR(name), auth, flags);
virResetLastError();
-ret = do_open(name, auth, flags);
+ret = virConnectOpenInternal(name, auth, flags);
if (!ret)
goto error;
return ret;
--
2.7.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: process: comment on min_guarantee validation
On 04/19/2016 04:39 AM, Peter Krempa wrote:
> On Mon, Apr 18, 2016 at 19:13:08 -0400, Cole Robinson wrote:
>> Explain why we check it at process startup time, and not parse time
>> where most other XML validation checks are performed
>
> This is far from being a singular case ...
>
>> ---
>> src/qemu/qemu_process.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index c087300..628b4b6 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -4550,6 +4550,8 @@ qemuProcessStartValidate(virQEMUDriverPtr driver,
>> virDomainDefCheckDuplicateDiskInfo(vm->def) < 0)
>
> ... this is yet another example of a similar check.
>
>> return -1;
>>
>> +/* Previously we silently accepted this parameter; we can't reject
>> + it at parse time without breaking those configs, so check it here */
>
> I don't think this helps here and implies that other checks are not here
> due to that case. If you want to be explicit I think it warrants a
> separate function with this fact stated in it's comment. If you insist
> on being explicit in the purpose of this check
>
It's not a matter of 'insist'ing or not; when in this area of the code I saw
the min_guarantee check, which seemed out of place, since there are already
several such checks in the postparse handler.There's no comment explaining why
the min_guarantee check is there specifically (or the other XML checks), and
nothing specific in the commit message for the min_guarantee block. Hence my
confusion and desire to document it, and hopefully save other people the
confusion, and prevent future XML validation checks being added there which
are safe to do at parse time.
So it seems like the XML validation checks are:
if (qemuValidateCpuCount(vm->def, qemuCaps) < 0)
return -1;
if (!migration && !snapshot &&
virDomainDefCheckDuplicateDiskInfo(vm->def) < 0)
return -1;
if (vm->def->mem.min_guarantee) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Parameter 'min_guarantee' "
"not supported by QEMU."));
return -1;
}
And they are explicitly done at process startup time for back compat reasons.
So I'll stick them in a function like qemuProcessStartValidateXML(), with a
comment explaining why here and not at parse time. Sound good?
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] lxc: explicitly error on interface type=ethernet
On Mon, Apr 11, 2016 at 10:56:57AM -0400, Cole Robinson wrote: > It isn't implemented and does not work: > > error: internal error: guest failed to start: /usr/lib/libvirt/libvirt_lxc: > option '--veth' requires an argument > syntax: /usr/lib/libvirt/libvirt_lxc [OPTIONS] ... > > We previously threw an explicit error, but this changed in > 22cff52a2b8e06c913b1f97767e5d390fb17fc3b , which I suspect was > untested for LXC > --- > src/lxc/lxc_process.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c > index 5e0bbe2..0044ee5 100644 > --- a/src/lxc/lxc_process.c > +++ b/src/lxc/lxc_process.c > @@ -559,8 +559,6 @@ static int virLXCProcessSetupInterfaces(virConnectPtr > conn, > break; > > case VIR_DOMAIN_NET_TYPE_ETHERNET: > -break; > - > case VIR_DOMAIN_NET_TYPE_USER: > case VIR_DOMAIN_NET_TYPE_VHOSTUSER: > case VIR_DOMAIN_NET_TYPE_SERVER: ACK Regards, 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] Revert "daemon: use socket activation with systemd"
ping. Martin you had suggested removing the socket file in one of the bugs, are you cool with this? Thanks, Cole On 04/11/2016 07:08 PM, Cole Robinson wrote: > This reverts commit 1e9808d3a1e00a7121bae8b163d9c42d441d2ca8. > > We shouldn't advertise libvirtd.socket activation, since currently > it means VM/network/... autostart won't work as expected. > > We tried to find a middle ground by installing the config file without > an [Install] section, since systemd won't allow .socket to be enabled > without one... or at least it did do that; presently on f24 it allows > activating the socket quite happily. This also caused user confusion[1] > > Just remove the socket file. I've filed a new RFE to track coming up > with a solution to the autostart problem[2], we can point users at that > if there's more confusion: > > [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1279348 > [2]: https://bugzilla.redhat.com/show_bug.cgi?id=1326136 > --- > .gitignore | 1 - > daemon/Makefile.am | 14 ++ > daemon/libvirtd.conf | 5 - > daemon/libvirtd.service.in | 5 + > daemon/libvirtd.socket.in | 11 --- > libvirt.spec.in| 7 ++- > 6 files changed, 9 insertions(+), 34 deletions(-) > delete mode 100644 daemon/libvirtd.socket.in > > diff --git a/.gitignore b/.gitignore > index 0d12c5c..381db69 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -63,7 +63,6 @@ > /daemon/libvirtd.pod > /daemon/libvirtd.policy > /daemon/libvirtd.service > -/daemon/libvirtd.socket > /daemon/test_libvirtd.aug > /docs/aclperms.htmlinc > /docs/apibuild.py.stamp > diff --git a/daemon/Makefile.am b/daemon/Makefile.am > index 2dbe81b..fc6fd95 100644 > --- a/daemon/Makefile.am > +++ b/daemon/Makefile.am > @@ -59,7 +59,6 @@ EXTRA_DIST = > \ > libvirt.rules \ > libvirtd.sasl \ > libvirtd.service.in \ > - libvirtd.socket.in \ > libvirtd.sysconf\ > libvirtd.sysctl \ > libvirtd.aug\ > @@ -446,18 +445,15 @@ endif ! LIBVIRT_INIT_SCRIPT_UPSTART > if LIBVIRT_INIT_SCRIPT_SYSTEMD > > SYSTEMD_UNIT_DIR = $(prefix)/lib/systemd/system > -BUILT_SOURCES += libvirtd.service libvirtd.socket > +BUILT_SOURCES += libvirtd.service > > -install-init-systemd: install-sysconfig libvirtd.service libvirtd.socket > +install-init-systemd: install-sysconfig libvirtd.service > $(MKDIR_P) $(DESTDIR)$(SYSTEMD_UNIT_DIR) > $(INSTALL_DATA) libvirtd.service \ > $(DESTDIR)$(SYSTEMD_UNIT_DIR)/libvirtd.service > - $(INSTALL_DATA) libvirtd.socket \ > - $(DESTDIR)$(SYSTEMD_UNIT_DIR)/libvirtd.socket > > uninstall-init-systemd: uninstall-sysconfig > rm -f $(DESTDIR)$(SYSTEMD_UNIT_DIR)/libvirtd.service > - rm -f $(DESTDIR)$(SYSTEMD_UNIT_DIR)/libvirtd.socket > rmdir $(DESTDIR)$(SYSTEMD_UNIT_DIR) || : > else ! LIBVIRT_INIT_SCRIPT_SYSTEMD > install-init-systemd: > @@ -481,12 +477,6 @@ libvirtd.service: libvirtd.service.in > $(top_builddir)/config.status > < $< > $@-t && \ > mv $@-t $@ > > -libvirtd.socket: libvirtd.socket.in $(top_builddir)/config.status > - $(AM_V_GEN)sed \ > - -e 's|[@]runstatedir[@]|$(runstatedir)|g' \ > - < $< > $@-t && \ > - mv $@-t $@ > - > > check-local: check-augeas > > diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf > index 5485f98..d2c439c 100644 > --- a/daemon/libvirtd.conf > +++ b/daemon/libvirtd.conf > @@ -77,11 +77,6 @@ > # UNIX socket access controls > # > > -# Beware that if you are changing *any* of these options, and you use > -# socket activation with systemd, you need to adjust the settings in > -# the libvirtd.socket file as well since it could impose a security > -# risk if you rely on file permission checking only. > - > # Set the UNIX domain socket group ownership. This can be used to > # allow a 'trusted' set of users access to management capabilities > # without becoming root. > diff --git a/daemon/libvirtd.service.in b/daemon/libvirtd.service.in > index 608221c..1616e7a 100644 > --- a/daemon/libvirtd.service.in > +++ b/daemon/libvirtd.service.in > @@ -1,3 +1,8 @@ > +# NB we don't use socket activation. When libvirtd starts it will > +# spawn any virtual machines registered for autostart. We want this > +# to occur on every boot, regardless of whether any client connects > +# to a socket. Thus socket activation doesn't have any benefit > + > [Unit] > Description=Virtualization daemon > Before=libvirt-guests.service > diff --git a/daemon/libvirtd.socket.in b/daemon/libvirtd.socket.in > deleted file mode 100644 >
Re: [libvirt] [PATCH] lxc: explicitly error on interface type=ethernet
ping On 04/11/2016 10:56 AM, Cole Robinson wrote: > It isn't implemented and does not work: > > error: internal error: guest failed to start: /usr/lib/libvirt/libvirt_lxc: > option '--veth' requires an argument > syntax: /usr/lib/libvirt/libvirt_lxc [OPTIONS] ... > > We previously threw an explicit error, but this changed in > 22cff52a2b8e06c913b1f97767e5d390fb17fc3b , which I suspect was > untested for LXC > --- > src/lxc/lxc_process.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c > index 5e0bbe2..0044ee5 100644 > --- a/src/lxc/lxc_process.c > +++ b/src/lxc/lxc_process.c > @@ -559,8 +559,6 @@ static int virLXCProcessSetupInterfaces(virConnectPtr > conn, > break; > > case VIR_DOMAIN_NET_TYPE_ETHERNET: > -break; > - > case VIR_DOMAIN_NET_TYPE_USER: > case VIR_DOMAIN_NET_TYPE_VHOSTUSER: > case VIR_DOMAIN_NET_TYPE_SERVER: > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Explicitly error on uri=qemu://system
On 04/19/2016 03:59 AM, Maxim Nestratov wrote: > 19.04.2016 2:04, Cole Robinson пишет: > >> It's a fairly common error that a user tries to connect to a URI >> like qemu://system or qemu://session (missing a slash). This errors >> like: >> >> $ virsh --connect qemu://session >> error: failed to connect to the hypervisor >> error: Unable to resolve address 'session' service '16514': No address >> associated with hostname >> >> If you already know that the standard qemu URI has 3 slashes, that >> error will make it obvious enough. But new user's may not get it. >> There's even a RHEL support page explicitly mentioning it!: >> >> https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/7/html/Virtualization_Deployment_and_Administration_Guide/sect-Troubleshooting-Common_libvirt_errors_and_troubleshooting.html >> >> >> Catch this error early in libvirt.c virConnectOpen for qemu (and vbox >> which has similar rules > > Please, add 'vz' also as it suffers from the problem too. Though it doesn't > support vz:///session it clearly says about it in the error message. Sounds good, I'll send a v2 - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Explicitly error on uri=qemu://system
On 04/19/2016 04:30 AM, Peter Krempa wrote:
> On Mon, Apr 18, 2016 at 19:04:04 -0400, Cole Robinson wrote:
>> It's a fairly common error that a user tries to connect to a URI
>> like qemu://system or qemu://session (missing a slash). This errors
>> like:
>>
>> $ virsh --connect qemu://session
>> error: failed to connect to the hypervisor
>> error: Unable to resolve address 'session' service '16514': No address
>> associated with hostname
>>
>> If you already know that the standard qemu URI has 3 slashes, that
>> error will make it obvious enough. But new user's may not get it.
>> There's even a RHEL support page explicitly mentioning it!:
>>
>> https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/7/html/Virtualization_Deployment_and_Administration_Guide/sect-Troubleshooting-Common_libvirt_errors_and_troubleshooting.html
>>
>> Catch this error early in libvirt.c virConnectOpen for qemu (and vbox
>> which has similar rules
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1038304
>> ---
>> src/libvirt.c | 32
>> 1 file changed, 32 insertions(+)
>>
>> diff --git a/src/libvirt.c b/src/libvirt.c
>> index a21d00e..7607ae3 100644
>> --- a/src/libvirt.c
>> +++ b/src/libvirt.c
>> @@ -928,6 +928,33 @@ virConnectGetDefaultURI(virConfPtr conf,
>> }
>>
>>
>> +/*
>> + * Check to see if an invalid URI like qemu://system (missing /) was passed,
>> + * offer the suggested fix.
>> + */
>> +static int
>> +check_uri_missing_slash(const char *uristr, virURIPtr uri)
>
> Please use a name with "vir" prefix and camel case. This is totaly
> against our naming convention.
>
Yes I know, but I was just following the pattern of the do_open function
below. I'll change it to virConnectCheckURIMissingSlash
>> +{
>> +/* These drivers _only_ accepts URIs with a 'path' element */
>
> Only these drivers accept ... ? I don't quite follow the message of this
> comment.
>
I'll change it to
/* To avoid false positives, only check drivers that mandate
a path component in the URI, like /system or /session */
>> +if (STRNEQ(uri->sceme, "qemu") &&
>> +STRNEQ(uri->scheme, "vbox"))
>> +return 0;
>> +
>> +if (uri->path != NULL)
>> +return 0;
>> +
>> +if (STREQ(uri->server, "session") ||
>> +STREQ(uri->server, "system")) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("invalid URI %s (maybe you want %s:///%s)"),
>> + uristr, uri->scheme, uri->server);
>> +return -1;
>> +}
>> +
>> +return 0;
>> +}
>
> ACK with the rename and fix of the comment.
>
Thanks, I'll send a v2 though with the vz change
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-php][PATCH 2/3] Fix some leaks related to get_string_from_xpath
2016-04-19 17:59 GMT+03:00 Michal Privoznik : > Yeah, I could not agree more. But problem is that would be a great > patch. Moreover, xmlFree() == free() unless some debug is turned on in > libxml2 during its compilation. And since Vasiliy will make this > function go away, I'd rather push this as is. Ok -- Vasiliy Tolstov, e-mail: v.tols...@selfip.ru -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/10] secret: Move and rename secretLoadAllConfigs
On 04/19/2016 08:01 AM, John Ferlan wrote: > > > On 04/18/2016 05:48 PM, Cole Robinson wrote: >> On 03/02/2016 01:55 PM, John Ferlan wrote: >>> Move to secret_conf.c and rename to virSecretLoadAllConfigs. Also includes >>> moving/renaming the supporting virSecretLoad, virSecretLoadValue, and >>> virSecretLoadValidateUUID. >>> >>> Signed-off-by: John Ferlan >>> --- >>> src/conf/secret_conf.c | 175 >>> + >>> src/conf/secret_conf.h | 3 + >>> src/libvirt_private.syms | 1 + >>> src/secret/secret_driver.c | 174 >>> +--- >>> 4 files changed, 181 insertions(+), 172 deletions(-) >> >> ACK, mirrors network_conf.c layout. >> >> (though honestly I'd rather we have separate files for XML handling and >> object >> handling... the existing conf.c files are too large anyways. I put an entry >> on >> the LibvirtFirstBugs wiki page about that type of code reorg though it's >> probably over an initial contributors head) >> > > I could go the route of a "virsecretobj.c" (to partially mimic the > virdomainobjlist.c) instead of cramming everything into secret_conf - > there's about 1000 new lines in secret_conf just for this series (with > the reduction of lines in secret_driver). > > It would mean a complete v2 of this series - although considering most > things have been ACK'd the second review should be easier. I think > there's only a couple of issues/questions to resolve from reviews. > Oh I wasn't trying to imply making it a requirement of this series. Definitely an additive thing, and I'm not even trying to pin you with the work, just floating the general idea - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-php][PATCH 2/3] Fix some leaks related to get_string_from_xpath
On Tue, Apr 19, 2016 at 04:59:58PM +0200, Michal Privoznik wrote:
> On 19.04.2016 16:09, Christophe Fergeau wrote:
> > On Tue, Apr 19, 2016 at 03:46:41PM +0200, Michal Privoznik wrote:
> >> This function not only did not free return value of
> >> xmlNodeListGetString() it strdup()-ed its return value. Therefore
> >> plenty of memory has been lost definitely upon return from this
> >> function.
> >>
> >> Signed-off-by: Michal Privoznik
> >> ---
> >> src/libvirt-php.c | 110
> >> ++
> >> 1 file changed, 62 insertions(+), 48 deletions(-)
> >>
> >> diff --git a/src/libvirt-php.c b/src/libvirt-php.c
> >> index 308e764..fb0679b 100644
> >> --- a/src/libvirt-php.c
> >> +++ b/src/libvirt-php.c
> >> @@ -3232,19 +3232,17 @@ char *get_string_from_xpath(char *xml, char
> >> *xpath, zval **val, int *retVal)
> >> if (val != NULL) {
> >> ret = 0;
> >> for (i = 0; i < nodeset->nodeNr; i++) {
> >> -if (xmlNodeListGetString(doc,
> >> nodeset->nodeTab[i]->xmlChildrenNode, 1) != NULL) {
> >> -value = (char *)xmlNodeListGetString(doc,
> >> nodeset->nodeTab[i]->xmlChildrenNode, 1);
> >> -
> >> +if ((value = xmlNodeListGetString(doc,
> >> nodeset->nodeTab[i]->xmlChildrenNode, 1))) {
> >> snprintf(key, sizeof(key), "%d", i);
> >> VIRT_ADD_ASSOC_STRING(*val, key, value);
> >> +free(value);
> >> +value = NULL;
> >
> > 'value' is an xmlChar so must be freed with xmlFree. I'd change the
> > return value of get_string_from_xpath() to make it clear that xmlFree needs
> > to
> > be used to free it as well.
> >
>
> Yeah, I could not agree more. But problem is that would be a great
> patch. Moreover, xmlFree() == free() unless some debug is turned on in
> libxml2 during its compilation. And since Vasiliy will make this
> function go away, I'd rather push this as is.
If this goes away soon, and if the patch would be big, fine with me.
Christophe
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] RFC: Drop unmaintained hv drivers? (phyp, xenapi, hyperv)
On Mon, Apr 18, 2016 at 09:11:24AM -0400, Cole Robinson wrote: > On 04/18/2016 08:58 AM, Daniel P. Berrange wrote: > > On Mon, Apr 18, 2016 at 08:54:15AM -0400, Cole Robinson wrote: > >> On 04/18/2016 05:21 AM, Daniel P. Berrange wrote: > >>> On Fri, Apr 15, 2016 at 03:54:00PM -0400, Cole Robinson wrote: > >>> > >>> > >>> In general I'm not really in favour of dropping virt drivers for > >>> hypervisor > >>> platforms that still exist and have potential users, even if we don't hear > >>> much about them. I don't think any of these drivers is really giving us a > >>> significant maintenance headache to justify deletion. > >>> > >> > >> It's just kind of a weird situation. We advertise support for these > >> platforms, > >> but if someone tries them and finds them lacking and reports an issue here, > >> the only response they are going to get is 'no one has worked on it for > >> years, > >> no one here has a setup to test, sorry we cannot help you'. That's if they > >> get > >> a response at all. Seems bizarre to me... > > > > This is implying that because we don't have an active maintainer who will > > answer bug reports, then the driver is entirely useless to everybody. I > > find that really dubious, because it ignores the possibility that people > > are happily using the other parts of it that actually do work. Sure it > > would be nice if we have active maintainers, but I don't think the lack > > of an active maintainer means the driver is useless to all users. > > My paragraph didn't fully describe all the assumptions; Those three drivers > may have zero active users in the wild. Certainly there is no obvious evidence > to the contrary. The most damning bit seems to be that there hasn't been a > single targeted bug fix for those drivers in an average of 4 years; if someone > was actively using those drivers I'm pretty sure we would have received a > drive by patch at some point > > Maybe someone will chime in to prove me wrong, but what if the situation is > that there's no active maintainer, and no active users? Then it truly _is_ > serving no purpose, and IMO even advertising it is actively harmful since it > may lead users down a dead end path > > Removing a driver doesn't need to be the end either... if someone motivated > shows up they can always revive the old code As Jan points out though, that's not so easy in practice because the rest of libvirt continues evolving and so the old code won't be re-creatable without many fixups. With the current setup people who do refactoring take responsibility for fixing up all drivers. Given the number of drivers we have in tree, deleting a couple of drivers won't materially affect that effort. Someone new to libvirt though who tries to restore the old code will have to fight through all this subsequent refactoring with little domain knowledge to help them. I don't think that's really a positive tradeoff overall. > ...now that I do some more targeted searches looking for patches, looks like > someone from bull.net did try to massively expand the hyperv driver a few > years back, but it was an awkward code dump: > > https://www.redhat.com/archives/libvir-list/2014-October/msg00257.html Oh wow, that is a huge set of enhancements - its a real shame we did not apply those, as that'd be a big step forward for hyperv. I wonder how easily they apply today Regards, 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] [libvirt-php][PATCH 2/3] Fix some leaks related to get_string_from_xpath
On 19.04.2016 16:09, Christophe Fergeau wrote:
> On Tue, Apr 19, 2016 at 03:46:41PM +0200, Michal Privoznik wrote:
>> This function not only did not free return value of
>> xmlNodeListGetString() it strdup()-ed its return value. Therefore
>> plenty of memory has been lost definitely upon return from this
>> function.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/libvirt-php.c | 110
>> ++
>> 1 file changed, 62 insertions(+), 48 deletions(-)
>>
>> diff --git a/src/libvirt-php.c b/src/libvirt-php.c
>> index 308e764..fb0679b 100644
>> --- a/src/libvirt-php.c
>> +++ b/src/libvirt-php.c
>> @@ -3232,19 +3232,17 @@ char *get_string_from_xpath(char *xml, char *xpath,
>> zval **val, int *retVal)
>> if (val != NULL) {
>> ret = 0;
>> for (i = 0; i < nodeset->nodeNr; i++) {
>> -if (xmlNodeListGetString(doc,
>> nodeset->nodeTab[i]->xmlChildrenNode, 1) != NULL) {
>> -value = (char *)xmlNodeListGetString(doc,
>> nodeset->nodeTab[i]->xmlChildrenNode, 1);
>> -
>> +if ((value = xmlNodeListGetString(doc,
>> nodeset->nodeTab[i]->xmlChildrenNode, 1))) {
>> snprintf(key, sizeof(key), "%d", i);
>> VIRT_ADD_ASSOC_STRING(*val, key, value);
>> +free(value);
>> +value = NULL;
>
> 'value' is an xmlChar so must be freed with xmlFree. I'd change the
> return value of get_string_from_xpath() to make it clear that xmlFree needs to
> be used to free it as well.
>
Yeah, I could not agree more. But problem is that would be a great
patch. Moreover, xmlFree() == free() unless some debug is turned on in
libxml2 during its compilation. And since Vasiliy will make this
function go away, I'd rather push this as is.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-php][PATCH 2/3] Fix some leaks related to get_string_from_xpath
2016-04-19 17:51 GMT+03:00 Michal Privoznik : > Yeah, but I was just too lazy to do that :) If you can post a patch that > would be great! Any time estimate, e.g. should I postpone the release > for tomorrow? I think you can release not waiting for me, after release i prepare patches for cleanup old xml stuff. -- Vasiliy Tolstov, e-mail: v.tols...@selfip.ru -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] docs: Don't leave any documentation behind
On 04/19/2016 10:48 AM, Michal Privoznik wrote: > On 19.04.2016 16:38, John Ferlan wrote: >> >> >> On 04/19/2016 09:50 AM, Michal Privoznik wrote: >>> Our uninstall script is not exact counterpart of install one. >>> Therefore we are leaving couple of files behind. This should not >>> happen. >>> >>> Signed-off-by: Michal Privoznik >>> --- >>> docs/Makefile.am | 6 ++ >>> 1 file changed, 6 insertions(+) >>> >> >> At 'install-data-local:', there's a : >> >> $(mkinstalldirs) $(DESTDIR)$(HTML_DIR) >> >> why not just the far more all encompassing: >> >>rm -rf $(DESTDIR)$(HTML_DIR) >> >> and >> >>rm -rf $(DESTDIR)$(DEVHELP_DIR) >> >> Rather than picking each part we install to uninstall? and missing >> something in the future or even now. Do the 'html' or 'internals' >> directories gets removed? And then of course the toplevel directory >> which we created. >> >> IOW: There's no corollary for the: >> >> $(mkinstalldirs) $(DESTDIR)$(HTML_DIR) >> $(mkinstalldirs) $(DESTDIR)$(HTML_DIR)/html >> $(mkinstalldirs) $(DESTDIR)$(HTML_DIR)/internals >> $(mkinstalldirs) $(DESTDIR)$(DEVHELP_DIR) >> >> > > Yeah. That's the other way of doing that. It's just that if users put > anything in $(DESTDIR)$(HTML_DIR) it will be removed by uninstall. But I > can propose v2 if you want. > I see there are other 'rf -rm' usages in other "clean" labels... I don't have a strong feeling either way - perhaps there's other opinionated folks that would like to chime in. If no one chimes in, then I'm OK with what's here... I also now see there's "-rm " usages - so looks like it makes my comment in 1/4 unnecessary. It just looked strange to me... John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-php][PATCH 2/3] Fix some leaks related to get_string_from_xpath
On 19.04.2016 16:48, Vasiliy Tolstov wrote: > 2016-04-19 16:46 GMT+03:00 Michal Privoznik : >> This function not only did not free return value of >> xmlNodeListGetString() it strdup()-ed its return value. Therefore >> plenty of memory has been lost definitely upon return from this >> function. > > > I don't think that this function need at all. I'm switching to get > full xml of domain and doing xpath via native php code. > Yeah, but I was just too lazy to do that :) If you can post a patch that would be great! Any time estimate, e.g. should I postpone the release for tomorrow? Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] docs: Don't leave any documentation behind
On 19.04.2016 16:38, John Ferlan wrote: > > > On 04/19/2016 09:50 AM, Michal Privoznik wrote: >> Our uninstall script is not exact counterpart of install one. >> Therefore we are leaving couple of files behind. This should not >> happen. >> >> Signed-off-by: Michal Privoznik >> --- >> docs/Makefile.am | 6 ++ >> 1 file changed, 6 insertions(+) >> > > At 'install-data-local:', there's a : > > $(mkinstalldirs) $(DESTDIR)$(HTML_DIR) > > why not just the far more all encompassing: > >rm -rf $(DESTDIR)$(HTML_DIR) > > and > >rm -rf $(DESTDIR)$(DEVHELP_DIR) > > Rather than picking each part we install to uninstall? and missing > something in the future or even now. Do the 'html' or 'internals' > directories gets removed? And then of course the toplevel directory > which we created. > > IOW: There's no corollary for the: > > $(mkinstalldirs) $(DESTDIR)$(HTML_DIR) > $(mkinstalldirs) $(DESTDIR)$(HTML_DIR)/html > $(mkinstalldirs) $(DESTDIR)$(HTML_DIR)/internals > $(mkinstalldirs) $(DESTDIR)$(DEVHELP_DIR) > > Yeah. That's the other way of doing that. It's just that if users put anything in $(DESTDIR)$(HTML_DIR) it will be removed by uninstall. But I can propose v2 if you want. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-php][PATCH 2/3] Fix some leaks related to get_string_from_xpath
2016-04-19 16:46 GMT+03:00 Michal Privoznik : > This function not only did not free return value of > xmlNodeListGetString() it strdup()-ed its return value. Therefore > plenty of memory has been lost definitely upon return from this > function. I don't think that this function need at all. I'm switching to get full xml of domain and doing xpath via native php code. -- Vasiliy Tolstov, e-mail: v.tols...@selfip.ru -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] RFC: Drop unmaintained hv drivers? (phyp, xenapi, hyperv)
On Mon, Apr 18, 2016 at 09:11:24AM -0400, Cole Robinson wrote: > Maybe someone will chime in to prove me wrong, but what if the situation is > that there's no active maintainer, and no active users? Then it truly _is_ > serving no purpose, and IMO even advertising it is actively harmful since it > may lead users down a dead end path > > Removing a driver doesn't need to be the end either... if someone motivated > shows up they can always revive the old code > If the code is removed, it will no longer be kept up to date with internal libvirt APIs and concepts and latest compiler warning workarounds, so the work of getting it up to date would be on this motivated person. That could make the motivation disappear rather quickly :) Jan > ...now that I do some more targeted searches looking for patches, looks like > someone from bull.net did try to massively expand the hyperv driver a few > years back, but it was an awkward code dump: > > https://www.redhat.com/archives/libvir-list/2014-October/msg00257.html > > -- > 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 4/4] docs: Don't leave any documentation behind
On 04/19/2016 09:50 AM, Michal Privoznik wrote: > Our uninstall script is not exact counterpart of install one. > Therefore we are leaving couple of files behind. This should not > happen. > > Signed-off-by: Michal Privoznik > --- > docs/Makefile.am | 6 ++ > 1 file changed, 6 insertions(+) > At 'install-data-local:', there's a : $(mkinstalldirs) $(DESTDIR)$(HTML_DIR) why not just the far more all encompassing: rm -rf $(DESTDIR)$(HTML_DIR) and rm -rf $(DESTDIR)$(DEVHELP_DIR) Rather than picking each part we install to uninstall? and missing something in the future or even now. Do the 'html' or 'internals' directories gets removed? And then of course the toplevel directory which we created. IOW: There's no corollary for the: $(mkinstalldirs) $(DESTDIR)$(HTML_DIR) $(mkinstalldirs) $(DESTDIR)$(HTML_DIR)/html $(mkinstalldirs) $(DESTDIR)$(HTML_DIR)/internals $(mkinstalldirs) $(DESTDIR)$(DEVHELP_DIR) John > diff --git a/docs/Makefile.am b/docs/Makefile.am > index c80c37b..9524c94 100644 > --- a/docs/Makefile.am > +++ b/docs/Makefile.am > @@ -363,8 +363,14 @@ install-data-local: > $(INSTALL_DATA) $(srcdir)/libvirtLogo.png $(DESTDIR)$(pkgdatadir) > > uninstall-local: > + for f in $(css) $(dot_html) $(gif) $(png); do \ > + rm -f $(DESTDIR)$(HTML_DIR)/$$f; \ > + done > for h in $(apihtml); do rm -f $(DESTDIR)$(HTML_DIR)/$$h; done > for p in $(apipng); do rm -f $(DESTDIR)$(HTML_DIR)/$$p; done > + for f in $(internals_html); do \ > + rm -f $(DESTDIR)$(HTML_DIR)/$$f; \ > + done > for f in $(devhelphtml) $(devhelppng) $(devhelpcss); do \ > rm -f $(DESTDIR)$(DEVHELP_DIR)/$$(basename $$f); \ > done > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network: Add support for dhcp-range lease time in the network XML configuration format and dnsmasq
On Tue, Apr 19, 2016 at 2:34 AM, Laine Stump wrote:
> On 04/18/2016 06:52 PM, Cole Robinson wrote:
>
>> On 04/15/2016 08:18 PM, Alberto Ruiz wrote:
>>
>>> From 112f61ec5cfdc39f7a157825c4209f7bae34c483 Mon Sep 17 00:00:00 2001
>>> From: Alberto Ruiz
>>> Date: Wed, 13 Apr 2016 17:00:45 +0100
>>> Subject: [PATCH] network: Add support for dhcp-range lease time in the
>>> network
>>> XML configuration format and dnsmasq
>>>
>>> Also mention the bug in the commit message, just link it like
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=913446
>>
>> Needs documentation but that will be dependent on what the final patch
>> looks
>> like, so fine to skip for now.
>>
>> The main questions are:
>>
>> 1) is the XML format fine? . lease sounds kinda
>> non-specific to me, maybe leasetime or leaseTime.
>>
>> 2) what to use for the input format? right now it's just string
>> passthrough to
>> dnsmasq, which takes a format like XX[s|m|h|d|w], or 'infinite'. Accepting
>> that format kind of sticks us with that for all time, which probably
>> isn't a
>> good precedent. the easy way would probably be to just say the value
>> needs to
>> be in minutes, and maybe -1 == infinite. But that will take a bit more
>> code to
>> adapt that value to the dnsmasq format.
>>
>
> Yeah, you should never just read an opaque string and pass it directly
> through to dnsmasq. Instead, parse an integer (and whatever scaling info -
> hours / minutes / seconds - I know we do that for bytes vs kbytes vs KiB
> etc, and if we don't already have the same thing for times somewhere, we
> should), scale it, check the range for some amount of sanity, and convert
> that scaled integer into whatever dnsmasq wants when building the
> commandline.
Agreed. Will work on a second version of this patch with taking this into
account.
>
>
>
>> CC laining for his thoughts
>>
>
> Aside from the missing documentation that you pointed out (and that is
> just a pain to put in until the exact placement in the XML is figured out
> anyway), my main sticking point is having the lease time put as an
> attribute to each range. That just seems odd. I know that dnsmasq
> allows for specifying a lease time per range, but is that the case for
> other dhcp server implementations? (yeah, I know we don't support any other
> now, but someday we might :-). And even if dnsmasq *allows* it, unless
> you're using their tagging to put certain clients into certain IP ranges,
> there's no practical value in having a different lease time for each range.
> Maybe it should be an attribute of the element (or, to allow for
> different scaling, a subelement); every range on the dnsmasq commandline
> would just get the same lease time. Something like this:
>
>
> 3600
>
>
>
>
>
Sounds fair, and solves another issue I was hoping to discuss which is
per-host leasetime.
> If the need for per-range leasetime arose later, that could be added as a
> sub-element to that would override the leasetime directly under
> .
>
> (It's been at least 15 years since I used ISC's dhcpd, but I glanced at
> the config file manpage just now and it looks like it's possible to have a
> single "max-lease" that applies to all "pools" (their name for ranges) or
> to specify a separate max-lease for each pool. I admit I skipped 98% of the
> contents though :-)).
>
> In practice, I doubt there will be much difference between what you
> proposed and what I've suggested - probably 100% of the libvirt virtual
> networks in existence have only a single range anyway. I *think* splitting
> it out from the range could prevent us from being painted into a corner
> though.
>
> Aside from all that, thanks for taking the time to code this up!
No worries, my pleasure, will update the patch and get back to you as soon
as I can.
> And one tiny comment below:
>>
>> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
>>> index 4fb2e2a..449c9ed 100644
>>> --- a/src/conf/network_conf.c
>>> +++ b/src/conf/network_conf.c
>>> @@ -313,6 +313,10 @@ static void
>>> virNetworkIpDefClear(virNetworkIpDefPtr def)
>>> {
>>> VIR_FREE(def->family);
>>> +
>>> +while (def->nranges)
>>> +VIR_FREE(def->ranges[--def->nranges].lease);
>>> +
>>> VIR_FREE(def->ranges);
>>> while (def->nhosts)
>>> @@ -855,7 +859,6 @@ int virNetworkIpDefNetmask(const virNetworkIpDef
>>> *def,
>>>
>>> VIR_SOCKET_ADDR_FAMILY(&def->address));
>>> }
>>> -
>>>
>> stray whitespace change here
>>
>> - Cole
>>
>>
>
--
Alberto Ruiz
Associate Engineering Manager - Desktop Management Tools
Red Hat
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-php][PATCH 0/3] Couple of small fixes
On Tue, Apr 19, 2016 at 9:46 AM, Michal Privoznik wrote: > While playing with our php bindings I've noticed couple of memory > leaks mostly. And one bug in our examples. > > Michal Privoznik (3): > examples: Check properly if connected > Fix some leaks related to get_string_from_xpath > Revisit some VIRT_RETURN_STRING > > examples/index.php | 4 +- > src/libvirt-php.c | 121 > ++--- > 2 files changed, 72 insertions(+), 53 deletions(-) > > -- > 2.7.3 > For the patch set, I couldn't get the first one through the ML (for some reason, the ML never sent it to my inbox). The other two, however, made it and I've tested them. They applied cleanly, and libvirt-php built and ran cleanly on PHP5 and PHP7 on CentOS 7 and PHP7 on Ubuntu 16.04. Reading the first patch through the list archive, I manually applied and tested. Everything seems fine with it. For the patch series as a whole, I give it a thumbs up! :) -- 真実はいつも一つ！/ Always, there's only one truth! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network: Add support for dhcp-range lease time in the network XML configuration format and dnsmasq
On Mon, Apr 18, 2016 at 11:52 PM, Cole Robinson wrote:
> On 04/15/2016 08:18 PM, Alberto Ruiz wrote:
> > From 112f61ec5cfdc39f7a157825c4209f7bae34c483 Mon Sep 17 00:00:00 2001
> > From: Alberto Ruiz
> > Date: Wed, 13 Apr 2016 17:00:45 +0100
> > Subject: [PATCH] network: Add support for dhcp-range lease time in the
> network
> > XML configuration format and dnsmasq
> >
>
> Also mention the bug in the commit message, just link it like
>
> https://bugzilla.redhat.com/show_bug.cgi?id=913446
>
> Needs documentation but that will be dependent on what the final patch
> looks
> like, so fine to skip for now.
>
> The main questions are:
>
> 1) is the XML format fine? . lease sounds kinda
> non-specific to me, maybe leasetime or leaseTime.
>
Sounds good to me, though since pointed out in a later email, we might want
to make this a child tag on its own within instead of a per range
property.
> 2) what to use for the input format? right now it's just string
> passthrough to
> dnsmasq, which takes a format like XX[s|m|h|d|w], or 'infinite'. Accepting
> that format kind of sticks us with that for all time, which probably isn't
> a
> good precedent. the easy way would probably be to just say the value needs
> to
> be in minutes, and maybe -1 == infinite. But that will take a bit more
> code to
> adapt that value to the dnsmasq format.
>
Sticking to minutes seems like a loss of granularity, we can't really
predict that a given setup might care about second level granularity for
leases can we? I'm all for the argument of sanity checking the input and
not doing an opaque passthrough, but I think in the end we might want to
support some sort of seconds/minutes/hours/days notation (or stick to
seconds and let people figure the amount out with a calculator).
Thanks a lot for the input.
> CC laining for his thoughts
>
> And one tiny comment below:
>
> > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> > index 4fb2e2a..449c9ed 100644
> > --- a/src/conf/network_conf.c
> > +++ b/src/conf/network_conf.c
> > @@ -313,6 +313,10 @@ static void
> > virNetworkIpDefClear(virNetworkIpDefPtr def)
> > {
> > VIR_FREE(def->family);
> > +
> > +while (def->nranges)
> > +VIR_FREE(def->ranges[--def->nranges].lease);
> > +
> > VIR_FREE(def->ranges);
> >
> > while (def->nhosts)
> > @@ -855,7 +859,6 @@ int virNetworkIpDefNetmask(const virNetworkIpDef
> *def,
> >
> VIR_SOCKET_ADDR_FAMILY(&def->address));
> > }
> >
> > -
>
> stray whitespace change here
>
> - Cole
>
--
Alberto Ruiz
Associate Engineering Manager - Desktop Management Tools
Red Hat
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-php][PATCH 3/3] Revisit some VIRT_RETURN_STRING
On Tue, Apr 19, 2016 at 9:46 AM, Michal Privoznik wrote:
> This macro sets return value and returns immediately. This can
> lead to a possible leak because any free() that would come
> afterwards is in fact never called.
>
> Signed-off-by: Michal Privoznik
> ---
> src/libvirt-php.c | 11 +++
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/src/libvirt-php.c b/src/libvirt-php.c
> index fb0679b..6f6e137 100644
> --- a/src/libvirt-php.c
> +++ b/src/libvirt-php.c
> @@ -4307,7 +4307,8 @@ PHP_FUNCTION(libvirt_domain_qemu_agent_command)
> ret = virDomainQemuAgentCommand(domain->domain, cmd, timeout, flags);
> if (ret == NULL) RETURN_FALSE;
>
> - VIRT_RETURN_STRING(ret);
> + VIRT_RETVAL_STRING(ret);
> + free(ret);
> }
>
> /*
> @@ -6691,7 +6692,7 @@ PHP_FUNCTION(libvirt_domain_memory_peek)
> buff=(char *)emalloc(size);
> retval=virDomainMemoryPeek(domain->domain,start,size,buff,flags);
> if (retval != 0) RETURN_FALSE;
> -VIRT_RETURN_STRINGL(buff, size);
> +VIRT_RETVAL_STRINGL(buff, size);
> efree(buff);
> }
>
> @@ -7872,7 +7873,8 @@ PHP_FUNCTION(libvirt_storagevolume_get_path)
> DPRINTF("%s: virStorageVolGetPath(%p) returned %s\n", PHPFUNC,
> volume->volume, retval);
> if (retval == NULL) RETURN_FALSE;
>
> -VIRT_RETURN_STRING(retval);
> +VIRT_RETVAL_STRING(retval);
> +free(retval);
> }
>
> /*
> @@ -9506,7 +9508,8 @@ PHP_FUNCTION(libvirt_network_get_bridge)
> RETURN_FALSE;
> }
>
> -VIRT_RETURN_STRING(name);
> +VIRT_RETVAL_STRING(name);
> +free(name);
> }
>
> /*
> --
> 2.7.3
>
Applied locally and tested. Works okay for me. :)
--
真実はいつも一つ！/ Always, there's only one truth!
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] qemu: Clarify usage of DomainObjIsActive for SetTime
On Fri, Apr 15, 2016 at 10:00:14AM -0400, Cole Robinson wrote:
> This converts SetTime to the common obj->agent->isactive pattern
> (which adds a _third_ IsActive check), and documents the extra
> checks
I would not expect a commit with the summary 'Clarify usage' to change
job handling.
> ---
> src/qemu/qemu_driver.c | 16 +---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f8dfa27..43f22f1 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18725,9 +18725,9 @@ qemuDomainSetTime(virDomainPtr dom,
>
> priv = vm->privateData;
>
> -if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> -goto cleanup;
> -
This will let us report the capability error without even getting the
job.
> +/* We also perform this check further down after grabbing the
> + job lock, but do it here too so we can throw an error for
> + an invalid config, before trying to talk to the guest agent */
s/trying to talk to the guest agent/acquiring the job/
> if (!virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> "%s", _("domain is not running"));
> @@ -18746,9 +18746,18 @@ qemuDomainSetTime(virDomainPtr dom,
> goto endjob;
This goto would end the job before it was even started.
> }
>
> +if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> +goto cleanup;
> +
> if (!qemuDomainAgentAvailable(vm, true))
> goto endjob;
>
> +if (!virDomainObjIsActive(vm)) {
> +virReportError(VIR_ERR_OPERATION_INVALID,
> + "%s", _("domain is not running"));
> +goto endjob;
> +}
> +
> qemuDomainObjEnterAgent(vm);
> rv = qemuAgentSetTime(priv->agent, seconds, nseconds, rtcSync);
> qemuDomainObjExitAgent(vm);
> @@ -18756,6 +18765,7 @@ qemuDomainSetTime(virDomainPtr dom,
> if (rv < 0)
> goto endjob;
>
> +/* The VM may have shut down inbetween, check state again */
This pattern is so common that commenting it is pointless.
Maybe the IsActive check could be moved inside qemuDomainObjExitAgent
as I've done for qemuDomainObjExitMonitor.
Jan
> if (!virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> "%s", _("domain is not running"));
> --
> 2.7.3
>
> --
> 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] [libvirt-php][PATCH 2/3] Fix some leaks related to get_string_from_xpath
On Tue, Apr 19, 2016 at 9:46 AM, Michal Privoznik wrote:
> This function not only did not free return value of
> xmlNodeListGetString() it strdup()-ed its return value. Therefore
> plenty of memory has been lost definitely upon return from this
> function.
>
> Signed-off-by: Michal Privoznik
> ---
> src/libvirt-php.c | 110
> ++
> 1 file changed, 62 insertions(+), 48 deletions(-)
>
> diff --git a/src/libvirt-php.c b/src/libvirt-php.c
> index 308e764..fb0679b 100644
> --- a/src/libvirt-php.c
> +++ b/src/libvirt-php.c
> @@ -3232,19 +3232,17 @@ char *get_string_from_xpath(char *xml, char *xpath,
> zval **val, int *retVal)
> if (val != NULL) {
> ret = 0;
> for (i = 0; i < nodeset->nodeNr; i++) {
> -if (xmlNodeListGetString(doc,
> nodeset->nodeTab[i]->xmlChildrenNode, 1) != NULL) {
> -value = (char *)xmlNodeListGetString(doc,
> nodeset->nodeTab[i]->xmlChildrenNode, 1);
> -
> +if ((value = xmlNodeListGetString(doc,
> nodeset->nodeTab[i]->xmlChildrenNode, 1))) {
> snprintf(key, sizeof(key), "%d", i);
> VIRT_ADD_ASSOC_STRING(*val, key, value);
> +free(value);
> +value = NULL;
> ret++;
> }
> }
> add_assoc_long(*val, "num", (long)ret);
> -}
> -else {
> -if (xmlNodeListGetString(doc, nodeset->nodeTab[0]->xmlChildrenNode,
> 1) != NULL)
> -value = (char *)xmlNodeListGetString(doc,
> nodeset->nodeTab[0]->xmlChildrenNode, 1);
> +} else {
> +value = xmlNodeListGetString(doc,
> nodeset->nodeTab[0]->xmlChildrenNode, 1);
> }
>
> cleanup:
> @@ -3255,7 +3253,7 @@ char *get_string_from_xpath(char *xml, char *xpath,
> zval **val, int *retVal)
> xmlFreeParserCtxt(xp);
> xmlFreeDoc(doc);
> xmlCleanupParser();
> -return (value != NULL) ? strdup(value) : NULL;
> +return value;
> }
>
> /*
> @@ -3327,11 +3325,8 @@ char **get_array_from_xpath(char *xml, char *xpath,
> int *num)
> ret = 0;
> val = (char **)malloc( nodeset->nodeNr * sizeof(char *) );
> for (i = 0; i < nodeset->nodeNr; i++) {
> -if (xmlNodeListGetString(doc, nodeset->nodeTab[i]->xmlChildrenNode,
> 1) != NULL) {
> -value = (char *)xmlNodeListGetString(doc,
> nodeset->nodeTab[i]->xmlChildrenNode, 1);
> -
> -val[ret++] = strdup(value);
> -}
> +if ((value = xmlNodeListGetString(doc,
> nodeset->nodeTab[i]->xmlChildrenNode, 1)))
> +val[ret++] = value;
> }
>
> xmlXPathFreeContext(context);
> @@ -3506,20 +3501,23 @@ long get_next_free_numeric_value(virDomainPtr domain,
> char *xpath)
> */
> char *connection_get_domain_type(virConnectPtr conn, char *arch TSRMLS_DC)
> {
> -int retval = -1;
> +char *ret = NULL;
> char *tmp = NULL;
> char *caps = NULL;
> +char *tmpArch = NULL;
> char xpath[1024] = { 0 };
> +int retval = -1;
>
> caps = virConnectGetCapabilities(conn);
> if (caps == NULL)
> return NULL;
>
> if (arch == NULL) {
> -arch = get_string_from_xpath(caps, "//capabilities/host/cpu/arch",
> NULL, &retval);
> -DPRINTF("%s: No architecture defined, got '%s' from capabilities
> XML\n", __FUNCTION__, arch);
> -if ((arch == NULL) || (retval < 0))
> -return NULL;
> +tmpArch = get_string_from_xpath(caps,
> "//capabilities/host/cpu/arch", NULL, &retval);
> +DPRINTF("%s: No architecture defined, got '%s' from capabilities
> XML\n", __FUNCTION__, tmpArch);
> +if (!tmpArch || retval < 0)
> +goto cleanup;
> +arch = tmpArch;
> }
>
> DPRINTF("%s: Requested domain type for arch '%s'\n", __FUNCTION__,
> arch);
> @@ -3529,12 +3527,17 @@ char *connection_get_domain_type(virConnectPtr conn,
> char *arch TSRMLS_DC)
> tmp = get_string_from_xpath(caps, xpath, NULL, &retval);
> if ((tmp == NULL) || (retval < 0)) {
> DPRINTF("%s: No domain type found in XML...\n", __FUNCTION__);
> -return NULL;
> +goto cleanup;
> }
>
> -DPRINTF("%s: Domain type is '%s'\n", __FUNCTION__, tmp);
> -
> -return tmp;
> +ret = tmp;
> +tmp = NULL;
> +DPRINTF("%s: Domain type is '%s'\n", __FUNCTION__, ret);
> + cleanup:
> +free(tmpArch);
> +free(caps);
> +free(tmp);
> +return ret;
> }
>
> /*
> @@ -3547,20 +3550,23 @@ char *connection_get_domain_type(virConnectPtr conn,
> char *arch TSRMLS_DC)
> */
> char *connection_get_emulator(virConnectPtr conn, char *arch TSRMLS_DC)
> {
> -int retval = -1;
> +char *ret = NULL;
> char *tmp = NULL;
> char *caps = NULL;
> +char *tmpArch = NULL;
> char xpath[1024] = { 0 };
> +int retval = -1;
>
> caps = virConnectGetCapabilities(conn);
> if (caps == NULL)
> return NULL;
>
> if (arch == NULL) {
> -arc
Re: [libvirt] [PATCH 3/4] docs: Uninstall libvirt logo too
On 04/19/2016 09:50 AM, Michal Privoznik wrote: > While we could leave it behind as an indelible sign that libvirt > has been running on host, other users might not be that fond of > it. > > Signed-off-by: Michal Privoznik > --- > docs/Makefile.am | 1 + > 1 file changed, 1 insertion(+) > ACK John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] examples: Try harder to uninstall nwfilter
On 04/19/2016 09:50 AM, Michal Privoznik wrote: > We have this code in our Makefile that tries to remove > /etc/libvirt/nwfilter if directory is left empty after all our > example nwfilters were uninstalled. However, the check for that > is missing quotation marks thus rendering the test useless: > > test -z allow-arp.xml allow-dhcp-server.xml .. qemu-announce-self.xml || \ > rmdir "/some/path/libvirt.git/_install/etc/libvirt/nwfilter" > /bin/sh: line 0: test: too many arguments > > Signed-off-by: Michal Privoznik > --- > examples/Makefile.am | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > ACK John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] qemu: Clarify usage of DomainObjIsActive for PMSuspended
On Fri, Apr 15, 2016 at 10:00:13AM -0400, Cole Robinson wrote:
> ---
> src/qemu/qemu_driver.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9a3d46b..f8dfa27 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18237,6 +18237,9 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
> if (virDomainPMSuspendForDurationEnsureACL(dom->conn, vm->def) < 0)
> goto cleanup;
>
> +/* We also perform this check further down after grabbing the
> + job lock, but do it here too so we can throw an error for
> + an invalid config, before trying to talk to the guest agent */
This is wrong.
Even without this check, we would not talk to the guest agent.
The real reason for this check is that for an inactive domain,
priv->qemuCaps is NULL and we would report QEMU_CAPS_WAKEUP as missing.
Jan
> if (!virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> "%s", _("domain is not running"));
> --
> 2.7.3
>
> --
> 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 1/4] nss: Try harder to uninstall
On 04/19/2016 09:50 AM, Michal Privoznik wrote: > On BSD we are creating this symlink to libnss_libvirt.so called > nss_libvirt.so. That's just the way it is on BSD. However, when > uninstalling, we try to remove libnss_libvirt.so instead of the > symlink. Moreover, if file we are trying to remove does not exist > we error out instead of ignoring the error. > > Signed-off-by: Michal Privoznik > --- > tools/Makefile.am | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/Makefile.am b/tools/Makefile.am > index 560a9a5..c474d75 100644 > --- a/tools/Makefile.am > +++ b/tools/Makefile.am > @@ -427,7 +427,7 @@ install-exec-hook: > $(LN_S) libnss_libvirt.so.$(NSS_SO_VER) nss_libvirt.so.$(NSS_SO_VER) > > uninstall-local: > - rm $(DESTDIR)$(libdir)/libnss_libvirt.so.$(NSS_SO_VER) > + -rm -f $(DESTDIR)$(libdir)/nss_libvirt.so.$(NSS_SO_VER) ^ Looks like a classic cut-n-paste error ACK w/ the adjustment John > else ! WITH_BSD_NSS > LIBVIRT_NSS_SYMBOL_FILE = \ > $(srcdir)/nss/libvirt_nss.syms > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-php][PATCH 2/3] Fix some leaks related to get_string_from_xpath
On Tue, Apr 19, 2016 at 03:46:41PM +0200, Michal Privoznik wrote:
> This function not only did not free return value of
> xmlNodeListGetString() it strdup()-ed its return value. Therefore
> plenty of memory has been lost definitely upon return from this
> function.
>
> Signed-off-by: Michal Privoznik
> ---
> src/libvirt-php.c | 110
> ++
> 1 file changed, 62 insertions(+), 48 deletions(-)
>
> diff --git a/src/libvirt-php.c b/src/libvirt-php.c
> index 308e764..fb0679b 100644
> --- a/src/libvirt-php.c
> +++ b/src/libvirt-php.c
> @@ -3232,19 +3232,17 @@ char *get_string_from_xpath(char *xml, char *xpath,
> zval **val, int *retVal)
> if (val != NULL) {
> ret = 0;
> for (i = 0; i < nodeset->nodeNr; i++) {
> -if (xmlNodeListGetString(doc,
> nodeset->nodeTab[i]->xmlChildrenNode, 1) != NULL) {
> -value = (char *)xmlNodeListGetString(doc,
> nodeset->nodeTab[i]->xmlChildrenNode, 1);
> -
> +if ((value = xmlNodeListGetString(doc,
> nodeset->nodeTab[i]->xmlChildrenNode, 1))) {
> snprintf(key, sizeof(key), "%d", i);
> VIRT_ADD_ASSOC_STRING(*val, key, value);
> +free(value);
> +value = NULL;
'value' is an xmlChar so must be freed with xmlFree. I'd change the
return value of get_string_from_xpath() to make it clear that xmlFree needs to
be used to free it as well.
> ret++;
> }
> }
> add_assoc_long(*val, "num", (long)ret);
> -}
> -else {
> -if (xmlNodeListGetString(doc, nodeset->nodeTab[0]->xmlChildrenNode,
> 1) != NULL)
> -value = (char *)xmlNodeListGetString(doc,
> nodeset->nodeTab[0]->xmlChildrenNode, 1);
> +} else {
> +value = xmlNodeListGetString(doc,
> nodeset->nodeTab[0]->xmlChildrenNode, 1);
> }
>
> cleanup:
> @@ -3255,7 +3253,7 @@ char *get_string_from_xpath(char *xml, char *xpath,
> zval **val, int *retVal)
> xmlFreeParserCtxt(xp);
> xmlFreeDoc(doc);
> xmlCleanupParser();
> -return (value != NULL) ? strdup(value) : NULL;
> +return value;
> }
>
> /*
> @@ -3327,11 +3325,8 @@ char **get_array_from_xpath(char *xml, char *xpath,
> int *num)
> ret = 0;
> val = (char **)malloc( nodeset->nodeNr * sizeof(char *) );
> for (i = 0; i < nodeset->nodeNr; i++) {
> -if (xmlNodeListGetString(doc, nodeset->nodeTab[i]->xmlChildrenNode,
> 1) != NULL) {
> -value = (char *)xmlNodeListGetString(doc,
> nodeset->nodeTab[i]->xmlChildrenNode, 1);
> -
> -val[ret++] = strdup(value);
> -}
> +if ((value = xmlNodeListGetString(doc,
> nodeset->nodeTab[i]->xmlChildrenNode, 1)))
> +val[ret++] = value;
> }
>
> xmlXPathFreeContext(context);
> @@ -3506,20 +3501,23 @@ long get_next_free_numeric_value(virDomainPtr domain,
> char *xpath)
> */
> char *connection_get_domain_type(virConnectPtr conn, char *arch TSRMLS_DC)
> {
> -int retval = -1;
> +char *ret = NULL;
> char *tmp = NULL;
> char *caps = NULL;
> +char *tmpArch = NULL;
> char xpath[1024] = { 0 };
> +int retval = -1;
>
> caps = virConnectGetCapabilities(conn);
> if (caps == NULL)
> return NULL;
>
> if (arch == NULL) {
> -arch = get_string_from_xpath(caps, "//capabilities/host/cpu/arch",
> NULL, &retval);
> -DPRINTF("%s: No architecture defined, got '%s' from capabilities
> XML\n", __FUNCTION__, arch);
> -if ((arch == NULL) || (retval < 0))
> -return NULL;
> +tmpArch = get_string_from_xpath(caps,
> "//capabilities/host/cpu/arch", NULL, &retval);
> +DPRINTF("%s: No architecture defined, got '%s' from capabilities
> XML\n", __FUNCTION__, tmpArch);
> +if (!tmpArch || retval < 0)
> +goto cleanup;
> +arch = tmpArch;
> }
>
> DPRINTF("%s: Requested domain type for arch '%s'\n", __FUNCTION__,
> arch);
> @@ -3529,12 +3527,17 @@ char *connection_get_domain_type(virConnectPtr conn,
> char *arch TSRMLS_DC)
> tmp = get_string_from_xpath(caps, xpath, NULL, &retval);
> if ((tmp == NULL) || (retval < 0)) {
> DPRINTF("%s: No domain type found in XML...\n", __FUNCTION__);
> -return NULL;
> +goto cleanup;
> }
>
> -DPRINTF("%s: Domain type is '%s'\n", __FUNCTION__, tmp);
> -
> -return tmp;
> +ret = tmp;
> +tmp = NULL;
> +DPRINTF("%s: Domain type is '%s'\n", __FUNCTION__, ret);
> + cleanup:
> +free(tmpArch);
> +free(caps);
> +free(tmp);
tmp/tmpArch should be freed xmlFree.
(and I assume more of the same below).
Christophe
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] qemu: Remove redundant DomainObjIsActive calls
On Fri, Apr 15, 2016 at 10:00:12AM -0400, Cole Robinson wrote: > The common idiom in the driver API implementations is roughly: > > - ACL check > - BeginJob (if needed) > - AgentAvailable (if needed) > - !IsActive > > A few calls had an extra !IsActive before BeginJob, which doesn't > seem to serve much use. Drop them > --- > src/qemu/qemu_driver.c | 24 > 1 file changed, 24 deletions(-) > ACK The call would help us error out early if: * the domain is not alive AND * we would not be able to get a job on it But we already acquired the domain object lock in qemuDomObjFromDomain. This means that either: * there is no job on the domain OR * there is a job that does unlock the domain object Since a job on inactive domain has no reason to unlock the object to enter the monitor or agent, this extra early call is unlikely to protect us from any unnecessary waiting for the job. Jan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/4] nss: Try harder to uninstall
On BSD we are creating this symlink to libnss_libvirt.so called nss_libvirt.so. That's just the way it is on BSD. However, when uninstalling, we try to remove libnss_libvirt.so instead of the symlink. Moreover, if file we are trying to remove does not exist we error out instead of ignoring the error. Signed-off-by: Michal Privoznik --- tools/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/Makefile.am b/tools/Makefile.am index 560a9a5..c474d75 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -427,7 +427,7 @@ install-exec-hook: $(LN_S) libnss_libvirt.so.$(NSS_SO_VER) nss_libvirt.so.$(NSS_SO_VER) uninstall-local: - rm $(DESTDIR)$(libdir)/libnss_libvirt.so.$(NSS_SO_VER) + -rm -f $(DESTDIR)$(libdir)/nss_libvirt.so.$(NSS_SO_VER) else ! WITH_BSD_NSS LIBVIRT_NSS_SYMBOL_FILE = \ $(srcdir)/nss/libvirt_nss.syms -- 2.7.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/4] docs: Don't leave any documentation behind
Our uninstall script is not exact counterpart of install one. Therefore we are leaving couple of files behind. This should not happen. Signed-off-by: Michal Privoznik --- docs/Makefile.am | 6 ++ 1 file changed, 6 insertions(+) diff --git a/docs/Makefile.am b/docs/Makefile.am index c80c37b..9524c94 100644 --- a/docs/Makefile.am +++ b/docs/Makefile.am @@ -363,8 +363,14 @@ install-data-local: $(INSTALL_DATA) $(srcdir)/libvirtLogo.png $(DESTDIR)$(pkgdatadir) uninstall-local: + for f in $(css) $(dot_html) $(gif) $(png); do \ + rm -f $(DESTDIR)$(HTML_DIR)/$$f; \ + done for h in $(apihtml); do rm -f $(DESTDIR)$(HTML_DIR)/$$h; done for p in $(apipng); do rm -f $(DESTDIR)$(HTML_DIR)/$$p; done + for f in $(internals_html); do \ + rm -f $(DESTDIR)$(HTML_DIR)/$$f; \ + done for f in $(devhelphtml) $(devhelppng) $(devhelpcss); do \ rm -f $(DESTDIR)$(DEVHELP_DIR)/$$(basename $$f); \ done -- 2.7.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/4] docs: Uninstall libvirt logo too
While we could leave it behind as an indelible sign that libvirt has been running on host, other users might not be that fond of it. Signed-off-by: Michal Privoznik --- docs/Makefile.am | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/Makefile.am b/docs/Makefile.am index f4ce9ce..c80c37b 100644 --- a/docs/Makefile.am +++ b/docs/Makefile.am @@ -368,3 +368,4 @@ uninstall-local: for f in $(devhelphtml) $(devhelppng) $(devhelpcss); do \ rm -f $(DESTDIR)$(DEVHELP_DIR)/$$(basename $$f); \ done + rm -f $(DESTDIR)$(pkgdatadir)/libvirtLogo.png -- 2.7.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/4] Make uninstall clean again
It's been a while since 'make uninstall' cleaned up everything that 'make install' created. I've noticed this while trying to figure out why some build test on *BSD is failing (fix for that is in the first patch btw). Michal Privoznik (4): nss: Try harder to uninstall examples: Try harder to uninstall nwfilter docs: Uninstall libvirt logo too docs: Don't leave any documentation behind docs/Makefile.am | 7 +++ examples/Makefile.am | 2 +- tools/Makefile.am| 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) -- 2.7.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/4] examples: Try harder to uninstall nwfilter
We have this code in our Makefile that tries to remove /etc/libvirt/nwfilter if directory is left empty after all our example nwfilters were uninstalled. However, the check for that is missing quotation marks thus rendering the test useless: test -z allow-arp.xml allow-dhcp-server.xml .. qemu-announce-self.xml || \ rmdir "/some/path/libvirt.git/_install/etc/libvirt/nwfilter" /bin/sh: line 0: test: too many arguments Signed-off-by: Michal Privoznik --- examples/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/Makefile.am b/examples/Makefile.am index 46465f9..e1c37f0 100644 --- a/examples/Makefile.am +++ b/examples/Makefile.am @@ -90,5 +90,5 @@ uninstall-local:: for f in $(FILTERS); do \ rm -f "$(NWFILTER_DIR)/`basename $$f`"; \ done - -test -z $(shell ls $(NWFILTER_DIR)) || rmdir $(NWFILTER_DIR) + -test -z "$(shell ls $(NWFILTER_DIR))" || rmdir $(NWFILTER_DIR) endif WITH_NWFILTER -- 2.7.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt-php][PATCH 1/3] examples: Check properly if connected
So far, we connect and issue an API immediately. And only after
it fails we assume we're not connected. This is just not right
as we should have checked before issuing the API whether we are
connected at all.
Signed-off-by: Michal Privoznik
---
examples/index.php | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/examples/index.php b/examples/index.php
index 381baef..b2f5b69 100644
--- a/examples/index.php
+++ b/examples/index.php
@@ -1,9 +1,11 @@
Cannot open connection to
hypervisor');
$hn = $lv->get_hostname();
if ($hn == false)
- die('Cannot open connection to hypervisor');
+ die('Cannot get hostname');
$action = array_key_exists('action', $_GET) ? $_GET['action'] : false;
$subaction = array_key_exists('subaction', $_GET) ? $_GET['subaction']
: false;
--
2.7.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt-php][PATCH 3/3] Revisit some VIRT_RETURN_STRING
This macro sets return value and returns immediately. This can
lead to a possible leak because any free() that would come
afterwards is in fact never called.
Signed-off-by: Michal Privoznik
---
src/libvirt-php.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/libvirt-php.c b/src/libvirt-php.c
index fb0679b..6f6e137 100644
--- a/src/libvirt-php.c
+++ b/src/libvirt-php.c
@@ -4307,7 +4307,8 @@ PHP_FUNCTION(libvirt_domain_qemu_agent_command)
ret = virDomainQemuAgentCommand(domain->domain, cmd, timeout, flags);
if (ret == NULL) RETURN_FALSE;
- VIRT_RETURN_STRING(ret);
+ VIRT_RETVAL_STRING(ret);
+ free(ret);
}
/*
@@ -6691,7 +6692,7 @@ PHP_FUNCTION(libvirt_domain_memory_peek)
buff=(char *)emalloc(size);
retval=virDomainMemoryPeek(domain->domain,start,size,buff,flags);
if (retval != 0) RETURN_FALSE;
-VIRT_RETURN_STRINGL(buff, size);
+VIRT_RETVAL_STRINGL(buff, size);
efree(buff);
}
@@ -7872,7 +7873,8 @@ PHP_FUNCTION(libvirt_storagevolume_get_path)
DPRINTF("%s: virStorageVolGetPath(%p) returned %s\n", PHPFUNC,
volume->volume, retval);
if (retval == NULL) RETURN_FALSE;
-VIRT_RETURN_STRING(retval);
+VIRT_RETVAL_STRING(retval);
+free(retval);
}
/*
@@ -9506,7 +9508,8 @@ PHP_FUNCTION(libvirt_network_get_bridge)
RETURN_FALSE;
}
-VIRT_RETURN_STRING(name);
+VIRT_RETVAL_STRING(name);
+free(name);
}
/*
--
2.7.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt-php][PATCH 0/3] Couple of small fixes
While playing with our php bindings I've noticed couple of memory leaks mostly. And one bug in our examples. Michal Privoznik (3): examples: Check properly if connected Fix some leaks related to get_string_from_xpath Revisit some VIRT_RETURN_STRING examples/index.php | 4 +- src/libvirt-php.c | 121 ++--- 2 files changed, 72 insertions(+), 53 deletions(-) -- 2.7.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt-php][PATCH 2/3] Fix some leaks related to get_string_from_xpath
This function not only did not free return value of
xmlNodeListGetString() it strdup()-ed its return value. Therefore
plenty of memory has been lost definitely upon return from this
function.
Signed-off-by: Michal Privoznik
---
src/libvirt-php.c | 110 ++
1 file changed, 62 insertions(+), 48 deletions(-)
diff --git a/src/libvirt-php.c b/src/libvirt-php.c
index 308e764..fb0679b 100644
--- a/src/libvirt-php.c
+++ b/src/libvirt-php.c
@@ -3232,19 +3232,17 @@ char *get_string_from_xpath(char *xml, char *xpath,
zval **val, int *retVal)
if (val != NULL) {
ret = 0;
for (i = 0; i < nodeset->nodeNr; i++) {
-if (xmlNodeListGetString(doc,
nodeset->nodeTab[i]->xmlChildrenNode, 1) != NULL) {
-value = (char *)xmlNodeListGetString(doc,
nodeset->nodeTab[i]->xmlChildrenNode, 1);
-
+if ((value = xmlNodeListGetString(doc,
nodeset->nodeTab[i]->xmlChildrenNode, 1))) {
snprintf(key, sizeof(key), "%d", i);
VIRT_ADD_ASSOC_STRING(*val, key, value);
+free(value);
+value = NULL;
ret++;
}
}
add_assoc_long(*val, "num", (long)ret);
-}
-else {
-if (xmlNodeListGetString(doc, nodeset->nodeTab[0]->xmlChildrenNode, 1)
!= NULL)
-value = (char *)xmlNodeListGetString(doc,
nodeset->nodeTab[0]->xmlChildrenNode, 1);
+} else {
+value = xmlNodeListGetString(doc,
nodeset->nodeTab[0]->xmlChildrenNode, 1);
}
cleanup:
@@ -3255,7 +3253,7 @@ char *get_string_from_xpath(char *xml, char *xpath, zval
**val, int *retVal)
xmlFreeParserCtxt(xp);
xmlFreeDoc(doc);
xmlCleanupParser();
-return (value != NULL) ? strdup(value) : NULL;
+return value;
}
/*
@@ -3327,11 +3325,8 @@ char **get_array_from_xpath(char *xml, char *xpath, int
*num)
ret = 0;
val = (char **)malloc( nodeset->nodeNr * sizeof(char *) );
for (i = 0; i < nodeset->nodeNr; i++) {
-if (xmlNodeListGetString(doc, nodeset->nodeTab[i]->xmlChildrenNode, 1)
!= NULL) {
-value = (char *)xmlNodeListGetString(doc,
nodeset->nodeTab[i]->xmlChildrenNode, 1);
-
-val[ret++] = strdup(value);
-}
+if ((value = xmlNodeListGetString(doc,
nodeset->nodeTab[i]->xmlChildrenNode, 1)))
+val[ret++] = value;
}
xmlXPathFreeContext(context);
@@ -3506,20 +3501,23 @@ long get_next_free_numeric_value(virDomainPtr domain,
char *xpath)
*/
char *connection_get_domain_type(virConnectPtr conn, char *arch TSRMLS_DC)
{
-int retval = -1;
+char *ret = NULL;
char *tmp = NULL;
char *caps = NULL;
+char *tmpArch = NULL;
char xpath[1024] = { 0 };
+int retval = -1;
caps = virConnectGetCapabilities(conn);
if (caps == NULL)
return NULL;
if (arch == NULL) {
-arch = get_string_from_xpath(caps, "//capabilities/host/cpu/arch",
NULL, &retval);
-DPRINTF("%s: No architecture defined, got '%s' from capabilities
XML\n", __FUNCTION__, arch);
-if ((arch == NULL) || (retval < 0))
-return NULL;
+tmpArch = get_string_from_xpath(caps, "//capabilities/host/cpu/arch",
NULL, &retval);
+DPRINTF("%s: No architecture defined, got '%s' from capabilities
XML\n", __FUNCTION__, tmpArch);
+if (!tmpArch || retval < 0)
+goto cleanup;
+arch = tmpArch;
}
DPRINTF("%s: Requested domain type for arch '%s'\n", __FUNCTION__, arch);
@@ -3529,12 +3527,17 @@ char *connection_get_domain_type(virConnectPtr conn,
char *arch TSRMLS_DC)
tmp = get_string_from_xpath(caps, xpath, NULL, &retval);
if ((tmp == NULL) || (retval < 0)) {
DPRINTF("%s: No domain type found in XML...\n", __FUNCTION__);
-return NULL;
+goto cleanup;
}
-DPRINTF("%s: Domain type is '%s'\n", __FUNCTION__, tmp);
-
-return tmp;
+ret = tmp;
+tmp = NULL;
+DPRINTF("%s: Domain type is '%s'\n", __FUNCTION__, ret);
+ cleanup:
+free(tmpArch);
+free(caps);
+free(tmp);
+return ret;
}
/*
@@ -3547,20 +3550,23 @@ char *connection_get_domain_type(virConnectPtr conn,
char *arch TSRMLS_DC)
*/
char *connection_get_emulator(virConnectPtr conn, char *arch TSRMLS_DC)
{
-int retval = -1;
+char *ret = NULL;
char *tmp = NULL;
char *caps = NULL;
+char *tmpArch = NULL;
char xpath[1024] = { 0 };
+int retval = -1;
caps = virConnectGetCapabilities(conn);
if (caps == NULL)
return NULL;
if (arch == NULL) {
-arch = get_string_from_xpath(caps, "//capabilities/host/cpu/arch",
NULL, &retval);
-DPRINTF("%s: No architecture defined, got '%s' from capabilities
XML\n", __FUNCTION__, arch);
-if ((arch == NULL) || (retval < 0))
-return NULL;
+tmpArch = get_string_from_xpath(caps, "//capabilities/host/cpu/arch",
NULL,
Re: [libvirt] [PATCH 12/10] secret: Introduce virSecretObjSaveConfig and virSecretObjSaveData
On 04/18/2016 06:12 PM, Cole Robinson wrote: > On 03/08/2016 12:36 PM, John Ferlan wrote: >> Move and rename the secretRewriteFile, secretSaveDef, and secretSaveValue >> from secret_driver to secret_conf >> >> Need to make some slight adjustments since the secretSave* functions >> called secretEnsureDirectory, but otherwise mostly just a move of code. >> >> Signed-off-by: John Ferlan >> --- >> src/conf/secret_conf.c | 69 +++ >> src/conf/secret_conf.h | 4 +++ >> src/libvirt_private.syms | 2 ++ >> src/secret/secret_driver.c | 90 >> +++--- >> 4 files changed, 87 insertions(+), 78 deletions(-) > > ACK > > Though there should probably be explicit virfile.c support for a generic > 'rewrite file with this passed string', rather than requiring a callback. > src/network/leaseshelper.c already has something similar > This patch was just moving existing code. I cannot say for sure, but if the 'opaque' data had some formatting considerations rather than just a string, such as virXMLSaveFile which also uses virFileRewrite, then the existing virFileRewrite is doing just what it was designed to do. I didn't spend any cycles looks which came first. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/10] secret: Move and rename secretLoadAllConfigs
On 04/18/2016 05:48 PM, Cole Robinson wrote: > On 03/02/2016 01:55 PM, John Ferlan wrote: >> Move to secret_conf.c and rename to virSecretLoadAllConfigs. Also includes >> moving/renaming the supporting virSecretLoad, virSecretLoadValue, and >> virSecretLoadValidateUUID. >> >> Signed-off-by: John Ferlan >> --- >> src/conf/secret_conf.c | 175 >> + >> src/conf/secret_conf.h | 3 + >> src/libvirt_private.syms | 1 + >> src/secret/secret_driver.c | 174 >> +--- >> 4 files changed, 181 insertions(+), 172 deletions(-) > > ACK, mirrors network_conf.c layout. > > (though honestly I'd rather we have separate files for XML handling and object > handling... the existing conf.c files are too large anyways. I put an entry on > the LibvirtFirstBugs wiki page about that type of code reorg though it's > probably over an initial contributors head) > I could go the route of a "virsecretobj.c" (to partially mimic the virdomainobjlist.c) instead of cramming everything into secret_conf - there's about 1000 new lines in secret_conf just for this series (with the reduction of lines in secret_driver). It would mean a complete v2 of this series - although considering most things have been ACK'd the second review should be easier. I think there's only a couple of issues/questions to resolve from reviews. Thanks for the review - John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] RFC: virtio-rng and /dev/urandom
On Fri, Apr 15, 2016 at 6:47 PM, Eric Blake wrote: > On 04/15/2016 04:41 AM, Cole Robinson wrote: > > Libvirt currently rejects using host /dev/urandom as an input source for > a > > virtio-rng device. The only accepted sources are /dev/random and > /dev/hwrng. > > This is the result of discussions on qemu-devel around when the feature > was > > first added (2013). Examples: > > > > http://lists.gnu.org/archive/html/qemu-devel/2012-09/msg02387.html > > https://lists.gnu.org/archive/html/qemu-devel/2013-03/threads.html#00023 > > > > libvirt's rejection of /dev/urandom has generated some complaints from > users: > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1074464 > > * cited: http://www.2uo.de/myths-about-urandom/ > > http://www.redhat.com/archives/libvir-list/2016-March/msg01062.html > > http://www.redhat.com/archives/libvir-list/2016-April/msg00186.html > > > > I think it's worth having another discussion about this, at least with a > > recent argument in one place so we can put it to bed. I'm CCing a bunch > of > > people. I think the questions are: > > > > 1) is the original recommendation to never use virtio-rng+/dev/urandom > correct? > > That I'm not sure about - and the answer may be context-dependent (for > example a FIPS user may care more than an ordinary user) > > > > > 2) regardless of #1, should we continue to reject that config in libvirt? > > This one, I have a pretty strong opinion: libvirt should NOT enforce > policy. If someone has a valid use case for doing it, we should permit > them to do it, even if it lets someone else shoot themselves in the > foot. So I think we should relax libvirt to allow users that source > their virtio-rng from /dev/urandom. > +1 I'd personally be happy (for some specific test-dev use case) with /dev/zero - I don't care about the security, but I want the entropy collection to be done as fast as possible. Y. > > -- > Eric Blake eblake redhat com+1-919-301-3266 > Libvirt virtualization library http://libvirt.org > > > -- > 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] how does the qemu command line executed by libvirt?
On Tue, Apr 19, 2016 at 04:43:09PM +0800, zhukaijie wrote: As you know, libvirt takes advantage of xml and generate corresponding qemu command line. But how does this command line executed? Does it executed in bash shell? Or any other api is used to create the qemu process? Thank you. fork(2) and execve(2) do the trick -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/7] Probe and expose GIC capabilities
On Mon, 2016-04-18 at 19:43 +0200, Andrea Bolognani wrote: > Changes from [RFC]: > > * Fix issues pointed out during review (see patches > 1 and 2 for details) > > * Add documentation > > The only thing missing AFAIK is some test cases... I'm not sure > whether it's possible to include QMP replies for QEMU 2.6 even > though it hasn't been released yet, and I wouldn't know how to > generate a .replies file anyway. Any pointers? Things I forgot to mention yesterday: * John suggested squashing some patches together, but I haven't done so because my understanding is that it would just have made review easier, and I think at this point deviating from the RFC would actually have the opposite effect. I'm not against doing so, though * It was also questioned whether patch 6, which deals with saving and restoring the QEMU capabilities, should appear earlier in the series. I don't expect a feature to work properly until the whole series has been applied, so I don't think it's worth shuffling patches around just because of that, but I could be convinced otherwise Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] how does the qemu command line executed by libvirt?
As you know, libvirt takes advantage of xml and generate corresponding qemu command line. But how does this command line executed? Does it executed in bash shell? Or any other api is used to create the qemu process? Thank you. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-php PATCH 29/29] Bump version to 0.5.2
On 19.04.2016 04:02, Neal Gompa wrote: > On Thu, Apr 14, 2016 at 12:30 PM, Michal Privoznik > wrote: >> >> I will push this one only after some testing of the patches I've just >> pushed. It has been a big change so I rather give our users chance to >> test it before calling it release. >> >> Michal > > If it helps any, since this has all made it up to the libvirt-php git, > I've tested it on PHP 5 and PHP 7 with some applications with good > success, so it looks good to me. I'd definitely appreciate a libvirt > 0.5.2 release. I'm also the php-libvirt package maintainer in Fedora > and EPEL, and I'd like to update the package to the latest stuff once > a release is made. And Remi can add it to his repository to provide > SCL versions of libvirt-php across the different PHP versions he > supports. > Agreed. I want to do the release ASAP. All I wanted to do before is some testing because the patches I've merged were quite aggressive. Your testing shows we should be good to go. I'll test it and do the release probably later today. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: command: drop redundant min_guarantee check
On Mon, Apr 18, 2016 at 19:12:56 -0400, Cole Robinson wrote: > We already reject a VM with min_guarantee early in the VM startup > in qemuProcessStartValidate > --- > src/qemu/qemu_command.c | 1 - > 1 file changed, 1 deletion(-) ACK signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REBASE 2/4] vz: fix detach disk to use new disk id
14.04.2016 13:45, Nikolay Shirokovskiy пишет: Actually using disk PrlVmDev_GetFriendlyName as id on detaching volumes is not a problem. We can only detach hard disks and these can not have empty friendly names. But upcoming update device functionality for cdroms can not use disk source as id at all as update operation typically change this same source value. Thus we will need to use cdrom bus and cdrom target name as cdrom id. So in attempt to use same id scheme for all purpuses lets fix hard disk detach function to use new id. Signed-off-by: Nikolay Shirokovskiy --- src/vz/vz_sdk.c | 106 ++-- 1 file changed, 56 insertions(+), 50 deletions(-) ACK -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: process: comment on min_guarantee validation
On Mon, Apr 18, 2016 at 19:13:08 -0400, Cole Robinson wrote:
> Explain why we check it at process startup time, and not parse time
> where most other XML validation checks are performed
This is far from being a singular case ...
> ---
> src/qemu/qemu_process.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index c087300..628b4b6 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4550,6 +4550,8 @@ qemuProcessStartValidate(virQEMUDriverPtr driver,
> virDomainDefCheckDuplicateDiskInfo(vm->def) < 0)
... this is yet another example of a similar check.
> return -1;
>
> +/* Previously we silently accepted this parameter; we can't reject
> + it at parse time without breaking those configs, so check it here */
I don't think this helps here and implies that other checks are not here
due to that case. If you want to be explicit I think it warrants a
separate function with this fact stated in it's comment. If you insist
on being explicit in the purpose of this check
> if (vm->def->mem.min_guarantee) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("Parameter 'min_guarantee' "
> --
> 2.7.3
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Explicitly error on uri=qemu://system
On Mon, Apr 18, 2016 at 19:04:04 -0400, Cole Robinson wrote:
> It's a fairly common error that a user tries to connect to a URI
> like qemu://system or qemu://session (missing a slash). This errors
> like:
>
> $ virsh --connect qemu://session
> error: failed to connect to the hypervisor
> error: Unable to resolve address 'session' service '16514': No address
> associated with hostname
>
> If you already know that the standard qemu URI has 3 slashes, that
> error will make it obvious enough. But new user's may not get it.
> There's even a RHEL support page explicitly mentioning it!:
>
> https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/7/html/Virtualization_Deployment_and_Administration_Guide/sect-Troubleshooting-Common_libvirt_errors_and_troubleshooting.html
>
> Catch this error early in libvirt.c virConnectOpen for qemu (and vbox
> which has similar rules
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1038304
> ---
> src/libvirt.c | 32
> 1 file changed, 32 insertions(+)
>
> diff --git a/src/libvirt.c b/src/libvirt.c
> index a21d00e..7607ae3 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -928,6 +928,33 @@ virConnectGetDefaultURI(virConfPtr conf,
> }
>
>
> +/*
> + * Check to see if an invalid URI like qemu://system (missing /) was passed,
> + * offer the suggested fix.
> + */
> +static int
> +check_uri_missing_slash(const char *uristr, virURIPtr uri)
Please use a name with "vir" prefix and camel case. This is totaly
against our naming convention.
> +{
> +/* These drivers _only_ accepts URIs with a 'path' element */
Only these drivers accept ... ? I don't quite follow the message of this
comment.
> +if (STRNEQ(uri->sceme, "qemu") &&
> +STRNEQ(uri->scheme, "vbox"))
> +return 0;
> +
> +if (uri->path != NULL)
> +return 0;
> +
> +if (STREQ(uri->server, "session") ||
> +STREQ(uri->server, "system")) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("invalid URI %s (maybe you want %s:///%s)"),
> + uristr, uri->scheme, uri->server);
> +return -1;
> +}
> +
> +return 0;
> +}
ACK with the rename and fix of the comment.
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Explicitly error on uri=qemu://system
19.04.2016 2:04, Cole Robinson пишет:
It's a fairly common error that a user tries to connect to a URI
like qemu://system or qemu://session (missing a slash). This errors
like:
$ virsh --connect qemu://session
error: failed to connect to the hypervisor
error: Unable to resolve address 'session' service '16514': No address
associated with hostname
If you already know that the standard qemu URI has 3 slashes, that
error will make it obvious enough. But new user's may not get it.
There's even a RHEL support page explicitly mentioning it!:
https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/7/html/Virtualization_Deployment_and_Administration_Guide/sect-Troubleshooting-Common_libvirt_errors_and_troubleshooting.html
Catch this error early in libvirt.c virConnectOpen for qemu (and vbox
which has similar rules
Please, add 'vz' also as it suffers from the problem too. Though it
doesn't support vz:///session it clearly says about it in the error message.
https://bugzilla.redhat.com/show_bug.cgi?id=1038304
---
src/libvirt.c | 32
1 file changed, 32 insertions(+)
diff --git a/src/libvirt.c b/src/libvirt.c
index a21d00e..7607ae3 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -928,6 +928,33 @@ virConnectGetDefaultURI(virConfPtr conf,
}
+/*
+ * Check to see if an invalid URI like qemu://system (missing /) was passed,
+ * offer the suggested fix.
+ */
+static int
+check_uri_missing_slash(const char *uristr, virURIPtr uri)
+{
+/* These drivers _only_ accepts URIs with a 'path' element */
+if (STRNEQ(uri->scheme, "qemu") &&
+STRNEQ(uri->scheme, "vbox"))
+return 0;
+
+if (uri->path != NULL)
+return 0;
+
+if (STREQ(uri->server, "session") ||
+STREQ(uri->server, "system")) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("invalid URI %s (maybe you want %s:///%s)"),
+ uristr, uri->scheme, uri->server);
+return -1;
+}
+
+return 0;
+}
+
+
static virConnectPtr
do_open(const char *name,
virConnectAuthPtr auth,
@@ -995,6 +1022,11 @@ do_open(const char *name,
NULLSTR(ret->uri->user), ret->uri->port,
NULLSTR(ret->uri->path));
+if (check_uri_missing_slash(alias ? alias : name, ret->uri) < 0) {
+VIR_FREE(alias);
+goto failed;
+}
+
VIR_FREE(alias);
} else {
VIR_DEBUG("no name, allowing driver auto-select");
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [python PATCH] fix crash in getAllDomainStats
On Mon, Apr 18, 2016 at 17:07:48 +0200, Pavel Hrdina wrote: > Commits 1d39dbaf and 827ed9b4 broke the libvirt-python API by removing > virDomainRef() and virDomainFree(). virDomainStatsRecordListFree() will > free that domain pointer and later when virDomain (python object) call > its destructor and tries to free that same pointer again. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1326839 > > Signed-off-by: Pavel Hrdina > --- > libvirt-override.c | 8 +++- > 1 file changed, 7 insertions(+), 1 deletion(-) > ACK signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
