Re: [libvirt] [Qemu-devel] libvirt/QEMU/SEV interaction

[Michael S. Tsirkin]

On Wed, Oct 18, 2017 at 08:18:48PM +0100, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (m...@redhat.com) wrote:
> > On Fri, Sep 08, 2017 at 10:48:10AM -0500, Brijesh Singh wrote:
> > > > >  > 11. GO verifies the measurement and if measurement matches 
> > > > > then it may
> > > > >  >  give a secret blob -- which must be injected into the guest 
> > > > > before
> > > > >  >  libvirt starts the VM. If verification failed, GO will 
> > > > > request cloud
> > > > >  >  provider to destroy the VM.
> > 
> > I realised I'm missing something here: how does GO limit the
> > secret to the specific VM? For example, what prevents hypervisor
> > from launching two VMs with the same GO's DH, getting measurement
> > from 1st one but injecting the secret into the second one?
> 
> Isn't that the 'trusted channel nonce currently associated with the
> guest' in the guest context?
> 
> Dave

Let me try to clarify the question. I understand that sometimes a key
is shared between VMs. If this is allowed, it seems that a hypervisor
can run any number of VMs with the same key. An unauthorised VM
will not get a measurement that guest owner authorizes, but
can the hypervisor get secret intended for an authorized VM and
then inject it into an unauthorized one sharing the same key?

> > Thanks,
> > 
> > -- 
> > MST
> > 
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v6 0/9] Work around the kernel mdev uevent race in nodedev

[John Ferlan]

On 10/18/2017 09:52 AM, Erik Skultety wrote:
> v5 here:
> https://www.redhat.com/archives/libvir-list/2017-October/msg00440.html
> 
> Since v5:
> - fixed minor nitpicks
> - added 3 more patches as per reviewer's suggestion to split some of the
> changes even more
> - patches {2,6,7,8,9}/9 are without any change
> 
> Erik Skultety (9):
>   nodedev: Move privileged flag from udev private data to driver's state (NEW)
>   nodedev: udev: Introduce udevEventMonitorSanityCheck helper function (ACKed)
>   nodedev: udev: Convert udev private data to a lockable object (ACKed with 
> fixes)
>   nodedev: udev: Remove driver locks from stateInitialize and (NEW)
> stateCleanup
>   nodedev: udev: Unlock the private data before setting up 'system' node (NEW)
>   nodedev: udev: Split udevEventHandleCallback in two functions (ACKed)
>   nodedev: udev: Convert udevEventHandleThread to an actual thread (ACKed)
> routine
>   util: Introduce virFileWaitForExists (ACKed)
>   nodedev: udev: Hook up virFileWaitForAccess to work around uevent race 
> (ACKed)
> 
>  src/conf/virnodedeviceobj.h|   1 +
>  src/libvirt_private.syms   |   1 +
>  src/node_device/node_device_udev.c | 283 
> +++--
>  src/node_device/node_device_udev.h |   3 -
>  src/util/virfile.c |  31 
>  src/util/virfile.h |   2 +
>  6 files changed, 245 insertions(+), 76 deletions(-)
> 
> --
> 2.13.6
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list>

w/ one minor, noted adjustment in patch 5

Reviewed-by: John Ferlan 
(series)

I ran through Coverity too...

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v6 5/9] nodedev: udev: Unlock the private data before setting up 'system' node

[John Ferlan]

On 10/18/2017 09:52 AM, Erik Skultety wrote:
> udevSetupSystemDev only needs the udev data lock to be locked because of
> calling udevGetDMIData which accesses some protected structure members,
> but it can do that on its own just fine, no need to hold the lock the
> whole time.
> 
> Signed-off-by: Erik Skultety 
> ---
>  src/node_device/node_device_udev.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/node_device/node_device_udev.c 
> b/src/node_device/node_device_udev.c
> index e0e5ba799..6882517e6 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1719,6 +1719,7 @@ udevGetDMIData(virNodeDevCapSystemPtr syscap)
>  virNodeDevCapSystemHardwarePtr hardware = >hardware;
>  virNodeDevCapSystemFirmwarePtr firmware = >firmware;
>  
> +virObjectLock(priv);
>  udev = udev_monitor_get_udev(priv->udev_monitor);
>  
>  device = udev_device_new_from_syspath(udev, DMI_DEVPATH);
> @@ -1731,6 +1732,7 @@ udevGetDMIData(virNodeDevCapSystemPtr syscap)

   virObjectUnlock(priv);

Pesky return statements ;-)

John

>  return;
>  }
>  }
> +virObjectUnlock(priv);
>  
>  if (udevGetStringSysfsAttr(device, "product_name",
> >product_name) < 0)
> @@ -1898,11 +1900,11 @@ nodeStateInitialize(bool privileged,
>  if (priv->watch == -1)
>  goto unlock;
>  
> +virObjectUnlock(priv);
> +
>  /* Create a fictional 'computer' device to root the device tree. */
>  if (udevSetupSystemDev() != 0)
> -goto unlock;
> -
> -virObjectUnlock(priv);
> +goto cleanup;
>  
>  /* Populate with known devices */
>  if (udevEnumerateDevices(udev) != 0)
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 00/14] introduce virDomainSetLifecycleAction() API

[John Ferlan]

On 10/16/2017 07:06 AM, Pavel Hrdina wrote:
> Pavel Hrdina (14):
>   conf: rename lifecycle enum values to correspond with typedef keyword
>   conf: rename virDomainLifecycleAction enum functions
>   conf: introduce virDomainLifecycle enum to list all lifecycle types
>   conf: merge virDomainLifecycleCrashAction with
> virDomainLifecycleAction
>   qemu: pass virDomainObjPtr to qemuBuildCommandLine
>   qemu: pass priv data to qemuBuildMonitorCommandLine
>   qemu: pass priv data to qemuBuildPMCommandLine
>   qemu: pass priv data to qemuBuildMasterKeyCommandLine
>   qemu: pass priv data instead of qemuCaps and autoNodeset
>   lib: introduce virDomainSetLifecycleAction() API
>   virsh: introduce set-lifecycle-action command
>   qemu: move detection whether to use -no-reboot to qemu_domain
>   qemu: send allowReboot in migration cookie
>   qemu: implement virDomainSetLifecycleAction() API
> 
>  docs/formatdomain.html.in|   6 +++
>  include/libvirt/libvirt-domain.h |  28 ++
>  src/conf/domain_conf.c   |  84 -
>  src/conf/domain_conf.h   |  26 ++---
>  src/driver-hypervisor.h  |   7 +++
>  src/libvirt-domain.c |  60 +
>  src/libvirt_private.syms |   7 ++-
>  src/libvirt_public.syms  |   5 ++
>  src/libxl/libxl_conf.c   |  43 ---
>  src/libxl/libxl_domain.c |  40 +++---
>  src/lxc/lxc_native.c |   6 +--
>  src/qemu/qemu_command.c  |  93 +---
>  src/qemu/qemu_command.h  |  11 +---
>  src/qemu/qemu_domain.c   |  40 ++
>  src/qemu/qemu_domain.h   |   9 
>  src/qemu/qemu_driver.c   | 112 
> +++
>  src/qemu/qemu_migration.c|   7 ++-
>  src/qemu/qemu_migration_cookie.c |  25 -
>  src/qemu/qemu_migration_cookie.h |   5 ++
>  src/qemu/qemu_parse_command.c|   8 +--
>  src/qemu/qemu_process.c  |  53 --
>  src/remote/remote_driver.c   |   1 +
>  src/remote/remote_protocol.x |  14 -
>  src/test/test_driver.c   |   8 +--
>  src/vmx/vmx.c|   6 +--
>  src/vz/vz_sdk.c  |  12 ++---
>  src/xenapi/xenapi_utils.c|  40 +++---
>  src/xenapi/xenapi_utils.h|   4 +-
>  src/xenconfig/xen_common.c   |  12 ++---
>  src/xenconfig/xen_sxpr.c |  18 +++
>  tests/qemuxml2xmltest.c  |   3 +-
>  tools/virsh-domain.c | 102 +++
>  tools/virsh.pod  |   7 +++
>  33 files changed, 658 insertions(+), 244 deletions(-)
> 

Reviewed-by: John Ferlan 
(series)

As long as you take care of the couple things I noted.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 11/14] virsh: introduce set-lifecycle-action command

[John Ferlan]

On 10/16/2017 07:06 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  tools/virsh-domain.c | 102 
> +++
>  tools/virsh.pod  |   7 
>  2 files changed, 109 insertions(+)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index a50713d6e4..bdafdf6f5d 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -5517,6 +5517,102 @@ cmdScreenshot(vshControl *ctl, const vshCmd *cmd)
>  }
>  
>  /*
> + * "set-lifecycle-action" command
> + */
> +static const vshCmdInfo info_setLifecycleAction[] = {
> +{.name = "help",
> + .data = N_("change lifecycle actions")
> +},
> +{.name = "desc",
> + .data = N_("Change lifecycle actions for the guest domain.")
> +},
> +{.name = NULL}
> +};
> +
> +static const vshCmdOptDef opts_setLifecycleAction[] = {
> +VIRSH_COMMON_OPT_DOMAIN_FULL,
> +{.name = "type",
> + .type = VSH_OT_STRING,
> + .flags = VSH_OFLAG_REQ,
> + .help = N_("lifecycle type to modify")
> +},
> +{.name = "action",
> + .type = VSH_OT_STRING,
> + .flags = VSH_OFLAG_REQ,
> + .help = N_("lifecycle action to set")
> +},
> +VIRSH_COMMON_OPT_DOMAIN_CONFIG,
> +VIRSH_COMMON_OPT_DOMAIN_LIVE,
> +VIRSH_COMMON_OPT_DOMAIN_CURRENT,
> +{.name = NULL}
> +};
> +
> +VIR_ENUM_IMPL(virDomainLifecycle, VIR_DOMAIN_LIFECYCLE_LAST,
> +  "poweroff",
> +  "reboot",
> +  "crash")
> +
> +VIR_ENUM_IMPL(virDomainLifecycleAction, VIR_DOMAIN_LIFECYCLE_ACTION_LAST,
> +  "destroy",
> +  "restart",
> +  "rename-restart",
> +  "preserve",
> +  "coredump-destroy",
> +  "coredump-restart")
> +
> +static bool
> +cmdSetLifecycleAction(vshControl *ctl, const vshCmd *cmd)
> +{
> +virDomainPtr dom;
> +bool ret = true;
> +bool config = vshCommandOptBool(cmd, "config");
> +bool live = vshCommandOptBool(cmd, "live");
> +bool current = vshCommandOptBool(cmd, "current");
> +const char *typeStr;
> +const char *actionStr;
> +unsigned int type;
> +unsigned int action;
> +unsigned int flags = 0;
> +int tmpVal;
> +
> +VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
> +VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
> +
> +if (config)
> +flags |= VIR_DOMAIN_AFFECT_CONFIG;
> +if (live)
> +flags |= VIR_DOMAIN_AFFECT_LIVE;
> +
> +if (vshCommandOptStringReq(ctl, cmd, "type", ) < 0 ||
> +vshCommandOptStringReq(ctl, cmd, "action", ) < 0) {
> +return false;
> +}
> +
> +if ((tmpVal = virDomainLifecycleTypeFromString(typeStr)) < 0) {
> +return false;
> +vshError(ctl, _("Invalid lifecycle type '%s'."), typeStr);

unreachable ;-) as the order is reversed (Coverity didn't even notice,
go figure!

> +}
> +type = tmpVal;
> +
> +if ((tmpVal = virDomainLifecycleActionTypeFromString(actionStr)) < 0) {
> +vshError(ctl, _("Invalid lifecycle action '%s'."), actionStr);
> +return false;
> +}
> +action = tmpVal;
> +
> +if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> +return false;
> +
> +if (virDomainSetLifecycleAction(dom, type, action, flags) < 0) {
> +vshError(ctl, "%s", _("Unable to change lifecycle action."));
> +ret = false;
> +}
> +
> +virshDomainFree(dom);
> +return ret;
> +}
> +
> +/*
>   * "set-user-password" command
>   */
>  static const vshCmdInfo info_set_user_password[] = {
> @@ -14249,6 +14345,12 @@ const vshCmdDef domManagementCmds[] = {
>   .info = info_screenshot,
>   .flags = 0
>  },
> +{.name = "set-lifecycle-action",
> + .handler = cmdSetLifecycleAction,
> + .opts = opts_setLifecycleAction,
> + .info = info_setLifecycleAction,
> + .flags = 0
> +},
>  {.name = "set-user-password",
>   .handler = cmdSetUserPassword,
>   .opts = opts_set_user_password,
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 024d027699..39cb67792a 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -2327,6 +2327,13 @@ the value from the host, use the B 
> command. In order to view
>  the current memory in use and the maximum value allowed to set memory, use
>  the B command.
>  
> +=item B I I I
> +[[I<--config>] [I<--live>] | [I<--current>]]
> +
> +Set the lifecycle action for specified lifecycle type. For the list of

s/action/I
s/type/I

consider "lifecycle event type" - IDC if you do or not, just a thought


> +lifecycle types and actions and possible combinations see the documentation 
> at
> +L.

Probably should add the obligatory what --config, --live, and --current do.


John
> +
>  =item B I I I [I<--encrypted>]
>  
>  Set the password for the I account in the guest domain.
> 

--
libvir-list mailing list
libvir-list@redhat.com

Re: [libvirt] [PATCH 10/14] lib: introduce virDomainSetLifecycleAction() API

[John Ferlan]

On 10/16/2017 07:06 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  docs/formatdomain.html.in|  6 +
>  include/libvirt/libvirt-domain.h | 28 
>  src/conf/domain_conf.h   | 19 --
>  src/driver-hypervisor.h  |  7 +
>  src/libvirt-domain.c | 56 
> 
>  src/libvirt_public.syms  |  5 
>  src/remote/remote_driver.c   |  1 +
>  src/remote/remote_protocol.x | 14 +-
>  8 files changed, 116 insertions(+), 20 deletions(-)

Of course the aforementioned src/remote_protocol-structs will be here...

> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index b93ace7cba..cb19a547fe 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1623,6 +1623,12 @@
>  
>  
>  
> +  The lifecycle events can be configured via
> +  virDomainSetLifecycleAction() API
> +  since 3.9.0 (QEMU only).
> +

s/via/via the/
s/()//

Add the href like other examples, such as


virDomainSetLifecycleAction


I also don't think you can say "(QEMU only)" since the API exists in
libvirt - the implementation is only on QEMU, so you have to add some
text indicating not all hypervisors support.

You may also want to consider text like:

Since 3.9.0, the lifecycle events can be
configured via the xxx API. Not all hypervisors support all events and
all actions.

I think having the since afterwards can be "confusing" sometimes, ymmv.

John

> +
> +
>The on_lockfailure element (since
>1.0.0) may be used to configure what action should be
>taken when a lock manager loses resource locks. The following

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] libvirt/QEMU/SEV interaction

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (m...@redhat.com) wrote:
> On Fri, Sep 08, 2017 at 10:48:10AM -0500, Brijesh Singh wrote:
> > > >  > 11. GO verifies the measurement and if measurement matches then 
> > > > it may
> > > >  >  give a secret blob -- which must be injected into the guest 
> > > > before
> > > >  >  libvirt starts the VM. If verification failed, GO will request 
> > > > cloud
> > > >  >  provider to destroy the VM.
> 
> I realised I'm missing something here: how does GO limit the
> secret to the specific VM? For example, what prevents hypervisor
> from launching two VMs with the same GO's DH, getting measurement
> from 1st one but injecting the secret into the second one?

Isn't that the 'trusted channel nonce currently associated with the
guest' in the guest context?

Dave

> Thanks,
> 
> -- 
> MST
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/6] domain: Allow 'model' attribute for ide controller.

[Daniel P. Berrange]

On Wed, Oct 18, 2017 at 12:23:24PM -0400, Dawid Zamirski wrote:
> On Tue, 2017-10-17 at 15:46 -0400, John Ferlan wrote:
> > 
> > On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
> > > From: Dawid Zamirski 
> > > 
> > > The optional values are 'piix3', 'piix4' or 'ich6'. Those will be
> > > needed to allow setting IDE controller model in VirtualBox driver.
> > > ---
> > >  docs/schemas/domaincommon.rng | 18 --
> > >  src/conf/domain_conf.c|  9 +
> > >  src/conf/domain_conf.h|  9 +
> > >  src/libvirt_private.syms  |  2 ++
> > >  4 files changed, 36 insertions(+), 2 deletions(-)
> > > 
> > 
> > Patches 2 & 3 should be combined and you'll need to provide an
> > xml2xml
> > example here, modifying tests/qemuxml2xmltest.c in order to add your
> > test. Search for controller type='scsi' and model= being set in any
> > of
> > the tests/*/*.xml files.  Then generate your test that would have
> > controller type='ide' ... model='xxx' (just one example is fine).
> > 
> 
> I understand that the patch with test cases should be separated,
> corrent?
> 
> > You may also need to alter virDomainControllerDefValidate to ensure
> > perhaps some existing assumptions on VIR_DOMAIN_CONTROLLER_TYPE_IDE
> > aren't lost if ->model is filled in.
> > 
> 
> That's clear, no problem...
> 
> > I am far from being an expert on IDE controllers and their existing
> > assumptions, but if you search on VIR_DOMAIN_CONTROLLER_TYPE_IDE
> > references you will find the existing ones.  My assumption has been
> > that
> > the current model assumption is piix3 - so by adding piix4 and ich6
> > you'll need to alter code in order to handle any assumptions those
> > models have; otherwise, once code finds IDE it assumes piix3 and
> > those
> > assumptions may not work well for piix4 and ich6.
> > 
> 
> ...though, I'm a little concerned with this part. While I'm somewhat
> familiar with libvirt qemu driver and can confirm that it implies PIIX3
> for IDE (at least libvirt does by checking if emulated machine is 440FX
> although qemu itself seems to support PIIX4 as well [1]), I'm not so
> sure I could easily determine "defaults" for other drivers like ESX
> (especially this one being closed-source) - AFAIK those drivers
> basically allow to attach "an IDE controller" without any specifics on
> what particular model of the southbridge the hypervisor should emulate

QEMU 'pc' machine type provides a piix3  IDE controller built-in,
and we can't (at time this) override this default.

You can then add extra IDE controllers which can be piix3 or piix4,
but we don't have that wired up for QEMU driver yet. It looks like
your proposed XML changes would let us do that though, which is nice.

> - I guess it's likely determined by the "machine" type or hwVersion
> (ESX)  and I suppose vbox is an "oddball" here allowing to set IDE
> controller model independently. Would it be acceptable to update each 
> driver to check that ->model == -1 and error out if it's not? In other
> words, qemu would allow model {-1,piix3}, vbox {-1,piix3,piix4,ich6}
> everything else would accept just -1 - guess those could be enforced
> by implementing virDomainDeviceDefValidateCallback in each driver.

vbox isn't really an odd-ball actually. vbox has as machine type of
piix3 by default if you look at the 'chipset' docs here:

  https://www.virtualbox.org/manual/ch03.html#settings-motherboard

the IDE controller model setting looks like it overrides the model
of this built-in IDE controller. This is slightly more flexible
than what QEMU does, in that you can overrde the default. Both
QEMU and VBox ways work.


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 2/6] domain: Allow 'model' attribute for ide controller.

[John Ferlan]

On 10/18/2017 12:23 PM, Dawid Zamirski wrote:
> On Tue, 2017-10-17 at 15:46 -0400, John Ferlan wrote:
>>
>> On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
>>> From: Dawid Zamirski 
>>>
>>> The optional values are 'piix3', 'piix4' or 'ich6'. Those will be
>>> needed to allow setting IDE controller model in VirtualBox driver.
>>> ---
>>>  docs/schemas/domaincommon.rng | 18 --
>>>  src/conf/domain_conf.c|  9 +
>>>  src/conf/domain_conf.h|  9 +
>>>  src/libvirt_private.syms  |  2 ++
>>>  4 files changed, 36 insertions(+), 2 deletions(-)
>>>
>>
>> Patches 2 & 3 should be combined and you'll need to provide an
>> xml2xml
>> example here, modifying tests/qemuxml2xmltest.c in order to add your
>> test. Search for controller type='scsi' and model= being set in any
>> of
>> the tests/*/*.xml files.  Then generate your test that would have
>> controller type='ide' ... model='xxx' (just one example is fine).
>>
> 
> I understand that the patch with test cases should be separated,
> corrent?
> 

