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

[John Ferlan]

On 10/18/2017 07:29 AM, Jiri Denemark wrote:
> 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(-)
> 

Reviewed-by: John Ferlan 

John

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

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

[John Ferlan]

On 10/18/2017 07:29 AM, Jiri Denemark wrote:
> 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(-)
> 

Heads up - you'll have some merge work to do... I was able to cobble
together by going back quite a bit and merging/applying...

Anyway,

Reviewed-by: John Ferlan 

John

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

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

[John Ferlan]

On 10/18/2017 07:29 AM, Jiri Denemark wrote:
> 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(-)
> 

There's no corresponding adjustment to qemuDomainObjPrivateXMLFormat and
qemuDomainObjPrivateXMLParse in order to handle the restart scenario.

The rest of this looks OK, but do you need the Format/Parse logic for
the bitmap?  It was late in my day and I was too lazy to go chase seeing
as I already did the merging ;-)

John

> 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, &caps);
> +
> +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)
> -

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

[John Ferlan]

On 10/18/2017 07:29 AM, Jiri Denemark wrote:
> 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(-)
> 

Reviewed-by: John Ferlan 

John

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

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

[John Ferlan]

On 10/18/2017 07:29 AM, Jiri Denemark wrote:
> 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);
>  
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [Qemu-devel] [RFC 0/6] enable numa configuration before machine_init() from HMP/QMP

[Eduardo Habkost]

On Thu, Oct 19, 2017 at 04:28:59PM +0100, Daniel P. Berrange wrote:
> On Thu, Oct 19, 2017 at 11:21:22AM -0400, Igor Mammedov wrote:
> > - Original Message -
> > > From: "Daniel P. Berrange" 
> > > To: "Igor Mammedov" 
> > > Cc: "peter maydell" , pkre...@redhat.com, 
> > > ehabk...@redhat.com, coh...@redhat.com,
> > > qemu-de...@nongnu.org, arm...@redhat.com, pbonz...@redhat.com, 
> > > da...@gibson.dropbear.id.au
> > > Sent: Wednesday, October 18, 2017 5:30:10 PM
> > > Subject: Re: [Qemu-devel] [RFC 0/6] enable numa configuration before 
> > > machine_init() from HMP/QMP
> > > 
> > > On Tue, Oct 17, 2017 at 06:06:35PM +0200, Igor Mammedov wrote:
> > > > On Tue, 17 Oct 2017 16:07:59 +0100
> > > > "Daniel P. Berrange"  wrote:
> > > > 
> > > > > On Tue, Oct 17, 2017 at 09:27:02AM +0200, Igor Mammedov wrote:
> > > > > > On Mon, 16 Oct 2017 17:36:36 +0100
> > > > > > "Daniel P. Berrange"  wrote:
> > > > > >   
> > > > > > > On Mon, Oct 16, 2017 at 06:22:50PM +0200, Igor Mammedov wrote:
> > > > > > > > Series allows to configure NUMA mapping at runtime using QMP/HMP
> > > > > > > > interface. For that to happen it introduces a new '-paused' CLI
> > > > > > > > option
> > > > > > > > which allows to pause QEMU before machine_init() is run and
> > > > > > > > adds new set-numa-node HMP/QMP commands which in conjuction with
> > > > > > > > info hotpluggable-cpus/query-hotpluggable-cpus allow to 
> > > > > > > > configure
> > > > > > > > NUMA mapping for cpus.
> > > > > > > 
> > > > > > > What's the problem we're seeking solve here compared to what we
> > > > > > > currently
> > > > > > > do for NUMA configuration ?
> > > > > > From RHBZ1382425
> > > > > > "
> > > > > > Current -numa CLI interface is quite limited in terms that allow map
> > > > > > CPUs to NUMA nodes as it requires to provide cpu_index values which
> > > > > > are non obvious and depend on machine/arch. As result libvirt has to
> > > > > > assume/re-implement cpu_index allocation logic to provide valid
> > > > > > values for -numa cpus=... QEMU CLI option.
> > > > > 
> > > > > In broad terms, this problem applies to every device / object libvirt
> > > > > asks QEMU to create. For everything else libvirt is able to assign a
> > > > > "id" string, which is can then use to identify the thing later. The
> > > > > CPU stuff is different because libvirt isn't able to provide 'id'
> > > > > strings for each CPU - QEMU generates a psuedo-id internally which
> > > > > libvirt has to infer. The latter is the same problem we had with
> > > > > devices before '-device' was introduced allowing 'id' naming.
> > > > > 
> > > > > IMHO we should take the same approach with CPUs and start modelling
> > > > > the individual CPUs as something we can explicitly create with -object
> > > > > or -device. That way libvirt can assign names and does not have to
> > > > > care about CPU index values, and it all works just the same way as
> > > > > any other devices / object we create
> > > > > 
> > > > > ie instead of:
> > > > > 
> > > > >   -smp 8,sockets=4,cores=2,threads=1
> > > > >   -numa node,nodeid=0,cpus=0-3
> > > > >   -numa node,nodeid=1,cpus=4-7
> > > > > 
> > > > > we could do:
> > > > > 
> > > > >   -object numa-node,id=numa0
> > > > >   -object numa-node,id=numa1
> > > > >   -object cpu,id=cpu0,node=numa0,socket=0,core=0,thread=0
> > > > >   -object cpu,id=cpu1,node=numa0,socket=0,core=1,thread=0
> > > > >   -object cpu,id=cpu2,node=numa0,socket=1,core=0,thread=0
> > > > >   -object cpu,id=cpu3,node=numa0,socket=1,core=1,thread=0
> > > > >   -object cpu,id=cpu4,node=numa1,socket=2,core=0,thread=0
> > > > >   -object cpu,id=cpu5,node=numa1,socket=2,core=1,thread=0
> > > > >   -object cpu,id=cpu6,node=numa1,socket=3,core=0,thread=0
> > > > >   -object cpu,id=cpu7,node=numa1,socket=3,core=1,thread=0
> > > > the follow up question would be where do "socket=3,core=1,thread=0"
> > > > come from, currently these options are the function of
> > > > (-M foo -smp ...) and can be queried vi query-hotpluggble-cpus at
> > > > runtime after qemu parses -M and -smp options.
> > > 
> > > NB, I realize my example was open to mis-interpretation. The values I'm
> > > illustrating here for socket=3,core=1,thread=0 and *not* ID values, they
> > > are a plain enumeration of values. ie this is saying the 4th socket, the
> > > 2nd core and the 1st thread.  Internally QEMU might have the 2nd core
> > > with a core-id of 8, or 7038 or whatever architecture specific numbering
> > > scheme makes sense, but that's not what the mgmt app gives at the CLI
> > > level
> > Even though fixed properties/values simplicity is tempting and it might even
> > work for what we have implemented in qemu currently (well, SPAPR will need
> > refactoring (if possible) to meet requirements + compat stuff for current
> > machines with sparse IDs).
> > But I have to disagree here and try to oppose it.
> > 
> > QEMU models concrete platforms/hw with certain non abstract properties
> > and it's libvirt'

Re: [libvirt] [PATCH 5/6] vbox: Process controller definitions from xml.

[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 
> > 
> Lots of potentially use DEBUG information being lost. IDC, but seems
> a
> shame to not have it. Although it can be overkill when DEBUG is
> enabled
> - it does help sometimes in debugging problems. Your call...

I've removed those because they basically print out the values of the
XML I just passed in and I know them from XML already. I've added a
more useful (at least for me) debug statement in (WIP) V2 that prints
the arguments passed to VBOX's AttachDevice method - those are
'computed' based on XML input by this very code so it's more helpful to
know what those final values are when debugging.

> 
> > -
> > -if (type == VIR_STORAGE_TYPE_FILE && src) {
> > -IMedium *medium = NULL;
> > -vboxIID mediumUUID;
> > -PRUnichar *mediumFileUtf16 = NULL;
> > -PRUint32 storageBus = StorageBus_Null;
> > -PRUint32 deviceType = DeviceType_Null;
> > -PRUint32 accessMode = AccessMode_ReadOnly;
> > -PRInt32 deviceInst = 0;
> > -PRInt32 devicePort = 0;
> > -PRInt32 deviceSlot = 0;
> > +for (i = 0; i < def->ndisks; i++) {
> > +disk = def->disks[i];
> > +const char *src = virDomainDiskGetSource(disk);
> > +int type = virDomainDiskGetType(disk);
> >  
> > -VBOX_IID_INITIALIZE(&mediumUUID);
> > -VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16);
> > +if (type != VIR_STORAGE_TYPE_FILE) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +   _("Unsupported storage type %s, the
> > only supported "
> > + "type is %s"),
> > +   virStorageTypeToString(type),
> > +   virStorageTypeToString(VIR_STORAGE_TYPE
> > _FILE));
> > +return -1;
> > +}
> >  
> > -if (def->disks[i]->device ==
> > VIR_DOMAIN_DISK_DEVICE_DISK) {
> > -deviceType = DeviceType_HardDisk;
> > -accessMode = AccessMode_ReadWrite;
> > -} else if (def->disks[i]->device ==
> > VIR_DOMAIN_DISK_DEVICE_CDROM) {
> > -deviceType = DeviceType_DVD;
> > -accessMode = AccessMode_ReadOnly;
> > -} else if (def->disks[i]->device ==
> > VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
> > -deviceType = DeviceType_Floppy;
> > -accessMode = AccessMode_ReadWrite;
> > -} else {
> > -VBOX_UTF16_FREE(mediumFileUtf16);
> > -continue;
> > -}
> > +if (!src && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> 
> I think more typically there are checks against != *_CDROM ||
> *_FLOPPY
> although perhaps this check should go slightly later [1]
> 
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +   _("Missing disk source file path"));
> > +return -1;
> > +}
> >  
> > -gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj,
> > mediumFileUtf16,
> > -   deviceType,
> > accessMode, &medium);
> > +IMedium *medium = NULL;
> > +vboxIID mediumUUID;
> 
> Since you call vboxIIDUnalloc later on this [2], perhaps this should
> be
> initialized to NULL... Although, I see it only gets set when "if
> (src)",
> so perhaps that becomes the key for Unalloc too.
> 
> > +PRUnichar *mediumFileUtf16 = NULL;
> > +PRUint32 deviceType = DeviceType_Null;
> > +PRUint32 accessMode = AccessMode_ReadOnly;
> > +PRInt32 deviceSlot = disk->info.addr.drive.bus;
> > +PRInt32 devicePort = disk->info.addr.drive.unit;
> > +int model = -1;
> 
> We really prefer to see definitions grouped together at the top
> rather
> than in the middle of code
> 
> > +
> > +if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> 
> [1]  If you move that !src check here, then you're accomplishing the
> same result as previously but making it more obvious (at least for
> me)
> since _LUN isn't supported...
> 
> > +deviceType = DeviceType_HardDisk;
> > +accessMode = AccessMode_ReadWrite;
> > +} else if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
> > +deviceType = DeviceType_DVD;
> > +accessMode = AccessMode_ReadOnly;
> > +} else if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)
> > {
> > +deviceType = DeviceType_Floppy;
> > +accessMode = AccessMode_ReadWrite;
> > +} else {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +   _("Unsupported disk device type %s"),
> > +   virDomainDiskDeviceTypeToString(disk-
> > >device));
> > +ret = -1;
> > +goto cleanup;
> > +}
> 
> Als

Re: [libvirt] [PATCH v5 07/16] conf: Add/Allow parsing the encryption in the disk source

[Peter Krempa]

On Thu, Oct 05, 2017 at 09:22:14 -0400, John Ferlan wrote:
> Since the virStorageEncryptionPtr encryption; is a member of
>  _virStorageSource it really should be allowed to be a subelement
> of the disk  for various disk formats:
> 
>Source{File|Dir|Block|Volume}
>SourceProtocol{RBD|ISCSI|NBD|Gluster|Simple|HTTP}
> 
> NB: Simple includes sheepdog, ftp, ftps, tftp
> 
> That way we can set up to allow the  element to be
> formatted within the disk source, but we still need to be wary
> from whence the element was read - see keep track and when it
> comes to format the data, ensure it's written in the correct place.
> 
> Modify the qemuxml2argvtest to add a parse failure when there is an
>  as a child of  *and* an  as a child
> of .
> 
> The virschematest will read the new test files and validate from a
> RNG viewpoint things are fine.
> 
> Signed-off-by: John Ferlan 
> ---
>  docs/formatdomain.html.in  | 15 +++-
>  docs/schemas/domaincommon.rng  | 30 
>  src/conf/domain_conf.c | 68 --
>  src/util/virstoragefile.h  |  1 +
>  .../qemuxml2argv-luks-disks-source-both.xml| 40 +++
>  .../qemuxml2argv-luks-disks-source.args| 62 
>  .../qemuxml2argv-luks-disks-source.xml | 81 +
>  tests/qemuxml2argvtest.c   |  2 +
>  .../qemuxml2xmlout-luks-disks-source.xml   | 84 
> ++
>  .../qemuxml2xmlout-luks-disks.xml  | 46 +++-
>  tests/qemuxml2xmltest.c|  1 +
>  11 files changed, 420 insertions(+), 10 deletions(-)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source-both.xml
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks-source.xml
>  mode change 12 => 100644 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml
>

ACK with the same comment as for 1/16


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

Re: [libvirt] [PATCH v5 01/16] conf: Add/Allow parsing the auth in the disk source

[Peter Krempa]

On Thu, Oct 05, 2017 at 09:22:08 -0400, John Ferlan wrote:
> Since the virStorageAuthDefPtr auth; is a member of _virStorageSource
> it really should be allowed to be a subelement of the disk 
> for the RBD and iSCSI prototcols. That way we can set up to allow
> the  element to be formatted within the disk source.
> 
> Since we've allowed the  to be a child of , we'll need
> to keep track of how it was read so that when writing out we'll know
> whether to format as child of  or . For the argv2xml
> parsing, let's format under  as a preference. Do not allow
>  to be both a child of  and .
> 
> Modify the qemuxml2argvtest to add a parse failure when there is an
>  as a child of  *and* an  as a child of .
> 
> Add tests to validate that if the  was found in , then
> the resulting xml2xml and xml2arg works just fine.  The two new .args
> file are exact copies of the non "-source" version of the file.
> 
> The virschematest will read the new test files and validate from a
> RNG viewpoint things are fine
> 
> Update the virstoragefile, virstoragetest, and args2xml file to show
> the "preference" to place  as a child of .
> 
> Signed-off-by: John Ferlan 
> ---
>  docs/formatdomain.html.in  | 67 
> +-
>  docs/schemas/domaincommon.rng  | 18 +-
>  src/conf/domain_conf.c | 67 
> +-
>  src/util/virstoragefile.c  |  1 +
>  src/util/virstoragefile.h  |  1 +
>  .../qemuargv2xml-disk-drive-network-rbd-auth.xml   |  6 +-
>  ...ml2argv-disk-drive-network-source-auth-both.xml | 51 
>  ...emuxml2argv-disk-drive-network-source-auth.args | 32 +++
>  ...qemuxml2argv-disk-drive-network-source-auth.xml | 45 +++
>  tests/qemuxml2argvtest.c   |  2 +
>  ...muxml2xmlout-disk-drive-network-source-auth.xml | 49 
>  tests/qemuxml2xmltest.c|  1 +
>  tests/virstoragetest.c |  6 ++
>  13 files changed, 311 insertions(+), 35 deletions(-)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-source-auth-both.xml
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-source-auth.args
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-source-auth.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-source-auth.xml

[...]

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index c0e3c22213..74f2090d06 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index 74dee10f2f..c8bb1066fe 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -238,6 +238,7 @@ struct _virStorageSource {
>  virStorageNetHostDefPtr hosts;
>  virStorageSourcePoolDefPtr srcpool;
>  virStorageAuthDefPtr auth;
> +bool authDefined;

I find this name slightly misleading. How about authInherited?

At any rate, this patch should be applicable without conflicts and it
will help in doing security related stuff for the backing chain.

ACK

You can push this right now. I'll pick out a few other things form this
series which will be helpful for the blockdev-add saga and pass back the
rest for the iSCSI work (mainly with pre-blockdev qemus).



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

[libvirt] [jenkins-ci PATCH] guests: update libvirt-freebsd-10 jenkins secret

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---

Pushed under trivial rule and no chance to review it.

 guests/host_vars/libvirt-freebsd-10/vault.yml | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/guests/host_vars/libvirt-freebsd-10/vault.yml 
b/guests/host_vars/libvirt-freebsd-10/vault.yml
index ac437ba..37def4c 100644
--- a/guests/host_vars/libvirt-freebsd-10/vault.yml
+++ b/guests/host_vars/libvirt-freebsd-10/vault.yml
@@ -1,10 +1,10 @@
 $ANSIBLE_VAULT;1.1;AES256
-62633664623566363031326631323930303561616235373966306363366536326162
-6466333764383932646530323730386561656530353430330a636433373638613064643165643536
-32376334653030336334643865396162363061383066326362633165346164303831616464636337
-3861613765393666340a313931663337633762313538316230613536303939343862306532666564
-61313939666564626632363835363238653830633666383337323263326363323766633933633862
-32363166643231613864626263303035303631616665336531633761656335646166656232303936
-6664326135666535636334393165343666366631353365623937653564326465393265333135
-62323366363834636263386230356238333133623735373730356539323761306237623266363032
-3436
+3839383039653739373135336531646461306138616330366331336138646664376465346364
+3866656165393039613636306533303539383036623736310a33383836323036316632643835
+31383362386133626662303038373731633264653462316234373834323130626633383664613965
+6332643165363932300a393662343061646539336532356434326230313135623162623730313539
+37393036633937393864663735326138313538356366616435616237303663336662313262393832
+6365366631343635666333636438636539313266333261316236663731656534336261633836
+35643732306662373064623234633366343764613336633232663365316436346535626233633661
+63336437643161356364643764336365373430363238326261626565666232383432373438373037
+6437
-- 
2.13.6

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

[libvirt] [jenkins-ci PATCH] projects: rename libvirt-freebsd into libvirt-freebsd-10

[Pavel Hrdina]

We will have freebsd-11 as well.

Signed-off-by: Pavel Hrdina 
---

Pushed under trivial rule.

 projects/libvirt.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/projects/libvirt.yaml b/projects/libvirt.yaml
index e5946e2..a70a58c 100644
--- a/projects/libvirt.yaml
+++ b/projects/libvirt.yaml
@@ -19,7 +19,7 @@
 - libvirt-fedora-25
 - libvirt-fedora-26
 - libvirt-fedora-rawhide
-- libvirt-freebsd
+- libvirt-freebsd-10
   - autotools-syntax-check-job:
   parent_jobs: 'libvirt-master-build'
   machines:
-- 
2.13.6

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

Re: [libvirt] [PATCH v3] interfaces: Convert virInterfaceObjList to virObjectRWLockable

[John Ferlan]

[...]

>>>
>>> With the patch split in 2 introducing 2 distinct changes + the NULL check:
>>> Reviewed-by: Erik Skultety 
>>
>> Hopefully you reconsider the desire for 2 patches...
> 
> Well, since I was apparently fine with the change when reviewing the same
> changes to nodedev, I guess I should be fine with it now too, I don't know 
> what
> made me change my opinion as time has passed, nevermind, it's not a deal 
> breaker
> for me.
> 

OK - thanks - I did note while looking deeper that I didn't remove
virInterfaceObjListFree() from src/conf/virinterfaceobj.h, so I removed
it...

>>
>>>
>>>
>>> PS: I'm also sad, that we have two backends here just like we have in 
>>> nodedev
>>> with one of them being essentially useless (just like in nodedev) we have 
>>> this
>>> 'conf' generic code (just like in nodedev), yet in this case it's only used 
>>> in
>>> the test driver. I'd very much appreciate if those backends could be 
>>> adjusted
>>> in a way where we could make use of these functions. I can also imagine a
>>> cooperation of the udev backend with the nodedev driver where we have an 
>>> active
>>> connection to the monitor, thus reacting to all events realtime, instead of
>>> defining a bunch of filters and then letting udev re-enumerate the list of
>>> interfaces each and every time (and by saying that I'm also aware that udev 
>>> is
>>> actually the useless backend here).
>>>
>>> Erik
>>>
>>
>> Yeah - the driver code here is quite different/strange and could
>> possibly use a bit more convergence. I feel too battered and bruised
>> over this convergence right now though ;-)  Besides the differences
> 
> I hope it didn't sound like a request, it was meant to be more like a wish 
> that
> we should do something about it (sometime).
> 
> Erik
> 

I didn't take it that way... Just making sure no one would be waiting
for patches from me any time soon that did that type of convergence ;-)

Tks -

John

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

Re: [libvirt] [PATCH v3] interfaces: Convert virInterfaceObjList to virObjectRWLockable

[Erik Skultety]

