- Devel - libvirt List Archives

[libvirt] [PULL 09/21] qmp: Add runnability information to query-cpu-definitions
by Eduardo Habkost 17 Oct '16

17 Oct '16

Add a new optional field to query-cpu-definitions schema: "unavailable-features". It will contain a list of QOM properties that prevent the CPU model from running in the current host. Cc: David Hildenbrand <dahi(a)linux.vnet.ibm.com> Cc: Michael Mueller <mimu(a)linux.vnet.ibm.com> Cc: Christian Borntraeger <borntraeger(a)de.ibm.com> Cc: Cornelia Huck <cornelia.huck(a)de.ibm.com> Cc: Jiri Denemark <jdenemar(a)redhat.com> Cc: libvir-list(a)redhat.com Reviewed-by: Eric Blake <eblake(a)redhat.com> Signed-off-by: Eduardo Habkost <ehabkost(a)redhat.com> --- qapi-schema.json | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/qapi-schema.json b/qapi-schema.json index ded1179..5a8ec38 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3101,10 +3101,31 @@ # QEMU version, machine type, machine options and accelerator options. # A static model is always migration-safe. (since 2.8) # +# @unavailable-features: #optional List of properties that prevent +# the CPU model from running in the current +# host. (since 2.8) +# +# @unavailable-features is a list of QOM property names that +# represent CPU model attributes that prevent the CPU from running. +# If the QOM property is read-only, that means there's no known +# way to make the CPU model run in the current host. Implementations +# that choose not to provide specific information return the +# property name "type". +# If the property is read-write, it means that it MAY be possible +# to run the CPU model in the current host if that property is +# changed. Management software can use it as hints to suggest or +# choose an alternative for the user, or just to generate meaningful +# error messages explaining why the CPU model can't be used. +# If @unavailable-features is an empty list, the CPU model is +# runnable using the current host and machine-type. +# If @unavailable-features is not present, runnability +# information for the CPU is not available. +# # Since: 1.2.0 ## { 'struct': 'CpuDefinitionInfo', - 'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool' } } + 'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool', + '*unavailable-features': [ 'str' ] } } ## # @query-cpu-definitions: -- 2.7.4

1 0

[libvirt] [PATCH v8 0/5] Add native TLS encrypted chardev TCP support
by John Ferlan 15 Oct '16

15 Oct '16

v7: http://www.redhat.com/archives/libvir-list/2016-September/msg00729.html Differences in v8 1. Update to use 2.4.0 instead of 2.3.0 2. Alter text in qemu.conf as requested in review of v7 3. After update to top of branch found one more virDomainChrDefNew in a recent commit that needed a NULL parameter John Ferlan (5): domain: Add optional 'tls' attribute for TCP chardev conf: Introduce {default|chardev}_tls_x509_secret_uuid qemu: Introduce qemuDomainChardevPrivatePtr qemu: Add a secret object to/for a chardev tcp with secret qemu: Add the ability to hotplug a secret object for TCP chardev TLS docs/formatdomain.html.in | 21 +++ docs/schemas/domaincommon.rng | 5 + src/conf/domain_conf.c | 51 ++++++-- src/conf/domain_conf.h | 5 +- src/libxl/libxl_domain.c | 2 +- src/lxc/lxc_native.c | 2 +- src/qemu/libvirtd_qemu.aug | 2 + src/qemu/qemu.conf | 24 ++++ src/qemu/qemu_command.c | 35 ++++- src/qemu/qemu_command.h | 1 + src/qemu/qemu_conf.c | 22 ++++ src/qemu/qemu_conf.h | 3 + src/qemu/qemu_domain.c | 143 ++++++++++++++++++++- src/qemu/qemu_domain.h | 30 ++++- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 64 ++++++++- src/qemu/qemu_hotplug.h | 3 +- src/qemu/qemu_parse_command.c | 4 +- src/qemu/qemu_process.c | 6 +- src/qemu/test_libvirtd_qemu.aug.in | 2 + src/vz/vz_sdk.c | 2 +- src/xenconfig/xen_sxpr.c | 2 +- src/xenconfig/xen_xl.c | 2 +- tests/qemuhotplugtest.c | 2 +- ...emuxml2argv-serial-tcp-tlsx509-chardev-tls.args | 30 +++++ ...qemuxml2argv-serial-tcp-tlsx509-chardev-tls.xml | 50 +++++++ .../qemuxml2argv-serial-tcp-tlsx509-chardev.xml | 2 +- ...xml2argv-serial-tcp-tlsx509-secret-chardev.args | 38 ++++++ ...uxml2argv-serial-tcp-tlsx509-secret-chardev.xml | 50 +++++++ tests/qemuxml2argvtest.c | 22 ++++ ...muxml2xmlout-serial-tcp-tlsx509-chardev-tls.xml | 1 + .../qemuxml2xmlout-serial-tcp-tlsx509-chardev.xml | 2 +- tests/qemuxml2xmltest.c | 1 + 33 files changed, 594 insertions(+), 37 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-tls.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-tls.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-tls.xml -- 2.7.4

2 17

[libvirt] [PATCH] conf: Explain some code in more detail
by Andrea Bolognani 14 Oct '16

14 Oct '16