Hmmm Typically when one makes .rng and domain_conf changes, then I
expect to also see *.html.in and tests/qemuxml2xmltest.c changes as well
as including the new .xml files in the input/output directories
(tests/qemuxml2argvdata and tests/qemuxml2xmloutdata). But this isn't
qemu So I guess I'm too used to reviewing qemu changes!

Since you're adding this for vbox and there isn't the same type xml
validation test for that (I see only tests/vboxsnapshotxmltest.c) that
just means you have less to do now. BTW: You will note there are
bhyvexml2xmltest and lxcxml2xmltests, but nothing for vbox, so I guess
it's a moot point since there's no way to test.  Hey it is something to
add some day in the future if you or someone else is so inclined.

FWIW: If you made qemu driver changes, such as qemu_command.c - then one
would change the qemuxml2argvtest.c and create a .args output file.

>> You may also need to alter virDomainControllerDefValidate to ensure
>> perhaps some existing assumptions on VIR_DOMAIN_CONTROLLER_TYPE_IDE
>> aren't lost if ->model is filled in.
>>
> 
> That's clear, no problem...
> 

Of course perhaps a moot point too ...

>> I am far from being an expert on IDE controllers and their existing
>> assumptions, but if you search on VIR_DOMAIN_CONTROLLER_TYPE_IDE
>> references you will find the existing ones.  My assumption has been
>> that
>> the current model assumption is piix3 - so by adding piix4 and ich6
>> you'll need to alter code in order to handle any assumptions those
>> models have; otherwise, once code finds IDE it assumes piix3 and
>> those
>> assumptions may not work well for piix4 and ich6.
>>
> 
> ...though, I'm a little concerned with this part. While I'm somewhat
> familiar with libvirt qemu driver and can confirm that it implies PIIX3
> for IDE (at least libvirt does by checking if emulated machine is 440FX
> although qemu itself seems to support PIIX4 as well [1]), I'm not so
> sure I could easily determine "defaults" for other drivers like ESX
> (especially this one being closed-source) - AFAIK those drivers
> basically allow to attach "an IDE controller" without any specifics on
> what particular model of the southbridge the hypervisor should emulate
> - I guess it's likely determined by the "machine" type or hwVersion
> (ESX)  and I suppose vbox is an "oddball" here allowing to set IDE
> controller model independently. Would it be acceptable to update each 
> driver to check that ->model == -1 and error out if it's not? In other
> words, qemu would allow model {-1,piix3}, vbox {-1,piix3,piix4,ich6}
> everything else would accept just -1 - guess those could be enforced
> by implementing virDomainDeviceDefValidateCallback in each driver.
> 
> 
> [1] https://github.com/qemu/qemu/blob/master/hw/ide/piix.c#L285
> 

Well, again, it's my qemu review bias clearly showing through. The qemu
code will know nothing about models so it'll just happily ignore the
model type (at least for now until someday someone decides differently).

So, how to handle this?  Hmmm...  Well it seems there's perhaps nothing
to do unless there is some need/desire to ensure the  has
specific values in which case you'd probably need that device def
callback specific to vbox similarly to the qemu device def callback.

One other thought - thinking about patch3 and formatdomain.html.in - you
should indicate there that model types are only supported and handled
for the vbox hypervisor only - there are other examples of that already.
For example,

Since 3.9.0 for the vbox driver, the
ide controller has an optional attribute
model, which is one of "piix3", "piix4", or "ich6".

I assume if the model='xxx' isn't provided, then some default is chosen.

Sorry for the confusion -

John

>>> diff --git a/docs/schemas/domaincommon.rng
>>> b/docs/schemas/domaincommon.rng
>>> index 4dbda6932..c3f1557f0 100644
>>> --- a/docs/schemas/domaincommon.rng
>>> +++ 

[libvirt] [libvirt-jenkins-ci PATCH v2 2/6] guests: Introduce lcitool

[Andrea Bolognani]

This script replaces the existing Makefile, and will be extended
to provide more functionality in future commits.

It also takes over ownership of the Ansible vault password, which
is now expected to be stored in lcitool's own config directory
along with more settings that will be introduced later.

Signed-off-by: Andrea Bolognani 
---
 guests/Makefile| 12 -
 guests/ansible.cfg |  1 -
 guests/lcitool | 76 ++
 3 files changed, 76 insertions(+), 13 deletions(-)
 delete mode 100644 guests/Makefile
 create mode 100755 guests/lcitool

diff --git a/guests/Makefile b/guests/Makefile
deleted file mode 100644
index 39ebe52..000
--- a/guests/Makefile
+++ /dev/null
@@ -1,12 +0,0 @@
-all:
-
-site:
-   @ansible-playbook site.yml
-
-bootstrap:
-   @ansible-playbook --ask-pass bootstrap.yml
-
-clean:
-   @rm -f *.retry log
-
-.PHONY: all site bootstrap clean
diff --git a/guests/ansible.cfg b/guests/ansible.cfg
index 84fde77..6b18c57 100644
--- a/guests/ansible.cfg
+++ b/guests/ansible.cfg
@@ -5,7 +5,6 @@ inventory = ./inventory
 log_path = ./log
 nocows = 1
 squash_actions = package
-vault_password_file = ~/.ansible/libvirt-jenkins-ci.vault-password
 
 [ssh_connection]
 pipelining = True
diff --git a/guests/lcitool b/guests/lcitool
new file mode 100755
index 000..aaee5f9
--- /dev/null
+++ b/guests/lcitool
@@ -0,0 +1,76 @@
+#!/bin/sh
+
+# ---
+#  Utility functions
+# ---
+
+# die MESSAGE
+#
+# Abort the program after displaying $MESSAGE on standard error.
+die() {
+echo "$1" >&2
+exit 1
+}
+
+# --
+#  User-visible actions
+# --
+
+do_help() {
+echo "\
+Usage: $CALL_NAME ACTION [OPTIONS]
+
+Actions:
+  list List known guests
+  prepare GUEST|allPrepare or update GUEST. Can be run multiple times
+  update  GUEST|allAlias for prepare
+  help Display this help"
+}
+
+do_list() {
+# List all guests present in the inventory. Skip group names,
+# comments and empty lines
+grep -vE '^#|^\[|^$' inventory | sort -u
+}
+
+do_prepare() {
+GUEST="$1"
+
+test "$GUEST" || {
+die "$(do_help)"
+}
+do_list | grep -q "$GUEST" || test "$GUEST" = all || {
+die "$PROGRAM_NAME: $GUEST: Unknown guest"
+}
+
+VAULT_PASS_FILE="$CONFIG_DIR/vault-password"
+
+# Make sure required passwords exist and are not invalid (empty)
+test -f "$VAULT_PASS_FILE" && test "$(cat "$VAULT_PASS_FILE")" || {
+die "$PROGRAM_NAME: $VAULT_PASS_FILE: Missing or invalid password"
+}
+
+ansible-playbook \
+--vault-password-file "$VAULT_PASS_FILE" \
+-l "$GUEST" \
+site.yml
+}
+
+# -
+#  Program entry point
+# -
+
+CALL_NAME="$0"
+PROGRAM_NAME="${0##*/}"
+CONFIG_DIR="$HOME/.config/$PROGRAM_NAME"
+
+test -f "$PROGRAM_NAME" || {
+die "$PROGRAM_NAME: Must be run from the source directory"
+}
+
+case "$1" in
+list)   do_list ;;
+prepare|update) do_prepare "$2" ;;
+*help)  do_help ;;
+*)  die "$(do_help)" ;;
+esac
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [libvirt-jenkins-ci PATCH v2 6/6] guests: Update documentation

[Andrea Bolognani]

Signed-off-by: Andrea Bolognani 
---
 guests/README.markdown | 91 +++---
 1 file changed, 64 insertions(+), 27 deletions(-)

diff --git a/guests/README.markdown b/guests/README.markdown
index 4d464e1..100ca31 100644
--- a/guests/README.markdown
+++ b/guests/README.markdown
@@ -1,38 +1,55 @@
-Ansible playbooks for libvirt CI
-
+libvirt CI - guest management tools
+===
 
-These can be used to turn a freshly installed machine into a worker for
-the Jenkins-based libvirt CI.
+The tools contained in this directory simplify and automate the management
+of the guests used by the Jenkins-based libvirt CI environment.
 
-There are two main playbooks:
+There are two steps to bringing up a guest:
 
-* `bootstrap.yml`, used to perform the bootstrapping phase, that is, getting
-  guests to the point where Ansible can manage them fully and prompting the
-  user for a password is no longer required;
+* `./lcitool install $guest` will perform an unattended installation
+  of `$guest`. Not all guests can be installed this way: see the "FreeBSD"
+  section below;
 
-* `site.yml`, used for the remaining configuration steps.
+* `./lcitool prepare $guest` will go through all the post-installation
+  configuration steps required to make the newly-created guest usable as
+  part of the Jenkins CI setup.
 
-Although you can use the playbooks directly, it's much more convenient to
-call either `make bootstrap` or `make site` instead.
+Once those steps have been performed, maintainance will involve running:
 
-Each guest only needs to be bootstrapped once; that said, both playbooks are
-idempotent so there's no harm in applying them over and over again.
+* `./lcitool update $guest`
 
+periodically to ensure the guest configuration is sane and all installed
+packages are updated.
 
-Requirements
-
 
-SSH must be running in the guest, and root login must be permitted.
+Host setup
+--
 
+Ansible and `virt-install` need to be available on the host.
 
-CI use
---
+Before you can start bringing up guests, you'll have to store your
+site-specific root password in the `~/.config/lcitool/root-password` file.
+This password will only be necessary for serial console access in case
+something goes horribly wrong; for day to day operations, SSH key
+authentication will be used instead.
 
-After you have reinstalled a Jenkins worker, run `make bootstrap` followed
-by `make site` and a reboot to get it ready for CI use. No further action
-should be necessary.
+Ansible expects to be able to connect to the guests by name: installing and
+enabling the [libvirt NSS plugin](https://wiki.libvirt.org/page/NSS_module)
+on the host is the easiest way to make sure that works. More specifically,
+you'll want to use the `libvirt_guest` variant of the plugin.
 
-Adding new workers will require tweaking the inventory and host variables,
+To keep guests up to date over time, it's recommended to have an entry
+along the lines of
+
+0 0 * * * cd ~/libvirt-jenkins-ci/guests && ./lcitool update all
+
+in your crontab.
+
+
+Adding new guests
+-
+
+Adding new guests will require tweaking the inventory and host variables,
 but it should be very easy to eg. use the Fedora 26 configuration to come
 up with a working Fedora 27 configuration.
 
@@ -40,11 +57,11 @@ up with a working Fedora 27 configuration.
 Development use
 ---
 
-If you are a developer trying to reproduce a bug on some OS you don't have
-easy access to, you can use these playbooks to create a suitable test
+If you are a developer trying to reproduce a bug on some OS you don't
+have easy access to, you can use these tools to create a suitable test
 environment.
 
-Since the playbooks are intended mainly for CI use, you'll have to tweak them
+Since the tools are intended mainly for CI use, you'll have to tweak them
 a bit first, including:
 
 * trimming down the `inventory` file to just the guest you're interested in;
@@ -56,5 +73,25 @@ a bit first, including:
 
 * deleting `host_vars/$guest/vault.yml` altogether.
 
-After performing these tweaks, you should be able to just run `make bootstrap`
-followed by `make site` as usual.
+After performing these tweaks, you should be able to use the same steps
+outlined above.
+
+A better way to deal with this use case will be provided in the future.
+
+
+FreeBSD
+---
+
+Installation of FreeBSD guests must be performed manually; alternatively,
+the official qcow2 images can be used to quickly bring up such guests.
+
+Some manual tweaking will be needed, in particular:
+
+* `/etc/ssh/sshd_config` must contain the `PermitRootLogin yes` directive;
+
+* `/etc/rc.conf` must contain the `sshd_enable="YES"` setting;
+
+* the root password must be manually set to "root" (without quotes).
+
+Once these steps have been performed, FreeBSD guests can be managed just
+like all other guests.

[libvirt] [libvirt-jenkins-ci PATCH v2 3/6] guests: Remove bootstrap phase

[Andrea Bolognani]

Having to bootstrap the guest as a separate phase is annoying and
can be avoided by assuming the root password is well-known.

This doesn't hurt security because we're going to be changing the
root password with a user-provided one the first time Ansible is
run; moreover, we only leave key-based SSH authentication enabled
for the root user.

Signed-off-by: Andrea Bolognani 
---
 guests/bootstrap.yml   | 15 ---
 guests/group_vars/all/main.yml |  5 +
 guests/lcitool | 25 +
 guests/site.yml|  8 
 guests/tasks/base.yml  | 11 ++-
 5 files changed, 48 insertions(+), 16 deletions(-)
 delete mode 100644 guests/bootstrap.yml

diff --git a/guests/bootstrap.yml b/guests/bootstrap.yml
deleted file mode 100644
index 544dd9d..000
--- a/guests/bootstrap.yml
+++ /dev/null
@@ -1,15 +0,0 @@

-- hosts: all
-  gather_facts: no
-
-  tasks:
-
-# Bootstrap Ansible itself
-- include: tasks/bootstrap.yml
-
-- hosts: all
-
-  tasks:
-
-# Prepare the base environment
-- include: tasks/base.yml
diff --git a/guests/group_vars/all/main.yml b/guests/group_vars/all/main.yml
index 81b7d43..d24af59 100644
--- a/guests/group_vars/all/main.yml
+++ b/guests/group_vars/all/main.yml
@@ -1,6 +1,11 @@
 ---
 ansible_user: root
 
+# This password is only used to access the guest the very first time
+# Ansible is used: afterwards, the user's SSH key will have been installed
+# in the guest and SSH password authentication will have been disabled
+ansible_ssh_pass: root
+
 jenkins_url: https://ci.centos.org/computer/{{ inventory_hostname 
}}/slave-agent.jnlp
 
 # Paths to various command. Can be overridden on a per-host basis
diff --git a/guests/lcitool b/guests/lcitool
index aaee5f9..10a72cf 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -12,6 +12,19 @@ die() {
 exit 1
 }
 
+# hash_file PASS_FILE
+#
+# Generate a password hash from the contents of PASS_FILE.
+hash_file() {
+PASS_FILE="$1"
+
+python2 -c "
+import crypt
+password = open('$PASS_FILE', 'r').read().strip()
+print(crypt.crypt(password,
+  crypt.mksalt(crypt.METHOD_SHA512)))"
+}
+
 # --
 #  User-visible actions
 # --
@@ -44,11 +57,23 @@ do_prepare() {
 }
 
 VAULT_PASS_FILE="$CONFIG_DIR/vault-password"
+ROOT_PASS_FILE="$CONFIG_DIR/root-password"
 
 # Make sure required passwords exist and are not invalid (empty)
 test -f "$VAULT_PASS_FILE" && test "$(cat "$VAULT_PASS_FILE")" || {
 die "$PROGRAM_NAME: $VAULT_PASS_FILE: Missing or invalid password"
 }
+test -f "$ROOT_PASS_FILE" && test "$(cat "$ROOT_PASS_FILE")" || {
+die "$PROGRAM_NAME: $ROOT_PASS_FILE: Missing or invalid password"
+}
+
+ROOT_HASH_FILE="$CONFIG_DIR/.root-password.hash"
+
+# Regenerate root password hash. Ansible expects passwords as hashes but
+# doesn't provide a built-in facility to generate one from plain text
+hash_file "$ROOT_PASS_FILE" >"$ROOT_HASH_FILE" || {
+die "$PROGRAM_NAME: Failure while hashing root password"
+}
 
 ansible-playbook \
 --vault-password-file "$VAULT_PASS_FILE" \
diff --git a/guests/site.yml b/guests/site.yml
index e6cf10d..9c75dcb 100644
--- a/guests/site.yml
+++ b/guests/site.yml
@@ -1,5 +1,13 @@
 ---
 - hosts: all
+  gather_facts: no
+
+  tasks:
+
+# Bootstrap Ansible itself
+- include: tasks/bootstrap.yml
+
+- hosts: all
 
   vars_files:
 - vars/mappings.yml
diff --git a/guests/tasks/base.yml b/guests/tasks/base.yml
index dd8d306..a9066e4 100644
--- a/guests/tasks/base.yml
+++ b/guests/tasks/base.yml
@@ -96,9 +96,10 @@
   hostname:
 name: '{{ inventory_hostname }}'
 
-- name: Configure root shell
+- name: Configure root password and shell
   user:
 name: root
+password: '{{ lookup("file", lookup("env", "HOME") + 
"/.config/lcitool/.root-password.hash") }}'
 shell: '{{ bash }}'
 
 - name: Configure ssh access for the root user
@@ -106,3 +107,11 @@
 user: root
 key: '{{ lookup("file", lookup("env", "HOME") + "/.ssh/id_rsa.pub") }}'
 state: present
+
+- name: Disable password authentication for the root user
+  lineinfile:
+path: /etc/ssh/sshd_config
+regexp: '^#*\s*PermitRootLogin\s*.*$'
+line: 'PermitRootLogin without-password'
+state: present
+backup: yes
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [libvirt-jenkins-ci PATCH v2 5/6] guests: Configure bootloaders

[Andrea Bolognani]

Both GRUB and the FreeBSD bootloader need some tweaking to make
sure the OS will display boot messages and provide a login prompt
on the serial console, which is useful when SSH access can't be
used for whatever reason.