On Thu, Oct 19, 2017 at 10:48:56AM -0400, John Ferlan wrote:
>
>
> On 10/19/2017 10:07 AM, Erik Skultety wrote:
> > On Fri, Oct 13, 2017 at 07:45:01AM -0400, John Ferlan wrote:
> >> Rather than a forward linked list, let's use the ObjectRWLockable object
> >> in order to manage the data.
> >>
> >> Requires numerous changes from List to Object management similar to
> >> many other drivers/vir*obj.c modules
> >>
> >
> > This patch should be split in two. One that converts it to RWLockable, the
> > other using a hash table instead of a linked list.
> >
> > [...]
>
> Personally, I don't recall during various conversions I've done to having:
>
> virObjectLockable parent;
>
> size_t count;
> virInterfaceObjPtr *objs;
>
> first, then converting to count/objs to (a) hash table(s). Although
> looking through history I see Michal had the networkobj code take a long
> winding path to get there, so it's not impossible - just IMO pointless.
>
> I think whenever I've converted from count/obs to a hash table with a
> 'real object', that the locking was done a the same time.
>
> As I recall, usually it's been converting from :
>
> size_t count;
> virInterfaceObjPtr *objs;
>
> to:
>
> virObjectLockable parent;
>
> /* name string -> virInterfaceObj  mapping
>  * for O(1), lockless lookup-by-name */
> virHashTable *objsName;
>
> then more recently the conversion from virObjectLockable to
> virObjectRWLockable
>
> Since this patch is taking @objs and converting to a hash table - it's
> avoiding the Lockable and going straight to RWLockable.
>
> >>
> >> @@ -253,9 +334,11 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr 
> >> interfaces,
> >>  {
> >>  virInterfaceObjPtr obj;
> >>
> >> -if ((obj = virInterfaceObjListFindByName(interfaces, def->name))) {
> >> +virObjectRWLockWrite(interfaces);
> >> +if ((obj = virInterfaceObjListFindByNameLocked(interfaces, 
> >> def->name))) {
> >>  virInterfaceDefFree(obj->def);
> >>  obj->def = def;
> >> +virObjectRWUnlock(interfaces);
> >>
> >>  return obj;
> >>  }
> >> @@ -263,13 +346,19 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr 
> >> interfaces,
> >>  if (!(obj = virInterfaceObjNew()))
> >>  return NULL;
> >>
> >> -if (VIR_APPEND_ELEMENT_COPY(interfaces->objs,
> >> -interfaces->count, obj) < 0) {
> >> -virInterfaceObjEndAPI(&obj);
> >> -return NULL;
> >> -}
> >> +if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0)
> >> +goto error;
> >> +virObjectRef(obj);
> >> +
> >>  obj->def = def;
> >> -return virObjectRef(obj);
> >> +virObjectRWUnlock(interfaces);
> >> +
> >> +return obj;
> >> +
> >> + error:
> >> +virInterfaceObjEndAPI(&obj);
> >> +virObjectRWUnlock(interfaces);
> >> +return NULL;
> >>  }
> >
> > ^This could be tweaked even more:
> >
>
> Sure, but in doing so eagle eyes now spots a problem in his own code...
>
>
> > diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c
> > index cf3626def..941893fc5 100644
> > --- a/src/conf/virinterfaceobj.c
> > +++ b/src/conf/virinterfaceobj.c
> > @@ -337,19 +337,15 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr 
> > interfaces,
> >  virObjectRWLockWrite(interfaces);
> >  if ((obj = virInterfaceObjListFindByNameLocked(interfaces, 
> > def->name))) {
> >  virInterfaceDefFree(obj->def);
> > -obj->def = def;
> > -virObjectRWUnlock(interfaces);
> > +} else {
> > +if (!(obj = virInterfaceObjNew()))
> > +return NULL;
>
> Leaving with RWLock, 
>
> >
> > -return obj;
> > +if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0)
> > +goto error;
> > +virObjectRef(obj);
> >  }
> >
> > -if (!(obj = virInterfaceObjNew()))
> > -return NULL;
> > -
> > -if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0)
> > -goto error;
> > -virObjectRef(obj);
> > -
> >  obj->def = def;
> >  virObjectRWUnlock(interfaces);
> >
> >>
> >>
> >> @@ -277,20 +366,37 @@ void
> >>  virInterfaceObjListRemove(virInterfaceObjListPtr interfaces,
> >>virInterfaceObjPtr obj)
> >>  {
> >> -size_t i;
> >
> > if (!obj)
> > return;
> >
>
> OK - I think at some point in time I figured it wasn't possible, but
> adding it doesn't hurt. It's there in my branch.
>
> > I didn't bother to check whether it could happen (it's used in test driver 
> > only
> > anyway), but just in case there's an early cleanup exit path like it was in 
> > my
> > recent nodedev code which was missing this very check too which would have 
> > made
> > it fail miserably in such scenario.
> >
> > With the patch split in 2 introducing 2 distinct changes + the NULL check:
> > Reviewed-by: Erik Skultety 
>
> Hopefully you reconsider the desire for 2 patches...

Well, since I was apparently fine with t

Re: [libvirt] [PATCH 0/5] qemu: Refactor placement of per-storage-source data (blockdev-add saga)

[John Ferlan]

On 10/19/2017 11:34 AM, Peter Krempa wrote:
> These patches are part of John's original series located here:
> 
> https://www.redhat.com/archives/libvir-list/2017-October/msg00228.html
> 
> The patches are reordered and fixed to make more sense.
> 
> Patch 1/5 is originally 5/16, without any modification. Patches 2/5 and
> 3/5 are split from patch 2/16 from original series.
> 
> Patches 3/16 and 4/16 were dropped. Allocating the private data via
> xmlopt does not make sense since the storage-driver originating
> virStorageSources would not have them allocated anyways. We will
> allocate them when necessary in the qemu driver.
> 
> The copy function was dropped, since the private data should not really
> be copied. It can be added later if necessary.
> 
> The rest of the series will be posted later. I'm interrested in parsing
> of auth/encryption for backing chain members (1/16, 7/16) and the JSON
> generator from 15/16. I'll extract those parts in my upcoming
> blockdev-add saga postings and post the rest later.
> 
> John Ferlan (5):
>   qemu: Add missing encinfo cleanup
>   util: storage: Introduce privateData for _virStorageSource
>   qemu: Introduce privateData object for virStorageSource
>   qemu: Relocate qemuDomainSecretInfoPtr to
> qemuDomainStorageSourcePrivate
>   qemu: Move encinfo from private disk to private disk src
> 
>  src/qemu/qemu_command.c   | 12 +-
>  src/qemu/qemu_domain.c| 60 
> ++-
>  src/qemu/qemu_domain.h| 26 +---
>  src/qemu/qemu_hotplug.c   | 11 +
>  src/util/virstoragefile.c |  1 +
>  src/util/virstoragefile.h |  3 +++
>  6 files changed, 82 insertions(+), 31 deletions(-)
> 

I'm fine with the refactor - whatever works best. Feels strange to ACK
or R-B my own code, but if that's what you need, then consider it provided.

John

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

Re: [libvirt] [PATCH v3] [libvirt-jenkins-ci] Build on supported Fedora releases (25-26)

[Pavel Hrdina]

On Thu, Sep 07, 2017 at 12:48:22PM +0200, Andrea Bolognani wrote:
> Fedora 23 has been out of support for quite a while now, with
> Fedora 24 recently joining it with the release of Fedora 26
> which, on the other hand, is fully supported and a prime candidate
> for building libvirt.
> 
> Signed-off-by: Andrea Bolognani 
> ---
> Yash first attempted this last December[1], without much luck.
> 
> Fedora 26 has been released in the meantime, which means we can
> get rid of two builders instead of one! Someone will have to
> prepare the 'libvirt-fedora-26' builder, though, because it
> doesn't exist at the moment :)
> 
> [1] https://www.redhat.com/archives/libvir-list/2016-December/msg00676.html
> 
>  projects/libosinfo.yaml   | 3 +--
>  projects/libvirt-cim.yaml | 3 +--
>  projects/libvirt-glib.yaml| 3 +--
>  projects/libvirt-go-xml.yaml  | 3 +--
>  projects/libvirt-go.yaml  | 3 +--
>  projects/libvirt-perl.yaml| 3 +--
>  projects/libvirt-python.yaml  | 3 +--
>  projects/libvirt-sandbox.yaml | 3 +--
>  projects/libvirt-tck.yaml | 3 +--
>  projects/libvirt.yaml | 9 +++--
>  projects/osinfo-db-tools.yaml | 3 +--
>  projects/osinfo-db.yaml   | 3 +--
>  projects/virt-manager.yaml| 3 +--
>  projects/virt-viewer.yaml | 3 +--
>  14 files changed, 16 insertions(+), 32 deletions(-)

Now that we have all the things automated we can finally push this
patch.

Reviewed-by: Pavel Hrdina 


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

[libvirt] [libvirt-jenkins-ci PATCH 3/6] guests: Implement flavors

[Andrea Bolognani]

Our CI infrastructure and developers have different requirements,
but really the overlap is almost complete and it's a shame that
we require developers to perform manual steps before we can use
our tools.

Flavors are a very simple and effective way to deal with the
issue: we'll be able to configure guests differently based on
whether they will be used for CI or development.

The default flavor is developer, which doesn't require the vault
password and as such can be used by anyone out of the box: the
Jenkins setup is skipped in this case.