The code is entirely correct, but it still managed to trip me up when I first ran into it because I did not realize right away that VIR_PCI_CONNECT_TYPES_ENDPOINT was not a single flag, but rather a mask including both VIR_PCI_CONNECT_TYPE_PCI_DEVICE and VIR_PCI_CONNECT_TYPE_PCIE_DEVICE. In order to save the next distracted traveler in PCI Address Land some time, document this fact with a comment. Add a test case for the behavior as well. --- src/conf/domain_addr.c | 4 +- .../qemuxml2argv-q35-pci-force-address.args | 25 +++++++++++ .../qemuxml2argv-q35-pci-force-address.xml | 32 +++++++++++++++ tests/qemuxml2argvtest.c | 7 ++++ .../qemuxml2xmlout-q35-pci-force-address.xml | 48 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 7 ++++ 6 files changed, 122 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pci-force-address.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pci-force-address.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-pci-force-address.xml diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 0406b50..bbeb611 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -100,7 +100,9 @@ virDomainPCIAddressFlagsCompatible(virPCIDeviceAddressPtr addr, if (fromConfig) { /* If the requested connection was manually specified in * config, allow a PCI device to connect to a PCIe slot, or - * vice versa. + * vice versa. In order to do so, we add *both* the PCI_DEVICE + * and the PCIE_DEVICE flags to the bus if it already has either + * of them, using the ENDPOINT mask. */ if (busFlags & VIR_PCI_CONNECT_TYPES_ENDPOINT) busFlags |= VIR_PCI_CONNECT_TYPES_ENDPOINT; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-pci-force-address.args b/tests/qemuxml2argvdata/qemuxml2argv-q35-pci-force-address.args new file mode 100644 index 0000000..9ca342b --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-pci-force-address.args @@ -0,0 +1,25 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/libexec/qemu-kvm \ +-name q35-test \ +-S \ +-M q35 \ +-m 2048 \ +-smp 2,sockets=2,cores=1,threads=1 \ +-uuid 11dbdcdd-4c3b-482b-8903-9bdb8c0a2774 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-q35-test/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \ +-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \ +-device e1000e,vlan=0,id=net0,mac=52:54:00:aa:bf:ef,bus=pci.2,addr=0x1 \ +-net user,vlan=0,name=hostnet0 \ +-device intel-hda,id=sound0,bus=pci.3,addr=0x0 \ +-device hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-pci-force-address.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35-pci-force-address.xml new file mode 100644 index 0000000..34ec7c7 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-pci-force-address.xml @@ -0,0 +1,32 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'/> + <controller type='pci' index='2' model='pci-bridge'/> + <controller type='pci' index='3' model='pcie-root-port'/> + <sound model='ich6'> + <address type='pci' domain='0x0000' bus='0x03' slot='0x00' function='0x0'/> + </sound> + <interface type='user'> + <model type='e1000e'/> + <mac address='52:54:00:aa:bf:ef'/> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </interface> + <controller type='usb' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b996182..fc18317 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1749,6 +1749,13 @@ mymain(void) QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1, QEMU_CAPS_NEC_USB_XHCI); + /* Make sure the user can always override libvirt's default device + * placement policy by providing an explicit PCI address */ + DO_TEST("q35-pci-force-address", + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_HDA_DUPLEX); DO_TEST_PARSE_ERROR("q35-wrong-root", QEMU_CAPS_DEVICE_PCI_BRIDGE, diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-pci-force-address.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-pci-force-address.xml new file mode 100644 index 0000000..c0332d4 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-pci-force-address.xml @@ -0,0 +1,48 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x10'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </controller> + <controller type='usb' index='0' model='none'/> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <interface type='user'> + <mac address='52:54:00:aa:bf:ef'/> + <model type='e1000e'/> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </interface> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <sound model='ich6'> + <address type='pci' domain='0x0000' bus='0x03' slot='0x00' function='0x0'/> + </sound> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 62bff8c..95c0bf2 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -727,6 +727,13 @@ mymain(void) QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM, QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1, QEMU_CAPS_NEC_USB_XHCI); + /* Make sure the user can always override libvirt's default device + * placement policy by providing an explicit PCI address */ + DO_TEST("q35-pci-force-address", + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_HDA_DUPLEX); DO_TEST("hostdev-scsi-lsi", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_LSI, -- 2.7.4

2 1

[libvirt] [PATCH v6 0/3] Add runnability info to query-cpu-definitions
by Eduardo Habkost 14 Oct '16

14 Oct '16

This series extends query-cpu-definitions to include an extra field: "unavailable-features". The new field can be used to find out reasons that prevent the CPU model from running in the current host. This will return information based on the current machine and accelerator only. In the future we may extend these mechanisms to allow querying other machines and other accelerators without restarting QEMU, but it will require some reorganization of QEMU's main code. To be able to implement this more cleanly, the series rework some of the feature/property name code. This series can be seen in the git branch at: https://github.com/ehabkost/qemu-hacks.git work/query-cpu-definitions-runnable-info The series is based on my x86-next branch: https://github.com/ehabkost/qemu.git x86-next Changes v5 -> v6: * Rebased to x86-next, that already has 8 of the previous patches from v5 applied * Removed x86_cpu_filter_features() from x86_cpu_load_features(), because some of the commands in the CPU model query API need info about CPU models before filtering * Recovered v3 of "target-i386: Move warning code outside x86_cpu_filter_features()" because now we can keep the simpler logic that checked the return value of x86_cpu_filter_features() Diff v5 -> v6: diff --git a/target-i386/cpu.c b/target-i386/cpu.c index d53e711..63330ce 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2052,6 +2052,7 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features, } static void x86_cpu_load_features(X86CPU *cpu, Error **errp); +static int x86_cpu_filter_features(X86CPU *cpu); /* Check for missing features that may prevent the CPU class from * running using the current machine and accelerator. @@ -2085,6 +2086,8 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc, next = &new->next; } + x86_cpu_filter_features(xc); + for (w = 0; w < FEATURE_WORDS; w++) { uint32_t filtered = xc->filtered_features[w]; int i; @@ -2234,11 +2237,14 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w, /* * Filters CPU feature words based on host availability of each feature. + * + * Returns: 0 if all flags are supported by the host, non-zero otherwise. */ -static void x86_cpu_filter_features(X86CPU *cpu) +static int x86_cpu_filter_features(X86CPU *cpu) { CPUX86State *env = &cpu->env; FeatureWord w; + int rv = 0; for (w = 0; w < FEATURE_WORDS; w++) { uint32_t host_feat = @@ -2246,22 +2252,21 @@ static void x86_cpu_filter_features(X86CPU *cpu) uint32_t requested_features = env->features[w]; env->features[w] &= host_feat; cpu->filtered_features[w] = requested_features & ~env->features[w]; + if (cpu->filtered_features[w]) { + rv = 1; + } } + + return rv; } -/* Report list of filtered features to stderr. - * Returns true if some features were found to be filtered, false otherwise - */ -static bool x86_cpu_report_filtered_features(X86CPU *cpu) +static void x86_cpu_report_filtered_features(X86CPU *cpu) { FeatureWord w; - uint32_t filtered = 0; for (w = 0; w < FEATURE_WORDS; w++) { - filtered |= cpu->filtered_features[w]; report_unavailable_features(w, cpu->filtered_features[w]); } - return filtered; } static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props) @@ -3136,8 +3141,6 @@ static void x86_cpu_load_features(X86CPU *cpu, Error **errp) env->cpuid_xlevel2 = env->cpuid_min_xlevel2; } - x86_cpu_filter_features(cpu); - out: if (local_err != NULL) { error_propagate(errp, local_err); @@ -3176,8 +3179,12 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) goto out; } - if (cpu->check_cpuid || cpu->enforce_cpuid) { - if (x86_cpu_report_filtered_features(cpu) && cpu->enforce_cpuid) { + x86_cpu_filter_features(cpu); + + if (x86_cpu_filter_features(cpu) && + (cpu->check_cpuid || cpu->enforce_cpuid)) { + x86_cpu_report_filtered_features(cpu); + if (cpu->enforce_cpuid) { error_setg(&local_err, kvm_enabled() ? "Host doesn't support requested features" : Changes v4 -> v5: * New patch: "target-i386: Register aliases for feature names with underscores" * On patch: "tests: Add test case for x86 feature parsing compatibility": * Fix typo on commit message Reported-by: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> * Add comment noting that the "[+-]feature" compatibility mode will be removed soon * On patch: "target-i386: Make plus_features/minus_features QOM-based": * Removed feat2prop() call on , as we now have property aliases for the old names containing underscores * On patch: "target-i386: Remove underscores from feat_names arrays": * Remove the feat2prop() call from the alias registration loop, too * Commit message update to enumerate all code that uses feat_names * On patch: "target-i386: x86_cpu_load_features() function": * Fix typo on x86_cpu_load_features() comment Reported-by: Paolo Bonzini <pbonzini(a)redhat.com> Diff v4 ->v5: diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 4dd3aee..620889f 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2002,12 +2002,10 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features, /* Compatibility syntax: */ if (featurestr[0] == '+') { - feat2prop(featurestr + 1); plus_features = g_list_append(plus_features, g_strdup(featurestr + 1)); continue; } else if (featurestr[0] == '-') { - feat2prop(featurestr + 1); minus_features = g_list_append(minus_features, g_strdup(featurestr + 1)); continue; @@ -3066,8 +3064,7 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu) env->features[FEAT_XSAVE_COMP_HI] = mask >> 32; } -/* Load CPUID data based on configureured features - */ +/* Load CPUID data based on configured features */ static void x86_cpu_load_features(X86CPU *cpu, Error **errp) { CPUX86State *env = &cpu->env; @@ -3443,7 +3440,10 @@ static void x86_cpu_register_feature_bit_props(X86CPU *cpu, return; } - /* Property names should use "-" instead of "_" */ + /* Property names should use "-" instead of "_". + * Old names containing underscores are registered as aliases + * using object_property_add_alias() + */ assert(!strchr(name, '_')); /* aliases don't use "|" delimiters anymore, they are registered * manually using object_property_add_alias() */ @@ -3496,7 +3496,6 @@ static void x86_cpu_initfn(Object *obj) } } - /* Alias for feature properties: */ object_property_add_alias(obj, "sse3", obj, "pni", &error_abort); object_property_add_alias(obj, "pclmuldq", obj, "pclmulqdq", &error_abort); object_property_add_alias(obj, "sse4-1", obj, "sse4.1", &error_abort); @@ -3505,6 +3504,28 @@ static void x86_cpu_initfn(Object *obj) object_property_add_alias(obj, "ffxsr", obj, "fxsr-opt", &error_abort); object_property_add_alias(obj, "i64", obj, "lm", &error_abort); + object_property_add_alias(obj, "ds_cpl", obj, "ds-cpl", &error_abort); + object_property_add_alias(obj, "tsc_adjust", obj, "tsc-adjust", &error_abort); + object_property_add_alias(obj, "fxsr_opt", obj, "fxsr-opt", &error_abort); + object_property_add_alias(obj, "lahf_lm", obj, "lahf-lm", &error_abort); + object_property_add_alias(obj, "cmp_legacy", obj, "cmp-legacy", &error_abort); + object_property_add_alias(obj, "nodeid_msr", obj, "nodeid-msr", &error_abort); + object_property_add_alias(obj, "perfctr_core", obj, "perfctr-core", &error_abort); + object_property_add_alias(obj, "perfctr_nb", obj, "perfctr-nb", &error_abort); + object_property_add_alias(obj, "kvm_nopiodelay", obj, "kvm-nopiodelay", &error_abort); + object_property_add_alias(obj, "kvm_mmu", obj, "kvm-mmu", &error_abort); + object_property_add_alias(obj, "kvm_asyncpf", obj, "kvm-asyncpf", &error_abort); + object_property_add_alias(obj, "kvm_steal_time", obj, "kvm-steal-time", &error_abort); + object_property_add_alias(obj, "kvm_pv_eoi", obj, "kvm-pv-eoi", &error_abort); + object_property_add_alias(obj, "kvm_pv_unhalt", obj, "kvm-pv-unhalt", &error_abort); + object_property_add_alias(obj, "svm_lock", obj, "svm-lock", &error_abort); + object_property_add_alias(obj, "nrip_save", obj, "nrip-save", &error_abort); + object_property_add_alias(obj, "tsc_scale", obj, "tsc-scale", &error_abort); + object_property_add_alias(obj, "vmcb_clean", obj, "vmcb-clean", &error_abort); + object_property_add_alias(obj, "pause_filter", obj, "pause-filter", &error_abort); + object_property_add_alias(obj, "sse4_1", obj, "sse4.1", &error_abort); + object_property_add_alias(obj, "sse4_2", obj, "sse4.2", &error_abort); + x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort); } diff --git a/tests/test-x86-cpuid-compat.c b/tests/test-x86-cpuid-compat.c index 7cff2b5..260dd27 100644 --- a/tests/test-x86-cpuid-compat.c +++ b/tests/test-x86-cpuid-compat.c @@ -81,9 +81,14 @@ static void test_plus_minus(void) char *path; /* Rules: - * "-foo" overrides "+foo" - * "[+-]foo" overrides "foo=..." - * "foo_bar" should be translated to "foo-bar" + * 1)"-foo" overrides "+foo" + * 2) "[+-]foo" overrides "foo=..." + * 3) Old feature names with underscores (e.g. "sse4_2") + * should keep working + * + * Note: rules 1 and 2 are planned to be removed soon, but we + * need to keep compatibility for a while until we start + * warning users about it. */ qtest_start("-cpu pentium,-fpu,+fpu,-mce,mce=on,+cx8,cx8=off,+sse4_1,sse4_2=on"); path = get_cpu0_qom_path(); Changes v3 -> v4: * Removed patch "Define CPUID filtering functions before x86_cpu_list" * New patch: "tests: Add test case for x86 feature parsing compatibility" * New patch: "target-i386: Disable VME by default with TCG" * Disable VME by default on TCG to avoid returning bogus results for all CPU models in TCG mode * New patch: "target-i386: Make plus_features/minus_features QOM-based" * New patch: "target-i386: Remove underscores from property names" * New patch: "target-i386: Register properties for feature aliases manually" * New patch: "target-i386: xsave: Add FP and SSE bits to x86_ext_save_areas" * New patch: "target-i386: x86_cpu_load_features() function" * On patch: "target-i386: Return runnability information on query-cpu-definitions" * Added code to handle unsupported XSAVE components cleanly * Use x86_cpu_load_features() function Changes v2 -> v3: * Small documentation reword * Suggested-by: Markus Armbruster <armbru(a)redhat.com> * Create a x86_cpu_feature_name() function, to isolate the code that returns the property name Changes v1 -> v2: * Fixed documentation to say "(since 2.7)" * Removed @runnable field, improved documentation Example command output: { "return": [ { "unavailable-features": [], "static": false, "name": "host" }, { "unavailable-features": [], "static": false, "name": "qemu64" }, { "unavailable-features": [], "static": false, "name": "qemu32" }, { "unavailable-features": ["npt", "sse4a", "3dnow", "3dnowext", "fxsr-opt", "mmxext"], "static": false, "name": "phenom" }, { "unavailable-features": [], "static": false, "name": "pentium3" }, { "unavailable-features": [], "static": false, "name": "pentium2" }, { "unavailable-features": [], "static": false, "name": "pentium" }, { "unavailable-features": [], "static": false, "name": "n270" }, { "unavailable-features": [], "static": false, "name": "kvm64" }, { "unavailable-features": [], "static": false, "name": "kvm32" }, { "unavailable-features": [], "static": false, "name": "coreduo" }, { "unavailable-features": [], "static": false, "name": "core2duo" }, { "unavailable-features": ["3dnow", "3dnowext", "mmxext"], "static": false, "name": "athlon" }, { "unavailable-features": [], "static": false, "name": "Westmere" }, { "unavailable-features": ["xgetbv1", "xsavec", "3dnowprefetch", "smap", "adx", "rdseed", "mpx", "rtm", "hle"], "static": false, "name": "Skylake-Client" }, { "unavailable-features": [], "static": false, "name": "SandyBridge" }, { "unavailable-features": [], "static": false, "name": "Penryn" }, { "unavailable-features": ["tbm", "fma4", "xop", "3dnowprefetch", "misalignsse", "sse4a"], "static": false, "name": "Opteron_G5" }, { "unavailable-features": ["fma4", "xop", "3dnowprefetch", "misalignsse", "sse4a"], "static": false, "name": "Opteron_G4" }, { "unavailable-features": ["misalignsse", "sse4a"], "static": false, "name": "Opteron_G3" }, { "unavailable-features": [], "static": false, "name": "Opteron_G2" }, { "unavailable-features": [], "static": false, "name": "Opteron_G1" }, { "unavailable-features": [], "static": false, "name": "Nehalem" }, { "unavailable-features": [], "static": false, "name": "IvyBridge" }, { "unavailable-features": ["rtm", "hle"], "static": false, "name": "Haswell" }, { "unavailable-features": [], "static": false, "name": "Haswell-noTSX" }, { "unavailable-features": [], "static": false, "name": "Conroe" }, { "unavailable-features": ["3dnowprefetch", "smap", "adx", "rdseed", "rtm", "hle"], "static": false, "name": "Broadwell" }, { "unavailable-features": ["3dnowprefetch", "smap", "adx", "rdseed"], "static": false, "name": "Broadwell-noTSX" }, { "unavailable-features": [], "static": false, "name": "486" } ]} Cc: David Hildenbrand <dahi(a)linux.vnet.ibm.com> Cc: Michael Mueller <mimu(a)linux.vnet.ibm.com> Cc: Christian Borntraeger <borntraeger(a)de.ibm.com> Cc: Cornelia Huck <cornelia.huck(a)de.ibm.com> Cc: Jiri Denemark <jdenemar(a)redhat.com> Cc: libvir-list(a)redhat.com Eduardo Habkost (3): target-i386: Move warning code outside x86_cpu_filter_features() target-i386: x86_cpu_load_features() function target-i386: Return runnability information on query-cpu-definitions target-i386/cpu.c | 169 +++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 137 insertions(+), 32 deletions(-) -- 2.7.4

2 18

[libvirt] [PATCH rfc v2 0/8] fspool: backend directory
by Olga Krishtal 14 Oct '16

14 Oct '16

Hi everyone, we would like to propose the first implementation of fspool with directory backend. Filesystem pools is a facility to manage filesystems resources similar to how storage pools manages volume resources. Furthermore new API follows storage API closely where it makes sense. Uploading/downloading operations are not defined yet as it is not obvious how to make it properly. I guess we can use some kind of tar to make a stream from a filesystem. Please share you thoughts on this particular issue. The patchset provides 'dir' backend which simply expose directories in some directory in host filesystem. The virsh commands are provided too. So it is ready to play with, just replace 'pool' in xml descriptions and virsh commands to 'fspool' and 'volume' to 'item'. Examle and usage: Define: virsh -c qemu:///system fspool-define-as fs_pool_name dir --target /path/on/host Build virsh -c qemu:///system fspool-build fs_pool_name Start virsh -c qemu:///system fspool-start fs_pool_name Look inside virsh -c qemu:///system fspool-list (--all) fspool_name Fspool called POOL, on the host fs uses /fs_driver to hold items. virsh -c qemu:///system fspool-dumpxml POOL <fspool type='dir'> <name>POOL</name> <uuid>c57c9d7c-b1d5-4c45-ba9c-67f03d4da160</uuid> <capacity unit='bytes'>733722615808</capacity> <allocation unit='bytes'>1331486720</allocation> <available unit='bytes'>534810800128</available> <source> </source> <target> <path>/fs_driver</path> <permissions> <mode>0755</mode> <owner>0</owner> <group>0</group> </permissions> </target> </fspool> virsh -c qemu:///system fspool-info POOL Name: POOL UUID: c57c9d7c-b1d5-4c45-ba9c-67f03d4da160 State: running Persistent: yes Autostart: no autostart Capacity: 683.33 GiB Allocation: 1.24 GiB Available: 498.08 GiB virsh -c qemu+unix:///system item-list POOL Name Path ------------------------------------------------------------------------------ item1 /fs_driver/item1 item10 /fs_driver/item10 item11 /fs_driver/item11 item12 /fs_driver/item12 item15 /fs_driver/item15 Fspool of directory type is some directory on host fs that holds items (subdirs). Example of usage for items: virsh -c vz+unix:///system item-create-as POOL item1 1g - create item virsh -c qemu+unix:///system item-dumpxml item1 POOL <fsitem> <name>item1</name> <key>/fs_driver/item1</key> <source> </source> <capacity unit='bytes'>0</capacity> <allocation unit='bytes'>0</allocation> <target> <format type='dir'/> </target> </fsitem> virsh -c qemu+unix:///system item-info item1 POOL Name: item1 Type: dir Capacity: 683.33 GiB Allocation: 634.87 MiB Autostart: no autostart Capacity: 683.33 GiB Allocation: 1.24 GiB Available: 498.08 GiB virsh -c qemu+unix:///system item-list POOL Name Path ------------------------------------------------------------------------------ item1 /fs_driver/item1 item10 /fs_driver/item10 item11 /fs_driver/item11 item12 /fs_driver/item12 item15 /fs_driver/item15 v2: - renamed Fs to FS - in configure.ac script macro m4 is used - updates docs - created simple tests - updated virsh.pod - added information abot fspool in fotmatfs.html Olga Krishtal (8): fspool: introduce filesystem pools API fspool: usual driver based implementation of filesystem pools API fspools: configuration and internal representation fspools: acl support for filesystem pools remote: filesystem pools driver implementation fspool: default implementation of filesystem pools virsh: filesystem pools commands fspools: docs and tests for fspool directory backend configure.ac | 38 + daemon/Makefile.am | 4 + daemon/libvirtd.c | 10 + daemon/remote.c | 35 + docs/formatfs.html.in | 208 ++ docs/fspool.html.in | 41 + docs/schemas/fsitem.rng | 66 + docs/schemas/fspool.rng | 82 + docs/sitemap.html.in | 4 + include/libvirt/libvirt-fs.h | 260 +++ include/libvirt/libvirt.h | 1 + include/libvirt/virterror.h | 8 + m4/virt-driver-fspool.m4 | 52 + po/POTFILES.in | 6 + src/Makefile.am | 46 + src/access/viraccessdriver.h | 12 + src/access/viraccessdrivernop.c | 19 + src/access/viraccessdriverpolkit.c | 47 + src/access/viraccessdriverstack.c | 49 + src/access/viraccessmanager.c | 31 + src/access/viraccessmanager.h | 11 + src/access/viraccessperm.c | 15 +- src/access/viraccessperm.h | 124 ++ src/check-driverimpls.pl | 2 + src/conf/fs_conf.c | 1637 ++++++++++++++++ src/conf/fs_conf.h | 323 +++ src/datatypes.c | 154 ++ src/datatypes.h | 94 + src/driver-fs.h | 192 ++ src/driver.h | 3 + src/fs/fs_backend.h | 107 + src/fs/fs_backend_dir.c | 355 ++++ src/fs/fs_backend_dir.h | 8 + src/fs/fs_driver.c | 2058 ++++++++++++++++++++ src/fs/fs_driver.h | 10 + src/libvirt-fs.c | 1556 +++++++++++++++ src/libvirt.c | 28 + src/libvirt_private.syms | 53 + src/libvirt_public.syms | 42 + src/remote/remote_driver.c | 66 + src/remote/remote_protocol.x | 466 ++++- src/remote_protocol-structs | 165 ++ src/rpc/gendispatch.pl | 23 +- src/util/virerror.c | 37 + tests/Makefile.am | 12 + tests/fsitemxml2xmlin/item.xml | 13 + tests/fsitemxml2xmlout/item.xml | 13 + tests/fsitemxml2xmltest.c | 105 + .../dir-missing-target-path-invalid.xml | 12 + tests/fspoolxml2xmlin/fspool-dir.xml | 16 + tests/fspoolxml2xmlout/fspool-dir.xml | 16 + tests/fspoolxml2xmltest.c | 81 + tools/Makefile.am | 2 + tools/virsh-fsitem.c | 1292 ++++++++++++ tools/virsh-fsitem.h | 39 + tools/virsh-fspool.c | 1586 +++++++++++++++ tools/virsh-fspool.h | 38 + tools/virsh.c | 4 + tools/virsh.h | 9 + tools/virsh.pod | 252 ++- 60 files changed, 12028 insertions(+), 10 deletions(-) create mode 100644 docs/formatfs.html.in create mode 100644 docs/fspool.html.in create mode 100644 docs/schemas/fsitem.rng create mode 100644 docs/schemas/fspool.rng create mode 100644 include/libvirt/libvirt-fs.h create mode 100644 m4/virt-driver-fspool.m4 create mode 100644 src/conf/fs_conf.c create mode 100644 src/conf/fs_conf.h create mode 100644 src/driver-fs.h create mode 100644 src/fs/fs_backend.h create mode 100644 src/fs/fs_backend_dir.c create mode 100644 src/fs/fs_backend_dir.h create mode 100644 src/fs/fs_driver.c create mode 100644 src/fs/fs_driver.h create mode 100644 src/libvirt-fs.c create mode 100644 tests/fsitemxml2xmlin/item.xml create mode 100644 tests/fsitemxml2xmlout/item.xml create mode 100644 tests/fsitemxml2xmltest.c create mode 100644 tests/fspoolschemadata/dir-missing-target-path-invalid.xml create mode 100644 tests/fspoolxml2xmlin/fspool-dir.xml create mode 100644 tests/fspoolxml2xmlout/fspool-dir.xml create mode 100644 tests/fspoolxml2xmltest.c create mode 100644 tools/virsh-fsitem.c create mode 100644 tools/virsh-fsitem.h create mode 100644 tools/virsh-fspool.c create mode 100644 tools/virsh-fspool.h -- 1.8.3.1

4 21

[libvirt] [PATCH 0/8] IVSHMEM -- third time's the charm
by Martin Kletzander 14 Oct '16

14 Oct '16

In this version we bring back the model, but disable migration for now. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1347049 Martin Kletzander (8): conf: Fix virDomainShmemDefFind qemu: Disable migration with ivshmem conf, qemu: Add support for shmem model conf, qemu: Add newer shmem models qemu: Add capabilities for ivshmem-{plain,doorbell} qemu: Save various defaults for shmem qemu: Support newer ivshmem device variants qemu: Add support for hot/cold-(un)plug of shmem devices docs/schemas/domaincommon.rng | 11 + src/conf/domain_conf.c | 50 +++-- src/conf/domain_conf.h | 10 + src/libvirt_private.syms | 2 + src/qemu/qemu_capabilities.c | 4 + src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_command.c | 100 ++++++++- src/qemu/qemu_command.h | 10 + src/qemu/qemu_domain.c | 49 +++- src/qemu/qemu_driver.c | 39 +++- src/qemu/qemu_hotplug.c | 247 ++++++++++++++++++++- src/qemu/qemu_hotplug.h | 6 + src/qemu/qemu_migration.c | 6 + .../caps_2.6.0-gicv2.aarch64.xml | 2 + .../caps_2.6.0-gicv3.aarch64.xml | 2 + tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 2 + tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 2 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 2 + tests/qemuhotplugtest.c | 21 ++ .../qemuhotplug-ivshmem-doorbell-detach.xml | 7 + .../qemuhotplug-ivshmem-doorbell.xml | 4 + .../qemuhotplug-ivshmem-plain-detach.xml | 6 + .../qemuhotplug-ivshmem-plain.xml | 3 + ...muhotplug-base-live+ivshmem-doorbell-detach.xml | 1 + .../qemuhotplug-base-live+ivshmem-doorbell.xml | 65 ++++++ .../qemuhotplug-base-live+ivshmem-plain-detach.xml | 1 + .../qemuhotplug-base-live+ivshmem-plain.xml | 58 +++++ .../qemuxml2argv-shmem-plain-doorbell.args | 43 ++++ ...m.xml => qemuxml2argv-shmem-plain-doorbell.xml} | 11 +- tests/qemuxml2argvdata/qemuxml2argv-shmem.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-shmem.xml | 2 + tests/qemuxml2argvtest.c | 3 + tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 9 + 33 files changed, 760 insertions(+), 22 deletions(-) create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-doorbell-detach.xml create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-doorbell.xml create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain-detach.xml create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain.xml create mode 120000 tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell-detach.xml create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell.xml create mode 120000 tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain-detach.xml create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args copy tests/qemuxml2argvdata/{qemuxml2argv-shmem.xml => qemuxml2argv-shmem-plain-doorbell.xml} (82%) -- 2.10.0

3 28

[libvirt] [PATCH 0/2] RFC: New libssh transport
by Pino Toscano 14 Oct '16

14 Oct '16

Hi, this series introduces a new libssh transport in libvirt, based on the libssh C library. This library supports what libssh2 does, and more: - easier API for known_hosts handling (there's a ticket upstream to request extensions for it, but what is implemented now works well) - potential GSSAPI authentication (not enabled yet because of a libssh bug [1] - easier API for ssh-agent support TODO: - implement the keyboard-interactive authentication method - more testing than my own The implementation for the new transport is based on the libssh2 one, hence it shares origin and style. [1] https://red.libssh.org/issues/242 Thanks, Pino Toscano (2): virNetSocket: allow to not close FD libssh_transport: add new libssh-based transport config-post.h | 2 + configure.ac | 3 + include/libvirt/virterror.h | 2 + m4/virt-libssh.m4 | 26 + src/Makefile.am | 21 +- src/libvirt_libssh.syms | 22 + src/remote/remote_driver.c | 41 ++ src/rpc/virnetclient.c | 123 ++++ src/rpc/virnetclient.h | 13 + src/rpc/virnetlibsshsession.c | 1267 +++++++++++++++++++++++++++++++++++++++++ src/rpc/virnetlibsshsession.h | 80 +++ src/rpc/virnetsocket.c | 184 +++++- src/rpc/virnetsocket.h | 13 + src/util/virerror.c | 9 +- 14 files changed, 1802 insertions(+), 4 deletions(-) create mode 100644 m4/virt-libssh.m4 create mode 100644 src/libvirt_libssh.syms create mode 100644 src/rpc/virnetlibsshsession.c create mode 100644 src/rpc/virnetlibsshsession.h -- 2.7.4

1 2

[libvirt] [PATCH 0/2] support qemu drive cache.* parameters
by Nikolay Shirokovskiy 14 Oct '16

14 Oct '16

Nikolay Shirokovskiy (2): conf: add disk cache tuning parameters after qemu qemu: support <cachetune> in domain disk xml .gnulib | 2 +- docs/schemas/domaincommon.rng | 22 ++++++ src/conf/domain_conf.c | 90 ++++++++++++++++++++++ src/conf/domain_conf.h | 7 ++ src/qemu/qemu_capabilities.c | 12 ++- src/qemu/qemu_capabilities.h | 3 + src/qemu/qemu_command.c | 30 ++++++++ tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml | 3 + tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml | 3 + tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml | 3 + tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml | 3 + tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml | 3 + .../caps_2.6.0-gicv2.aarch64.xml | 3 + .../caps_2.6.0-gicv3.aarch64.xml | 3 + tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 3 + tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 3 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 3 + 17 files changed, 194 insertions(+), 2 deletions(-) -- 1.8.3.1

3 8

[libvirt] [PATCH] util: Fix build on s390
by Jiri Denemark 14 Oct '16

14 Oct '16

GCC on s390 complains util/virconf.c: In function 'virConfGetValueSizeT': util/virconf.c:1220:9: error: format '%zu' expects argument of type 'size_t', but argument 9 has type 'unsigned int' [-Werror=format=] Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com> --- src/util/virconf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virconf.c b/src/util/virconf.c index 3e49f41..1372389 100644 --- a/src/util/virconf.c +++ b/src/util/virconf.c @@ -1219,7 +1219,7 @@ int virConfGetValueSizeT(virConfPtr conf, if (((unsigned long long)cval->l) > SIZE_MAX) { virReportError(VIR_ERR_INTERNAL_ERROR, _("%s: value for '%s' parameter must be in range 0:%zu"), - conf->filename, setting, SIZE_MAX); + conf->filename, setting, (size_t) SIZE_MAX); return -1; } #endif -- 2.10.1

5 9

Re: [libvirt] [PATCH] util: Alter return value of virReadFCHost and fix mem leak
by Erik Skultety 14 Oct '16

14 Oct '16

On Wed, Oct 12, 2016 at 09:20:29AM -0400, John Ferlan wrote: > > > On 10/12/2016 06:40 AM, Erik Skultety wrote: > > On Tue, Oct 11, 2016 at 05:25:49PM -0400, John Ferlan wrote: > >> https://bugzilla.redhat.com/show_bug.cgi?id=1357416 > >> > >> Rather than return a 0 or -1 and the *result string, return just the result > >> string to the caller. Alter all the callers to handle the different return. > >> > >> As a side effect or result of this, it's much clearer that we cannot just > >> assign the returned string into the scsi_host wwnn, wwpn, and fabric_wwn > >> fields - rather we should fetch a temporary string, then as long as our > >> fetch was good, VIR_FREE what may have been there, and STEAL what we just got. > >> This fixes a memory leak in the virNodeDeviceCreateXML code path through > >> find_new_device and nodeDeviceLookupSCSIHostByWWN which will continually > >> call nodeDeviceSysfsGetSCSIHostCaps until the expected wwnn/wwpn is found > >> in the device object capabilities. > >> > >> Signed-off-by: John Ferlan <jferlan(a)redhat.com> > >> --- > >> > >> I suppose I could have made two patches out of this, but it felt like > >> overkill once I realized what the issue was. OK a third one line patch > >> could have been added to change the virGetFCHostNameByWWN comment as well, > >> but that'd really be overkill. > >> > >> src/node_device/node_device_linux_sysfs.c | 55 ++++++++++++------------------- > >> src/util/virutil.c | 34 ++++++++----------- > >> src/util/virutil.h | 9 +++-- > >> tests/fchosttest.c | 30 ++++++----------- > >> 4 files changed, 49 insertions(+), 79 deletions(-) > >> > >> diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c > >> index 549d32c..be99c41 100644 > >> --- a/src/node_device/node_device_linux_sysfs.c > >> +++ b/src/node_device/node_device_linux_sysfs.c > >> @@ -44,8 +44,7 @@ VIR_LOG_INIT("node_device.node_device_linux_sysfs"); > >> int > >> nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d) > >> { > >> - char *max_vports = NULL; > >> - char *vports = NULL; > >> + char *tmp = NULL; > >> int ret = -1; > >> > >> if (virReadSCSIUniqueId(NULL, d->scsi_host.host, > >> @@ -59,64 +58,53 @@ nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d) > >> if (virIsCapableFCHost(NULL, d->scsi_host.host)) { > >> d->scsi_host.flags |= VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST; > >> > >> - if (virReadFCHost(NULL, > >> - d->scsi_host.host, > >> - "port_name", > >> - &d->scsi_host.wwpn) < 0) { > >> + if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "port_name"))) { > >> VIR_WARN("Failed to read WWPN for host%d", d->scsi_host.host); > >> goto cleanup; > >> } > >> + VIR_FREE(d->scsi_host.wwpn); > >> + VIR_STEAL_PTR(d->scsi_host.wwpn, tmp); > >> > >> - if (virReadFCHost(NULL, > >> - d->scsi_host.host, > >> - "node_name", > >> - &d->scsi_host.wwnn) < 0) { > >> + if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "node_name"))) { > >> VIR_WARN("Failed to read WWNN for host%d", d->scsi_host.host); > >> goto cleanup; > >> } > >> + VIR_FREE(d->scsi_host.wwnn); > >> + VIR_STEAL_PTR(d->scsi_host.wwnn, tmp); > >> > >> - if (virReadFCHost(NULL, > >> - d->scsi_host.host, > >> - "fabric_name", > >> - &d->scsi_host.fabric_wwn) < 0) { > >> + if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "fabric_name"))) { > >> VIR_WARN("Failed to read fabric WWN for host%d", > >> d->scsi_host.host); > >> goto cleanup; > >> } > >> + VIR_FREE(d->scsi_host.fabric_wwn); > >> + VIR_STEAL_PTR(d->scsi_host.fabric_wwn, tmp); > >> } > >> > > > > So if I understand correctly, the problem is basically that we did not call > > VIR_FREE on the data that had previously been filled to the virNodeDevCapData > > structure during replacing it with new data. So, we could just keep the > > signature of virReadFCHost as is and just add the VIR_FREE and VIR_STEAL_PTR > > to the function, thus there won't be any need to use VIR_FREE+STEAL repeatedly > > in here. > > > > True - we're overwriting the &d->scsi_host.{wwnn|wwpn|fabric_wwn} fields > and that's the primary issue (e.g. mem leak). > > While I understand your point, I'm not sure adding a VIR_FREE(*result) > to virReadFCHost just prior to the "if VIR_STRDUP(*result, p) < 0)" is a > proper solution since virReadFCHost does not "own" that memory. It > doesn't state that if *result has something in it, the code will perform > a VIR_FREE (although that's a nice convenience in this case). Sure it's > simple enough to add that comment, but then I believe that the I don't see a major problem here... > VIR_FREE() would have to be done at the top of the function; otherwise, > how does the caller distinguish which error occurred when -1 gets > returned and whether it should VIR_FREE itself? > Well, I have to admin that this^^ is a fair argument because there are 3 different spots where the function can fail, not that the caller could not check result for NULL but the fact that a function touched caller's argument and then failed would be just weird. So, yeah, good point. > Also, what if the caller didn't want the *result wiped out? What if it > was using it to compare what it found "this" time vs. what was present > in the data/file a previous time? That caller would then need to be > adjusted to pass a temporary variable anyway. Exactly, if a caller wanted to just compare 2 values - old and new one - they would use a temporary variable, that would IMHO be expected correct behaviour. Anyway, it's irrelevant now that I agree with your point above. > > I think for me most compelling reason to alter virReadFCHost is that it > follows the mindset of not returning two values from a function when one > is perfectly reasonable. > > John > > BTW: The OCD would still kick in and want to change the comment below to > match the function name... Yeah, since the patch is going to be relatively beefy, this change will blend in fairly easily, so go ahead, ACK. Erik NOTE: I've recently switched to mutt and it's still quite new, so I forgot to hit 'g' for group reply, therefore most of this conversation 'officially' never happened, so CC'ng libvir-list at least for the last bit, I suppose the whole 'thread' is going to look odd, sigh.... > > >> -/* virGetHostNameByWWN: > >> +/* virGetFCHostNameByWWN: > > > > Normally, I'd say sure, you're absolutely right about not creating a separate > > patch for this kind of change, but once you make the adjustment I mentioned > > above none of the below changes will be necessary and thus this rename change > > would look sort of unrelated imho. > > > > ACK with the adjustment. > > > > Erik > > > >> * > >> * Iterate over the sysfs tree to get FC host name (e.g. host5) > >> * by the provided "wwnn,wwpn" pair. > >> @@ -2298,7 +2293,7 @@ virFindFCHostCapableVport(const char *sysfs_prefix) > >> if (!virIsCapableVport(prefix, host)) > >> continue; > >> > >> - if (virReadFCHost(prefix, host, "port_state", &state) < 0) { > >> + if (!(state = virReadFCHost(prefix, host, "port_state"))) { > >> VIR_DEBUG("Failed to read port_state for host%d", host); > >> continue; > >> } > >> @@ -2310,12 +2305,12 @@ virFindFCHostCapableVport(const char *sysfs_prefix) > >> } > >> VIR_FREE(state); > >> > >> - if (virReadFCHost(prefix, host, "max_npiv_vports", &max_vports) < 0) { > >> + if (!(max_vports = virReadFCHost(prefix, host, "max_npiv_vports"))) { > >> VIR_DEBUG("Failed to read max_npiv_vports for host%d", host); > >> continue; > >> } > >> > >> - if (virReadFCHost(prefix, host, "npiv_vports_inuse", &vports) < 0) { > >> + if (!(vports = virReadFCHost(prefix, host, "npiv_vports_inuse"))) { > >> VIR_DEBUG("Failed to read npiv_vports_inuse for host%d", host); > >> VIR_FREE(max_vports); > >> continue; > >> @@ -2379,14 +2374,13 @@ virGetSCSIHostNameByParentaddr(unsigned int domain ATTRIBUTE_UNUSED, > >> return NULL; > >> } > >> > >> -int > >> +char * > >> virReadFCHost(const char *sysfs_prefix ATTRIBUTE_UNUSED, > >> int host ATTRIBUTE_UNUSED, > >> - const char *entry ATTRIBUTE_UNUSED, > >> - char **result ATTRIBUTE_UNUSED) > >> + const char *entry ATTRIBUTE_UNUSED) > >> { > >> virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); > >> - return -1; > >> + return NULL; > >> } > >> > >> bool > >> diff --git a/src/util/virutil.h b/src/util/virutil.h > >> index 703ec53..8c0d83c 100644 > >> --- a/src/util/virutil.h > >> +++ b/src/util/virutil.h > >> @@ -182,11 +182,10 @@ virGetSCSIHostNameByParentaddr(unsigned int domain, > >> unsigned int slot, > >> unsigned int function, > >> unsigned int unique_id); > >> -int virReadFCHost(const char *sysfs_prefix, > >> - int host, > >> - const char *entry, > >> - char **result) > >> - ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); > >> +char *virReadFCHost(const char *sysfs_prefix, > >> + int host, > >> + const char *entry) > >> + ATTRIBUTE_NONNULL(3); > >> > >> bool virIsCapableFCHost(const char *sysfs_prefix, int host); > >> bool virIsCapableVport(const char *sysfs_prefix, int host); > >> diff --git a/tests/fchosttest.c b/tests/fchosttest.c > >> index e9b89a7..a08a2e8 100644 > >> --- a/tests/fchosttest.c > >> +++ b/tests/fchosttest.c > >> @@ -68,35 +68,25 @@ test3(const void *data ATTRIBUTE_UNUSED) > >> char *vports = NULL; > >> int ret = -1; > >> > >> - if (virReadFCHost(TEST_FC_HOST_PREFIX, > >> - TEST_FC_HOST_NUM, > >> - "node_name", > >> - &wwnn) < 0) > >> + if (!(wwnn = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM, > >> + "node_name"))) > >> return -1; > >> > >> - if (virReadFCHost(TEST_FC_HOST_PREFIX, > >> - TEST_FC_HOST_NUM, > >> - "port_name", > >> - &wwpn) < 0) > >> + if (!(wwpn = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM, > >> + "port_name"))) > >> goto cleanup; > >> > >> - if (virReadFCHost(TEST_FC_HOST_PREFIX, > >> - TEST_FC_HOST_NUM, > >> - "fabric_name", > >> - &fabric_wwn) < 0) > >> + if (!(fabric_wwn = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM, > >> + "fabric_name"))) > >> goto cleanup; > >> > >> - if (virReadFCHost(TEST_FC_HOST_PREFIX, > >> - TEST_FC_HOST_NUM, > >> - "max_npiv_vports", > >> - &max_vports) < 0) > >> + if (!(max_vports = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM, > >> + "max_npiv_vports"))) > >> goto cleanup; > >> > >> > >> - if (virReadFCHost(TEST_FC_HOST_PREFIX, > >> - TEST_FC_HOST_NUM, > >> - "npiv_vports_inuse", > >> - &vports) < 0) > >> + if (!(vports = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM, > >> + "npiv_vports_inuse"))) > >> goto cleanup; > >> > >> if (STRNEQ(expect_wwnn, wwnn) || > >> -- > >> 2.7.4 > >> > >> -- > >> libvir-list mailing list > >> libvir-list(a)redhat.com > >> https://www.redhat.com/mailman/listinfo/libvir-list

2 2