Signed-off-by: Andrea Bolognani 
---
 guests/tasks/base.yml | 50 ++
 1 file changed, 50 insertions(+)

diff --git a/guests/tasks/base.yml b/guests/tasks/base.yml
index a9066e4..db805b8 100644
--- a/guests/tasks/base.yml
+++ b/guests/tasks/base.yml
@@ -115,3 +115,53 @@
 line: 'PermitRootLogin without-password'
 state: present
 backup: yes
+
+- name: Look for GRUB configuration
+  stat:
+path: /etc/default/grub
+  register: grubdefault
+
+- name: Look for GRUB configuration
+  stat:
+path: /boot/grub/grub.cfg
+  register: grubcfg
+
+- name: Look for GRUB configuration
+  stat:
+path: /boot/grub2/grub.cfg
+  register: grub2cfg
+
+- name: Configure GRUB
+  lineinfile:
+path: /etc/default/grub
+regexp: '^{{ item.key }}=.*$'
+line: '{{ item.key }}="{{ item.value }}"'
+backup: yes
+  with_items:
+- { key: 'GRUB_TIMEOUT', value: '1' }
+- { key: 'GRUB_CMDLINE_LINUX_DEFAULT', value: 'console=ttyS0' }
+- { key: 'GRUB_CMDLINE_LINUX', value: 'console=ttyS0' }
+- { key: 'GRUB_TERMINAL', value: 'serial' }
+- { key: 'GRUB_SERIAL_COMMAND', value: 'serial' }
+  when:
+- grubdefault.stat.exists
+
+- name: Apply GRUB configuration
+  command: 'grub-mkconfig -o /boot/grub/grub.cfg'
+  when:
+- grubcfg.stat.exists
+
+- name: Apply GRUB configuration
+  command: 'grub2-mkconfig -o /boot/grub2/grub.cfg'
+  when:
+- grub2cfg.stat.exists
+
+- name: Configure the FreeBSD bootloader
+  lineinfile:
+path: /boot/loader.conf
+regexp: '^console=.*$'
+line: 'console="comconsole"'
+create: yes
+backup: yes
+  when:
+- os_name == 'FreeBSD'
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [libvirt-jenkins-ci PATCH v2 1/6] guests: Rename from 'ansible'

[Andrea Bolognani]

There will be more than just Ansible playbooks in that directory
pretty soon, so a more generic name is more appropriate.

Signed-off-by: Andrea Bolognani 
---
 {ansible => guests}/.gitignore | 0
 {ansible => guests}/Makefile   | 0
 {ansible => guests}/README.markdown| 0
 {ansible => guests}/ansible.cfg| 0
 {ansible => guests}/bootstrap.yml  | 0
 {ansible => guests}/group_vars/all/main.yml| 0
 {ansible => guests}/host_vars/libvirt-centos-6/main.yml| 0
 {ansible => guests}/host_vars/libvirt-centos-6/vault.yml   | 0
 {ansible => guests}/host_vars/libvirt-centos-7/main.yml| 0
 {ansible => guests}/host_vars/libvirt-centos-7/vault.yml   | 0
 {ansible => guests}/host_vars/libvirt-debian-8/main.yml| 0
 {ansible => guests}/host_vars/libvirt-debian-8/vault.yml   | 0
 {ansible => guests}/host_vars/libvirt-debian-9/main.yml| 0
 {ansible => guests}/host_vars/libvirt-debian-9/vault.yml   | 0
 {ansible => guests}/host_vars/libvirt-fedora-25/main.yml   | 0
 {ansible => guests}/host_vars/libvirt-fedora-25/vault.yml  | 0
 {ansible => guests}/host_vars/libvirt-fedora-26/main.yml   | 0
 {ansible => guests}/host_vars/libvirt-fedora-26/vault.yml  | 0
 {ansible => guests}/host_vars/libvirt-fedora-rawhide/main.yml  | 0
 {ansible => guests}/host_vars/libvirt-fedora-rawhide/vault.yml | 0
 {ansible => guests}/host_vars/libvirt-freebsd-10/main.yml  | 0
 {ansible => guests}/host_vars/libvirt-freebsd-10/vault.yml | 0
 {ansible => guests}/host_vars/libvirt-freebsd-11/main.yml  | 0
 {ansible => guests}/host_vars/libvirt-freebsd-11/vault.yml | 0
 {ansible => guests}/host_vars/libvirt-ubuntu-12/main.yml   | 0
 {ansible => guests}/host_vars/libvirt-ubuntu-12/vault.yml  | 0
 {ansible => guests}/host_vars/libvirt-ubuntu-14/main.yml   | 0
 {ansible => guests}/host_vars/libvirt-ubuntu-14/vault.yml  | 0
 {ansible => guests}/host_vars/libvirt-ubuntu-16/main.yml   | 0
 {ansible => guests}/host_vars/libvirt-ubuntu-16/vault.yml  | 0
 {ansible => guests}/inventory  | 0
 {ansible => guests}/site.yml   | 0
 {ansible => guests}/tasks/base.yml | 0
 {ansible => guests}/tasks/bootstrap.yml| 0
 {ansible => guests}/tasks/compat.yml   | 0
 {ansible => guests}/tasks/jenkins.yml  | 0
 {ansible => guests}/tasks/packages.yml | 0
 {ansible => guests}/templates/jenkins.service.j2   | 0
 {ansible => guests}/vars/mappings.yml  | 0
 {ansible => guests}/vars/projects/base.yml | 0
 {ansible => guests}/vars/projects/jenkins.yml  | 0
 {ansible => guests}/vars/projects/libosinfo.yml| 0
 {ansible => guests}/vars/projects/libvirt-cim.yml  | 0
 {ansible => guests}/vars/projects/libvirt-glib.yml | 0
 {ansible => guests}/vars/projects/libvirt-go-xml.yml   | 0
 {ansible => guests}/vars/projects/libvirt-go.yml   | 0
 {ansible => guests}/vars/projects/libvirt-perl.yml | 0
 {ansible => guests}/vars/projects/libvirt-python.yml   | 0
 {ansible => guests}/vars/projects/libvirt-sandbox.yml  | 0
 {ansible => guests}/vars/projects/libvirt-tck.yml  | 0
 {ansible => guests}/vars/projects/libvirt.yml  | 0
 {ansible => guests}/vars/projects/osinfo-db-tools.yml  | 0
 {ansible => guests}/vars/projects/osinfo-db.yml| 0
 {ansible => guests}/vars/projects/virt-manager.yml | 0
 {ansible => guests}/vars/projects/virt-viewer.yml  | 0
 55 files changed, 0 insertions(+), 0 deletions(-)
 rename {ansible => guests}/.gitignore (100%)
 rename {ansible => guests}/Makefile (100%)
 rename {ansible => guests}/README.markdown (100%)
 rename {ansible => guests}/ansible.cfg (100%)
 rename {ansible => guests}/bootstrap.yml (100%)
 rename {ansible => guests}/group_vars/all/main.yml (100%)
 rename {ansible => guests}/host_vars/libvirt-centos-6/main.yml (100%)
 rename {ansible => guests}/host_vars/libvirt-centos-6/vault.yml (100%)
 rename {ansible => guests}/host_vars/libvirt-centos-7/main.yml (100%)
 rename {ansible => guests}/host_vars/libvirt-centos-7/vault.yml (100%)
 rename {ansible => guests}/host_vars/libvirt-debian-8/main.yml (100%)
 rename {ansible => guests}/host_vars/libvirt-debian-8/vault.yml (100%)
 rename {ansible => guests}/host_vars/libvirt-debian-9/main.yml (100%)
 rename {ansible => guests}/host_vars/libvirt-debian-9/vault.yml (100%)
 rename {ansible => guests}/host_vars/libvirt-fedora-25/main.yml (100%)
 rename {ansible => guests}/host_vars/libvirt-fedora-25/vault.yml (100%)
 rename 

[libvirt] [libvirt-jenkins-ci PATCH v2 0/6] Unattended guest installation and more

[Andrea Bolognani]

Changes from [v1]:

* guest changes
  - set root password to a user-provided value
  - only allow key-based SSH login for root
  - configure serial console for emergency access
  - disable audio, video and USB
  - allocate 256 MiB for swap instead of 2 GiB
  - allocate 15 GiB for storage instead of 10 GiB
  - use host-passthrough CPU mode
  - set timezone to UTC

* tool changes
  - rename to lcitool
  - implement basic configuration support
  - refuse to run from outside the source directory
  - provide better error messages

* others
  - improve documentation, mainly the host setup part


[v1] https://www.redhat.com/archives/libvir-list/2017-October/msg00651.html

Andrea Bolognani (6):
  guests: Rename from 'ansible'
  guests: Introduce lcitool
  guests: Remove bootstrap phase
  guests: Add unattended installation support
  guests: Configure bootloaders
  guests: Update documentation

 ansible/Makefile   |  12 --
 ansible/README.markdown|  60 ---
 ansible/bootstrap.yml  |  15 --
 {ansible => guests}/.gitignore |   0
 guests/README.markdown |  97 +++
 {ansible => guests}/ansible.cfg|   1 -
 guests/group_vars/all/install.yml  |  11 ++
 {ansible => guests}/group_vars/all/main.yml|   5 +
 guests/host_vars/libvirt-centos-6/install.yml  |   3 +
 .../host_vars/libvirt-centos-6/main.yml|   0
 .../host_vars/libvirt-centos-6/vault.yml   |   0
 guests/host_vars/libvirt-centos-7/install.yml  |   3 +
 .../host_vars/libvirt-centos-7/main.yml|   0
 .../host_vars/libvirt-centos-7/vault.yml   |   0
 guests/host_vars/libvirt-debian-8/install.yml  |   3 +
 .../host_vars/libvirt-debian-8/main.yml|   0
 .../host_vars/libvirt-debian-8/vault.yml   |   0
 guests/host_vars/libvirt-debian-9/install.yml  |   3 +
 .../host_vars/libvirt-debian-9/main.yml|   0
 .../host_vars/libvirt-debian-9/vault.yml   |   0
 guests/host_vars/libvirt-fedora-25/install.yml |   3 +
 .../host_vars/libvirt-fedora-25/main.yml   |   0
 .../host_vars/libvirt-fedora-25/vault.yml  |   0
 guests/host_vars/libvirt-fedora-26/install.yml |   3 +
 .../host_vars/libvirt-fedora-26/main.yml   |   0
 .../host_vars/libvirt-fedora-26/vault.yml  |   0
 .../host_vars/libvirt-fedora-rawhide/install.yml   |   3 +
 .../host_vars/libvirt-fedora-rawhide/main.yml  |   0
 .../host_vars/libvirt-fedora-rawhide/vault.yml |   0
 .../host_vars/libvirt-freebsd-10/main.yml  |   0
 .../host_vars/libvirt-freebsd-10/vault.yml |   0
 .../host_vars/libvirt-freebsd-11/main.yml  |   0
 .../host_vars/libvirt-freebsd-11/vault.yml |   0
 guests/host_vars/libvirt-ubuntu-12/install.yml |   3 +
 .../host_vars/libvirt-ubuntu-12/main.yml   |   0
 .../host_vars/libvirt-ubuntu-12/vault.yml  |   0
 guests/host_vars/libvirt-ubuntu-14/install.yml |   3 +
 .../host_vars/libvirt-ubuntu-14/main.yml   |   0
 .../host_vars/libvirt-ubuntu-14/vault.yml  |   0
 guests/host_vars/libvirt-ubuntu-16/install.yml |   3 +
 .../host_vars/libvirt-ubuntu-16/main.yml   |   0
 .../host_vars/libvirt-ubuntu-16/vault.yml  |   0
 {ansible => guests}/inventory  |   0
 guests/kickstart.cfg   |  60 +++
 guests/lcitool | 183 +
 guests/preseed.cfg |  85 ++
 {ansible => guests}/site.yml   |   8 +
 {ansible => guests}/tasks/base.yml |  61 ++-
 {ansible => guests}/tasks/bootstrap.yml|   0
 {ansible => guests}/tasks/compat.yml   |   0
 {ansible => guests}/tasks/jenkins.yml  |   0
 {ansible => guests}/tasks/packages.yml |   0
 {ansible => guests}/templates/jenkins.service.j2   |   0
 {ansible => guests}/vars/mappings.yml  |   0
 {ansible => guests}/vars/projects/base.yml |   0
 {ansible => guests}/vars/projects/jenkins.yml  |   0
 {ansible => guests}/vars/projects/libosinfo.yml|   0
 {ansible => guests}/vars/projects/libvirt-cim.yml  |   0
 {ansible => guests}/vars/projects/libvirt-glib.yml |   0
 .../vars/projects/libvirt-go-xml.yml   |   0
 {ansible => guests}/vars/projects/libvirt-go.yml   |   0
 {ansible => guests}/vars/projects/libvirt-perl.yml |   0
 .../vars/projects/libvirt-python.yml   |   0
 .../vars/projects/libvirt-sandbox.yml  |   0
 {ansible => guests}/vars/projects/libvirt-tck.yml  |   0
 {ansible => guests}/vars/projects/libvirt.yml  |   0
 .../vars/projects/osinfo-db-tools.yml  |   0
 {ansible => guests}/vars/projects/osinfo-db.yml|   0
 {ansible => guests}/vars/projects/virt-manager.yml 

[libvirt] [libvirt-jenkins-ci PATCH v2 4/6] guests: Add unattended installation support

[Andrea Bolognani]

The lcitool script can now be used to install most known guests
without requiring user interaction.

Signed-off-by: Andrea Bolognani 
---
 guests/group_vars/all/install.yml  | 11 +++
 guests/host_vars/libvirt-centos-6/install.yml  |  3 +
 guests/host_vars/libvirt-centos-7/install.yml  |  3 +
 guests/host_vars/libvirt-debian-8/install.yml  |  3 +
 guests/host_vars/libvirt-debian-9/install.yml  |  3 +
 guests/host_vars/libvirt-fedora-25/install.yml |  3 +
 guests/host_vars/libvirt-fedora-26/install.yml |  3 +
 .../host_vars/libvirt-fedora-rawhide/install.yml   |  3 +
 guests/host_vars/libvirt-ubuntu-12/install.yml |  3 +
 guests/host_vars/libvirt-ubuntu-14/install.yml |  3 +
 guests/host_vars/libvirt-ubuntu-16/install.yml |  3 +
 guests/kickstart.cfg   | 60 +++
 guests/lcitool | 82 +
 guests/preseed.cfg | 85 ++
 14 files changed, 268 insertions(+)
 create mode 100644 guests/group_vars/all/install.yml
 create mode 100644 guests/host_vars/libvirt-centos-6/install.yml
 create mode 100644 guests/host_vars/libvirt-centos-7/install.yml
 create mode 100644 guests/host_vars/libvirt-debian-8/install.yml
 create mode 100644 guests/host_vars/libvirt-debian-9/install.yml
 create mode 100644 guests/host_vars/libvirt-fedora-25/install.yml
 create mode 100644 guests/host_vars/libvirt-fedora-26/install.yml
 create mode 100644 guests/host_vars/libvirt-fedora-rawhide/install.yml
 create mode 100644 guests/host_vars/libvirt-ubuntu-12/install.yml
 create mode 100644 guests/host_vars/libvirt-ubuntu-14/install.yml
 create mode 100644 guests/host_vars/libvirt-ubuntu-16/install.yml
 create mode 100644 guests/kickstart.cfg
 create mode 100644 guests/preseed.cfg

diff --git a/guests/group_vars/all/install.yml 
b/guests/group_vars/all/install.yml
new file mode 100644
index 000..94b752f
--- /dev/null
+++ b/guests/group_vars/all/install.yml
@@ -0,0 +1,11 @@
+---
+# Sizes are in GiB
+install_virt_type: kvm
+install_arch: x86_64
+install_machine: pc
+install_cpu_model: host-passthrough
+install_vcpus: 2
+install_memory_size: 2
+install_disk_size: 15
+install_storage_pool: default
+install_network: default
diff --git a/guests/host_vars/libvirt-centos-6/install.yml 
b/guests/host_vars/libvirt-centos-6/install.yml
new file mode 100644
index 000..3a9459b
--- /dev/null
+++ b/guests/host_vars/libvirt-centos-6/install.yml
@@ -0,0 +1,3 @@
+---
+install_url: http://mirror.centos.org/centos/6/os/x86_64/
+install_config: kickstart.cfg
diff --git a/guests/host_vars/libvirt-centos-7/install.yml 
b/guests/host_vars/libvirt-centos-7/install.yml
new file mode 100644
index 000..f003b89
--- /dev/null
+++ b/guests/host_vars/libvirt-centos-7/install.yml
@@ -0,0 +1,3 @@
+---
+install_url: http://mirror.centos.org/centos/7/os/x86_64/
+install_config: kickstart.cfg
diff --git a/guests/host_vars/libvirt-debian-8/install.yml 
b/guests/host_vars/libvirt-debian-8/install.yml
new file mode 100644
index 000..a2c8341
--- /dev/null
+++ b/guests/host_vars/libvirt-debian-8/install.yml
@@ -0,0 +1,3 @@
+---
+install_url: http://deb.debian.org/debian/dists/jessie/main/installer-amd64/
+install_config: preseed.cfg
diff --git a/guests/host_vars/libvirt-debian-9/install.yml 
b/guests/host_vars/libvirt-debian-9/install.yml
new file mode 100644
index 000..5b1da76
--- /dev/null
+++ b/guests/host_vars/libvirt-debian-9/install.yml
@@ -0,0 +1,3 @@
+---
+install_url: http://deb.debian.org/debian/dists/stretch/main/installer-amd64/
+install_config: preseed.cfg
diff --git a/guests/host_vars/libvirt-fedora-25/install.yml 
b/guests/host_vars/libvirt-fedora-25/install.yml
new file mode 100644
index 000..bb4bde3
--- /dev/null
+++ b/guests/host_vars/libvirt-fedora-25/install.yml
@@ -0,0 +1,3 @@
+---
+install_url: 
https://download.fedoraproject.org/pub/fedora/linux/releases/25/Server/x86_64/os
+install_config: kickstart.cfg
diff --git a/guests/host_vars/libvirt-fedora-26/install.yml 
b/guests/host_vars/libvirt-fedora-26/install.yml
new file mode 100644
index 000..eff160d
--- /dev/null
+++ b/guests/host_vars/libvirt-fedora-26/install.yml
@@ -0,0 +1,3 @@
+---
+install_url: 
https://download.fedoraproject.org/pub/fedora/linux/releases/26/Server/x86_64/os
+install_config: kickstart.cfg
diff --git a/guests/host_vars/libvirt-fedora-rawhide/install.yml 
b/guests/host_vars/libvirt-fedora-rawhide/install.yml
new file mode 100644
index 000..2216e81
--- /dev/null
+++ b/guests/host_vars/libvirt-fedora-rawhide/install.yml
@@ -0,0 +1,3 @@
+---
+install_url: 
https://download.fedoraproject.org/pub/fedora/linux/development/rawhide/Everything/x86_64/os
+install_config: kickstart.cfg
diff --git a/guests/host_vars/libvirt-ubuntu-12/install.yml 
b/guests/host_vars/libvirt-ubuntu-12/install.yml
new file mode 100644
index 000..997304f
--- /dev/null
+++ 

