[libvirt] libvirt-0.10.2-46 compress failed
Hello When compress libvirt-0.10.2-46.el6.src.rpm to support rbd, then failed; Error log: cc1: warnings being treated as errors storage/storage_backend_rbd.c: In function 'virStorageBackendRBDRefreshPool': storage/storage_backend_rbd.c:284: error: declaration of 'stat' shadows a global declaration [-Wshadow] /usr/include/sys/stat.h:455: error: shadowed declaration is here [-Wshadow] At top level: cc1: error: unrecognized command line option "-Wno-suggest-attribute=const" cc1: error: unrecognized command line option "-Wno-suggest-attribute=pure" make[3]: *** [libvirt_driver_storage_impl_la-storage_backend_rbd.lo] Error 1 make[3]: *** Waiting for unfinished jobs make[3]: Leaving directory `/home/ada/rpmbuild/BUILD/libvirt-0.10.2/src' make[2]: *** [all] Error 2 make[2]: Leaving directory `/home/ada/rpmbuild/BUILD/libvirt-0.10.2/src' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/home/ada/rpmbuild/BUILD/libvirt-0.10.2' make: *** [all] Error 2 error: Bad exit status from /var/tmp/rpm-tmp.g0IuFq (%build) RPM build errors: Bad exit status from /var/tmp/rpm-tmp.g0IuFq (%build) Thanks!!! The information in this email is confidential and may be legally privileged. If you have received this email in error or are not the intended recipient, please immediately notify the sender and delete this message from your computer. Any use, distribution, or copying of this email other than by the intended recipient is strictly prohibited. All messages sent to and from us may be monitored to ensure compliance with internal policies and to protect our business. Emails are not secure and cannot be guaranteed to be error free as they can be intercepted, amended, lost or destroyed, or contain viruses. Anyone who communicates with us by email is taken to accept these risks. ��跺�浠惰��璇锋敞���锛� ��浠跺��淇�瀵�淇℃��锛���ヨ舵�浠讹��璇峰�″���ュ�浜哄苟��存�ュ伙��涓�寰�浣跨�ㄣ��浼���澶���舵�浠躲�� 杩���洪��浠跺���版���稿��瑙��с�浠跺藉�琚��琚�淇���广��涓㈠け���琚���村���璁＄虹��姣�绛�涓�瀹���ㄦ点�� -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] rework the check for the numa cpus
On 04/20/2015 10:39 PM, Michal Privoznik wrote: On 02.04.2015 10:02, Luyao Huang wrote: We introduce a check for numa cpu total count in 5f7b71, But seems this check cannot work well. There are some scenarioes: 1. one of cpu id is out of maxvcpus, can set success(cpu count = 5 < 10): 10 the cpus '100' exceed maxvcpus, this setting is not valid (although qemu do not output error). 2. use the same cpu in 2 cell, can set success(cpu count = 8 < 10): 10 I guess nobody will use this setting if he really want use numa in his vm. (qemu not output error, but we can find some interesting things in vm, all cpus have bind in one numa node) 3. use the same cpu in 2 cell, cannot set success(cpu count = 11 > 10): 10 No need forbid this scenario if scenario 2 is a 'valid' setting. However add new check during parse xml will make vm has broken settings disappear after update libvirtd, so possible solutions: 1. add new check when parse xml, and find a way to avoid vm evaporate. I chose this way and i don't think just drop the invalid settings is a good idea for numa cpus, so i add a new function. 2. add new check when start vm. I think this is a configuration issue, so no need report it so late. 3. just remove this check and do not add new check... :) Luyao Huang (2): conf: introduce a new function to add check avoid losing track conf: rework the cpu check for vm numa settings src/bhyve/bhyve_driver.c | 4 ++-- src/conf/domain_conf.c | 21 ++--- src/conf/domain_conf.h | 1 + src/conf/numa_conf.c | 37 ++--- src/conf/numa_conf.h | 2 +- src/esx/esx_driver.c | 2 +- src/libxl/libxl_driver.c | 4 ++-- src/lxc/lxc_driver.c | 4 ++-- src/openvz/openvz_driver.c | 4 ++-- src/parallels/parallels_driver.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_driver.c | 4 ++-- src/test/test_driver.c | 4 ++-- src/uml/uml_driver.c | 4 ++-- src/vbox/vbox_common.c | 2 +- src/vmware/vmware_driver.c | 4 ++-- src/xen/xen_driver.c | 4 ++-- src/xenapi/xenapi_driver.c | 4 ++-- 18 files changed, 70 insertions(+), 39 deletions(-) I agree with renaming and extending the virDomainNumaGetCPUCountTotal(). Even the diff in 2/2 shows the function is utterly broken. However, I think this function can be called unconditionally - even when the flag is not set. Having said that, the flag is redundant. Oh, good news to me :) So if this function can be called unconditionally, there is no reason to introduce this new flag. Moreover, when sending a patchset, the sources should compile cleanly after each patch. This is not the case with this one. Get it, i will pay more attention for this things next time, thanks for pointing out that. Looking forward to v2. Thanks for your review and advise, i will propose a new version these days. Michal Luyao -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] lxc: fix 2 issue around cpuset
On 04/20/2015 10:04 PM, Ján Tomko wrote: On Fri, Apr 03, 2015 at 06:11:15PM +0800, Luyao Huang wrote: There are two bugs in this function: 1. cannot start a vm with cpuset but without numatune settings # virsh -c lxc:/// start helloworld error: Failed to start domain helloworld error: internal error: guest failed to start: Invalid value '1-3' for 'cpuset.mems': Invalid argument we don't free &mask after use it for virCgroupSetCpusetCpus() and then virDomainNumatuneMaybeFormatNodeset() do not get a new &mask, then we use it in virCgroupSetCpusetMems(). 2. when start a lxc with numatune memory mode not strict I think these two fixes would be better pushed separately. # virsh -c lxc:/// start helloworld error: Failed to start domain helloworld error: internal error: guest failed to start: Unknown failure in libvirt_lxc startup We shouldn't set anything in cpuset.mems for these mode. Signed-off-by: Luyao Huang --- src/lxc/lxc_cgroup.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) ACK, I've split the commit into two and pushed both. Thanks a lot for your review and split. Jan Luyao -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: set macvtap physdevs online when macvtap is set online
On 04/17/2015 03:15 PM, Laine Stump wrote:
> On 04/15/2015 02:59 PM, John Ferlan wrote:
>> On 04/13/2015 01:44 PM, Laine Stump wrote:
>>> A further fix for:
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1113474
>>>
>>> Since there is no possibility that any type of macvtap will work if
>>> the parent physdev it's attached to is offline, we should bring the
>>> physdev online at the same time as the macvtap. When taking the
>>> macvtap offline, it's also necessary to take the physdev offline for
>>> macvtap passthrough mode (because the physdev has the same MAC address
>>> as the macvtap device, so could potentially cause problems with
>>> misdirected packets during migration, as outlined in commits 829770
>>> and 879c13). We can't set the physdev offline for other macvtap modes
>>> 1) because there may be other macvtap devices attached to the same
>>> physdev in the other modes whereas passthrough mode is exclusive to
>>> one macvtap at a time, and 2) there's no practical reason to do so
>>> anyway.
>>> ---
>>> src/qemu/qemu_hotplug.c | 8 +++-
>>> src/qemu/qemu_interface.c | 29 +++--
>>> 2 files changed, 30 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>>> index 2f0549e..9be2ea3 100644
>>> --- a/src/qemu/qemu_hotplug.c
>>> +++ b/src/qemu/qemu_hotplug.c
>>> @@ -3931,11 +3931,9 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
>>> VIR_WARN("cannot clear bandwidth setting for device : %s",
>>> detach->ifname);
>>>
>>> -/* deactivate the tap/macvtap device on the host (currently this
>>> - * isn't necessary, as everything done in
>>> - * qemuInterfaceStopDevice() is made meaningless when the device
>>> - * is deleted anyway, but in the future it may be important, and
>>> - * doesn't hurt anything for now)
>>> +/* deactivate the tap/macvtap device on the host, which could also
>>> + * affect the parent device (e.g. macvtap passthrough mode sets
>>> + * the parent device offline)
>>> */
>>> ignore_value(qemuInterfaceStopDevice(detach));
>>>
>>> diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
>>> index 201a7dd..01226ac 100644
>>> --- a/src/qemu/qemu_interface.c
>>> +++ b/src/qemu/qemu_interface.c
>>> @@ -63,7 +63,20 @@ qemuInterfaceStartDevice(virDomainNetDefPtr net)
>>> goto cleanup;
>>> }
>>> break;
>>> -case VIR_DOMAIN_NET_TYPE_DIRECT:
>>> +
>>> +case VIR_DOMAIN_NET_TYPE_DIRECT: {
>>> +const char *physdev = virDomainNetGetActualDirectDev(net);
>>> +bool isOnline = true;
>> Seems unnecessary to set = true, since virNetDevGetIFFlag (what
>> virNetDevGetOnline calls) will set either True or False on success.
>
> This is some defensive programming to protect against physdev being NULL
> (which *should* be impossible for type='direct', but not verifiably so
> without a much larger context).
>
> The idea is that I never call virNetDevGetOnline at all unless physdev
> != NULL. So if physdev was NULL, I want isOnline to be set to true so I
> won't try to "virNetDevSetOnline(NULL, true);".
>
> If I was more trustful of the code (much of which was probably written
> or reviewed by me) I wouldn't need this.
>
>> Doesn't hurt... Your call though.
>>
>>> +
>>> +/* set the physdev online if necessary. It may already be up,
>>> + * in which case we shouldn't re-up it just in case that causes
>>> + * some sort of "blip" in the physdev's status.
>>> + */
>>> +if (physdev && virNetDevGetOnline(physdev, &isOnline) < 0)
>>> +goto cleanup;
>>> +if (!isOnline && virNetDevSetOnline(physdev, true) < 0)
>>> +goto cleanup;
>>> +
>> No issue putting it online in order for us to use it; however, if it is
>> online before we got here, shouldn't we know that for Stop?
>
> Yes and no. On one hand, it's nice to put everything back exactly how we
> found it. On the other, when an interface is being used for either
> hostdev passthrough or macvtap passthrough, it can't be used for
> anything else, so we know that at the time we started using it it was
> unused, and immediately after we're finished it will be unused, and
> ifup'ing an interface that isn't in use only leads to grief (for
> example, I believe certain versions of NetworkManager like to notice
> things like this and start up dhclient on the interface).
>
> Also there is the hostdev passthrough usage to consider - in that case,
> we detach the device from the host net driver, then reattach it; when we
> reattach it always ends up offline, so this behavior in macvtap
> passthrough is no different.
>
> I considered modifying the replace/restoreconfig functions to save and
> restore the online status of the interface so that all types of
> networking use would terminate with the device in exactly the same state
> in which it was found. Unfortunately, when d
Re: [libvirt] [PATCH 6/7] util: Introduce virIsSameHostnameInfo
On 04/20/2015 10:31 AM, Peter Krempa wrote:
> On Sun, Apr 19, 2015 at 20:49:11 -0400, John Ferlan wrote:
>> In order to assist with existing pool source hostname validation against
>> an incoming pool source definition, we need a way to validate that the
>> resolved hostname provided for the pool doesn't match some existing
>> pool; otherwise, we'd have a scenario where two storage pools could be
>> looking at the same data.
>>
>> Search through all the existing pools resolved names against the proposed
>> definitions resolved hostnames - if any match, we return true; otherwise,
>> false is returned
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/libvirt_private.syms | 1 +
>> src/util/virutil.c | 106
>> +++
>> src/util/virutil.h | 2 +
>> 3 files changed, 109 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 5ba9635..a5f4d7f 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2323,6 +2323,7 @@ virIndexToDiskName;
>> virIsCapableFCHost;
>> virIsCapableVport;
>> virIsDevMapperDevice;
>> +virIsSameHostnameInfo;
>> virIsSUID;
>> virIsValidHostname;
>> virManageVport;
>> diff --git a/src/util/virutil.c b/src/util/virutil.c
>> index f6cc9af..c152b8c 100644
>> --- a/src/util/virutil.c
>> +++ b/src/util/virutil.c
>> @@ -739,6 +739,112 @@ virIsValidHostname(const char *hostname)
>> }
>>
>>
>> +/*
>> + * virIsSameHostnameInfo:
>
> The hostname is not same as the STREQ check is not enough as explained
> below. virIsSameHost perhaps?
>
Sure - Just figured that since I was using the 'getaddrinfo' and
'getnameinfo' that I should incorporate the 'name' and 'info' into the
API name
>> + *
>> + * Take two hostname representations and compare their 'getnameinfo' results
>> + * to ensure that they differ. This will be used by the network storage pool
>> + * validation to ensure an incoming definition doesn't match some existing
>> + * pool definition just because someone tried to change the hostname string
>> + * representation. For example, "localhost" and "127.0.0.1" would fail the
>> + * plain STREQ check, but they essentially are the same host, so the
>> incoming
>> + * definition should be rejected since a pool for the host already exists.
>
> This function is under the genric utils section so the docs should
> explain its general usage.
>
Ok - I'll adjust
>> + *
>> + * @poolhostname: String stored as the pool's hostname
>> + * @defhostname: String to be used by the def's hostname
>
> Same here, the variable naming and this comment doesn't hint that the
> function is universal.
>
yep
>> + *
>> + * Returns true if they are the same, false if not
>
> "may report libvirt error if false is returned" ... but see below [1]
>
>> + */
>> +bool
>> +virIsSameHostnameInfo(const char *poolhostname,
>> + const char *defhostname)
>> +{
>> +int r;
>> +struct addrinfo poolhints, *poolinfo = NULL, *poolcur;
>> +struct addrinfo defhints, *definfo = NULL, *defcur;
>
>
> The hints argument for getaddrinfo is const so you don't neet two
> separate ones [2] ...
>
Right.
>
>> +char poolhost[NI_MAXHOST];
>> +char defhost[NI_MAXHOST];
>
> These take up 2KiB on the stack, wouldn't it be worth allocate them on
> the heap?
>
OK - I'll adjust
>> +
>> +memset(&poolhints, 0, sizeof(poolhints));
>> +poolhints.ai_family = AF_UNSPEC;
>> +poolhints.ai_protocol = IPPROTO_TCP;
>> +
>> +if ((r = getaddrinfo(poolhostname, NULL, &poolhints, &poolinfo)) != 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("IP address lookup for poolhostname '%s' failed:
>> %s"),
>> + poolhostname, gai_strerror(r));
>
> [1] Since you are using this function to check that the addresses and
> the 'false' case is when you define the pool, every single error
> reported by this function will just pollute logs. Additionally the
> function that is calling this is not resetting the error, so you might
> get an inaccurate error if something forgets to set error later.
>
I struggled with this too - trying to have less change, but I've
adjusted the calls in my branch to handle the error.
>> +return false;
>> +}
>> +
>> +if (!poolinfo) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("No IP address for poolhostname '%s' found"),
>> + poolhostname);
>> +return false;
>> +}
>> +
>> +memset(&defhints, 0, sizeof(defhints));
>> +defhints.ai_family = AF_UNSPEC;
>> +defhints.ai_protocol = IPPROTO_TCP;
>
> [2] ... and can avoid one of these.
>
>> +
>> +if ((r = getaddrinfo(defhostname, NULL, &defhints, &definfo)) != 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("IP address lookup for defhostname '%s' failed:
>> %s"),
>> + defhostname, gai_strerr
Re: [libvirt] [PATCH 7/7] processNicRxFilterChangedEvent: Take appropriate actions for NET_TYPE_NETWORK too
On 04/14/2015 12:59 PM, Michal Privoznik wrote:
> So, as partly explained in previous commit, there is an issue
> with NATed networks on a guest originated MAC change. When user
> sets @floor in under it results in a QoS
> hierarchy being built on the corresponding bridge (yes, this
> feature is type='network' only). The reason is we can make
> shaping decisions only there, where the whole traffic can be
> seen. In case of bridged networks it's bridge. But, on the
> bridge, in egress qdiscs (outgoing traffic, after routing
> decision), due to some Linux kernel internal implementation, we
> are not able to see the device packet came from. So we have a set
> of filters that place the packet into correct qdisc based on the
> MAC recorded in the packet. But with recent changes, this is not
> working as expected. We taught libvirt to listen to MAC change
> events from guests. However, the QoS hierarchy is not updated
> accordingly resulting in applying incorrect shaping rules on a
> vNIC that changed the MAC address.
How about something like this?
Because packets going through the egress from a bridge (where our
bandwidth limiting takes place) have no information about which
interface they came from, the QoS rules that we create instead use the
source MAC address of the packets to make their decisions about which
QDisc the packet should be in.
One flaw in this is that when a guest changed the MAC address it used, packets
from the guest would no longer be put into the correct QDisc, but would instead
be put in an "unprivileged" class, resulting in the bandwidth "floor" (minimum
guaranteed) being no longer honored.
Now that libvirt has infrastructure to capture and respond to RX_FILTER_CHANGE
events from
qemu (sent whenever a guest interface modifies its MAC address, among
other things), we can notice when a guest MAC address changes, and
update the QoS rules accordingly, so that bandwidth floor is honored even after
a guest MAC address change.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_driver.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f5a3ef9..6aab1d0 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4382,6 +4382,28 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
> syncNicRxFilterDeviceOptions(def->ifname, guestFilter, hostFilter);
> }
We had discussed in private mail whether this should always be done
regardless of the setting of trustGuestRxFilters. In some ways it seems
like it should, since the guest is free to change its MAC address
anyway. On the other hand, if the admin hasn't specifically allowed
changing the MAC address, there's nothing saying that we must honor the
bandwidth floor for the guest even though it is "going rogue".
Note that when macTableManager="libvirt", traffic from to/from the guest
*won't* pass at all after a MAC address change.
So maybe it *is* proper to not do this unless trustGuestRxFilters is
"yes" (as you have it doing right now).
>
> +if (virDomainNetGetActualType(def) == VIR_DOMAIN_NET_TYPE_NETWORK) {
> +const char *brname = virDomainNetGetActualBridgeName(def);
> +
> +if (virNetDevGetRxFilter(def->ifname, &hostFilter)) {
> +VIR_WARN("Couldn't get current RX filter for device %s "
> + "while responding to NIC_RX_FILTER_CHANGED",
> + def->ifname);
> +goto endjob;
> +}
> +
> +/* For TUN/TAP connections, set the following macvtap network device
> + * attributes to match those of the guest network device:
> + * - MAC address
> + * - QoS filters (which are based on MAC address)
> + */
> +syncNicRxFilterMacAddr(def->ifname, guestFilter, hostFilter);
syncNicRxFilterMacAddr() is only useful for macvtap devices. It sets the
MAC address of the tap (macvtap in that case) to the same address used
by the guest. Doing so for a tap device would be disastrous, as the
kernel would no longer understand that it had to forward packets
*through* the tap device to the guest, but would believe that the host
itself was the final destination.
If anything, the MAC address of the tap could be set to the guest's MAC
with the initial byte changed to 0xFE, but even that isn't really
necessary (the exact MAC address of the tap device isn't important, it's
just important that it is unique within the L2 domain that the tap is
connected to).
> +
> +if (virNetDevBandwidthUpdateFilter(brname, &guestFilter->mac,
> +
> def->data.network.actual->class_id) < 0)
> +goto endjob;
> +}
> +
> endjob:
> qemuDomainObjEndJob(driver, vm);
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/7] virnetdevbandwidth.c: Separate tc filter creation to a function
On 04/17/2015 04:51 AM, Michal Privoznik wrote: > On 16.04.2015 19:25, Laine Stump wrote: >> On 04/14/2015 12:59 PM, Michal Privoznik wrote: >>> Not only this simplifies the code a bit, it prepares the >>> environment for upcoming patches. The new >>> virNetDevBandwidthManipulateFilter() function is capable of both >>> removing a filter and adding a new one. At the same time! Yeah, >>> this is not currently used anywhere but look at the next commit >>> + * >>> + * Returns: 0 on success, >>> + * -1 otherwise (with error reported). >>> + */ >>> +static int ATTRIBUTE_NONNULL(1) >>> +virNetDevBandwidthManipulateFilter(const char *ifname, >>> + const virMacAddr *ifmac_ptr, >>> + unsigned int id, >>> + const char *class_id, >>> + bool remove_old, >>> + bool create_new) >> How about making these two flags so that it will be easier to tell >> what's intended when looking at a call to the function? > I was thinking about this too, but then I went with booleans. My idea > was that it's shorter this way than inventing new enum items like > VIR_NET_DEV_BANDWIDTH_FILTER_CREATE or > VIR_NET_DEV_BANDWIDTH_FILTER_REMOVE. But if somebody prefers the other > way, I can switch to that. I'm fine if you want to keep it as booleans, too. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/7] virDomainActualNetDefContentsFormat: Format class_id more frequently
On 04/16/2015 01:00 PM, Laine Stump wrote:
> On 04/14/2015 12:59 PM, Michal Privoznik wrote:
>> After a360912179 the formatting of virDomainActualNetDefPtr was
>> changed a bit. However, during the function rewrite, iface's class_id
>> is not formatted as frequently as it could be. In fact, after rewrite
>> it's formatted only for iface of type VIR_DOMAIN_NET_TYPE_DIRECT where
>> it makes no sense and is unused. While where needed (_TYPE_NETWORK) is
>> not formatted at all.
> Yikes! That was a typo when moving the code around. Should have been
> VIR_DOMAIN_NET_TYPE_NETWORK (that bit was inside "case
> VIR_DOMAIN_NET_TYPE_NETWORK previous to the patch)
>
>> This makes the daemon forget it upon daemon
>> restart resulting in bad behaviour.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/conf/domain_conf.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 4d7e3c9..ab4f2bf 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -18608,8 +18608,7 @@ virDomainActualNetDefContentsFormat(virBufferPtr buf,
>>
>> virBufferAddLit(buf, "/>\n");
>> }
>> -if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT &&
>> -def->data.network.actual && def->data.network.actual->class_id) {
>> +if (def->data.network.actual && def->data.network.actual->class_id) {
>> virBufferAsprintf(buf, "\n",
>>def->data.network.actual->class_id);
>> }
> I suppose since the modes that wouldn't have been able to setup any
> bandwidth limiting would have class_id == 0, this works. If you for some
> reason wanted to protect against using it for hostdev interfaces, yoou
> could move it up into the preceding else clause, but I guess I don't see
> any gain from that.
An interesting aside note - just a few days ago I noticed that at
most/all SRIOV cards have support for setting a bandwidth maximum on
each VF (and at least a couple support setting a minimum). Of course in
this case tc wouldn't be used, so no class_id would be needed (in other
words, it's not directly relevant to this patch), I just thought it was
interesting - a nice simple project for someone with a few spare cycles
and access to the proper SRIOV hardware for testing.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/11] domain: conf: Better errors on bad os values
On 04/20/2015 09:53 AM, Michal Privoznik wrote:
> On 18.04.2015 03:45, Cole Robinson wrote:
>> If no was specified:
>> before: unknown OS type no OS type
>> after : xml error: an os must be specified
>>
>> If an is specified that's not in our capabiliities data:
>> before: unknown OS type: $type
>> after : unsupported configuration: no support found for os '$type'
>>
>> VIR_ERR_OS_TYPE is now unused (as it should be frankly) so drop its strings
>> as well to save our translators some effort.
>> ---
>> src/conf/domain_conf.c | 9 +
>> src/util/virerror.c| 5 +
>> 2 files changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index ab4f2bf..8458f5b 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -14638,8 +14638,8 @@ virDomainDefParseXML(xmlDocPtr xml,
>> if (VIR_STRDUP(def->os.type, "xen") < 0)
>> goto error;
>> } else {
>> -virReportError(VIR_ERR_OS_TYPE,
>> - "%s", _("no OS type"));
>> +virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("an os must be specified"));
>> goto error;
>> }
>> }
>> @@ -14656,8 +14656,9 @@ virDomainDefParseXML(xmlDocPtr xml,
>> }
>>
>> if (!virCapabilitiesSupportsGuestOSType(caps, def->os.type)) {
>> -virReportError(VIR_ERR_OS_TYPE,
>> - "%s", def->os.type);
>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("no support found for os '%s'"),
>> + def->os.type);
>> goto error;
>> }
>>
>> diff --git a/src/util/virerror.c b/src/util/virerror.c
>> index 73dae95..aab36ae 100644
>> --- a/src/util/virerror.c
>> +++ b/src/util/virerror.c
>> @@ -900,10 +900,7 @@ virErrorMsg(virErrorNumber error, const char *info)
>> errmsg = _("failed Xen syscall %s");
>> break;
>> case VIR_ERR_OS_TYPE:
>> -if (info == NULL)
>> -errmsg = _("unknown OS type");
>> -else
>> -errmsg = _("unknown OS type %s");
>> +errmsg = "%s";
>
> Even though there are no other calls with VIR_ERR_OS_TYPE, I'd feel more
> safe with:
>
> if (info == NULL)
> errmsg = _("invalid or missing OS type");
> else
> errmsg = "%s";
>
> I find it more future proof.
Thanks for the review. I just the dropped virerror.c diff, rather than add a
new string that needs translation yet isn't used anywhere.
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/11] caps: various internal cleanups
On 04/20/2015 09:52 AM, Michal Privoznik wrote: > On 18.04.2015 03:45, Cole Robinson wrote: >> First two patches are straight bug fixes. >> >> The rest is a bunch of internal cleanup I felt compelled to do. Summary is >> >> - Switch caps and domain to use an enum for os.type value (hvm, xen, exe, >> ...) >> - Switch caps to use VIR_DOMAIN_VIRT* internally >> - Add a single function for looking up domain XML relevant values from >> capabilities, and test the crap out of it. >> - Use that function to simplfy and improve a lot of code. >> >> Thanks, >> Cole >> >> Cole Robinson (11): >> domain: conf: Better errors on bad os values >> domain: conf: Don't validate VM ostype/arch at daemon startup >> caps: Use an enum internally for ostype value >> caps: Switch AddGuest to take VIR_DOMAIN_OSTYPE value >> domain: Convert os.type to VIR_DOMAIN_OSTYPE enum >> caps: Convert to use VIR_DOMAIN_VIRT internally >> caps: Add virCapabilitiesDomainDataLookup >> domain: conf: Do ostype/arch/machine parsing earlier >> domain: conf: Use CapabilitiesDomainDataLookup for caps validation >> caps: Use DomainDataLookup to replace GuestDefault* >> domain: conf: Drop expectedVirtTypes >> >> src/bhyve/bhyve_capabilities.c | 6 +- >> src/bhyve/bhyve_driver.c | 5 - >> src/conf/capabilities.c| 307 -- >> src/conf/capabilities.h| 50 ++--- >> src/conf/domain_audit.c| 2 +- >> src/conf/domain_conf.c | 331 >> +++-- >> src/conf/domain_conf.h | 25 ++- >> src/conf/snapshot_conf.c | 18 +- >> src/conf/snapshot_conf.h | 2 - >> src/esx/esx_driver.c | 12 +- >> src/hyperv/hyperv_driver.c | 4 +- >> src/libvirt_private.syms | 6 +- >> src/libxl/libxl_conf.c | 14 +- >> src/libxl/libxl_domain.c | 7 +- >> src/libxl/libxl_driver.c | 8 +- >> src/libxl/libxl_migration.c| 2 - >> src/lxc/lxc_conf.c | 8 +- >> src/lxc/lxc_controller.c | 1 - >> src/lxc/lxc_driver.c | 7 +- >> src/lxc/lxc_native.c | 4 +- >> src/openvz/openvz_conf.c | 7 +- >> src/openvz/openvz_driver.c | 7 +- >> src/parallels/parallels_driver.c | 23 +- >> src/parallels/parallels_sdk.c | 6 +- >> src/parallels/parallels_utils.h| 2 +- >> src/phyp/phyp_driver.c | 10 +- >> src/qemu/qemu_capabilities.c | 21 +- >> src/qemu/qemu_command.c| 29 +-- >> src/qemu/qemu_domain.c | 1 - >> src/qemu/qemu_domain.h | 6 - >> src/qemu/qemu_driver.c | 14 +- >> src/qemu/qemu_migration.c | 5 +- >> src/security/virt-aa-helper.c | 9 +- >> src/test/test_driver.c | 12 +- >> src/uml/uml_conf.c | 4 +- >> src/uml/uml_driver.c | 6 +- >> src/util/virerror.c| 5 +- >> src/vbox/vbox_common.c | 23 +- >> src/vmware/vmware_conf.c | 8 +- >> src/vmware/vmware_driver.c | 4 +- >> src/vmx/vmx.c | 3 +- >> src/xen/xen_driver.c | 5 +- >> src/xen/xen_hypervisor.c | 4 +- >> src/xen/xend_internal.c| 6 +- >> src/xenapi/xenapi_driver.c | 20 +- >> src/xenapi/xenapi_utils.c | 4 +- >> src/xenconfig/xen_common.c | 64 +++--- >> src/xenconfig/xen_sxpr.c | 5 +- >> src/xenconfig/xen_xl.c | 17 +- >> src/xenconfig/xen_xm.c | 12 +- >> tests/Makefile.am | 8 +- >> tests/domainconftest.c | 3 +- >> tests/domainsnapshotxml2xmltest.c | 1 - >> tests/lxcxml2xmltest.c | 1 - >> tests/openvzutilstest.c| 2 +- >> tests/qemuagenttest.c | 1 - >> tests/qemuhotplugtest.c| 1 - >> tests/qemuxml2argvtest.c | 1 - >> tests/qemuxml2xmltest.c| 3 +- >> tests/qemuxmlnstest.c | 1 - >> tests/securityselinuxlabeldata/chardev.xml | 2 +- >> tests/securityselinuxlabeldata/disks.xml | 2 +- >> tests/securityselinuxlabeldata/kernel.xml | 2 +- >> tests/securityselinuxlabeldata/nfs.xml | 2 +- >> tests/securityselinuxlabeltest.c | 4 +- >> tests/testutils.c | 8 +- >> tests/testutilslxc.c
Re: [libvirt] [PATCH 4/7] gluster: Check for validity of pool source hostname
On 04/20/2015 09:45 AM, Peter Krempa wrote: > On Sun, Apr 19, 2015 at 20:49:09 -0400, John Ferlan wrote: >> Prior to attempting to open the gluster connection, let's make sure we >> can resolve the source pool hostname. >> >> Signed-off-by: John Ferlan >> --- >> src/storage/storage_backend_gluster.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/src/storage/storage_backend_gluster.c >> b/src/storage/storage_backend_gluster.c >> index d2e79bc..e3a8c21 100644 >> --- a/src/storage/storage_backend_gluster.c >> +++ b/src/storage/storage_backend_gluster.c >> @@ -96,6 +96,9 @@ virStorageBackendGlusterOpen(virStoragePoolObjPtr pool) >> trailing_slash = false; >> } >> >> +if (!virIsValidHostname(pool->def->source.hosts[0].name)) >> +return NULL; >> + > > Okay, it might be marginally worth at this point, since gluster is/was > leaking some memory and threads after it was initialized. > > On the other hand you'll be resolving the hostname again a few lines > below. > Which call, glfs_set_volfile_server? It wasn't intuitively obvious... John > >> if (VIR_ALLOC(ret) < 0) >> return NULL; > > Peter > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/7] iscsi: Check for validity of pool source hostname
On 04/20/2015 09:41 AM, Peter Krempa wrote:
> On Sun, Apr 19, 2015 at 20:49:07 -0400, John Ferlan wrote:
>> Ensure that the pool that's being started has a source pool hostname
>> that can be resolved before trying to start an iSCSI session.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/storage/storage_backend_iscsi.c | 7 +++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/src/storage/storage_backend_iscsi.c
>> b/src/storage/storage_backend_iscsi.c
>> index 197d333..958c347 100644
>> --- a/src/storage/storage_backend_iscsi.c
>> +++ b/src/storage/storage_backend_iscsi.c
>> @@ -385,6 +385,13 @@ virStorageBackendISCSIStartPool(virConnectPtr conn,
>> return -1;
>> }
>>
>> +if (!virIsValidHostname(pool->def->source.hosts[0].name)) {
>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("cannot resolve hostname '%s' on this host"),
>> + pool->def->source.hosts[0].name);
>> +return -1;
>
> This overwrites the error from virIsValidHostname()
>
Oh right - relic of first change...
Removed the ReportError here
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/7] virutil: Introduce virIsValidHostname
>> As a hindsight from reviewing 6/7. This function should also be in >> virsocketaddr.c >> >> > > > hmmm.. yes I see.. Guess I got hung up on "virSocketAddr..." and didn't > focus as closely on the implementation where virSocketAddrParse can take > NULL as the first parameter... Guess, that means patches 2-5 can just be > called as: > > if (virSocketAddrParse(NULL, pool->def->source.hosts[0].name, > AF_UNSPEC) < 0) > > Without actually trying it - seemed like a good idea; however, the virSocketAddrParse uses/sets "hints.ai_flags = AI_NUMERICHOST;" thus it requires a numeric value and not one that could be a name or a number, so it seems this particular code cannot use it. I really see those virSocketAddr* API's as different, very specific to supporting the network socket's and socket address formats; whereas, this code will take a string representation of either the name or the number as provided in the XML and validate it. I don't think this set of API's belongs there as it's not manipulating virSocketAddr's. So, I'll change the function intro to: * Unlike virGetHostname, this variant of the code receives a hostname and * retrieves the getaddrinfo. If the passed hostname can be successfully * resolved via getaddrinfo, then return true; otherwise, if the hostname * cannot be resolved for any reason, return false. and remove the localhost specific checking and adjust the commit message to remove the 'getnameinfo' reference. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] parallels: fix return status for parallelsConnectOpen
On 04/20/2015 09:24 AM, Nikolay Shirokovskiy wrote: We should return VIR_DRV_OPEN_ERROR in case if we handle scheme in query but some error occur. Previously we sometimes return VIR_DRV_OPEN_DECLINE. Signed-off-by: Nikolay Shirokovskiy --- src/parallels/parallels_network.c |2 +- src/parallels/parallels_storage.c |2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index 47f4886..42ef836 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -349,7 +349,7 @@ parallelsNetworkOpen(virConnectPtr conn, error: virObjectUnref(privconn->networks); privconn->networks = NULL; -return VIR_DRV_OPEN_DECLINED; +return VIR_DRV_OPEN_ERROR; } int parallelsNetworkClose(virConnectPtr conn) diff --git a/src/parallels/parallels_storage.c b/src/parallels/parallels_storage.c index 6397601..4091124 100644 --- a/src/parallels/parallels_storage.c +++ b/src/parallels/parallels_storage.c @@ -494,7 +494,7 @@ parallelsStorageOpen(virConnectPtr conn, error: parallelsStorageUnlock(storageState); parallelsStorageClose(conn); -return -1; +return VIR_DRV_OPEN_ERROR; } static int ACKed and pushed, thanks! -- Dmitry Guryanov -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] parallels: make device addressing consistent
On 04/20/2015 09:25 AM, Nikolay Shirokovskiy wrote:
In Parallels we do not support device name hints
aka option and full-fledged device
disk device addressing through
and have only one index instead.
In this situation to be consistent we can only take
one-to-one mapping from some reasonable subset
of full address. Values outside this subset are
invalid to create Parallels VMs.
Reasonable mapping is default one defined in virDomainDiskDefAssignAddress.
Signed-off-by: Nikolay Shirokovskiy
---
src/parallels/parallels_sdk.c | 72 ++--
1 files changed, 61 insertions(+), 11 deletions(-)
diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c
index d199898..80e9d00 100644
--- a/src/parallels/parallels_sdk.c
+++ b/src/parallels/parallels_sdk.c
@@ -462,7 +462,7 @@ prlsdkGetDiskInfo(PRL_HANDLE prldisk,
PRL_UINT32 emulatedType;
PRL_UINT32 ifType;
PRL_UINT32 pos;
-PRL_UINT32 prldiskIndex;
+virDomainDeviceDriveAddressPtr address;
int ret = -1;
pret = PrlVmDev_GetEmulatedType(prldisk, &emulatedType);
@@ -499,15 +499,32 @@ prlsdkGetDiskInfo(PRL_HANDLE prldisk,
pret = PrlVmDev_GetIfaceType(prldisk, &ifType);
prlsdkCheckRetGoto(pret, cleanup);
+
+pret = PrlVmDev_GetStackIndex(prldisk, &pos);
+prlsdkCheckRetGoto(pret, cleanup);
+
+address = &disk->info.addr.drive;
switch (ifType) {
case PMS_IDE_DEVICE:
disk->bus = VIR_DOMAIN_DISK_BUS_IDE;
+disk->dst = virIndexToDiskName(pos, "hd");
+address->bus = pos / 2;
+address->target = 0;
+address->unit = pos % 2;
break;
case PMS_SCSI_DEVICE:
disk->bus = VIR_DOMAIN_DISK_BUS_SCSI;
+disk->dst = virIndexToDiskName(pos, "sd");
+address->bus = 0;
+address->target = 0;
+address->unit = pos;
break;
case PMS_SATA_DEVICE:
disk->bus = VIR_DOMAIN_DISK_BUS_SATA;
+disk->dst = virIndexToDiskName(pos, "sd");
+address->bus = 0;
+address->target = 0;
+address->unit = pos;
break;
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -516,17 +533,10 @@ prlsdkGetDiskInfo(PRL_HANDLE prldisk,
break;
}
-pret = PrlVmDev_GetStackIndex(prldisk, &pos);
-prlsdkCheckRetGoto(pret, cleanup);
+if (!disk->dst)
+goto cleanup;
disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
-disk->info.addr.drive.target = pos;
-
-pret = PrlVmDev_GetIndex(prldisk, &prldiskIndex);
-prlsdkCheckRetGoto(pret, cleanup);
-
-if (!(disk->dst = virIndexToDiskName(prldiskIndex, "sd")))
-goto cleanup;
ret = 0;
@@ -2852,6 +2862,7 @@ static int prlsdkAddDisk(PRL_HANDLE sdkdom, virDomainDiskDefPtr disk, bool bootD
virDomainDeviceDriveAddressPtr drive;
PRL_UINT32 devIndex;
PRL_DEVICE_TYPE devType;
+char *dst = NULL;
if (prlsdkCheckDiskUnsupportedParams(disk) < 0)
return -1;
@@ -2909,22 +2920,50 @@ static int prlsdkAddDisk(PRL_HANDLE sdkdom,
virDomainDiskDefPtr disk, bool bootD
/* We have only one controller of each type */
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid drive "
"address of disk %s, Parallels Cloud Server has "
- "only one controller."), disk->src->path);
+ "only one controller."), disk->dst);
+goto cleanup;
+}
+
+if (drive->target > 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid drive "
+ "address of disk %s, Parallels Cloud Server has "
+ "only target 0."), disk->dst);
goto cleanup;
}
switch (disk->bus) {
case VIR_DOMAIN_DISK_BUS_IDE:
+if (drive->unit > 1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid drive "
+ "address of disk %s, Parallels Cloud Server has "
+ "only units 0-1 for IDE bus."), disk->dst);
+goto cleanup;
+}
sdkbus = PMS_IDE_DEVICE;
idx = 2 * drive->bus + drive->unit;
+dst = virIndexToDiskName(idx, "hd");
break;
case VIR_DOMAIN_DISK_BUS_SCSI:
+if (drive->bus > 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid drive "
+ "address of disk %s, Parallels Cloud Server has "
+ "only bus 0 for SCSI bus."), disk->dst);
+goto cleanup;
+}
sdkbus = PMS_SCSI_DEVICE;
idx = drive->unit;
+dst = virIndexToDiskName(idx, "sd");
break;
case VIR_DOMAIN_DISK_BUS_SATA:
+if (drive->bus > 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid drive "
+ "address of disk %s, Parallels Cloud Server has "
+ "only bus 0 f
Re: [libvirt] [PATCH v2] parallels: use SDK disks system names for paths
On 04/20/2015 12:56 PM, Nikolay Shirokovskiy wrote: For block devices SDK friendly name do not refer to block device path but rather some description while system name refer to device path. For ploop devices both names refer to image path. Thus system name is better choice. Signed-off-by: Nikolay Shirokovskiy --- src/parallels/parallels_sdk.c |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index 72c1773..e1ba7cf 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -485,13 +485,13 @@ prlsdkGetDiskInfo(PRL_HANDLE prldisk, disk->device = VIR_DOMAIN_DISK_DEVICE_DISK; } -pret = PrlVmDev_GetFriendlyName(prldisk, NULL, &buflen); +pret = PrlVmDev_GetSysName(prldisk, NULL, &buflen); prlsdkCheckRetGoto(pret, cleanup); if (VIR_ALLOC_N(buf, buflen) < 0) goto cleanup; -pret = PrlVmDev_GetFriendlyName(prldisk, buf, &buflen); +pret = PrlVmDev_GetSysName(prldisk, buf, &buflen); prlsdkCheckRetGoto(pret, cleanup); Unfortunately, we should use PrlVmDev_GetFriendlyName for block devices and PrlVmDev_GetSysName for other types of disks. if (virDomainDiskSetSource(disk, buf) < 0) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 1/4] storage: Split out the valid path check
On 04/20/2015 12:23 PM, Eric Blake wrote:
> On 04/19/2015 06:38 PM, John Ferlan wrote:
>> For virStorageBackendStablePath, in order to make decisions in other code
>> split out the checks regarding whether the pool's target is empty, using
>> /dev,
>> using /dev/, or doesn't start with /dev
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/storage/storage_backend.c | 26 +-
>> src/storage/storage_backend.h | 1 +
>> 2 files changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
>> index 0435983..b07e0d9 100644
>> --- a/src/storage/storage_backend.c
>> +++ b/src/storage/storage_backend.c
>> @@ -1674,6 +1674,17 @@
>> virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target,
>> return 0;
>> }
>>
>> +bool
>> +virStorageBackendPoolPathIsStable(const char *path)
>> +{
>> +if (path == NULL || STREQ(path, "/dev") || STREQ(path, "/dev/"))
>> +return false;
>> +
>> +if (!STRPREFIX(path, "/dev"))
>> +return false;
>
> I think you want "/dev/" here as the prefix to be required; otherwise,
> "/device" would match the prefix. (This also means that someone using
> "//dev/..." would fail the check, but that's probably something we don't
> need to worry about).
>
Hmm... Sure I see that... I can make that adjustment. I'll wait a bit
before pushing just so see if there's other feedback...
John
>
>> -/* Short circuit if pool has no target, or if its /dev */
>> -if (pool->def->target.path == NULL ||
>> -STREQ(pool->def->target.path, "/dev") ||
>> -STREQ(pool->def->target.path, "/dev/"))
>> -goto ret_strdup;
>> -
>> -/* Skip whole thing for a pool which isn't in /dev
>> - * so we don't mess filesystem/dir based pools
>> - */
>> -if (!STRPREFIX(pool->def->target.path, "/dev"))
>> -goto ret_strdup;
>
> Of course, the bug was pre-existing in the code you are just refactoring
> from, but now that we've spotted it, we might as well fix it.
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 2/4] scsi: Adjust return value for virStorageBackendSCSINewLun
...
>> +/* Check if the pool is using a stable target path. The call to
>> + * virStorageBackendStablePath will fail if the pool target path
>> + * isn't stable and just return the strdup'd 'devpath' anyway.
>> + * This would be indistinguishable to failing to find the stable
>> + * path to the device if the virDirRead loop to search the
>> + * target pool path for our devpath had failed.
>> + */
>> +if (!virStorageBackendPoolPathIsStable(pool->def->target.path)) {
>> +virReportError(VIR_ERR_INVALID_ARG,
>> + _("unable to use target path '%s' for dev '%s'"),
>> + NULLSTR(pool->def->target.path), dev);
>> +goto cleanup;
>> +}
>
> /dev is a valid non-stable pool target path.
>
>> +
>> if (VIR_ALLOC(vol) < 0)
>> goto cleanup;
>>
>> @@ -187,13 +211,12 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>> true)) == NULL)
>> goto cleanup;
>>
>> -if (STREQ(devpath, vol->target.path) &&
>> -!(STREQ(pool->def->target.path, "/dev") ||
>> - STREQ(pool->def->target.path, "/dev/"))) {
>> +if (STREQ(devpath, vol->target.path)) {
>>
>
> Before, when virStorageBackendStablePath returned the same devpath because
> the pool path was "/dev", we continued with processing the volume.
>
> After this patch, we won't even get here because of the first check.
>
> Failure to stabilize the path should be expected here, if the pool
> target path is not stable.
>
OK, but because virStorageBackendStablePath won't process the
pool->target.path of "/dev", we'll return the duplicated 'devpath' and
return -2 for every volume in the pool thus making it look empty.
What good is that?
Wouldn't it be better to tell the user that "/dev" is not a valid stable
path name... The path really needs to be more specific... I suppose one
could change virStorageBackendStablePath to accept "/dev" and do the
search, but that "could" take a while depending on the size of the
"/dev" file system.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] domtop: Properly free cpu status
On 04/20/2015 05:41 AM, Michal Privoznik wrote: > So, in the example the cpu stats are collected within a function > called do_top. At the beginning of the function we ask the daemon for > how much vCPUs can we get stats, and how many stats for a vCPU can we > get. This is because it's how our API works - users are required to > preallocate a chunk of memory for the results. Now, at the end, we try > to free the allocated array, but we are not doing it correctly. > There's this virTypedParamsFree() function which gets a pointer to the > array and the length of the array. However, if there was an error in > getting vCPU stats we pass a negative number instead of the originally > computed value. This flaw results in SIGSEGV: > > libvirt: QEMU Driver error : Requested operation is not valid: domain is not > running > ERROR do_top:333 : Unable to get cpu stats > ==29201== Invalid read of size 4 > ==29201==at 0x4F1DF8B: virTypedParamsClear (virtypedparam.c:1145) > ==29201==by 0x4F1DFEB: virTypedParamsFree (virtypedparam.c:1165) > ==29201==by 0x4023C3: do_top (domtop.c:349) > ==29201==by 0x40260B: main (domtop.c:386) > ==29201== Address 0x131cd7c0 is 16 bytes after a block of size 768 alloc'd > ==29201==at 0x4C2C070: calloc (in > /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) > ==29201==by 0x401FF1: do_top (domtop.c:295) > ==29201==by 0x40260B: main (domtop.c:386) > > Signed-off-by: Michal Privoznik > --- > examples/domtop/domtop.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/examples/domtop/domtop.c b/examples/domtop/domtop.c > index e50988e..2283994 100644 > --- a/examples/domtop/domtop.c > +++ b/examples/domtop/domtop.c > @@ -346,8 +346,8 @@ do_top(virConnectPtr conn, > > ret = 0; > cleanup: > -virTypedParamsFree(now_params, now_nparams * max_id); > -virTypedParamsFree(then_params, then_nparams * max_id); > +virTypedParamsFree(now_params, nparams * max_id); > +virTypedParamsFree(then_params, nparams * max_id); nparams can also be set to -1 when we reach cleanup (if the attempt to initially set it results in an error). But of course in those cases now_params and then_params will still be NULL, so virTypedParamsFree() will be a NOP (and will ignore the count entirely), so that's okay. ACK. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] xend: Remove a couple of unused function prototypes.
Commit 70f446631f142ae92b4d4eb349fcf11408171556 (from 2008) introduced some functions for testing whether xend was returning correct sound models. Those functions have long gone, but the function prototypes remain. This commit removes the unused prototypes. Signed-off-by: Richard W.M. Jones --- src/xen/xend_internal.h | 4 1 file changed, 4 deletions(-) diff --git a/src/xen/xend_internal.h b/src/xen/xend_internal.h index 814330d..9064b47 100644 --- a/src/xen/xend_internal.h +++ b/src/xen/xend_internal.h @@ -84,10 +84,6 @@ xenDaemonDomainFetch(virConnectPtr xend, const char *cpus); - int is_sound_model_valid(const char *model); - int is_sound_model_conflict(const char *model, const char *soundstr); - - /* refactored ones */ int xenDaemonOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags); -- 2.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 1/4] storage: Split out the valid path check
On 04/19/2015 06:38 PM, John Ferlan wrote:
> For virStorageBackendStablePath, in order to make decisions in other code
> split out the checks regarding whether the pool's target is empty, using /dev,
> using /dev/, or doesn't start with /dev
>
> Signed-off-by: John Ferlan
> ---
> src/storage/storage_backend.c | 26 +-
> src/storage/storage_backend.h | 1 +
> 2 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 0435983..b07e0d9 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -1674,6 +1674,17 @@
> virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target,
> return 0;
> }
>
> +bool
> +virStorageBackendPoolPathIsStable(const char *path)
> +{
> +if (path == NULL || STREQ(path, "/dev") || STREQ(path, "/dev/"))
> +return false;
> +
> +if (!STRPREFIX(path, "/dev"))
> +return false;
I think you want "/dev/" here as the prefix to be required; otherwise,
"/device" would match the prefix. (This also means that someone using
"//dev/..." would fail the check, but that's probably something we don't
need to worry about).
> -/* Short circuit if pool has no target, or if its /dev */
> -if (pool->def->target.path == NULL ||
> -STREQ(pool->def->target.path, "/dev") ||
> -STREQ(pool->def->target.path, "/dev/"))
> -goto ret_strdup;
> -
> -/* Skip whole thing for a pool which isn't in /dev
> - * so we don't mess filesystem/dir based pools
> - */
> -if (!STRPREFIX(pool->def->target.path, "/dev"))
> -goto ret_strdup;
Of course, the bug was pre-existing in the code you are just refactoring
from, but now that we've spotted it, we might as well fix it.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/7] storage: Check for duplicate host for incoming pool def
On 04/20/2015 11:11 AM, Ján Tomko wrote:
> On Sun, Apr 19, 2015 at 08:49:12PM -0400, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1171984
>>
>> Use the new virIsSameHostnameInfo API to determine whether the proposed
>> storage pool definition matches the existing storage pool definition
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/conf/storage_conf.c | 11 ++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index 4852dfb..c1bc242 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -2415,7 +2415,16 @@
>> virStoragePoolSourceMatchSingleHost(virStoragePoolSourcePtr poolsrc,
>> if (poolsrc->hosts[0].port != defsrc->hosts[0].port)
>> return false;
>>
>
> This function is called when parsing the configuration, which should not
> depend on host state.
>
> For example, if libvirt is started really early at boot time and the
> hostnames cannot be resolved by the DNS yet, they will pass the check
> but they will disappear on libvirtd restart.
Hmm... the downside of unreliable dependencies.
>
> The hostname->ip pairings are not stable either, so if we do this, I
> think it should be done on pool startup, not config parsing.
>
Right, but by the time we get to pool startup we'd be at the same point
as referenced above - if done early enough at boot time, then it's not
going to fail, but perhaps better than nothing.
I suppose at least moving to startup allows for better error paths since
currently errors can be overwritten or ignored.
>> -return STREQ(poolsrc->hosts[0].name, defsrc->hosts[0].name);
>> +if (STRNEQ(poolsrc->hosts[0].name, defsrc->hosts[0].name)) {
>> +/* Matching just a name isn't reliable as someone could provide
>> + * the name for one pool and the IP Address for another pool, so
>
> Resolving them is IMHO just as unreliable.
>
> Re: the original bug - is it possible to check that we have connected to
> a session with a different hostname than what we requested?
What does the connected session hostname have to do with the original
bug? The bug requests checking that an iSCSI pool doesn't use a "
and a resolved name for another pool on the same host:
Where :
# cat /etc/hosts
10.66.6.12 test1
The bug pointed out iSCSI in particular, but since other pools use the
... /> XML formatting it seemed logical
to have them all use the same checks.
>
> Or just disallow starting two pools with the same targetname, since they
> are supposed to be unique?
>
'targetname' as in ?
A 'target.path' per pool would need to be unique, but using the same
target.path into two different networked pools is something that should
work. And the pool target path (/dev/disk/by-path) is used by multiple
iSCSI pools on the same host, so it cannot be used as something unique
per host.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 3/4] scsi: Change return values for virStorageBackendSCSIFindLUs
On Sun, Apr 19, 2015 at 08:38:35PM -0400, John Ferlan wrote: > Rather than passing/returning a pointer to a boolean to indicate that > perhaps we should try again - adjust the return of the call to return > the count of LU's found during processing, then let the caller decide > what to do with that value. > > Signed-off-by: John Ferlan > --- > src/storage/storage_backend_scsi.c | 34 ++ > 1 file changed, 14 insertions(+), 20 deletions(-) > ACK Jan signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 2/4] scsi: Adjust return value for virStorageBackendSCSINewLun
On Sun, Apr 19, 2015 at 08:38:34PM -0400, John Ferlan wrote:
> Use virStorageBackendPoolUseDevPath API to determine whether creation of
> stable target path is possible for the volume.
>
> This will differentiate a failed virStorageBackendStablePath which won't
> need to be fatal. Thus, we'll add a -2 return value to differentiate that
> the failure was a result of either the inability to find the symlink for
> the device or failure to open the target path directory
>
> Signed-off-by: John Ferlan
> ---
> src/storage/storage_backend_scsi.c | 29 ++---
> 1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/src/storage/storage_backend_scsi.c
> b/src/storage/storage_backend_scsi.c
> index b96caec..ae3cd9a 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -146,6 +146,16 @@ virStorageBackendSCSISerial(const char *dev)
> }
>
>
> +/*
> + * Attempt to create a new LUN
> + *
> + * Returns:
> + *
> + * 0 => Success
> + * -1 => Failure due to some sort of OOM or other fatal issue found when
> + *attempting to get/update information about a found volume
> + * -2 => Failure to find a stable path, not fatal, caller can try another
> + */
> static int
> virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
> uint32_t host ATTRIBUTE_UNUSED,
> @@ -158,6 +168,20 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
> char *devpath = NULL;
> int retval = -1;
>
> +/* Check if the pool is using a stable target path. The call to
> + * virStorageBackendStablePath will fail if the pool target path
> + * isn't stable and just return the strdup'd 'devpath' anyway.
> + * This would be indistinguishable to failing to find the stable
> + * path to the device if the virDirRead loop to search the
> + * target pool path for our devpath had failed.
> + */
> +if (!virStorageBackendPoolPathIsStable(pool->def->target.path)) {
> +virReportError(VIR_ERR_INVALID_ARG,
> + _("unable to use target path '%s' for dev '%s'"),
> + NULLSTR(pool->def->target.path), dev);
> +goto cleanup;
> +}
/dev is a valid non-stable pool target path.
> +
> if (VIR_ALLOC(vol) < 0)
> goto cleanup;
>
> @@ -187,13 +211,12 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
> true)) == NULL)
> goto cleanup;
>
> -if (STREQ(devpath, vol->target.path) &&
> -!(STREQ(pool->def->target.path, "/dev") ||
> - STREQ(pool->def->target.path, "/dev/"))) {
> +if (STREQ(devpath, vol->target.path)) {
>
Before, when virStorageBackendStablePath returned the same devpath because
the pool path was "/dev", we continued with processing the volume.
After this patch, we won't even get here because of the first check.
Failure to stabilize the path should be expected here, if the pool
target path is not stable.
Jan
> VIR_DEBUG("No stable path found for '%s' in '%s'",
>devpath, pool->def->target.path);
>
> +retval = -2;
> goto cleanup;
> }
>
> --
> 2.1.0
>
> --
> 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 v4 1/4] storage: Split out the valid path check
s/valid/stable/ in the subject On Sun, Apr 19, 2015 at 08:38:33PM -0400, John Ferlan wrote: > For virStorageBackendStablePath, in order to make decisions in other code > split out the checks regarding whether the pool's target is empty, using /dev, > using /dev/, or doesn't start with /dev > > Signed-off-by: John Ferlan > --- > src/storage/storage_backend.c | 26 +- > src/storage/storage_backend.h | 1 + > 2 files changed, 14 insertions(+), 13 deletions(-) > ACK Jan signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] qemu: add QEMU_CAPS_MACHINE_VMPORT_OPT
On 04/15/2015 02:34 AM, Martin Kletzander wrote: >> But this will break on qemu 2.3, unless something changes there. I >> don't want to do a version check, but I also don't want something that >> works by accident in qemu 2.2 start failing with qemu 2.3. >> > > I'm failing to find any compromise here. We would have to wait for > QEMU to fix the introspection, but in the meantime we'd be unable to > fix it ourselves. And since 2.3 is right around the corner, we'd have > to wait for 2.4. > > Can we at least add all the patches except the hunks that set the > capability and decide later on how to enable that, at least for any > possible adopters to minimize their need for non-upstream patches? I could live with a compromise patch that merges both approaches - query-command-line-options for the qemu versions that work, and a hard-coded version check for qemu 2.3 where it doesn't work. That way, if someone backports it to 2.2 (and includes as part of their backport the gunk to get QMP command line detection working), libvirt can use it; and when using upstream 2.3, we use it by virtue of version numbers. It's a bit annoying that the feature is machine-specific; we have to be careful that the hard-coded version check only enables the libvirt capability bit on machines where it is likely to work in qemu (that is, the reason that qemu 2.3 no longer advertises the bit is that it is not global to all machines; so if powerpc doesn't support it, we must not try to use the bit when targetting a powerpc qemu). -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 3/5] qemu: add QEMU_CAPS_MACHINE_VMPORT_OPT
On 20 April 2015 at 13:39, Marc-André Lureau wrote:
> Hi
>
> On Tue, Apr 14, 2015 at 6:07 PM, Eric Blake wrote:
>>
>> > +{ "machine", "vmport", QEMU_CAPS_MACHINE_VMPORT_OPT },
>>
>> Ouch. qemu commit 0a7cf21 fixes what would have been a regression in
>> 2.3 at exposing "mem-merge" through query-command-line-options, but it
>> does NOT expose "vmport", which is a per-architecture option rather than
>> a generic -machine option. Which means that even though qemu 2.2
>> (perhaps wrongly) advertised "vmport" for all machines (even when it was
>> not supported), 2.3 will not advertise it, and we are hoping for a
>> better solution in 2.4 for properly advertising vmport on an
>> as-appropriate basis.
>
>
> Thanks Eric for finding out this regression.
>
> Is anybody working on a better solution? I can imagine either a new qmp
> machine-list-properties, or perhaps more reasonably a new properties array
> in query-machines reply.
Unless this is an absolutely critical "we cannot ship 2.3 without
a fix" bug (in which case somebody should yell now and you really
need a patch on the list within a day or so), you've probably missed
the 2.3 boat at this point.
-- PMM
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/7] virutil: Introduce virIsValidHostname
On 04/20/2015 10:34 AM, Peter Krempa wrote: > On Mon, Apr 20, 2015 at 15:40:32 +0200, Peter Krempa wrote: >> On Sun, Apr 19, 2015 at 20:49:06 -0400, John Ferlan wrote: >>> Similar to virGetHostname, but this time taking a parameter which is a >>> hostname or ipaddress from a ... /> >>> XML property and validating that the name can be resolved. >>> >>> Return true or false depending on whether we can ascertain the hostname >>> address from calls to 'getnameinfo' and 'getaddrinfo'. Subsequent patches >>> will be validating a proposed pool hostname definition against existing >>> pool hostnames to ensure they are not the same hostname (and thus having >>> two pools looking at the same data) >>> >>> Signed-off-by: John Ferlan >>> --- >>> src/libvirt_private.syms | 1 + >>> src/util/virutil.c | 49 >>> >>> src/util/virutil.h | 1 + >>> 3 files changed, 51 insertions(+) >>> >>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms >>> index 8c37303..5ba9635 100644 >>> --- a/src/libvirt_private.syms >>> +++ b/src/libvirt_private.syms >>> @@ -2324,6 +2324,7 @@ virIsCapableFCHost; >>> virIsCapableVport; >>> virIsDevMapperDevice; >>> virIsSUID; >>> +virIsValidHostname; >>> virManageVport; >>> virMemoryLimitIsSet; >>> virMemoryLimitTruncate; >>> diff --git a/src/util/virutil.c b/src/util/virutil.c >>> index 79cdb7a..f6cc9af 100644 >>> --- a/src/util/virutil.c >>> +++ b/src/util/virutil.c >>> @@ -690,6 +690,55 @@ char *virGetHostname(void) >>> } >>> >>> >>> +/* >>> + * virIsValidHostname: >>> + * >>> + * Unlike virGetHostname, this variant of the code receives a hostname >>> + * retrieves the getaddrinfo. If the passed hostname can be successfully >>> + * and if successfully resolved via getaddrinfo, then success is declared. >>> + * >>> + * On error in lookup, if an IP Address is not found, or error formatting >>> + * the name, an error is generated and a NULL is returned. >>> + * >>> + * On success, a pointer to a character representation of the numeric IP >>> + * Address is returned. >>> + * >>> + * It is the caller's responsibility to check for a non NULL return and >>> + * VIR_FREE the resultant string. >> >> This comment does not describe how this function works. >> Err... Oh yeah - I changed how I did things, but forget to update the comment, >>> + */ >>> +bool >>> +virIsValidHostname(const char *hostname) > > As a hindsight from reviewing 6/7. This function should also be in > virsocketaddr.c > > hmmm.. yes I see.. Guess I got hung up on "virSocketAddr..." and didn't focus as closely on the implementation where virSocketAddrParse can take NULL as the first parameter... Guess, that means patches 2-5 can just be called as: if (virSocketAddrParse(NULL, pool->def->source.hosts[0].name, AF_UNSPEC) < 0) Tks - John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/7] storage: Check for duplicate host for incoming pool def
On Sun, Apr 19, 2015 at 08:49:12PM -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1171984
>
> Use the new virIsSameHostnameInfo API to determine whether the proposed
> storage pool definition matches the existing storage pool definition
>
> Signed-off-by: John Ferlan
> ---
> src/conf/storage_conf.c | 11 ++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 4852dfb..c1bc242 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -2415,7 +2415,16 @@
> virStoragePoolSourceMatchSingleHost(virStoragePoolSourcePtr poolsrc,
> if (poolsrc->hosts[0].port != defsrc->hosts[0].port)
> return false;
>
This function is called when parsing the configuration, which should not
depend on host state.
For example, if libvirt is started really early at boot time and the
hostnames cannot be resolved by the DNS yet, they will pass the check
but they will disappear on libvirtd restart.
The hostname->ip pairings are not stable either, so if we do this, I
think it should be done on pool startup, not config parsing.
> -return STREQ(poolsrc->hosts[0].name, defsrc->hosts[0].name);
> +if (STRNEQ(poolsrc->hosts[0].name, defsrc->hosts[0].name)) {
> +/* Matching just a name isn't reliable as someone could provide
> + * the name for one pool and the IP Address for another pool, so
Resolving them is IMHO just as unreliable.
Re: the original bug - is it possible to check that we have connected to
a session with a different hostname than what we requested?
Or just disallow starting two pools with the same targetname, since they
are supposed to be unique?
Jan
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/7] virutil: Introduce virIsValidHostname
On Sun, Apr 19, 2015 at 08:49:06PM -0400, John Ferlan wrote:
> Similar to virGetHostname, but this time taking a parameter which is a
> hostname or ipaddress from a ... />
> XML property and validating that the name can be resolved.
>
> Return true or false depending on whether we can ascertain the hostname
> address from calls to 'getnameinfo' and 'getaddrinfo'. Subsequent patches
> will be validating a proposed pool hostname definition against existing
> pool hostnames to ensure they are not the same hostname (and thus having
> two pools looking at the same data)
Why do we need this function? The failure to resolve the host is already handled
by the underlying commands.
I don't think adding all this code just to save us a fork on wrong input is
worth it.
> +bool
> +virIsValidHostname(const char *hostname)
> +{
> +int r;
> +struct addrinfo hints, *info;
> +
> +if (STRPREFIX(hostname, "localhost") ||
> +STREQ(hostname, "127.0.0.1") || STREQ(hostname, "::1"))
getaddrinfo handles these two numeric representations of localhost just
fine, just as the rest of them (see DO_TEST_LOCALHOST in sockettest.c
for a few interesting examples)
Jan
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/2] rework the check for the numa cpus
On 02.04.2015 10:02, Luyao Huang wrote: > We introduce a check for numa cpu total count in 5f7b71, > But seems this check cannot work well. There are some scenarioes: > > 1. one of cpu id is out of maxvcpus, can set success(cpu count = 5 < 10): > > 10 > > > the cpus '100' exceed maxvcpus, this setting is not valid (although qemu > do not output error). > > 2. use the same cpu in 2 cell, can set success(cpu count = 8 < 10): > 10 > > > > I guess nobody will use this setting if he really want use numa in his > vm. (qemu not output error, but we can find some interesting things in > vm, all cpus have bind in one numa node) > > 3. use the same cpu in 2 cell, cannot set success(cpu count = 11 > 10): > 10 > > > > No need forbid this scenario if scenario 2 is a 'valid' setting. > > However add new check during parse xml will make vm has broken settings > disappear after update libvirtd, so possible solutions: > > 1. add new check when parse xml, and find a way to avoid vm evaporate. > I chose this way and i don't think just drop the invalid settings is a good > idea for numa cpus, so i add a new function. > > 2. add new check when start vm. > I think this is a configuration issue, so no need report it so late. > > 3. just remove this check and do not add new check... :) > > Luyao Huang (2): > conf: introduce a new function to add check avoid losing track > conf: rework the cpu check for vm numa settings > > src/bhyve/bhyve_driver.c | 4 ++-- > src/conf/domain_conf.c | 21 ++--- > src/conf/domain_conf.h | 1 + > src/conf/numa_conf.c | 37 ++--- > src/conf/numa_conf.h | 2 +- > src/esx/esx_driver.c | 2 +- > src/libxl/libxl_driver.c | 4 ++-- > src/lxc/lxc_driver.c | 4 ++-- > src/openvz/openvz_driver.c | 4 ++-- > src/parallels/parallels_driver.c | 2 +- > src/phyp/phyp_driver.c | 2 +- > src/qemu/qemu_driver.c | 4 ++-- > src/test/test_driver.c | 4 ++-- > src/uml/uml_driver.c | 4 ++-- > src/vbox/vbox_common.c | 2 +- > src/vmware/vmware_driver.c | 4 ++-- > src/xen/xen_driver.c | 4 ++-- > src/xenapi/xenapi_driver.c | 4 ++-- > 18 files changed, 70 insertions(+), 39 deletions(-) > I agree with renaming and extending the virDomainNumaGetCPUCountTotal(). Even the diff in 2/2 shows the function is utterly broken. However, I think this function can be called unconditionally - even when the flag is not set. Having said that, the flag is redundant. Moreover, when sending a patchset, the sources should compile cleanly after each patch. This is not the case with this one. Looking forward to v2. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/7] virutil: Introduce virIsValidHostname
On Mon, Apr 20, 2015 at 15:40:32 +0200, Peter Krempa wrote: > On Sun, Apr 19, 2015 at 20:49:06 -0400, John Ferlan wrote: > > Similar to virGetHostname, but this time taking a parameter which is a > > hostname or ipaddress from a ... /> > > XML property and validating that the name can be resolved. > > > > Return true or false depending on whether we can ascertain the hostname > > address from calls to 'getnameinfo' and 'getaddrinfo'. Subsequent patches > > will be validating a proposed pool hostname definition against existing > > pool hostnames to ensure they are not the same hostname (and thus having > > two pools looking at the same data) > > > > Signed-off-by: John Ferlan > > --- > > src/libvirt_private.syms | 1 + > > src/util/virutil.c | 49 > > > > src/util/virutil.h | 1 + > > 3 files changed, 51 insertions(+) > > > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > > index 8c37303..5ba9635 100644 > > --- a/src/libvirt_private.syms > > +++ b/src/libvirt_private.syms > > @@ -2324,6 +2324,7 @@ virIsCapableFCHost; > > virIsCapableVport; > > virIsDevMapperDevice; > > virIsSUID; > > +virIsValidHostname; > > virManageVport; > > virMemoryLimitIsSet; > > virMemoryLimitTruncate; > > diff --git a/src/util/virutil.c b/src/util/virutil.c > > index 79cdb7a..f6cc9af 100644 > > --- a/src/util/virutil.c > > +++ b/src/util/virutil.c > > @@ -690,6 +690,55 @@ char *virGetHostname(void) > > } > > > > > > +/* > > + * virIsValidHostname: > > + * > > + * Unlike virGetHostname, this variant of the code receives a hostname > > + * retrieves the getaddrinfo. If the passed hostname can be successfully > > + * and if successfully resolved via getaddrinfo, then success is declared. > > + * > > + * On error in lookup, if an IP Address is not found, or error formatting > > + * the name, an error is generated and a NULL is returned. > > + * > > + * On success, a pointer to a character representation of the numeric IP > > + * Address is returned. > > + * > > + * It is the caller's responsibility to check for a non NULL return and > > + * VIR_FREE the resultant string. > > This comment does not describe how this function works. > > > + */ > > +bool > > +virIsValidHostname(const char *hostname) As a hindsight from reviewing 6/7. This function should also be in virsocketaddr.c 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 7/7] storage: Check for duplicate host for incoming pool def
On Sun, Apr 19, 2015 at 20:49:12 -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1171984
>
> Use the new virIsSameHostnameInfo API to determine whether the proposed
> storage pool definition matches the existing storage pool definition
>
> Signed-off-by: John Ferlan
> ---
> src/conf/storage_conf.c | 11 ++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 4852dfb..c1bc242 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -2415,7 +2415,16 @@
> virStoragePoolSourceMatchSingleHost(virStoragePoolSourcePtr poolsrc,
> if (poolsrc->hosts[0].port != defsrc->hosts[0].port)
> return false;
>
> -return STREQ(poolsrc->hosts[0].name, defsrc->hosts[0].name);
> +if (STRNEQ(poolsrc->hosts[0].name, defsrc->hosts[0].name)) {
This check is almost worthless. In the normal use case the user won't
try to define the pool with same name twice. On many other cases the
pool def might use different hostnames or so. Using the resolve helper
always might save this condition
> +/* Matching just a name isn't reliable as someone could provide
> + * the name for one pool and the IP Address for another pool, so
> + * for a "true" check we must compare the resolved name, but that's
> + * expensive, so only do this check if using different names
> + */
... and you wouldn't need to explain why you've done it this way.
> +return virIsSameHostnameInfo(poolsrc->hosts[0].name,
> + defsrc->hosts[0].name);
> +}
> +return true;
> }
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 6/7] util: Introduce virIsSameHostnameInfo
On Sun, Apr 19, 2015 at 20:49:11 -0400, John Ferlan wrote:
> In order to assist with existing pool source hostname validation against
> an incoming pool source definition, we need a way to validate that the
> resolved hostname provided for the pool doesn't match some existing
> pool; otherwise, we'd have a scenario where two storage pools could be
> looking at the same data.
>
> Search through all the existing pools resolved names against the proposed
> definitions resolved hostnames - if any match, we return true; otherwise,
> false is returned
>
> Signed-off-by: John Ferlan
> ---
> src/libvirt_private.syms | 1 +
> src/util/virutil.c | 106
> +++
> src/util/virutil.h | 2 +
> 3 files changed, 109 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 5ba9635..a5f4d7f 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2323,6 +2323,7 @@ virIndexToDiskName;
> virIsCapableFCHost;
> virIsCapableVport;
> virIsDevMapperDevice;
> +virIsSameHostnameInfo;
> virIsSUID;
> virIsValidHostname;
> virManageVport;
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index f6cc9af..c152b8c 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -739,6 +739,112 @@ virIsValidHostname(const char *hostname)
> }
>
>
> +/*
> + * virIsSameHostnameInfo:
The hostname is not same as the STREQ check is not enough as explained
below. virIsSameHost perhaps?
> + *
> + * Take two hostname representations and compare their 'getnameinfo' results
> + * to ensure that they differ. This will be used by the network storage pool
> + * validation to ensure an incoming definition doesn't match some existing
> + * pool definition just because someone tried to change the hostname string
> + * representation. For example, "localhost" and "127.0.0.1" would fail the
> + * plain STREQ check, but they essentially are the same host, so the incoming
> + * definition should be rejected since a pool for the host already exists.
This function is under the genric utils section so the docs should
explain its general usage.
> + *
> + * @poolhostname: String stored as the pool's hostname
> + * @defhostname: String to be used by the def's hostname
Same here, the variable naming and this comment doesn't hint that the
function is universal.
> + *
> + * Returns true if they are the same, false if not
"may report libvirt error if false is returned" ... but see below [1]
> + */
> +bool
> +virIsSameHostnameInfo(const char *poolhostname,
> + const char *defhostname)
> +{
> +int r;
> +struct addrinfo poolhints, *poolinfo = NULL, *poolcur;
> +struct addrinfo defhints, *definfo = NULL, *defcur;
The hints argument for getaddrinfo is const so you don't neet two
separate ones [2] ...
> +char poolhost[NI_MAXHOST];
> +char defhost[NI_MAXHOST];
These take up 2KiB on the stack, wouldn't it be worth allocate them on
the heap?
> +
> +memset(&poolhints, 0, sizeof(poolhints));
> +poolhints.ai_family = AF_UNSPEC;
> +poolhints.ai_protocol = IPPROTO_TCP;
> +
> +if ((r = getaddrinfo(poolhostname, NULL, &poolhints, &poolinfo)) != 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("IP address lookup for poolhostname '%s' failed:
> %s"),
> + poolhostname, gai_strerror(r));
[1] Since you are using this function to check that the addresses and
the 'false' case is when you define the pool, every single error
reported by this function will just pollute logs. Additionally the
function that is calling this is not resetting the error, so you might
get an inaccurate error if something forgets to set error later.
> +return false;
> +}
> +
> +if (!poolinfo) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("No IP address for poolhostname '%s' found"),
> + poolhostname);
> +return false;
> +}
> +
> +memset(&defhints, 0, sizeof(defhints));
> +defhints.ai_family = AF_UNSPEC;
> +defhints.ai_protocol = IPPROTO_TCP;
[2] ... and can avoid one of these.
> +
> +if ((r = getaddrinfo(defhostname, NULL, &defhints, &definfo)) != 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("IP address lookup for defhostname '%s' failed:
> %s"),
> + defhostname, gai_strerror(r));
> +goto cleanup;
> +}
> +
> +if (!definfo) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("No IP address for defhostname '%s' found"),
> + defhostname);
> +goto cleanup;
> +}
> +
> +for (poolcur = poolinfo; poolcur; poolcur = poolcur->ai_next) {
> +
> +if ((r = getnameinfo(poolcur->ai_addr, poolcur->ai_addrlen,
> + poolhost, sizeof(poolhost), NULL, 0,
> + NI_NUMERICHOST
Re: [libvirt] [PATCH] lxc: fix 2 issue around cpuset
On Fri, Apr 03, 2015 at 06:11:15PM +0800, Luyao Huang wrote: > There are two bugs in this function: > > 1. cannot start a vm with cpuset but without numatune settings > > # virsh -c lxc:/// start helloworld > error: Failed to start domain helloworld > error: internal error: guest failed to start: Invalid value '1-3' for > 'cpuset.mems': Invalid argument > > we don't free &mask after use it for virCgroupSetCpusetCpus() and > then virDomainNumatuneMaybeFormatNodeset() do not get a new &mask, > then we use it in virCgroupSetCpusetMems(). > > 2. when start a lxc with numatune memory mode not strict I think these two fixes would be better pushed separately. > # virsh -c lxc:/// start helloworld > error: Failed to start domain helloworld > error: internal error: guest failed to start: Unknown failure in libvirt_lxc > startup > > We shouldn't set anything in cpuset.mems for these mode. > > Signed-off-by: Luyao Huang > --- > src/lxc/lxc_cgroup.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > ACK, I've split the commit into two and pushed both. Jan signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/11] caps: various internal cleanups
On 18.04.2015 03:45, Cole Robinson wrote: > First two patches are straight bug fixes. > > The rest is a bunch of internal cleanup I felt compelled to do. Summary is > > - Switch caps and domain to use an enum for os.type value (hvm, xen, exe, ...) > - Switch caps to use VIR_DOMAIN_VIRT* internally > - Add a single function for looking up domain XML relevant values from > capabilities, and test the crap out of it. > - Use that function to simplfy and improve a lot of code. > > Thanks, > Cole > > Cole Robinson (11): > domain: conf: Better errors on bad os values > domain: conf: Don't validate VM ostype/arch at daemon startup > caps: Use an enum internally for ostype value > caps: Switch AddGuest to take VIR_DOMAIN_OSTYPE value > domain: Convert os.type to VIR_DOMAIN_OSTYPE enum > caps: Convert to use VIR_DOMAIN_VIRT internally > caps: Add virCapabilitiesDomainDataLookup > domain: conf: Do ostype/arch/machine parsing earlier > domain: conf: Use CapabilitiesDomainDataLookup for caps validation > caps: Use DomainDataLookup to replace GuestDefault* > domain: conf: Drop expectedVirtTypes > > src/bhyve/bhyve_capabilities.c | 6 +- > src/bhyve/bhyve_driver.c | 5 - > src/conf/capabilities.c| 307 -- > src/conf/capabilities.h| 50 ++--- > src/conf/domain_audit.c| 2 +- > src/conf/domain_conf.c | 331 > +++-- > src/conf/domain_conf.h | 25 ++- > src/conf/snapshot_conf.c | 18 +- > src/conf/snapshot_conf.h | 2 - > src/esx/esx_driver.c | 12 +- > src/hyperv/hyperv_driver.c | 4 +- > src/libvirt_private.syms | 6 +- > src/libxl/libxl_conf.c | 14 +- > src/libxl/libxl_domain.c | 7 +- > src/libxl/libxl_driver.c | 8 +- > src/libxl/libxl_migration.c| 2 - > src/lxc/lxc_conf.c | 8 +- > src/lxc/lxc_controller.c | 1 - > src/lxc/lxc_driver.c | 7 +- > src/lxc/lxc_native.c | 4 +- > src/openvz/openvz_conf.c | 7 +- > src/openvz/openvz_driver.c | 7 +- > src/parallels/parallels_driver.c | 23 +- > src/parallels/parallels_sdk.c | 6 +- > src/parallels/parallels_utils.h| 2 +- > src/phyp/phyp_driver.c | 10 +- > src/qemu/qemu_capabilities.c | 21 +- > src/qemu/qemu_command.c| 29 +-- > src/qemu/qemu_domain.c | 1 - > src/qemu/qemu_domain.h | 6 - > src/qemu/qemu_driver.c | 14 +- > src/qemu/qemu_migration.c | 5 +- > src/security/virt-aa-helper.c | 9 +- > src/test/test_driver.c | 12 +- > src/uml/uml_conf.c | 4 +- > src/uml/uml_driver.c | 6 +- > src/util/virerror.c| 5 +- > src/vbox/vbox_common.c | 23 +- > src/vmware/vmware_conf.c | 8 +- > src/vmware/vmware_driver.c | 4 +- > src/vmx/vmx.c | 3 +- > src/xen/xen_driver.c | 5 +- > src/xen/xen_hypervisor.c | 4 +- > src/xen/xend_internal.c| 6 +- > src/xenapi/xenapi_driver.c | 20 +- > src/xenapi/xenapi_utils.c | 4 +- > src/xenconfig/xen_common.c | 64 +++--- > src/xenconfig/xen_sxpr.c | 5 +- > src/xenconfig/xen_xl.c | 17 +- > src/xenconfig/xen_xm.c | 12 +- > tests/Makefile.am | 8 +- > tests/domainconftest.c | 3 +- > tests/domainsnapshotxml2xmltest.c | 1 - > tests/lxcxml2xmltest.c | 1 - > tests/openvzutilstest.c| 2 +- > tests/qemuagenttest.c | 1 - > tests/qemuhotplugtest.c| 1 - > tests/qemuxml2argvtest.c | 1 - > tests/qemuxml2xmltest.c| 3 +- > tests/qemuxmlnstest.c | 1 - > tests/securityselinuxlabeldata/chardev.xml | 2 +- > tests/securityselinuxlabeldata/disks.xml | 2 +- > tests/securityselinuxlabeldata/kernel.xml | 2 +- > tests/securityselinuxlabeldata/nfs.xml | 2 +- > tests/securityselinuxlabeltest.c | 4 +- > tests/testutils.c | 8 +- > tests/testutilslxc.c | 10 +- > tests/testutilsqemu.c | 38 ++-- > tests/testutilsxen.c | 18 +- > tests/vircapstest.c
Re: [libvirt] [PATCH 11/11] domain: conf: Drop expectedVirtTypes
On 18.04.2015 03:45, Cole Robinson wrote:
> This needs to specified in way too many places for a simple validation
> check. The ostype/arch/virttype validation checks later in
> DomainDefParseXML should catch most of the cases that this was covering.
> ---
> src/bhyve/bhyve_driver.c | 5 ---
> src/conf/domain_conf.c| 79
> +--
> src/conf/domain_conf.h| 6 ---
> src/conf/snapshot_conf.c | 14 ++-
> src/conf/snapshot_conf.h | 2 -
> src/esx/esx_driver.c | 4 +-
> src/libxl/libxl_domain.c | 1 -
> src/libxl/libxl_driver.c | 6 ---
> src/libxl/libxl_migration.c | 2 -
> src/lxc/lxc_controller.c | 1 -
> src/lxc/lxc_driver.c | 5 ---
> src/openvz/openvz_driver.c| 3 --
> src/parallels/parallels_driver.c | 1 -
> src/phyp/phyp_driver.c| 1 -
> src/qemu/qemu_domain.c| 1 -
> src/qemu/qemu_domain.h| 6 ---
> src/qemu/qemu_driver.c| 12 --
> src/qemu/qemu_migration.c | 5 +--
> src/test/test_driver.c| 7
> src/uml/uml_driver.c | 4 --
> src/vbox/vbox_common.c| 4 +-
> src/vmware/vmware_driver.c| 2 -
> src/xen/xen_driver.c | 3 --
> tests/domainconftest.c| 3 +-
> tests/domainsnapshotxml2xmltest.c | 1 -
> tests/lxcxml2xmltest.c| 1 -
> tests/qemuagenttest.c | 1 -
> tests/qemuhotplugtest.c | 1 -
> tests/qemuxml2argvtest.c | 1 -
> tests/qemuxml2xmltest.c | 3 +-
> tests/qemuxmlnstest.c | 1 -
> tests/securityselinuxlabeltest.c | 4 +-
> tests/xlconfigtest.c | 1 -
> tests/xmconfigtest.c | 1 -
> tests/xml2sexprtest.c | 1 -
> tests/xml2vmxtest.c | 1 -
> 36 files changed, 19 insertions(+), 175 deletions(-)
This is missing:
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 63b65b9..479b4c2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -36,7 +36,6 @@
#include "domain_conf.h"
#include "snapshot_conf.h"
#include "viralloc.h"
-#include "verify.h"
#include "virxml.h"
#include "viruuid.h"
#include "virbuffer.h"
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 37867cf..35423b5 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -745,7 +745,6 @@ get_definition(vahControl * ctl, const char *xmlStr)
ctl->def = virDomainDefParseString(xmlStr,
ctl->caps, ctl->xmlopt,
- -1,
VIR_DOMAIN_DEF_PARSE_INACTIVE);
if (ctl->def == NULL) {
vah_error(ctl, 0, _("could not parse XML"));
diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c
index 881f5b2..11f6e91 100644
--- a/src/xenapi/xenapi_driver.c
+++ b/src/xenapi/xenapi_driver.c
@@ -565,7 +565,6 @@ xenapiDomainCreateXML(virConnectPtr conn,
virDomainDefPtr defPtr = virDomainDefParseString(xmlDesc,
priv->caps, priv->xmlopt,
- 1 << VIR_DOMAIN_VIRT_XEN,
parse_flags);
if (!defPtr)
return NULL;
@@ -1740,7 +1739,6 @@ xenapiDomainDefineXMLFlags(virConnectPtr conn, const char
*xml, unsigned int fla
return NULL;
virDomainDefPtr defPtr = virDomainDefParseString(xml,
priv->caps, priv->xmlopt,
- 1 << VIR_DOMAIN_VIRT_XEN,
parse_flags);
if (!defPtr)
return NULL;
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/11] domain: conf: Better errors on bad os values
On 18.04.2015 03:45, Cole Robinson wrote:
> If no was specified:
> before: unknown OS type no OS type
> after : xml error: an os must be specified
>
> If an is specified that's not in our capabiliities data:
> before: unknown OS type: $type
> after : unsupported configuration: no support found for os '$type'
>
> VIR_ERR_OS_TYPE is now unused (as it should be frankly) so drop its strings
> as well to save our translators some effort.
> ---
> src/conf/domain_conf.c | 9 +
> src/util/virerror.c| 5 +
> 2 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ab4f2bf..8458f5b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -14638,8 +14638,8 @@ virDomainDefParseXML(xmlDocPtr xml,
> if (VIR_STRDUP(def->os.type, "xen") < 0)
> goto error;
> } else {
> -virReportError(VIR_ERR_OS_TYPE,
> - "%s", _("no OS type"));
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("an os must be specified"));
> goto error;
> }
> }
> @@ -14656,8 +14656,9 @@ virDomainDefParseXML(xmlDocPtr xml,
> }
>
> if (!virCapabilitiesSupportsGuestOSType(caps, def->os.type)) {
> -virReportError(VIR_ERR_OS_TYPE,
> - "%s", def->os.type);
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("no support found for os '%s'"),
> + def->os.type);
> goto error;
> }
>
> diff --git a/src/util/virerror.c b/src/util/virerror.c
> index 73dae95..aab36ae 100644
> --- a/src/util/virerror.c
> +++ b/src/util/virerror.c
> @@ -900,10 +900,7 @@ virErrorMsg(virErrorNumber error, const char *info)
> errmsg = _("failed Xen syscall %s");
> break;
> case VIR_ERR_OS_TYPE:
> -if (info == NULL)
> -errmsg = _("unknown OS type");
> -else
> -errmsg = _("unknown OS type %s");
> +errmsg = "%s";
Even though there are no other calls with VIR_ERR_OS_TYPE, I'd feel more
safe with:
if (info == NULL)
errmsg = _("invalid or missing OS type");
else
errmsg = "%s";
I find it more future proof.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/11] caps: Use an enum internally for ostype value
On 18.04.2015 03:45, Cole Robinson wrote: > But the internal API stays the same, and we just convert the value as > needed. Not useful yet, but this is the beginning step of using an enum > for ostype throughout the code. > --- > src/conf/capabilities.c | 74 > ++--- > src/conf/capabilities.h | 2 +- > src/conf/domain_conf.c | 8 ++ > src/conf/domain_conf.h | 13 + > src/xenconfig/xen_xl.c | 2 +- > 5 files changed, 81 insertions(+), 18 deletions(-) Missing src/libvirt_private.syms adjustment. You need to export virDomainOsTypeFromString and virDomainOsTypeToString. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/11] caps: Switch AddGuest to take VIR_DOMAIN_OSTYPE value
On 18.04.2015 03:45, Cole Robinson wrote:
> Rather than an opencoded string. This should be a no-op
> ---
> src/bhyve/bhyve_capabilities.c | 2 +-
> src/conf/capabilities.c | 11 ++-
> src/conf/capabilities.h | 2 +-
> src/esx/esx_driver.c | 4 ++--
> src/libxl/libxl_conf.c | 2 +-
> src/lxc/lxc_conf.c | 4 ++--
> src/openvz/openvz_conf.c | 2 +-
> src/parallels/parallels_driver.c | 6 +++---
> src/phyp/phyp_driver.c | 2 +-
> src/qemu/qemu_capabilities.c | 2 +-
> src/security/virt-aa-helper.c| 9 +++--
> src/test/test_driver.c | 3 ++-
> src/uml/uml_conf.c | 2 +-
> src/vbox/vbox_common.c | 2 +-
> src/vmware/vmware_conf.c | 4 ++--
> src/xen/xen_hypervisor.c | 2 +-
> src/xenapi/xenapi_driver.c | 4 ++--
> tests/testutils.c| 4 ++--
> tests/testutilslxc.c | 6 --
> tests/testutilsqemu.c| 18 +-
> tests/testutilsxen.c | 10 ++
> tests/vmx2xmltest.c | 4 ++--
> tests/xml2vmxtest.c | 4 ++--
> 23 files changed, 56 insertions(+), 53 deletions(-)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 5a070d7..5a742a5 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -707,7 +707,7 @@ caps_mockup(vahControl * ctl, const char *xmlStr)
> static int
> get_definition(vahControl * ctl, const char *xmlStr)
> {
> -int rc = -1;
> +int rc = -1, ostype;
> virCapsGuestPtr guest; /* this is freed when caps is freed */
>
> /*
> @@ -727,8 +727,13 @@ get_definition(vahControl * ctl, const char *xmlStr)
> goto exit;
> }
>
> +if ((ostype = virDomainOSTypeFromString(ctl->hvm)) < 0) {
> +vah_error(ctl, _("unknown OS type"));
This won't compile. You need to:
-vah_error(ctl, _("unknown OS type"));
+vah_error(ctl, 0, _("unknown OS type"));
> +goto exit;
> +}
> +
> if ((guest = virCapabilitiesAddGuest(ctl->caps,
> - ctl->hvm,
> + ostype,
> ctl->arch,
> NULL,
> NULL,
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 07/11] caps: Add virCapabilitiesDomainDataLookup
On 18.04.2015 03:45, Cole Robinson wrote:
> This is a helper function to look up all capabilities data for all
> the OS bits that are relevant to . This is
>
> - os type
> - arch
> - domain type
> - emulator
> - machine type
>
> This will be used to replace several functions in later commits.
> ---
> src/conf/capabilities.c | 163 +++-
> src/conf/capabilities.h | 18
> src/libvirt_private.syms | 1 +
> tests/Makefile.am| 8 +-
> tests/vircapstest.c | 209
> ++-
> 5 files changed, 393 insertions(+), 6 deletions(-)
>
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index 9f84766..8d3d2a3 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
> @@ -225,7 +225,6 @@ virCapabilitiesDispose(void *object)
> virCPUDefFree(caps->host.cpu);
> }
>
> -
> /**
> * virCapabilitiesAddHostFeature:
> * @caps: capabilities to extend
> @@ -568,6 +567,168 @@
> virCapabilitiesHostSecModelAddBaseLabel(virCapsHostSecModelPtr secmodel,
> return -1;
> }
>
> +static bool
> +virCapsDomainDataCompare(virCapsGuestPtr guest,
> + virCapsGuestDomainPtr domain,
> + virCapsGuestMachinePtr machine,
> + int ostype,
> + virArch arch,
> + int domaintype,
> + const char *emulator,
> + const char *machinetype)
> +{
> +char *check_emulator = NULL;
const char please to make it obvious we don't modify it.
> +
> +if (ostype != -1 && guest->ostype != ostype)
> +return false;
> +if ((arch != VIR_ARCH_NONE) && (guest->arch.id != arch))
> +return false;
> +
> +if (domaintype != -1 && (!domain || domain->type != domaintype))
> +return false;
> +
> +if (emulator) {
> +if (domain)
> +check_emulator = domain->info.emulator;
> +if (!check_emulator)
> +check_emulator = guest->arch.defaultInfo.emulator;
> +if (!check_emulator || STRNEQ(check_emulator, emulator))
or STRNEQ_NULLABLE()
> +return false;
> +}
> +
> +if (machinetype) {
> +if (!machine)
> +return false;
> +if (STRNEQ(machine->name, machinetype) &&
> +(!machine->canonical || STRNEQ(machine->canonical, machinetype)))
and here as well.
> +return false;
> +}
> +
> +return true;
> +}
> +
> /**
> * virCapabilitiesSupportsGuestArch:
> * @caps: capabilities to query
> diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
> index 7ee834f..87acf39 100644
> --- a/src/conf/capabilities.h
> +++ b/src/conf/capabilities.h
> @@ -192,6 +192,16 @@ struct _virCaps {
> virCapsGuestPtr *guests;
> };
>
> +typedef struct _virCapsDomainData virCapsDomainData;
> +typedef virCapsDomainData *virCapsDomainDataPtr;
> +struct _virCapsDomainData {
> +int ostype;
> +int arch;
> +int domaintype;
> +char *emulator;
> +char *machinetype;
const char to both please. You are copying over the pointer to live data
so we must discourage people to free any of the pair. I know that
'const' is not a sufficient protection, but it's at least something.
> +};
> +
>
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/11] domain: Convert os.type to VIR_DOMAIN_OSTYPE enum
On 18.04.2015 03:45, Cole Robinson wrote:
> ---
> src/conf/capabilities.c | 55 ---
> src/conf/capabilities.h | 10 ++--
> src/conf/domain_audit.c | 2 +-
> src/conf/domain_conf.c | 115
> +++
> src/conf/domain_conf.h | 2 +-
> src/hyperv/hyperv_driver.c | 4 +-
> src/libvirt_private.syms | 2 +
> src/libxl/libxl_conf.c | 10 ++--
> src/libxl/libxl_domain.c | 6 +-
> src/libxl/libxl_driver.c | 2 +-
> src/lxc/lxc_driver.c | 2 +-
> src/lxc/lxc_native.c | 4 +-
> src/openvz/openvz_conf.c | 3 +-
> src/openvz/openvz_driver.c | 4 +-
> src/parallels/parallels_driver.c | 8 +--
> src/parallels/parallels_sdk.c| 6 +-
> src/parallels/parallels_utils.h | 2 +-
> src/phyp/phyp_driver.c | 3 +-
> src/qemu/qemu_capabilities.c | 2 +-
> src/qemu/qemu_command.c | 10 ++--
> src/qemu/qemu_driver.c | 2 +-
> src/uml/uml_driver.c | 2 +-
> src/vbox/vbox_common.c | 15 ++---
> src/vmware/vmware_driver.c | 2 +-
> src/vmx/vmx.c| 3 +-
> src/xen/xen_driver.c | 2 +-
> src/xen/xend_internal.c | 6 +-
> src/xenapi/xenapi_driver.c | 12 +---
> src/xenapi/xenapi_utils.c| 4 +-
> src/xenconfig/xen_common.c | 29 +-
> src/xenconfig/xen_sxpr.c | 5 +-
> src/xenconfig/xen_xl.c | 15 ++---
> src/xenconfig/xen_xm.c | 12 ++--
> tests/openvzutilstest.c | 2 +-
> 34 files changed, 152 insertions(+), 211 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 8c37303..a587597 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -400,6 +400,8 @@ virDomainObjSetDefTransient;
> virDomainObjSetMetadata;
> virDomainObjSetState;
> virDomainObjTaint;
> +virDomainOSTypeFromString;
> +virDomainOSTypeToString;
> virDomainParseMemory;
> virDomainPausedReasonTypeFromString;
> virDomainPausedReasonTypeToString;
See? This should have been in one of the previous patches.
> diff --git a/src/parallels/parallels_driver.c
> b/src/parallels/parallels_driver.c
> index d80fe24..f7a75e0 100644
> --- a/src/parallels/parallels_driver.c
> +++ b/src/parallels/parallels_driver.c
> @@ -184,7 +184,7 @@ parallelsDomainDeviceDefPostParse(virDomainDeviceDefPtr
> dev,
> (dev->data.net->type == VIR_DOMAIN_NET_TYPE_NETWORK ||
> dev->data.net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) &&
> !dev->data.net->model &&
> -STREQ(def->os.type, "hvm") &&
> +def->os.type == VIR_DOMAIN_OSTYPE_HVM &&
> VIR_STRDUP(dev->data.net->model, "e1000") < 0)
> goto cleanup;
>
> @@ -575,7 +575,7 @@ parallelsDomainGetOSType(virDomainPtr domain)
> goto cleanup;
> }
>
> -ignore_value(VIR_STRDUP(ret, privdom->def->os.type));
> +ignore_value(VIR_STRDUP(ret,
> virDomainOSTypeToString(privdom->def->os.type)));
>
> cleanup:
> if (privdom)
This part is missing:
diff --git a/src/parallels/parallels_driver.c
b/src/parallels/parallels_driver.c
index f7a75e0..a654861 100644
--- a/src/parallels/parallels_driver.c
+++ b/src/parallels/parallels_driver.c
@@ -720,7 +720,8 @@ parallelsDomainDefineXMLFlags(virConnectPtr conn,
const char *xml, unsigned int
goto cleanup;
} else {
virReportError(VIR_ERR_INVALID_ARG,
- _("Unsupported OS type: %s"), def->os.type);
+ _("Unsupported OS type: %s"),
+ virDomainOSTypeToString(def->os.type));
goto cleanup;
}
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/7] gluster: Check for validity of pool source hostname
On Sun, Apr 19, 2015 at 20:49:09 -0400, John Ferlan wrote: > Prior to attempting to open the gluster connection, let's make sure we > can resolve the source pool hostname. > > Signed-off-by: John Ferlan > --- > src/storage/storage_backend_gluster.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/storage/storage_backend_gluster.c > b/src/storage/storage_backend_gluster.c > index d2e79bc..e3a8c21 100644 > --- a/src/storage/storage_backend_gluster.c > +++ b/src/storage/storage_backend_gluster.c > @@ -96,6 +96,9 @@ virStorageBackendGlusterOpen(virStoragePoolObjPtr pool) > trailing_slash = false; > } > > +if (!virIsValidHostname(pool->def->source.hosts[0].name)) > +return NULL; > + Okay, it might be marginally worth at this point, since gluster is/was leaking some memory and threads after it was initialized. On the other hand you'll be resolving the hostname again a few lines below. > if (VIR_ALLOC(ret) < 0) > return NULL; 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 2/7] iscsi: Check for validity of pool source hostname
On Sun, Apr 19, 2015 at 20:49:07 -0400, John Ferlan wrote:
> Ensure that the pool that's being started has a source pool hostname
> that can be resolved before trying to start an iSCSI session.
>
> Signed-off-by: John Ferlan
> ---
> src/storage/storage_backend_iscsi.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/src/storage/storage_backend_iscsi.c
> b/src/storage/storage_backend_iscsi.c
> index 197d333..958c347 100644
> --- a/src/storage/storage_backend_iscsi.c
> +++ b/src/storage/storage_backend_iscsi.c
> @@ -385,6 +385,13 @@ virStorageBackendISCSIStartPool(virConnectPtr conn,
> return -1;
> }
>
> +if (!virIsValidHostname(pool->def->source.hosts[0].name)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("cannot resolve hostname '%s' on this host"),
> + pool->def->source.hosts[0].name);
> +return -1;
This overwrites the error from virIsValidHostname()
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 1/7] virutil: Introduce virIsValidHostname
On Sun, Apr 19, 2015 at 20:49:06 -0400, John Ferlan wrote:
> Similar to virGetHostname, but this time taking a parameter which is a
> hostname or ipaddress from a ... />
> XML property and validating that the name can be resolved.
>
> Return true or false depending on whether we can ascertain the hostname
> address from calls to 'getnameinfo' and 'getaddrinfo'. Subsequent patches
> will be validating a proposed pool hostname definition against existing
> pool hostnames to ensure they are not the same hostname (and thus having
> two pools looking at the same data)
>
> Signed-off-by: John Ferlan
> ---
> src/libvirt_private.syms | 1 +
> src/util/virutil.c | 49
>
> src/util/virutil.h | 1 +
> 3 files changed, 51 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 8c37303..5ba9635 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2324,6 +2324,7 @@ virIsCapableFCHost;
> virIsCapableVport;
> virIsDevMapperDevice;
> virIsSUID;
> +virIsValidHostname;
> virManageVport;
> virMemoryLimitIsSet;
> virMemoryLimitTruncate;
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 79cdb7a..f6cc9af 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -690,6 +690,55 @@ char *virGetHostname(void)
> }
>
>
> +/*
> + * virIsValidHostname:
> + *
> + * Unlike virGetHostname, this variant of the code receives a hostname
> + * retrieves the getaddrinfo. If the passed hostname can be successfully
> + * and if successfully resolved via getaddrinfo, then success is declared.
> + *
> + * On error in lookup, if an IP Address is not found, or error formatting
> + * the name, an error is generated and a NULL is returned.
> + *
> + * On success, a pointer to a character representation of the numeric IP
> + * Address is returned.
> + *
> + * It is the caller's responsibility to check for a non NULL return and
> + * VIR_FREE the resultant string.
This comment does not describe how this function works.
> + */
> +bool
> +virIsValidHostname(const char *hostname)
> +{
> +int r;
> +struct addrinfo hints, *info;
> +
> +if (STRPREFIX(hostname, "localhost") ||
This certainly is not a good idea:
$ host localhost.pipo.sk
localhost.pipo.sk has address 46.255.230.242
> +STREQ(hostname, "127.0.0.1") || STREQ(hostname, "::1"))
> +return true;
> +
> +memset(&hints, 0, sizeof(hints));
> +hints.ai_family = AF_UNSPEC;
> +hints.ai_protocol = IPPROTO_TCP;
> +
> +if ((r = getaddrinfo(hostname, NULL, &hints, &info)) != 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("IP address lookup for host '%s' failed: %s"),
> + hostname, gai_strerror(r));
> +return false;
> +}
> +
> +if (!info) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("No IP address for host '%s' found"),
> + hostname);
> +return false;
> +}
> +freeaddrinfo(info);
> +
> +return true;
> +}
> +
> +
Ghm, this seems a bit too excesive IMO, let see how it's used.
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 3/5] qemu: add QEMU_CAPS_MACHINE_VMPORT_OPT
Hi
On Tue, Apr 14, 2015 at 6:07 PM, Eric Blake wrote:
> > +{ "machine", "vmport", QEMU_CAPS_MACHINE_VMPORT_OPT },
>
> Ouch. qemu commit 0a7cf21 fixes what would have been a regression in
> 2.3 at exposing "mem-merge" through query-command-line-options, but it
> does NOT expose "vmport", which is a per-architecture option rather than
> a generic -machine option. Which means that even though qemu 2.2
> (perhaps wrongly) advertised "vmport" for all machines (even when it was
> not supported), 2.3 will not advertise it, and we are hoping for a
> better solution in 2.4 for properly advertising vmport on an
> as-appropriate basis.
>
Thanks Eric for finding out this regression.
Is anybody working on a better solution? I can imagine either a new qmp
machine-list-properties, or perhaps more reasonably a new properties array
in query-machines reply.
--
Marc-André Lureau
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Building libvirt under Windows
Hello! > I haven't done much testing of the resulting project, but > the ./autobuild.sh script is able to cross-build for mingw when run on > a Fedora system, if you have enough of the mingw-* packages installed > on Fedora. This includes mingw64-portablexdr (I have no idea if the > mingw project will be shifting to a different xdr package any time > soon). So, we need portablexdr on Windows. Can we convince the author to add the necessary fixes, or take over the maintainership ? Below is a snip from his original reply to me: --- cut --- I'm unlikely to fix any bugs in portableXDR now. The original reason for having it was because XDR was under a non-free license, but that has been resolved, so you can use XDR code on Windows. Anyway, you might want to send your observations to libvir-list@redhat.com since that's where the libvirt developers hang out. --- cut --- I also have here the necessary patches which allow me to build libvirt natively under MinGW using portablexdr and its rpcgen. But, first we need fixed portablexdr version. Its rpcgen is currently broken twice (i found the second bug after sending that email). Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia > -Original Message- > From: Eric Blake [mailto:ebl...@redhat.com] > Sent: Thursday, April 16, 2015 3:43 PM > To: Pavel Fedin; libvir-list@redhat.com > Subject: Re: [libvirt] Building libvirt under Windows > > On 04/16/2015 03:52 AM, Pavel Fedin wrote: > > Hello! > > > > I am currently building libvirt under Windows64, and i would like to > > ask for correct way to do it. > > The first thing to resolve is absence of RPC XDR library. From > > configure source i figured out that portablexdr library may help. I > > have downloaded it, but it comes with a different RPC code generator > named portable-rpcgen. > > I managed to use it, but i had to patch both libvirt (in order to > > recognize this generator and handle it properly) and portable-rpcgen > > (in order to correctly work under Windows, as well as get rid of some > crashes). > > Initially i wanted to upstream portablexdr fixes and emailed the > author. > > But the author replied that he is not going to fix portablexdr > because > > it's now deprecated, since original XDR code comes under free license > > now. So, i have all necessary fixes in my own fork. He suggested me > to > > come here to ask about these things. > > So, guys, how do you do it ? I cannot find any rpcgen package usable > > under Windows (MinGW, not Cygwin). > > > -- > 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
Re: [libvirt] [PATCH] parallels: implement .domainGetMaxMemory
19.03.2015 18:39, Dmitry Guryanov пишет:
Since we haven't implemented balloon parameters tuning
we can just return amount of memory in this function.
Signed-off-by: Dmitry Guryanov
---
src/parallels/parallels_driver.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c
index 391e927..d13a4e2 100644
--- a/src/parallels/parallels_driver.c
+++ b/src/parallels/parallels_driver.c
@@ -1089,6 +1089,24 @@ parallelsDomainManagedSaveRemove(virDomainPtr domain,
unsigned int flags)
return ret;
}
+static unsigned long long
+parallelsDomainGetMaxMemory(virDomainPtr domain)
+{
+parallelsConnPtr privconn = domain->conn->privateData;
+virDomainObjPtr dom = NULL;
+int ret = -1;
+
+dom = virDomainObjListFindByUUID(privconn->domains, domain->uuid);
+if (dom == NULL) {
+parallelsDomNotFoundError(domain);
+return -1;
+}
+
+ret = dom->def->mem.max_balloon;
+virObjectUnlock(dom);
+return ret;
+}
+
static virHypervisorDriver parallelsDriver = {
.name = "Parallels",
.connectOpen = parallelsConnectOpen,/* 0.10.0 */
@@ -1133,6 +1151,7 @@ static virHypervisorDriver parallelsDriver = {
.domainHasManagedSaveImage = parallelsDomainHasManagedSaveImage, /*
1.2.13 */
.domainManagedSave = parallelsDomainManagedSave, /* 1.2.14 */
.domainManagedSaveRemove = parallelsDomainManagedSaveRemove, /* 1.2.14 */
+.domainGetMaxMemory = parallelsDomainGetMaxMemory, /* 1.2.14 */
};
static virConnectDriver parallelsConnectDriver = {
ACK.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Xen-devel] [PATCH] libxl: initialize vfb defbools in libxlMakeVfb
On Mon, 2015-04-20 at 12:32 +0200, Olaf Hering wrote: > On Mon, Apr 20, Ian Campbell wrote: > > > It makes no sense to do that at init time, the whole purpose of a > > defbool is to allow the calling application to choose a value or to > > explicitly leave it as a request to for the default (which might vary > > depending on other selections). > > Yes, and thats why my patch does? This is a libvirt patch. It might be correct in its own right, if libvirt has different ideas about the defaults to libxl, but it is also masking an underlying libxl bug. Ian. > > libxl_device_vfb_init(x_vfb); > +libxl_defbool_set(&x_vfb->sdl.opengl, 0); > if (libxl_defbool_val(vfb.sdl.opengl)) >; > > Olaf -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-php] memory overflow when using libvirt_list_networks
On 18.04.2015 16:34, Kash Pande wrote: > On 18/04/15 04:25 AM, Michal Novotny wrote: >> Hi Kash, >> >> there were no patches for libvirt-php for a long time. Are you sure >> this is libvirt-php related and not a bug in libvirt itself? >> >> Thanks, >> Michal >> > > I've just started using libvirt-php this week but I've been using > libvirt for ages. > > I did learn last night, the difference between my two systems that is > relevant is that the overflow system ran Gentoo while the "working" > system ran Debian. I was missing a USE flag for "virt-network" on the > Gentoo machine. It is a development environment that I am attempting to > create a supported configuration with, so these kinds of missing USE > flags and other things are just facts of life I must accept. > > Unfortunately, it was kind-of difficult to get this information or even > workaround it without rebuilding libvirt on the Gentoo machine, which > means I won't be able to give a proper error message to users who > encounter this problem as well. > > It's a problem in libvirt-php to overflow when requesting > libvirt_networks_list on a machine without networking support compiled > in. The PHP bindings should error out properly, not memory overflow and > put errors in httpd log with cryptic results in the interpreter :-) > > > I do like the PHP bindings but they appear to be lacking attention. Am I > the only one using it? ;-) I guess you are not, but truth to be told it's been a while since I saw a libvirt-php patch. Mind posting one and break the silence? Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] missing php binding to storage volume download , resize and upload
Can somebody add missed functions to libvirt php api binding to download,resize and upload volume? -- Vasiliy Tolstov, e-mail: v.tols...@selfip.ru jabber: v...@selfip.ru -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Xen-devel] [PATCH] libxl: initialize vfb defbools in libxlMakeVfb
On Mon, Apr 20, Ian Campbell wrote: > It makes no sense to do that at init time, the whole purpose of a > defbool is to allow the calling application to choose a value or to > explicitly leave it as a request to for the default (which might vary > depending on other selections). Yes, and thats why my patch does? libxl_device_vfb_init(x_vfb); +libxl_defbool_set(&x_vfb->sdl.opengl, 0); if (libxl_defbool_val(vfb.sdl.opengl)) ; Olaf -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Xen-devel] [PATCH] libxl: initialize vfb defbools in libxlMakeVfb
On Mon, 2015-04-20 at 12:20 +0200, Olaf Hering wrote: > On Mon, Apr 20, Ian Campbell wrote: > > > If what you said were true then an assert would be a rather harsh > > overreaction to an application coding error. > > Currently both libxl and libvirt are coded that way. Since the sdl code > path in libvirt was never executed the crash in libxlMakeVfbList was not > noticed. > > So you are saying that an external user of libxl_device_vfb_init has to > call yet another function to initialize all defbool to give them either > true of false? Or should libxl_device_vfb_init call > libxl__device_vfb_setdefault directly? > Neither. Any code within libxl which consumes a libxl_device_vfb must, at some point before touching the defbools therein, have arranged to call the appropriate setdefaults function. It makes no sense to do that at init time, the whole purpose of a defbool is to allow the calling application to choose a value or to explicitly leave it as a request to for the default (which might vary depending on other selections). It is up to *libxl* to convert such requests for default behaviour into a specific value before trying to use it, that is the purpose of the internal setdefaults functions and for the assert when reading a defbool. Ian. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Xen-devel] [PATCH] libxl: initialize vfb defbools in libxlMakeVfb
On Mon, Apr 20, Ian Campbell wrote: > If what you said were true then an assert would be a rather harsh > overreaction to an application coding error. Currently both libxl and libvirt are coded that way. Since the sdl code path in libvirt was never executed the crash in libxlMakeVfbList was not noticed. So you are saying that an external user of libxl_device_vfb_init has to call yet another function to initialize all defbool to give them either true of false? Or should libxl_device_vfb_init call libxl__device_vfb_setdefault directly? Olaf -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] parallels: use SDK disks system names for paths
For block devices SDK friendly name do not refer to block device path but rather some description while system name refer to device path. For ploop devices both names refer to image path. Thus system name is better choice. Signed-off-by: Nikolay Shirokovskiy --- src/parallels/parallels_sdk.c |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index 72c1773..e1ba7cf 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -485,13 +485,13 @@ prlsdkGetDiskInfo(PRL_HANDLE prldisk, disk->device = VIR_DOMAIN_DISK_DEVICE_DISK; } -pret = PrlVmDev_GetFriendlyName(prldisk, NULL, &buflen); +pret = PrlVmDev_GetSysName(prldisk, NULL, &buflen); prlsdkCheckRetGoto(pret, cleanup); if (VIR_ALLOC_N(buf, buflen) < 0) goto cleanup; -pret = PrlVmDev_GetFriendlyName(prldisk, buf, &buflen); +pret = PrlVmDev_GetSysName(prldisk, buf, &buflen); prlsdkCheckRetGoto(pret, cleanup); if (virDomainDiskSetSource(disk, buf) < 0) -- 1.7.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] parallels: use SDK disks system names for paths
For block devices SDK friendly name do not refer to block device path but rather some description while system name refer to device path. For ploop devices both names refer to image path. Thus system name is better choice. Signed-off-by: Nikolay Shirokovskiy --- src/parallels/parallels_sdk.c |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index 72c1773..e1ba7cf 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -485,13 +485,13 @@ prlsdkGetDiskInfo(PRL_HANDLE prldisk, disk->device = VIR_DOMAIN_DISK_DEVICE_DISK; } -pret = PrlVmDev_GetFriendlyName(prldisk, NULL, &buflen); +pret = PrlVmDev_GetSysName(prldisk, NULL, &buflen); prlsdkCheckRetGoto(pret, cleanup); if (VIR_ALLOC_N(buf, buflen) < 0) goto cleanup; -pret = PrlVmDev_GetFriendlyName(prldisk, buf, &buflen); +pret = PrlVmDev_GetSysName(prldisk, buf, &buflen); prlsdkCheckRetGoto(pret, cleanup); if (virDomainDiskSetSource(disk, buf) < 0) -- 1.7.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Xen-devel] [PATCH] libxl: initialize vfb defbools in libxlMakeVfb
On Mon, 2015-04-20 at 11:32 +0200, Olaf Hering wrote: > On Mon, Apr 20, Ian Campbell wrote: > > > On Fri, 2015-04-17 at 13:40 -0600, Jim Fehlig wrote: > > > On 04/17/2015 11:19 AM, Olaf Hering wrote: > > > > If the domU configu has sdl enabled libvirtd crashes: > > > > libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion > > > > `!libxl_defbool_is_default(db)' failed. > > > > > > The assertion seems harsh considering the offense... > > > > libxl should be setting a default for this value if the application > > hasn't, by calling libxl__device_vfb_setdefault at some correct point. > > This is an internal function. Its up to the caller of > libxl_device_vfb_init to initialize the defbool members. No, it isn't. It's up to libxl to call libxl__device_vfb_setdefault somewhere on the code path between entering the library and actually using this defbool. (Possibly indirectly via the containing struct in this case) We normally do this as close to entry into the library as we can, e.g. in the libxl_device_FOO_add or e.g. in libxl_domain_create. It is always a libxl bug if a defbool gets used without having had a defaulted value applied by the library first, hence the assert. If what you said were true then an assert would be a rather harsh overreaction to an application coding error. Ian. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] domtop: Properly free cpu status
So, in the example the cpu stats are collected within a function called do_top. At the beginning of the function we ask the daemon for how much vCPUs can we get stats, and how many stats for a vCPU can we get. This is because it's how our API works - users are required to preallocate a chunk of memory for the results. Now, at the end, we try to free the allocated array, but we are not doing it correctly. There's this virTypedParamsFree() function which gets a pointer to the array and the length of the array. However, if there was an error in getting vCPU stats we pass a negative number instead of the originally computed value. This flaw results in SIGSEGV: libvirt: QEMU Driver error : Requested operation is not valid: domain is not running ERROR do_top:333 : Unable to get cpu stats ==29201== Invalid read of size 4 ==29201==at 0x4F1DF8B: virTypedParamsClear (virtypedparam.c:1145) ==29201==by 0x4F1DFEB: virTypedParamsFree (virtypedparam.c:1165) ==29201==by 0x4023C3: do_top (domtop.c:349) ==29201==by 0x40260B: main (domtop.c:386) ==29201== Address 0x131cd7c0 is 16 bytes after a block of size 768 alloc'd ==29201==at 0x4C2C070: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==29201==by 0x401FF1: do_top (domtop.c:295) ==29201==by 0x40260B: main (domtop.c:386) Signed-off-by: Michal Privoznik --- examples/domtop/domtop.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/domtop/domtop.c b/examples/domtop/domtop.c index e50988e..2283994 100644 --- a/examples/domtop/domtop.c +++ b/examples/domtop/domtop.c @@ -346,8 +346,8 @@ do_top(virConnectPtr conn, ret = 0; cleanup: -virTypedParamsFree(now_params, now_nparams * max_id); -virTypedParamsFree(then_params, then_nparams * max_id); +virTypedParamsFree(now_params, nparams * max_id); +virTypedParamsFree(then_params, nparams * max_id); if (dom) virDomainFree(dom); return ret; -- 2.0.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Xen-devel] [PATCH] libxl: initialize vfb defbools in libxlMakeVfb
On Mon, Apr 20, Ian Campbell wrote: > On Fri, 2015-04-17 at 13:40 -0600, Jim Fehlig wrote: > > On 04/17/2015 11:19 AM, Olaf Hering wrote: > > > If the domU configu has sdl enabled libvirtd crashes: > > > libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion > > > `!libxl_defbool_is_default(db)' failed. > > > > The assertion seems harsh considering the offense... > > libxl should be setting a default for this value if the application > hasn't, by calling libxl__device_vfb_setdefault at some correct point. This is an internal function. Its up to the caller of libxl_device_vfb_init to initialize the defbool members. Olaf -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 0/9] qemu: Add quorum support to libvirt
On Thu, Apr 16, 2015 at 3:43 PM, John Ferlan wrote: > > > On 03/17/2015 03:25 PM, Matthias Gatto wrote: >> The purpose of these patches is to introduce quorum for libvirt >> I've try to follow this proposal: >> http://www.redhat.com/archives/libvir-list/2014-May/msg00533.html >> > > > Before we reach a year waiting on this and so we make some progress... > > I started looking at the series and naturally found conflicts with the > patches. Not to be deterred I started with the first 5 patches since I > figured they were mostly setup and not functional change. > > Of those patch 2 had the most conflict. Once resolved, patches 3-5 > applied without issue... Patch 6, more conflicts, so I just focused on > 1-5... > > Patch 2 had numerous conflicts in virStorageBackendCreateQemuImgCmd due > to commit id fbcf7da95b872ac45cabc4356fc9c06e809d0237. > > After applying patch 5, I cscope searched on "->backingStore" in order > to find any 'new' references that might have crept in (ignoring any > backingStoreRaw references) and found: > > f9ea3d60119e82c02c00fbf3678c3ed20634dea1 (qemu_monitor_json.c) > > which I adjusted patch 2 in order to keep it in line. > > Beyond that in patch 2: > > * In storage_conf.c/*VolDefFormat() - I changed the "&(def->target)" > to just "&def->target" which was mostly a consistency thing. I found a > couple of others as well and adjusted them. I don't think it matters, > since none of the references were "&(x->y)->z" > > * In storage_backend_fs.c/virStorageBackendProbeTarget() - you created > a *local* backingStore reference and then used that for an assignment, > but didn't update target->backingStore in at least the first > case/instance, so since we're not updating the setting in this pass, I > adjusted the code as follows: > > -if (!(target->backingStore = virStorageSourceNewFromBacking(meta))) > +backingStore = virStorageSourceGetBackingStore(target, 0); > +if (!(backingStore = virStorageSourceNewFromBacking(meta))) > > back to setting target->backingStore in the if statement, then fetching > after the if statement into the local variable: > > -backingStore = virStorageSourceGetBackingStore(target, 0); > -if (!(backingStore = virStorageSourceNewFromBacking(meta))) > +if (!(target->backingStore = virStorageSourceNewFromBacking(meta))) > goto cleanup; > > +backingStore = virStorageSourceGetBackingStore(target, 0); > > > I believe the virStorageSourceFree(backingStore) that follows will be OK > since you'd be replacing it anyway in the if statement. > > I'll post my changes as a reply to patch 2 shortly. > > Of course changing the storage_backend_fs.c in patch 2 caused a ripple > effect (like I expected) in patch 4, but no changes were necessary since > patch 4 did the set and I had added the fetch after the set as part of > my patch 2 adjustment. > > In patch 3, you have a 'bool' function returning a pointer value (which > can either be "something" or NULL. I'm changing that to "return > !!src->backingStore;", which is what I believe you're really trying to > return here. There's a couple places where it's checked, but luckily so > far nothing happens with those checks and from what I read... > > * virDomainDiskBackingStoreParse -> Code has already dereferenced > backingStore so we know it won't return false > > * virStorageBackendProbeTarget -> virStorageSourceNewFromBacking > fetch/failure would be the same > > * virStorageBackendLogicalMakeVol -> If backingStore was NULL we would > have failed earlier > > * virStorageSourceCopy -> virStorageSourceCopy fetch/failure would be > the same > > As long as you think that looks good - I can push patches 1-4. > > While Patch 5 did apply cleanly, there are some issues with it. In patch > 3 it's returning true/false and failures in virStorageBackendProbeTarget > and virStorageSourceCopy would return some message as to why it failed > (most likely memory allocation failures) which then propagate back to > the caller to get a "reason" for the failure. With your change to patch > 5, you're returning true/false only without always setting the "reason" > (eg virReportError). That reason needs to be set so failure reason can > be propagated back to the caller instead of the infamous "failed for > some reason". > > Additionally in all the places that don't check the return, you should > add a ignore_value() around them to signify the code doesn't care (or > maybe check those places to see if that "don't care" assumption is true). > > Once 5 is reworked, you'll have to rework patches 6-9 and repost since > that's where the real/new functionality starts. > > Tks, > > John > >> This feature ask for 6 task: >> >> 1) Allow a _virStorageSource to contain more than one backing store. >> >> Because all the actual libvirt code use the backingStore field >> as a pointer and we needs want to change that, I've decide to encapsulate >> the backingStore field to simplifie the array manip
Re: [libvirt] [PATCH] parallels: use SDK disks system names for paths
20.04.2015 9:26, Nikolay Shirokovskiy пишет: For block devices SDK friendly name do not refer to block device path but rather some description while system name refer to device path. For ploop devices both names refer to image path. Thus system name is better choice. Signed-off-by: Nikolay Shirokovskiy --- src/parallels/parallels_sdk.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index 80e9d00..4911ab7 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -491,7 +491,7 @@ prlsdkGetDiskInfo(PRL_HANDLE prldisk, if (VIR_ALLOC_N(buf, buflen) < 0) goto cleanup; -pret = PrlVmDev_GetFriendlyName(prldisk, buf, &buflen); +pret = PrlVmDev_GetSysName(prldisk, buf, &buflen); The only problem with this is that buflen is calculated by PrlVmDev_GetFriendlyName still a few lines above. prlsdkCheckRetGoto(pret, cleanup); if (virDomainDiskSetSource(disk, buf) < 0) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Xen-devel] [PATCH] libxl: initialize vfb defbools in libxlMakeVfb
On Fri, 2015-04-17 at 13:40 -0600, Jim Fehlig wrote: > On 04/17/2015 11:19 AM, Olaf Hering wrote: > > If the domU configu has sdl enabled libvirtd crashes: > > libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion > > `!libxl_defbool_is_default(db)' failed. > > The assertion seems harsh considering the offense... libxl should be setting a default for this value if the application hasn't, by calling libxl__device_vfb_setdefault at some correct point. Not to say the application shouldn't also set it if it has a different default preference, but there is a libxl bug here somewhere too. Ian. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] rng: Move sgio attr definition to a separate block
it might be worth having sgio attribute defined in a separate block the same way as rawio attribute. --- As Jan suggested, I moved unrelated hunks into a separate commit and then pushed it as trivial. Erik docs/schemas/domaincommon.rng | 22 ++ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 9cc5e76..19461f5 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1193,12 +1193,7 @@ - - -filtered -unfiltered - - + @@ -3680,12 +3675,7 @@ scsi - - - filtered - unfiltered - - + @@ -5190,4 +5180,12 @@ + + + +filtered +unfiltered + + + -- 1.9.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] rng: Forbid to validate mismatched 'device' and 'type' attributes
On 04/17/2015 03:10 PM, Ján Tomko wrote: > On Fri, Apr 17, 2015 at 01:31:09PM +0200, Erik Skultety wrote: >> According to docs, using 'lun' as a value for device attribute is only valid >> with disk types 'block' and 'network'. However current RNG schema also allows >> a combination type='file' device='lun' which results in a successfull >> xml validation, but fails at qemuBuildCommandLine. >> Besides fixing the RNG schema, this patch also adds a qemuxml2argvtest >> for this case. >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1210669 >> --- >> docs/schemas/domaincommon.rng | 42 >> ++ >> .../qemuxml2argv-disk-device-lun-type-invalid.xml | 28 +++ >> tests/qemuxml2argvtest.c | 2 ++ >> 3 files changed, 58 insertions(+), 14 deletions(-) >> create mode 100644 >> tests/qemuxml2argvdata/qemuxml2argv-disk-device-lun-type-invalid.xml >> > > ACK if you split out the sgIO reference. > >> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng >> index 03fd541..ee32b73 100644 >> --- a/docs/schemas/domaincommon.rng >> +++ b/docs/schemas/domaincommon.rng >> @@ -1316,37 +1316,43 @@ > ... > >> - >> - >> -filtered >> -unfiltered >> - >> - >> + > > This hunk... > >> >> + >> + >> + >> + >> + >> + >> + >> >> >> >> >> >> - >> - >> - >> - >> - >> >> >> >> + >> + >> + >> + >> + >> + >> + >> >> >> >> @@ -5315,4 +5321,12 @@ >> >> >> > >> + >> + >> + >> +filtered >> +unfiltered >> + >> + >> + > > ... and this hunk are not required to fix the bug and should be pushed > separately. > >> > > > > Jan > > > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > Thank you for review, I moved the hunks into a separate commit and pushed both. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] Apparmor qemu abstraction fixes for SLES
Hi Jamie,
On Thu, 2015-04-09 at 20:29 -0500, Jamie Strandboge wrote:
> On 04/09/2015 04:25 AM, Cédric Bosdonnat wrote:
> > SLES 11 has legacy qemu-kvm package, /usr/bin/qemu-kvm and
> > /usr/share/qemu-kvm need to be accessed by domains.
> > ---
> > examples/apparmor/libvirt-qemu | 9 +
> > 1 file changed, 9 insertions(+)
> >
>
> It is ok as is, but see my comments below.
>
> Acked-By: Jamie Strandboge
>
> > diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
> > index 7aad391..a3043dd 100644
> > --- a/examples/apparmor/libvirt-qemu
> > +++ b/examples/apparmor/libvirt-qemu
>
> ...
>
> > @@ -118,12 +120,19 @@
> >/bin/dd rmix,
> >/bin/cat rmix,
> >
> > + # for restore
> > + /bin/bash rmix,
> > +
>
> This one is curious. You have it with rmix, so it's ok though.
I didn't investigate too deeply to know why we need it. Maybe that would
be a good thing for me to do ;)
> Acked-By: Jamie Strandboge
>
> ># for usb access
> >/dev/bus/usb/ r,
> >/etc/udev/udev.conf r,
> >/sys/bus/ r,
> >/sys/class/ r,
> >
> > + # nscd pieces
> > + /run/nscd/group r,
> > + /run/nscd/passwd r,
> > +
>
> These should already be in the nameservice abstraction via this rule:
> /{var/db,var/cache,var/run,run}/nscd/{passwd,group,services,host}r,
>
> which is already included by libvirt-qemu:
> #include
>
> It's ok to have duplicates-- apparmor handles them, but perhaps these aren't
> actually needed?
Ouch, indeed... this rule seems more recent than what we have in SLES,
I'll remove those lines from the profile.
Thanks for the heads up.
--
Cedric
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