Signed-off-by: Andrea Bolognani 
---
 guests/lcitool  | 35 ---
 guests/site.yml |  1 +
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 883e0eb..bf270f1 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -59,13 +59,39 @@ load_install_config() {
 load_config() {
 CONFIG_DIR="$HOME/.config/$PROGRAM_NAME"
 
+mkdir -p "$CONFIG_DIR" >/dev/null 2>&1 || {
+die "$PROGRAM_NAME: $CONFIG_DIR: Unable to create config directory"
+}
+
+FLAVOR_FILE="$CONFIG_DIR/flavor"
 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"
+# Two flavors are supported: developer (default) and ci. Read the
+# flavor from configuration, validate it and write it back in case
+# it was not present
+FLAVOR="$(cat "$FLAVOR_FILE" 2>/dev/null)"
+FLAVOR=${FLAVOR:-developer}
+test "$FLAVOR" = developer || test "$FLAVOR" = ci || {
+die "$PROGRAM_NAME: Invalid flavor '$FLAVOR'"
 }
+echo "$FLAVOR" >"$FLAVOR_FILE" || {
+die "$PROGRAM_NAME: $FLAVOR_FILE: Unable to save flavor"
+}
+
+test "$FLAVOR" = ci && {
+# The vault password is only needed for the ci flavor, so only
+# validate it in that case
+test -f "$VAULT_PASS_FILE" && test "$(cat "$VAULT_PASS_FILE")" || {
+die "$PROGRAM_NAME: $VAULT_PASS_FILE: Missing or invalid password"
+}
+} || {
+# For other flavors, undefine the variable so that Ansible
+# will not try to read the file at all
+VAULT_PASS_FILE=
+}
+
+# Make sure the root password has been configured properly
 test -f "$ROOT_PASS_FILE" && test "$(cat "$ROOT_PASS_FILE")" || {
 die "$PROGRAM_NAME: $ROOT_PASS_FILE: Missing or invalid password"
 }
@@ -164,8 +190,11 @@ do_prepare() {
 
 load_config
 
+EXTRA_VARS="flavor=$FLAVOR"
+
 ansible-playbook \
 --vault-password-file "$VAULT_PASS_FILE" \
+--extra-vars "$EXTRA_VARS" \
 -l "$GUEST" \
 site.yml
 }
diff --git a/guests/site.yml b/guests/site.yml
index 9c75dcb..35e3220 100644
--- a/guests/site.yml
+++ b/guests/site.yml
@@ -30,6 +30,7 @@
 # Configure the Jenkins agent
 - include: tasks/jenkins.yml
   when:
+- flavor == 'ci'
 - projects is defined
 # jenkins is a pseudo-project
 - ( 'jenkins' in projects )
-- 
2.13.6

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

[libvirt] [libvirt-jenkins-ci PATCH 2/6] guests: Move configuration handling to load_config()

[Andrea Bolognani]

Just a code move. We'll be adding more logic soon, and it'll
be nice not to pollute the do_prepare() function too much
because of it. Rename the existing load_config() function to
load_install_config() accordingly.

Signed-off-by: Andrea Bolognani 
---
 guests/lcitool | 54 +++---
 1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 4578327..883e0eb 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -34,12 +34,12 @@ yaml_var() {
 grep "^$2:\\s*" "$1" 2>/dev/null | tail -1 | sed "s/$2:\\s*//g"
 }
 
-# load_config FILE
+# load_install_config FILE
 #
 # Read all known configuration variables from $FILE and set them in the
 # environment. Configuration variables that have already been set in
 # the environment will not be updated.
-load_config() {
+load_install_config() {
 INSTALL_URL=${INSTALL_URL:-$(yaml_var "$1" install_url)}
 INSTALL_CONFIG=${INSTALL_CONFIG:-$(yaml_var "$1" install_config)}
 INSTALL_VIRT_TYPE=${INSTALL_VIRT_TYPE:-$(yaml_var "$1" install_virt_type)}
@@ -53,6 +53,32 @@ load_config() {
 INSTALL_NETWORK=${INSTALL_NETWORK:-$(yaml_var "$1" install_network)}
 }
 
+# load_config
+#
+# Read tool configuration and perform the necessary validation.
+load_config() {
+CONFIG_DIR="$HOME/.config/$PROGRAM_NAME"
+
+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"
+}
+}
+
 # --
 #  User-visible actions
 # --
@@ -92,8 +118,8 @@ do_install()
 # Load configuration files. Values don't get overwritten after being
 # set the first time, so loading the host-specific configuration before
 # the group configuration ensures overrides work as expected
-load_config "host_vars/$GUEST/install.yml"
-load_config "group_vars/all/install.yml"
+load_install_config "host_vars/$GUEST/install.yml"
+load_install_config "group_vars/all/install.yml"
 
 # Both memory size and disk size use GiB as unit, but virt-install wants
 # disk size in GiB and memory size in *MiB*, so perform conversion here
@@ -136,24 +162,7 @@ do_prepare() {
 die "$PROGRAM_NAME: $GUEST: Unknown guest"
 }
 
-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"
-}
+load_config
 
 ansible-playbook \
 --vault-password-file "$VAULT_PASS_FILE" \
@@ -167,7 +176,6 @@ do_prepare() {
 
 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"
-- 
2.13.6

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

[libvirt] [libvirt-jenkins-ci PATCH 0/6] Implement developer flavor

[Andrea Bolognani]

Make lcitool useful outside of the libvirt CI context, even though
doing so openly contradicts its name. We're just savage like that.

Andrea Bolognani (6):
  guests: Open vault on demand
  guests: Move configuration handling to load_config()
  guests: Implement flavors
  guests: Implement developer flavor
  guests: Hand root password location over to Ansible
  guests: Update documentation

 guests/README.markdown| 45 ++--
 guests/group_vars/all/main.yml|  4 +-
 guests/host_vars/libvirt-centos-6/main.yml|  2 -
 guests/host_vars/libvirt-centos-6/vault.yml   | 10 ---
 guests/host_vars/libvirt-centos-7/main.yml|  2 -
 guests/host_vars/libvirt-centos-7/vault.yml   | 10 ---
 guests/host_vars/libvirt-debian-8/main.yml|  2 -
 guests/host_vars/libvirt-debian-8/vault.yml   | 10 ---
 guests/host_vars/libvirt-debian-9/main.yml|  2 -
 guests/host_vars/libvirt-debian-9/vault.yml   | 10 ---
 guests/host_vars/libvirt-fedora-25/main.yml   |  2 -
 guests/host_vars/libvirt-fedora-25/vault.yml  | 10 ---
 guests/host_vars/libvirt-fedora-26/main.yml   |  2 -
 guests/host_vars/libvirt-fedora-26/vault.yml  | 10 ---
 guests/host_vars/libvirt-fedora-rawhide/main.yml  |  2 -
 guests/host_vars/libvirt-fedora-rawhide/vault.yml | 10 ---
 guests/host_vars/libvirt-freebsd-10/main.yml  |  3 +-
 guests/host_vars/libvirt-freebsd-10/vault.yml | 10 ---
 guests/host_vars/libvirt-freebsd-11/main.yml  |  3 +-
 guests/host_vars/libvirt-freebsd-11/vault.yml | 10 ---
 guests/host_vars/libvirt-ubuntu-12/main.yml   |  2 -
 guests/host_vars/libvirt-ubuntu-12/vault.yml  |  8 --
 guests/host_vars/libvirt-ubuntu-14/main.yml   |  2 -
 guests/host_vars/libvirt-ubuntu-14/vault.yml  |  8 --
 guests/host_vars/libvirt-ubuntu-16/main.yml   |  2 -
 guests/host_vars/libvirt-ubuntu-16/vault.yml  |  8 --
 guests/lcitool| 90 +--
 guests/site.yml   |  6 ++
 guests/tasks/base.yml |  2 +-
 guests/tasks/developer.yml| 21 ++
 guests/tasks/jenkins.yml  |  8 ++
 guests/vars/vault.yml | 54 ++
 32 files changed, 186 insertions(+), 184 deletions(-)
 delete mode 100644 guests/host_vars/libvirt-centos-6/vault.yml
 delete mode 100644 guests/host_vars/libvirt-centos-7/vault.yml
 delete mode 100644 guests/host_vars/libvirt-debian-8/vault.yml
 delete mode 100644 guests/host_vars/libvirt-debian-9/vault.yml
 delete mode 100644 guests/host_vars/libvirt-fedora-25/vault.yml
 delete mode 100644 guests/host_vars/libvirt-fedora-26/vault.yml
 delete mode 100644 guests/host_vars/libvirt-fedora-rawhide/vault.yml
 delete mode 100644 guests/host_vars/libvirt-freebsd-10/vault.yml
 delete mode 100644 guests/host_vars/libvirt-freebsd-11/vault.yml
 delete mode 100644 guests/host_vars/libvirt-ubuntu-12/vault.yml
 delete mode 100644 guests/host_vars/libvirt-ubuntu-14/vault.yml
 delete mode 100644 guests/host_vars/libvirt-ubuntu-16/vault.yml
 create mode 100644 guests/tasks/developer.yml
 create mode 100644 guests/vars/vault.yml

-- 
2.13.6

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

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

[Andrea Bolognani]

Signed-off-by: Andrea Bolognani 
---
 guests/README.markdown | 45 -
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/guests/README.markdown b/guests/README.markdown
index 100ca31..51d9012 100644
--- a/guests/README.markdown
+++ b/guests/README.markdown
@@ -11,8 +11,7 @@ There are two steps to bringing up a guest:
   section below;
 
 * `./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.
+  configuration steps required to make the newly-created guest usable;
 
 Once those steps have been performed, maintainance will involve running:
 
@@ -46,14 +45,6 @@ along the lines of
 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.
-
-
 Development use
 ---
 
@@ -61,22 +52,26 @@ 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 tools are intended mainly for CI use, you'll have to tweak them
-a bit first, including:
+The `developer` flavor is used by default, so you don't need to do
+anything special in order to use it: just follow the steps outlined
+above. Once a guest has been prepared, you'll be able to log in as
+`developer` either via SSH (your public key will have been authorized)
+or on the serial console (password: `developer`).
 
-* trimming down the `inventory` file to just the guest you're interested in;
+Once logged in, you'll be able to perform administrative tasks using
+`sudo`. Regular root access will still be available, either through
+SSH or on the serial console.
 
-* removing any references to the `jenkins` pseudo-project from
-  `host_vars/$guest/main.yml`, along with any references to projects you're
-  not interested to (this will cut down on the number of packages installed)
-  and any references to `jenkins_secret`;
 
-* deleting `host_vars/$guest/vault.yml` altogether.
+CI use
+--
 
-After performing these tweaks, you should be able to use the same steps
-outlined above.
+You'll need to configure `lcitool` to use the `ci` flavor for guests:
+to do so, just write `ci` in the `~/.config/lcitool/flavor` file.
 
-A better way to deal with this use case will be provided in the future.
+Once a guest has been prepared, you'll be able to log in as root either
+via SSH (your public key will have been authorized) or on the serial
+console (using the password configured earlier).
 
 
 FreeBSD
@@ -95,3 +90,11 @@ Some manual tweaking will be needed, in particular:
 
 Once these steps have been performed, FreeBSD guests can be managed just
 like all other guests.
+
+
+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.
-- 
2.13.6

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

[libvirt] [libvirt-jenkins-ci PATCH 4/6] guests: Implement developer flavor

[Andrea Bolognani]

The developer is given key-based SSH access to the guest and
granted passwordless sudo privilege for maximum convenience.

Signed-off-by: Andrea Bolognani 
---
 guests/group_vars/all/main.yml   |  4 +++-
 guests/host_vars/libvirt-freebsd-10/main.yml |  1 +
 guests/host_vars/libvirt-freebsd-11/main.yml |  1 +
 guests/lcitool   |  9 -
 guests/site.yml  |  5 +
 guests/tasks/developer.yml   | 21 +
 6 files changed, 39 insertions(+), 2 deletions(-)
 create mode 100644 guests/tasks/developer.yml

diff --git a/guests/group_vars/all/main.yml b/guests/group_vars/all/main.yml
index d24af59..410077f 100644
--- a/guests/group_vars/all/main.yml
+++ b/guests/group_vars/all/main.yml
@@ -8,8 +8,10 @@ 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
+# Paths to various commands and files that might be OS-dependent. Can
+# be overridden on a per-host basis
 bash: /bin/bash
 java: /usr/bin/java
 make: /usr/bin/make
 sudo: /usr/bin/sudo
+sudoers: /etc/sudoers
diff --git a/guests/host_vars/libvirt-freebsd-10/main.yml 
b/guests/host_vars/libvirt-freebsd-10/main.yml
index 80d16d6..4f33c53 100644
--- a/guests/host_vars/libvirt-freebsd-10/main.yml
+++ b/guests/host_vars/libvirt-freebsd-10/main.yml
@@ -5,6 +5,7 @@ bash: /usr/local/bin/bash
 java: /usr/local/bin/java
 make: /usr/local/bin/gmake
 sudo: /usr/local/bin/sudo
+sudoers: /usr/local/etc/sudoers
 
 projects:
   - base
diff --git a/guests/host_vars/libvirt-freebsd-11/main.yml 
b/guests/host_vars/libvirt-freebsd-11/main.yml
index 80d16d6..4f33c53 100644
--- a/guests/host_vars/libvirt-freebsd-11/main.yml
+++ b/guests/host_vars/libvirt-freebsd-11/main.yml
@@ -5,6 +5,7 @@ bash: /usr/local/bin/bash
 java: /usr/local/bin/java
 make: /usr/local/bin/gmake
 sudo: /usr/local/bin/sudo
+sudoers: /usr/local/etc/sudoers
 
 projects:
   - base
diff --git a/guests/lcitool b/guests/lcitool
index bf270f1..018640b 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -141,6 +141,8 @@ do_install()
 die "$PROGRAM_NAME: $GUEST: Missing configuration, guest must be 
installed manually"
 }
 
+load_config
+
 # Load configuration files. Values don't get overwritten after being
 # set the first time, so loading the host-specific configuration before
 # the group configuration ensures overrides work as expected
@@ -158,6 +160,11 @@ do_install()
 *kickstart*|*ks*) EXTRA_ARGS="ks=file:/${INSTALL_CONFIG##*/}" ;;
 esac
 
+# Only configure autostart for the guest for the ci flavor
+test "$FLAVOR" = ci && {
+AUTOSTART="--autostart"
+}
+
 virt-install \
 --name "$GUEST" \
 --location "$INSTALL_URL" \
@@ -174,7 +181,7 @@ do_install()
 --sound none \
 --initrd-inject "$INSTALL_CONFIG" \
 --extra-args "console=ttyS0 $EXTRA_ARGS" \
---autostart \
+$AUTOSTART \
 --wait 0
 }
 
diff --git a/guests/site.yml b/guests/site.yml
index 35e3220..76437bb 100644
--- a/guests/site.yml
+++ b/guests/site.yml
@@ -34,3 +34,8 @@
 - projects is defined
 # jenkins is a pseudo-project
 - ( 'jenkins' in projects )
+
+# Configure the developer account
+- include: tasks/developer.yml
+  when:
+- flavor == 'developer'
diff --git a/guests/tasks/developer.yml b/guests/tasks/developer.yml
new file mode 100644
index 000..1dad8fc
--- /dev/null
+++ b/guests/tasks/developer.yml
@@ -0,0 +1,21 @@
+---
+- name: Create developer user account
+  user:
+name: developer
+comment: Developer
+password: 
$6$YEzeb0A3t7jn/IwW$oMPH0mpKPPeuABH3gKDom08rLccOKBm6CrXT/deBsdP77MjBHxwHQ5EJM0MAc/sOsGKCNX0zjYYjlXP.KNUmP0
+shell: '{{ bash }}'
+
+- name: Configure ssh access for the developer
+  authorized_key:
+user: developer
+key: '{{ lookup("file", lookup("env", "HOME") + "/.ssh/id_rsa.pub") }}'
+state: present
+
+- name: Grant passwordless sudo access to the developer
+  lineinfile:
+path: '{{ sudoers }}'
+line: 'developer ALL=(ALL) NOPASSWD: ALL'
+state: present
+backup: yes
+validate: 'visudo -cf %s'
-- 
2.13.6

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

[libvirt] [libvirt-jenkins-ci PATCH 5/6] guests: Hand root password location over to Ansible

[Andrea Bolognani]

Instead of hard-coding the location in the playbook, we hand it
over at runtime when calling ansible-playbook, ensuring better
separation of concerns.

Signed-off-by: Andrea Bolognani 
---
 guests/lcitool| 2 +-
 guests/tasks/base.yml | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 018640b..1efe7e5 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -197,7 +197,7 @@ do_prepare() {
 
 load_config
 
-EXTRA_VARS="flavor=$FLAVOR"
+EXTRA_VARS="flavor=$FLAVOR root_password_file=$ROOT_HASH_FILE"
 
 ansible-playbook \
 --vault-password-file "$VAULT_PASS_FILE" \
diff --git a/guests/tasks/base.yml b/guests/tasks/base.yml
index b220bb0..8949632 100644
--- a/guests/tasks/base.yml
+++ b/guests/tasks/base.yml
@@ -99,7 +99,7 @@
 - name: Configure root password and shell
   user:
 name: root
-password: '{{ lookup("file", lookup("env", "HOME") + 
"/.config/lcitool/.root-password.hash") }}'
+password: '{{ lookup("file", root_password_file) }}'
 shell: '{{ bash }}'
 
 - name: Configure ssh access for the root user
-- 
2.13.6

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

[libvirt] [libvirt-jenkins-ci PATCH 1/6] guests: Open vault on demand

[Andrea Bolognani]

By storing the vault out of the inventory, we can open it on
demand rather than automatically. This will eventually make it
possible to use the playbooks even without knowing the vault
password.

Signed-off-by: Andrea Bolognani 
---
 guests/host_vars/libvirt-centos-6/main.yml|  2 -
 guests/host_vars/libvirt-centos-6/vault.yml   | 10 -
 guests/host_vars/libvirt-centos-7/main.yml|  2 -
 guests/host_vars/libvirt-centos-7/vault.yml   | 10 -
 guests/host_vars/libvirt-debian-8/main.yml|  2 -
 guests/host_vars/libvirt-debian-8/vault.yml   | 10 -
 guests/host_vars/libvirt-debian-9/main.yml|  2 -
 guests/host_vars/libvirt-debian-9/vault.yml   | 10 -
 guests/host_vars/libvirt-fedora-25/main.yml   |  2 -
 guests/host_vars/libvirt-fedora-25/vault.yml  | 10 -
 guests/host_vars/libvirt-fedora-26/main.yml   |  2 -
 guests/host_vars/libvirt-fedora-26/vault.yml  | 10 -
 guests/host_vars/libvirt-fedora-rawhide/main.yml  |  2 -
 guests/host_vars/libvirt-fedora-rawhide/vault.yml | 10 -
 guests/host_vars/libvirt-freebsd-10/main.yml  |  2 -
 guests/host_vars/libvirt-freebsd-10/vault.yml | 10 -
 guests/host_vars/libvirt-freebsd-11/main.yml  |  2 -
 guests/host_vars/libvirt-freebsd-11/vault.yml | 10 -
 guests/host_vars/libvirt-ubuntu-12/main.yml   |  2 -
 guests/host_vars/libvirt-ubuntu-12/vault.yml  |  8 
 guests/host_vars/libvirt-ubuntu-14/main.yml   |  2 -
 guests/host_vars/libvirt-ubuntu-14/vault.yml  |  8 
 guests/host_vars/libvirt-ubuntu-16/main.yml   |  2 -
 guests/host_vars/libvirt-ubuntu-16/vault.yml  |  8 
 guests/tasks/jenkins.yml  |  8 
 guests/vars/vault.yml | 54 +++
 26 files changed, 62 insertions(+), 138 deletions(-)
 delete mode 100644 guests/host_vars/libvirt-centos-6/vault.yml
 delete mode 100644 guests/host_vars/libvirt-centos-7/vault.yml
 delete mode 100644 guests/host_vars/libvirt-debian-8/vault.yml
 delete mode 100644 guests/host_vars/libvirt-debian-9/vault.yml
 delete mode 100644 guests/host_vars/libvirt-fedora-25/vault.yml
 delete mode 100644 guests/host_vars/libvirt-fedora-26/vault.yml
 delete mode 100644 guests/host_vars/libvirt-fedora-rawhide/vault.yml
 delete mode 100644 guests/host_vars/libvirt-freebsd-10/vault.yml
 delete mode 100644 guests/host_vars/libvirt-freebsd-11/vault.yml
 delete mode 100644 guests/host_vars/libvirt-ubuntu-12/vault.yml
 delete mode 100644 guests/host_vars/libvirt-ubuntu-14/vault.yml
 delete mode 100644 guests/host_vars/libvirt-ubuntu-16/vault.yml
 create mode 100644 guests/vars/vault.yml

diff --git a/guests/host_vars/libvirt-centos-6/main.yml 
b/guests/host_vars/libvirt-centos-6/main.yml
index 69ef616..d717ae7 100644
--- a/guests/host_vars/libvirt-centos-6/main.yml
+++ b/guests/host_vars/libvirt-centos-6/main.yml
@@ -6,5 +6,3 @@ projects:
   - libvirt-cim
   - libvirt-perl
   - libvirt-python
-
-jenkins_secret: '{{ vault.jenkins_secret }}'
diff --git a/guests/host_vars/libvirt-centos-6/vault.yml 
b/guests/host_vars/libvirt-centos-6/vault.yml
deleted file mode 100644
index e28b263..000
--- a/guests/host_vars/libvirt-centos-6/vault.yml
+++ /dev/null
@@ -1,10 +0,0 @@
-$ANSIBLE_VAULT;1.1;AES256
-36623466366139303234633662663431663135396238653132346239306336616463393733343064
-6131386366613438643532353536393435623464333863350a34616430626361373536363638
-65333633306236343066303165326137626432656439663738383765323862373161363165353936
-3637356536616637390a313135306462353830626438653465343730616437633634363866313432
-62376634393738373834663939646463626232323235666364653462343435313564333132353864
-38643731383435393465393633356466303661323966306431303435366533623062303363653364
-3135386137613832306535303634666138616438616430316434356233666364333864646265
-3131316361333761316530386231353330376135363364653661616663346631613761373864
-3338
diff --git a/guests/host_vars/libvirt-centos-7/main.yml 
b/guests/host_vars/libvirt-centos-7/main.yml
index 2e66a70..30c826a 100644
--- a/guests/host_vars/libvirt-centos-7/main.yml
+++ b/guests/host_vars/libvirt-centos-7/main.yml
@@ -15,5 +15,3 @@ projects:
   - osinfo-db-tools
   - virt-manager
   - virt-viewer
-
-jenkins_secret: '{{ vault.jenkins_secret }}'
diff --git a/guests/host_vars/libvirt-centos-7/vault.yml 
b/guests/host_vars/libvirt-centos-7/vault.yml
deleted file mode 100644
index 81b32c4..000
--- a/guests/host_vars/libvirt-centos-7/vault.yml
+++ /dev/null
@@ -1,10 +0,0 @@
-$ANSIBLE_VAULT;1.1;AES256
-35393538623463386331376531613663336438656535663037326364666434613463396233633638
-6365326431306637326366366533306630373039356664660a633430633463666462626662313330
-3665336434383833343963356135393664643537323634336162393533363465386533363132
-3838643263643862370a39326563323363383838646463353635366336383834633236386632
-653762313530313364653364636464616235383739376536646230656237653036616132

[libvirt] [PATCH] util: Missing 'removeTimeoutImpl' check variable inside virEventRegisterImpl() function.

[Julio Faracco]

The function virEventRegisterImpl() checks the attempt to replace the
registered events. But there is a duplicate variable inside the IF statement.
The variable 'removeHandleImpl' was wrongly repeated. One of them needs to be
replaced by 'removeTimeoutImpl'.

Signed-off-by: Julio Faracco 
---
 src/util/virevent.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virevent.c b/src/util/virevent.c
index 51d8714..87069e3 100644
--- a/src/util/virevent.c
+++ b/src/util/virevent.c
@@ -241,7 +241,7 @@ void virEventRegisterImpl(virEventAddHandleFunc addHandle,
   addTimeout, updateTimeout, removeTimeout);
 
 if (addHandleImpl || updateHandleImpl || removeHandleImpl ||
-addTimeoutImpl || updateTimeoutImpl || removeHandleImpl) {
+addTimeoutImpl || updateTimeoutImpl || removeTimeoutImpl) {
 VIR_WARN("Ignoring attempt to replace registered event loop");
 return;
 }
-- 
2.7.4

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

[libvirt] [PATCH 4/5] qemu: Relocate qemuDomainSecretInfoPtr to qemuDomainStorageSourcePrivate

[Peter Krempa]

From: John Ferlan 

Since the secret information is really virStorageSource specific
piece of data, let's manage the privateData from there instead of
at the Disk level.

Signed-off-by: John Ferlan 
Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_command.c |  6 --
 src/qemu/qemu_domain.c  | 14 ++
 src/qemu/qemu_domain.h  |  4 
 src/qemu/qemu_hotplug.c |  7 +--
 4 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 40d9eeffd..7c511263b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1362,7 +1362,8 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
 {
 int actualType = virStorageSourceGetActualType(disk->src);
 qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
-qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo;
+qemuDomainStorageSourcePrivatePtr srcpriv = 
QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
+qemuDomainSecretInfoPtr secinfo = srcpriv->secinfo;
 qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo;
 virJSONValuePtr srcprops = NULL;
 char *source = NULL;
@@ -2239,7 +2240,8 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
 bool driveBoot = false;
 virDomainDiskDefPtr disk = def->disks[i];
 qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
-qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo;
+qemuDomainStorageSourcePrivatePtr srcPriv = 
QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
+qemuDomainSecretInfoPtr secinfo = srcPriv->secinfo;
 qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo;

 if (disk->info.bootIndex) {
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f44e1436d..aa8370866 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -927,7 +927,6 @@ qemuDomainDiskPrivateDispose(void *obj)
 {
 qemuDomainDiskPrivatePtr priv = obj;

-qemuDomainSecretInfoFree(&priv->secinfo);
 qemuDomainSecretInfoFree(&priv->encinfo);
 }

@@ -1344,9 +1343,10 @@ void
 qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk)
 {
 qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+qemuDomainStorageSourcePrivatePtr srcPriv = 
QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);

-if (diskPriv && diskPriv->secinfo)
-qemuDomainSecretInfoFree(&diskPriv->secinfo);
+if (srcPriv && srcPriv->secinfo)
+qemuDomainSecretInfoFree(&srcPriv->secinfo);

 if (diskPriv && diskPriv->encinfo)
 qemuDomainSecretInfoFree(&diskPriv->encinfo);
@@ -1395,6 +1395,12 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
 {
 virStorageSourcePtr src = disk->src;
 qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+qemuDomainStorageSourcePrivatePtr srcPriv;
+
+if (!(disk->src->privateData = qemuDomainStorageSourcePrivateNew()))
+return -1;
+
+srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);

 if (qemuDomainSecretDiskCapable(src)) {
 virSecretUsageType usageType = VIR_SECRET_USAGE_TYPE_ISCSI;
@@ -1402,7 +1408,7 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
 if (src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)
 usageType = VIR_SECRET_USAGE_TYPE_CEPH;

-if (!(diskPriv->secinfo =
+if (!(srcPriv->secinfo =
   qemuDomainSecretInfoNew(conn, priv, disk->info.alias,
   usageType, src->auth->username,
   &src->auth->seclookupdef, false)))
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 2e1515fa1..30e0a5d8d 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -349,10 +349,6 @@ struct _qemuDomainDiskPrivate {

 bool migrating; /* the disk is being migrated */

-/* for storage devices using auth/secret
- * NB: *not* to be written to qemu domain object XML */
-qemuDomainSecretInfoPtr secinfo;
-
 /* for storage devices using encryption/secret
  * Can have both  and  for some disks
  * NB:*not* to be written to qemu domain object XML */
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 5efc60aea..d65aa3a72 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -258,6 +258,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
 char *driveAlias = NULL;
 qemuDomainObjPrivatePtr priv = vm->privateData;
 qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+qemuDomainStorageSourcePrivatePtr srcPriv = 
QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
 const char *format = NULL;
 char *sourcestr = NULL;

@@ -299,7 +300,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
 }

 if (!virStorageSourceIsEmpty(newsrc)) {
-if (qemuGetDriveSourceString(newsrc, diskPriv->secinfo, &sourcestr) < 
0)
+if (qemuGetDriveSourceString(newsrc, srcPriv->secinfo, &sourcestr) < 0)
 goto e

[libvirt] [PATCH 2/5] util: storage: Introduce privateData for _virStorageSource

[Peter Krempa]

From: John Ferlan 

Introduce the bare necessities to add privateData to _virStorageSource.

Signed-off-by: John Ferlan 
Signed-off-by: Peter Krempa 
---
 src/util/virstoragefile.c | 1 +
 src/util/virstoragefile.h | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index d2b21a816..d88183591 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2295,6 +2295,7 @@ virStorageSourceClear(virStorageSourcePtr def)

 virStorageNetHostDefFree(def->nhosts, def->hosts);
 virStorageAuthDefFree(def->auth);
+virObjectUnref(def->privateData);

 VIR_FREE(def->nodestorage);
 VIR_FREE(def->nodeformat);
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 1e36a6671..2f56aea1d 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -27,6 +27,7 @@
 # include 

 # include "virbitmap.h"
+# include "virobject.h"
 # include "virseclabel.h"
 # include "virstorageencryption.h"
 # include "virutil.h"
@@ -241,6 +242,8 @@ struct _virStorageSource {
 virStorageAuthDefPtr auth;
 virStorageEncryptionPtr encryption;

+virObjectPtr privateData;
+
 char *driverName;
 int format; /* virStorageFileFormat in domain backing chains, but
  * pool-specific enum for storage volumes */
-- 
2.14.1

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

Re: [libvirt] [PATCH v1 01/14] conf: Fix virDomainDeviceGetInfo const correctness

[Michal Privoznik]

On 10/19/2017 11:55 AM, Martin Kletzander wrote:
> On Thu, Oct 19, 2017 at 10:10:56AM +0200, Michal Privoznik wrote:
>> This function is not changing passed domain definition.
>> Therefore, mark the argument as 'const'.
>>
> 
> Is there any other reason for this then just your new code using const?
> I don't see any pointer that's passed through your later code that would
> be const nowadays, so why would we play the const-correctness game here
> when it doesn't make much sense in C when the pointer is to a struct
> since the const doesn't propagate?
> 
> If s/const virDomainDeviceDef/virDomainDeviceDef/ on your later patch
> makes this one unnecessary, then NACK to this.  If there is a reason,
> however, feel free to keep this in.

Well, later in 08/14 I am indeed calling this from function which takes
const virDomainDeviceDef * and I can change it to virDomainDeviceDefPtr.
And while I think this patch still makes things better (solves const
correctness), I don't want it to be show stopper. Thus I'm dropping it.

Michal

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

[libvirt] [PATCH 3/5] qemu: Introduce privateData object for virStorageSource

[Peter Krempa]

From: John Ferlan 

Add the object definition and helpers to store security-related private
data for virStorageSources.

Signed-off-by: John Ferlan 
Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_domain.c | 43 +++
 src/qemu/qemu_domain.h | 17 +
 2 files changed, 60 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 41009331a..f44e1436d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -932,6 +932,49 @@ qemuDomainDiskPrivateDispose(void *obj)
 }


+static virClassPtr qemuDomainStorageSourcePrivateClass;
+static void qemuDomainStorageSourcePrivateDispose(void *obj);
+
+static int
+qemuDomainStorageSourcePrivateOnceInit(void)
+{
+qemuDomainStorageSourcePrivateClass = virClassNew(virClassForObject(),
+  
"qemuDomainStorageSourcePrivate",
+  
sizeof(qemuDomainStorageSourcePrivate),
+  
qemuDomainStorageSourcePrivateDispose);
+if (!qemuDomainStorageSourcePrivateClass)
+return -1;
+else
+return 0;
+}
+
+VIR_ONCE_GLOBAL_INIT(qemuDomainStorageSourcePrivate)
+
+virObjectPtr
+qemuDomainStorageSourcePrivateNew(void)
+{
+qemuDomainStorageSourcePrivatePtr priv;
+
+if (qemuDomainStorageSourcePrivateInitialize() < 0)
+return NULL;
+
+if (!(priv = virObjectNew(qemuDomainStorageSourcePrivateClass)))
+return NULL;
+
+return (virObjectPtr) priv;
+}
+
+
+static void
+qemuDomainStorageSourcePrivateDispose(void *obj)
+{
+qemuDomainStorageSourcePrivatePtr priv = obj;
+
+qemuDomainSecretInfoFree(&priv->secinfo);
+qemuDomainSecretInfoFree(&priv->encinfo);
+}
+
+
 static virClassPtr qemuDomainHostdevPrivateClass;
 static void qemuDomainHostdevPrivateDispose(void *obj);

diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 7c9364f35..2e1515fa1 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -363,6 +363,23 @@ struct _qemuDomainDiskPrivate {
 bool removable; /* device media can be removed/changed */
 };

+# define QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src) \
+((qemuDomainStorageSourcePrivatePtr) (src)->privateData)
+
+typedef struct _qemuDomainStorageSourcePrivate qemuDomainStorageSourcePrivate;
+typedef qemuDomainStorageSourcePrivate *qemuDomainStorageSourcePrivatePtr;
+struct _qemuDomainStorageSourcePrivate {
+virObject parent;
+
+/* data required for authentication to the storage source */
+qemuDomainSecretInfoPtr secinfo;
+
+/* data required for decryption of encrypted storage source */
+qemuDomainSecretInfoPtr encinfo;
+};
+
+virObjectPtr qemuDomainStorageSourcePrivateNew(void);
+
 # define QEMU_DOMAIN_HOSTDEV_PRIVATE(hostdev)   \
 ((qemuDomainHostdevPrivatePtr) (hostdev)->privateData)

-- 
2.14.1

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

Re: [libvirt] [PATCH v1 07/14] conf: Parse user supplied aliases

[Michal Privoznik]

On 10/19/2017 11:55 AM, Martin Kletzander wrote:
> On Thu, Oct 19, 2017 at 10:11:02AM +0200, Michal Privoznik wrote:
>> If driver that is calling the parse supports user supplied
>> aliases, they can be parsed even for inactive XMLs. However, to
>> avoid any clashes with aliases that libvirt generates, the user
>> ones have to have "ua-" prefix.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>> src/conf/domain_conf.c | 13 +++--
>> src/conf/domain_conf.h |  1 +
>> 2 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index f4de4e288..f1386c116 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -6398,6 +6398,10 @@ virDomainDeviceAddressParseXML(xmlNodePtr address,
>> }
>>
>>
>> +#define USER_ALIAS_PREFIX "ua-"
>> +#define USER_ALIAS_CHARS \
>> +    "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-"
>> +
>> /* Parse the XML definition for a device address
>>  * @param node XML nodeset to parse for device address definition
>>  */
>> @@ -6424,7 +6428,6 @@
>> virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt
>> ATTRIBUTE_UNUSED,
>>     while (cur != NULL) {
>>     if (cur->type == XML_ELEMENT_NODE) {
>>     if (alias == NULL &&
>> -    !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&
>>     virXMLNodeNameEqual(cur, "alias")) {
>>     alias = cur;
>>     } else if (address == NULL &&
>> @@ -6446,8 +6449,14 @@
>> virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt
>> ATTRIBUTE_UNUSED,
>>     cur = cur->next;
>>     }
>>
>> -    if (alias)
>> +    if (alias) {
>>     info->alias = virXMLPropString(alias, "name");
>> +    if (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE &&
>> +    (!STRPREFIX(info->alias, USER_ALIAS_PREFIX) ||
>> + !(xmlopt->config.features &
>> VIR_DOMAIN_DEF_FEATURE_USER_ALIAS) ||
>> + strspn(info->alias, USER_ALIAS_CHARS) <
>> strlen(info->alias)))
>> +    VIR_FREE(info->alias);
> 
> Assigning the pointer just so you can clear it after some hard to read
> condition
> seems just plain wrong.  Only assign it if the condition is false.
> 
>> +    }
>>
>>     if (master) {
>>     info->mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB;
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 1e007346f..5ff85057f 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -2508,6 +2508,7 @@ typedef enum {
>>     VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN = (1 << 2),
>>     VIR_DOMAIN_DEF_FEATURE_NAME_SLASH = (1 << 3),
>>     VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS = (1 << 4),
>> +    VIR_DOMAIN_DEF_FEATURE_USER_ALIAS = (1 << 5),
> 
> Can you elaborate on why is this not the default and whydo we need to
> have a
> flag for it?  If it's just so that individual drivers can decide this,
> adding
> this to the commit message is enough and ACK with this and the above fixed.

I've described the reasons in cover letter. In short, rather than
enabling this for all drivers, we can take the safe approach and enable
it just for the drivers we know are safe. For now. Also, not every
driver we have has notion of aliases. But if it turns out that we have
enabled it for all the drivers, this flag can be dropped. Until then,
I'd like to be rather safe than sorry.

Due to Pavel's findings (Thanks! Nice catch), I will send v2 (enhanced
with news.xml entry and tests). Meanwhile, I'm pushing 02-06

Michal

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

[libvirt] [PATCH 5/5] qemu: Move encinfo from private disk to private disk src

[Peter Krempa]

From: John Ferlan 

Since the encryption information can also be disk source specific
move it from qemuDomainDiskPrivate to qemuDomainStorageSourcePrivate

Since the last allocated element from qemuDomainDiskPrivate is
removed, that means we no longer need qemuDomainDiskPrivateDispose.

Signed-off-by: John Ferlan 
Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_command.c |  6 ++
 src/qemu/qemu_domain.c  | 20 
 src/qemu/qemu_domain.h  |  5 -
 src/qemu/qemu_hotplug.c |  4 +---
 4 files changed, 7 insertions(+), 28 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 7c511263b..b1cfafa79 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1361,10 +1361,9 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
 virQEMUCapsPtr qemuCaps)
 {
 int actualType = virStorageSourceGetActualType(disk->src);
-qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
 qemuDomainStorageSourcePrivatePtr srcpriv = 
QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
 qemuDomainSecretInfoPtr secinfo = srcpriv->secinfo;
-qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo;
+qemuDomainSecretInfoPtr encinfo = srcpriv->encinfo;
 virJSONValuePtr srcprops = NULL;
 char *source = NULL;
 int ret = -1;
@@ -2239,10 +2238,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
 unsigned int bootindex = 0;
 bool driveBoot = false;
 virDomainDiskDefPtr disk = def->disks[i];
-qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
 qemuDomainStorageSourcePrivatePtr srcPriv = 
QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
 qemuDomainSecretInfoPtr secinfo = srcPriv->secinfo;
-qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo;
+qemuDomainSecretInfoPtr encinfo = srcPriv->encinfo;

 if (disk->info.bootIndex) {
 bootindex = disk->info.bootIndex;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index aa8370866..2bd3dcb49 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -890,7 +890,6 @@ qemuDomainSecretInfoFree(qemuDomainSecretInfoPtr *secinfo)


 static virClassPtr qemuDomainDiskPrivateClass;
-static void qemuDomainDiskPrivateDispose(void *obj);

 static int
 qemuDomainDiskPrivateOnceInit(void)
@@ -898,7 +897,7 @@ qemuDomainDiskPrivateOnceInit(void)
 qemuDomainDiskPrivateClass = virClassNew(virClassForObject(),
  "qemuDomainDiskPrivate",
  sizeof(qemuDomainDiskPrivate),
- qemuDomainDiskPrivateDispose);
+ NULL);
 if (!qemuDomainDiskPrivateClass)
 return -1;
 else
@@ -922,15 +921,6 @@ qemuDomainDiskPrivateNew(void)
 }


-static void
-qemuDomainDiskPrivateDispose(void *obj)
-{
-qemuDomainDiskPrivatePtr priv = obj;
-
-qemuDomainSecretInfoFree(&priv->encinfo);
-}
-
-
 static virClassPtr qemuDomainStorageSourcePrivateClass;
 static void qemuDomainStorageSourcePrivateDispose(void *obj);

@@ -1342,14 +1332,13 @@ qemuDomainSecretInfoTLSNew(virConnectPtr conn,
 void
 qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk)
 {
-qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
 qemuDomainStorageSourcePrivatePtr srcPriv = 
QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);

 if (srcPriv && srcPriv->secinfo)
 qemuDomainSecretInfoFree(&srcPriv->secinfo);

-if (diskPriv && diskPriv->encinfo)
-qemuDomainSecretInfoFree(&diskPriv->encinfo);
+if (srcPriv && srcPriv->encinfo)
+qemuDomainSecretInfoFree(&srcPriv->encinfo);
 }


@@ -1394,7 +1383,6 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
 virDomainDiskDefPtr disk)
 {
 virStorageSourcePtr src = disk->src;
-qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
 qemuDomainStorageSourcePrivatePtr srcPriv;

 if (!(disk->src->privateData = qemuDomainStorageSourcePrivateNew()))
@@ -1416,7 +1404,7 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
 }

 if (qemuDomainDiskHasEncryptionSecret(src)) {
-if (!(diskPriv->encinfo =
+if (!(srcPriv->encinfo =
   qemuDomainSecretInfoNew(conn, priv, disk->info.alias,
   VIR_SECRET_USAGE_TYPE_VOLUME, NULL,
   
&src->encryption->secrets[0]->seclookupdef,
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 30e0a5d8d..39cb68b3c 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -349,11 +349,6 @@ struct _qemuDomainDiskPrivate {

 bool migrating; /* the disk is being migrated */

-/* for storage devices using encryption/secret
- * Can have both  and  for some disks
- * NB:*not* to be written to qemu domain object XML */
-qemuDomainSecretInfoPtr encinfo;
-

[libvirt] [PATCH 1/5] qemu: Add missing encinfo cleanup

[Peter Krempa]

From: John Ferlan 

When commit id 'da86c6c22' added support for diskPriv->encinfo in
qemuDomainSecretDiskPrepare a change to qemuDomainSecretDiskDestroy
to was missed. Although qemuDomainDiskPrivateDispose probably would
do the trick.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_domain.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 23b9c6c13..41009331a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1302,10 +1302,11 @@ qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk)
 {
 qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);

-if (!diskPriv || !diskPriv->secinfo)
-return;
+if (diskPriv && diskPriv->secinfo)
+qemuDomainSecretInfoFree(&diskPriv->secinfo);

-qemuDomainSecretInfoFree(&diskPriv->secinfo);
+if (diskPriv && diskPriv->encinfo)
+qemuDomainSecretInfoFree(&diskPriv->encinfo);
 }


-- 
2.14.1

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

[libvirt] [PATCH 0/5] qemu: Refactor placement of per-storage-source data (blockdev-add saga)

[Peter Krempa]

These patches are part of John's original series located here:

https://www.redhat.com/archives/libvir-list/2017-October/msg00228.html

The patches are reordered and fixed to make more sense.

Patch 1/5 is originally 5/16, without any modification. Patches 2/5 and
3/5 are split from patch 2/16 from original series.

Patches 3/16 and 4/16 were dropped. Allocating the private data via
xmlopt does not make sense since the storage-driver originating
virStorageSources would not have them allocated anyways. We will
allocate them when necessary in the qemu driver.

The copy function was dropped, since the private data should not really
be copied. It can be added later if necessary.

The rest of the series will be posted later. I'm interrested in parsing
of auth/encryption for backing chain members (1/16, 7/16) and the JSON
generator from 15/16. I'll extract those parts in my upcoming
blockdev-add saga postings and post the rest later.

John Ferlan (5):
  qemu: Add missing encinfo cleanup
  util: storage: Introduce privateData for _virStorageSource
  qemu: Introduce privateData object for virStorageSource
  qemu: Relocate qemuDomainSecretInfoPtr to
qemuDomainStorageSourcePrivate
  qemu: Move encinfo from private disk to private disk src

 src/qemu/qemu_command.c   | 12 +-
 src/qemu/qemu_domain.c| 60 ++-
 src/qemu/qemu_domain.h| 26 +---
 src/qemu/qemu_hotplug.c   | 11 +
 src/util/virstoragefile.c |  1 +
 src/util/virstoragefile.h |  3 +++
 6 files changed, 82 insertions(+), 31 deletions(-)

-- 
2.14.1

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

Re: [libvirt] [libvirt-jenkins-ci PATCH 0/4] guests: Fixes and improvements

[Pavel Hrdina]

On Thu, Oct 19, 2017 at 04:59:37PM +0200, Andrea Bolognani wrote:
> First of probably several batches :)
> 
> Andrea Bolognani (4):
>   guests: preseed: Fix root partition size
>   guests: lcitool: Don't disable USB controller
>   guests: libvirt: Install dwarves
>   guests: jenkins: Always download freshest agent

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [PATCH REPOST 0/8] Privatize _virStoragePoolObj and _virStorageVolDefList (Batch #3)

[John Ferlan]

ping?

Tks -

John

On 10/06/2017 10:42 AM, John Ferlan wrote:
> Since the original series (19 patches):
> 
> https://www.redhat.com/archives/libvir-list/2017-September/msg00594.html
> 
> didn't garner any attention, I'm going with smaller patch piles to make
> forward progress.
> 
> This is the last half of the storage backends plus one missed merge
> (because I broke things up )
> 
> John Ferlan (8):
>   storage: Use virStoragePoolObjGetDef accessor for iSCSI backend
>   storage: Use virStoragePoolObjGetDef accessor for MPATH backend
>   storage: Use virStoragePoolObjGetDef accessor for RBD backend
>   storage: Use virStoragePoolObjGetDef accessor for SCSI backend
>   storage: Use virStoragePoolObjGetDef accessor for VSTORAGE backend
>   storage: Use virStoragePoolObjGetDef accessor for ZFS backend
>   storage: Use virStoragePoolObjGetDef accessor for new driver events
>   storage: Privatize virStoragePoolObj and virStorageVolDefList
> 
>  src/conf/storage_conf.h|  4 ---
>  src/conf/virstorageobj.c   | 20 +++
>  src/conf/virstorageobj.h   | 15 
>  src/storage/storage_backend_iscsi.c| 41 --
>  src/storage/storage_backend_mpath.c|  8 +++--
>  src/storage/storage_backend_rbd.c  | 64 
> ++
>  src/storage/storage_backend_scsi.c | 30 +---
>  src/storage/storage_backend_vstorage.c | 31 
>  src/storage/storage_backend_zfs.c  | 39 -
>  src/storage/storage_driver.c   |  8 ++---
>  10 files changed, 144 insertions(+), 116 deletions(-)
> 

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

[libvirt] [libvirt-jenkins-ci PATCH 4/4] guests: jenkins: Always download freshest agent

[Andrea Bolognani]

Make sure that a newer Jenkins agent is downloaded if it's
available on the server.

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

diff --git a/guests/tasks/jenkins.yml b/guests/tasks/jenkins.yml
index 9c8eda1..a1b8f46 100644
--- a/guests/tasks/jenkins.yml
+++ b/guests/tasks/jenkins.yml
@@ -11,6 +11,7 @@
 dest: /home/jenkins/slave.jar
 owner: jenkins
 group: jenkins
+force: yes
 
 - name: Configure and enable Jenkins agent
   lineinfile:
-- 
2.13.6

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

[libvirt] [libvirt-jenkins-ci PATCH 3/4] guests: libvirt: Install dwarves

[Andrea Bolognani]

It can optionally be used during build.

Signed-off-by: Andrea Bolognani 
---
 guests/vars/mappings.yml | 5 +
 guests/vars/projects/libvirt.yml | 1 +
 2 files changed, 6 insertions(+)

diff --git a/guests/vars/mappings.yml b/guests/vars/mappings.yml
index 6b06c15..cae9d23 100644
--- a/guests/vars/mappings.yml
+++ b/guests/vars/mappings.yml
@@ -82,6 +82,11 @@ mappings:
 deb: systemtap-sdt-dev
 rpm: systemtap-sdt-devel
 
+  dwarves:
+default: dwarves
+CentOS:
+FreeBSD:
+
   ebtables:
 default: ebtables
 FreeBSD:
diff --git a/guests/vars/projects/libvirt.yml b/guests/vars/projects/libvirt.yml
index 1c50499..407881d 100644
--- a/guests/vars/projects/libvirt.yml
+++ b/guests/vars/projects/libvirt.yml
@@ -7,6 +7,7 @@ packages:
   - device-mapper
   - dnsmasq
   - dtrace
+  - dwarves
   - ebtables
   - fuse
   - glusterfs
-- 
2.13.6

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

[libvirt] [libvirt-jenkins-ci PATCH 1/4] guests: preseed: Fix root partition size

[Andrea Bolognani]

The existing configuration didn't succeed in causing the root
partition to use up all disk space not assigned to swap.

Signed-off-by: Andrea Bolognani 
---
 guests/preseed.cfg | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/guests/preseed.cfg b/guests/preseed.cfg
index 20b5513..03c47a9 100644
--- a/guests/preseed.cfg
+++ b/guests/preseed.cfg
@@ -39,7 +39,7 @@ d-i partman-auto/disk string /dev/vda
 d-i partman-auto/method string regular
 d-i partman-auto/expert_recipe string \
 custom :: \
-2048 2048 -1 ext4 \
+2048 20480 -1 ext4 \
 $primary{ } $bootable{ } \
 method{ format } format{ } \
 use_filesystem{ } filesystem{ ext4 } \
-- 
2.13.6

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

[libvirt] [libvirt-jenkins-ci PATCH 0/4] guests: Fixes and improvements

[Andrea Bolognani]

First of probably several batches :)

Andrea Bolognani (4):
  guests: preseed: Fix root partition size
  guests: lcitool: Don't disable USB controller
  guests: libvirt: Install dwarves
  guests: jenkins: Always download freshest agent

 guests/lcitool   | 1 -
 guests/preseed.cfg   | 2 +-
 guests/tasks/jenkins.yml | 1 +
 guests/vars/mappings.yml | 5 +
 guests/vars/projects/libvirt.yml | 1 +
 5 files changed, 8 insertions(+), 2 deletions(-)

-- 
2.13.6

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

[libvirt] [libvirt-jenkins-ci PATCH 2/4] guests: lcitool: Don't disable USB controller

[Andrea Bolognani]

Some version of virt-install enable USB input devices despite
the fact neither a USB controller or a graphical display are
available, resulting in installation failures.

Let's just leave the USB controller enabled, it doesn't cause
any issue anyway.

Signed-off-by: Andrea Bolognani 
---
 guests/lcitool | 1 -
 1 file changed, 1 deletion(-)

diff --git a/guests/lcitool b/guests/lcitool
index a1114be..4578327 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -120,7 +120,6 @@ do_install()
 --graphics none \
 --console pty \
 --sound none \
---controller usb,model=none \
 --initrd-inject "$INSTALL_CONFIG" \
 --extra-args "console=ttyS0 $EXTRA_ARGS" \
 --autostart \
-- 
2.13.6

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

Re: [libvirt] [PATCH v3] interfaces: Convert virInterfaceObjList to virObjectRWLockable

[John Ferlan]

On 10/19/2017 10:07 AM, Erik Skultety wrote:
> On Fri, Oct 13, 2017 at 07:45:01AM -0400, John Ferlan wrote:
>> Rather than a forward linked list, let's use the ObjectRWLockable object
>> in order to manage the data.
>>
>> Requires numerous changes from List to Object management similar to
>> many other drivers/vir*obj.c modules
>>
> 
> This patch should be split in two. One that converts it to RWLockable, the
> other using a hash table instead of a linked list.
> 
> [...]

Personally, I don't recall during various conversions I've done to having:

virObjectLockable parent;

size_t count;
virInterfaceObjPtr *objs;

first, then converting to count/objs to (a) hash table(s). Although
looking through history I see Michal had the networkobj code take a long
winding path to get there, so it's not impossible - just IMO pointless.

I think whenever I've converted from count/obs to a hash table with a
'real object', that the locking was done a the same time.

As I recall, usually it's been converting from :

size_t count;
virInterfaceObjPtr *objs;

to:

virObjectLockable parent;

/* name string -> virInterfaceObj  mapping
 * for O(1), lockless lookup-by-name */
virHashTable *objsName;

then more recently the conversion from virObjectLockable to
virObjectRWLockable

Since this patch is taking @objs and converting to a hash table - it's
avoiding the Lockable and going straight to RWLockable.

>>
>> @@ -253,9 +334,11 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr 
>> interfaces,
>>  {
>>  virInterfaceObjPtr obj;
>>
>> -if ((obj = virInterfaceObjListFindByName(interfaces, def->name))) {
>> +virObjectRWLockWrite(interfaces);
>> +if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) 
>> {
>>  virInterfaceDefFree(obj->def);
>>  obj->def = def;
>> +virObjectRWUnlock(interfaces);
>>
>>  return obj;
>>  }
>> @@ -263,13 +346,19 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr 
>> interfaces,
>>  if (!(obj = virInterfaceObjNew()))
>>  return NULL;
>>
>> -if (VIR_APPEND_ELEMENT_COPY(interfaces->objs,
>> -interfaces->count, obj) < 0) {
>> -virInterfaceObjEndAPI(&obj);
>> -return NULL;
>> -}
>> +if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0)
>> +goto error;
>> +virObjectRef(obj);
>> +
>>  obj->def = def;
>> -return virObjectRef(obj);
>> +virObjectRWUnlock(interfaces);
>> +
>> +return obj;
>> +
>> + error:
>> +virInterfaceObjEndAPI(&obj);
>> +virObjectRWUnlock(interfaces);
>> +return NULL;
>>  }
> 
> ^This could be tweaked even more:
> 

Sure, but in doing so eagle eyes now spots a problem in his own code...


> diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c
> index cf3626def..941893fc5 100644
> --- a/src/conf/virinterfaceobj.c
> +++ b/src/conf/virinterfaceobj.c
> @@ -337,19 +337,15 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr 
> interfaces,
>  virObjectRWLockWrite(interfaces);
>  if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) {
>  virInterfaceDefFree(obj->def);
> -obj->def = def;
> -virObjectRWUnlock(interfaces);
> +} else {
> +if (!(obj = virInterfaceObjNew()))
> +return NULL;

Leaving with RWLock, 

> 
> -return obj;
> +if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0)
> +goto error;
> +virObjectRef(obj);
>  }
> 
> -if (!(obj = virInterfaceObjNew()))
> -return NULL;
> -
> -if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0)
> -goto error;
> -virObjectRef(obj);
> -
>  obj->def = def;
>  virObjectRWUnlock(interfaces);
> 
>>
>>
>> @@ -277,20 +366,37 @@ void
>>  virInterfaceObjListRemove(virInterfaceObjListPtr interfaces,
>>virInterfaceObjPtr obj)
>>  {
>> -size_t i;
> 
> if (!obj)
> return;
> 

OK - I think at some point in time I figured it wasn't possible, but
adding it doesn't hurt. It's there in my branch.

> I didn't bother to check whether it could happen (it's used in test driver 
> only
> anyway), but just in case there's an early cleanup exit path like it was in my
> recent nodedev code which was missing this very check too which would have 
> made
> it fail miserably in such scenario.
> 
> With the patch split in 2 introducing 2 distinct changes + the NULL check:
> Reviewed-by: Erik Skultety 

Hopefully you reconsider the desire for 2 patches...

> 
> 
> PS: I'm also sad, that we have two backends here just like we have in nodedev
> with one of them being essentially useless (just like in nodedev) we have this
> 'conf' generic code (just like in nodedev), yet in this case it's only used in
> the test driver. I'd very much appreciate if those backends could be adjusted
> in a way where we could make use of these functions. I can als

Re: [libvirt] [PATCH] qemu-ns: Detect /dev/* mount point duplicates better

[Erik Skultety]

On Thu, Oct 19, 2017 at 03:34:34PM +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1495511
>
> When creating new /dev for domain ran in namespace we try to
> preserve all sub-mounts of /dev. Well, not quite all. For
> instance if /dev/foo/bar and /dev/foo are both mount points, only
> /dev/foo needs preserving. /dev/foo/bar is preserved with it too.
> Now, to identify such cases like this one STRPREFIX() is used.
> That is not good enough. While it works for [/dev/foo/bar;
> /dev/foo] case, it fails for [/dev/prefix; /dev/prefix2] where
> the strings share the same prefix but are in fact two different
> paths. The solution is to use STRSKIP().
>
> Signed-off-by: Michal Privoznik 

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v3] interfaces: Convert virInterfaceObjList to virObjectRWLockable

[Erik Skultety]

On Fri, Oct 13, 2017 at 07:45:01AM -0400, John Ferlan wrote:
> Rather than a forward linked list, let's use the ObjectRWLockable object
> in order to manage the data.
>
> Requires numerous changes from List to Object management similar to
> many other drivers/vir*obj.c modules
>

This patch should be split in two. One that converts it to RWLockable, the
other using a hash table instead of a linked list.

[...]
>
> @@ -253,9 +334,11 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr 
> interfaces,
>  {
>  virInterfaceObjPtr obj;
>
> -if ((obj = virInterfaceObjListFindByName(interfaces, def->name))) {
> +virObjectRWLockWrite(interfaces);
> +if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) {
>  virInterfaceDefFree(obj->def);
>  obj->def = def;
> +virObjectRWUnlock(interfaces);
>
>  return obj;
>  }
> @@ -263,13 +346,19 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr 
> interfaces,
>  if (!(obj = virInterfaceObjNew()))
>  return NULL;
>
> -if (VIR_APPEND_ELEMENT_COPY(interfaces->objs,
> -interfaces->count, obj) < 0) {
> -virInterfaceObjEndAPI(&obj);
> -return NULL;
> -}
> +if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0)
> +goto error;
> +virObjectRef(obj);
> +
>  obj->def = def;
> -return virObjectRef(obj);
> +virObjectRWUnlock(interfaces);
> +
> +return obj;
> +
> + error:
> +virInterfaceObjEndAPI(&obj);
> +virObjectRWUnlock(interfaces);
> +return NULL;
>  }

^This could be tweaked even more:

diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c
index cf3626def..941893fc5 100644
--- a/src/conf/virinterfaceobj.c
+++ b/src/conf/virinterfaceobj.c
@@ -337,19 +337,15 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr 
interfaces,
 virObjectRWLockWrite(interfaces);
 if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) {
 virInterfaceDefFree(obj->def);
-obj->def = def;
-virObjectRWUnlock(interfaces);
+} else {
+if (!(obj = virInterfaceObjNew()))
+return NULL;

-return obj;
+if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0)
+goto error;
+virObjectRef(obj);
 }

-if (!(obj = virInterfaceObjNew()))
-return NULL;
-
-if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0)
-goto error;
-virObjectRef(obj);
-
 obj->def = def;
 virObjectRWUnlock(interfaces);

>
>
> @@ -277,20 +366,37 @@ void
>  virInterfaceObjListRemove(virInterfaceObjListPtr interfaces,
>virInterfaceObjPtr obj)
>  {
> -size_t i;

if (!obj)
return;

I didn't bother to check whether it could happen (it's used in test driver only
anyway), but just in case there's an early cleanup exit path like it was in my
recent nodedev code which was missing this very check too which would have made
it fail miserably in such scenario.

With the patch split in 2 introducing 2 distinct changes + the NULL check:
Reviewed-by: Erik Skultety 


PS: I'm also sad, that we have two backends here just like we have in nodedev
with one of them being essentially useless (just like in nodedev) we have this
'conf' generic code (just like in nodedev), yet in this case it's only used in
the test driver. I'd very much appreciate if those backends could be adjusted
in a way where we could make use of these functions. I can also imagine a
cooperation of the udev backend with the nodedev driver where we have an active
connection to the monitor, thus reacting to all events realtime, instead of
defining a bunch of filters and then letting udev re-enumerate the list of
interfaces each and every time (and by saying that I'm also aware that udev is
actually the useless backend here).

Erik

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

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

[Pavel Hrdina]

On Wed, Oct 18, 2017 at 07:11:45PM +0200, Andrea Bolognani wrote:
> 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

There are some nits pointed out but otherwise this looks really good,
thanks for all the work :).

Reviewed-by: Pavel Hrdina 


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

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

[Pavel Hrdina]

On Thu, Oct 19, 2017 at 03:53:48PM +0200, Andrea Bolognani wrote:
> On Thu, 2017-10-19 at 14:53 +0200, Pavel Hrdina wrote:
> > > So if your only argument against it is that you don't like it very
> > > much, my reply is that I do like it quite a bit and, well, I get to
> > > name the programs I write :)
> > 
> > Well, yes and no :) you can name the program but you also need to have
> > an ACK from community to accept that name.  "licito" is just a cool name
> > that doesn't tell you anything from the first glance what it is.  On the
> > other hand lcitool tells you that it's some kind of tool and that the
> > "lci" part specifies what kind of tool it is.  It's not only that I
> > don't personally like it but it also looks like some randomly chosen
> > name even though there is some pattern behind it.
> > 
> > I vote for lcitool instead of licito.
> 
> I don't feel like any of your arguments have much weight, since
> for most applications the name only has a very vague correlation
> with the functionality or intended purpose, if that: see mutt,
> dnf, evince, firefox, ansible and so, so many more examples.

And there could be a lot of examples to support my statement.

> That said, point taken about the need for the community to stand
> behind a name before it can be adopted.
> 
> Most importantly, I feel like we could both spend our time in a
> more productive way than argue about this, so let's just stick
> with the existing name unless someone comes up with a different
> one that manages to make everyone happy.

Since lcitool was also your idea I didn't think that you would not like
to use it and prefer the new one.  Anyway, thanks for sticking with
the current name.

Pavel


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

[libvirt] [PATCH 5/7] qemu: Don't misuse "ret" in qemuMigrationRun

[Jiri Denemark]

The "ret" variable is used for storing the return value of a function
and should not be used as a temporary variable.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_migration.c | 19 +--
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index cbf255704..2d8a634f9 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3788,16 +3788,17 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 VIR_FREE(timestamp);
 }
 
+rc = -1;
 switch (spec->destType) {
 case MIGRATION_DEST_HOST:
 if (STREQ(spec->dest.host.protocol, "rdma") &&
 virProcessSetMaxMemLock(vm->pid, vm->def->mem.hard_limit << 10) < 
0) {
 goto exit_monitor;
 }
-ret = qemuMonitorMigrateToHost(priv->mon, migrate_flags,
-   spec->dest.host.protocol,
-   spec->dest.host.name,
-   spec->dest.host.port);
+rc = qemuMonitorMigrateToHost(priv->mon, migrate_flags,
+  spec->dest.host.protocol,
+  spec->dest.host.name,
+  spec->dest.host.port);
 break;
 
 case MIGRATION_DEST_CONNECT_HOST:
@@ -3809,16 +3810,14 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 fd = spec->dest.fd.local;
 spec->dest.fd.local = -1;
 }
-ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags,
- spec->dest.fd.qemu);
+rc = qemuMonitorMigrateToFd(priv->mon, migrate_flags,
+spec->dest.fd.qemu);
 VIR_FORCE_CLOSE(spec->dest.fd.qemu);
 break;
 }
-if (qemuDomainObjExitMonitor(driver, vm) < 0)
-ret = -1;
-if (ret < 0)
+
+if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
 goto error;
-ret = -1;
 
 /* From this point onwards we *must* call cancel to abort the
  * migration on source if anything goes wrong */
-- 
2.14.2

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

[libvirt] [PATCH 0/7] Preparation for new QEMU migration states

[Jiri Denemark]

Mostly refactoring of the horrible mess in qemuMigrationRun.

Jiri Denemark (7):
  qemu: Use switch in qemuMigrationCompleted
  qemu: Refactor qemuMigrationRun a bit
  qemu: Split cleanup and error code in qemuMigrationRun
  qemu: Unite error handling in qemuMigrationRun
  qemu: Don't misuse "ret" in qemuMigrationRun
  qemu: Consistently use exit_monitor in qemuMigrationRun
  qemu: Set correct job status when qemuMigrationRun fails

 src/qemu/qemu_migration.c | 196 +-
 1 file changed, 105 insertions(+), 91 deletions(-)

-- 
2.14.2

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

[libvirt] [PATCH 7/7] qemu: Set correct job status when qemuMigrationRun fails

[Jiri Denemark]

Instead of enumerating all states which need to be turned into
QEMU_DOMAIN_JOB_STATUS_FAILED (and failing to add all of them), it's
better to mention just the one which needs to be left alone.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_migration.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 8a529f9ad..f785c308c 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3942,9 +3942,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 if (iothread)
 qemuMigrationStopTunnel(iothread, true);
 
-if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
-priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
-priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY)
+if (priv->job.current->status != QEMU_DOMAIN_JOB_STATUS_CANCELED)
 priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
 
 goto cleanup;
-- 
2.14.2

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

[libvirt] [PATCH 2/7] qemu: Refactor qemuMigrationRun a bit

[Jiri Denemark]

Some code which was supposed to be executed only when migration
succeeded was buried inside the cleanup code.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_migration.c | 63 +++
 1 file changed, 36 insertions(+), 27 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index c3f9c38b2..f0d4f9d98 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3867,8 +3867,36 @@ qemuMigrationRun(virQEMUDriverPtr driver,
qemuMigrationSetOffline(driver, vm) < 0) {
 goto cancelPostCopy;
 }
-if (priv->job.completed)
+
+if (mig && mig->nbd &&
+qemuMigrationCancelDriveMirror(driver, vm, true,
+   QEMU_ASYNC_JOB_MIGRATION_OUT,
+   dconn) < 0)
+goto cancelPostCopy;
+
+if (iothread) {
+qemuMigrationIOThreadPtr io;
+
+VIR_STEAL_PTR(io, iothread);
+if (qemuMigrationStopTunnel(io, false) < 0)
+goto cancelPostCopy;
+}
+
+if (priv->job.completed) {
 priv->job.completed->stopped = priv->job.current->stopped;
+qemuDomainJobInfoUpdateTime(priv->job.completed);
+qemuDomainJobInfoUpdateDowntime(priv->job.completed);
+ignore_value(virTimeMillisNow(&priv->job.completed->sent));
+}
+
+cookieFlags |= QEMU_MIGRATION_COOKIE_NETWORK |
+   QEMU_MIGRATION_COOKIE_STATS;
+
+if (qemuMigrationCookieAddPersistent(mig, &persistDef) < 0 ||
+qemuMigrationBakeCookie(mig, driver, vm, cookieout,
+cookieoutlen, cookieFlags) < 0) {
+VIR_WARN("Unable to encode migration cookie");
+}
 
 ret = 0;
 
@@ -3877,43 +3905,24 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 orig_err = virSaveLastError();
 
 /* cancel any outstanding NBD jobs */
-if (mig && mig->nbd) {
-if (qemuMigrationCancelDriveMirror(driver, vm, ret == 0,
-   QEMU_ASYNC_JOB_MIGRATION_OUT,
-   dconn) < 0)
-ret = -1;
-}
+if (ret < 0 && mig && mig->nbd)
+qemuMigrationCancelDriveMirror(driver, vm, false,
+   QEMU_ASYNC_JOB_MIGRATION_OUT,
+   dconn);
 
 VIR_FREE(tlsAlias);
 VIR_FREE(secAlias);
 virObjectUnref(cfg);
 
-if (spec->fwdType != MIGRATION_FWD_DIRECT) {
-if (iothread && qemuMigrationStopTunnel(iothread, ret < 0) < 0)
-ret = -1;
-}
-VIR_FORCE_CLOSE(fd);
+if (ret < 0 && iothread)
+qemuMigrationStopTunnel(iothread, true);
 
-if (priv->job.completed) {
-qemuDomainJobInfoUpdateTime(priv->job.completed);
-qemuDomainJobInfoUpdateDowntime(priv->job.completed);
-ignore_value(virTimeMillisNow(&priv->job.completed->sent));
-}
+VIR_FORCE_CLOSE(fd);
 
 if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
 priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING)
 priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
 
-cookieFlags |= QEMU_MIGRATION_COOKIE_NETWORK |
-   QEMU_MIGRATION_COOKIE_STATS;
-
-if (ret == 0 &&
-(qemuMigrationCookieAddPersistent(mig, &persistDef) < 0 ||
- qemuMigrationBakeCookie(mig, driver, vm, cookieout,
- cookieoutlen, cookieFlags) < 0)) {
-VIR_WARN("Unable to encode migration cookie");
-}
-
 virDomainDefFree(persistDef);
 qemuMigrationCookieFree(mig);
 
-- 
2.14.2

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

[libvirt] [PATCH 6/7] qemu: Consistently use exit_monitor in qemuMigrationRun

[Jiri Denemark]

Almost every failure in qemuMigrationRun while we are talking to QEMU
monitor results in a jump to exit_monitor label. The only exception is
removed by this patch.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_migration.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 2d8a634f9..8a529f9ad 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3765,12 +3765,11 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 /* explicitly do this *after* we entered the monitor,
  * as this is a critical section so we are guaranteed
  * priv->job.abortJob will not change */
-ignore_value(qemuDomainObjExitMonitor(driver, vm));
 priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_CANCELED;
 virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
_("canceled by client"));
-goto error;
+goto exit_monitor;
 }
 
 if (qemuMonitorSetMigrationSpeed(priv->mon, migrate_speed) < 0)
-- 
2.14.2

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

[libvirt] [PATCH 3/7] qemu: Split cleanup and error code in qemuMigrationRun

[Jiri Denemark]

Let cleanup only do things common to both failure and success paths and
move error handling code inside the new "error" section.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_migration.c | 79 ---
 1 file changed, 40 insertions(+), 39 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index f0d4f9d98..6956901be 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3657,20 +3657,20 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 if (persist_xml) {
 if (!(persistDef = qemuMigrationPrepareDef(driver, persist_xml,
NULL, NULL)))
-goto cleanup;
+goto error;
 } else {
 virDomainDefPtr def = vm->newDef ? vm->newDef : vm->def;
 if (!(persistDef = qemuDomainDefCopy(driver, def,
  VIR_DOMAIN_XML_SECURE |
  VIR_DOMAIN_XML_MIGRATABLE)))
-goto cleanup;
+goto error;
 }
 }
 
 mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen,
  cookieFlags | QEMU_MIGRATION_COOKIE_GRAPHICS);
 if (!mig)
-goto cleanup;
+goto error;
 
 if (qemuDomainMigrateGraphicsRelocate(driver, vm, mig, graphicsuri) < 0)
 VIR_WARN("unable to provide data for graphics client relocation");
@@ -3683,7 +3683,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 if (qemuMigrationAddTLSObjects(driver, vm, cfg, false,
QEMU_ASYNC_JOB_MIGRATION_OUT,
&tlsAlias, &secAlias, migParams) < 0)
-goto cleanup;
+goto error;
 
 /* We need to add tls-hostname whenever QEMU itself does not
  * connect directly to the destination. */
@@ -3691,17 +3691,17 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 spec->destType == MIGRATION_DEST_FD) {
 if (VIR_STRDUP(migParams->migrateTLSHostname,
spec->dest.host.name) < 0)
-goto cleanup;
+goto error;
 } else {
 /* Be sure there's nothing from a previous migration */
 if (VIR_STRDUP(migParams->migrateTLSHostname, "") < 0)
-goto cleanup;
+goto error;
 }
 } else {
 if (qemuMigrationSetEmptyTLSParams(driver, vm,
QEMU_ASYNC_JOB_MIGRATION_OUT,
migParams) < 0)
-goto cleanup;
+goto error;
 }
 
 if (migrate_flags & (QEMU_MONITOR_MIGRATE_NON_SHARED_DISK |
@@ -3715,7 +3715,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
  nmigrate_disks,
  migrate_disks,
  dconn) < 0) {
-goto cleanup;
+goto error;
 }
 } else {
 /* Destination doesn't support NBD server.
@@ -3729,37 +3729,37 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 if (!(flags & VIR_MIGRATE_LIVE) &&
 virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
 if (qemuMigrationSetOffline(driver, vm) < 0)
-goto cleanup;
+goto error;
 }
 
 if (qemuMigrationSetCompression(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT,
 compression, migParams) < 0)
-goto cleanup;
+goto error;
 
 if (qemuMigrationSetOption(driver, vm,
QEMU_MONITOR_MIGRATION_CAPS_AUTO_CONVERGE,
flags & VIR_MIGRATE_AUTO_CONVERGE,
QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
-goto cleanup;
+goto error;
 
 if (qemuMigrationSetOption(driver, vm,
QEMU_MONITOR_MIGRATION_CAPS_RDMA_PIN_ALL,
flags & VIR_MIGRATE_RDMA_PIN_ALL,
QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
-goto cleanup;
+goto error;
 
 if (qemuMigrationSetPostCopy(driver, vm,
  flags & VIR_MIGRATE_POSTCOPY,
  QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
-goto cleanup;
+goto error;
 
 if (qemuMigrationSetParams(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT,
migParams) < 0)
-goto cleanup;
+goto error;
 
 if (qemuDomainObjEnterMonitorAsync(driver, vm,
QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
-goto cleanup;
+goto error;
 
 if (priv->job.abortJob) {
 /* explicitly do this *after* we entered the monitor,
@@ -3770,7 +3770,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
   

[libvirt] [PATCH 1/7] qemu: Use switch in qemuMigrationCompleted

[Jiri Denemark]

When adding a new job state it's useful to let the compiler complain
about places where we need to think about what to do with the new
state.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_migration.c | 23 ++-
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 72edbb667..c3f9c38b2 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1531,18 +1531,31 @@ qemuMigrationCompleted(virQEMUDriverPtr driver,
 return 0;
 
  error:
-/* state can not be active or completed at this point */
-if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
-jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
+switch (jobInfo->status) {
+case QEMU_DOMAIN_JOB_STATUS_MIGRATING:
+case QEMU_DOMAIN_JOB_STATUS_POSTCOPY:
 /* The migration was aborted by us rather than QEMU itself. */
 jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
 return -2;
-} else if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED) {
+
+case QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED:
+/* Something failed after QEMU already finished the migration. */
 jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
 return -1;
-} else {
+
+case QEMU_DOMAIN_JOB_STATUS_FAILED:
+case QEMU_DOMAIN_JOB_STATUS_CANCELED:
+/* QEMU aborted the migration. */
 return -1;
+
+case QEMU_DOMAIN_JOB_STATUS_ACTIVE:
+case QEMU_DOMAIN_JOB_STATUS_COMPLETED:
+case QEMU_DOMAIN_JOB_STATUS_NONE:
+/* Impossible. */
+break;
 }
+
+return -1;
 }
 
 
-- 
2.14.2

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

[libvirt] [REPOST PATCH 3/4] storage: Allow creation of a LUKS using logical volume

[John Ferlan]

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

Use virStorageBackendCreateVolUsingQemuImg to apply the LUKS information
to the logical volume just created.  As part of the processing of the
lvcreate command add 2MB to the capacity to account for the LUKS header
when it's determined that the volume desires to use encryption.

Signed-off-by: John Ferlan 
---
 src/storage/storage_backend_logical.c | 24 ++--
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/src/storage/storage_backend_logical.c 
b/src/storage/storage_backend_logical.c
index 93f4e0a595..5df30de29d 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -942,13 +942,14 @@ virStorageBackendLogicalLVCreate(virStorageVolDefPtr vol,
  virStoragePoolDefPtr def)
 {
 int ret;
+unsigned long long capacity = vol->target.capacity;
 virCommandPtr cmd = NULL;
 
 cmd = virCommandNewArgList(LVCREATE,
"--name", vol->name,
NULL);
 virCommandAddArg(cmd, "-L");
-if (vol->target.capacity != vol->target.allocation) {
+if (capacity != vol->target.allocation) {
 virCommandAddArgFormat(cmd, "%lluK",
VIR_DIV_UP(vol->target.allocation
   ? vol->target.allocation : 1, 1024));
@@ -956,8 +957,14 @@ virStorageBackendLogicalLVCreate(virStorageVolDefPtr vol,
 virCommandAddArg(cmd, "--virtualsize");
 vol->target.sparse = true;
 }
-virCommandAddArgFormat(cmd, "%lluK", VIR_DIV_UP(vol->target.capacity,
-1024));
+
+/* If we're going to encrypt using LUKS, then we could need up to
+ * an extra 2MB for the LUKS header - so account for that now */
+if (vol->target.encryption &&
+vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS)
+capacity += 2 * 1024 * 1024;
+virCommandAddArgFormat(cmd, "%lluK", VIR_DIV_UP(capacity, 1024));
+
 if (virStorageSourceHasBacking(&vol->target))
 virCommandAddArgList(cmd, "-s", vol->target.backingStore->path, NULL);
 else
@@ -979,13 +986,6 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
 virErrorPtr err;
 struct stat sb;
 
-if (vol->target.encryption != NULL) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   "%s", _("storage pool does not support encrypted "
-   "volumes"));
-return -1;
-}
-
 vol->type = VIR_STORAGE_VOL_BLOCK;
 
 VIR_FREE(vol->target.path);
@@ -996,6 +996,10 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
 if (virStorageBackendLogicalLVCreate(vol, def) < 0)
 return -1;
 
+if (vol->target.encryption &&
+virStorageBackendCreateVolUsingQemuImg(conn, pool, vol, NULL, 0) < 0)
+goto error;
+
 if ((fd = virStorageBackendVolOpen(vol->target.path, &sb,
VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0)
 goto error;
-- 
2.13.6

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

[libvirt] [REPOST PATCH 2/4] storage: Introduce virStorageBackendCreateVolUsingQemuImg

[John Ferlan]

Create a shim that will allow other backends to make use of qemu-img
functionality to create or possibly modify the volume.

Signed-off-by: John Ferlan 
---
 src/storage/storage_util.c | 42 ++
 src/storage/storage_util.h |  8 
 2 files changed, 50 insertions(+)

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 5252e429fd..e9bafd3b15 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -1409,6 +1409,48 @@ storageBackendCreateQemuImg(virConnectPtr conn,
 return ret;
 }
 
+
+/**
+ * virStorageBackendCreateVolUsingQemuImg
+ * @conn: Connection pointer
+ * @pool: Storage Pool Object
+ * @vol: Volume definition
+ * @inputvol: Volume to use for creation
+ * @flags: Flags for creation options
+ *
+ * A shim to storageBackendCreateQemuImg to allow other backends to
+ * utilize qemu-img processing in order to create or alter the volume.
+ *
+ * NB: If a volume target format is not supplied (per usual for some
+ * backends), temporarily adjust the format to be RAW. Once completed,
+ * reset the format back to NONE.
+ *
+ * Returns: 0 on success, -1 on failure.
+ */
+int
+virStorageBackendCreateVolUsingQemuImg(virConnectPtr conn,
+   virStoragePoolObjPtr pool,
+   virStorageVolDefPtr vol,
+   virStorageVolDefPtr inputvol,
+   unsigned int flags)
+{
+int ret = -1;
+bool changeFormat = false;
+
+if (vol->target.format == VIR_STORAGE_FILE_NONE) {
+vol->target.format = VIR_STORAGE_FILE_RAW;
+changeFormat = true;
+}
+
+ret = storageBackendCreateQemuImg(conn, pool, vol, inputvol, flags);
+
+if (changeFormat)
+vol->target.format = VIR_STORAGE_FILE_NONE;
+
+return ret;
+}
+
+
 virStorageBackendBuildVolFrom
 virStorageBackendGetBuildVolFromFunction(virStorageVolDefPtr vol,
  virStorageVolDefPtr inputvol)
diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h
index 00793ff3a4..dc7e62517b 100644
--- a/src/storage/storage_util.h
+++ b/src/storage/storage_util.h
@@ -27,6 +27,14 @@
 # include "storage_backend.h"
 
 /* File creation/cloning functions used for cloning between backends */
+
+int
+virStorageBackendCreateVolUsingQemuImg(virConnectPtr conn,
+   virStoragePoolObjPtr pool,
+   virStorageVolDefPtr vol,
+   virStorageVolDefPtr inputvol,
+   unsigned int flags);
+
 virStorageBackendBuildVolFrom
 virStorageBackendGetBuildVolFromFunction(virStorageVolDefPtr vol,
  virStorageVolDefPtr inputvol);
-- 
2.13.6

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

[libvirt] [PATCH 4/7] qemu: Unite error handling in qemuMigrationRun

[Jiri Denemark]

Merge cancel and cancelPostCopy sections with the generic error section,
where we can easily decide whether canceling the ongoing migration is
required.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_migration.c | 61 ++-
 1 file changed, 28 insertions(+), 33 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 6956901be..cbf255704 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3616,7 +3616,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 unsigned int cookieFlags = 0;
 bool abort_on_error = !!(flags & VIR_MIGRATE_ABORT_ON_ERROR);
 bool events = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
-bool inPostCopy = false;
+bool cancel = false;
 unsigned int waitFlags;
 virDomainDefPtr persistDef = NULL;
 char *timestamp;
@@ -3822,10 +3822,11 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 
 /* From this point onwards we *must* call cancel to abort the
  * migration on source if anything goes wrong */
+cancel = true;
 
 if (spec->fwdType != MIGRATION_FWD_DIRECT) {
 if (!(iothread = qemuMigrationStartTunnel(spec->fwd.stream, fd)))
-goto cancel;
+goto error;
 /* If we've created a tunnel, then the 'fd' will be closed in the
  * qemuMigrationIOFunc as data->sock.
  */
@@ -3843,13 +3844,18 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 rc = qemuMigrationWaitForCompletion(driver, vm,
 QEMU_ASYNC_JOB_MIGRATION_OUT,
 dconn, waitFlags);
-if (rc == -2)
-goto cancel;
-else if (rc == -1)
+if (rc == -2) {
 goto error;
+} else if (rc == -1) {
+/* QEMU reported failed migration, nothing to cancel anymore */
+cancel = false;
+goto error;
+}
 
-if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY)
-inPostCopy = true;
+if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED) {
+/* QEMU finished migration, nothing to cancel anymore */
+cancel = false;
+}
 
 /* When migration completed, QEMU will have paused the CPUs for us.
  * Wait for the STOP event to be processed or explicitly stop CPUs
@@ -3861,25 +3867,25 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 rc = virDomainObjWait(vm);
 priv->signalStop = false;
 if (rc < 0)
-goto cancelPostCopy;
+goto error;
 }
 } else if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING &&
qemuMigrationSetOffline(driver, vm) < 0) {
-goto cancelPostCopy;
+goto error;
 }
 
 if (mig && mig->nbd &&
 qemuMigrationCancelDriveMirror(driver, vm, true,
QEMU_ASYNC_JOB_MIGRATION_OUT,
dconn) < 0)
-goto cancelPostCopy;
+goto error;
 
 if (iothread) {
 qemuMigrationIOThreadPtr io;
 
 VIR_STEAL_PTR(io, iothread);
 if (qemuMigrationStopTunnel(io, false) < 0)
-goto cancelPostCopy;
+goto error;
 }
 
 if (priv->job.completed) {
@@ -3919,8 +3925,15 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 return ret;
 
  error:
-if (!orig_err)
-orig_err = virSaveLastError();
+orig_err = virSaveLastError();
+
+if (cancel &&
+virDomainObjIsActive(vm) &&
+qemuDomainObjEnterMonitorAsync(driver, vm,
+   QEMU_ASYNC_JOB_MIGRATION_OUT) == 0) {
+qemuMonitorMigrateCancel(priv->mon);
+ignore_value(qemuDomainObjExitMonitor(driver, vm));
+}
 
 /* cancel any outstanding NBD jobs */
 if (mig && mig->nbd)
@@ -3932,7 +3945,8 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 qemuMigrationStopTunnel(iothread, true);
 
 if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
-priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING)
+priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
+priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY)
 priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
 
 goto cleanup;
@@ -3940,25 +3954,6 @@ qemuMigrationRun(virQEMUDriverPtr driver,
  exit_monitor:
 ignore_value(qemuDomainObjExitMonitor(driver, vm));
 goto error;
-
- cancel:
-orig_err = virSaveLastError();
-
-if (virDomainObjIsActive(vm)) {
-if (qemuDomainObjEnterMonitorAsync(driver, vm,
-   QEMU_ASYNC_JOB_MIGRATION_OUT) == 0) 
{
-qemuMonitorMigrateCancel(priv->mon);
-ignore_value(qemuDomainObjExitMonitor(driver, vm));
-}
-}
-goto error;
-
- cancelPostCopy:
-priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
-if (inPostCopy)
-goto cancel;

[libvirt] [REPOST PATCH 0/4] Add the ability to LUKS encrypt during LV creation

[John Ferlan]

Repost to address merge conflict from commit id '0a294a8e2' which
used if (virStorageSourceHasBacking(&vol->target)) instead of
if (vol->target.backingStore).

Original series:
https://www.redhat.com/archives/libvir-list/2017-October/msg00340.html


John Ferlan (4):
  storage: Extract out the LVCREATE
  storage: Introduce virStorageBackendCreateVolUsingQemuImg
  storage: Allow creation of a LUKS using logical volume
  docs: Add news article

 docs/news.xml | 13 ++
 src/storage/storage_backend_logical.c | 75 ---
 src/storage/storage_util.c| 42 
 src/storage/storage_util.h|  8 
 4 files changed, 105 insertions(+), 33 deletions(-)

-- 
2.13.6

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

[libvirt] [REPOST PATCH 4/4] docs: Add news article

[John Ferlan]

Signed-off-by: John Ferlan 
---
 docs/news.xml | 13 +
 1 file changed, 13 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index ff36c800a4..cb59359d76 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -42,6 +42,19 @@
   
 
 
+  
+
+  Allow a logical volume to be create using LUKS
+
+
+  A logical volume may be created using an encryption
+  element using "luks" format. This does require a previously created
+  secret to store the passphrase used to encrypt the
+  volume Adding the volume to a domain can then either provide the
+  secret or allow the consumer in the guest to provide the passphrase
+  in order to decrypt the volume.
+
+  
 
 
   
-- 
2.13.6

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

[libvirt] [REPOST PATCH 1/4] storage: Extract out the LVCREATE

[John Ferlan]

Refactor to extract out the LVCREATE command.  This also removes the
need for the local @created since the error path can now only be reached
after the creation of the logical volume.

Signed-off-by: John Ferlan 
---
 src/storage/storage_backend_logical.c | 65 +++
 1 file changed, 35 insertions(+), 30 deletions(-)

diff --git a/src/storage/storage_backend_logical.c 
b/src/storage/storage_backend_logical.c
index a872a2f881..93f4e0a595 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -938,30 +938,11 @@ virStorageBackendLogicalDeleteVol(virConnectPtr conn 
ATTRIBUTE_UNUSED,
 
 
 static int
-virStorageBackendLogicalCreateVol(virConnectPtr conn,
-  virStoragePoolObjPtr pool,
-  virStorageVolDefPtr vol)
+virStorageBackendLogicalLVCreate(virStorageVolDefPtr vol,
+ virStoragePoolDefPtr def)
 {
-int fd = -1;
-virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
+int ret;
 virCommandPtr cmd = NULL;
-virErrorPtr err;
-struct stat sb;
-bool created = false;
-
-if (vol->target.encryption != NULL) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   "%s", _("storage pool does not support encrypted "
-   "volumes"));
-return -1;
-}
-
-vol->type = VIR_STORAGE_VOL_BLOCK;
-
-VIR_FREE(vol->target.path);
-if (virAsprintf(&vol->target.path, "%s/%s",
-def->target.path, vol->name) < 0)
-return -1;
 
 cmd = virCommandNewArgList(LVCREATE,
"--name", vol->name,
@@ -982,12 +963,38 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
 else
 virCommandAddArg(cmd, def->source.name);
 
-if (virCommandRun(cmd, NULL) < 0)
-goto error;
-
-created = true;
+ret = virCommandRun(cmd, NULL);
 virCommandFree(cmd);
-cmd = NULL;
+return ret;
+}
+
+
+static int
+virStorageBackendLogicalCreateVol(virConnectPtr conn,
+  virStoragePoolObjPtr pool,
+  virStorageVolDefPtr vol)
+{
+int fd = -1;
+virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
+virErrorPtr err;
+struct stat sb;
+
+if (vol->target.encryption != NULL) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   "%s", _("storage pool does not support encrypted "
+   "volumes"));
+return -1;
+}
+
+vol->type = VIR_STORAGE_VOL_BLOCK;
+
+VIR_FREE(vol->target.path);
+if (virAsprintf(&vol->target.path, "%s/%s",
+def->target.path, vol->name) < 0)
+return -1;
+
+if (virStorageBackendLogicalLVCreate(vol, def) < 0)
+return -1;
 
 if ((fd = virStorageBackendVolOpen(vol->target.path, &sb,
VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0)
@@ -1031,9 +1038,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
  error:
 err = virSaveLastError();
 VIR_FORCE_CLOSE(fd);
-if (created)
-virStorageBackendLogicalDeleteVol(conn, pool, vol, 0);
-virCommandFree(cmd);
+virStorageBackendLogicalDeleteVol(conn, pool, vol, 0);
 virSetError(err);
 virFreeError(err);
 return -1;
-- 
2.13.6

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

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

[Andrea Bolognani]

On Thu, 2017-10-19 at 14:53 +0200, Pavel Hrdina wrote:
> > So if your only argument against it is that you don't like it very
> > much, my reply is that I do like it quite a bit and, well, I get to
> > name the programs I write :)
> 
> Well, yes and no :) you can name the program but you also need to have
> an ACK from community to accept that name.  "licito" is just a cool name
> that doesn't tell you anything from the first glance what it is.  On the
> other hand lcitool tells you that it's some kind of tool and that the
> "lci" part specifies what kind of tool it is.  It's not only that I
> don't personally like it but it also looks like some randomly chosen
> name even though there is some pattern behind it.
> 
> I vote for lcitool instead of licito.

I don't feel like any of your arguments have much weight, since
for most applications the name only has a very vague correlation
with the functionality or intended purpose, if that: see mutt,
dnf, evince, firefox, ansible and so, so many more examples.

That said, point taken about the need for the community to stand
behind a name before it can be adopted.

Most importantly, I feel like we could both spend our time in a
more productive way than argue about this, so let's just stick
with the existing name unless someone comes up with a different
one that manages to make everyone happy.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 0/6] qemu: hotplug: Kill duplicated code in disk hotplug (blockdev-add saga)

[Ján Tomko]

On Thu, Oct 19, 2017 at 03:16:32PM +0200, Peter Krempa wrote:

A lot of code was copypasted into the three disk attaching functions
which are (almost) identical (one was forgotten), without refactoring it
first. Let's reduce two copies of the ugly code. BTW this adds support
for disk encryption and storage source authentication for USB disks.

Peter Krempa (6):
 qemu: address: Remove dead code when un-reserving PCI address
 qemu: hotplug: Remove wrong check for empty disks
 qemu: hotplug: Use disk target in debug/warning messages where
   appropriate
 qemu: hotplug: extract disk hotplug worker code
 qemu: hotplug: Reuse qemuDomainAttachDiskGeneric in
   qemuDomainAttachSCSIDisk
 qemu: hotplug: Reuse qemuDomainAttachDiskGeneric in
   qemuDomainAttachUSBMassStorageDevice

src/conf/domain_addr.c |   3 +-
src/conf/domain_addr.h |   4 +-
src/qemu/qemu_domain_address.c |   7 +-
src/qemu/qemu_hotplug.c| 273 -
4 files changed, 53 insertions(+), 234 deletions(-)


ACK series

Jan


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

[libvirt] [PATCH] qemu-ns: Detect /dev/* mount point duplicates better

[Michal Privoznik]

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

When creating new /dev for domain ran in namespace we try to
preserve all sub-mounts of /dev. Well, not quite all. For
instance if /dev/foo/bar and /dev/foo are both mount points, only
/dev/foo needs preserving. /dev/foo/bar is preserved with it too.
Now, to identify such cases like this one STRPREFIX() is used.
That is not good enough. While it works for [/dev/foo/bar;
/dev/foo] case, it fails for [/dev/prefix; /dev/prefix2] where
the strings share the same prefix but are in fact two different
paths. The solution is to use STRSKIP().

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 05e8b96aa..8aa66c69c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8182,7 +8182,9 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg,
 for (i = 1; i < nmounts; i++) {
 j = i + 1;
 while (j < nmounts) {
-if (STRPREFIX(mounts[j], mounts[i])) {
+char *c = STRSKIP(mounts[j], mounts[i]);
+
+if (c && *c == '/') {
 VIR_DEBUG("Dropping path %s because of %s", mounts[j], 
mounts[i]);
 VIR_DELETE_ELEMENT(mounts, j, nmounts);
 } else {
-- 
2.13.6

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

Re: [libvirt] [PATCH v1 00/14] Never ending story of user supplied aliases

[Pavel Hrdina]

On Thu, Oct 19, 2017 at 10:10:55AM +0200, Michal Privoznik wrote:
> As discussed earlier [1], we should allow users to set device
> aliases at the define time. While the discussed approach calls
> for generating missing aliases too, I'm saving that for another
> patch set. There are couple of reasons for that:
> 
> a) I don't think it's really necessary (if users are interested
>in a device they can just set the alias).
> 
> b) This patch set is already big enough as is.
> 
> c) Generating aliases is going to have its own problems.
> 
> Therefore, for now I'm only proposing parsing user supplied
> aliases. The idea is that it's not enabled by default for all
> drivers. Any driver that want to/can provide this feature has to
> set VIR_DOMAIN_DEF_FEATURE_USER_ALIAS. Note that we have some
> drivers that don't have notion of device aliases. But the code is
> generic enough so that it should be easy to use in the drivers
> that do know what aliases are.

This patch series missed some of the important parts of code
that relies on our generated aliases:

1. qemuGetNextChrDevIndex() ... this will fail while attaching new
   chardev device without an alias if there is one already provided
   by user and doesn't match the one that we generate.

2. qemuAssignDeviceRedirdevAlias() ... same as 1)

3. qemuAssignDeviceShmemAlias() ... same as 1)

4. qemuDomainNetVLAN() ... similar to the previous ones, hot-plugging
   network interface with user alias will fail because the alias is used
   to figure out VLAN of the interface.

Pavel


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

[libvirt] [PATCH 2/6] qemu: hotplug: Remove wrong check for empty disks

[Peter Krempa]

The check if the disk is empty is wrong and would spuriously reject NBD
sources. Remove it.
---
 src/qemu/qemu_hotplug.c | 8 
 1 file changed, 8 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 1a0844701..6bec69a5d 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -766,7 +766,6 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr 
driver,
 char *devstr = NULL;
 bool driveAdded = false;
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
-const char *src = virDomainDiskGetSource(disk);
 bool releaseaddr = false;

 if (priv->usbaddrs) {
@@ -778,13 +777,6 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr 
driver,
 if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0)
 goto cleanup;

-/* XXX not correct once we allow attaching a USB CDROM */
-if (!src) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("disk source path is missing"));
-goto error;
-}
-
 if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0)
 goto error;

-- 
2.14.1

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

[libvirt] [PATCH 0/6] qemu: hotplug: Kill duplicated code in disk hotplug (blockdev-add saga)

[Peter Krempa]

A lot of code was copypasted into the three disk attaching functions
which are (almost) identical (one was forgotten), without refactoring it
first. Let's reduce two copies of the ugly code. BTW this adds support
for disk encryption and storage source authentication for USB disks.

Peter Krempa (6):
  qemu: address: Remove dead code when un-reserving PCI address
  qemu: hotplug: Remove wrong check for empty disks
  qemu: hotplug: Use disk target in debug/warning messages where
appropriate
  qemu: hotplug: extract disk hotplug worker code
  qemu: hotplug: Reuse qemuDomainAttachDiskGeneric in
qemuDomainAttachSCSIDisk
  qemu: hotplug: Reuse qemuDomainAttachDiskGeneric in
qemuDomainAttachUSBMassStorageDevice

 src/conf/domain_addr.c |   3 +-
 src/conf/domain_addr.h |   4 +-
 src/qemu/qemu_domain_address.c |   7 +-
 src/qemu/qemu_hotplug.c| 273 -
 4 files changed, 53 insertions(+), 234 deletions(-)

-- 
2.14.1

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

[libvirt] [PATCH 5/6] qemu: hotplug: Reuse qemuDomainAttachDiskGeneric in qemuDomainAttachSCSIDisk

[Peter Krempa]

Get rid of the first copy of the mess.
---
 src/qemu/qemu_hotplug.c | 127 ++--
 1 file changed, 5 insertions(+), 122 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index f251af904..299e25845 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -627,32 +627,13 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
  virDomainDiskDefPtr disk)
 {
 size_t i;
-qemuDomainObjPrivatePtr priv = vm->privateData;
-virErrorPtr orig_err;
-char *drivestr = NULL;
-char *devstr = NULL;
-bool driveAdded = false;
-bool encobjAdded = false;
-bool secobjAdded = false;
-char *drivealias = NULL;
-int ret = -1;
-int rv;
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
-virJSONValuePtr encobjProps = NULL;
-virJSONValuePtr secobjProps = NULL;
-qemuDomainDiskPrivatePtr diskPriv;
-qemuDomainSecretInfoPtr encinfo;
-qemuDomainSecretInfoPtr secinfo;
-
-if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0)
-goto cleanup;

 /* We should have an address already, so make sure */
 if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected disk address type %s"),
virDomainDeviceAddressTypeToString(disk->info.type));
-goto error;
+return -1;
 }

 /* Let's make sure the disk has a controller defined and loaded before
@@ -664,111 +645,13 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
  */
 for (i = 0; i <= disk->info.addr.drive.controller; i++) {
 if (!qemuDomainFindOrCreateSCSIDiskController(driver, vm, i))
-goto error;
-}
-
-if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0)
-goto error;
-
-if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0)
-goto error;
-
-diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
-secinfo = diskPriv->secinfo;
-if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
-if (qemuBuildSecretInfoProps(secinfo, &secobjProps) < 0)
-goto error;
-}
-
-encinfo = diskPriv->encinfo;
-if (encinfo && qemuBuildSecretInfoProps(encinfo, &encobjProps) < 0)
-goto error;
-
-if (!(devstr = qemuBuildDriveDevStr(vm->def, disk, 0, priv->qemuCaps)))
-goto error;
-
-if (qemuDomainPrepareDiskSourceTLS(disk->src, disk->info.alias, cfg) < 0)
-goto error;
-
-if (disk->src->haveTLS &&
-qemuDomainAddDiskSrcTLSObject(driver, vm, disk->src,
-  disk->info.alias) < 0)
-goto error;
-
-if (!(drivestr = qemuBuildDriveStr(disk, cfg, false, priv->qemuCaps)))
-goto error;
-
-if (!(drivealias = qemuAliasFromDisk(disk)))
-goto error;
-
-if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0)
-goto error;
-
-qemuDomainObjEnterMonitor(driver, vm);
-
-if (secobjProps) {
-rv = qemuMonitorAddObject(priv->mon, "secret", secinfo->s.aes.alias,
-  secobjProps);
-secobjProps = NULL; /* qemuMonitorAddObject consumes */
-if (rv < 0)
-goto exit_monitor;
-secobjAdded = true;
-}
-
-if (encobjProps) {
-rv = qemuMonitorAddObject(priv->mon, "secret", encinfo->s.aes.alias,
-  encobjProps);
-encobjProps = NULL; /* qemuMonitorAddObject consumes */
-if (rv < 0)
-goto exit_monitor;
-encobjAdded = true;
-}
-
-if (qemuMonitorAddDrive(priv->mon, drivestr) < 0)
-goto exit_monitor;
-driveAdded = true;
-
-if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
-goto exit_monitor;
-
-if (qemuDomainObjExitMonitor(driver, vm) < 0)
-goto error;
-
-virDomainAuditDisk(vm, NULL, disk->src, "attach", true);
-
-virDomainDiskInsertPreAlloced(vm->def, disk);
-ret = 0;
-
- cleanup:
-virJSONValueFree(secobjProps);
-virJSONValueFree(encobjProps);
-qemuDomainSecretDiskDestroy(disk);
-VIR_FREE(devstr);
-VIR_FREE(drivestr);
-VIR_FREE(drivealias);
-virObjectUnref(cfg);
-return ret;
-
- exit_monitor:
-virErrorPreserveLast(&orig_err);
-if (driveAdded && qemuMonitorDriveDel(priv->mon, drivealias) < 0) {
-VIR_WARN("Unable to remove drive %s (%s) after failed "
- "qemuMonitorAddDevice", drivealias, drivestr);
+return -1;
 }
-if (secobjAdded)
-ignore_value(qemuMonitorDelObject(priv->mon, secinfo->s.aes.alias));
-if (encobjAdded)
-ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias));
-ignore_value(qemuDomainObjExitMonitor(driver, vm));
-virErrorRestore(&orig_err);

-virDomainAuditDisk(vm, NULL, disk->src, "attach", false);
-
- error:
-qemuDomainDelDiskSrcTLSObject(driver, vm, 

[libvirt] [PATCH 1/6] qemu: address: Remove dead code when un-reserving PCI address

[Peter Krempa]

The code can't fail so having error handling is pointless.
---
 src/conf/domain_addr.c | 3 +--
 src/conf/domain_addr.h | 4 ++--
 src/qemu/qemu_domain_address.c | 7 ++-
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 531fc6800..642268239 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -687,12 +687,11 @@ virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr 
addrs,
 }


-int
+void
 virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs,
virPCIDeviceAddressPtr addr)
 {
 addrs->buses[addr->bus].slot[addr->slot].functions &= ~(1 << 
addr->function);
-return 0;
 }

 virDomainPCIAddressSetPtr
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index 205e7cfe5..173101465 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -166,8 +166,8 @@ int virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr 
addrs,
   virDomainPCIConnectFlags flags)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

-int virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs,
-   virPCIDeviceAddressPtr addr)
+void virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs,
+virPCIDeviceAddressPtr addr)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

 void virDomainPCIAddressSetAllMulti(virDomainDefPtr def)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index b94b73eaa..7f4ac0f45 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -2892,11 +2892,8 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm,
 if (!devstr)
 devstr = info->alias;

-if (virDeviceInfoPCIAddressPresent(info) &&
-virDomainPCIAddressReleaseAddr(priv->pciaddrs,
-   &info->addr.pci) < 0)
-VIR_WARN("Unable to release PCI address on %s",
- NULLSTR(devstr));
+if (virDeviceInfoPCIAddressPresent(info))
+virDomainPCIAddressReleaseAddr(priv->pciaddrs, &info->addr.pci);

 if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB &&
 priv->usbaddrs &&
-- 
2.14.1

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

[libvirt] [PATCH 6/6] qemu: hotplug: Reuse qemuDomainAttachDiskGeneric in qemuDomainAttachUSBMassStorageDevice

[Peter Krempa]

Apart from killing a lot of code this also "implements" authentication
and encryption for USB disks.
---
 src/qemu/qemu_hotplug.c | 84 +
 1 file changed, 7 insertions(+), 77 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 299e25845..5efc60aea 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -656,94 +656,24 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,


 static int
-qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver,
+qemuDomainAttachUSBMassStorageDevice(virConnectPtr conn,
+ virQEMUDriverPtr driver,
  virDomainObjPtr vm,
  virDomainDiskDefPtr disk)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
-virErrorPtr orig_err;
-int ret = -1;
-char *drivealias = NULL;
-char *drivestr = NULL;
-char *devstr = NULL;
-bool driveAdded = false;
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
-bool releaseaddr = false;

 if (priv->usbaddrs) {
 if (virDomainUSBAddressEnsure(priv->usbaddrs, &disk->info) < 0)
-goto cleanup;
-releaseaddr = true;
+return -1;
 }

-if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0)
-goto cleanup;
-
-if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0)
-goto error;
-
-if (qemuDomainPrepareDiskSourceTLS(disk->src, disk->info.alias, cfg) < 0)
-goto error;
-
-if (disk->src->haveTLS &&
-qemuDomainAddDiskSrcTLSObject(driver, vm, disk->src,
-  disk->info.alias) < 0)
-goto error;
-
-if (!(drivestr = qemuBuildDriveStr(disk, cfg, false, priv->qemuCaps)))
-goto error;
-
-if (!(drivealias = qemuAliasFromDisk(disk)))
-goto error;
-
-if (!(devstr = qemuBuildDriveDevStr(vm->def, disk, 0, priv->qemuCaps)))
-goto error;
-
-if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0)
-goto error;
-
-qemuDomainObjEnterMonitor(driver, vm);
-
-if (qemuMonitorAddDrive(priv->mon, drivestr) < 0)
-goto exit_monitor;
-driveAdded = true;
-
-if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
-goto exit_monitor;
-
-if (qemuDomainObjExitMonitor(driver, vm) < 0)
-goto error;
-
-virDomainAuditDisk(vm, NULL, disk->src, "attach", true);
-
-virDomainDiskInsertPreAlloced(vm->def, disk);
-ret = 0;
-
- cleanup:
-if (ret < 0 && releaseaddr)
+if (qemuDomainAttachDiskGeneric(conn, driver, vm, disk) < 0) {
 virDomainUSBAddressRelease(priv->usbaddrs, &disk->info);
-VIR_FREE(devstr);
-VIR_FREE(drivealias);
-VIR_FREE(drivestr);
-virObjectUnref(cfg);
-return ret;
-
- exit_monitor:
-virErrorPreserveLast(&orig_err);
-if (driveAdded && qemuMonitorDriveDel(priv->mon, drivealias) < 0) {
-VIR_WARN("Unable to remove drive %s (%s) after failed "
- "qemuMonitorAddDevice", drivealias, drivestr);
+return -1;
 }
-ignore_value(qemuDomainObjExitMonitor(driver, vm));
-virErrorRestore(&orig_err);
-
-virDomainAuditDisk(vm, NULL, disk->src, "attach", false);

- error:
-qemuDomainDelDiskSrcTLSObject(driver, vm, disk->src);
-
-ignore_value(qemuDomainPrepareDisk(driver, vm, disk, NULL, true));
-goto cleanup;
+return 0;
 }


@@ -813,7 +743,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
_("disk device='lun' is not supported for usb 
bus"));
 break;
 }
-ret = qemuDomainAttachUSBMassStorageDevice(driver, vm, disk);
+ret = qemuDomainAttachUSBMassStorageDevice(conn, driver, vm, disk);
 break;

 case VIR_DOMAIN_DISK_BUS_VIRTIO:
-- 
2.14.1

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

[libvirt] [PATCH 3/6] qemu: hotplug: Use disk target in debug/warning messages where appropriate

[Peter Krempa]

Some messages deal with the disk itself thus using the disk target is
better than using the disk source name which can be NULL in some cases.
---
 src/qemu/qemu_hotplug.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 6bec69a5d..f00d4f546 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -364,7 +364,6 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
 bool secobjAdded = false;
 bool encobjAdded = false;
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
-const char *src = virDomainDiskGetSource(disk);
 virJSONValuePtr secobjProps = NULL;
 virJSONValuePtr encobjProps = NULL;
 qemuDomainDiskPrivatePtr diskPriv;
@@ -481,7 +480,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
 qemuDomainDelDiskSrcTLSObject(driver, vm, disk->src);

 if (releaseaddr)
-qemuDomainReleaseDeviceAddress(vm, &disk->info, src);
+qemuDomainReleaseDeviceAddress(vm, &disk->info, disk->dst);

 ignore_value(qemuDomainPrepareDisk(driver, vm, disk, NULL, true));
 goto cleanup;
@@ -855,12 +854,11 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
 virDomainDiskDefPtr disk = dev->data.disk;
 virDomainDiskDefPtr orig_disk = NULL;
 int ret = -1;
-const char *src = virDomainDiskGetSource(disk);

 if (STRNEQ_NULLABLE(virDomainDiskGetDriver(disk), "qemu")) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unsupported driver name '%s' for disk '%s'"),
-   virDomainDiskGetDriver(disk), src);
+   virDomainDiskGetDriver(disk), disk->dst);
 goto cleanup;
 }

-- 
2.14.1

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

[libvirt] [PATCH 4/6] qemu: hotplug: extract disk hotplug worker code

[Peter Krempa]

This horrible piece of spaghetti code is copy-past(ae)d in the SCSI and
USB disk hotplug code with minimal changes. Extract it for further
reuse.
---
 src/qemu/qemu_hotplug.c | 50 ++---
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index f00d4f546..f251af904 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -345,21 +345,24 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
 }


+/**
+ * qemuDomainAttachDiskGeneric:
+ *
+ * Attaches disk to a VM. This function aggregates common code for all bus 
types.
+ * In cases when the VM crashed while adding the disk, -2 is returned. */
 static int
-qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
- virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDiskDefPtr disk)
+qemuDomainAttachDiskGeneric(virConnectPtr conn,
+virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+virDomainDiskDefPtr disk)
 {
 int ret = -1;
 int rv;
 qemuDomainObjPrivatePtr priv = vm->privateData;
-virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_DISK, { .disk = disk } };
 virErrorPtr orig_err;
 char *devstr = NULL;
 char *drivestr = NULL;
 char *drivealias = NULL;
-bool releaseaddr = false;
 bool driveAdded = false;
 bool secobjAdded = false;
 bool encobjAdded = false;
@@ -373,9 +376,6 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
 if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0)
 goto cleanup;

-if (qemuDomainEnsureVirtioAddress(&releaseaddr, vm, &dev, disk->dst) < 0)
-goto error;
-
 if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0)
 goto error;

@@ -441,7 +441,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
 goto exit_monitor;

 if (qemuDomainObjExitMonitor(driver, vm) < 0) {
-releaseaddr = false;
+ret = -2;
 goto error;
 }

@@ -471,22 +471,42 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
 if (encobjAdded)
 ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias));
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
-releaseaddr = false;
+ret = -2;
 virErrorRestore(&orig_err);

 virDomainAuditDisk(vm, NULL, disk->src, "attach", false);

  error:
 qemuDomainDelDiskSrcTLSObject(driver, vm, disk->src);
-
-if (releaseaddr)
-qemuDomainReleaseDeviceAddress(vm, &disk->info, disk->dst);
-
 ignore_value(qemuDomainPrepareDisk(driver, vm, disk, NULL, true));
 goto cleanup;
 }


+static int
+qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
+ virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk)
+{
+virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_DISK, { .disk = disk } };
+bool releaseaddr = false;
+int rv;
+
+if (qemuDomainEnsureVirtioAddress(&releaseaddr, vm, &dev, disk->dst) < 0)
+return -1;
+
+if ((rv = qemuDomainAttachDiskGeneric(conn, driver, vm, disk)) < 0) {
+if (rv == -1 && releaseaddr)
+qemuDomainReleaseDeviceAddress(vm, &disk->info, disk->dst);
+
+return -1;
+}
+
+return 0;
+}
+
+
 int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
  virDomainControllerDefPtr controller)
-- 
2.14.1

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

Re: [libvirt] [PATCH 12/12] qemu: implement input device hotunplug

[Ján Tomko]

On Thu, Oct 19, 2017 at 08:11:39AM -0400, John Ferlan wrote:



On 10/17/2017 11:04 AM, Ján Tomko wrote:


[...]


@@ -4430,6 +4430,31 @@ qemuDomainRemoveWatchdog(virQEMUDriverPtr driver,
 }


+static int
+qemuDomainRemoveInputDevice(virDomainObjPtr vm,
+virDomainInputDefPtr dev)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+virQEMUDriverPtr driver = priv->driver;


So none of the Remove* API's need to pass @driver... doh! Never even
considered that - the first non-lemming ;-)!



Not since commit 2e6ecba, but the cleanup is TBD.

Jan


John

[...]


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

Re: [libvirt] [PATCH 04/12] Move qemuCheckCCWS390AddressSupport to qemu_domain

[Ján Tomko]

On Thu, Oct 19, 2017 at 08:08:34AM -0400, John Ferlan wrote:



On 10/17/2017 11:04 AM, Ján Tomko wrote:

Let it be reused in qemu_domain_address.


Alternatively you could have added "#include qemu_command.h" to
qemu_domain_address.c, right?



The function does not deal directly with the command line and
including qemu_command.h would feel wrong.


IDC about moving, but if you do



[...]


diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 5201c6a0a..6abefc929 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -978,4 +978,9 @@ qemuDomainFixupCPUs(virDomainObjPtr vm,
 char *
 qemuDomainGetMachineName(virDomainObjPtr vm);

+bool qemuCheckCCWS390AddressSupport(const virDomainDef *def,
+virDomainDeviceInfo info,
+virQEMUCapsPtr qemuCaps,
+const char *devicename);


More recently I've been encouraged by some reviewers to use:

bool
qemuDomainCheck...();

type format to follow the .c file format when adding functions.



I purposefully formatted it this way to match most of the prototypes
in the file.

Jan


John


+
 #endif /* __QEMU_DOMAIN_H__ */



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

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

[Pavel Hrdina]

On Thu, Oct 19, 2017 at 02:42:15PM +0200, Andrea Bolognani wrote:
> On Thu, 2017-10-19 at 14:04 +0200, Pavel Hrdina wrote:
> > > > 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
> > > 
> > > FYI: I came up with a cuter name, licito[1], and I will use that when
> > > pushing unless someone feel strongly otherwise .
> > 
> > I liked manage better that lcitool, but lcitool makes sense as well.
> > However, I don't like licito, it might sound cool but IMHO it's not
> > useful at all.
> 
> I liked 'manage' better as well but it's also extremely generic, so
> the moment we started writing data outside the source directory it
> was automatically off the table.

I agree with that, manage is not usable in this case.

> Neither of the other names is particularly useful, they're just
> names but at least they're both fairly unique and reasonably easy
> to remember. I think that 'licito' is a very fair name, and using
> it would definitely not be illegal :P
> 
>   http://www.spanishcentral.com/translate/l%C3%ADcito
> 
> So if your only argument against it is that you don't like it very
> much, my reply is that I do like it quite a bit and, well, I get to
> name the programs I write :)

Well, yes and no :) you can name the program but you also need to have
an ACK from community to accept that name.  "licito" is just a cool name
that doesn't tell you anything from the first glance what it is.  On the
other hand lcitool tells you that it's some kind of tool and that the
"lci" part specifies what kind of tool it is.  It's not only that I
don't personally like it but it also looks like some randomly chosen
name even though there is some pattern behind it.

I vote for lcitool instead of licito.

Pavel


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

[libvirt] [PATCH] fix error message spacing in qemuDomainDetachNetDevice

[Ján Tomko]

Move the space after the colon.
---
Pushed as trivial, suggested by John in review:
https://www.redhat.com/archives/libvir-list/2017-October/msg00833.html

 src/qemu/qemu_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 57240ce67..1a0844701 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5219,7 +5219,7 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
 
 if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) {
 virReportError(VIR_ERR_OPERATION_FAILED,
-   _("cannot hot unplug multifunction PCI device :%s"),
+   _("cannot hot unplug multifunction PCI device: %s"),
dev->data.disk->dst);
 goto cleanup;
 }
-- 
2.13.0

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

Re: [libvirt] [PATCH v1 09/14] virDomainDeviceInfoCheckABIStability: Check for alias too

[Martin Kletzander]

On Thu, Oct 19, 2017 at 01:27:11PM +0100, Daniel P. Berrange wrote:

On Thu, Oct 19, 2017 at 02:16:41PM +0200, Martin Kletzander wrote:

On Thu, Oct 19, 2017 at 10:11:04AM +0200, Michal Privoznik wrote:
> Since we'll be passing user's input onto qemu command line, we
> have to make sure aliases don't change during migration and all
> the other places where ABI is checked. Aliases are part of ABI
> now.
>

I don't see how change of an alias would have an impact on the guest OS since it
is not visible at all.  Is QEMU using the IDs to refer to objects in the
migrated data stream?  AFAIK (actually not me, I just asked random stranger
about it) it does not (except the memory modules that Peter told us about in one
of previous series).  I see no reason why we would disable the option to rename
a device when doing migration.  We are reconstructing the command-line anyway.
Did migration fail for you when you tried this?


QEMU never includes the device ID in the migration stream, except for that
one screw-up wrt memory modules that we know about.

So technically the change below could be loosened to only apply to the
memory module alias. Since the whole point of aliases is to have a long
term stable identifier for devices though, I figure it is probably
beneficial to enforce no-renames for all devices even if QEMU is safe
without it.



Well, depends how you define long-term.  Since it is the user/mgmt app that
defines these aliases, I don't see why they could not change them.



> Signed-off-by: Michal Privoznik 
> ---
> src/conf/domain_conf.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0cf67dff1..cb80939af 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -19870,6 +19870,13 @@ 
virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src,
> return false;
> }
>
> +if (STRNEQ_NULLABLE(src->alias, dst->alias)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Target device alias %s does not match source %s"),
> +   NULLSTR(src->alias), NULLSTR(dst->alias));
> +return false;
> +}
> +
> switch ((virDomainDeviceAddressType) src->type) {
> case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
> if (src->addr.pci.domain != dst->addr.pci.domain ||


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 :|


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

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

[Marc Haber]

On Thu, Oct 19, 2017 at 01:37:45PM +0200, Michal Privoznik wrote:
> Aha! the thing is, you're using netcf backend while I'm using the udev
> one. This error message comes from netcf. It's a netcf's bug. CCing
> Laine who should know more.

Where can I read up about the different backends available? Is it a
compile-time setting or can I configure that on my system?

Actually, I'm doing the configuration outside of libvirt anyway (with
systemd-networkd), I just want the interface to be in virt-manager's
selection dropdown, so I don't care too much about netcf.

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

Re: [libvirt] How can I tell if I'm using qemu-kvm or qemu-kvm-ev from libvirt Python binding?

[Martin Kletzander]

On Thu, Oct 19, 2017 at 11:43:44AM +0200, Pavel Hrdina wrote:

On Thu, Oct 19, 2017 at 10:22:59AM +0200, Martin Kletzander wrote:

On Thu, Oct 19, 2017 at 10:34:21AM +0300, Yaniv Kaul wrote:
> I'm trying to (optionally) use iothreads, which only exist on qemu-kvm-ev.
> I was hoping to see it in the domcapabilities, but I don't see it there.
> How do I know if it's supported or not?
>

Well, you can get the emulator binary from /domainCapabilities/path, but that's
not what you should be using for any support-related decisions.

I guess there is no easy way to do that, feel free to create a BZ that we add it
to domcapabilities (or just capabilities maybe) and assign it to me so that we
don't lose track of it.  I'll post a patch for it then, it will be trivial, the
only thing we need is design decision about how to name it and where to put it.


I don't think that this is something we would like to have in upstream
code.  The differentiation between qemu-kvm and qemu-kvm-ev is
dowsntream thing.  In addition, there is no qemu-kvm in upstream, there
is only QEMU.

What we can add into capabilities is information whether the QEMU binary
supports iothreads or not, but please, don't introduce anything that's
downstream only.



That's what I meant by "feel free to create a BZ that we add it to
domcapabilities (or just capabilities maybe)", it was meant for the iothread
support.  I already have the patch for it, just need to add it to tests.


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

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

[Andrea Bolognani]

On Thu, 2017-10-19 at 14:04 +0200, Pavel Hrdina wrote:
> > > 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
> > 
> > FYI: I came up with a cuter name, licito[1], and I will use that when
> > pushing unless someone feel strongly otherwise .
> 
> I liked manage better that lcitool, but lcitool makes sense as well.
> However, I don't like licito, it might sound cool but IMHO it's not
> useful at all.

I liked 'manage' better as well but it's also extremely generic, so
the moment we started writing data outside the source directory it
was automatically off the table.

Neither of the other names is particularly useful, they're just
names but at least they're both fairly unique and reasonably easy
to remember. I think that 'licito' is a very fair name, and using
it would definitely not be illegal :P

  http://www.spanishcentral.com/translate/l%C3%ADcito

So if your only argument against it is that you don't like it very
much, my reply is that I do like it quite a bit and, well, I get to
name the programs I write :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v1 09/14] virDomainDeviceInfoCheckABIStability: Check for alias too

[Daniel P. Berrange]

On Thu, Oct 19, 2017 at 02:16:41PM +0200, Martin Kletzander wrote:
> On Thu, Oct 19, 2017 at 10:11:04AM +0200, Michal Privoznik wrote:
> > Since we'll be passing user's input onto qemu command line, we
> > have to make sure aliases don't change during migration and all
> > the other places where ABI is checked. Aliases are part of ABI
> > now.
> > 
> 
> I don't see how change of an alias would have an impact on the guest OS since 
> it
> is not visible at all.  Is QEMU using the IDs to refer to objects in the
> migrated data stream?  AFAIK (actually not me, I just asked random stranger
> about it) it does not (except the memory modules that Peter told us about in 
> one
> of previous series).  I see no reason why we would disable the option to 
> rename
> a device when doing migration.  We are reconstructing the command-line anyway.
> Did migration fail for you when you tried this?

QEMU never includes the device ID in the migration stream, except for that
one screw-up wrt memory modules that we know about.

So technically the change below could be loosened to only apply to the
memory module alias. Since the whole point of aliases is to have a long
term stable identifier for devices though, I figure it is probably
beneficial to enforce no-renames for all devices even if QEMU is safe
without it.

> 
> > Signed-off-by: Michal Privoznik 
> > ---
> > src/conf/domain_conf.c | 7 +++
> > 1 file changed, 7 insertions(+)
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 0cf67dff1..cb80939af 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -19870,6 +19870,13 @@ 
> > virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src,
> > return false;
> > }
> > 
> > +if (STRNEQ_NULLABLE(src->alias, dst->alias)) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +   _("Target device alias %s does not match source 
> > %s"),
> > +   NULLSTR(src->alias), NULLSTR(dst->alias));
> > +return false;
> > +}
> > +
> > switch ((virDomainDeviceAddressType) src->type) {
> > case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
> > if (src->addr.pci.domain != dst->addr.pci.domain ||

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/2] qemu: remove pointless address validation on hot unplug

[John Ferlan]

On 10/18/2017 09:55 AM, Ján Tomko wrote:
> 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(&detach->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(&detach->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(&detach->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(&detach->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, &detach->info)) {
> -virReportError(VIR_ERR_OPERATION_FAILED,
> -_("cannot hot unplug multifunction PCI device 
> :%s"),
> -dev->data.disk->dst);
> -goto cleanup;
> -}
> +if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) {
> +virReportError(VIR_ERR_OPERATION_FAILED,
> +   _("cannot hot unplug multifunction PCI device :%s"),

May as well clean up the spacing while we're at it...

s/device :%s/device: %s/

Reviewed-by: John Ferlan 

John

> +   dev->data.disk->dst);
> +goto cleanup;
>  }
>  
>  if (!detach->info.alias) {
> 

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

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

[John Ferlan]

On 10/18/2017 09:55 AM, Ján Tomko wrote:
> 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(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v1 09/14] virDomainDeviceInfoCheckABIStability: Check for alias too

[Martin Kletzander]

On Thu, Oct 19, 2017 at 10:11:04AM +0200, Michal Privoznik wrote:

Since we'll be passing user's input onto qemu command line, we
have to make sure aliases don't change during migration and all
the other places where ABI is checked. Aliases are part of ABI
now.



I don't see how change of an alias would have an impact on the guest OS since it
is not visible at all.  Is QEMU using the IDs to refer to objects in the
migrated data stream?  AFAIK (actually not me, I just asked random stranger
about it) it does not (except the memory modules that Peter told us about in one
of previous series).  I see no reason why we would disable the option to rename
a device when doing migration.  We are reconstructing the command-line anyway.
Did migration fail for you when you tried this?


Signed-off-by: Michal Privoznik 
---
src/conf/domain_conf.c | 7 +++
1 file changed, 7 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0cf67dff1..cb80939af 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -19870,6 +19870,13 @@ 
virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src,
return false;
}

+if (STRNEQ_NULLABLE(src->alias, dst->alias)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Target device alias %s does not match source %s"),
+   NULLSTR(src->alias), NULLSTR(dst->alias));
+return false;
+}
+
switch ((virDomainDeviceAddressType) src->type) {
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
if (src->addr.pci.domain != dst->addr.pci.domain ||
--
2.13.6

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


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

Re: [libvirt] [PATCH 00/12] qemu: input device hotplug

[John Ferlan]

On 10/17/2017 11:04 AM, Ján Tomko wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1379603
> 
> Ján Tomko (12):
>   conf: audit passthrough input devices at domain startup
>   qemuDomainAttachControllerDevice: remove dead code
>   qemuDomainAttachRNGDevice: do not access source.file randomly
>   Move qemuCheckCCWS390AddressSupport to qemu_domain
>   Split out qemuDomainEnsureVirtioAddress
>   Use qemuDomainEnsureVirtioAddress where possible
>   qemu: allow coldplugging input devices
>   qemu: allow cold unplugging of input devices
>   split out qemuAssignDeviceInputAlias
>   Introduce qemuBuildInputDevStr
>   qemu: implement input device hotplug
>   qemu: implement input device hotunplug
> 
>  src/conf/domain_audit.c|  44 
>  src/conf/domain_audit.h|   5 +
>  src/conf/domain_conf.c |  37 +++
>  src/conf/domain_conf.h |   3 +
>  src/libvirt_private.syms   |   3 +
>  src/qemu/qemu_alias.c  |  24 -
>  src/qemu/qemu_alias.h  |   3 +
>  src/qemu/qemu_command.c|  82 +-
>  src/qemu/qemu_command.h|  12 ++-
>  src/qemu/qemu_domain.c |  40 +++
>  src/qemu/qemu_domain.h |   5 +
>  src/qemu/qemu_domain_address.c |  45 
>  src/qemu/qemu_domain_address.h |   4 +
>  src/qemu/qemu_driver.c |  25 -
>  src/qemu/qemu_hotplug.c| 239 
> +++--
>  src/qemu/qemu_hotplug.h|   8 ++
>  16 files changed, 436 insertions(+), 143 deletions(-)
> 

Noted a couple of issues along the way to clean up for a...

Reviewed-by: John Ferlan 
(series)

John

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

Re: [libvirt] [PATCH 12/12] qemu: implement input device hotunplug

[John Ferlan]

On 10/17/2017 11:04 AM, Ján Tomko wrote:
> Allow unplugging USB and virtio USB devices.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1379603
> ---
>  src/qemu/qemu_driver.c  |  4 ++-
>  src/qemu/qemu_hotplug.c | 76 
> +
>  src/qemu/qemu_hotplug.h |  3 ++
>  3 files changed, 82 insertions(+), 1 deletion(-)
> 

Another aha moment ;-)... No issue here...

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 75a0e42aa..a9d3ba778 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7764,9 +7764,11 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>  case VIR_DOMAIN_DEVICE_WATCHDOG:
>  ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog);
>  break;
> +case VIR_DOMAIN_DEVICE_INPUT:
> +ret = qemuDomainDetachInputDevice(vm, dev->data.input);
> +break;
>  
>  case VIR_DOMAIN_DEVICE_FS:
> -case VIR_DOMAIN_DEVICE_INPUT:
>  case VIR_DOMAIN_DEVICE_SOUND:
>  case VIR_DOMAIN_DEVICE_VIDEO:
>  case VIR_DOMAIN_DEVICE_GRAPHICS:
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index b32acb71e..85faa2a46 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -4430,6 +4430,31 @@ qemuDomainRemoveWatchdog(virQEMUDriverPtr driver,
>  }
>  
>  
> +static int
> +qemuDomainRemoveInputDevice(virDomainObjPtr vm,
> +virDomainInputDefPtr dev)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +virQEMUDriverPtr driver = priv->driver;

So none of the Remove* API's need to pass @driver... doh! Never even
considered that - the first non-lemming ;-)!

John

[...]

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

Re: [libvirt] [PATCH 10/12] Introduce qemuBuildInputDevStr

[John Ferlan]

On 10/17/2017 11:04 AM, Ján Tomko wrote:
> A function that builds the -device string for input devices.
> ---
>  src/qemu/qemu_command.c | 42 +-
>  src/qemu/qemu_command.h |  7 +++
>  2 files changed, 36 insertions(+), 13 deletions(-)
> 

[...]

> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 1254ad4df..0961ec8cb 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -204,4 +204,11 @@ char *qemuBuildWatchdogDevStr(const virDomainDef *def,
>virDomainWatchdogDefPtr dev,
>virQEMUCapsPtr qemuCaps);
>  
> +int qemuBuildInputDevStr(char **devstr,
> + const virDomainDef *def,
> + virDomainInputDefPtr input,
> + virQEMUCapsPtr qemuCaps)
> +ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> +ATTRIBUTE_NONNULL(4);

Not sure it matters if qemuCaps is NULL or not since virQEMUCapsGet
checks anyway.

John

> +
>  #endif /* __QEMU_COMMAND_H__*/
> 

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

Re: [libvirt] [PATCH 09/12] split out qemuAssignDeviceInputAlias

[John Ferlan]

On 10/17/2017 11:04 AM, Ján Tomko wrote:
> Move assignment of input device alias into a separate function,
> for reuse on hotplug.
> ---
>  src/qemu/qemu_alias.c | 24 +++-
>  src/qemu/qemu_alias.h |  3 +++
>  2 files changed, 26 insertions(+), 1 deletion(-)
> 

[...]

> diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
> index 652ffea0c..35424829f 100644
> --- a/src/qemu/qemu_alias.h
> +++ b/src/qemu/qemu_alias.h
> @@ -66,6 +66,9 @@ int qemuAssignDeviceShmemAlias(virDomainDefPtr def,
> int idx);
>  
>  int qemuAssignDeviceWatchdogAlias(virDomainWatchdogDefPtr watchdog);

Add an empty line here for readability.

John

> +int qemuAssignDeviceInputAlias(virDomainDefPtr def,
> +   virDomainInputDefPtr input,
> +   int idx);
>  
>  int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps);
>  
> 

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

Re: [libvirt] [PATCH 05/12] Split out qemuDomainEnsureVirtioAddress

[John Ferlan]

On 10/17/2017 11:04 AM, Ján Tomko wrote:
> Split out the common code responsible for reserving/assigning
> PCI/CCW addresses for virtio disks into a helper function
> for reuse by other virtio devices.
> ---
>  src/qemu/qemu_domain_address.c | 45 
> ++
>  src/qemu/qemu_domain_address.h |  4 
>  src/qemu/qemu_hotplug.c| 29 +++
>  3 files changed, 52 insertions(+), 26 deletions(-)
> 

Not an issue - just a note from review...

> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index c4485d4ab..ebe9eb861 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -2904,3 +2904,48 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm,
>  VIR_WARN("Unable to release USB address on %s",
>   NULLSTR(devstr));
>  }
> +
> +
> +int
> +qemuDomainEnsureVirtioAddress(bool *releaseAddr,
> +  virDomainObjPtr vm,
> +  virDomainDeviceDefPtr dev,
> +  const char *devicename)
> +{
> +virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev);
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +virDomainCCWAddressSetPtr ccwaddrs = NULL;
> +virQEMUDriverPtr driver = priv->driver;
> +int ret = -1;
> +
> +if (!info->type) {
> +if (qemuDomainIsS390CCW(vm->def) &&
> +virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW))
> +info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
> +else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390))
> +info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390;
> +} else {
> +if (!qemuCheckCCWS390AddressSupport(vm->def, *info, priv->qemuCaps,
> +devicename))
> +return -1;
> +}
> +
> +if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
> +if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def)))
> +goto cleanup;
> +if (virDomainCCWAddressAssign(info, ccwaddrs,
> +  !info->addr.ccw.assigned) < 0)
> +goto cleanup;
> +} else if (!info->type ||
> +   info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
> +if (qemuDomainEnsurePCIAddress(vm, dev, driver) < 0)
> +goto cleanup;
> +*releaseAddr = true;

This is only setting *releaseAddr in this instance; whereas, the
previous code would also set it for info->type CCW [1]

Still looking at how @releaseaddr is used, we see that it's used to call
qemuDomainReleaseDeviceAddress which only cares about PCI and USB, so I
suppose this is fine - just different.

> +}
> +
> +ret = 0;
> +
> + cleanup:
> +virDomainCCWAddressSetFree(ccwaddrs);
> +return ret;
> +}
> diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h
> index b5644fa9c..e951a4c88 100644
> --- a/src/qemu/qemu_domain_address.h
> +++ b/src/qemu/qemu_domain_address.h
> @@ -59,6 +59,10 @@ qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def)
>  int qemuDomainAssignMemoryDeviceSlot(virDomainDefPtr def,
>   virDomainMemoryDefPtr mem);
>  
> +int qemuDomainEnsureVirtioAddress(bool *releaseAddr,
> +  virDomainObjPtr vm,
> +  virDomainDeviceDefPtr dev,
> +  const char *devicename);
>  
>  # define __QEMU_DOMAIN_ADDRESS_H__
>  
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 1e7931a84..177c8a01d 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -363,7 +363,6 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
>  bool driveAdded = false;
>  bool secobjAdded = false;
>  bool encobjAdded = false;
> -virDomainCCWAddressSetPtr ccwaddrs = NULL;
>  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>  const char *src = virDomainDiskGetSource(disk);
>  virJSONValuePtr secobjProps = NULL;
> @@ -372,33 +371,12 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
>  qemuDomainSecretInfoPtr secinfo;
>  qemuDomainSecretInfoPtr encinfo;
>  
> -if (!disk->info.type) {
> -if (qemuDomainIsS390CCW(vm->def) &&
> -virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW))
> -disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
> -else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390))
> -disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390;
> -} else {
> -if (!qemuCheckCCWS390AddressSupport(vm->def, disk->info, 
> priv->qemuCaps,
> -disk->dst))
> -goto cleanup;
> -}
> -
>  if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0)
>  goto cleanup;
>  
> -if (disk->info.type == VIR_DOMA

Re: [libvirt] [PATCH 04/12] Move qemuCheckCCWS390AddressSupport to qemu_domain

[John Ferlan]

On 10/17/2017 11:04 AM, Ján Tomko wrote:
> Let it be reused in qemu_domain_address.

Alternatively you could have added "#include qemu_command.h" to
qemu_domain_address.c, right?

IDC about moving, but if you do

> ---
>  src/qemu/qemu_command.c | 40 
>  src/qemu/qemu_command.h |  5 -
>  src/qemu/qemu_domain.c  | 40 
>  src/qemu/qemu_domain.h  |  5 +
>  4 files changed, 45 insertions(+), 45 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index f68b82d08..138bbdf1a 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1263,46 +1263,6 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk)
>  }
>  
>  
> -/* Check whether the device address is using either 'ccw' or default s390
> - * address format and whether that's "legal" for the current qemu and/or
> - * guest os.machine type. This is the corollary to the code which doesn't
> - * find the address type set using an emulator that supports either 'ccw'
> - * or s390 and sets the address type based on the capabilities.
> - *
> - * If the address is using 'ccw' or s390 and it's not supported, generate
> - * an error and return false; otherwise, return true.
> - */
> -bool
> -qemuCheckCCWS390AddressSupport(const virDomainDef *def,
> -   virDomainDeviceInfo info,
> -   virQEMUCapsPtr qemuCaps,
> -   const char *devicename)
> -{
> -if (info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
> -if (!qemuDomainIsS390CCW(def)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -   _("cannot use CCW address type for device "
> - "'%s' using machine type '%s'"),
> -   devicename, def->os.machine);
> -return false;
> -} else if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_CCW)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -   _("CCW address type is not supported by "
> - "this QEMU"));
> -return false;
> -}
> -} else if (info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) {
> -if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -   _("virtio S390 address type is not supported by "
> - "this QEMU"));
> -return false;
> -}
> -}
> -return true;
> -}
> -
> -
>  /* QEMU 1.2 and later have a binary flag -enable-fips that must be
>   * used for VNC auth to obey FIPS settings; but the flag only
>   * exists on Linux, and with no way to probe for it via QMP.  Our
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 94e4592cc..1254ad4df 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -189,11 +189,6 @@ int qemuCheckDiskConfig(virDomainDiskDefPtr disk);
>  bool
>  qemuCheckFips(void);
>  
> -bool qemuCheckCCWS390AddressSupport(const virDomainDef *def,
> -virDomainDeviceInfo info,
> -virQEMUCapsPtr qemuCaps,
> -const char *devicename);
> -
>  virJSONValuePtr qemuBuildHotpluggableCPUProps(const virDomainVcpuDef *vcpu)
>  ATTRIBUTE_NONNULL(1);
>  
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 05e8b96aa..f6eb4277a 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -10122,3 +10122,43 @@ qemuDomainGetMachineName(virDomainObjPtr vm)
>  
>  return ret;
>  }
> +
> +
> +/* Check whether the device address is using either 'ccw' or default s390
> + * address format and whether that's "legal" for the current qemu and/or
> + * guest os.machine type. This is the corollary to the code which doesn't
> + * find the address type set using an emulator that supports either 'ccw'
> + * or s390 and sets the address type based on the capabilities.
> + *
> + * If the address is using 'ccw' or s390 and it's not supported, generate
> + * an error and return false; otherwise, return true.
> + */
> +bool
> +qemuCheckCCWS390AddressSupport(const virDomainDef *def,
> +   virDomainDeviceInfo info,
> +   virQEMUCapsPtr qemuCaps,
> +   const char *devicename)


... This should be renamed to qemuDomainCheckCCWS390AddressSupport since
it's in qemu_domain.c

> +{
> +if (info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
> +if (!qemuDomainIsS390CCW(def)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("cannot use CCW address type for device "
> + "'%s' using machine type '%s'"),
> +   devicename, def->os.machine);
> +return f

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

[Pavel Hrdina]

On Thu, Oct 19, 2017 at 01:40:39PM +0200, Andrea Bolognani wrote:
> On Wed, 2017-10-18 at 19:11 +0200, Andrea Bolognani wrote:
> > 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
> 
> FYI: I came up with a cuter name, licito[1], and I will use that when
> pushing unless someone feel strongly otherwise .

I liked manage better that lcitool, but lcitool makes sense as well.
However, I don't like licito, it might sound cool but IMHO it's not
useful at all.

Pavel


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

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

[Andrea Bolognani]

On Wed, 2017-10-18 at 19:11 +0200, Andrea Bolognani wrote:
> 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

FYI: I came up with a cuter name, licito[1], and I will use that when
pushing unless someone feel strongly otherwise .


[1] As in LIbvirt CI TOol
-- 
Andrea Bolognani / Red Hat / Virtualization

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

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

[Michal Privoznik]

On 10/19/2017 12:10 PM, Marc Haber wrote:
> On Thu, Oct 19, 2017 at 11:52:34AM +0200, Michal Privoznik wrote:
>> On 10/18/2017 04:48 PM, Marc Haber wrote:
>>> 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?
>>
>> This is weird. However, I'm unable to reproduce with the latest libvirt.
>> What's your version? Have you tried the latest one?
> 
> I have Debian unstable, libvirt 3.8.0.

Aha! the thing is, you're using netcf backend while I'm using the udev
one. This error message comes from netcf. It's a netcf's bug. CCing
Laine who should know more.

Michal

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

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

[Pavel Hrdina]

On Wed, Oct 18, 2017 at 07:11:50PM +0200, Andrea Bolognani wrote:
> 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

s/GRUB/GRUB2/

> +  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

s/GRUB/GRUB2/

> +  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


signature.asc
Description: PGP signature
--
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/19/2017 02:54 AM, Erik Skultety wrote:
> On Wed, Oct 18, 2017 at 05:13:41PM -0400, John Ferlan wrote:
>>
>>
>> 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 = &syscap->hardware;
>>>  virNodeDevCapSystemFirmwarePtr firmware = &syscap->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 ;-)
>>
> 
> Wow, you really must have been a sharpshooter in the previous life ;), thanks 
> a
> lot.
> 
> Erik
> 

Sadly they seem to work better when reviewing code... They often fail me
on my own code ;-)

John

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

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

[Pavel Hrdina]

On Wed, Oct 18, 2017 at 07:11:49PM +0200, Andrea Bolognani wrote:
> The lcitool script can now be used to install most known guests
> without requiring user interaction.
> 
> Signed-off-by: Andrea Bolognani 
> ---

[...]

> diff --git a/guests/kickstart.cfg b/guests/kickstart.cfg
> new file mode 100644
> index 000..603c0ae
> --- /dev/null
> +++ b/guests/kickstart.cfg
> @@ -0,0 +1,60 @@
> +# Installer configuration
> +#
> +# Perform a text based installation followed by a reboot, and disable
> +# the first boot assistant
> +text
> +install
> +reboot
> +firstboot --disable
> +
> +
> +# Environment configuration
> +#
> +# Locale, keyboard and timezone. All these will be configured again
> +# later with Ansible, but they're required information so we must
> +# provide them
> +lang en_US.UTF-8
> +keyboard us
> +timezone --utc UTC
> +
> +
> +# User creation
> +#
> +# We don't create any user except for root. We can use a very insecure
> +# root password here because it will be replaced with a used-defined one
> +# with Ansible immediately after installation
> +authconfig --enableshadow --passalgo=sha512
> +rootpw --plaintext root
> +
> +
> +# Partition disk
> +#
> +# Erase everything and set up a 2 GiB swap partition, then assign all

s/2 GiB/256 MiB/

> +# remaining space to the root partition
> +ignoredisk --only-use=vda
> +zerombr
> +clearpart --none
> +part / --fstype=ext4 --size=2048 --grow
> +part swap --fstype=swap --size=256
> +
> +
> +# Install bootloader
> +#
> +# The bootloader will be installed in the MBR
> +bootloader --location=mbr --timeout=1
> +
> +
> +# Configure networking
> +#
> +# The only network interface available to the guest will come up
> +# at boot using IPv4-only DHCP
> +network --bootproto=dhcp --noipv6 --activate --onboot=yes
> +
> +
> +# Software installation
> +#
> +# Only install the very base packages: everything else will be
> +# installed later using Ansible
> +%packages
> +@core
> +%end

[...]

> diff --git a/guests/preseed.cfg b/guests/preseed.cfg
> new file mode 100644
> index 000..00fd20d
> --- /dev/null
> +++ b/guests/preseed.cfg
> @@ -0,0 +1,85 @@
> +# Installer configuration
> +#
> +# Perform an automated installation where only critical questions
> +# are asked interactively
> +d-i auto-install/enable boolean true
> +d-i debconf/priority string critical
> +d-i finish-install/reboot_in_progress note
> +
> +
> +# Environment configuration
> +#
> +# Locale, keyboard and timezone. All these will be configured again
> +# later with Ansible, but they're required information so we must
> +# provide them
> +d-i debian-installer/locale string en_US.UTF-8
> +d-i keyboard-configuration/xkb-keymap select us
> +d-i time/zone string UTC
> +d-i clock-setup/utc boolean true
> +d-i clock-setup/ntp boolean true
> +
> +
> +# User creation
> +#
> +# We don't create any user except for root. We can use a very insecure
> +# root password here because it will be replaced with a used-defined one
> +# with Ansible immediately after installation
> +d-i passwd/make-user boolean false
> +d-i passwd/root-login boolean true
> +d-i passwd/root-password password root
> +d-i passwd/root-password-again password root
> +d-i user-setup/allow-password-weak boolean true
> +
> +
> +# Partition disk
> +#
> +# Erase everything and set up a 2 GiB swap partition, then assign all

s/2 GiB/256 MiB/

> +# remaining space to the root partition
> +d-i partman-auto/disk string /dev/vda
> +d-i partman-auto/method string regular
> +d-i partman-auto/expert_recipe string \
> +custom :: \
> +2048 2048 -1 ext4 \
> +$primary{ } $bootable{ } \
> +method{ format } format{ } \
> +use_filesystem{ } filesystem{ ext4 } \
> +mountpoint{ / } \
> +. \
> +256 256 256 linux-swap \
> +$primary{ } \
> +method{ swap } format{ } \
> +.
> +d-i partman-partitioning/confirm_write_new_label boolean true
> +d-i partman/choose_partition select finish
> +d-i partman/confirm boolean true
> +d-i partman/confirm_nooverwrite boolean true
> +
> +
> +# Install bootloader
> +#
> +# The bootloader will be installed in the MBR
> +d-i grub-installer/skip boolean false
> +d-i grub-installer/bootdev string /dev/vda
> +d-i grub-installer/only_debian boolean true
> +
> +
> +# Configure networking
> +#
> +# The only network interface available to the guest will come up
> +# at boot using DHCP
> +d-i netcfg/enable boolean true
> +d-i netcfg/choose_interface select auto
> +d-i netcfg/get_hostname string localhost
> +d-i netcfg/get_domain string localdomain
> +
> +
> +# Software installation
> +#
> +# Only install the very base packages: everything else will be
> +# installed later using Ansible. We need to install openssh-server
> +# and configure it to permit root login now, though, otherwise we
> +# won't be able to access the machine for Ansible use later on
> +tasksel tasksel/first multiselect standard
> +d-i pkgsel/upgrade select none
> +d-i p

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

[Marc Haber]

On Thu, Oct 19, 2017 at 11:52:34AM +0200, Michal Privoznik wrote:
> On 10/18/2017 04:48 PM, Marc Haber wrote:
> > 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?
> 
> This is weird. However, I'm unable to reproduce with the latest libvirt.
> What's your version? Have you tried the latest one?

I have Debian unstable, libvirt 3.8.0.

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

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

[Pavel Hrdina]

On Wed, Oct 18, 2017 at 04:39:10PM -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(-)
> > 
> 
> Reviewed-by: John Ferlan 
> (series)
> 
> As long as you take care of the couple things I noted.

Thanks for review, I've fixed the issues and pushed it.

Pavel


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

Re: [libvirt] [PATCH v1 04/14] qemu: Move device alias assignment to separate functions

[Martin Kletzander]

On Thu, Oct 19, 2017 at 10:10:59AM +0200, Michal Privoznik wrote:

Let's move all the virAsprintf()-s into separate functions for
better structure of the code. Later, when somebody wants to
generate a device alias, all they need is to expose the function.



I'm just waiting for these functions to totally fail when we introduce a hotplug
of such device.  There could be a universal wrapper for these, but that's not
the scope of this patchset.

ACK


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

Re: [libvirt] [PATCH v1 07/14] conf: Parse user supplied aliases

[Martin Kletzander]

On Thu, Oct 19, 2017 at 10:11:02AM +0200, Michal Privoznik wrote:

If driver that is calling the parse supports user supplied
aliases, they can be parsed even for inactive XMLs. However, to
avoid any clashes with aliases that libvirt generates, the user
ones have to have "ua-" prefix.

Signed-off-by: Michal Privoznik 
---
src/conf/domain_conf.c | 13 +++--
src/conf/domain_conf.h |  1 +
2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f4de4e288..f1386c116 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6398,6 +6398,10 @@ virDomainDeviceAddressParseXML(xmlNodePtr address,
}


+#define USER_ALIAS_PREFIX "ua-"
+#define USER_ALIAS_CHARS \
+"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-"
+
/* Parse the XML definition for a device address
 * @param node XML nodeset to parse for device address definition
 */
@@ -6424,7 +6428,6 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt 
ATTRIBUTE_UNUSED,
while (cur != NULL) {
if (cur->type == XML_ELEMENT_NODE) {
if (alias == NULL &&
-!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&
virXMLNodeNameEqual(cur, "alias")) {
alias = cur;
} else if (address == NULL &&
@@ -6446,8 +6449,14 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt 
ATTRIBUTE_UNUSED,
cur = cur->next;
}

-if (alias)
+if (alias) {
info->alias = virXMLPropString(alias, "name");
+if (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE &&
+(!STRPREFIX(info->alias, USER_ALIAS_PREFIX) ||
+ !(xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_USER_ALIAS) ||
+ strspn(info->alias, USER_ALIAS_CHARS) < strlen(info->alias)))
+VIR_FREE(info->alias);


Assigning the pointer just so you can clear it after some hard to read condition
seems just plain wrong.  Only assign it if the condition is false.


+}

if (master) {
info->mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 1e007346f..5ff85057f 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2508,6 +2508,7 @@ typedef enum {
VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN = (1 << 2),
VIR_DOMAIN_DEF_FEATURE_NAME_SLASH = (1 << 3),
VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS = (1 << 4),
+VIR_DOMAIN_DEF_FEATURE_USER_ALIAS = (1 << 5),


Can you elaborate on why is this not the default and whydo we need to have a
flag for it?  If it's just so that individual drivers can decide this, adding
this to the commit message is enough and ACK with this and the above fixed.

Also ACK to previous patches.


} virDomainDefFeatures;


--
2.13.6

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


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

Re: [libvirt] [PATCH v1 01/14] conf: Fix virDomainDeviceGetInfo const correctness

[Martin Kletzander]

On Thu, Oct 19, 2017 at 10:10:56AM +0200, Michal Privoznik wrote:

This function is not changing passed domain definition.
Therefore, mark the argument as 'const'.



Is there any other reason for this then just your new code using const?
I don't see any pointer that's passed through your later code that would
be const nowadays, so why would we play the const-correctness game here
when it doesn't make much sense in C when the pointer is to a struct
since the const doesn't propagate?

If s/const virDomainDeviceDef/virDomainDeviceDef/ on your later patch
makes this one unnecessary, then NACK to this.  If there is a reason,
however, feel free to keep this in.


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

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

[Michal Privoznik]

On 10/18/2017 04:48 PM, Marc Haber wrote:
> 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?

This is weird. However, I'm unable to reproduce with the latest libvirt.
What's your version? Have you tried the latest one?

Michal

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

Re: [libvirt] How can I tell if I'm using qemu-kvm or qemu-kvm-ev from libvirt Python binding?

[Pavel Hrdina]

On Thu, Oct 19, 2017 at 10:22:59AM +0200, Martin Kletzander wrote:
> On Thu, Oct 19, 2017 at 10:34:21AM +0300, Yaniv Kaul wrote:
> > I'm trying to (optionally) use iothreads, which only exist on qemu-kvm-ev.
> > I was hoping to see it in the domcapabilities, but I don't see it there.
> > How do I know if it's supported or not?
> > 
> 
> Well, you can get the emulator binary from /domainCapabilities/path, but 
> that's
> not what you should be using for any support-related decisions.
> 
> I guess there is no easy way to do that, feel free to create a BZ that we add 
> it
> to domcapabilities (or just capabilities maybe) and assign it to me so that we
> don't lose track of it.  I'll post a patch for it then, it will be trivial, 
> the
> only thing we need is design decision about how to name it and where to put 
> it.

I don't think that this is something we would like to have in upstream
code.  The differentiation between qemu-kvm and qemu-kvm-ev is
dowsntream thing.  In addition, there is no qemu-kvm in upstream, there
is only QEMU.

What we can add into capabilities is information whether the QEMU binary
supports iothreads or not, but please, don't introduce anything that's
downstream only.

Pavel


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

[libvirt] [PATCH] bhyve: Fix build

[Jiri Denemark]

Commit v3.8.0-95-gfd885a06a dropped nmodels parameter from several APIs
in src/cpu/cpu.h, but failed to update all callers.

Signed-off-by: Jiri Denemark 
---

Pushed under the build-breaker rule.

 src/bhyve/bhyve_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index c096b5562..5c432b25e 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -1433,7 +1433,7 @@ bhyveConnectBaselineCPU(virConnectPtr conn,
 if (!(cpus = virCPUDefListParse(xmlCPUs, ncpus, VIR_CPU_TYPE_HOST)))
 goto cleanup;
 
-if (!(cpu = cpuBaseline(cpus, ncpus, NULL, 0,
+if (!(cpu = cpuBaseline(cpus, ncpus, NULL,
 !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE
 goto cleanup;
 
-- 
2.14.2

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

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

[Daniel P. Berrange]

On Thu, Oct 19, 2017 at 11:10:21AM +0200, Martin Kletzander wrote:
> On Wed, Oct 18, 2017 at 07:11:45PM +0200, Andrea Bolognani wrote:
> > 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
> 
> Why wap in such a virtual machine?

Even if you don't expect to use / need it, it is generally a good idea to
give a few 100 MB of swap. This gives the kernel MM greater flexibility to
re-arrange memory when it starts to get low on free RAM. Without it, you
can hit OOM despite technically having enough RAM free.

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