Re: [libvirt] [PATCH 2/6] domain: Allow 'model' attribute for ide controller.

[Dawid Zamirski]

On Tue, 2017-10-17 at 15:46 -0400, John Ferlan wrote:
> 
> On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
> > From: Dawid Zamirski 
> > 
> > The optional values are 'piix3', 'piix4' or 'ich6'. Those will be
> > needed to allow setting IDE controller model in VirtualBox driver.
> > ---
> >  docs/schemas/domaincommon.rng | 18 --
> >  src/conf/domain_conf.c|  9 +
> >  src/conf/domain_conf.h|  9 +
> >  src/libvirt_private.syms  |  2 ++
> >  4 files changed, 36 insertions(+), 2 deletions(-)
> > 
> 
> Patches 2 & 3 should be combined and you'll need to provide an
> xml2xml
> example here, modifying tests/qemuxml2xmltest.c in order to add your
> test. Search for controller type='scsi' and model= being set in any
> of
> the tests/*/*.xml files.  Then generate your test that would have
> controller type='ide' ... model='xxx' (just one example is fine).
> 

I understand that the patch with test cases should be separated,
corrent?

> You may also need to alter virDomainControllerDefValidate to ensure
> perhaps some existing assumptions on VIR_DOMAIN_CONTROLLER_TYPE_IDE
> aren't lost if ->model is filled in.
> 

That's clear, no problem...

> I am far from being an expert on IDE controllers and their existing
> assumptions, but if you search on VIR_DOMAIN_CONTROLLER_TYPE_IDE
> references you will find the existing ones.  My assumption has been
> that
> the current model assumption is piix3 - so by adding piix4 and ich6
> you'll need to alter code in order to handle any assumptions those
> models have; otherwise, once code finds IDE it assumes piix3 and
> those
> assumptions may not work well for piix4 and ich6.
> 

...though, I'm a little concerned with this part. While I'm somewhat
familiar with libvirt qemu driver and can confirm that it implies PIIX3
for IDE (at least libvirt does by checking if emulated machine is 440FX
although qemu itself seems to support PIIX4 as well [1]), I'm not so
sure I could easily determine "defaults" for other drivers like ESX
(especially this one being closed-source) - AFAIK those drivers
basically allow to attach "an IDE controller" without any specifics on
what particular model of the southbridge the hypervisor should emulate
- I guess it's likely determined by the "machine" type or hwVersion
(ESX)  and I suppose vbox is an "oddball" here allowing to set IDE
controller model independently. Would it be acceptable to update each 
driver to check that ->model == -1 and error out if it's not? In other
words, qemu would allow model {-1,piix3}, vbox {-1,piix3,piix4,ich6}
everything else would accept just -1 - guess those could be enforced
by implementing virDomainDeviceDefValidateCallback in each driver.


[1] https://github.com/qemu/qemu/blob/master/hw/ide/piix.c#L285

> > diff --git a/docs/schemas/domaincommon.rng
> > b/docs/schemas/domaincommon.rng
> > index 4dbda6932..c3f1557f0 100644
> > --- a/docs/schemas/domaincommon.rng
> > +++ b/docs/schemas/domaincommon.rng
> > @@ -1927,12 +1927,11 @@
> >
> >  
> >  
> > -  
> > +  
> >
> >  
> >
> >  fdc
> > -ide
> >  sata
> >  ccid
> >
> > @@ -1993,6 +1992,21 @@
> >
> >  
> >
> > +  
> > +  
> > +
> > +  ide
> > +
> > +
> > +  
> > +
> > +  piix3
> > +  piix4
> > +  ich6
> > +
> > +  
> > +
> > +  
> >
> >
> >  
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 54be9028d..493bf83ff 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -378,6 +378,11 @@ VIR_ENUM_IMPL(virDomainControllerModelUSB,
> > VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST,
> >"qemu-xhci",
> >"none")
> >  
> > +VIR_ENUM_IMPL(virDomainControllerModelIDE,
> > VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST,
> > +  "piix3",
> > +  "piix4",
> > +  "ich6")
> > +
> >  VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST,
> >"mount",
> >"block",
> > @@ -9467,6 +9472,8 @@ virDomainControllerModelTypeFromString(const
> > virDomainControllerDef *def,
> >  return virDomainControllerModelUSBTypeFromString(model);
> >  else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> >  return virDomainControllerModelPCITypeFromString(model);
> > +else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> > +return virDomainControllerModelIDETypeFromString(model);
> >  
> >  return -1;
> >  }
> > @@ -9482,6 +9489,8 @@
> > virDomainControllerModelTypeToString(virDomainControllerDefPtr def,
> >  return 

[libvirt] libvirt not wanting to read back its own interface XML

[Marc Haber]

Hi,

I'm trying to give libvirt knowlegde of a VLAN interface that is
configured by a different means and not by libvirt. To figure out the
syntax, I created a VLAN interface through virt-manager and dumped out
the XML:


  


  
  
  

  


I then removed the Interface in virt-manager and tried to have libvirt
read back its own XML.

[12/784]mh@testsid85:~ $ virsh iface-dumpxml ens3.180 > ens3.180.xml
[13/785]mh@testsid85:~ $ virsh iface-define ens3.180.xml
error: Failed to define interface from ens3.180.xml
error: XML error: could not get interface XML description: XML invalid - 
Expecting an element start, got nothing

What is going on here? How do I define a VLAN interface via XML?

Greetings
Marc

-- 
-
Marc Haber | "I don't trust Computers. They | Mailadresse im Header
Leimen, Germany|  lose things."Winona Ryder | Fon: *49 6224 1600402
Nordisch by Nature |  How to make an American Quilt | Fax: *49 6224 1600421

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [CFP] Virt & IaaS Devroom at FOSDEM18

[Stefan Hajnoczi]

I am excited to announce that the call for proposals is now open for the
Virtualization & IaaS devroom at the upcoming FOSDEM 2018, to be hosted
on February 3 and 4, 2018.

This year will mark FOSDEM’s 18th anniversary as one of the longest-running
free and open source software developer events, attracting thousands of
developers and users from all over the world. FOSDEM will be held once
again in Brussels, Belgium, on February 3 & 4, 2018.

This devroom is a collaborative effort, and is organized by dedicated
folks from projects such as OpenStack, Xen Project, oVirt, QEMU, KVM,
libvirt, and Foreman. We would like to invite all those who are involved
in these fields to submit your proposals by December 1st, 2017.

About the Devroom

The Virtualization & IaaS devroom will feature session topics such as open
source hypervisors and virtual machine managers such as Xen Project, KVM,
bhyve, and VirtualBox, and Infrastructure-as-a-Service projects such as
Apache CloudStack, OpenStack, oVirt, QEMU, OpenNebula, and Ganeti.

This devroom will host presentations that focus on topics of shared
interest, such as KVM; libvirt; shared storage; virtualized networking;
cloud security; clustering and high availability; interfacing with multiple
hypervisors; hyperconverged deployments; and scaling across hundreds or
thousands of servers.

Presentations in this devroom will be aimed at developers working on these
platforms who are looking to collaborate and improve shared infrastructure
or solve common problems. We seek topics that encourage dialog between
projects and continued work post-FOSDEM.

Important Dates

Submission deadline: 01 December 2017
Acceptance notifications: 14 December 2017
Final schedule announcement: 21 December 2017
Devroom: 03 and 04 February 2018 (two days- different rooms)

Submit Your Proposal

All submissions must be made via the Pentabarf event planning site[1]. If
you have not used Pentabarf before, you will need to create an account. If
you submitted proposals for FOSDEM in previous years, you can use your
existing account.

After creating the account, select Create Event to start the submission
process. Make sure to select Virtualization and IaaS devroom from the Track
list. Please fill out all the required fields, and provide a meaningful
abstract and description of your proposed session.

Submission Guidelines

We expect more proposals than we can possibly accept, so it is vitally
important that you submit your proposal on or before the deadline. Late
submissions are unlikely to be considered.

All presentation slots are 45 minutes, with 35 minutes planned for
presentations, and 10 minutes for Q

All presentations will be recorded and made available under Creative
Commons licenses. In the Submission notes field, please indicate that you
agree that your presentation will be licensed under the CC-By-SA-4.0 or
CC-By-4.0 license and that you agree to have your presentation recorded.
For example:

"If my presentation is accepted for FOSDEM, I hereby agree to license all
recordings, slides, and other associated materials under the Creative
Commons Attribution Share-Alike 4.0 International License. Sincerely,
."

In the Submission notes field, please also confirm that if your talk is
accepted, you will be able to attend FOSDEM and deliver your presentation.
We will not consider proposals from prospective speakers who are unsure
whether they will be able to secure funds for travel and lodging to attend
FOSDEM. (Sadly, we are not able to offer travel funding for prospective
speakers.)

Speaker Mentoring Program

As a part of the rising efforts to grow our communities and encourage a
diverse and inclusive conference ecosystem, we're happy to announce that
we'll be offering mentoring for new speakers. Our mentors can help you with
tasks such as reviewing your abstract, reviewing your presentation outline
or slides, or practicing your talk with you.

You may apply to the mentoring program as a newcomer speaker if you:

Never presented before or
Presented only lightning talks or
Presented full-length talks at small meetups (<50 ppl)

Submission Guidelines

Mentored presentations will have 25-minute slots, where 20 minutes will
include the presentation and 5 minutes will be reserved for questions.

The number of newcomer session slots is limited, so we will probably not be
able to accept all applications.

You must submit your talk and abstract to apply for the mentoring program,
our mentors are volunteering their time and will happily provide feedback
but won't write your presentation for you!

If you are experiencing problems with Pentabarf, the proposal submission
interface, or have other questions, you can email our devroom mailing
list[2] and we will try to help you.

How to Apply

In addition to agreeing to video recording and confirming that you can
attend FOSDEM in case your session is accepted, please write "speaker
mentoring program application" in the "Submission notes" field, and list
any 

Re: [libvirt] [PATCH 00/14] introduce virDomainSetLifecycleAction() API

[Pavel Hrdina]

On Wed, Oct 18, 2017 at 09:59:46AM -0400, John Ferlan wrote:
> 
> 
> On 10/16/2017 07:06 AM, Pavel Hrdina wrote:
> > Pavel Hrdina (14):
> >   conf: rename lifecycle enum values to correspond with typedef keyword
> >   conf: rename virDomainLifecycleAction enum functions
> >   conf: introduce virDomainLifecycle enum to list all lifecycle types
> >   conf: merge virDomainLifecycleCrashAction with
> > virDomainLifecycleAction
> >   qemu: pass virDomainObjPtr to qemuBuildCommandLine
> >   qemu: pass priv data to qemuBuildMonitorCommandLine
> >   qemu: pass priv data to qemuBuildPMCommandLine
> >   qemu: pass priv data to qemuBuildMasterKeyCommandLine
> >   qemu: pass priv data instead of qemuCaps and autoNodeset
> >   lib: introduce virDomainSetLifecycleAction() API
> >   virsh: introduce set-lifecycle-action command
> >   qemu: move detection whether to use -no-reboot to qemu_domain
> >   qemu: send allowReboot in migration cookie
> >   qemu: implement virDomainSetLifecycleAction() API
> > 
> >  docs/formatdomain.html.in|   6 +++
> >  include/libvirt/libvirt-domain.h |  28 ++
> >  src/conf/domain_conf.c   |  84 -
> >  src/conf/domain_conf.h   |  26 ++---
> >  src/driver-hypervisor.h  |   7 +++
> >  src/libvirt-domain.c |  60 +
> >  src/libvirt_private.syms |   7 ++-
> >  src/libvirt_public.syms  |   5 ++
> >  src/libxl/libxl_conf.c   |  43 ---
> >  src/libxl/libxl_domain.c |  40 +++---
> >  src/lxc/lxc_native.c |   6 +--
> >  src/qemu/qemu_command.c  |  93 +---
> >  src/qemu/qemu_command.h  |  11 +---
> >  src/qemu/qemu_domain.c   |  40 ++
> >  src/qemu/qemu_domain.h   |   9 
> >  src/qemu/qemu_driver.c   | 112 
> > +++
> >  src/qemu/qemu_migration.c|   7 ++-
> >  src/qemu/qemu_migration_cookie.c |  25 -
> >  src/qemu/qemu_migration_cookie.h |   5 ++
> >  src/qemu/qemu_parse_command.c|   8 +--
> >  src/qemu/qemu_process.c  |  53 --
> >  src/remote/remote_driver.c   |   1 +
> >  src/remote/remote_protocol.x |  14 -
> >  src/test/test_driver.c   |   8 +--
> >  src/vmx/vmx.c|   6 +--
> >  src/vz/vz_sdk.c  |  12 ++---
> >  src/xenapi/xenapi_utils.c|  40 +++---
> >  src/xenapi/xenapi_utils.h|   4 +-
> >  src/xenconfig/xen_common.c   |  12 ++---
> >  src/xenconfig/xen_sxpr.c |  18 +++
> >  tests/qemuxml2xmltest.c  |   3 +-
> >  tools/virsh-domain.c | 102 +++
> >  tools/virsh.pod  |   7 +++
> >  33 files changed, 658 insertions(+), 244 deletions(-)
> > 
> 
> FYI: The complete series doesn't pass make check:
> 
> make  check-local
> make[3]: Entering directory '/home/jferlan/git/libvirt.work/src'
>   GEN  remote_protocol-struct
> --- remote_protocol-structs   2017-10-17 15:55:01.279467242 -0400
> +++ remote_protocol-struct-t3 2017-10-18 09:59:23.818661775 -0400
> @@ -2865,6 +2865,12 @@
>  uint64_t   threshold;
>  u_int  flags;
>  };
> +struct remote_domain_set_lifecycle_action_args {
> +remote_nonnull_domain  dom;
> +u_int  type;
> +u_int  action;
> +u_int  flags;
> +};
>  enum remote_procedure {
>  REMOTE_PROC_CONNECT_OPEN = 1,
>  REMOTE_PROC_CONNECT_CLOSE = 2,
> @@ -3255,4 +3261,5 @@
>  REMOTE_PROC_DOMAIN_MIGRATE_GET_MAX_DOWNTIME = 387,
>  REMOTE_PROC_DOMAIN_MANAGED_SAVE_GET_XML_DESC = 388,
>  REMOTE_PROC_DOMAIN_MANAGED_SAVE_DEFINE_XML = 389,
> +REMOTE_PROC_DOMAIN_SET_LIFECYCLE_ACTION = 390,
>  };

Interesting is that it worked for me even without this.  I'll squash it
into the patch "lib: introduce virDomainSetLifecycleAction() API".

I'm missing pdwtags so this check was skipped.

Thanks

Pavel

> 
> John
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 00/14] introduce virDomainSetLifecycleAction() API

[John Ferlan]

On 10/16/2017 07:06 AM, Pavel Hrdina wrote:
> Pavel Hrdina (14):
>   conf: rename lifecycle enum values to correspond with typedef keyword
>   conf: rename virDomainLifecycleAction enum functions
>   conf: introduce virDomainLifecycle enum to list all lifecycle types
>   conf: merge virDomainLifecycleCrashAction with
> virDomainLifecycleAction
>   qemu: pass virDomainObjPtr to qemuBuildCommandLine
>   qemu: pass priv data to qemuBuildMonitorCommandLine
>   qemu: pass priv data to qemuBuildPMCommandLine
>   qemu: pass priv data to qemuBuildMasterKeyCommandLine
>   qemu: pass priv data instead of qemuCaps and autoNodeset
>   lib: introduce virDomainSetLifecycleAction() API
>   virsh: introduce set-lifecycle-action command
>   qemu: move detection whether to use -no-reboot to qemu_domain
>   qemu: send allowReboot in migration cookie
>   qemu: implement virDomainSetLifecycleAction() API
> 
>  docs/formatdomain.html.in|   6 +++
>  include/libvirt/libvirt-domain.h |  28 ++
>  src/conf/domain_conf.c   |  84 -
>  src/conf/domain_conf.h   |  26 ++---
>  src/driver-hypervisor.h  |   7 +++
>  src/libvirt-domain.c |  60 +
>  src/libvirt_private.syms |   7 ++-
>  src/libvirt_public.syms  |   5 ++
>  src/libxl/libxl_conf.c   |  43 ---
>  src/libxl/libxl_domain.c |  40 +++---
>  src/lxc/lxc_native.c |   6 +--
>  src/qemu/qemu_command.c  |  93 +---
>  src/qemu/qemu_command.h  |  11 +---
>  src/qemu/qemu_domain.c   |  40 ++
>  src/qemu/qemu_domain.h   |   9 
>  src/qemu/qemu_driver.c   | 112 
> +++
>  src/qemu/qemu_migration.c|   7 ++-
>  src/qemu/qemu_migration_cookie.c |  25 -
>  src/qemu/qemu_migration_cookie.h |   5 ++
>  src/qemu/qemu_parse_command.c|   8 +--
>  src/qemu/qemu_process.c  |  53 --
>  src/remote/remote_driver.c   |   1 +
>  src/remote/remote_protocol.x |  14 -
>  src/test/test_driver.c   |   8 +--
>  src/vmx/vmx.c|   6 +--
>  src/vz/vz_sdk.c  |  12 ++---
>  src/xenapi/xenapi_utils.c|  40 +++---
>  src/xenapi/xenapi_utils.h|   4 +-
>  src/xenconfig/xen_common.c   |  12 ++---
>  src/xenconfig/xen_sxpr.c |  18 +++
>  tests/qemuxml2xmltest.c  |   3 +-
>  tools/virsh-domain.c | 102 +++
>  tools/virsh.pod  |   7 +++
>  33 files changed, 658 insertions(+), 244 deletions(-)
> 

FYI: The complete series doesn't pass make check:

make  check-local
make[3]: Entering directory '/home/jferlan/git/libvirt.work/src'
  GEN  remote_protocol-struct
--- remote_protocol-structs 2017-10-17 15:55:01.279467242 -0400
+++ remote_protocol-struct-t3   2017-10-18 09:59:23.818661775 -0400
@@ -2865,6 +2865,12 @@
 uint64_t   threshold;
 u_int  flags;
 };
+struct remote_domain_set_lifecycle_action_args {
+remote_nonnull_domain  dom;
+u_int  type;
+u_int  action;
+u_int  flags;
+};
 enum remote_procedure {
 REMOTE_PROC_CONNECT_OPEN = 1,
 REMOTE_PROC_CONNECT_CLOSE = 2,
@@ -3255,4 +3261,5 @@
 REMOTE_PROC_DOMAIN_MIGRATE_GET_MAX_DOWNTIME = 387,
 REMOTE_PROC_DOMAIN_MANAGED_SAVE_GET_XML_DESC = 388,
 REMOTE_PROC_DOMAIN_MANAGED_SAVE_DEFINE_XML = 389,
+REMOTE_PROC_DOMAIN_SET_LIFECYCLE_ACTION = 390,
 };

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 0/2] More hotplug cleanups

[Ján Tomko]

Remove some dead code.

Ján Tomko (2):
  qemuIsMultiFunctionDevice: return early for non-PCI addresses
  qemu: remove pointless address validation on hot unplug

 src/qemu/qemu_hotplug.c | 56 -
 1 file changed, 9 insertions(+), 47 deletions(-)

-- 
2.13.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/2] qemuIsMultiFunctionDevice: return early for non-PCI addresses

[Ján Tomko]

There is no point in iterating over all devices if none of them
could possibly match.
---
 src/qemu/qemu_hotplug.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 0288986d8..aebd00598 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3752,6 +3752,9 @@ static int qemuComparePCIDevice(virDomainDefPtr def 
ATTRIBUTE_UNUSED,
 static bool qemuIsMultiFunctionDevice(virDomainDefPtr def,
   virDomainDeviceInfoPtr dev)
 {
+if (dev->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
+return false;
+
 if (virDomainDeviceInfoIterate(def, qemuComparePCIDevice, dev) < 0)
 return true;
 return false;
@@ -4828,8 +4831,7 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr 
driver,
 goto cleanup;
 }
 
-if (detach->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
-qemuIsMultiFunctionDevice(vm->def, >info)) {
+if (qemuIsMultiFunctionDevice(vm->def, >info)) {
 virReportError(VIR_ERR_OPERATION_FAILED,
_("cannot hot unplug multifunction PCI device: %s"),
dev->data.disk->dst);
-- 
2.13.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/2] qemu: remove pointless address validation on hot unplug

[Ján Tomko]

Back in the times of using 'pci_del', unplugging a device without
a PCI address was not wired up.

After completely removing support for qemu without QEMU_CAPS_DEVICE,
aliases are used to uniquely identify devices in all cases.

Remove the pointless validation of data that was already present
in the domain definition.
---
 src/qemu/qemu_hotplug.c | 50 +
 1 file changed, 5 insertions(+), 45 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index aebd00598..ac18a94d9 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -4610,23 +4610,6 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
 goto cleanup;
 }
 
-if (qemuDomainIsS390CCW(vm->def) &&
-virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) {
-if (!virDomainDeviceAddressIsValid(>info,
-   
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-   _("device cannot be detached without a valid CCW 
address"));
-goto cleanup;
-}
-} else {
-if (!virDomainDeviceAddressIsValid(>info,
-   
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-   _("device cannot be detached without a valid PCI 
address"));
-goto cleanup;
-}
-}
-
 if (!detach->info.alias) {
 if (qemuAssignDeviceDiskAlias(vm->def, detach, priv->qemuCaps) < 0)
 goto cleanup;
@@ -4884,13 +4867,6 @@ qemuDomainDetachHostPCIDevice(virQEMUDriverPtr driver,
 return -1;
 }
 
-if (!virDomainDeviceAddressIsValid(detach->info,
-   VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
-virReportError(VIR_ERR_OPERATION_FAILED,
-   "%s", _("device cannot be detached without a PCI 
address"));
-return -1;
-}
-
 qemuDomainMarkDeviceForRemoval(vm, detach->info);
 
 qemuDomainObjEnterMonitor(driver, vm);
@@ -5216,28 +5192,12 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
  
virDomainNetGetActualHostdev(detach));
 goto cleanup;
 }
-if (qemuDomainIsS390CCW(vm->def) &&
-virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) {
-if (!virDomainDeviceAddressIsValid(>info,
-   
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)) {
-virReportError(VIR_ERR_OPERATION_FAILED,
-"%s", _("device cannot be detached without a CCW 
address"));
-goto cleanup;
-}
-} else {
-if (!virDomainDeviceAddressIsValid(>info,
-   
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
-virReportError(VIR_ERR_OPERATION_FAILED,
-"%s", _("device cannot be detached without a PCI 
address"));
-goto cleanup;
-}
 
-if (qemuIsMultiFunctionDevice(vm->def, >info)) {
-virReportError(VIR_ERR_OPERATION_FAILED,
-_("cannot hot unplug multifunction PCI device 
:%s"),
-dev->data.disk->dst);
-goto cleanup;
-}
+if (qemuIsMultiFunctionDevice(vm->def, >info)) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("cannot hot unplug multifunction PCI device :%s"),
+   dev->data.disk->dst);
+goto cleanup;
 }
 
 if (!detach->info.alias) {
-- 
2.13.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virsh: domifaddr: clarify description of --full option

[John Ferlan]

On 10/16/2017 02:53 AM, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
>   Option --full will always display the name and MAC
>   address of the interface.
>   Both virsh help and virsh man page didn't mention that.
> 
> Signed-off-by: Chen Hanxiao 
> ---
>  tools/virsh-domain-monitor.c | 2 +-
>  tools/virsh.pod  | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 

Reviewed-by: John Ferlan 

OK - in general; however, before pushing I cleaned up the syntax and
grammar a bit for the --full argument as well as separating it so that
it has its own paragraph (causing --source to have its own paragraph too).

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v6 2/9] nodedev: udev: Introduce udevEventMonitorSanityCheck helper function

[Erik Skultety]

We need to perform a sanity check on the udev monitor before every
use so that we know nothing has changed in the meantime. The reason for
moving the code to a separate helper is to enhance readability and shift
the focus on the important stuff within the udevEventHandleCallback
handler.

Signed-off-by: Erik Skultety 
---
 src/node_device/node_device_udev.c | 43 --
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index 8ea5d1e62..8314b3834 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1618,24 +1618,20 @@ udevHandleOneDevice(struct udev_device *device)
 }
 
 
-static void
-udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
-int fd,
-int events ATTRIBUTE_UNUSED,
-void *data ATTRIBUTE_UNUSED)
+static bool
+udevEventMonitorSanityCheck(struct udev_monitor *udev_monitor,
+int fd)
 {
-struct udev_device *device = NULL;
-struct udev_monitor *udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
-int udev_fd = -1;
+int rc = -1;
 
-udev_fd = udev_monitor_get_fd(udev_monitor);
-if (fd != udev_fd) {
+rc = udev_monitor_get_fd(udev_monitor);
+if (fd != rc) {
 udevPrivate *priv = driver->privateData;
 
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("File descriptor returned by udev %d does not "
  "match node device file descriptor %d"),
-   fd, udev_fd);
+   fd, rc);
 
 /* this is a non-recoverable error, let's remove the handle, so that we
  * don't get in here again because of some spurious behaviour and 
report
@@ -1644,21 +1640,36 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
 virEventRemoveHandle(priv->watch);
 priv->watch = -1;
 
-goto cleanup;
+return false;
 }
 
+return true;
+}
+
+
+static void
+udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
+int fd,
+int events ATTRIBUTE_UNUSED,
+void *data ATTRIBUTE_UNUSED)
+{
+struct udev_device *device = NULL;
+struct udev_monitor *udev_monitor = NULL;
+
+udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
+
+if (!udevEventMonitorSanityCheck(udev_monitor, fd))
+return;
+
 device = udev_monitor_receive_device(udev_monitor);
 if (device == NULL) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("udev_monitor_receive_device returned NULL"));
-goto cleanup;
+return;
 }
 
 udevHandleOneDevice(device);
-
- cleanup:
 udev_device_unref(device);
-return;
 }
 
 
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v6 6/9] nodedev: udev: Split udevEventHandleCallback in two functions

[Erik Skultety]

This patch splits udevEventHandleCallback in two (introduces
udevEventHandleThread) in order to be later able to refactor the latter
to actually become a normal thread which will wait some time for the
kernel to create the whole sysfs tree for a device as we cannot do that
in the event loop directly.

Signed-off-by: Erik Skultety 
---
 src/node_device/node_device_udev.c | 41 +++---
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index 6882517e6..0167ad596 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1679,32 +1679,49 @@ udevEventMonitorSanityCheck(udevEventDataPtr priv,
 
 
 static void
+udevEventHandleThread(void *opaque)
+{
+udevEventDataPtr priv = driver->privateData;
+int fd = (intptr_t) opaque;
+struct udev_device *device = NULL;
+
+virObjectLock(priv);
+
+if (!udevEventMonitorSanityCheck(priv, fd)) {
+virObjectUnlock(priv);
+return;
+}
+
+device = udev_monitor_receive_device(priv->udev_monitor);
+virObjectUnlock(priv);
+
+if (device == NULL) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("udev_monitor_receive_device returned NULL"));
+return;
+}
+
+udevHandleOneDevice(device);
+udev_device_unref(device);
+}
+
+
+static void
 udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
 int fd,
 int events ATTRIBUTE_UNUSED,
 void *data ATTRIBUTE_UNUSED)
 {
 udevEventDataPtr priv = driver->privateData;
-struct udev_device *device = NULL;
 
 virObjectLock(priv);
-
 if (!udevEventMonitorSanityCheck(priv, fd)) {
 virObjectUnlock(priv);
 return;
 }
-
-device = udev_monitor_receive_device(priv->udev_monitor);
 virObjectUnlock(priv);
 
-if (device == NULL) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("udev_monitor_receive_device returned NULL"));
-return;
-}
-
-udevHandleOneDevice(device);
-udev_device_unref(device);
+udevEventHandleThread((void *)(intptr_t) fd);
 }
 
 
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v6 7/9] nodedev: udev: Convert udevEventHandleThread to an actual thread routine

[Erik Skultety]

Adjust udevEventHandleThread to be a proper thread routine running in an
infinite loop handling devices. The handler thread pulls all available
data from the udev monitor and only then waits until a wakeup signal for
new incoming data has been emitted by udevEventHandleCallback.

Signed-off-by: Erik Skultety 
---
 src/node_device/node_device_udev.c | 106 +
 1 file changed, 84 insertions(+), 22 deletions(-)

diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index 0167ad596..9fa90257e 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -61,6 +61,12 @@ struct _udevEventData {
 
 struct udev_monitor *udev_monitor;
 int watch;
+
+/* Thread data */
+virThread th;
+virCond threadCond;
+bool threadQuit;
+bool dataReady;
 };
 
 static virClassPtr udevEventDataClass;
