Re: [libvirt] [PATCH 1/1] qemu: hide details of fake reboot
On 19.12.2019 04:30, Cole Robinson wrote:
> On 10/30/19 4:09 AM, Nikolay Shirokovskiy wrote:
>> If we use fake reboot then domain goes thru running->shutdown->running
>> state changes with shutdown state only for short period of time. At
>> least this is implementation details leaking into API. And also there is
>> one real case when this is not convinient. I'm doing a backup with the
>> help of temporary block snapshot (with the help of qemu's API which is
>> used in the newly created libvirt's backup API). If guest is shutdowned
>> I want to continue to backup so I don't kill the process and domain is
>> in shutdown state. Later when backup is finished I want to destroy qemu
>> process. So I check if it is in shutdowned state and destroy it if it
>> is. Now if instead of shutdown domain got fake reboot then I can destroy
>> process in the middle of fake reboot process.
>>
>> After shutdown event we also get stop event and now as domain state is
>> running it will be transitioned to paused state and back to running
>> later. Though this is not critical for the described case I guess it is
>> better not to leak these details to user too. So let's leave domain in
>> running state on stop event if fake reboot is in process.
>>
>> As we don't know when stop event really arrive let's move resetting flag
>> after starting CPUs - at this point stop event should be handled.
>>
>
> Sorry this didn't receive a timely response. Unsurprisingly it's
> conflicting with master now.
>
> Conceptually what you say makes sense, that the 'shutdown' is an
> implementation detail of our 'reboot' implementation and details
> shouldn't leak to apps.
>
> Here's the events I see with: sudo virsh reboot --mode=acpi f30
>
> $ sudo virsh event --domain f30 --all --loop
> event 'agent-lifecycle' for domain f30: state: 'disconnected' reason:
> 'channel event'
> event 'lifecycle' for domain f30: Shutdown Finished after guest request
> event 'reboot' for domain f30
> event 'lifecycle' for domain f30: Resumed Unpaused
> event 'reboot' for domain f30
> event 'agent-lifecycle' for domain f30: state: 'connected' reason:
> 'channel event'
>
> Using --mode=agent gives:
>
> event 'agent-lifecycle' for domain f30: state: 'disconnected' reason:
> 'channel event'
> event 'reboot' for domain f30
> event 'reboot' for domain f30
> event 'agent-lifecycle' for domain f30: state: 'connected' reason:
> 'channel event'
>
> So this change is moving the first more towards the latter. (not sure
> what those two reboot events are about)
>
>> Signed-off-by: Nikolay Shirokovskiy
>> ---
>> src/qemu/qemu_process.c | 57 ++---
>> 1 file changed, 31 insertions(+), 26 deletions(-)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index ed8666e9d1..2d37f92724 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -500,6 +500,8 @@ qemuProcessFakeReboot(void *opaque)
>> goto endjob;
>> }
>>
>> +qemuDomainSetFakeReboot(driver, vm, false);
>> +
>
> I think this should also be in the cleanup path of this function,
> otherwise an earlier error looks like it would leave priv->fakeReboot
> still set. But it makes sense it needs to be here too, so it's saved in
> the domain status
Fake reboot is not reseted only in case of error but in this case domain
is killed forcefully and qemuProcessInit resets fake reboot flag so seems
good as it is to me.
By the way I reset fake reboot flag this late so that is seen as set by
qemuProcessHandleStop.
>
>> if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm,
>> driver->caps) < 0) {
>> VIR_WARN("Unable to save status on vm %s after state change",
>> vm->def->name);
>> @@ -525,7 +527,6 @@ qemuProcessShutdownOrReboot(virQEMUDriverPtr driver,
>> qemuDomainObjPrivatePtr priv = vm->privateData;
>>
>> if (priv->fakeReboot) {
>> -qemuDomainSetFakeReboot(driver, vm, false);
>> virObjectRef(vm);
>> virThread th;
>> if (virThreadCreate(,
>> @@ -534,6 +535,7 @@ qemuProcessShutdownOrReboot(virQEMUDriverPtr driver,
>> vm) < 0) {
>> VIR_ERROR(_("Failed to create reboot thread, killing domain"));
>> ignore_value(qemuProcessKill(vm, VIR_QEMU_PROCESS_KILL_NOWAIT));
>> +qemuDomainSetFakeReboot(driver, vm, false);
>> virObjectUnref(vm);
>> }
>> } else {
>> @@ -595,35 +597,37 @@ qemuProcessHandleShutdown(qemuMonitorPtr mon
>> G_GNUC_UNUSED,
>> goto unlock;
>> }
>>
>> -VIR_DEBUG("Transitioned guest %s to shutdown state",
>> - vm->def->name);
>> -virDomainObjSetState(vm,
>> - VIR_DOMAIN_SHUTDOWN,
>> - VIR_DOMAIN_SHUTDOWN_UNKNOWN);
>> +if (!priv->fakeReboot) {
>
> Here it would be helpful to have a comment like
>
> /* If the qemu process was stopped as part of fakeReboot
Re: [libvirt] [PATCH 1/1] qemu: hide details of fake reboot
On 10/30/19 4:09 AM, Nikolay Shirokovskiy wrote:
> If we use fake reboot then domain goes thru running->shutdown->running
> state changes with shutdown state only for short period of time. At
> least this is implementation details leaking into API. And also there is
> one real case when this is not convinient. I'm doing a backup with the
> help of temporary block snapshot (with the help of qemu's API which is
> used in the newly created libvirt's backup API). If guest is shutdowned
> I want to continue to backup so I don't kill the process and domain is
> in shutdown state. Later when backup is finished I want to destroy qemu
> process. So I check if it is in shutdowned state and destroy it if it
> is. Now if instead of shutdown domain got fake reboot then I can destroy
> process in the middle of fake reboot process.
>
> After shutdown event we also get stop event and now as domain state is
> running it will be transitioned to paused state and back to running
> later. Though this is not critical for the described case I guess it is
> better not to leak these details to user too. So let's leave domain in
> running state on stop event if fake reboot is in process.
>
> As we don't know when stop event really arrive let's move resetting flag
> after starting CPUs - at this point stop event should be handled.
>
Sorry this didn't receive a timely response. Unsurprisingly it's
conflicting with master now.
Conceptually what you say makes sense, that the 'shutdown' is an
implementation detail of our 'reboot' implementation and details
shouldn't leak to apps.
Here's the events I see with: sudo virsh reboot --mode=acpi f30
$ sudo virsh event --domain f30 --all --loop
event 'agent-lifecycle' for domain f30: state: 'disconnected' reason:
'channel event'
event 'lifecycle' for domain f30: Shutdown Finished after guest request
event 'reboot' for domain f30
event 'lifecycle' for domain f30: Resumed Unpaused
event 'reboot' for domain f30
event 'agent-lifecycle' for domain f30: state: 'connected' reason:
'channel event'
Using --mode=agent gives:
event 'agent-lifecycle' for domain f30: state: 'disconnected' reason:
'channel event'
event 'reboot' for domain f30
event 'reboot' for domain f30
event 'agent-lifecycle' for domain f30: state: 'connected' reason:
'channel event'
So this change is moving the first more towards the latter. (not sure
what those two reboot events are about)
> Signed-off-by: Nikolay Shirokovskiy
> ---
> src/qemu/qemu_process.c | 57 ++---
> 1 file changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index ed8666e9d1..2d37f92724 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -500,6 +500,8 @@ qemuProcessFakeReboot(void *opaque)
> goto endjob;
> }
>
> +qemuDomainSetFakeReboot(driver, vm, false);
> +
I think this should also be in the cleanup path of this function,
otherwise an earlier error looks like it would leave priv->fakeReboot
still set. But it makes sense it needs to be here too, so it's saved in
the domain status
> if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps)
> < 0) {
> VIR_WARN("Unable to save status on vm %s after state change",
> vm->def->name);
> @@ -525,7 +527,6 @@ qemuProcessShutdownOrReboot(virQEMUDriverPtr driver,
> qemuDomainObjPrivatePtr priv = vm->privateData;
>
> if (priv->fakeReboot) {
> -qemuDomainSetFakeReboot(driver, vm, false);
> virObjectRef(vm);
> virThread th;
> if (virThreadCreate(,
> @@ -534,6 +535,7 @@ qemuProcessShutdownOrReboot(virQEMUDriverPtr driver,
> vm) < 0) {
> VIR_ERROR(_("Failed to create reboot thread, killing domain"));
> ignore_value(qemuProcessKill(vm, VIR_QEMU_PROCESS_KILL_NOWAIT));
> +qemuDomainSetFakeReboot(driver, vm, false);
> virObjectUnref(vm);
> }
> } else {
> @@ -595,35 +597,37 @@ qemuProcessHandleShutdown(qemuMonitorPtr mon
> G_GNUC_UNUSED,
> goto unlock;
> }
>
> -VIR_DEBUG("Transitioned guest %s to shutdown state",
> - vm->def->name);
> -virDomainObjSetState(vm,
> - VIR_DOMAIN_SHUTDOWN,
> - VIR_DOMAIN_SHUTDOWN_UNKNOWN);
> +if (!priv->fakeReboot) {
Here it would be helpful to have a comment like
/* If the qemu process was stopped as part of fakeReboot reset, we skip
sending a shutdown event */
> +VIR_DEBUG("Transitioned guest %s to shutdown state",
> + vm->def->name);
> +virDomainObjSetState(vm,
> + VIR_DOMAIN_SHUTDOWN,
> + VIR_DOMAIN_SHUTDOWN_UNKNOWN);
>
> -switch (guest_initiated) {
> -case VIR_TRISTATE_BOOL_YES:
> -detail = VIR_DOMAIN_EVENT_SHUTDOWN_GUEST;
> -break;
> +switch
Re: [libvirt] [PATCH 2/2] qemu_firmware: Try to autofill for old style UEFI specification
On 12/17/19 12:25 PM, Michal Privoznik wrote:
> While we discourage people to use the old style of specifying
> UEFI for their domains (the old style is putting path to the FW
> image under /domain/os/loader/ whilst the new one is using
> /domain/os/@firmware), some applications might have not adopted
> yet. They still rely on libvirt autofilling NVRAM path and
> figuring out NVRAM template when using the old way (notably
> virt-install does this). And in a way they are right. However,
> since we really want distro maintainers to leave
> --with-loader-nvram configure option and rely on JSON
> descriptors, we need to implement autofilling of NVRAM template
> for the old way too.
>
> Fedora: https://bugzilla.redhat.com/show_bug.cgi?id=1782778
> RHEL: https://bugzilla.redhat.com/show_bug.cgi?id=1776949
>
> Signed-off-by: Michal Privoznik
> ---
So this uses firmware.json to info to turn this input XML
/CODE.fd
into this output XML
/CODE.fd
independent of --with-loader-nvram, which was previously used to handle
that case. And virt-install still uses this method by default. Is that
correct?
I'm surprised we don't have an an XML test case to cover this. How hard
is it to add?
> src/qemu/qemu_firmware.c | 82 +++-
> 1 file changed, 72 insertions(+), 10 deletions(-)
>
> diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
> index 96058c9b45..a31bde5d04 100644
> --- a/src/qemu/qemu_firmware.c
> +++ b/src/qemu/qemu_firmware.c
> @@ -935,17 +935,53 @@ qemuFirmwareMatchDomain(const virDomainDef *def,
> const qemuFirmware *fw,
> const char *path)
> {
> -size_t i;
> +qemuFirmwareOSInterface want = QEMU_FIRMWARE_OS_INTERFACE_NONE;
> bool supportsS3 = false;
> bool supportsS4 = false;
> bool requiresSMM = false;
> bool supportsSEV = false;
> +size_t i;
> +
> +switch (def->os.firmware) {
> +case VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS:
> +want = QEMU_FIRMWARE_OS_INTERFACE_BIOS;
> +break;
> +case VIR_DOMAIN_OS_DEF_FIRMWARE_EFI:
> +want = QEMU_FIRMWARE_OS_INTERFACE_UEFI;
> +break;
> +case VIR_DOMAIN_OS_DEF_FIRMWARE_NONE:
> +case VIR_DOMAIN_OS_DEF_FIRMWARE_LAST:
> +break;
> +}
> +
This is a refactoring that could have been done first. It's hard to
digest all the details in this patch.
> +if (want == QEMU_FIRMWARE_OS_INTERFACE_NONE &&
> +def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE &&
> +def->os.loader) {
> +switch (def->os.loader->type) {
> +case VIR_DOMAIN_LOADER_TYPE_ROM:
> +want = QEMU_FIRMWARE_OS_INTERFACE_BIOS;
> +break;
> +case VIR_DOMAIN_LOADER_TYPE_PFLASH:
> +want = QEMU_FIRMWARE_OS_INTERFACE_UEFI;
> +break;
> +case VIR_DOMAIN_LOADER_TYPE_NONE:
> +case VIR_DOMAIN_LOADER_TYPE_LAST:
> +break;
> +}
> +
And it might be overkill but it would help readability if these switches
were moved to their own functions, like
qemuFirmwareTypeFromInterfaceType or similar
> +if (fw->mapping.device != QEMU_FIRMWARE_DEVICE_FLASH ||
> +STRNEQ(def->os.loader->path,
> fw->mapping.data.flash.executable.filename)) {
> +VIR_DEBUG("Not matching FW interface %s or loader "
> + "path '%s' for user provided path '%s'",
> + qemuFirmwareDeviceTypeToString(fw->mapping.device),
> + fw->mapping.data.flash.executable.filename,
> + def->os.loader->path);
> +return false;
> +}
> +}
>
> for (i = 0; i < fw->ninterfaces; i++) {
> -if ((def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS &&
> - fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_BIOS) ||
> -(def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI &&
> - fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_UEFI))
> +if (fw->interfaces[i] == want)
> break;
> }
>
> @@ -1211,14 +1247,33 @@ qemuFirmwareFillDomain(virQEMUDriverPtr driver,
> qemuFirmwarePtr *firmwares = NULL;
> ssize_t nfirmwares = 0;
> const qemuFirmware *theone = NULL;
> +bool needResult = true;
> size_t i;
> int ret = -1;
>
> if (!(flags & VIR_QEMU_PROCESS_START_NEW))
> return 0;
>
> -if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE)
> -return 0;
> +/* Fill in FW paths if either os.firmware is enabled, or
> + * loader path was provided with no nvram varstore. */
> +if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE) {
> +/* This is horrific check, but loosely said, if UEFI
> + * image was provided by the old method (by specifying
> + * its path in domain XML) but no template for NVRAM was
> + * specified ... */
> +if (!(def->os.loader &&
> +
Re: [libvirt] [PATCH 1/2] qemu_firmware: Pass virDomainDef into qemuFirmwareFillDomain()
On 12/17/19 12:25 PM, Michal Privoznik wrote: > This function needs domain definition really, we don't need to > pass the whole domain object. This saves couple of dereferences > and characters esp. in more checks to come. > > Signed-off-by: Michal Privoznik > --- > src/qemu/qemu_firmware.c | 12 ++-- > src/qemu/qemu_firmware.h | 2 +- > src/qemu/qemu_process.c | 2 +- > 3 files changed, 8 insertions(+), 8 deletions(-) > Reviewed-by: Cole Robinson - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] remote_daemon: Log host boot time
On 12/17/19 8:25 AM, Michal Privoznik wrote:
> This is not strictly needed, but it makes sure we initialize the
> @bootTime global variable. Thing is, in order to validate XATTRs
> and prune those set in some previous runs of the host, a
> timestamp is recorded in XATTRs. The host boot time was unique
> enough so it was chosen as the timestamp value. And to avoid
> querying and parsing /proc/uptime every time, the query function
> does that only once and stores the boot time in a global
> variable. However, the only time the query function is called is
> in a child process that does lock files and changes seclabels. So
> effectively, we are doing exactly what we wanted to prevent from
> happening.
>
> The fix is simple, call the query function whilst the daemon is
> initializing.
>
> Signed-off-by: Michal Privoznik
> ---
>
> Since this will be used by security driver only, I was tempted to put
> this somewhere under src/security/, but especially because of split
> daemon I didn't. Placing this into remote_daemon.c makes sure all
> sub-daemons will have the variable initialized and won't suffer the
> problem.
>
> src/remote/remote_daemon.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> index b400b1dd10..5debb6ce97 100644
> --- a/src/remote/remote_daemon.c
> +++ b/src/remote/remote_daemon.c
> @@ -57,6 +57,7 @@
> #include "virgettext.h"
> #include "util/virnetdevopenvswitch.h"
> #include "virsystemd.h"
> +#include "virhostuptime.h"
>
> #include "driver.h"
>
> @@ -1020,6 +1021,7 @@ int main(int argc, char **argv) {
> bool implicit_conf = false;
> char *run_dir = NULL;
> mode_t old_umask;
> +unsigned long long bootTime;
>
> struct option opts[] = {
> { "verbose", no_argument, , 'v'},
> @@ -1151,6 +1153,12 @@ int main(int argc, char **argv) {
> exit(EXIT_FAILURE);
> }
>
> +if (virHostGetBootTime() < 0) {
> +VIR_DEBUG("Unable to get host boot time");
> +} else {
> +VIR_DEBUG("host boot time: %lld", bootTime);
> +}
> +
Please add a comment that this is initializing some global state that we
need elsewhere, because otherwise it won't be obvious why this is here.
With that:
Reviewed-by: Cole Robinson
Better IMO would be have an explicit virHostBootTimeInit function, and
any usage of GetBootTime would fail if the init hasn't been called yet.
It would make this case more self descriptive, and make sure any new
users aren't doing it wrong
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Remove VIR_FILE_*_SEPARATOR_*
On 12/18/19 3:17 PM, Fabiano Fidêncio wrote: > Let's use the defines provided by GLib, instead of keeping our own. > > Fabiano Fidêncio (2): > util: Use G_DIR_SEPARATOR instead of VIR_FILE_DIR_SEPARATOR > util: Remove VIR_FILE_*_SEPARATOR* > > src/util/virfile.c | 2 +- > src/util/virfile.h | 19 --- > 2 files changed, 1 insertion(+), 20 deletions(-) > I'm guessing this series is based on your previous virFile removals. An independent series should be based on master, otherwise 'git am' gets confused about missing ancestors which makes it harder to test+review. I applied manually though, so: Reviewed-by: Cole Robinson Possible follow up: virFileRemoveLastComponent only has one user, and it may be pretty close to g_path_get_dirname(), so that can possibly be dropped. Might want to check the glib implementation though - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] Remove unused virFile*() functions
On 12/18/19 2:59 PM, Fabiano Fidêncio wrote: > - virFileSkipRoot() is no longer used since faf2d811f3e9a4; > - virFileIsAbsPath() is no longer used since faf2d811f3e9a4; > - VIR_FILE_IS_DIR_SEPARATOR is no longer used since virFileIsAbsPath has > been dropped; > > Fabiano Fidêncio (3): > util: Remove virFileSkipRoot() > util: Remove virFileIsAbsPath() > util: Remove VIR_FILE_IS_DIR_SEPARATOR > > src/libvirt_private.syms | 2 -- > src/util/virfile.c | 73 > src/util/virfile.h | 4 --- > 3 files changed, 79 deletions(-) > Reviewed-by: Cole Robinson - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] Remove unused virFile*() functions
On 12/18/19 4:59 PM, Fabiano Fidêncio wrote: - virFileSkipRoot() is no longer used since faf2d811f3e9a4; - virFileIsAbsPath() is no longer used since faf2d811f3e9a4; - VIR_FILE_IS_DIR_SEPARATOR is no longer used since virFileIsAbsPath has been dropped; Fabiano Fidêncio (3): util: Remove virFileSkipRoot() util: Remove virFileIsAbsPath() util: Remove VIR_FILE_IS_DIR_SEPARATOR src/libvirt_private.syms | 2 -- src/util/virfile.c | 73 src/util/virfile.h | 4 --- 3 files changed, 79 deletions(-) All patches: Reviewed-by: Daniel Henrique Barboza -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] Remove VIR_FILE_*_SEPARATOR_*
Let's use the defines provided by GLib, instead of keeping our own. Fabiano Fidêncio (2): util: Use G_DIR_SEPARATOR instead of VIR_FILE_DIR_SEPARATOR util: Remove VIR_FILE_*_SEPARATOR* src/util/virfile.c | 2 +- src/util/virfile.h | 19 --- 2 files changed, 1 insertion(+), 20 deletions(-) -- 2.23.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] util: Remove VIR_FILE_*_SEPARATOR*
None of those are used and we should prefer using the ones provided by GLib, as G_DIR_SEPARATOR, G_DIR_SEPARATOR_S, G_SEARCHPATH_SEPARATOR, and G_SEARCHPATH_SEPARATOR_S. Signed-off-by: Fabiano Fidêncio --- src/util/virfile.h | 19 --- 1 file changed, 19 deletions(-) diff --git a/src/util/virfile.h b/src/util/virfile.h index 3fd1795813..264b12c03d 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -278,25 +278,6 @@ char *virFileBuildPath(const char *dir, const char *ext) G_GNUC_WARN_UNUSED_RESULT; -#ifdef WIN32 -/* On Win32, the canonical directory separator is the backslash, and - * the search path separator is the semicolon. Note that also the - * (forward) slash works as directory separator. - */ -# define VIR_FILE_DIR_SEPARATOR '\\' -# define VIR_FILE_DIR_SEPARATOR_S "\\" -# define VIR_FILE_PATH_SEPARATOR ';' -# define VIR_FILE_PATH_SEPARATOR_S ";" - -#else /* !WIN32 */ - -# define VIR_FILE_DIR_SEPARATOR '/' -# define VIR_FILE_DIR_SEPARATOR_S "/" -# define VIR_FILE_PATH_SEPARATOR ':' -# define VIR_FILE_PATH_SEPARATOR_S ":" - -#endif /* !WIN32 */ - int virFileAbsPath(const char *path, char **abspath) G_GNUC_WARN_UNUSED_RESULT; void virFileRemoveLastComponent(char *path); -- 2.23.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] util: Use G_DIR_SEPARATOR instead of VIR_FILE_DIR_SEPARATOR
Signed-off-by: Fabiano Fidêncio
---
src/util/virfile.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index b72d18b3d2..0f0d607c59 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3278,7 +3278,7 @@ virFileRemoveLastComponent(char *path)
{
char *tmp;
-if ((tmp = strrchr(path, VIR_FILE_DIR_SEPARATOR)))
+if ((tmp = strrchr(path, G_DIR_SEPARATOR)))
tmp[1] = '\0';
else
path[0] = '\0';
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] util: Remove VIR_FILE_IS_DIR_SEPARATOR
The define is not used since virFileIsAbsPath() has been dropped. Signed-off-by: Fabiano Fidêncio --- src/util/virfile.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/util/virfile.h b/src/util/virfile.h index 550d06ce94..3fd1795813 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -285,7 +285,6 @@ char *virFileBuildPath(const char *dir, */ # define VIR_FILE_DIR_SEPARATOR '\\' # define VIR_FILE_DIR_SEPARATOR_S "\\" -# define VIR_FILE_IS_DIR_SEPARATOR(c) ((c) == VIR_FILE_DIR_SEPARATOR || (c) == '/') # define VIR_FILE_PATH_SEPARATOR ';' # define VIR_FILE_PATH_SEPARATOR_S ";" @@ -293,7 +292,6 @@ char *virFileBuildPath(const char *dir, # define VIR_FILE_DIR_SEPARATOR '/' # define VIR_FILE_DIR_SEPARATOR_S "/" -# define VIR_FILE_IS_DIR_SEPARATOR(c) ((c) == VIR_FILE_DIR_SEPARATOR) # define VIR_FILE_PATH_SEPARATOR ':' # define VIR_FILE_PATH_SEPARATOR_S ":" -- 2.23.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/3] Remove unused virFile*() functions
- virFileSkipRoot() is no longer used since faf2d811f3e9a4; - virFileIsAbsPath() is no longer used since faf2d811f3e9a4; - VIR_FILE_IS_DIR_SEPARATOR is no longer used since virFileIsAbsPath has been dropped; Fabiano Fidêncio (3): util: Remove virFileSkipRoot() util: Remove virFileIsAbsPath() util: Remove VIR_FILE_IS_DIR_SEPARATOR src/libvirt_private.syms | 2 -- src/util/virfile.c | 73 src/util/virfile.h | 4 --- 3 files changed, 79 deletions(-) -- 2.23.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] util: Remove virFileIsAbsPath()
The function is no longer used since commit faf2d811f3e9a4.
Signed-off-by: Fabiano Fidêncio
---
src/libvirt_private.syms | 1 -
src/util/virfile.c | 19 ---
src/util/virfile.h | 1 -
3 files changed, 21 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 797c15bb42..a7b1ef23bc 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1994,7 +1994,6 @@ virFileGetMountSubtree;
virFileGetXAttr;
virFileGetXAttrQuiet;
virFileInData;
-virFileIsAbsPath;
virFileIsCDROM;
virFileIsDir;
virFileIsExecutable;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 624bd9e3d0..b72d18b3d2 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3178,25 +3178,6 @@ virFileOpenTty(int *ttymaster G_GNUC_UNUSED,
}
#endif /* WIN32 */
-bool
-virFileIsAbsPath(const char *path)
-{
-if (!path)
-return false;
-
-if (VIR_FILE_IS_DIR_SEPARATOR(path[0]))
-return true;
-
-#ifdef WIN32
-if (g_ascii_isalpha(path[0]) &&
-path[1] == ':' &&
-VIR_FILE_IS_DIR_SEPARATOR(path[2]))
-return true;
-#endif
-
-return false;
-}
-
/*
* Creates an absolute path for a potentially relative path.
* Return 0 if the path was not relative, or on success.
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 80641f763a..550d06ce94 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -299,7 +299,6 @@ char *virFileBuildPath(const char *dir,
#endif /* !WIN32 */
-bool virFileIsAbsPath(const char *path);
int virFileAbsPath(const char *path,
char **abspath) G_GNUC_WARN_UNUSED_RESULT;
void virFileRemoveLastComponent(char *path);
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] util: Remove virFileSkipRoot()
The function is no longer used since commit faf2d811f3e9a4.
Signed-off-by: Fabiano Fidêncio
---
src/libvirt_private.syms | 1 -
src/util/virfile.c | 54
src/util/virfile.h | 1 -
3 files changed, 56 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9852f5d4a6..797c15bb42 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2038,7 +2038,6 @@ virFileSanitizePath;
virFileSetACLs;
virFileSetupDev;
virFileSetXAttr;
-virFileSkipRoot;
virFileTouch;
virFileUnlock;
virFileUpdatePerm;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 4fd865dd83..624bd9e3d0 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3197,60 +3197,6 @@ virFileIsAbsPath(const char *path)
return false;
}
-
-const char *
-virFileSkipRoot(const char *path)
-{
-#ifdef WIN32
-/* Skip \\server\share or //server/share */
-if (VIR_FILE_IS_DIR_SEPARATOR(path[0]) &&
-VIR_FILE_IS_DIR_SEPARATOR(path[1]) &&
-path[2] &&
-!VIR_FILE_IS_DIR_SEPARATOR(path[2]))
-{
-const char *p = strchr(path + 2, VIR_FILE_DIR_SEPARATOR);
-const char *q = strchr(path + 2, '/');
-
-if (p == NULL || (q != NULL && q < p))
-p = q;
-
-if (p && p > path + 2 && p[1]) {
-path = p + 1;
-
-while (path[0] &&
- !VIR_FILE_IS_DIR_SEPARATOR(path[0]))
-path++;
-
-/* Possibly skip a backslash after the share name */
-if (VIR_FILE_IS_DIR_SEPARATOR(path[0]))
-path++;
-
-return path;
-}
-}
-#endif
-
-/* Skip initial slashes */
-if (VIR_FILE_IS_DIR_SEPARATOR(path[0])) {
-while (VIR_FILE_IS_DIR_SEPARATOR(path[0]))
-path++;
-
-return path;
-}
-
-#ifdef WIN32
-/* Skip X:\ */
-if (g_ascii_isalpha(path[0]) &&
-path[1] == ':' &&
-VIR_FILE_IS_DIR_SEPARATOR(path[2]))
-return path + 3;
-#endif
-
-return path;
-}
-
-
-
/*
* Creates an absolute path for a potentially relative path.
* Return 0 if the path was not relative, or on success.
diff --git a/src/util/virfile.h b/src/util/virfile.h
index bcae40ee06..80641f763a 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -302,7 +302,6 @@ char *virFileBuildPath(const char *dir,
bool virFileIsAbsPath(const char *path);
int virFileAbsPath(const char *path,
char **abspath) G_GNUC_WARN_UNUSED_RESULT;
-const char *virFileSkipRoot(const char *path);
void virFileRemoveLastComponent(char *path);
int virFileOpenTty(int *ttymaster,
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] docs: Only distribute sanlock manpage if WITH_SANLOCK
On Wed, Dec 18, 2019 at 8:48 PM Cole Robinson wrote: > > This fixes mingw-libvirt RPM build > > Signed-off-by: Cole Robinson > --- > docs/Makefile.am | 9 ++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/docs/Makefile.am b/docs/Makefile.am > index eb8de80b9c..66f164a1ac 100644 > --- a/docs/Makefile.am > +++ b/docs/Makefile.am > @@ -214,9 +214,7 @@ manpages7_rst = \ >$(KEYCODES:%=manpages/virkeycode-%.rst) \ >$(KEYNAMES:%=manpages/virkeyname-%.rst) \ >$(NULL) > -manpages8_rst = \ > - manpages/virt-sanlock-cleanup.rst \ > - $(NULL) > +manpages8_rst = $(NULL) > manpages_rst += \ >$(manpages1_rst) \ >$(manpages7_rst) \ > @@ -245,6 +243,11 @@ if WITH_LOGIN_SHELL > else ! WITH_LOGIN_SHELL >manpages_rst += manpages/virt-login-shell.rst > endif ! WITH_LOGIN_SHELL > +if WITH_SANLOCK > + manpages8_rst += manpages/virt-sanlock-cleanup.rst > +else ! WITH_SANLOCK > + manpages_rst += manpages/virt-sanlock-cleanup.rst > +endif ! WITH_SANLOCK > manpages_rst_html_in = \ >$(manpages_rst:%.rst=%.html.in) > manpages_html = \ > -- > 2.23.0 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > Reviewed-by: Fabiano Fidêncio -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Remove phyp driver
[I'm CCing Eduardo, who worked a lot on this driver in the past. Maybe
he knows if this hypervisor is still alive.]
On 12/18/19 4:43 PM, Cole Robinson wrote:
> The phyp driver was added in 2009 and does not appear to have had any
> real feature change since 2011. There's virtually no evidence online
> of users actually using it. IMO it's time to kill it.
>
> Signed-off-by: Cole Robinson
> ---
> I raised this in 3.5 years ago:
> https://www.redhat.com/archives/libvir-list/2016-April/msg01060.html
>
> Not much on phyp/ side has changed since then, except dozens of dev
> patches transitioning the code forward.
>
> That mail also mentions xenapi and hyperv. hyperv saw signs of life
> afterwards and is still around. xenapi has been removed, along with uml.
>
> Considering the amount of code transitions we are currently undergoing
> (gnulib, glib, memory auto cleanup, etc), phyp/ will probably have an
> uptick of dev energy in the medium term. Let's bite the bullet and
> remove it!
>
> docs/aclpolkit.html.in|4 -
> docs/api.html.in |2 +-
> docs/drivers.html.in |1 -
> docs/drvphyp.html.in | 50 -
> docs/schemas/capability.rng |3 +-
> docs/schemas/domaincommon.rng |2 +-
> libvirt.spec.in | 11 +-
> m4/virt-driver-phyp.m4| 48 -
> mingw-libvirt.spec.in |7 -
> po/POTFILES.in|1 -
> src/Makefile.am |1 -
> src/README|1 -
> src/libvirt.c | 10 -
> src/phyp/Makefile.inc.am | 21 -
> src/phyp/phyp_driver.c| 3739 -
> src/phyp/phyp_driver.h| 24 -
> 16 files changed, 4 insertions(+), 3921 deletions(-)
> delete mode 100644 docs/drvphyp.html.in
> delete mode 100644 m4/virt-driver-phyp.m4
> delete mode 100644 src/phyp/Makefile.inc.am
> delete mode 100644 src/phyp/phyp_driver.c
> delete mode 100644 src/phyp/phyp_driver.h
Apart from what you proposed to squash in, I'd add/change the following:
diff --git i/include/libvirt/virterror.h w/include/libvirt/virterror.h
index 685e171235..7c7e5fd145 100644
--- i/include/libvirt/virterror.h
+++ w/include/libvirt/virterror.h
@@ -84,7 +84,7 @@ typedef enum {
VIR_FROM_ONE = 27, /* The OpenNebula driver no longer exists.
Retained for ABI/API compat only */
VIR_FROM_ESX = 28, /* Error from ESX driver */
-VIR_FROM_PHYP = 29, /* Error from IBM power hypervisor */
+VIR_FROM_PHYP = 29, /* Error from IBM power hypervisor;
unused since 6.0.0 */
VIR_FROM_SECRET = 30, /* Error from secret storage */
VIR_FROM_CPU = 31, /* Error from CPU driver */
diff --git i/src/util/virhostcpu.c w/src/util/virhostcpu.c
index 22102f2c75..f17a888638 100644
--- i/src/util/virhostcpu.c
+++ w/src/util/virhostcpu.c
@@ -650,7 +650,7 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo,
* If the user tampers the cpu online/offline states using chcpu or
other
* means, then it is an unsupported configuration for kvm.
* The code below tries to keep in mind
- * - when the libvirtd is run inside a KVM guest or Phyp based guest.
+ * - when the libvirtd is run inside a KVM guest.
* - Or on the kvm host where user manually tampers the cpu states to
*offline/online randomly.
* On hosts other than POWER this will be 0, in which case a simpler
@@ -1133,7 +1133,7 @@ virHostCPUGetThreadsPerSubcore(virArch arch)
goto out;
}
-/* For Phyp and KVM based guests the ioctl for KVM_CAP_PPC_SMT
+/* For KVM based guests the ioctl for KVM_CAP_PPC_SMT
* returns zero and both primary and secondary threads will be
* online */
threads_per_subcore = ioctl(kvmfd,
And also, I'd mention in news.xml that the driver was removed.
Reviewed-by: Michal Privoznik
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] docs: Only distribute sanlock manpage if WITH_SANLOCK
This fixes mingw-libvirt RPM build Signed-off-by: Cole Robinson --- docs/Makefile.am | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/docs/Makefile.am b/docs/Makefile.am index eb8de80b9c..66f164a1ac 100644 --- a/docs/Makefile.am +++ b/docs/Makefile.am @@ -214,9 +214,7 @@ manpages7_rst = \ $(KEYCODES:%=manpages/virkeycode-%.rst) \ $(KEYNAMES:%=manpages/virkeyname-%.rst) \ $(NULL) -manpages8_rst = \ - manpages/virt-sanlock-cleanup.rst \ - $(NULL) +manpages8_rst = $(NULL) manpages_rst += \ $(manpages1_rst) \ $(manpages7_rst) \ @@ -245,6 +243,11 @@ if WITH_LOGIN_SHELL else ! WITH_LOGIN_SHELL manpages_rst += manpages/virt-login-shell.rst endif ! WITH_LOGIN_SHELL +if WITH_SANLOCK + manpages8_rst += manpages/virt-sanlock-cleanup.rst +else ! WITH_SANLOCK + manpages_rst += manpages/virt-sanlock-cleanup.rst +endif ! WITH_SANLOCK manpages_rst_html_in = \ $(manpages_rst:%.rst=%.html.in) manpages_html = \ -- 2.23.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: Simplify Windows version of virGetUserDirectoryByUID()
On 12/18/19 12:53 PM, Fabiano Fidêncio wrote:
> Let's just use the plain g_get_home_dir(), from GLib, instead of
> maintaining a code adapted from the GLib's one.
>
> Signed-off-by: Fabiano Fidêncio
> ---
> src/util/virutil.c | 92 +-
> 1 file changed, 1 insertion(+), 91 deletions(-)
>
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index a28feb3daa..4075047cf9 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -1070,100 +1070,10 @@ virDoesGroupExist(const char *name G_GNUC_UNUSED)
> }
>
> # ifdef WIN32
> -/* These methods are adapted from GLib2 under terms of LGPLv2+ */
> -static int
> -virGetWin32SpecialFolder(int csidl, char **path)
> -{
> -char buf[MAX_PATH+1];
> -LPITEMIDLIST pidl = NULL;
> -int ret = 0;
> -
> -*path = NULL;
> -
> -if (SHGetSpecialFolderLocation(NULL, csidl, ) == S_OK) {
> -if (SHGetPathFromIDList(pidl, buf))
> -*path = g_strdup(buf);
> -CoTaskMemFree(pidl);
> -}
> -return ret;
> -}
> -
> -static int
> -virGetWin32DirectoryRoot(char **path)
> -{
> -char windowsdir[MAX_PATH];
> -
> -*path = NULL;
> -
> -if (GetWindowsDirectory(windowsdir, G_N_ELEMENTS(windowsdir))) {
> -const char *tmp;
> -/* Usually X:\Windows, but in terminal server environments
> - * might be an UNC path, AFAIK.
> - */
> -tmp = virFileSkipRoot(windowsdir);
> -if (VIR_FILE_IS_DIR_SEPARATOR(tmp[-1]) &&
> -tmp[-2] != ':')
> -tmp--;
> -
> -windowsdir[tmp - windowsdir] = '\0';
> -} else {
> -strcpy(windowsdir, "C:\\");
> -}
> -
> -*path = g_strdup(windowsdir);
> -return 0;
> -}
> -
> -
> -
> char *
> virGetUserDirectoryByUID(uid_t uid G_GNUC_UNUSED)
> {
> -/* Since Windows lacks setuid binaries, and since we already fake
> - * geteuid(), we can safely assume that this is only called when
> - * querying about the current user */
Keep this comment, it explains why this function does not abide the
passed in uid argument. With that:
Reviewed-by: Cole Robinson
After this, virFileSkipRoot and virFileIsAbsPath are no longer used, and
after that I think VIR_FILE_IS_DIR_SEPARATOR, maybe check for others.
Should be a follow up patch though
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v6 0/4] PCI hostdev partial assignment support
On 12/17/19 3:35 PM, Daniel Henrique Barboza wrote: > changes from previous version 5 [1]: > - changes in the commit message of patch 1 and the > documentation included in patches 3 and 4, all of them > suggested/hinted by Alex Williamson. > > > Daniel Henrique Barboza (4): > Introducing new address type='unassigned' for PCI hostdevs > qemu: handle unassigned PCI hostdevs in command line > formatdomain.html.in: document > news.xml: add address type='unassigned' entry > > > [1] https://www.redhat.com/archives/libvir-list/2019-December/msg01097.html > > docs/formatdomain.html.in | 10 > docs/news.xml | 14 + > docs/schemas/domaincommon.rng | 5 ++ > src/conf/device_conf.c| 2 + > src/conf/device_conf.h| 1 + > src/conf/domain_conf.c| 7 ++- > src/qemu/qemu_command.c | 5 ++ > src/qemu/qemu_domain.c| 1 + > src/qemu/qemu_domain_address.c| 5 ++ > .../hostdev-pci-address-unassigned.args | 31 ++ > .../hostdev-pci-address-unassigned.xml| 42 ++ > tests/qemuxml2argvtest.c | 4 ++ > .../hostdev-pci-address-unassigned.xml| 58 +++ > tests/qemuxml2xmltest.c | 1 + > 14 files changed, 185 insertions(+), 1 deletion(-) > create mode 100644 tests/qemuxml2argvdata/hostdev-pci-address-unassigned.args > create mode 100644 tests/qemuxml2argvdata/hostdev-pci-address-unassigned.xml > create mode 100644 > tests/qemuxml2xmloutdata/hostdev-pci-address-unassigned.xml > Reviewed-by: Cole Robinson and pushed - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/4] qemu: move qemuBuildSmartcardCommandLine validation to qemu_domain.c
On 12/17/19 7:36 PM, Daniel Henrique Barboza wrote:
> Move smartcard validation being done by qemuBuildSmartcardCommandLine()
> to the existing qemuDomainSmartcardDefValidate() function. This
> function is called by qemuDomainDeviceDefValidate(), allowing smartcard
> validation in domain define time.
>
> Tests were adapted to consider the new caps being needed in
> this earlier stage.
>
> Signed-off-by: Daniel Henrique Barboza
> ---
> src/qemu/qemu_command.c | 24
> src/qemu/qemu_domain.c | 33 +
> tests/qemuxml2xmltest.c | 16 +---
> 3 files changed, 42 insertions(+), 31 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index b3f069d5d4..67f7caf9c6 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -8275,24 +8275,10 @@ qemuBuildSmartcardCommandLine(virLogManagerPtr
> logManager,
>
> switch (smartcard->type) {
> case VIR_DOMAIN_SMARTCARD_TYPE_HOST:
> -if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_CCID_EMULATED)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("this QEMU binary lacks smartcard host "
> - "mode support"));
> -return -1;
> -}
> -
> virBufferAddLit(, "ccid-card-emulated,backend=nss-emulated");
> break;
>
> case VIR_DOMAIN_SMARTCARD_TYPE_HOST_CERTIFICATES:
> -if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_CCID_EMULATED)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("this QEMU binary lacks smartcard host "
> - "mode support"));
> -return -1;
> -}
> -
> virBufferAddLit(, "ccid-card-emulated,backend=certificates");
> for (i = 0; i < VIR_DOMAIN_SMARTCARD_NUM_CERTIFICATES; i++) {
> virBufferAsprintf(, ",cert%zu=", i + 1);
> @@ -8308,13 +8294,6 @@ qemuBuildSmartcardCommandLine(virLogManagerPtr
> logManager,
> break;
>
> case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
> -if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_CCID_PASSTHRU)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("this QEMU binary lacks smartcard "
> - "passthrough mode support"));
> -return -1;
> -}
> -
> if (!(devstr = qemuBuildChrChardevStr(logManager, secManager,
>cmd, cfg, def,
>smartcard->data.passthru,
> @@ -8330,9 +8309,6 @@ qemuBuildSmartcardCommandLine(virLogManagerPtr
> logManager,
> break;
>
> default:
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("unexpected smartcard type %d"),
> - smartcard->type);
> return -1;
> }
>
We want a virReportEnumRangeError here too. I fixed that and pushed this
series
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] util: Simplify Windows version of virGetUserDirectoryByUID()
Let's just use the plain g_get_home_dir(), from GLib, instead of
maintaining a code adapted from the GLib's one.
Signed-off-by: Fabiano Fidêncio
---
src/util/virutil.c | 92 +-
1 file changed, 1 insertion(+), 91 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index a28feb3daa..4075047cf9 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1070,100 +1070,10 @@ virDoesGroupExist(const char *name G_GNUC_UNUSED)
}
# ifdef WIN32
-/* These methods are adapted from GLib2 under terms of LGPLv2+ */
-static int
-virGetWin32SpecialFolder(int csidl, char **path)
-{
-char buf[MAX_PATH+1];
-LPITEMIDLIST pidl = NULL;
-int ret = 0;
-
-*path = NULL;
-
-if (SHGetSpecialFolderLocation(NULL, csidl, ) == S_OK) {
-if (SHGetPathFromIDList(pidl, buf))
-*path = g_strdup(buf);
-CoTaskMemFree(pidl);
-}
-return ret;
-}
-
-static int
-virGetWin32DirectoryRoot(char **path)
-{
-char windowsdir[MAX_PATH];
-
-*path = NULL;
-
-if (GetWindowsDirectory(windowsdir, G_N_ELEMENTS(windowsdir))) {
-const char *tmp;
-/* Usually X:\Windows, but in terminal server environments
- * might be an UNC path, AFAIK.
- */
-tmp = virFileSkipRoot(windowsdir);
-if (VIR_FILE_IS_DIR_SEPARATOR(tmp[-1]) &&
-tmp[-2] != ':')
-tmp--;
-
-windowsdir[tmp - windowsdir] = '\0';
-} else {
-strcpy(windowsdir, "C:\\");
-}
-
-*path = g_strdup(windowsdir);
-return 0;
-}
-
-
-
char *
virGetUserDirectoryByUID(uid_t uid G_GNUC_UNUSED)
{
-/* Since Windows lacks setuid binaries, and since we already fake
- * geteuid(), we can safely assume that this is only called when
- * querying about the current user */
-const char *dir;
-char *ret;
-
-dir = getenv("HOME");
-
-/* Only believe HOME if it is an absolute path and exists */
-if (dir) {
-if (!virFileIsAbsPath(dir) ||
-!virFileExists(dir))
-dir = NULL;
-}
-
-/* In case HOME is Unix-style (it happens), convert it to
- * Windows style.
- */
-if (dir) {
-char *p;
-while ((p = strchr(dir, '/')) != NULL)
-*p = '\\';
-}
-
-if (!dir)
-/* USERPROFILE is probably the closest equivalent to $HOME? */
-dir = getenv("USERPROFILE");
-
-ret = g_strdup(dir);
-
-if (!ret &&
-virGetWin32SpecialFolder(CSIDL_PROFILE, ) < 0)
-return NULL;
-
-if (!ret &&
-virGetWin32DirectoryRoot() < 0)
-return NULL;
-
-if (!ret) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Unable to determine home directory"));
-return NULL;
-}
-
-return ret;
+return g_strdup(g_get_home_dir());
}
char *
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 0/4] Rewrite virGetUser*Directory() functions using g_get_*_dir()
On 12/18/19 11:26 AM, Fabiano Fidêncio wrote: > By rewriting virGetUser*Directory() functions using g_get_*_dir() > functions allows us to drop all the different implementations we > keep, as GLib already takes care of those for us. > > Changes since v3: > https://www.redhat.com/archives/libvir-list/2019-December/msg01123.html > - Used g_build_filename() instead of g_strdup_printf(); > > Changes since v2: > https://www.redhat.com/archives/libvir-list/2019-December/msg01064.html > - Addressed comments made by Cole about: > - virGetUserDirectory() should return $HOME; > - virGetUser*Directory() don't append "libvirt" to the returned path > on Windows; > > Changes since v1: > https://www.redhat.com/archives/libvir-list/2019-December/msg01055.html > - Don't check for the return of g_get_*_dir(), as it cannot be NULL; > > Fabiano Fidêncio (4): > util: Rewrite virGetUserDirectory() using g_get_home_dir() > util: Rewrite virGetUserConfigDirectory() using > g_get_user_config_dir() > util: Rewrite virGetUserCacheDirectory() using g_get_user_cache_dir() > util: Rewrite virGetUserRuntimeDirectory() using > g_get_user_runtime_dir() > > src/util/virutil.c | 137 ++--- > 1 file changed, 31 insertions(+), 106 deletions(-) > Reviewed-by: Cole Robinson - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()
On 12/18/19 11:12 AM, Fabiano Fidêncio wrote:
> On Wed, Dec 18, 2019 at 5:08 PM Daniel P. Berrangé
> wrote:
>>
>> On Wed, Dec 18, 2019 at 10:53:41AM -0500, Cole Robinson wrote:
>>> On 12/18/19 5:34 AM, Fabiano Fidêncio wrote:
On Wed, Dec 18, 2019 at 11:19 AM Marc Hartmayer
wrote:
>
> On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio
> wrote:
>> Signed-off-by: Fabiano Fidêncio
>> ---
>> src/util/virutil.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/util/virutil.c b/src/util/virutil.c
>> index ed1f696e37..8c255abd7f 100644
>> --- a/src/util/virutil.c
>> +++ b/src/util/virutil.c
>> @@ -582,7 +582,7 @@ virGetHostnameQuiet(void)
>> char *
>> virGetUserDirectory(void)
>> {
>> -return virGetUserDirectoryByUID(geteuid());
>> +return g_strdup_printf("%s/libvirt", g_get_home_dir());
>
>
> I would suggest to use 'g_build_path' instead of 'g_strdup_printf'.
> E.g.
>
> g_build_path(G_DIR_SEPARATOR_S, g_get_home_dir(), "libvirt");
>>>
>>> Good catch
>>>
>>> There's also g_build_filename which sounds simpler. Any reason for one
>>> over the other?
>>
>> Based on the docs g_build_filename looks better, as it keeps
>> '/' vs '\' consistent on windows. Aside from that, their
>> internal impl is basically the same.
>
> Okay, I'll use g_build_filename() then.
> I wonder whether consistently using g_build_filename() wherever it's
> possible should be added to the BiteSizedTasks as well.
Certainly in places where we have windows specific code.
I feel like we must get path separator stuff wrong already in code that
is compiled for windows (netclient at least), but maybe there's some
gnulib magic that is handling path conversion for us?
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 2/4] util: Rewrite virGetUserConfigDirectory() using g_get_user_config_dir()
Signed-off-by: Fabiano Fidêncio
---
src/util/virutil.c | 39 ++-
1 file changed, 10 insertions(+), 29 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 7008c3119c..9d98b0051a 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -586,6 +586,16 @@ virGetUserDirectory(void)
}
+char *virGetUserConfigDirectory(void)
+{
+#ifdef WIN32
+return g_strdup(g_get_user_config_dir());
+#else
+return g_build_filename(g_get_user_config_dir(), "libvirt", NULL);
+#endif
+}
+
+
#ifdef HAVE_GETPWUID_R
/* Look up fields from the user database for the given user. On
* error, set errno, report the error if not instructed otherwise via @quiet,
@@ -754,11 +764,6 @@ static char *virGetXDGDirectory(const char *xdgenvname,
const char *xdgdefdir)
return ret;
}
-char *virGetUserConfigDirectory(void)
-{
-return virGetXDGDirectory("XDG_CONFIG_HOME", ".config");
-}
-
char *virGetUserCacheDirectory(void)
{
return virGetXDGDirectory("XDG_CACHE_HOME", ".cache");
@@ -1187,21 +1192,6 @@ virGetUserShell(uid_t uid G_GNUC_UNUSED)
return NULL;
}
-char *
-virGetUserConfigDirectory(void)
-{
-char *ret;
-if (virGetWin32SpecialFolder(CSIDL_LOCAL_APPDATA, ) < 0)
-return NULL;
-
-if (!ret) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Unable to determine config directory"));
-return NULL;
-}
-return ret;
-}
-
char *
virGetUserCacheDirectory(void)
{
@@ -1242,15 +1232,6 @@ virGetUserShell(uid_t uid G_GNUC_UNUSED)
return NULL;
}
-char *
-virGetUserConfigDirectory(void)
-{
-virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("virGetUserConfigDirectory is not available"));
-
-return NULL;
-}
-
char *
virGetUserCacheDirectory(void)
{
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 3/4] util: Rewrite virGetUserCacheDirectory() using g_get_user_cache_dir()
Signed-off-by: Fabiano Fidêncio
---
src/util/virutil.c | 57 --
1 file changed, 10 insertions(+), 47 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 9d98b0051a..db9378fa57 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -596,6 +596,16 @@ char *virGetUserConfigDirectory(void)
}
+char *virGetUserCacheDirectory(void)
+{
+#ifdef WIN32
+return g_strdup(g_get_user_cache_dir());
+#else
+return g_build_filename(g_get_user_cache_dir(), "libvirt", NULL);
+#endif
+}
+
+
#ifdef HAVE_GETPWUID_R
/* Look up fields from the user database for the given user. On
* error, set errno, report the error if not instructed otherwise via @quiet,
@@ -746,29 +756,6 @@ char *virGetUserShell(uid_t uid)
}
-static char *virGetXDGDirectory(const char *xdgenvname, const char *xdgdefdir)
-{
-const char *path = getenv(xdgenvname);
-char *ret = NULL;
-char *home = NULL;
-
-if (path && path[0]) {
-ret = g_strdup_printf("%s/libvirt", path);
-} else {
-home = virGetUserDirectory();
-if (home)
-ret = g_strdup_printf("%s/%s/libvirt", home, xdgdefdir);
-}
-
-VIR_FREE(home);
-return ret;
-}
-
-char *virGetUserCacheDirectory(void)
-{
-return virGetXDGDirectory("XDG_CACHE_HOME", ".cache");
-}
-
char *virGetUserRuntimeDirectory(void)
{
const char *path = getenv("XDG_RUNTIME_DIR");
@@ -1192,21 +1179,6 @@ virGetUserShell(uid_t uid G_GNUC_UNUSED)
return NULL;
}
-char *
-virGetUserCacheDirectory(void)
-{
-char *ret;
-if (virGetWin32SpecialFolder(CSIDL_INTERNET_CACHE, ) < 0)
-return NULL;
-
-if (!ret) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Unable to determine config directory"));
-return NULL;
-}
-return ret;
-}
-
char *
virGetUserRuntimeDirectory(void)
{
@@ -1232,15 +1204,6 @@ virGetUserShell(uid_t uid G_GNUC_UNUSED)
return NULL;
}
-char *
-virGetUserCacheDirectory(void)
-{
-virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("virGetUserCacheDirectory is not available"));
-
-return NULL;
-}
-
char *
virGetUserRuntimeDirectory(void)
{
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 4/4] util: Rewrite virGetUserRuntimeDirectory() using g_get_user_runtime_dir()
Signed-off-by: Fabiano Fidêncio
---
src/util/virutil.c | 39 ++-
1 file changed, 10 insertions(+), 29 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index db9378fa57..a28feb3daa 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -606,6 +606,16 @@ char *virGetUserCacheDirectory(void)
}
+char *virGetUserRuntimeDirectory(void)
+{
+#ifdef WIN32
+return g_strdup(g_get_user_runtime_dir());
+#else
+return g_build_filename(g_get_user_runtime_dir(), "libvirt", NULL);
+#endif
+}
+
+
#ifdef HAVE_GETPWUID_R
/* Look up fields from the user database for the given user. On
* error, set errno, report the error if not instructed otherwise via @quiet,
@@ -756,20 +766,6 @@ char *virGetUserShell(uid_t uid)
}
-char *virGetUserRuntimeDirectory(void)
-{
-const char *path = getenv("XDG_RUNTIME_DIR");
-
-if (!path || !path[0]) {
-return virGetUserCacheDirectory();
-} else {
-char *ret;
-
-ret = g_strdup_printf("%s/libvirt", path);
-return ret;
-}
-}
-
char *virGetUserName(uid_t uid)
{
char *ret;
@@ -1179,12 +1175,6 @@ virGetUserShell(uid_t uid G_GNUC_UNUSED)
return NULL;
}
-char *
-virGetUserRuntimeDirectory(void)
-{
-return virGetUserCacheDirectory();
-}
-
# else /* !HAVE_GETPWUID_R && !WIN32 */
char *
virGetUserDirectoryByUID(uid_t uid G_GNUC_UNUSED)
@@ -1203,15 +1193,6 @@ virGetUserShell(uid_t uid G_GNUC_UNUSED)
return NULL;
}
-
-char *
-virGetUserRuntimeDirectory(void)
-{
-virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("virGetUserRuntimeDirectory is not available"));
-
-return NULL;
-}
# endif /* ! HAVE_GETPWUID_R && ! WIN32 */
char *
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 0/4] Rewrite virGetUser*Directory() functions using g_get_*_dir()
By rewriting virGetUser*Directory() functions using g_get_*_dir() functions allows us to drop all the different implementations we keep, as GLib already takes care of those for us. Changes since v3: https://www.redhat.com/archives/libvir-list/2019-December/msg01123.html - Used g_build_filename() instead of g_strdup_printf(); Changes since v2: https://www.redhat.com/archives/libvir-list/2019-December/msg01064.html - Addressed comments made by Cole about: - virGetUserDirectory() should return $HOME; - virGetUser*Directory() don't append "libvirt" to the returned path on Windows; Changes since v1: https://www.redhat.com/archives/libvir-list/2019-December/msg01055.html - Don't check for the return of g_get_*_dir(), as it cannot be NULL; Fabiano Fidêncio (4): util: Rewrite virGetUserDirectory() using g_get_home_dir() util: Rewrite virGetUserConfigDirectory() using g_get_user_config_dir() util: Rewrite virGetUserCacheDirectory() using g_get_user_cache_dir() util: Rewrite virGetUserRuntimeDirectory() using g_get_user_runtime_dir() src/util/virutil.c | 137 ++--- 1 file changed, 31 insertions(+), 106 deletions(-) -- 2.23.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()
Signed-off-by: Fabiano Fidêncio
---
src/util/virutil.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index ed1f696e37..7008c3119c 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -582,7 +582,7 @@ virGetHostnameQuiet(void)
char *
virGetUserDirectory(void)
{
-return virGetUserDirectoryByUID(geteuid());
+return g_strdup(g_get_home_dir());
}
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()
On Wed, Dec 18, 2019 at 5:08 PM Daniel P. Berrangé wrote:
>
> On Wed, Dec 18, 2019 at 10:53:41AM -0500, Cole Robinson wrote:
> > On 12/18/19 5:34 AM, Fabiano Fidêncio wrote:
> > > On Wed, Dec 18, 2019 at 11:19 AM Marc Hartmayer
> > > wrote:
> > >>
> > >> On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio
> > >> wrote:
> > >>> Signed-off-by: Fabiano Fidêncio
> > >>> ---
> > >>> src/util/virutil.c | 2 +-
> > >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/src/util/virutil.c b/src/util/virutil.c
> > >>> index ed1f696e37..8c255abd7f 100644
> > >>> --- a/src/util/virutil.c
> > >>> +++ b/src/util/virutil.c
> > >>> @@ -582,7 +582,7 @@ virGetHostnameQuiet(void)
> > >>> char *
> > >>> virGetUserDirectory(void)
> > >>> {
> > >>> -return virGetUserDirectoryByUID(geteuid());
> > >>> +return g_strdup_printf("%s/libvirt", g_get_home_dir());
> > >>
> > >>
> > >> I would suggest to use 'g_build_path' instead of 'g_strdup_printf'.
> > >> E.g.
> > >>
> > >> g_build_path(G_DIR_SEPARATOR_S, g_get_home_dir(), "libvirt");
> > >
> >
> > Good catch
> >
> > There's also g_build_filename which sounds simpler. Any reason for one
> > over the other?
>
> Based on the docs g_build_filename looks better, as it keeps
> '/' vs '\' consistent on windows. Aside from that, their
> internal impl is basically the same.
Okay, I'll use g_build_filename() then.
I wonder whether consistently using g_build_filename() wherever it's
possible should be added to the BiteSizedTasks as well.
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Remove phyp driver
On 12/18/19 11:04 AM, Daniel Henrique Barboza wrote: > > > On 12/18/19 12:43 PM, Cole Robinson wrote: >> The phyp driver was added in 2009 and does not appear to have had any >> real feature change since 2011. There's virtually no evidence online >> of users actually using it. IMO it's time to kill it. >> >> Signed-off-by: Cole Robinson > > > I understand that this was a feature pushed by IBM years ago. I can't say > about the primary author, but as far as current IBM stance goes, +1 > for the removal. > > My suggestion would be to put the RFC link in the commit message of this > patch, for reference. > Thanks. I will amend the commit message to point to that thread, and this thread, if it's fully ACK'd. I'll wait for danpb's approval though Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Remove phyp driver
On 12/18/19 12:43 PM, Cole Robinson wrote:
The phyp driver was added in 2009 and does not appear to have had any
real feature change since 2011. There's virtually no evidence online
of users actually using it. IMO it's time to kill it.
Signed-off-by: Cole Robinson
I understand that this was a feature pushed by IBM years ago. I can't say
about the primary author, but as far as current IBM stance goes, +1
for the removal.
My suggestion would be to put the RFC link in the commit message of this
patch, for reference.
Thanks,
DHB
---
I raised this in 3.5 years ago:
https://www.redhat.com/archives/libvir-list/2016-April/msg01060.html
Not much on phyp/ side has changed since then, except dozens of dev
patches transitioning the code forward.
That mail also mentions xenapi and hyperv. hyperv saw signs of life
afterwards and is still around. xenapi has been removed, along with uml.
Considering the amount of code transitions we are currently undergoing
(gnulib, glib, memory auto cleanup, etc), phyp/ will probably have an
uptick of dev energy in the medium term. Let's bite the bullet and
remove it!
docs/aclpolkit.html.in|4 -
docs/api.html.in |2 +-
docs/drivers.html.in |1 -
docs/drvphyp.html.in | 50 -
docs/schemas/capability.rng |3 +-
docs/schemas/domaincommon.rng |2 +-
libvirt.spec.in | 11 +-
m4/virt-driver-phyp.m4| 48 -
mingw-libvirt.spec.in |7 -
po/POTFILES.in|1 -
src/Makefile.am |1 -
src/README|1 -
src/libvirt.c | 10 -
src/phyp/Makefile.inc.am | 21 -
src/phyp/phyp_driver.c| 3739 -
src/phyp/phyp_driver.h| 24 -
16 files changed, 4 insertions(+), 3921 deletions(-)
delete mode 100644 docs/drvphyp.html.in
delete mode 100644 m4/virt-driver-phyp.m4
delete mode 100644 src/phyp/Makefile.inc.am
delete mode 100644 src/phyp/phyp_driver.c
delete mode 100644 src/phyp/phyp_driver.h
diff --git a/docs/aclpolkit.html.in b/docs/aclpolkit.html.in
index 4a8877d5e7..04cb39006a 100644
--- a/docs/aclpolkit.html.in
+++ b/docs/aclpolkit.html.in
@@ -365,10 +365,6 @@
openvz
OPENVZ
-
- phyp
- PHYP
-
qemu
QEMU
diff --git a/docs/api.html.in b/docs/api.html.in
index 288b9ecf88..85b196417a 100644
--- a/docs/api.html.in
+++ b/docs/api.html.in
@@ -330,7 +330,7 @@
daemon through the remote driver via an
RPC. Some hypervisors do support
client-side connections and responses, such as Test, OpenVZ, VMware,
-Power VM (phyp), VirtualBox (vbox), ESX, Hyper-V, Xen, and Virtuozzo.
+VirtualBox (vbox), ESX, Hyper-V, Xen, and Virtuozzo.
The libvirtd daemon service is started on the host at system boot
time and can also be restarted at any time by a properly privileged
user, such as root. The libvirtd daemon uses the same libvirt API
diff --git a/docs/drivers.html.in b/docs/drivers.html.in
index 4539eedbcd..8743301ebd 100644
--- a/docs/drivers.html.in
+++ b/docs/drivers.html.in
@@ -34,7 +34,6 @@
VMware
Workstation/Player
Xen
Microsoft Hyper-V
- IBM PowerVM (phyp)
Virtuozzo
Bhyve - The BSD
Hypervisor
diff --git a/docs/drvphyp.html.in b/docs/drvphyp.html.in
deleted file mode 100644
index 8e0b43c869..00
--- a/docs/drvphyp.html.in
+++ /dev/null
@@ -1,50 +0,0 @@
-
-
-http://www.w3.org/1999/xhtml;>
-
-IBM PowerVM hypervisor driver (phyp)
-
-
-The IBM PowerVM driver can manage both HMC and IVM PowerVM
-guests. VIOS connections are tunneled through HMC.
-
-
-
-Project Links
-
-
-The http://www-03.ibm.com/systems/power/software/virtualization/index.html;>IBM
-PowerVM hypervisor
-
-
-
-
-Connections to the PowerVM driver
-
-Some example remote connection URIs for the driver are:
-
-
-phyp://user@hmc/system (HMC connection)
-phyp://user@ivm/system (IVM connection)
-
-
-Note: In contrast to other drivers, the
-PowerVM (or phyp) driver is a client-side-only driver,
-internally using ssh to connect to the specified hmc or ivm
-server. Therefore, the remote transport
-mechanism provided by the remote driver and libvirtd will
-not work, and you cannot use URIs like
-phyp+ssh://example.com.
-
-
-
-URI Format
-
-URIs have this general form ([...] marks an
-optional part, {...|...} marks a mandatory choice).
-
-
-phyp://[username@]{hmc|ivm}/managed_system
-
-
-
diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
index 26d313d652..91ee523116 100644
--- a/docs/schemas/capability.rng
+++ b/docs/schemas/capability.rng
@@ -408,8 +408,7 @@
xen
-
Re: [libvirt] [PATCH v2 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()
On Wed, Dec 18, 2019 at 10:53:41AM -0500, Cole Robinson wrote:
> On 12/18/19 5:34 AM, Fabiano Fidêncio wrote:
> > On Wed, Dec 18, 2019 at 11:19 AM Marc Hartmayer
> > wrote:
> >>
> >> On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio
> >> wrote:
> >>> Signed-off-by: Fabiano Fidêncio
> >>> ---
> >>> src/util/virutil.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/src/util/virutil.c b/src/util/virutil.c
> >>> index ed1f696e37..8c255abd7f 100644
> >>> --- a/src/util/virutil.c
> >>> +++ b/src/util/virutil.c
> >>> @@ -582,7 +582,7 @@ virGetHostnameQuiet(void)
> >>> char *
> >>> virGetUserDirectory(void)
> >>> {
> >>> -return virGetUserDirectoryByUID(geteuid());
> >>> +return g_strdup_printf("%s/libvirt", g_get_home_dir());
> >>
> >>
> >> I would suggest to use 'g_build_path' instead of 'g_strdup_printf'.
> >> E.g.
> >>
> >> g_build_path(G_DIR_SEPARATOR_S, g_get_home_dir(), "libvirt");
> >
>
> Good catch
>
> There's also g_build_filename which sounds simpler. Any reason for one
> over the other?
Based on the docs g_build_filename looks better, as it keeps
'/' vs '\' consistent on windows. Aside from that, their
internal impl is basically the same.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()
On 12/18/19 5:34 AM, Fabiano Fidêncio wrote:
> On Wed, Dec 18, 2019 at 11:19 AM Marc Hartmayer
> wrote:
>>
>> On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio
>> wrote:
>>> Signed-off-by: Fabiano Fidêncio
>>> ---
>>> src/util/virutil.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/util/virutil.c b/src/util/virutil.c
>>> index ed1f696e37..8c255abd7f 100644
>>> --- a/src/util/virutil.c
>>> +++ b/src/util/virutil.c
>>> @@ -582,7 +582,7 @@ virGetHostnameQuiet(void)
>>> char *
>>> virGetUserDirectory(void)
>>> {
>>> -return virGetUserDirectoryByUID(geteuid());
>>> +return g_strdup_printf("%s/libvirt", g_get_home_dir());
>>
>>
>> I would suggest to use 'g_build_path' instead of 'g_strdup_printf'.
>> E.g.
>>
>> g_build_path(G_DIR_SEPARATOR_S, g_get_home_dir(), "libvirt");
>
Good catch
There's also g_build_filename which sounds simpler. Any reason for one
over the other?
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Remove phyp driver
On 12/18/19 10:43 AM, Cole Robinson wrote:
> The phyp driver was added in 2009 and does not appear to have had any
> real feature change since 2011. There's virtually no evidence online
> of users actually using it. IMO it's time to kill it.
>
> Signed-off-by: Cole Robinson
> ---
> I raised this in 3.5 years ago:
> https://www.redhat.com/archives/libvir-list/2016-April/msg01060.html
>
> Not much on phyp/ side has changed since then, except dozens of dev
> patches transitioning the code forward.
>
> That mail also mentions xenapi and hyperv. hyperv saw signs of life
> afterwards and is still around. xenapi has been removed, along with uml.
>
> Considering the amount of code transitions we are currently undergoing
> (gnulib, glib, memory auto cleanup, etc), phyp/ will probably have an
> uptick of dev energy in the medium term. Let's bite the bullet and
> remove it!
With this squashed in too, forgot about 'grep -i'
Maybe the auth.html.in bit can be entirely removed, I need to study how
that works
- Cole
diff --git a/configure.ac b/configure.ac
index 6c7c186d18..002a3dcdb0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -456,7 +456,6 @@ dnl
LIBVIRT_DRIVER_ARG_QEMU
LIBVIRT_DRIVER_ARG_OPENVZ
LIBVIRT_DRIVER_ARG_VMWARE
-LIBVIRT_DRIVER_ARG_PHYP
LIBVIRT_DRIVER_ARG_LIBXL
LIBVIRT_DRIVER_ARG_VBOX
LIBVIRT_DRIVER_ARG_LXC
@@ -473,7 +472,6 @@ LIBVIRT_DRIVER_ARG_INTERFACE
LIBVIRT_DRIVER_CHECK_QEMU
LIBVIRT_DRIVER_CHECK_OPENVZ
LIBVIRT_DRIVER_CHECK_VMWARE
-LIBVIRT_DRIVER_CHECK_PHYP
LIBVIRT_DRIVER_CHECK_LIBXL
LIBVIRT_DRIVER_CHECK_VBOX
LIBVIRT_DRIVER_CHECK_LXC
@@ -953,7 +951,6 @@ LIBVIRT_DRIVER_RESULT_VMWARE
LIBVIRT_DRIVER_RESULT_VBOX
LIBVIRT_DRIVER_RESULT_LIBXL
LIBVIRT_DRIVER_RESULT_LXC
-LIBVIRT_DRIVER_RESULT_PHYP
LIBVIRT_DRIVER_RESULT_ESX
LIBVIRT_DRIVER_RESULT_HYPERV
LIBVIRT_DRIVER_RESULT_VZ
diff --git a/docs/auth.html.in b/docs/auth.html.in
index 62af43a915..f198f39b64 100644
--- a/docs/auth.html.in
+++ b/docs/auth.html.in
@@ -129,7 +129,7 @@ credentials=defgrp
libvirt - used for connections to a libvirtd
server, which is configured with SASL auth
ssh - used for connections to a Phyp server
-over SSH
+over SSH, but the Phyp driver has been removed
esx - used for connections to an ESX or
VirtualCenter server
diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
index 685e171235..3c65299840 100644
--- a/include/libvirt/virterror.h
+++ b/include/libvirt/virterror.h
@@ -84,7 +84,7 @@ typedef enum {
VIR_FROM_ONE = 27, /* The OpenNebula driver no longer exists.
Retained for ABI/API compat only */
VIR_FROM_ESX = 28, /* Error from ESX driver */
-VIR_FROM_PHYP = 29, /* Error from IBM power hypervisor */
+VIR_FROM_PHYP = 29, /* Error from the (removed) phyp driver */
VIR_FROM_SECRET = 30, /* Error from secret storage */
VIR_FROM_CPU = 31, /* Error from CPU driver */
diff --git a/src/README b/src/README
index 0c4d3b58c7..7ddbc08bfe 100644
--- a/src/README
+++ b/src/README
@@ -40,7 +40,7 @@ Then there are the hypervisor implementations:
Finally some secondary drivers that are shared for several HVs.
Currently these are used by LXC, OpenVZ, QEMU and Xen drivers.
-The ESX, Hyper-V, Power Hypervisor, Remote, Test & VirtualBox drivers all
+The ESX, Hyper-V, Remote, Test & VirtualBox drivers all
implement the secondary drivers directly
* cpu/ - CPU feature management
diff --git a/tools/virsh.c b/tools/virsh.c
index e70711f5d2..9fb9ed6430 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -522,9 +522,6 @@ virshShowVersion(vshControl *ctl G_GNUC_UNUSED)
#ifdef WITH_VMWARE
vshPrint(ctl, " VMware");
#endif
-#ifdef WITH_PHYP
-vshPrint(ctl, " PHYP");
-#endif
#ifdef WITH_VBOX
vshPrint(ctl, " VirtualBox");
#endif
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Remove phyp driver
The phyp driver was added in 2009 and does not appear to have had any
real feature change since 2011. There's virtually no evidence online
of users actually using it. IMO it's time to kill it.
Signed-off-by: Cole Robinson
---
I raised this in 3.5 years ago:
https://www.redhat.com/archives/libvir-list/2016-April/msg01060.html
Not much on phyp/ side has changed since then, except dozens of dev
patches transitioning the code forward.
That mail also mentions xenapi and hyperv. hyperv saw signs of life
afterwards and is still around. xenapi has been removed, along with uml.
Considering the amount of code transitions we are currently undergoing
(gnulib, glib, memory auto cleanup, etc), phyp/ will probably have an
uptick of dev energy in the medium term. Let's bite the bullet and
remove it!
docs/aclpolkit.html.in|4 -
docs/api.html.in |2 +-
docs/drivers.html.in |1 -
docs/drvphyp.html.in | 50 -
docs/schemas/capability.rng |3 +-
docs/schemas/domaincommon.rng |2 +-
libvirt.spec.in | 11 +-
m4/virt-driver-phyp.m4| 48 -
mingw-libvirt.spec.in |7 -
po/POTFILES.in|1 -
src/Makefile.am |1 -
src/README|1 -
src/libvirt.c | 10 -
src/phyp/Makefile.inc.am | 21 -
src/phyp/phyp_driver.c| 3739 -
src/phyp/phyp_driver.h| 24 -
16 files changed, 4 insertions(+), 3921 deletions(-)
delete mode 100644 docs/drvphyp.html.in
delete mode 100644 m4/virt-driver-phyp.m4
delete mode 100644 src/phyp/Makefile.inc.am
delete mode 100644 src/phyp/phyp_driver.c
delete mode 100644 src/phyp/phyp_driver.h
diff --git a/docs/aclpolkit.html.in b/docs/aclpolkit.html.in
index 4a8877d5e7..04cb39006a 100644
--- a/docs/aclpolkit.html.in
+++ b/docs/aclpolkit.html.in
@@ -365,10 +365,6 @@
openvz
OPENVZ
-
- phyp
- PHYP
-
qemu
QEMU
diff --git a/docs/api.html.in b/docs/api.html.in
index 288b9ecf88..85b196417a 100644
--- a/docs/api.html.in
+++ b/docs/api.html.in
@@ -330,7 +330,7 @@
daemon through the remote driver via an
RPC. Some hypervisors do support
client-side connections and responses, such as Test, OpenVZ, VMware,
-Power VM (phyp), VirtualBox (vbox), ESX, Hyper-V, Xen, and Virtuozzo.
+VirtualBox (vbox), ESX, Hyper-V, Xen, and Virtuozzo.
The libvirtd daemon service is started on the host at system boot
time and can also be restarted at any time by a properly privileged
user, such as root. The libvirtd daemon uses the same libvirt API
diff --git a/docs/drivers.html.in b/docs/drivers.html.in
index 4539eedbcd..8743301ebd 100644
--- a/docs/drivers.html.in
+++ b/docs/drivers.html.in
@@ -34,7 +34,6 @@
VMware
Workstation/Player
Xen
Microsoft Hyper-V
- IBM PowerVM (phyp)
Virtuozzo
Bhyve - The BSD
Hypervisor
diff --git a/docs/drvphyp.html.in b/docs/drvphyp.html.in
deleted file mode 100644
index 8e0b43c869..00
--- a/docs/drvphyp.html.in
+++ /dev/null
@@ -1,50 +0,0 @@
-
-
-http://www.w3.org/1999/xhtml;>
-
-IBM PowerVM hypervisor driver (phyp)
-
-
-The IBM PowerVM driver can manage both HMC and IVM PowerVM
-guests. VIOS connections are tunneled through HMC.
-
-
-
-Project Links
-
-
-The http://www-03.ibm.com/systems/power/software/virtualization/index.html;>IBM
-PowerVM hypervisor
-
-
-
-
-Connections to the PowerVM driver
-
-Some example remote connection URIs for the driver are:
-
-
-phyp://user@hmc/system (HMC connection)
-phyp://user@ivm/system (IVM connection)
-
-
-Note: In contrast to other drivers, the
-PowerVM (or phyp) driver is a client-side-only driver,
-internally using ssh to connect to the specified hmc or ivm
-server. Therefore, the remote transport
-mechanism provided by the remote driver and libvirtd will
-not work, and you cannot use URIs like
-phyp+ssh://example.com.
-
-
-
-URI Format
-
-URIs have this general form ([...] marks an
-optional part, {...|...} marks a mandatory choice).
-
-
-phyp://[username@]{hmc|ivm}/managed_system
-
-
-
diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
index 26d313d652..91ee523116 100644
--- a/docs/schemas/capability.rng
+++ b/docs/schemas/capability.rng
@@ -408,8 +408,7 @@
xen
-linux
+linux
hvm
exe
uml
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index e964773f5e..7238d19268 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -216,7 +216,7 @@
vmware
hyperv
vbox
-phyp
+phyp
vz
Re: [libvirt] [PATCH v3 1/1] qemu_command: tidy up qemuBuildHostdevCommandLine loop
On 12/18/19 10:28 AM, Daniel Henrique Barboza wrote:
> The current 'for' loop with 5 consecutive 'ifs' inside
> qemuBuildHostdevCommandLine can be a bit smarter:
>
> - all 5 'ifs' fails if hostdev->mode is not equal to
> VIR_DOMAIN_HOSTDEV_MODE_SUBSYS. This check can be moved to the
> start of the loop, failing to the next element immediately
> in case it fails;
>
> - all 5 'ifs' checks for a specific subsys->type to build the proper
> command line argument (virHostdevIsSCSIDevice and virHostdevIsMdevDevice
> do that but within a helper). Problem is that the code will keep
> checking for matches even if one was already found, and there is
> no way a hostdev will fit more than one 'if' (i.e. a hostdev can't
> have 2+ different types). This means that a SUBSYS_TYPE_USB will
> create its command line argument in the first 'if', then all other
> conditionals will surely fail but will end up being checked anyway.
>
> All of this can be avoided by moving the hostdev->mode comparing
> to the start of the loop and using a switch statement with
> subsys->type to execute the proper code for a given hostdev
> type.
>
> Suggested-by: Ján Tomko
> Signed-off-by: Daniel Henrique Barboza
> ---
> src/qemu/qemu_command.c | 52 +
> 1 file changed, 32 insertions(+), 20 deletions(-)
Sorry for letting this slip review.
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 836057a4ff..948ef688f2 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -5309,23 +5309,31 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
> for (i = 0; i < def->nhostdevs; i++) {
> virDomainHostdevDefPtr hostdev = def->hostdevs[i];
> virDomainHostdevSubsysPtr subsys = >source.subsys;
> +virDomainHostdevSubsysSCSIPtr scsisrc;
> +virDomainHostdevSubsysMediatedDevPtr mdevsrc;
I think we can initialize these variables upfront, just like we are
doing in virDomainAuditHostdev(), virDomainHostdevDefParseXMLSubsys(),
qemuDomainGetHostdevPath() and many other places.
> g_autofree char *devstr = NULL;
> +g_autofree char *drvstr = NULL;
> +g_autofree char *vhostfdName = NULL;
> +unsigned int bootIndex;
And also this one.
> +int vhostfd = -1;
>
> -/* USB */
> -if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> -subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
> +if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
> +continue;
>
> +switch (subsys->type) {
> +/* USB */
> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
> virCommandAddArg(cmd, "-device");
> if (!(devstr =
>qemuBuildUSBHostdevDevStr(def, hostdev, qemuCaps)))
> return -1;
> virCommandAddArg(cmd, devstr);
> -}
> +
> +break;
>
> /* PCI */
> -if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> -subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
> -unsigned int bootIndex = hostdev->info->bootIndex;
> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
> +bootIndex = hostdev->info->bootIndex;
>
> /* bootNet will be non-0 if boot order was set and no other
> * net devices were encountered
> @@ -5343,13 +5351,12 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
> if (!devstr)
> return -1;
> virCommandAddArg(cmd, devstr);
> -}
> +
> +break;
>
> /* SCSI */
> -if (virHostdevIsSCSIDevice(hostdev)) {
> -virDomainHostdevSubsysSCSIPtr scsisrc =
> ->source.subsys.u.scsi;
> -g_autofree char *drvstr = NULL;
> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
> +scsisrc = >source.subsys.u.scsi;
>
> if (scsisrc->protocol ==
> VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
> virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc =
> @@ -5372,15 +5379,13 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
> if (!(devstr = qemuBuildSCSIHostdevDevStr(def, hostdev)))
> return -1;
> virCommandAddArg(cmd, devstr);
> -}
> +
> +break;
>
> /* SCSI_host */
> -if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> -subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST) {
> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST:
> if (hostdev->source.subsys.u.scsi_host.protocol ==
> VIR_DOMAIN_HOSTDEV_SUBSYS_SCSI_HOST_PROTOCOL_TYPE_VHOST) {
> -g_autofree char *vhostfdName = NULL;
> -int vhostfd = -1;
>
> if (virSCSIVHostOpenVhostSCSI() < 0)
> return -1;
> @@ -5399,11 +5404,12 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
>
>
Re: [libvirt] [PATCH] qemu: store the emulator name in the capabilities XML
On Wed, Dec 18, 2019 at 03:41:15PM +0100, Michal Prívozník wrote:
> On 12/18/19 3:03 PM, Daniel P. Berrangé wrote:
> > We don't need this for any functional purpose, but when debugging hosts
> > it is useful to know what binary a given capabilities XML document is
> > associated with.
> >
> > Signed-off-by: Daniel P. Berrangé
> > ---
> > src/qemu/qemu_capabilities.c | 15 +++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index 2223589058..7d47fa4d02 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -3852,6 +3852,7 @@ virQEMUCapsParseSEVInfo(virQEMUCapsPtr qemuCaps,
> > xmlXPathContextPtr ctxt)
> > * Parsing a doc that looks like
> > *
> > *
> > + * /some/path
> > * 234235253
> > * 234235253
> > * 1002016
> > @@ -3895,6 +3896,18 @@ virQEMUCapsLoadCache(virArch hostArch,
> > goto cleanup;
> > }
> >
> > +if (!(str = virXPathString("string(./emulator)", ctxt))) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("missing emulator in QEMU capabilities cache"));
> > +goto cleanup;
>
> Since no caps stored on a disk have this, this change will trigger full
> caps reprobe. I'm not saying it's a bad thing, just so that we are aware
> of this.
We reprobe any time libvirtd itself changes its modification
time so all those existing caps are already invalidated.
> > +virBufferEscapeString(, "%s\n",
> > + qemuCaps->binary);
> > virBufferAsprintf(, "%llu\n",
> >(long long)qemuCaps->ctime);
> > virBufferAsprintf(, "%llu\n",
> >
>
> What I'm missing here is change to our tests/qemucapabilitiesdata/*.xml
> that would introduce the to each one of them.
Sigh, yes, I knew there was something I forgot todo yesterday when
writing this.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: store the emulator name in the capabilities XML
On 12/18/19 11:03 AM, Daniel P. Berrangé wrote:
We don't need this for any functional purpose, but when debugging hosts
it is useful to know what binary a given capabilities XML document is
associated with.
Signed-off-by: Daniel P. Berrangé
---
src/qemu/qemu_capabilities.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 2223589058..7d47fa4d02 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3852,6 +3852,7 @@ virQEMUCapsParseSEVInfo(virQEMUCapsPtr qemuCaps,
xmlXPathContextPtr ctxt)
* Parsing a doc that looks like
*
*
+ * /some/path
* 234235253
* 234235253
* 1002016
@@ -3895,6 +3896,18 @@ virQEMUCapsLoadCache(virArch hostArch,
goto cleanup;
}
+if (!(str = virXPathString("string(./emulator)", ctxt))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing emulator in QEMU capabilities cache"));
+goto cleanup;
+}
+if (!STREQ(str, qemuCaps->binary)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Expected caps for '%s' but saw '%s'"),
+ qemuCaps->binary, str);
+goto cleanup;
+}
Looks like "make syntax-check" is not happy about this if statement:
../src/qemu/qemu_capabilities.c:3904:if (!STREQ(str, qemuCaps->binary)) {
build-aux/syntax-check.mk: Use STRNEQ instead of !STREQ and STREQ instead of
!STRNEQ
make: *** [../build-aux/syntax-check.mk:1104: sc_prohibit_not_streq] Error 1
make: *** Waiting for unfinished jobs
8.84 prohibit_backslash_alignment
8.96 prohibit_canonicalize_without_use
This patch also broke 6 tests in 'make check', at least here in my env:
FAIL: domaincapstest
FAIL: qemuxml2argvtest
FAIL: qemuxml2xmltest
FAIL: qemublocktest
FAIL: qemusecuritytest
FAIL: qemucapabilitiestest
Thanks,
DHB
+VIR_FREE(str);
if (virXPathLongLong("string(./qemuctime)", ctxt, ) < 0) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("missing qemuctime in QEMU capabilities XML"));
@@ -4232,6 +4245,8 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps)
virBufferAddLit(, "\n");
virBufferAdjustIndent(, 2);
+virBufferEscapeString(, "%s\n",
+ qemuCaps->binary);
virBufferAsprintf(, "%llu\n",
(long long)qemuCaps->ctime);
virBufferAsprintf(, "%llu\n",
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: store the emulator name in the capabilities XML
On 12/18/19 3:03 PM, Daniel P. Berrangé wrote:
> We don't need this for any functional purpose, but when debugging hosts
> it is useful to know what binary a given capabilities XML document is
> associated with.
>
> Signed-off-by: Daniel P. Berrangé
> ---
> src/qemu/qemu_capabilities.c | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 2223589058..7d47fa4d02 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -3852,6 +3852,7 @@ virQEMUCapsParseSEVInfo(virQEMUCapsPtr qemuCaps,
> xmlXPathContextPtr ctxt)
> * Parsing a doc that looks like
> *
> *
> + * /some/path
> * 234235253
> * 234235253
> * 1002016
> @@ -3895,6 +3896,18 @@ virQEMUCapsLoadCache(virArch hostArch,
> goto cleanup;
> }
>
> +if (!(str = virXPathString("string(./emulator)", ctxt))) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing emulator in QEMU capabilities cache"));
> +goto cleanup;
Since no caps stored on a disk have this, this change will trigger full
caps reprobe. I'm not saying it's a bad thing, just so that we are aware
of this.
> +}
> +if (!STREQ(str, qemuCaps->binary)) {
Use STRNEQ() instead, please, to make syntax-check happy.
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Expected caps for '%s' but saw '%s'"),
> + qemuCaps->binary, str);
> +goto cleanup;
> +}
> +VIR_FREE(str);
> if (virXPathLongLong("string(./qemuctime)", ctxt, ) < 0) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> _("missing qemuctime in QEMU capabilities XML"));
> @@ -4232,6 +4245,8 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps)
> virBufferAddLit(, "\n");
> virBufferAdjustIndent(, 2);
>
> +virBufferEscapeString(, "%s\n",
> + qemuCaps->binary);
> virBufferAsprintf(, "%llu\n",
>(long long)qemuCaps->ctime);
> virBufferAsprintf(, "%llu\n",
>
What I'm missing here is change to our tests/qemucapabilitiesdata/*.xml
that would introduce the to each one of them.
Otherwise the patch looks good. This is something I wanted a long time ago.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: fix populating of fake NUMA in multi-node hosts
On 12/18/19 12:58 PM, Daniel P. Berrangé wrote: > If the host OS doesn't have NUMA present, we fallback to > populating fake NUMA info and the code thus assumes only a > single NUMA node. > > Unfortunately we also fallback to fake NUMA if numactl-devel > was not present, and in this case we can still have multiple > NUMA nodes. In this case we create all CPUs, but only the > CPUs in the first node have any data filled in, resulting in > capabilities like: > > > > > 15977572 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > With this new code we get something slightly less broken > > > > > 15977572 > > > > > > > > > > > > > > > > > 15977572 > > > > > > > > > > > > > > > > > > > The topology at least now reflects what 'virsh nodeinfo' reports. > The main bug is that the CPU "id" values won't match what the Linux > host actually uses. > > Signed-off-by: Daniel P. Berrangé > --- > src/conf/capabilities.c | 67 ++--- > 1 file changed, 36 insertions(+), 31 deletions(-) Reviewed-by: Michal Privoznik Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: store the emulator name in the capabilities XML
We don't need this for any functional purpose, but when debugging hosts
it is useful to know what binary a given capabilities XML document is
associated with.
Signed-off-by: Daniel P. Berrangé
---
src/qemu/qemu_capabilities.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 2223589058..7d47fa4d02 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3852,6 +3852,7 @@ virQEMUCapsParseSEVInfo(virQEMUCapsPtr qemuCaps,
xmlXPathContextPtr ctxt)
* Parsing a doc that looks like
*
*
+ * /some/path
* 234235253
* 234235253
* 1002016
@@ -3895,6 +3896,18 @@ virQEMUCapsLoadCache(virArch hostArch,
goto cleanup;
}
+if (!(str = virXPathString("string(./emulator)", ctxt))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing emulator in QEMU capabilities cache"));
+goto cleanup;
+}
+if (!STREQ(str, qemuCaps->binary)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Expected caps for '%s' but saw '%s'"),
+ qemuCaps->binary, str);
+goto cleanup;
+}
+VIR_FREE(str);
if (virXPathLongLong("string(./qemuctime)", ctxt, ) < 0) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("missing qemuctime in QEMU capabilities XML"));
@@ -4232,6 +4245,8 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps)
virBufferAddLit(, "\n");
virBufferAdjustIndent(, 2);
+virBufferEscapeString(, "%s\n",
+ qemuCaps->binary);
virBufferAsprintf(, "%llu\n",
(long long)qemuCaps->ctime);
virBufferAsprintf(, "%llu\n",
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [EXTERNAL]Re: [PATCH-for-4.2] hw/mips: Deprecate the r4k machine
From: Thomas Huth Sent: Tuesday, December 17, 2019 7:10 PM To: Philippe Mathieu-Daudé; qemu-de...@nongnu.org Cc: libvir-list@redhat.com; Hervé Poussineau; Aleksandar Markovic; Aleksandar Rikalo; Aurelien Jarno Subject: [EXTERNAL]Re: [PATCH-for-4.2] hw/mips: Deprecate the r4k machine Hi, On 25/11/2019 11.41, Philippe Mathieu-Daudé wrote: > > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi > > index 4b4b7425ac..05265b43c8 100644 > > --- a/qemu-deprecated.texi > > +++ b/qemu-deprecated.texi > > @@ -266,6 +266,11 @@ The 'scsi-disk' device is deprecated. Users should use > > 'scsi-hd' or > > > > @section System emulator machines > > > > +@subsection mips r4k platform (since 4.2) > > Since the patch has now been merged after the release of 4.2, the mips > 4k platform will be deprecated in 5.0 instead. Could you send a patch to > fix it up? OK, I'll send a patch that'll certainly be applied to the next MIPS queue. Thanks for spotting this, Thomas. Aleksandar > Thanks, > Thomas -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/5] Various memleak fixes
On 12/18/19 5:37 AM, Michal Privoznik wrote: *** BLURB HERE *** Michal Prívozník (5): domaincapstest: Don't leak cpu definitions testutilsxen: Avoid double free of driver caps virCapabilitiesHostNUMAUnref: Accept NULL qemu: Reoder cleanup in qemuStateCleanup() qemu: Don't leak hostcpu or hostnuma on driver cleanup Just a couple of grammatical nits in 2 patches. For all patches: Reviewed-by: Daniel Henrique Barboza -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] qemu: Reoder cleanup in qemuStateCleanup()
Commit title has a typo: "Reoder" instead of "Reorder" On 12/18/19 5:37 AM, Michal Privoznik wrote: This function is supposed to clean up virQEMUDriver structure and free individual members. However, it's doing that in random order which makes it hard to track which members are being freed and which are not. Do the free in reverse order than the structure definition - assuming that the most important members (like mutex) are declared first and freed last. Signed-off-by: Michal Privoznik --- -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] virCapabilitiesHostNUMAUnref: Accept NULL
On 12/18/19 5:37 AM, Michal Privoznik wrote: Fortunately, this is not causing any problems now because glib does this check for us when calling this function via attribute cleanup. But in future commit we will explicitly call this nit: "But in a future commit ..." -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: avoid mem leak re-allocating fake NUMA capabilities
On 12/18/19 8:57 AM, Daniel P. Berrangé wrote: The 'caps' object is already allocated when the fake NUMA initialization takes place. Signed-off-by: Daniel P. Berrangé --- Reviewed-by: Daniel Henrique Barboza -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] conf: fix populating of fake NUMA in multi-node hosts
If the host OS doesn't have NUMA present, we fallback to
populating fake NUMA info and the code thus assumes only a
single NUMA node.
Unfortunately we also fallback to fake NUMA if numactl-devel
was not present, and in this case we can still have multiple
NUMA nodes. In this case we create all CPUs, but only the
CPUs in the first node have any data filled in, resulting in
capabilities like:
15977572
With this new code we get something slightly less broken
15977572
15977572
The topology at least now reflects what 'virsh nodeinfo' reports.
The main bug is that the CPU "id" values won't match what the Linux
host actually uses.
Signed-off-by: Daniel P. Berrangé
---
src/conf/capabilities.c | 67 ++---
1 file changed, 36 insertions(+), 31 deletions(-)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index 81a2004dba..2a183d7070 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -1593,7 +1593,7 @@ virCapabilitiesHostNUMAInitFake(virCapsHostNUMAPtr caps)
virNodeInfo nodeinfo;
virCapsHostNUMACellCPUPtr cpus;
int ncpus;
-int s, c, t;
+int n, s, c, t;
int id, cid;
int onlinecpus G_GNUC_UNUSED;
bool tmp;
@@ -1602,47 +1602,52 @@ virCapabilitiesHostNUMAInitFake(virCapsHostNUMAPtr caps)
return -1;
ncpus = VIR_NODEINFO_MAXCPUS(nodeinfo);
-onlinecpus = nodeinfo.cpus;
-if (VIR_ALLOC_N(cpus, ncpus) < 0)
-return -1;
-id = cid = 0;
-for (s = 0; s < nodeinfo.sockets; s++) {
-for (c = 0; c < nodeinfo.cores; c++) {
-for (t = 0; t < nodeinfo.threads; t++) {
-if (virHostCPUGetOnline(id, ) < 0)
-goto error;
-if (tmp) {
-cpus[cid].id = id;
-cpus[cid].socket_id = s;
-cpus[cid].core_id = c;
-if (!(cpus[cid].siblings = virBitmapNew(ncpus)))
+id = 0;
+for (n = 0; n < nodeinfo.nodes; n++) {
+int nodecpus = nodeinfo.sockets * nodeinfo.cores * nodeinfo.threads;
+cid = 0;
+
+if (VIR_ALLOC_N(cpus, nodecpus) < 0)
+return -1;
+
+for (s = 0; s < nodeinfo.sockets; s++) {
+for (c = 0; c < nodeinfo.cores; c++) {
+g_autoptr(virBitmap) siblings = virBitmapNew(ncpus);
+for (t = 0; t < nodeinfo.threads; t++)
+ignore_value(virBitmapSetBit(siblings, id + t));
+
+for (t = 0; t < nodeinfo.threads; t++) {
+if (virHostCPUGetOnline(id, ) < 0)
goto error;
-ignore_value(virBitmapSetBit(cpus[cid].siblings, id));
-cid++;
+if (tmp) {
+cpus[cid].id = id;
+cpus[cid].socket_id = s;
+cpus[cid].core_id = c;
+if (!(cpus[cid].siblings = virBitmapNew(ncpus)))
+goto error;
+virBitmapCopy(cpus[cid].siblings, siblings);
+cid++;
+}
+
+id++;
}
-
-id++;
}
}
-}
-virCapabilitiesHostNUMAAddCell(caps, 0,
- nodeinfo.memory,
-#ifdef __linux__
- onlinecpus, cpus,
-#else
- ncpus, cpus,
-#endif
- 0, NULL,
- 0, NULL);
+virCapabilitiesHostNUMAAddCell(caps, 0,
+ nodeinfo.memory,
+ cid, cpus,
+ 0, NULL,
+ 0, NULL);
+}
return 0;
error:
-for (; id >= 0; id--)
-virBitmapFree(cpus[id].siblings);
+for (; cid >= 0; cid--)
+virBitmapFree(cpus[cid].siblings);
VIR_FREE(cpus);
return -1;
}
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] conf: avoid mem leak re-allocating fake NUMA capabilities
The 'caps' object is already allocated when the fake NUMA initialization takes place. Signed-off-by: Daniel P. Berrangé --- src/conf/capabilities.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 4fac59e6f7..81a2004dba 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1628,9 +1628,6 @@ virCapabilitiesHostNUMAInitFake(virCapsHostNUMAPtr caps) } } -caps = g_new0(virCapsHostNUMA, 1); -caps->cells = g_ptr_array_new_with_free_func( -(GDestroyNotify)virCapabilitiesFreeHostNUMACell); virCapabilitiesHostNUMAAddCell(caps, 0, nodeinfo.memory, #ifdef __linux__ -- 2.23.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] qemu: use g_autofree instead of VIR_FREE in qemuMonitorTextCreateSnapshot()
On Tue, Dec 17, 2019 at 10:55:24AM -0500, Cole Robinson wrote: > On 12/6/19 4:11 AM, Pavel Mores wrote: > > While at bugfixing, convert the whole function to the new-style memory > > allocation handling. > > > > Signed-off-by: Pavel Mores > > --- > > src/qemu/qemu_monitor_text.c | 18 ++ > > 1 file changed, 6 insertions(+), 12 deletions(-) > > > > Reviewed-by: Cole Robinson > > Looks like this patch was missed. Pushed now Thanks! I dropped this patch from v2 intentionally because it was just a clean-up of a previous patch which was dropped (for duplicating a recent commit) and without which this one felt out of place and context in v2. But on its own, this patch is still relevant so it's good you pushed it. Thanks, pvl -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()
On Wed, Dec 18, 2019 at 11:19 AM Marc Hartmayer wrote:
>
> On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio
> wrote:
> > Signed-off-by: Fabiano Fidêncio
> > ---
> > src/util/virutil.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/util/virutil.c b/src/util/virutil.c
> > index ed1f696e37..8c255abd7f 100644
> > --- a/src/util/virutil.c
> > +++ b/src/util/virutil.c
> > @@ -582,7 +582,7 @@ virGetHostnameQuiet(void)
> > char *
> > virGetUserDirectory(void)
> > {
> > -return virGetUserDirectoryByUID(geteuid());
> > +return g_strdup_printf("%s/libvirt", g_get_home_dir());
>
>
> I would suggest to use 'g_build_path' instead of 'g_strdup_printf'.
> E.g.
>
> g_build_path(G_DIR_SEPARATOR_S, g_get_home_dir(), "libvirt");
Indeed. I'll do the change before pushing, in case Cole ACKs the v3. :-)
Best Regards,
--
Fabiano Fidêncio
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] util: Rewrite virGetUserDirectory() using g_get_home_dir()
On Tue, Dec 17, 2019 at 05:41 PM +0100, Fabiano Fidêncio
wrote:
> Signed-off-by: Fabiano Fidêncio
> ---
> src/util/virutil.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index ed1f696e37..8c255abd7f 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -582,7 +582,7 @@ virGetHostnameQuiet(void)
> char *
> virGetUserDirectory(void)
> {
> -return virGetUserDirectoryByUID(geteuid());
> +return g_strdup_printf("%s/libvirt", g_get_home_dir());
I would suggest to use 'g_build_path' instead of 'g_strdup_printf'.
E.g.
g_build_path(G_DIR_SEPARATOR_S, g_get_home_dir(), "libvirt");
> }
>
>
> --
> 2.23.0
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] Some coverity adjustments
On 12/17/19 9:30 PM, John Ferlan wrote: > I upgraded to f31 and it resulted in an essentially hosed Coverity > build/analysis environment with the following message during cov-emit > processing (a preprocessing of sorts): > > "/usr/include/glib-2.0/glib/gspawn.h", line 76: error #67: expected a "}" > G_SPAWN_ERROR_2BIG > GLIB_DEPRECATED_ENUMERATOR_IN_2_32_FOR(G_SPAWN_ERROR_TOO_BIG) = > G_SPAWN_ERROR_TOO_BIG, >^ > > So instead, I'm using a guest to run Coverity "when I remember/can". > > I also found that my f31 environment doesn't like building w/ docs as > I get the following messages while running the convert command: > > ... > usr/bin/mv: cannot stat '/tmp/magick-1191987h12h27ex0lZD.svg': No such file > or directory > GEN kbase.html.tmp > convert: delegate failed `'uniconvertor' '%i' '%o.svg'; /usr/bin/mv '%o.svg' > '%o'' @ error/delegate.c/InvokeDelegate/1958. > convert: unable to open file `/tmp/magick-1191987OqYJwrq8isaG': No such file > or directory @ error/constitute.c/ReadImage/605. > convert: no images defined `migration-managed-p2p.png' @ > error/convert.c/ConvertImageCommand/3235. > > > I haven't followed along as closely as I used to, but my vpath env > uses obj as a subdirectory of my main git tree/target. Whether the > new build env has anything to do with it or it's just f31, I haven't > been able to determine. > > > Beyond these 3 patches here - there is one other adjustment that is > necessary to build libvirt under Coverity and that's removing the > ATTRIBUTE_NONNULL(2) from the virDomainDefFormat definition in > src/conf/domain_conf.h. This was added in commit 92d412149 which > also included two calls to virDomainDefFormat with NULL as the 2nd > argument (hyperv_driver.c and security_apparmor.c); however, the > commit message notes preparation for future work, so I'll keep a > hack for that local for now at least. Well, obviously we pass NULL and at least in the apparmor case it is valid (we don't need to format private data for devices), so I guess we can remove the attribute, but I will let Dan chime in. > > The virsh change below is innocuous yes, but it showed up in a > coverity analysis because it wasn't sure if the resulting variables > could point to the same address and if they did, then there was a > possible use after free because the @source is free'd even though > the @target_node is later referenced. The patch here avoids that > and provides a slight adjustment to not search for either node by > name if it was already found. Whether there's a weird latent issue > because can be repeated while cannot be is something > I suppose a reviewer can warn me about ;-). > > John Ferlan (3): > conf: Fix ATTRIBUTE_NONNULL usages > vbox: Reset @ret after xmlFreeNode > virsh: Adjust logic checks in virshUpdateDiskXML > > src/conf/domain_conf.h| 15 ++- > src/vbox/vbox_snapshot_conf.c | 1 + > tools/virsh-domain.c | 5 ++--- > 3 files changed, 9 insertions(+), 12 deletions(-) > Reviewed-by: Michal Privoznik Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 0/1] qemu_command: tidy up qemuBuildHostdevCommandLine loop
This fella got buried a few months ago. Re-sending as a v3 due to rebase changes. changes from previous version [1]: - rebased to master at 6c17606b7c [1] https://www.redhat.com/archives/libvir-list/2019-October/msg00457.html Daniel Henrique Barboza (1): qemu_command: tidy up qemuBuildHostdevCommandLine loop src/qemu/qemu_command.c | 52 + 1 file changed, 32 insertions(+), 20 deletions(-) -- 2.23.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 1/1] qemu_command: tidy up qemuBuildHostdevCommandLine loop
The current 'for' loop with 5 consecutive 'ifs' inside
qemuBuildHostdevCommandLine can be a bit smarter:
- all 5 'ifs' fails if hostdev->mode is not equal to
VIR_DOMAIN_HOSTDEV_MODE_SUBSYS. This check can be moved to the
start of the loop, failing to the next element immediately
in case it fails;
- all 5 'ifs' checks for a specific subsys->type to build the proper
command line argument (virHostdevIsSCSIDevice and virHostdevIsMdevDevice
do that but within a helper). Problem is that the code will keep
checking for matches even if one was already found, and there is
no way a hostdev will fit more than one 'if' (i.e. a hostdev can't
have 2+ different types). This means that a SUBSYS_TYPE_USB will
create its command line argument in the first 'if', then all other
conditionals will surely fail but will end up being checked anyway.
All of this can be avoided by moving the hostdev->mode comparing
to the start of the loop and using a switch statement with
subsys->type to execute the proper code for a given hostdev
type.
Suggested-by: Ján Tomko
Signed-off-by: Daniel Henrique Barboza
---
src/qemu/qemu_command.c | 52 +
1 file changed, 32 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 836057a4ff..948ef688f2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5309,23 +5309,31 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
for (i = 0; i < def->nhostdevs; i++) {
virDomainHostdevDefPtr hostdev = def->hostdevs[i];
virDomainHostdevSubsysPtr subsys = >source.subsys;
+virDomainHostdevSubsysSCSIPtr scsisrc;
+virDomainHostdevSubsysMediatedDevPtr mdevsrc;
g_autofree char *devstr = NULL;
+g_autofree char *drvstr = NULL;
+g_autofree char *vhostfdName = NULL;
+unsigned int bootIndex;
+int vhostfd = -1;
-/* USB */
-if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
-subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
+if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
+continue;
+switch (subsys->type) {
+/* USB */
+case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
virCommandAddArg(cmd, "-device");
if (!(devstr =
qemuBuildUSBHostdevDevStr(def, hostdev, qemuCaps)))
return -1;
virCommandAddArg(cmd, devstr);
-}
+
+break;
/* PCI */
-if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
-subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
-unsigned int bootIndex = hostdev->info->bootIndex;
+case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
+bootIndex = hostdev->info->bootIndex;
/* bootNet will be non-0 if boot order was set and no other
* net devices were encountered
@@ -5343,13 +5351,12 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
if (!devstr)
return -1;
virCommandAddArg(cmd, devstr);
-}
+
+break;
/* SCSI */
-if (virHostdevIsSCSIDevice(hostdev)) {
-virDomainHostdevSubsysSCSIPtr scsisrc =
->source.subsys.u.scsi;
-g_autofree char *drvstr = NULL;
+case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
+scsisrc = >source.subsys.u.scsi;
if (scsisrc->protocol ==
VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc =
@@ -5372,15 +5379,13 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
if (!(devstr = qemuBuildSCSIHostdevDevStr(def, hostdev)))
return -1;
virCommandAddArg(cmd, devstr);
-}
+
+break;
/* SCSI_host */
-if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
-subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST) {
+case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST:
if (hostdev->source.subsys.u.scsi_host.protocol ==
VIR_DOMAIN_HOSTDEV_SUBSYS_SCSI_HOST_PROTOCOL_TYPE_VHOST) {
-g_autofree char *vhostfdName = NULL;
-int vhostfd = -1;
if (virSCSIVHostOpenVhostSCSI() < 0)
return -1;
@@ -5399,11 +5404,12 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
virCommandAddArg(cmd, devstr);
}
-}
+
+break;
/* MDEV */
-if (virHostdevIsMdevDevice(hostdev)) {
-virDomainHostdevSubsysMediatedDevPtr mdevsrc = >u.mdev;
+case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
+mdevsrc = >u.mdev;
switch ((virMediatedDeviceModelType) mdevsrc->model) {
case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
@@ -5422,6 +5428,12 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
[libvirt] [libvirt-tck PATCH v2] Add cases for nvram
From: Dan Zheng
This is to add the tests for below flags:
- Sys::Virt::Domain::UNDEFINE_KEEP_NVRAM
- Sys::Virt::Domain::UNDEFINE_NVRAM
v1: https://www.redhat.com/archives/libvir-list/2019-December/msg00932.html
Signed-off-by: Dan Zheng
---
scripts/domain/401-ovmf-nvram.t | 144
1 file changed, 144 insertions(+)
create mode 100644 scripts/domain/401-ovmf-nvram.t
diff --git a/scripts/domain/401-ovmf-nvram.t b/scripts/domain/401-ovmf-nvram.t
new file mode 100644
index 000..4af2117
--- /dev/null
+++ b/scripts/domain/401-ovmf-nvram.t
@@ -0,0 +1,144 @@
+# -*- perl -*-
+#
+# Copyright (C) 2009 Red Hat, Inc.
+# Copyright (C) 2018 Dan Zheng (dzh...@redhat.com)
+#
+# This program is free software; You can redistribute it and/or modify
+# it under the GNU General Public License as published by the Free
+# Software Foundation; either version 2, or (at your option) any
+# later version
+#
+# The file "LICENSE" distributed along with this file provides full
+# details of the terms and conditions
+#
+
+=pod
+
+=head1 NAME
+
+domain/401-ovmf-nvram.t - Test OVMF related functions and flags
+
+=head1 DESCRIPTION
+
+The test cases validates OVMF related APIs and flags
+
+Sys::Virt::Domain::UNDEFINE_KEEP_NVRAM
+Sys::Virt::Domain::UNDEFINE_NVRAM
+
+=cut
+
+use strict;
+use warnings;
+
+use Test::More tests => 6;
+
+use Sys::Virt::TCK;
+use File::stat;
+use File::Copy;
+
+
+my $tck = Sys::Virt::TCK->new();
+my $conn = eval { $tck->setup(); };
+BAIL_OUT "failed to setup test harness: $@" if $@;
+END { $tck->cleanup if $tck; }
+
+
+sub setup_nvram {
+
+my $loader_path = shift;
+my $nvram_template = shift;
+my $nvram_path = shift;
+
+# Check below two files should exist
+# - /usr/share/OVMF/OVMF_CODE.secboot.fd
+# - /usr/share/OVMF/OVMF_VARS.fd
+if (!stat($loader_path) or !stat($nvram_template)) {
+return undef;
+}
+
+# Ensure the sample nvram file exists
+copy($nvram_template, $nvram_path) or die "Copy failed: $!";
+
+# Use 'q35' as machine type and add below lines to guest xml
+# /usr/share/OVMF/OVMF_CODE.secboot.fd
+# /var/lib/libvirt/qemu/nvram/test_VARS.fd
+
+my $xml = $tck->generic_domain(name => "tck")->as_xml;
+my $xp = XML::XPath->new($xml);
+
+if ($xp->getNodeText("/domain/os/type/\@machine") ne 'q35') {
+diag "Changing guest machine type to q35";
+$xp->setNodeText("/domain/os/type/\@machine", "q35");
+}
+
+my $loader_node = XML::XPath::Node::Element->new('loader');
+my $loader_text = XML::XPath::Node::Text->new($loader_path);
+my $attr_ro = XML::XPath::Node::Attribute->new('readonly');
+my $attr_secure = XML::XPath::Node::Attribute->new('secure');
+my $attr_type = XML::XPath::Node::Attribute->new('type');
+
+$attr_ro -> setNodeValue('yes');
+$attr_secure -> setNodeValue('yes');
+$attr_type -> setNodeValue('pflash');
+
+$loader_node->appendChild($loader_text);
+$loader_node->appendAttribute($attr_ro);
+$loader_node->appendAttribute($attr_secure);
+$loader_node->appendAttribute($attr_type);
+
+my $nvram_node= XML::XPath::Node::Element->new('nvram');
+my $nvram_text= XML::XPath::Node::Text->new($nvram_path);
+my $attr_template = XML::XPath::Node::Attribute->new('template');
+
+$attr_template -> setNodeValue($nvram_template);
+
+$nvram_node->appendChild($nvram_text);
+$nvram_node->appendAttribute($attr_template);
+
+my $smm_node = XML::XPath::Node::Element->new('smm');
+my $attr_state = XML::XPath::Node::Attribute->new('state');
+$attr_state -> setNodeValue("on");
+$smm_node -> appendAttribute($attr_state);
+
+my ($root) = $xp->findnodes('/domain/os');
+$root->appendChild($loader_node);
+$root->appendChild($nvram_node);
+($root) = $xp->findnodes('/domain/features');
+$root->appendChild($smm_node);
+
+$xml = $xp->findnodes_as_string('/');
+diag $xml;
+return $xml;
+}
+
+diag "Defining an inactive domain config with nvram";
+my $loader_file_path = '/usr/share/OVMF/OVMF_CODE.secboot.fd';
+my $nvram_file_template = '/usr/share/OVMF/OVMF_VARS.fd';
+my $nvram_file_path = '/var/lib/libvirt/qemu/nvram/test_VARS.fd';
+
+my $xml = setup_nvram($loader_file_path, $nvram_file_template,
$nvram_file_path);
+
+SKIP: {
+diag "Require files ($loader_file_path, $nvram_file_template) for testing";
+skip "Please install OVMF and ensure necessary files exist", 5 if
!defined($xml);
+my $dom;
+
+diag "Test Sys::Virt::Domain::UNDEFINE_KEEP_NVRAM";
+ok_domain(sub { $dom = $conn->define_domain($xml) }, "defined domain with
nvram configure");
+diag "Checking nvram file already exists";
+my $st = stat($nvram_file_path);
+ok($st, "File '$nvram_file_path' exists as expected");
+$dom->undefine(Sys::Virt::Domain::UNDEFINE_KEEP_NVRAM);
+diag "Checking nvram file still exists";
+$st = stat($nvram_file_path);
+ok($st,
[libvirt] [PATCH] test: qemucaps: Refresh x86_64 caps probe data for the qemu-4.2 release
Signed-off-by: Peter Krempa
---
Pushed as trivial.
tests/qemucapabilitiesdata/caps_4.2.0.x86_64.replies | 10 +-
tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 6 +++---
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.replies
b/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.replies
index ae07ffdb49..b9481b6f85 100644
--- a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.replies
+++ b/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.replies
@@ -17,11 +17,11 @@
{
"return": {
"qemu": {
- "micro": 92,
- "minor": 1,
+ "micro": 0,
+ "minor": 2,
"major": 4
},
-"package": "v4.2.0-rc2-19-g2061735ff0"
+"package": "v4.2.0"
},
"id": "libvirt-2"
}
@@ -23614,7 +23614,7 @@
"kvm-steal-time": true,
"kvmclock": true,
"vmx-zero-len-inject": false,
-"pschange-mc-no": false,
+"pschange-mc-no": true,
"vmx-rdrand-exit": true,
"lwp": false,
"amd-ssbd": false,
@@ -23930,7 +23930,7 @@
"kvm-steal-time": true,
"kvmclock": true,
"vmx-zero-len-inject": false,
-"pschange-mc-no": false,
+"pschange-mc-no": true,
"vmx-rdrand-exit": true,
"lwp": false,
"amd-ssbd": false,
diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml
index 31302c9a7b..7d886d9a87 100644
--- a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml
@@ -217,10 +217,10 @@
- 4001092
+ 4002000
0
43100242
- v4.2.0-rc2-19-g2061735ff0
+ v4.2.0
x86_64
@@ -416,7 +416,7 @@
-
+
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/5] domaincapstest: Don't leak cpu definitions
When generating domain capabilities, we need to fake host CPU to
get reproducible result. We do this by copying a pre-existent CPU
config and setting VIR_TEST_MOCK_FAKE_HOST_CPU env variable which
is then consumed by qemucpumock. However, we forget to free the
CPU copy afterwards.
2,196 (2,016 direct, 180 indirect) bytes in 18 blocks are definitely lost in
loss record 291 of 297
at 0x4838B86: calloc (vg_replace_malloc.c:762)
by 0x57CB6A0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.7)
by 0x4A0F72D: virCPUDefNew (cpu_conf.c:87)
by 0x4A0FAC7: virCPUDefCopyWithoutModel (cpu_conf.c:235)
by 0x4A0FBBE: virCPUDefCopy (cpu_conf.c:273)
by 0x10E3C0: testUtilsHostCpusGetDefForArch (testutilshostcpus.h:157)
by 0x10E3C0: fakeHostCPU (domaincapstest.c:61)
by 0x10E3C0: fillQemuCaps (domaincapstest.c:86)
by 0x10E3C0: test_virDomainCapsFormat (domaincapstest.c:234)
by 0x10F4BC: virTestRun (testutils.c:146)
by 0x10DE93: doTestQemuInternal (domaincapstest.c:301)
by 0x10E13D: doTestQemu (domaincapstest.c:332)
by 0x1124CF: testQemuCapsIterate (testutilsqemu.c:635)
by 0x10DCE3: mymain (domaincapstest.c:435)
by 0x10FD8B: virTestMain (testutils.c:916)
Signed-off-by: Michal Privoznik
---
tests/domaincapstest.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c
index a4a443b1d6..9f5eab3230 100644
--- a/tests/domaincapstest.c
+++ b/tests/domaincapstest.c
@@ -56,7 +56,7 @@ fillStringValues(virDomainCapsStringValuesPtr values, ...)
static int
fakeHostCPU(virArch arch)
{
-virCPUDefPtr cpu;
+g_autoptr(virCPUDef) cpu = NULL;
if (!(cpu = testUtilsHostCpusGetDefForArch(arch))) {
virReportError(VIR_ERR_INTERNAL_ERROR,
--
2.24.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/5] qemu: Don't leak hostcpu or hostnuma on driver cleanup
When freeing qemu driver struct members, we forgot to free @hostcpu and @hostnuma members. Signed-off-by: Michal Privoznik --- src/qemu/qemu_driver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0b9e31b344..ec8faf384c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1135,6 +1135,8 @@ qemuStateCleanup(void) virObjectUnref(qemu_driver->domainEventState); virObjectUnref(qemu_driver->qemuCapsCache); virObjectUnref(qemu_driver->xmlopt); +virCPUDefFree(qemu_driver->hostcpu); +virCapabilitiesHostNUMAUnref(qemu_driver->hostnuma); virObjectUnref(qemu_driver->caps); ebtablesContextFree(qemu_driver->ebtables); VIR_FREE(qemu_driver->qemuImgBinary); -- 2.24.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/5] Various memleak fixes
*** BLURB HERE *** Michal Prívozník (5): domaincapstest: Don't leak cpu definitions testutilsxen: Avoid double free of driver caps virCapabilitiesHostNUMAUnref: Accept NULL qemu: Reoder cleanup in qemuStateCleanup() qemu: Don't leak hostcpu or hostnuma on driver cleanup src/conf/capabilities.c | 3 +++ src/qemu/qemu_driver.c | 45 + tests/domaincapstest.c | 2 +- tests/testutilsxen.c| 1 - 4 files changed, 23 insertions(+), 28 deletions(-) -- 2.24.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/5] testutilsxen: Avoid double free of driver caps
In testXLInitDriver() a dummy driver structure is filled and it
is freed later in testXLFreeDriver(). However, it is sufficient
to unref just driver->config because that results in
libxlDriverConfigDispose() being called which unrefs
driver->config->caps. There is no need to unref it again in
testXLFreeDriver() - in fact it's undesired.
Signed-off-by: Michal Privoznik
---
tests/testutilsxen.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/tests/testutilsxen.c b/tests/testutilsxen.c
index 75cd42ec43..b73c79581d 100644
--- a/tests/testutilsxen.c
+++ b/tests/testutilsxen.c
@@ -105,7 +105,6 @@ libxlDriverPrivatePtr testXLInitDriver(void)
void testXLFreeDriver(libxlDriverPrivatePtr driver)
{
-virObjectUnref(driver->config->caps);
virObjectUnref(driver->config);
virObjectUnref(driver->xmlopt);
virMutexDestroy(>lock);
--
2.24.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/5] qemu: Reoder cleanup in qemuStateCleanup()
This function is supposed to clean up virQEMUDriver structure and free individual members. However, it's doing that in random order which makes it hard to track which members are being freed and which are not. Do the free in reverse order than the structure definition - assuming that the most important members (like mutex) are declared first and freed last. Signed-off-by: Michal Privoznik --- src/qemu/qemu_driver.c | 43 +- 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 65c3be58d3..0b9e31b344 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1122,38 +1122,29 @@ qemuStateCleanup(void) if (!qemu_driver) return -1; -if (qemu_driver->lockFD != -1) -virPidFileRelease(qemu_driver->config->stateDir, "driver", qemu_driver->lockFD); -virThreadPoolFree(qemu_driver->workerPool); -virObjectUnref(qemu_driver->config); -virObjectUnref(qemu_driver->hostdevMgr); -virHashFree(qemu_driver->sharedDevices); -virObjectUnref(qemu_driver->caps); -virObjectUnref(qemu_driver->qemuCapsCache); - -virObjectUnref(qemu_driver->domains); -virPortAllocatorRangeFree(qemu_driver->remotePorts); -virPortAllocatorRangeFree(qemu_driver->webSocketPorts); -virPortAllocatorRangeFree(qemu_driver->migrationPorts); virObjectUnref(qemu_driver->migrationErrors); - -virObjectUnref(qemu_driver->xmlopt); - -virSysinfoDefFree(qemu_driver->hostsysinfo); - virObjectUnref(qemu_driver->closeCallbacks); - -VIR_FREE(qemu_driver->qemuImgBinary); - +virLockManagerPluginUnref(qemu_driver->lockManager); +virSysinfoDefFree(qemu_driver->hostsysinfo); +virPortAllocatorRangeFree(qemu_driver->migrationPorts); +virPortAllocatorRangeFree(qemu_driver->webSocketPorts); +virPortAllocatorRangeFree(qemu_driver->remotePorts); +virHashFree(qemu_driver->sharedDevices); +virObjectUnref(qemu_driver->hostdevMgr); virObjectUnref(qemu_driver->securityManager); - -ebtablesContextFree(qemu_driver->ebtables); - -/* Free domain callback list */ virObjectUnref(qemu_driver->domainEventState); +virObjectUnref(qemu_driver->qemuCapsCache); +virObjectUnref(qemu_driver->xmlopt); +virObjectUnref(qemu_driver->caps); +ebtablesContextFree(qemu_driver->ebtables); +VIR_FREE(qemu_driver->qemuImgBinary); +virObjectUnref(qemu_driver->domains); +virThreadPoolFree(qemu_driver->workerPool); -virLockManagerPluginUnref(qemu_driver->lockManager); +if (qemu_driver->lockFD != -1) +virPidFileRelease(qemu_driver->config->stateDir, "driver", qemu_driver->lockFD); +virObjectUnref(qemu_driver->config); virMutexDestroy(_driver->lock); VIR_FREE(qemu_driver); -- 2.24.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/5] virCapabilitiesHostNUMAUnref: Accept NULL
Fortunately, this is not causing any problems now because glib
does this check for us when calling this function via attribute
cleanup. But in future commit we will explicitly call this
function over a struct member that might be NULL.
Signed-off-by: Michal Privoznik
---
src/conf/capabilities.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index 4fac59e6f7..a782d92956 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -184,6 +184,9 @@ virCapabilitiesFreeStoragePool(virCapsStoragePoolPtr pool)
void
virCapabilitiesHostNUMAUnref(virCapsHostNUMAPtr caps)
{
+if (!caps)
+return;
+
if (g_atomic_int_dec_and_test(>refs)) {
g_ptr_array_unref(caps->cells);
--
2.24.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