@@ -80,6 +86,8 @@ udevEventDataDispose(void *obj)
 udev = udev_monitor_get_udev(priv->udev_monitor);
 udev_monitor_unref(priv->udev_monitor);
 udev_unref(udev);
+
+virCondDestroy(>threadCond);
 }
 
 
@@ -108,6 +116,11 @@ udevEventDataNew(void)
 if (!(ret = virObjectLockableNew(udevEventDataClass)))
 return NULL;
 
+if (virCondInit(>threadCond) < 0) {
+virObjectUnref(ret);
+return NULL;
+}
+
 ret->watch = -1;
 return ret;
 }
@@ -1616,10 +1629,21 @@ udevPCITranslateDeinit(void)
 static int
 nodeStateCleanup(void)
 {
+udevEventDataPtr priv = NULL;
+
 if (!driver)
 return -1;
 
-virObjectUnref(driver->privateData);
+priv = driver->privateData;
+if (priv) {
+virObjectLock(priv);
+priv->threadQuit = true;
+virCondSignal(>threadCond);
+virObjectUnlock(priv);
+virThreadJoin(>th);
+}
+
+virObjectUnref(priv);
 virObjectUnref(driver->nodeDeviceEventState);
 
 virNodeDeviceObjListFree(driver->devs);
@@ -1679,30 +1703,61 @@ udevEventMonitorSanityCheck(udevEventDataPtr priv,
 
 
 static void
-udevEventHandleThread(void *opaque)
+udevEventHandleThread(void *opaque ATTRIBUTE_UNUSED)
 {
 udevEventDataPtr priv = driver->privateData;
-int fd = (intptr_t) opaque;
 struct udev_device *device = NULL;
 
-virObjectLock(priv);
+/* continue rather than break from the loop on non-fatal errors */
+while (1) {
+virObjectLock(priv);
+while (!priv->dataReady && !priv->threadQuit) {
+if (virCondWait(>threadCond, >parent.lock)) {
+virReportSystemError(errno, "%s",
+ _("handler failed to wait on condition"));
+virObjectUnlock(priv);
+return;
+}
+}
 
-if (!udevEventMonitorSanityCheck(priv, fd)) {
+if (priv->threadQuit) {
+virObjectUnlock(priv);
+return;
+}
+
+errno = 0;
+device = udev_monitor_receive_device(priv->udev_monitor);
 virObjectUnlock(priv);
-return;
-}
 
-device = udev_monitor_receive_device(priv->udev_monitor);
-virObjectUnlock(priv);
+if (!device) {
+if (errno == 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("failed to receive device from udev 
monitor"));
+return;
+}
 
-if (device == NULL) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("udev_monitor_receive_device returned NULL"));
-return;
-}
+/* POSIX allows both EAGAIN and EWOULDBLOCK to be used
+ * interchangeably when the read would block or timeout was fired
+ */
+VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
+if (errno != EAGAIN && errno != EWOULDBLOCK) {
+VIR_WARNINGS_RESET
+virReportSystemError(errno, "%s",
+ _("failed to receive device from udev "
+   "monitor"));
+return;
+}
+
+virObjectLock(priv);
+priv->dataReady = false;
+virObjectUnlock(priv);
 
-udevHandleOneDevice(device);
-udev_device_unref(device);
+continue;
+}
+
+udevHandleOneDevice(device);
+udev_device_unref(device);
+}
 }
 
 
@@ -1715,13 +1770,14 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
 udevEventDataPtr priv = driver->privateData;
 
 virObjectLock(priv);
-if (!udevEventMonitorSanityCheck(priv, fd)) {
-virObjectUnlock(priv);
-return;
-}
+
+if (!udevEventMonitorSanityCheck(priv, fd))
+priv->threadQuit = true;
+else
+priv->dataReady = true;
+
+virCondSignal(>threadCond);
 virObjectUnlock(priv);
-
-udevEventHandleThread((void *)(intptr_t) fd);
 }
 
 
@@ -1903,6 +1959,12 @@ nodeStateInitialize(bool privileged,
   

[libvirt] [PATCH v6 5/9] nodedev: udev: Unlock the private data before setting up 'system' node

[Erik Skultety]

udevSetupSystemDev only needs the udev data lock to be locked because of
calling udevGetDMIData which accesses some protected structure members,
but it can do that on its own just fine, no need to hold the lock the
whole time.

Signed-off-by: Erik Skultety 
---
 src/node_device/node_device_udev.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index e0e5ba799..6882517e6 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1719,6 +1719,7 @@ udevGetDMIData(virNodeDevCapSystemPtr syscap)
 virNodeDevCapSystemHardwarePtr hardware = >hardware;
 virNodeDevCapSystemFirmwarePtr firmware = >firmware;
 
+virObjectLock(priv);
 udev = udev_monitor_get_udev(priv->udev_monitor);
 
 device = udev_device_new_from_syspath(udev, DMI_DEVPATH);
@@ -1731,6 +1732,7 @@ udevGetDMIData(virNodeDevCapSystemPtr syscap)
 return;
 }
 }
+virObjectUnlock(priv);
 
 if (udevGetStringSysfsAttr(device, "product_name",
>product_name) < 0)
@@ -1898,11 +1900,11 @@ nodeStateInitialize(bool privileged,
 if (priv->watch == -1)
 goto unlock;
 
+virObjectUnlock(priv);
+
 /* Create a fictional 'computer' device to root the device tree. */
 if (udevSetupSystemDev() != 0)
-goto unlock;
-
-virObjectUnlock(priv);
+goto cleanup;
 
 /* Populate with known devices */
 if (udevEnumerateDevices(udev) != 0)
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v6 9/9] nodedev: udev: Hook up virFileWaitForAccess to work around uevent race

[Erik Skultety]

If we find ourselves in the situation that the 'add' uevent has been
fired earlier than the sysfs tree for a device was created, we should
use the best-effort approach and give kernel some predetermined amount
of time, thus waiting for the attributes to be ready rather than
discarding the device from our device list forever. If those don't appear
in the given time frame, we need to move on, since libvirt can't wait
indefinitely.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1463285

Signed-off-by: Erik Skultety 
---
 src/node_device/node_device_udev.c | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index 9fa90257e..6686d4f3f 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1188,9 +1188,23 @@ udevProcessMediatedDevice(struct udev_device *dev,
 char *canonicalpath = NULL;
 virNodeDevCapMdevPtr data = >caps->data.mdev;
 
-if (virAsprintf(, "%s/mdev_type", udev_device_get_syspath(dev)) < 
0)
+/* Because of a kernel uevent race, we might get the 'add' event prior to
+ * the sysfs tree being ready, so any attempt to access any sysfs attribute
+ * would result in ENOENT and us dropping the device, so let's work around
+ * it by waiting for the attributes to become available.
+ */
+
+if (virAsprintf(, "%s/mdev_type",
+udev_device_get_syspath(dev)) < 0)
 goto cleanup;
 
+if (virFileWaitForExists(linkpath, 1, 100) < 0) {
+virReportSystemError(errno,
+ _("failed to wait for file '%s' to appear"),
+ linkpath);
+goto cleanup;
+}
+
 if (virFileResolveLink(linkpath, ) < 0) {
 virReportSystemError(errno, _("failed to resolve '%s'"), linkpath);
 goto cleanup;
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v6 8/9] util: Introduce virFileWaitForExists

[Erik Skultety]

Since we have a number of places where we workaround timing issues with
devices, attributes (files in general) not being available at the time
of processing them by calling usleep in a loop for a fixed number of
tries, we could as well have a utility function that would do that.
Therefore we won't have to duplicate this ugly workaround even more.

Signed-off-by: Erik Skultety 
---
 src/libvirt_private.syms |  1 +
 src/util/virfile.c   | 31 +++
 src/util/virfile.h   |  2 ++
 3 files changed, 34 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7bd21ae23..3b5df28e5 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1756,6 +1756,7 @@ virFileStripSuffix;
 virFileTouch;
 virFileUnlock;
 virFileUpdatePerm;
+virFileWaitForExists;
 virFileWrapperFdClose;
 virFileWrapperFdFree;
 virFileWrapperFdNew;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 7ca60052d..82cb36dbc 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -4177,3 +4177,34 @@ virFileReadValueString(char **value, const char *format, 
...)
 VIR_FREE(str);
 return ret;
 }
+
+
+/**
+ * virFileWaitForExists:
+ * @path: absolute path to a sysfs attribute (can be a symlink)
+ * @ms: how long to wait (in milliseconds)
+ * @tries: how many times should we try to wait for @path to become accessible
+ *
+ * Checks the existence of @path. In case the file defined by @path
+ * doesn't exist, we wait for it to appear in @ms milliseconds (for up to
+ * @tries attempts).
+ *
+ * Returns 0 on success, -1 on error, setting errno appropriately.
+ */
+int
+virFileWaitForExists(const char *path,
+ size_t ms,
+ size_t tries)
+{
+errno = 0;
+
+/* wait for @path to be accessible in @ms milliseconds, up to @tries */
+while (tries-- > 0 && !virFileExists(path)) {
+if (tries == 0 || errno != ENOENT)
+return -1;
+
+usleep(ms * 1000);
+}
+
+return 0;
+}
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 21fb41b70..91d318622 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -349,6 +349,8 @@ int virFileReadValueScaledInt(unsigned long long *value, 
const char *format, ...
 int virFileReadValueString(char **value, const char *format, ...)
  ATTRIBUTE_FMT_PRINTF(2, 3);
 
+int virFileWaitForExists(const char *path, size_t ms, size_t tries);
+
 
 int virFileInData(int fd,
   int *inData,
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v6 4/9] nodedev: udev: Remove driver locks from stateInitialize and stateCleanup

[Erik Skultety]

The driver locks are unnecessary here, since currently the cleanup is
only called from the main daemon thread, so we can't race here. Moreover
@devs and @privateData are self-lockable objects, so no problem there
either.

Signed-off-by: Erik Skultety 
---
 src/node_device/node_device_udev.c | 16 
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index a7b628153..e0e5ba799 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1619,13 +1619,10 @@ nodeStateCleanup(void)
 if (!driver)
 return -1;
 
-nodeDeviceLock();
-
 virObjectUnref(driver->privateData);
 virObjectUnref(driver->nodeDeviceEventState);
 
 virNodeDeviceObjListFree(driver->devs);
-nodeDeviceUnlock();
 virMutexDestroy(>lock);
 VIR_FREE(driver);
 
@@ -1846,23 +1843,21 @@ nodeStateInitialize(bool privileged,
 return -1;
 }
 
-nodeDeviceLock();
-
 if (!(driver->devs = virNodeDeviceObjListNew()) ||
 !(priv = udevEventDataNew()))
-goto unlock;
+goto cleanup;
 
 driver->privateData = priv;
 driver->nodeDeviceEventState = virObjectEventStateNew();
 
 if (udevPCITranslateInit(privileged) < 0)
-goto unlock;
+goto cleanup;
 
 udev = udev_new();
 if (!udev) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("failed to create udev context"));
-goto unlock;
+goto cleanup;
 }
 #if HAVE_UDEV_LOGGING
 /* cast to get rid of missing-format-attribute warning */
@@ -1908,7 +1903,6 @@ nodeStateInitialize(bool privileged,
 goto unlock;
 
 virObjectUnlock(priv);
-nodeDeviceUnlock();
 
 /* Populate with known devices */
 if (udevEnumerateDevices(udev) != 0)
@@ -1921,9 +1915,7 @@ nodeStateInitialize(bool privileged,
 return -1;
 
  unlock:
-if (priv)
-virObjectUnlock(priv);
-nodeDeviceUnlock();
+virObjectUnlock(priv);
 goto cleanup;
 }
 
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v6 3/9] nodedev: udev: Convert udev private data to a lockable object

[Erik Skultety]

Since there's going to be a worker thread which needs to have some data
protected by a lock, the whole code would just simply get unnecessary
complex, since two sets of locks would be necessary, driver lock (for
udev monitor and event handle) and a mutex protecting thread-local data.
Given the future thread will need to access the udev monitor socket as
well, why not protect everything with a single lock, even better, by
converting the driver's private data to a lockable object, we get the
automatic object disposal feature for free.

Signed-off-by: Erik Skultety 
---
 src/node_device/node_device_udev.c | 134 +++--
 src/node_device/node_device_udev.h |   3 -
 2 files changed, 85 insertions(+), 52 deletions(-)

diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index 8314b3834..a7b628153 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -53,11 +53,65 @@ VIR_LOG_INIT("node_device.node_device_udev");
 # define TYPE_RAID 12
 #endif
 
-struct _udevPrivate {
+typedef struct _udevEventData udevEventData;
+typedef udevEventData *udevEventDataPtr;
+
+struct _udevEventData {
+virObjectLockable parent;
+
 struct udev_monitor *udev_monitor;
 int watch;
 };
 
+static virClassPtr udevEventDataClass;
+
+static void
+udevEventDataDispose(void *obj)
+{
+struct udev *udev = NULL;
+udevEventDataPtr priv = obj;
+
+if (priv->watch != -1)
+virEventRemoveHandle(priv->watch);
+
+if (!priv->udev_monitor)
+return;
+
+udev = udev_monitor_get_udev(priv->udev_monitor);
+udev_monitor_unref(priv->udev_monitor);
+udev_unref(udev);
+}
+
+
+static int
+udevEventDataOnceInit(void)
+{
+if (!(udevEventDataClass = virClassNew(virClassForObjectLockable(),
+   "udevEventData",
+   sizeof(udevEventData),
+   udevEventDataDispose)))
+return -1;
+
+return 0;
+}
+
+VIR_ONCE_GLOBAL_INIT(udevEventData)
+
+static udevEventDataPtr
+udevEventDataNew(void)
+{
+udevEventDataPtr ret = NULL;
+
+if (udevEventDataInitialize() < 0)
+return NULL;
+
+if (!(ret = virObjectLockableNew(udevEventDataClass)))
+return NULL;
+
+ret->watch = -1;
+return ret;
+}
+
 
 static bool
 udevHasDeviceProperty(struct udev_device *dev,
@@ -1562,39 +1616,18 @@ udevPCITranslateDeinit(void)
 static int
 nodeStateCleanup(void)
 {
-udevPrivate *priv = NULL;
-struct udev_monitor *udev_monitor = NULL;
-struct udev *udev = NULL;
-
 if (!driver)
 return -1;
 
 nodeDeviceLock();
 
+virObjectUnref(driver->privateData);
 virObjectUnref(driver->nodeDeviceEventState);
 
-priv = driver->privateData;
-
-if (priv) {
-if (priv->watch != -1)
-virEventRemoveHandle(priv->watch);
-
-udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
-
-if (udev_monitor != NULL) {
-udev = udev_monitor_get_udev(udev_monitor);
-udev_monitor_unref(udev_monitor);
-}
-}
-
-if (udev != NULL)
-udev_unref(udev);
-
 virNodeDeviceObjListFree(driver->devs);
 nodeDeviceUnlock();
 virMutexDestroy(>lock);
 VIR_FREE(driver);
-VIR_FREE(priv);
 
 udevPCITranslateDeinit();
 return 0;
@@ -1618,16 +1651,17 @@ udevHandleOneDevice(struct udev_device *device)
 }
 
 
+/* the caller must be holding the udevEventData object lock prior to calling
+ * this function
+ */
 static bool
-udevEventMonitorSanityCheck(struct udev_monitor *udev_monitor,
+udevEventMonitorSanityCheck(udevEventDataPtr priv,
 int fd)
 {
 int rc = -1;
 
-rc = udev_monitor_get_fd(udev_monitor);
+rc = udev_monitor_get_fd(priv->udev_monitor);
 if (fd != rc) {
-udevPrivate *priv = driver->privateData;
-
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("File descriptor returned by udev %d does not "
  "match node device file descriptor %d"),
@@ -1653,15 +1687,19 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
 int events ATTRIBUTE_UNUSED,
 void *data ATTRIBUTE_UNUSED)
 {
+udevEventDataPtr priv = driver->privateData;
 struct udev_device *device = NULL;
-struct udev_monitor *udev_monitor = NULL;
 
-udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
+virObjectLock(priv);
 
-if (!udevEventMonitorSanityCheck(udev_monitor, fd))
+if (!udevEventMonitorSanityCheck(priv, fd)) {
+virObjectUnlock(priv);
 return;
+}
+
+device = udev_monitor_receive_device(priv->udev_monitor);
+virObjectUnlock(priv);
 
-device = udev_monitor_receive_device(udev_monitor);
 if (device == NULL) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("udev_monitor_receive_device 

[libvirt] [PATCH v6 1/9] nodedev: Move privileged flag from udev private data to driver's state

[Erik Skultety]

Even though hal doesn't make use of it, the privileged flag is related
to the daemon/driver rather than the backend actually used.
While at it, get rid of some tab indentation in the driver state struct.

Signed-off-by: Erik Skultety 
---
 src/conf/virnodedeviceobj.h|  1 +
 src/node_device/node_device_udev.c | 11 +++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h
index e7c26abbd..87f908369 100644
--- a/src/conf/virnodedeviceobj.h
+++ b/src/conf/virnodedeviceobj.h
@@ -40,6 +40,7 @@ struct _virNodeDeviceDriverState {
 
 virNodeDeviceObjListPtr devs;   /* currently-known devices */
 void *privateData;  /* driver-specific private data */
+bool privileged;/* whether we run in privileged mode */
 
 /* Immutable pointer, self-locking APIs */
 virObjectEventStatePtr nodeDeviceEventState;
diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index f4177455c..8ea5d1e62 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -56,7 +56,6 @@ VIR_LOG_INIT("node_device.node_device_udev");
 struct _udevPrivate {
 struct udev_monitor *udev_monitor;
 int watch;
-bool privileged;
 };
 
 
@@ -447,9 +446,13 @@ udevProcessPCI(struct udev_device *device,
 virNodeDevCapPCIDevPtr pci_dev = >caps->data.pci_dev;
 virPCIEDeviceInfoPtr pci_express = NULL;
 virPCIDevicePtr pciDev = NULL;
-udevPrivate *priv = driver->privateData;
 int ret = -1;
 char *p;
+bool privileged;
+
+nodeDeviceLock();
+privileged = driver->privileged;
+nodeDeviceUnlock();
 
 if (udevGetUintProperty(device, "PCI_CLASS", _dev->class, 16) < 0)
 goto cleanup;
@@ -498,7 +501,7 @@ udevProcessPCI(struct udev_device *device,
 goto cleanup;
 
 /* We need to be root to read PCI device configs */
-if (priv->privileged) {
+if (privileged) {
 if (virPCIGetHeaderType(pciDev, _dev->hdrType) < 0)
 goto cleanup;
 
@@ -1787,7 +1790,6 @@ nodeStateInitialize(bool privileged,
 return -1;
 
 priv->watch = -1;
-priv->privileged = privileged;
 
 if (VIR_ALLOC(driver) < 0) {
 VIR_FREE(priv);
@@ -1802,6 +1804,7 @@ nodeStateInitialize(bool privileged,
 return -1;
 }
 
+driver->privileged = privileged;
 driver->privateData = priv;
 nodeDeviceLock();
 
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v6 0/9] Work around the kernel mdev uevent race in nodedev

[Erik Skultety]

v5 here:
https://www.redhat.com/archives/libvir-list/2017-October/msg00440.html

Since v5:
- fixed minor nitpicks
- added 3 more patches as per reviewer's suggestion to split some of the
changes even more
- patches {2,6,7,8,9}/9 are without any change

Erik Skultety (9):
  nodedev: Move privileged flag from udev private data to driver's state (NEW)
  nodedev: udev: Introduce udevEventMonitorSanityCheck helper function (ACKed)
  nodedev: udev: Convert udev private data to a lockable object (ACKed with 
fixes)
  nodedev: udev: Remove driver locks from stateInitialize and (NEW)
stateCleanup
  nodedev: udev: Unlock the private data before setting up 'system' node (NEW)
  nodedev: udev: Split udevEventHandleCallback in two functions (ACKed)
  nodedev: udev: Convert udevEventHandleThread to an actual thread (ACKed)
routine
  util: Introduce virFileWaitForExists (ACKed)
  nodedev: udev: Hook up virFileWaitForAccess to work around uevent race (ACKed)

 src/conf/virnodedeviceobj.h|   1 +
 src/libvirt_private.syms   |   1 +
 src/node_device/node_device_udev.c | 283 +++--
 src/node_device/node_device_udev.h |   3 -
 src/util/virfile.c |  31 
 src/util/virfile.h |   2 +
 6 files changed, 245 insertions(+), 76 deletions(-)

--
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v5 4/6] udev: Convert udevEventHandleThread to an actual thread routine

[Erik Skultety]

On Sun, Oct 15, 2017 at 10:23:56AM -0400, John Ferlan wrote:
>
>
> On 10/11/2017 10:52 AM, Erik Skultety wrote:
> > Adjust udevEventHandleThread to be a proper thread routine running in an
> > infinite loop handling devices. The handler thread pulls all available
> > data from the udev monitor and only then waits until a wakeup signal for
> > new incoming data has been emitted by udevEventHandleCallback.
> >
>
> This doing a bit more by removing the driver locks from initialization
> too.  Perhaps those locks should be kept on Initialization for now and
> then in a followup patch remove them since @privateData (or whatever
> name it becomes) is then totally self locking.
>
> I don't think we'd run into any deadlocks since Initialization and
> Cleanup would still be the only consumers.

Like I said, patch will be added.

[...]
> >  static bool
> > @@ -1625,15 +1639,25 @@ udevPCITranslateDeinit(void)
> >  static int
> >  nodeStateCleanup(void)
> >  {
> > +udevEventDataPtr priv = NULL;
> > +
> >  if (!driver)
> >  return -1;
> >
> > +priv = driver->privateData;
> > +
>
> Although perhaps impossible, better safe than sorry

Here it's actually quite possible, when allocation of the private data in
Initialize fails, cleanup is performed, so this would get a SIGSEGV, thanks.

Erik

> Perhaps we should keep it just for this patch and remove in the "next"
> patch leaving the goto unlock intact, but modifying it to be :
>
>  unlock:
> if (priv)
> virObjectUnlock(priv);
> nodeDeviceUnlock();
>
> Then the next patch alters the gots's and unlock: label to what this
> patch has. It also removes nodeDeviceLock from Cleanup

Yeah, I'll handle the locking prior to adding this thread-related stuff.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v5 3/6] udev: Split udevEventHandleCallback in two functions

[Erik Skultety]

On Sun, Oct 15, 2017 at 10:23:52AM -0400, John Ferlan wrote:
>
>
> On 10/11/2017 10:52 AM, Erik Skultety wrote:
> > This patch splits udevEventHandleCallback in two (introduces
> > udevEventHandleThread) in order to be later able to refactor the latter
> > to actually become a normal thread which will wait some time for the
> > kernel to create the whole sysfs tree for a device as we cannot do that
> > in the event loop directly.
> >
> > Signed-off-by: Erik Skultety 
> > ---
> >  src/node_device/node_device_udev.c | 41 
> > +++---
> >  1 file changed, 29 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/node_device/node_device_udev.c 
> > b/src/node_device/node_device_udev.c
> > index bb9787fdb..f7646cd8a 100644
> > --- a/src/node_device/node_device_udev.c
> > +++ b/src/node_device/node_device_udev.c
> > @@ -1691,32 +1691,49 @@ udevEventMonitorSanityCheck(udevEventDataPtr priv,
> >
> >
> >  static void
> > +udevEventHandleThread(void *opaque)
> > +{
> > +udevEventDataPtr priv = driver->privateData;
> > +int fd = (intptr_t) opaque;
> > +struct udev_device *device = NULL;
> > +
>
> To go along with my thought from patch 2, should there be a "if (!priv)"
> check here too?  I believe it doesn't matter yet because there are
> driver level locks that would seemingly prevent the thread code from
> running if Cleanup occurs, but soon enough that won't be the case.

It's called from within Initialize which is the main thread, if !priv,
Initialize would have failed by this point

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v5 2/6] nodedev: udev: Convert udev private data to a lockable object

[Erik Skultety]

[...]
> > +struct _udevEventData {
> > +virObjectLockable parent;
> > +
> >  struct udev_monitor *udev_monitor;
> >  int watch;
> >  bool privileged;
> >  };
>
> Mental note - maybe the driver->privateData should change to
> driver->udevEventData in _virNodeDeviceDriverState

I like the notion of generic private data per backend, even though one of them
doesn't use it all. But in principle, I agree with you, I'd just wait 'till we
decide that we can  safely deprecate hal backend in upstream completely.

>
> You interchange using @priv and @data throughout which right now could
> make sense, but in the future may make less sense. I have a preference
> for consistency, so calling @data in some places and @priv in others
> leads to inconsistency. I believe pick one and be consistent.

Good point, noted.

>
> >
> > +static virClassPtr udevEventDataClass;
> > +
> > +static void
> > +udevEventDataDispose(void *obj)
> > +{
> > +struct udev *udev = NULL;
> > +udevEventDataPtr data = obj;
> > +
> > +if (data->watch != -1)
> > +virEventRemoveHandle(data->watch);
> > +
> > +if (data->udev_monitor)
> > +udev = udev_monitor_get_udev(data->udev_monitor);
> > +
> > +udev_unref(udev);
> > +udev_monitor_unref(data->udev_monitor);
>
> This is a different order than previous cleanup - perhaps should be:
>
> if (data->udev_monitor) {
> udev = udev_monitor_get_udev(data->udev_monitor);
> udev_monitor_unref(data->udev_monitor);
> udev_unref(udev);
> }
>

libudev handles NULLs gracefully...

>
> or
>
> if (!data->udev_monitor)
> return;
>
> udev = udev_monitor_get_udev(data->udev_monitor);
> udev_monitor_unref(data->udev_monitor);
> udev_unref(udev);

I'll go with this version then, I did it this way, because udev keeps a pointer
to the context withing the monitor, but doesn't touch it when freeing, leaving
the responsibility to the caller. But I agree that releasing the monitor before
releasing the context makes more sense.

>
> Just not clear what happens to udev if you unref udev before unref
> udev_monitor.  Better safe than sorry and go in the opposite order of
> Initialization.
>
> > +}
> > +
> > +
> > +static int
> > +udevEventDataOnceInit(void)
> > +{
> > +if (!(udevEventDataClass = virClassNew(virClassForObjectLockable(),
> > +   "udevEventData",
> > +   sizeof(udevEventData),
> > +   udevEventDataDispose)))
> > +return -1;
> > +
> > +return 0;
> > +}
> > +
> > +VIR_ONCE_GLOBAL_INIT(udevEventData)
> > +
> > +static udevEventDataPtr
> > +udevEventDataNew(void)
> > +{
> > +udevEventDataPtr ret = NULL;
> > +
> > +if (udevEventDataInitialize() < 0)
> > +return NULL;
> > +
> > +if (!(ret = virObjectLockableNew(udevEventDataClass)))
> > +return NULL;
> > +
> > +ret->watch = -1;
> > +return ret;
> > +}
> > +
> > +
> > +static bool
> > +udevEventDataIsPrivileged(udevEventDataPtr data)
> > +{
> > +bool privileged;
>
> Hmmm... is @data privileged or is the driver that needs the privilege
> check... IOW: should privileged by a DriverState value (even though it'd
> be unused in _hal).  It comes in as a virDrvStateInitialize value. That
> way it's just a driver->privileged check regardless of whether there is
> an event thread running.

Good observation, a patch will be added.

>
> I know - should have thought of this sooner, but sometimes things are
> more obvious at odd points in review time. Moving privileged would be a
> patch 1.5 and thus make this helper unnecessary. It'd be a simple move
> from private to NodeDeviceDriverState and then store/read from there
> instead of @priv.  Then udevGetDMIData wouldn't need to get @priv.

No problem.


[..]
> >  {
> > +udevEventDataPtr priv = driver->privateData;
> >  struct udev_device *device = NULL;
> > -struct udev_monitor *udev_monitor = NULL;
> >
> > -udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
>
> Should we add a "if (!priv) return" before this?

This would only be necessary if that was a user's input, but it's us who's
completely responsible for that and @data are only touched by stateCleanup,
which runs after the eventloop ends, so this check would be essentially useless.

>
> > +virObjectLock(priv);
> >
> > -if (!udevEventMonitorSanityCheck(udev_monitor, fd))
> > +if (!udevEventMonitorSanityCheck(priv, fd)) {
> > +virObjectUnlock(priv);
> >  return;
> > +}
> > +
> > +device = udev_monitor_receive_device(priv->udev_monitor);
> > +virObjectUnlock(priv);
> >
> > -device = udev_monitor_receive_device(udev_monitor);
> >  if (device == NULL) {
> >  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > _("udev_monitor_receive_device returned NULL"));
> > @@ -1675,12 +1725,13 @@ udevEventHandleCallback(int 

Re: [libvirt] [PATCH 1/6] vbox: Close media when undefining domains.

[John Ferlan]

On 10/17/2017 05:28 PM, Dawid Zamirski wrote:
> On Tue, 2017-10-17 at 15:44 -0400, John Ferlan wrote:
>>
>> On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
>>> From: Dawid Zamirski 
>>>
>>> When registering a VM we call OpenMedium on each disk image which
>>> adds it
>>> to vbox's global media registry. Therefore, we should make sure to
>>> call
>>> Close when unregistering VM so we cleanup the media registry
>>> entries
>>> after ourselves - this does not remove disk image files. This
>>> follows
>>> the behaviour of the VBoxManage unregistervm command.
>>> ---
>>>  src/vbox/vbox_tmpl.c | 24 +++-
>>>  1 file changed, 19 insertions(+), 5 deletions(-)
>>>
>>
>> I'm not a vbox expert by any stretch, looking at this mostly because
>> it's been sitting on list unreviewed...
> 
> Thank you :-)
> 
>>
>>> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
>>> index dffeabde0..ac3b8fa00 100644
>>> --- a/src/vbox/vbox_tmpl.c
>>> +++ b/src/vbox/vbox_tmpl.c
>>> @@ -378,6 +378,8 @@ _unregisterMachine(vboxDriverPtr data, vboxIID
>>> *iid, IMachine **machine)
>>>  {
>>>  nsresult rc;
>>>  vboxArray media = VBOX_ARRAY_INITIALIZER;
>>> +size_t i;
>>> +
>>>  rc = data->vboxObj->vtbl->FindMachine(data->vboxObj, iid-
 value, machine);
>>>  if (NS_FAILED(rc)) {
>>>  virReportError(VIR_ERR_NO_DOMAIN, "%s",
>>> @@ -385,12 +387,24 @@ _unregisterMachine(vboxDriverPtr data,
>>> vboxIID *iid, IMachine **machine)
>>>  return rc;
>>>  }
>>>  
>>> -/* We're not interested in the array returned by the
>>> Unregister method,
>>> - * but in the side effect of unregistering the virtual
>>> machine. In order
>>> - * to call the Unregister method correctly we need to use the
>>> vboxArray
>>> - * wrapper here. */
>>
>> I think you should keep the second sentence here - "In order to
>> call..."
>>  - IDC either way, but that would seem to still be important.
>>
> 
> Since this patch adds a code that loops over this array to close each
> IMedium instance it's self-documenting so no there's need for code
> comments here.
> 

That's fine - for you I'm assuming it's obvious why the wrapper is
needed to call Unregister, but for me - less so.

>>>  rc = vboxArrayGetWithUintArg(, *machine, (*machine)-
 vtbl->Unregister,
>>> - CleanupMode_DetachAllReturnNone);
>>> + CleanupMode_DetachAllReturnHardDi
>>> sksOnly);
>>> +
>>> +if (NS_FAILED(rc))
>>> +goto cleanup;
>>> +
>>> +/* close each medium attached to VM to remove from media
>>> registry */
>>> +for (i = 0; i < media.count; i++) {
>>> +IMedium *medium = media.items[i];
>>> +
>>> +if (!medium)
>>> +continue;
>>> +
>>
>> So the vboxSnapshotRedefine and vboxDomainSnapshotDeleteMetadataOnly
>> APIs that use CleanupMode_DetachAllReturnHardDisksOnly seem to go
>> through some great pain to determine whether it's a "fake" disk or
>> not -
>> would this code need the same kind of logic?
>>
>> /me wonders how much the existing code could be made more common -
>> beyond the "isCurrent" in the latter function the code appears to be
>> the
>> same. If this code needs it, then it might be worth the effort to
>> pass
>> 'isCurrent' to some common helper and for the other caller, just pass
>> true instead... Which would be similar if this code needed that.
> 
> I intentionally tried to stay away from touching the snapshot related
> functions even though it seems that they could share some code. Since
> I've been working on this on and off between projects at work, at
> initial stages it seemed like a couple of lines of code "here and
> there" would let me set/change vbox storage controller models via
> libvirtxml. Unfortunately, as I dug deeper and tested all sorts of
> combinations more bugs (even in master branch) seemed to pop out and
> the series grew to be much bigger than I originally intended. However,
> since I'll have to reorganize the commits for V2 anyway, I'll take a
> look at those functions and see what I can do.
> 

I try to avoid snapshot too ;-)  I understand how making adjustments in
libvirt can be like peeling back layers of an onion that cause you to
cry each time you have to enter some other piece of code because either
you discover some new bug/issue or what you're doing affects some other
piece of code. That is though one reason why we encourage using "small
enough" functionality changing/fixing patches rather than some massive
patch doing many different things. Another reason for smaller patches
has to do with git bisect and being able to determine which patch broke
some functionality - you bisect back to some 800 line patch bomb and
attempt to figure out what caused the failure...  Many times what I find
myself doing when I trip across some existing issue is stashing work and
rebasing to the patch before I've stashed, generating a patch for the
issue I uncover 

[libvirt] [PATCH 4/5] qemu: Drop qemuMonitorGetMigrationCapability

[Jiri Denemark]

The only remaining user of qemuMonitorGetMigrationCapability is our test
suite. Let's replace qemuMonitorGetMigrationCapability with
qemuMonitorGetMigrationCapabilities there and drop the unused function.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_monitor.c  | 19 ---
 src/qemu/qemu_monitor.h  |  2 --
 src/qemu/qemu_monitor_json.c | 18 --
 src/qemu/qemu_monitor_json.h |  2 --
 tests/qemumonitorjsontest.c  | 16 ++--
 5 files changed, 10 insertions(+), 47 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 8ffce5a35d..55b123e5f5 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3938,25 +3938,6 @@ qemuMonitorGetMigrationCapabilities(qemuMonitorPtr mon,
 }
 
 
-/**
- * Returns 1 if @capability is supported, 0 if it's not, or -1 on error.
- */
-int
-qemuMonitorGetMigrationCapability(qemuMonitorPtr mon,
-  qemuMonitorMigrationCaps capability)
-{
-VIR_DEBUG("capability=%d", capability);
-
-QEMU_CHECK_MONITOR(mon);
-
-/* No capability is supported without JSON monitor */
-if (!mon->json)
-return 0;
-
-return qemuMonitorJSONGetMigrationCapability(mon, capability);
-}
-
-
 int
 qemuMonitorSetMigrationCapability(qemuMonitorPtr mon,
   qemuMonitorMigrationCaps capability,
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 57893c61c6..0365b0f397 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -714,8 +714,6 @@ VIR_ENUM_DECL(qemuMonitorMigrationCaps);
 
 int qemuMonitorGetMigrationCapabilities(qemuMonitorPtr mon,
 char ***capabilities);
-int qemuMonitorGetMigrationCapability(qemuMonitorPtr mon,
-  qemuMonitorMigrationCaps capability);
 int qemuMonitorSetMigrationCapability(qemuMonitorPtr mon,
   qemuMonitorMigrationCaps capability,
   bool state);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 663fce3c3c..f7567eb771 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6068,24 +6068,6 @@ qemuMonitorJSONGetMigrationCapabilities(qemuMonitorPtr 
mon,
 }
 
 
-int
-qemuMonitorJSONGetMigrationCapability(qemuMonitorPtr mon,
-  qemuMonitorMigrationCaps capability)
-{
-int ret;
-char **capsList = NULL;
-const char *cap = qemuMonitorMigrationCapsTypeToString(capability);
-
-if (qemuMonitorJSONGetMigrationCapabilities(mon, ) < 0)
-return -1;
-
-ret = virStringListHasString((const char **) capsList, cap);
-
-virStringListFree(capsList);
-return ret;
-}
-
-
 int
 qemuMonitorJSONSetMigrationCapability(qemuMonitorPtr mon,
   qemuMonitorMigrationCaps capability,
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 7c45be6725..b17348a119 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -146,8 +146,6 @@ int qemuMonitorJSONGetMigrationStats(qemuMonitorPtr mon,
 
 int qemuMonitorJSONGetMigrationCapabilities(qemuMonitorPtr mon,
 char ***capabilities);
-int qemuMonitorJSONGetMigrationCapability(qemuMonitorPtr mon,
-  qemuMonitorMigrationCaps capability);
 int qemuMonitorJSONSetMigrationCapability(qemuMonitorPtr mon,
   qemuMonitorMigrationCaps capability,
   bool state);
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 475fd270e1..4d3b738e52 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -2214,7 +2214,8 @@ 
testQemuMonitorJSONqemuMonitorJSONGetMigrationCapability(const void *data)
 virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
 qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt);
 int ret = -1;
-int cap;
+const char *cap;
+char **caps = NULL;
 const char *reply =
 "{"
 "\"return\": ["
@@ -2234,12 +2235,14 @@ 
testQemuMonitorJSONqemuMonitorJSONGetMigrationCapability(const void *data)
"{\"return\":{}}") < 0)
 goto cleanup;
 
-cap = 
qemuMonitorJSONGetMigrationCapability(qemuMonitorTestGetMonitor(test),
-  
QEMU_MONITOR_MIGRATION_CAPS_XBZRLE);
-if (cap != 1) {
+if (qemuMonitorGetMigrationCapabilities(qemuMonitorTestGetMonitor(test),
+) < 0)
+goto cleanup;
+
+cap = 
qemuMonitorMigrationCapsTypeToString(QEMU_MONITOR_MIGRATION_CAPS_XBZRLE);
+if (!virStringListHasString((const char **) caps, cap)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   

[libvirt] [PATCH 1/5] qemu: Create a wrapper around qemuMonitorSetCapabilities

[Jiri Denemark]

The new function is called qemuProcessInitMonitor and it will enter/exit
the monitor so that the caller doesn't have to deal with this.

The goal of this patch is to simplify the code in qemuConnectMonitor
which would otherwise be a bit hairy after the following patch.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_process.c | 34 +-
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e062194294..24675498a2 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1730,12 +1730,31 @@ qemuProcessMonitorLogFree(void *opaque)
 virObjectUnref(logCtxt);
 }
 
+
+static int
+qemuProcessInitMonitor(virQEMUDriverPtr driver,
+   virDomainObjPtr vm,
+   qemuDomainAsyncJob asyncJob)
+{
+int ret;
+
+if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
+return -1;
+
+ret = qemuMonitorSetCapabilities(QEMU_DOMAIN_PRIVATE(vm)->mon);
+
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ret = -1;
+
+return ret;
+}
+
+
 static int
 qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob,
qemuDomainLogContextPtr logCtxt)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
-int ret = -1;
 qemuMonitorPtr mon = NULL;
 unsigned long long timeout = 0;
 
@@ -1794,13 +1813,12 @@ qemuConnectMonitor(virQEMUDriverPtr driver, 
virDomainObjPtr vm, int asyncJob,
 return -1;
 }
 
+if (qemuProcessInitMonitor(driver, vm, asyncJob) < 0)
+return -1;
 
 if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
 return -1;
 
-if (qemuMonitorSetCapabilities(priv->mon) < 0)
-goto cleanup;
-
 if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT) &&
 qemuMonitorSetMigrationCapability(priv->mon,
   QEMU_MONITOR_MIGRATION_CAPS_EVENTS,
@@ -1809,12 +1827,10 @@ qemuConnectMonitor(virQEMUDriverPtr driver, 
virDomainObjPtr vm, int asyncJob,
 virQEMUCapsClear(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
 }
 
-ret = 0;
-
- cleanup:
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
-ret = -1;
-return ret;
+return -1;
+
+return 0;
 }
 
 
-- 
2.14.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/5] qemu: Store supported migration capabilities in a bitmap

[Jiri Denemark]

Each time we need to check whether a given migration capability is
supported by QEMU, we call query-migrate-capabilities QMP command and
lookup the capability in the returned list. Asking for the list of
supported capabilities once when we connect to QEMU and storing the
result in a bitmap is much better and we don't need to enter a monitor
just to check whether a migration capability is supported.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_domain.c  | 68 +
 src/qemu/qemu_domain.h  |  9 +++
 src/qemu/qemu_process.c | 13 +-
 3 files changed, 78 insertions(+), 12 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 05e8b96aa4..a8cabc5727 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1767,6 +1767,9 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr 
priv)
 priv->namespaces = NULL;
 
 priv->reconnectBlockjobs = VIR_TRISTATE_BOOL_ABSENT;
+
+virBitmapFree(priv->migrationCaps);
+priv->migrationCaps = NULL;
 }
 
 
@@ -10122,3 +10125,68 @@ qemuDomainGetMachineName(virDomainObjPtr vm)
 
 return ret;
 }
+
+
+int
+qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ qemuDomainAsyncJob asyncJob)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+char **caps = NULL;
+char **capStr;
+int ret = -1;
+int rc;
+
+if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
+return -1;
+
+rc = qemuMonitorGetMigrationCapabilities(priv->mon, );
+
+if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
+goto cleanup;
+
+if (!caps) {
+ret = 0;
+goto cleanup;
+}
+
+priv->migrationCaps = virBitmapNew(QEMU_MONITOR_MIGRATION_CAPS_LAST);
+if (!priv->migrationCaps)
+goto cleanup;
+
+for (capStr = caps; *capStr; capStr++) {
+int cap = qemuMonitorMigrationCapsTypeFromString(*capStr);
+
+if (cap < 0) {
+VIR_DEBUG("Unknown migration capability: '%s'", *capStr);
+} else {
+ignore_value(virBitmapSetBit(priv->migrationCaps, cap));
+VIR_DEBUG("Found migration capability: '%s'", *capStr);
+}
+}
+
+if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) {
+if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
+goto cleanup;
+
+rc = qemuMonitorSetMigrationCapability(priv->mon,
+   
QEMU_MONITOR_MIGRATION_CAPS_EVENTS,
+   true);
+
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
+goto cleanup;
+
+if (rc < 0) {
+virResetLastError();
+VIR_DEBUG("Cannot enable migration events; clearing capability");
+virQEMUCapsClear(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
+}
+}
+
+ret = 0;
+
+ cleanup:
+virStringListFree(caps);
+return ret;
+}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 5201c6a0ac..fb20d8ea63 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -323,6 +323,10 @@ struct _qemuDomainObjPrivate {
 
 /* Tracks blockjob state for vm. Valid only while reconnecting to qemu. */
 virTristateBool reconnectBlockjobs;
+
+/* Migration capabilities. Rechecked on reconnect, not to be saved in
+ * private XML. */
+virBitmapPtr migrationCaps;
 };
 
 # define QEMU_DOMAIN_PRIVATE(vm)   \
@@ -978,4 +982,9 @@ qemuDomainFixupCPUs(virDomainObjPtr vm,
 char *
 qemuDomainGetMachineName(virDomainObjPtr vm);
 
+int
+qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ qemuDomainAsyncJob asyncJob);
+
 #endif /* __QEMU_DOMAIN_H__ */
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 24675498a2..cea2f90ce1 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1816,18 +1816,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, 
virDomainObjPtr vm, int asyncJob,
 if (qemuProcessInitMonitor(driver, vm, asyncJob) < 0)
 return -1;
 
-if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
-return -1;
-
-if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT) &&
-qemuMonitorSetMigrationCapability(priv->mon,
-  QEMU_MONITOR_MIGRATION_CAPS_EVENTS,
-  true) < 0) {
-VIR_DEBUG("Cannot enable migration events; clearing capability");
-virQEMUCapsClear(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
-}
-
-if (qemuDomainObjExitMonitor(driver, vm) < 0)
+if (qemuDomainCheckMigrationCapabilities(driver, vm, asyncJob) < 0)
 return -1;
 
 return 0;
-- 
2.14.2

--
libvir-list mailing list

[libvirt] [PATCH 5/5] qemu: Enhance debug message in qemuMonitorSetMigrationCapability

[Jiri Denemark]

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_monitor.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 55b123e5f5..64efb89e83 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3943,7 +3943,8 @@ qemuMonitorSetMigrationCapability(qemuMonitorPtr mon,
   qemuMonitorMigrationCaps capability,
   bool state)
 {
-VIR_DEBUG("capability=%d", capability);
+VIR_DEBUG("capability=%s, state=%d",
+  qemuMonitorMigrationCapsTypeToString(capability), state);
 
 QEMU_CHECK_MONITOR_JSON(mon);
 
-- 
2.14.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 0/5] qemu: Improve the way we handle migration capabilities

[Jiri Denemark]

Jiri Denemark (5):
  qemu: Create a wrapper around qemuMonitorSetCapabilities
  qemu: Store supported migration capabilities in a bitmap
  qemu: Use bitmap with migration capabilities
  qemu: Drop qemuMonitorGetMigrationCapability
  qemu: Enhance debug message in qemuMonitorSetMigrationCapability

 src/qemu/qemu_domain.c   | 75 
 src/qemu/qemu_domain.h   |  9 ++
 src/qemu/qemu_driver.c   | 32 ---
 src/qemu/qemu_migration.c| 45 +++---
 src/qemu/qemu_migration.h|  4 +++
 src/qemu/qemu_monitor.c  | 22 ++---
 src/qemu/qemu_monitor.h  |  2 --
 src/qemu/qemu_monitor_json.c | 18 ---
 src/qemu/qemu_monitor_json.h |  2 --
 src/qemu/qemu_process.c  | 43 ++---
 tests/qemumonitorjsontest.c  | 16 ++
 11 files changed, 163 insertions(+), 105 deletions(-)

-- 
2.14.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 3/5] qemu: Use bitmap with migration capabilities

[Jiri Denemark]

All calls to qemuMonitorGetMigrationCapability in QEMU driver are
replaced with qemuMigrationCapsGet.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_domain.c|  7 +++
 src/qemu/qemu_driver.c| 32 +---
 src/qemu/qemu_migration.c | 45 ++---
 src/qemu/qemu_migration.h |  4 
 4 files changed, 50 insertions(+), 38 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index a8cabc5727..1dcf263ce5 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -10184,6 +10184,13 @@ qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr 
driver,
 }
 }
 
+/* Migration events capability must always be enabled, clearing it from
+ * migration capabilities bitmap makes sure it won't be touched anywhere
+ * else.
+ */
+ignore_value(virBitmapClearBit(priv->migrationCaps,
+   QEMU_MONITOR_MIGRATION_CAPS_EVENTS));
+
 ret = 0;
 
  cleanup:
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fb4d722368..7f77dcb994 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13413,20 +13413,17 @@ qemuDomainMigrateGetCompressionCache(virDomainPtr dom,
 
 priv = vm->privateData;
 
-qemuDomainObjEnterMonitor(driver, vm);
-
-ret = qemuMonitorGetMigrationCapability(
-priv->mon,
-QEMU_MONITOR_MIGRATION_CAPS_XBZRLE);
-if (ret == 0) {
+if (!qemuMigrationCapsGet(vm, QEMU_MONITOR_MIGRATION_CAPS_XBZRLE)) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("Compressed migration is not supported by "
  "QEMU binary"));
-ret = -1;
-} else if (ret > 0) {
-ret = qemuMonitorGetMigrationCacheSize(priv->mon, cacheSize);
+goto endjob;
 }
 
+qemuDomainObjEnterMonitor(driver, vm);
+
+ret = qemuMonitorGetMigrationCacheSize(priv->mon, cacheSize);
+
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
 ret = -1;
 
@@ -13467,21 +13464,18 @@ qemuDomainMigrateSetCompressionCache(virDomainPtr dom,
 
 priv = vm->privateData;
 
-qemuDomainObjEnterMonitor(driver, vm);
-
-ret = qemuMonitorGetMigrationCapability(
-priv->mon,
-QEMU_MONITOR_MIGRATION_CAPS_XBZRLE);
-if (ret == 0) {
+if (!qemuMigrationCapsGet(vm, QEMU_MONITOR_MIGRATION_CAPS_XBZRLE)) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("Compressed migration is not supported by "
  "QEMU binary"));
-ret = -1;
-} else if (ret > 0) {
-VIR_DEBUG("Setting compression cache to %llu B", cacheSize);
-ret = qemuMonitorSetMigrationCacheSize(priv->mon, cacheSize);
+goto endjob;
 }
 
+qemuDomainObjEnterMonitor(driver, vm);
+
+VIR_DEBUG("Setting compression cache to %llu B", cacheSize);
+ret = qemuMonitorSetMigrationCacheSize(priv->mon, cacheSize);
+
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
 ret = -1;
 
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index b286d68061..72edbb667c 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1274,17 +1274,12 @@ qemuMigrationSetOption(virQEMUDriverPtr driver,
 qemuDomainObjPrivatePtr priv = vm->privateData;
 int ret;
 
-if (qemuDomainObjEnterMonitorAsync(driver, vm, job) < 0)
-return -1;
+if (!qemuMigrationCapsGet(vm, capability)) {
+if (!state) {
+/* Unsupported but we want it off anyway */
+return 0;
+}
 
-ret = qemuMonitorGetMigrationCapability(priv->mon, capability);
-
-if (ret < 0) {
-goto cleanup;
-} else if (ret == 0 && !state) {
-/* Unsupported but we want it off anyway */
-goto cleanup;
-} else if (ret == 0) {
 if (job == QEMU_ASYNC_JOB_MIGRATION_IN) {
 virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
_("Migration option '%s' is not supported by "
@@ -1296,15 +1291,17 @@ qemuMigrationSetOption(virQEMUDriverPtr driver,
  "source QEMU binary"),
qemuMonitorMigrationCapsTypeToString(capability));
 }
-ret = -1;
-goto cleanup;
+return -1;
 }
 
+if (qemuDomainObjEnterMonitorAsync(driver, vm, job) < 0)
+return -1;
+
 ret = qemuMonitorSetMigrationCapability(priv->mon, capability, state);
 
- cleanup:
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
 ret = -1;
+
 return ret;
 }
 
@@ -5923,12 +5920,8 @@ qemuMigrationReset(virQEMUDriverPtr driver,
 goto cleanup;
 
 for (cap = 0; cap < QEMU_MONITOR_MIGRATION_CAPS_LAST; cap++) {
-/* "events" capability is set (when supported) in qemuConnectMonitor
- * and should never be cleared */
-if (cap == QEMU_MONITOR_MIGRATION_CAPS_EVENTS)
- 

Re: [libvirt] [PATCH 1/3] daemon: finish threads on close

[Nikolay Shirokovskiy]

On 18.10.2017 14:17, John Ferlan wrote:
> [...]
> 
>> So I can split this patch to 2 and clear servers hash table as you
>> suggusted instead of tossing virThreadPoolFree and add 2 extra
>> patches to make referencing at virNetDaemonAddServerPostExec straight.
>> This will take almost no time thus let's move to the other parts of the 
>> series.
>>
>> Nikolay
>>
> 
> As I said originally I wasn't considering the StateShutdown part of this
> series. Post that separately, but be sure to properly state what the
> purpose is. I think there is one sentence from the patch1 commit

Ok.

> message. It doesn't seem that it would avoid a crash on its own. When I
> first took a quick look I wondered what the difference with that new
> state was and the stateStop functionality - then I noted stateStop
> functionality in daemon/libvirtd.c was gated on WITH_DBUS, but that's
> about as far as I got to thinking about adding a new state and its
> usefulness or purpose.

stateShutdown does not really introduces a state. It just helps
to terminate state driver.

Nikolay

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/3] daemon: finish threads on close

[John Ferlan]

[...]

> So I can split this patch to 2 and clear servers hash table as you
> suggusted instead of tossing virThreadPoolFree and add 2 extra
> patches to make referencing at virNetDaemonAddServerPostExec straight.
> This will take almost no time thus let's move to the other parts of the 
> series.
> 
> Nikolay
> 

As I said originally I wasn't considering the StateShutdown part of this
series. Post that separately, but be sure to properly state what the
purpose is. I think there is one sentence from the patch1 commit
message. It doesn't seem that it would avoid a crash on its own. When I
first took a quick look I wondered what the difference with that new
state was and the stateStop functionality - then I noted stateStop
functionality in daemon/libvirtd.c was gated on WITH_DBUS, but that's
about as far as I got to thinking about adding a new state and its
usefulness or purpose.

John


[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] maint: Replace tabs with spaces in all source files in repo

[Ján Tomko]

On Tue, Oct 17, 2017 at 05:24:30PM +0200, Erik Skultety wrote:

So we have a syntax-check rule to catch all tab indents but it naturally
can't catch tab spacing, i.e. as a delimiter. This patch is a result of
running 'vim -en +retab +wq'
(using tabstop=8 softtabstop=4 shiftwidth=4 expandtab) on each file from
a list generated by the following:
find . -regextype gnu-awk \
-regex ".*\.(rng|syms|html|s?[ch]|py|pl|php(\.code)?)(\.in)?" \
| xargs git grep -lP "\t"

Signed-off-by: Erik Skultety 
---
docs/search.php.code.in | 350 ++--
include/libvirt/virterror.h | 278 +--
src/bhyve/bhyve_monitor.c   |   2 +-
src/bhyve/bhyve_process.c   |   2 +-
src/conf/node_device_conf.h |  52 +++
src/conf/nwfilter_conf.h|   2 +-
src/conf/virnodedeviceobj.c |   2 +-
src/conf/virnodedeviceobj.h |   4 +-
src/qemu/qemu_domain.h  |   8 +-
src/qemu/qemu_monitor.h |   2 +-
src/util/viraudit.h |   4 +-
src/util/virconf.c  |  38 ++---
src/util/virconf.h  |  10 +-
src/util/virerror.h |   6 +-
src/util/virsysinfo.c   |   2 +-
src/util/virxml.c   |   8 +-
src/xen/xen_driver.h|   2 +-
src/xen/xen_hypervisor.c| 168 ++---
src/xen/xen_hypervisor.h|   2 +-
src/xen/xend_internal.c |  12 +-
src/xen/xend_internal.h |   8 +-
src/xen/xs_internal.c   |  20 +--
src/xen/xs_internal.h   |  14 +-
23 files changed, 498 insertions(+), 498 deletions(-)



There is no point in replacing tabs with spaces in the comments just to
delete them.

NACK to the following hunks:


diff --git a/src/util/virconf.c b/src/util/virconf.c
index e505848f1..a88cc9901 100644
--- a/src/util/virconf.c
+++ b/src/util/virconf.c
@@ -45,9 +45,9 @@
VIR_LOG_INIT("util.conf");

/
- * *
- * Structures and macros used by the mini parser   *
- * *
+ *  *
+ *  Structures and macros used by the mini parser   *
+ *  *
 /

typedef struct _virConfParserCtxt virConfParserCtxt;
@@ -69,16 +69,16 @@ struct _virConfParserCtxt {

#define SKIP_BLANKS_AND_EOL \
  do { while ((ctxt->cur < ctxt->end) && (c_isblank(CUR) || IS_EOL(CUR))) { \
- if (CUR == '\n') ctxt->line++; \
+ if (CUR == '\n') ctxt->line++; \
 ctxt->cur++; } } while (0)


... with the exception of this line ^^

ACK to the rest.

Jan


#define SKIP_BLANKS \
  do { while ((ctxt->cur < ctxt->end) && (c_isblank(CUR)))  \
  ctxt->cur++; } while (0)

/
- * *
- * Structures used by configuration data   *
- * *
+ *  *
+ *  Structures used by configuration data   *
+ *  *
 /

VIR_ENUM_IMPL(virConf, VIR_CONF_LAST,
@@ -134,9 +134,9 @@ virConfErrorHelper(const char *file, const char *func, 
size_t line,


/
- * *
- * Structures allocations and deallocations*
- * *
+ *  *
+ *  Structures allocations and deallocations*
+ *  *
 /

/**
@@ -261,9 +261,9 @@ virConfAddEntry(virConfPtr conf, char *name, 
virConfValuePtr value, char *comm)
}

/
- * *
- * Serialization   *
- * *
+ *  *
+ *  Serialization

Re: [libvirt] [PATCH 2/2] maint: Remove not-so-much informative block commentaries

[Ján Tomko]

On Tue, Oct 17, 2017 at 05:24:31PM +0200, Erik Skultety wrote:

There were a bunch of commentary blocks that were literally useless in
terms of describing what the code following them does, since most of
them were documenting "the obvious" or it just wouldn't help at all.

Signed-off-by: Erik Skultety 
---
daemon/remote.c |  7 ---
src/network/bridge_driver.c |  2 --
src/util/virconf.c  | 33 -
src/util/virerror.h |  5 -
src/util/virxml.c   |  6 --
src/vbox/vbox_XPCOMCGlue.c  |  9 -
src/xen/xend_internal.c | 11 ---
src/xen/xs_internal.c   | 10 --
src/xenconfig/xen_sxpr.c|  5 -
9 files changed, 88 deletions(-)



ACK

Jan


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/2] Increase default file handle limits for virtlogd

[Christian Ehrhardt]

The initial assumption was ~2 files per guest, but some common setups
like Openstack drive up to 4 files per guest.

E.g. on Arm where the following XML leads to 4 file handles:

  
  
  


  
  
  


With that in mind and the target to support 4k guests by default we
should raise the limit to 16k.

Signed-off-by: Christian Ehrhardt 
---
 src/logging/virtlogd.service.in | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/logging/virtlogd.service.in b/src/logging/virtlogd.service.in
index ec7712b..aa9aa69 100644
--- a/src/logging/virtlogd.service.in
+++ b/src/logging/virtlogd.service.in
@@ -15,9 +15,11 @@ ExecReload=/bin/kill -USR1 $MAINPID
 OOMScoreAdjust=-900
 # Need to have at least one file open per guest (eg QEMU
 # stdio log), but might be more (eg serial console logs)
+# A common case is OpenStack which often has up to 4 file
+# handles per guest.
 # libvirtd.service written to expect 4096 guests, so if we
-# guess at 2 log files per guest here (stdio + 1 serial):
-LimitNOFILE=8192
+# guess at 4 files per guest here that is 16k:
+LimitNOFILE=16384
 
 [Install]
 Also=virtlogd.socket
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 0/2] Further Increase default file handle limits

[Christian Ehrhardt]

In 27cd7635 new default limits were set:
  Author: Daniel P. Berrange 
  Date:   Wed Mar 15 16:51:51 2017 +

Increase default file handle limits for daemons

But I faced some constraints with these values and think it is time to
reconsider the defaults to only have to be tweaked in really uncommon cases.

Christian Ehrhardt (2):
  Increase default file handle limits for virtlogd
  Increase default file handle limits for virtlockd

 src/locking/virtlockd.service.in | 4 ++--
 src/logging/virtlogd.service.in  | 6 --
 2 files changed, 6 insertions(+), 4 deletions(-)

-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/2] Increase default file handle limits for virtlockd

[Christian Ehrhardt]

The assumption so far was an average of 4 disks per guest.
But some architectures, like s390x, still often use plenty of smaller disks.

To include those in the considerations an assumption of an average of 10
disks is more reasonable.

Signed-off-by: Christian Ehrhardt 
---
 src/locking/virtlockd.service.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/locking/virtlockd.service.in b/src/locking/virtlockd.service.in
index 928bf63..07e4847 100644
--- a/src/locking/virtlockd.service.in
+++ b/src/locking/virtlockd.service.in
@@ -15,8 +15,8 @@ ExecReload=/bin/kill -USR1 $MAINPID
 OOMScoreAdjust=-900
 # Needs to allow for max guests * average disks per guest
 # libvirtd.service written to expect 4096 guests, so if we
-# allow for 4 disks per guest, we get:
-LimitNOFILE=16384
+# allow for 10 disks per guest, we get:
+LimitNOFILE=40960
 
 [Install]
 Also=virtlockd.socket
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/3] daemon: finish threads on close

[Nikolay Shirokovskiy]

On 17.10.2017 15:34, John Ferlan wrote:
> 
> 
> On 10/17/2017 03:40 AM, Nikolay Shirokovskiy wrote:
>>
>>
>> On 16.10.2017 15:47, John Ferlan wrote:
>>>
>>>
>>> On 09/27/2017 08:45 AM, Nikolay Shirokovskiy wrote:
 Current daemon shutdown can cause crashes. The problem is that threads
 serving client request are joined on daemon dispose after drivers already
 cleaned up. But this threads typically uses drivers and thus crashes come.
 We need to join threads before virStateCleanup. virNetDaemonClose is
 a good candidate.

 But it turns out that we can hang on join. The problem is that at this
 moment event loop is not functional and for example threads waiting for
 qemu response will never finish. Let's introduce extra shutdown step
 for drivers so that they can signal any API calls in progress to finish.
 ---
  daemon/libvirtd.c|  2 ++
  src/driver-state.h   |  4 
  src/libvirt.c| 18 ++
  src/libvirt_internal.h   |  1 +
  src/libvirt_private.syms |  1 +
  src/rpc/virnetserver.c   |  5 +++--
  6 files changed, 29 insertions(+), 2 deletions(-)

>>>
>>> So - first off - this patch is doing 2 things:
>>>
>>>  1. Introduce a new driver State function - "Shutdown"
>>>
>>>  2. Moving virThreadPoolFree from virNetServerDispose to virNetServerClose
>>>
>>> The two do not seem to be related so, they'd need to be separated.
>>>
>>> It appears the motivation behind the StateShutdown is to "force" the
>>> close the qemu monitor and agent, but it doesn't seem that's really
>>> related to the virNet{Daemon|Server}* timing issue. So let's consider
>>> that separately...  and I'm not considering it now...
>>>
>>> Focusing more on #2 - the move of the virThreadPoolFree would seem to
>>> hasten the eventual free. Or more to the point ensure it happens. While
>>> that does resolve the problem, I don't think it's the best or actual fix
>>> to what it appears the real problem is.
>>
>> The problem is related to the fact that virThreadPoolFree does 2 things.
>>
>> 1. finishes all worker threads
>> 2. do actual free
>>
>> What I want is to hasten #1 not actual free.
>>
> 
> Right - but because your fix just calls virThreadPoolFree and sets
> srv->workers = NULL - that caused me to wonder why we had to do that
> when it "should" be done once we're done using the servers hash table
> entry... IOW: Something should Unref sooner.
> 
> BTW: Your 1 and 2 are all the same to me - the Unref being the more key
> component because we know virObjectUnref(srv) should be the "last"
> reference to @srv.  It's one of those ordering things. It should be do
> A, then B to allocate/start and then undo B and undo A on cleanup. In
> this case we partially undo B, then undo A, and eventually complete the
> undo B much later on.
> 
>>>
>>> Taking it from the Start/Alloc/Ref side - @srv gets a Ref at
>>> virNetServerNew and then again at virNetDaemonAddServer. So each @srv
>>> has two refs, which means in order for virNetServerDispose to be called,
>>> there would need to be two Unref's; however, I can only find one.
>>>
>>> During cleanup: @srv is Unref'd after virNetDaemonClose, but I'm not
>>> finding the other one. Do you recall where you may have seen it? I'm
>>
>> When the daemon object is unref'd at the end of daemon main function
>> servers are unrefed as part of daemon dispose.>
> 
> OK - right, virHashFree does end up making that free call, but as you
> note, much too late.
> 
>>
>>> assuming the answer is no, there wasn't one and hence why you moved that
>>> virThreadPoolFree call.
>>
>> Not entirely true. I moved this call so that all client request
>> are finished before we clean up drivers state. Otherwise client requests
>> will see disposed objects in the middele of operation and crashes occur.
>>
> 
> So while the move "works" it does so because it's bypassing the 'proper'
> way to resolve this by Unref() the table elements when the decision is
> made by the the code that it is done with them...
> 
>>>
>>> Since at virNetDaemonAddServer we add a Ref to @srv, then during
>>> virNetDaemonClose I'd expect that for each server on dmn->servers
>>> there'd be the virNetServerClose and a removal from the HashTable and
>>> Unref of the @srv object which I'm not seeing. If that was there, then
>>> the virNetServerDispose would call virThreadPoolFree right when the
>>> Unref was done on @srv. The better fix, I believe is a call to
>>> virHashRemoveAll in virNetDaemonClose which does that removal of @srv
>>> from the dmn->servers hash table. NB this would fix a memory leak since
>>> the eventual virHashFree(dmn->servers) doesn't do free the elements of
>>> the hash table when virNetDaemonDispose is called as a result of the
>>> virObjectUnref(dmn) at the bottom of main() in daemon/libvirtd.c.
>>
>> Servers hash table created with free function virObjectFreeHashData
>> which will unref servers when hash table is freed.
>>
>> As