date:20160519

On Thu, May 19, 2016 at 05:14:33PM -0400, Laine Stump wrote:
> On 05/19/2016 01:23 PM, Ján Tomko wrote:
> > Yes, qemu_hotplug.c has a few of those places using == 
> > DEVICE_ADDRESS_TYPE_PCI
> > untouched by this series.
> 
> Since they happen after parse is finished, it should be safe.
> 
> And anyway, looking at those uses, I think what most of them are doing 
> (calling virDomainPCIAddressEnsureAddr()) is 100% unnecessary, since 
> it's now already done when addresses are assigned in 
> qemuDomainDefAssignAddresses(), which has already been called.

Actually, virDomainPCIAddressEnsureAddr is the place where the address
gets assigned. But only when address == NONE or PCI, which is OK because
virDomainPCIAddressEnsureAddr calls PCIAddressIsPresent to decide
whether it needs to allocate a new one.

qemuDomainDefAssignAddresses is only called after parsing the whole
domain, not just one device.

> (Back in 
> Jan 2010 when the calls to qemuDomainPCIAddressEnsureAddr() were added 
> (commit d8acc4), this was not the case - they were needed in order for 
> the new devices to get an address assigned, but a lot has changed since 
> then - even before Cole put in the postparse callback address assignment 
> stuff and called it when attaching a device, we had already been 
> assigning addresses during the higher level qemuDomainAttachDeviceConfig 
> for a long time (since commit f5dd58a in June 2012;

qemuDomainAttachDeviceConfig is for changing the persistent definition.
Hotplug in qemuDomainAttachDeviceLive does not call any other address
assignment function.

Jan

> long enough that the 
> calls to qemuDomainCCWAddressAssign() that are also sprinkled throughout 
> qemu_hotplug.c in the same vicinity as the calls to 
> qemuDomainPCIAddressEnsureAddr() weren't needed *even at the time they 
> were added!* (commit f94646, March 2013). This was purely cargo-cult 
> coding, caused by commit f5dd58a failing to remove the calls to 
> ...EnsureAddr(). The interesting bit is that both of these commits were 
> put in in support of s390 virtio devices.).
> 
> (Well, *that* was a nice trip down git "memory lane" (aka "blame")
> 
> So, the result is that most of the code in qemu_hotplug that requires 
> checking for address type is unneeded, and I'm going to send a patch to 
> remove it.
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

Re: [libvirt] [PATCH v3 1/1] perf: add support to perf event for MBM

2016-05-19 Thread Ren, Qiaowei


> -Original Message-
> From: Peter Krempa [mailto:pkre...@redhat.com]
> Sent: Thursday, May 19, 2016 9:46 PM
> To: Ren, Qiaowei 
> Cc: libvir-list@redhat.com
> Subject: Re: [libvirt] [PATCH v3 1/1] perf: add support to perf event for MBM
> 
> On Fri, May 13, 2016 at 12:26:07 +0800, Qiaowei Ren wrote:
> > Some Intel processor families (e.g. the Intel Xeon processor E5 v3
> > family) introduced some RDT (Resource Director Technology) features to
> > monitor or control shared resource. Among these features, MBM (Memory
> > Bandwidth Monitoring), which is build on the CMT (Cache Monitoring
> > Technology) infrastructure, provides OS/VMM a way to monitor bandwidth
> > from one level of cache to another.
> >
> > With current perf framework, this patch adds support to perf event for
> > MBM.
> >
> > Signed-off-by: Qiaowei Ren 
> > ---
> >  include/libvirt/libvirt-domain.h | 26 -
> >  src/libvirt-domain.c | 12 
> >  src/qemu/qemu_driver.c   | 41 +++---
> >  src/util/virperf.c   | 63 
> > 
> >  src/util/virperf.h   |  2 ++
> >  5 files changed, 108 insertions(+), 36 deletions(-)
> 
> [...]
> 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index
> > c4c4968..670f620 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> 
> [...]
> 
> > @@ -19494,24 +19496,38 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr
> > driver,
> >
> >  #undef QEMU_ADD_COUNT_PARAM
> >
> > +#define QEMU_ADD_PERF_PARAM_ULL(record, maxparams, name, value) \
> do
> > +{ \
> > +char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> > +snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> > + "perf.%s", name); \
> > +if (virTypedParamsAddULLong(&(record)->params, \
> > +&(record)->nparams, \
> > +maxparams, \
> > +param_name, \
> > +value) < 0) \
> > +goto cleanup; \
> > +} while (0)
> 
> This macro is used once so it's not really necessary.
> 
> > +
> >  static int
> > -qemuDomainGetStatsPerfCmt(virPerfPtr perf,
> > +qemuDomainGetStatsPerfRdt(virPerfPtr perf,
> > +  virPerfEventType type,
> >virDomainStatsRecordPtr record,
> >int *maxparams)  {
> > -uint64_t cache = 0;
> > +uint64_t value = 0;
> >
> > -if (virPerfReadEvent(perf, VIR_PERF_EVENT_CMT, &cache) < 0)
> > +if (virPerfReadEvent(perf, type, &value) < 0)
> >  return -1;
> >
> > -if (virTypedParamsAddULLong(&record->params,
> > -&record->nparams,
> > -maxparams,
> > -"perf.cache",
> > -cache) < 0)
> > -return -1;
> > +QEMU_ADD_PERF_PARAM_ULL(record, maxparams,
> > +virPerfEventTypeToString(type),
> > +value);
> 
> Otherwise looks good. Thanks for tweaking the documentation.
> 
> I'll push this with the macro dropped in a while.
> 

Peter, thanks very much!

Thanks,
Qiaowei

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

Re: [libvirt] [PATCH 4/4] conf: permit auto-assignment of controller indexes


On 05/16/2016 06:26 PM, Cole Robinson wrote:

On 05/15/2016 02:57 PM, Laine Stump wrote:

On 05/15/2016 02:38 PM, Cole Robinson wrote:

On 05/15/2016 02:30 PM, Laine Stump wrote:

On 05/14/2016 10:56 AM, Cole Robinson wrote:

I agree with the goal of the patch, but I think all the index assignment code
should be moved to somewhere in the PostParse call path. The fact that the
controller ParseXML now needs to act on the entire domain def is a giveaway
that it's in the wrong place.

I originally did it that way, but there was some problem with it, either
actual or imagined/potential. I *think* possibly the problem was that
auto-added controllers (which is done in the driver-specific postparse, called
prior to the common postparse) are added when an existing controller of the
desired index can't be found, but if an index was added in postparse, I would
want it to be done in the common postparse (since it is a requirement for
*all* drivers);

Hmm. Most implicit controllers are added in common code actually, however
there are some added in qemu code it seems, for PCI, which is probably what
you were referring to.

There are several controllers/devices added which are not just hypervisor
specific, but specific to particular machinetypes/arches within that
hypervisor. In particular, qemuDomainDefAddDefaultDevices.


   In that case we may need to do two passes of index
assignment, or when adding a PCI controller outside common code we just
informally require the hv driver to assign the index themselves.

It's not that the new controller needs an index assigned. It's that the
controller is "maybe added" depending on whether of not there is already a
controller of the requested type at a particular index (usually 0). For
example, when we add a default USB controller in
qemuDomainDefAddDefaultDevices().


Yeah I forgot about all those :/ addPCIe, etc. Note, those bits are already
called as part of virDomainDefPostParse, via the driver specific
qemuDomainDefPostParse


Exactly - the driver-specific post-parse is called before the common 
post-parse, and the controller indexes need to be known by the time 
those controllers are added, because they are added at *specific 
indexes* depending on whether or not there is already a controller of 
that exact type/index. The auto-indexing is the same for all drivers 
though, so if it goes in a post-parse callback it should go in the 
common postparse, which happens too late.


(note that we don't need to worry about auto-indexing controllers added 
by post-parse functions - they are already using the same method to give 
each controller an index immediately).




As for doing two passes, where would the first pass be run? I don't want it to
be done in the driver-specific postparse because then it would need to be
called separately for every driver, which is prone to people not doing it. And
the common postparse is too late to do any good. The only place we're left
with, other than in the controller parser itself, is  the top-level domain
parser function prior to calling the driver-specific postparse.


So the current flow is

virDomainDefPostParse
   1) qemuDomainDefPostParse
   2) per device qemuDomainDeviceDefPostParse
   3) per device virDomainDeviceDefPostParseInternal
   4) virDomainDefAddImplicitDevices

5) qemuDomainAssignAddresses

For qemu, it looks like controllers can be added in step 1, 4, and 5. At the
very least I suspect we need a pass after step #4 since ImplicitControllers
can be added. That may cover everything that your original approach covers,
since all those places aren't hitting the XML parser anyways and instead
building controllers manually AFAICT.


The only controllers that need this auto-index step are those added to 
the config by the user. None of the controllers added later need it 
(because they all determine their own index).




Also there is the case where only a single controller is parsed - the toplevel
domain parse is never called there in the first place (of course I'm skeptical
that is ever called for any controller - are any controllers hotpluggable?)



Unfortunately these types of ordering problems are basically unavoidable in
certain cases... there's no one size fits all ordering for validation, setting
defaults, adding default devices, and assigning addresses.

Yeah, there's no way to deal with this in the current postparse callback
framework, and the only way to solve all possible ordering problems in that
way would be, it seems, to call all of the callbacks repeatedly in a loop
until it went through an entire cycle without any change :-P


Right. I don't think we need to come up with a 100% future proof solution at
this point, just one that works for all cases we presently care about.



Yeah, that was just generalizing on the point that even the current set 
of callbacks is insufficient to solve this, and if we add more 
callbacks, we'll eventually end up with another problem that all *those* 
callbacks can't solve.


If you pu

Re: [libvirt] [PATCH 0/6] auto-assign addresses when is specified


On 05/19/2016 03:33 PM, Cole Robinson wrote:

My patches had an explicit PostParse check in generic code that would throw an
error if  was never correctly filled in by the
hypervisor, so 


That sounds like a good idea, and should be trivial to add to the tail 
end of your address assignment callback. I'll make a followup patch for it.





upsides:
- less magic
- I think it will make allocating address at hotplug time simpler as well


I hadn't thought about that yet -  more of the "90% of the coding effort going
to 0.005% of the users" :-P


Yeah it's certainly not a blocker for this series, but hotplug will be
relevant for aarch64 eventually


Actually it should "just work".

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

[libvirt] [PATCH 2/2] qemu: command: Error on accel2d

qemu doesn't have any accel2d support wired up. Explicitly error
if a user tries it out, or typos the accel3d option
---
 src/qemu/qemu_command.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 731f4c0..cca59a6 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4125,6 +4125,12 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
 
 virBufferAsprintf(&buf, "%s,id=%s", model, video->info.alias);
 
+if (video->accel && video->accel->accel2d == VIR_TRISTATE_SWITCH_ON) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("qemu does not support the accel2d setting"));
+goto error;
+}
+
 if (video->accel && video->accel->accel3d == VIR_TRISTATE_SWITCH_ON) {
 if (video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO ||
 !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL)) {
-- 
2.7.4

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

[libvirt] [PATCH 0/2] qemu: reject invalid video accel* settings

This series adds a couple checks for invalid video accel* settings,
incase users screw up their XML when trying to enable spice GL

Cole Robinson (2):
  qemu: command: Error if accel3d isn't supported for non-virtio
  qemu: command: Explicitly error if accel2d specified

 src/qemu/qemu_command.c | 28 ++--
 1 file changed, 18 insertions(+), 10 deletions(-)

-- 
2.7.4

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

[libvirt] [PATCH 1/2] qemu: command: Error on accel3d with non-virtio

We should be raising an error if accel3d is present for any
non-virtio video as well, incase someone hand edits incorrect XML
---
 src/qemu/qemu_command.c | 24 +---
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4a2fa1d..731f4c0 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4125,18 +4125,20 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
 
 virBufferAsprintf(&buf, "%s,id=%s", model, video->info.alias);
 
-if (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO) {
-if (video->accel && video->accel->accel3d == VIR_TRISTATE_SWITCH_ON) {
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   "%s", _("virtio-gpu 3d acceleration is not 
supported"));
-goto error;
-}
-
-virBufferAsprintf(&buf, ",virgl=%s",
-  
virTristateSwitchTypeToString(video->accel->accel3d));
+if (video->accel && video->accel->accel3d == VIR_TRISTATE_SWITCH_ON) {
+if (video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO ||
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("%s 3d acceleration is not supported"),
+   virDomainVideoTypeToString(video->type));
+goto error;
 }
-} else if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL) {
+
+virBufferAsprintf(&buf, ",virgl=%s",
+  
virTristateSwitchTypeToString(video->accel->accel3d));
+}
+
+if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL) {
 if (video->vram > (UINT_MAX / 1024)) {
 virReportError(VIR_ERR_OVERFLOW,
_("value for 'vram' must be less than '%u'"),
-- 
2.7.4

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

Re: [libvirt] [PATCH v4 08/14] qemu_command: refactor spice channel code

On 05/19/2016 07:35 AM, Pavel Hrdina wrote:
> This prepares the code for other listen types.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_command.c | 57 
> ++---
>  1 file changed, 26 insertions(+), 31 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ee43e21..e401ac5 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7500,10 +7500,11 @@ 
> qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>  {
>  virBuffer opt = VIR_BUFFER_INITIALIZER;
>  virDomainGraphicsListenDefPtr glisten = NULL;
> -int defaultMode = graphics->data.spice.defaultMode;
>  int port = graphics->data.spice.port;
>  int tlsPort = graphics->data.spice.tlsPort;
>  size_t i;
> +bool hasSecure = false;
> +bool hasInsecure = false;
>  
>  if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE)) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -7513,8 +7514,10 @@ 
> qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>  
>  glisten = virDomainGraphicsGetListen(graphics, 0);
>  
> -if (port > 0)
> +if (port > 0) {
>  virBufferAsprintf(&opt, "port=%u,", port);
> +hasInsecure = true;
> +}
>  
>  if (tlsPort > 0) {
>  if (!cfg->spiceTLS) {
> @@ -7524,6 +7527,7 @@ 
> qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>  goto error;
>  }
>  virBufferAsprintf(&opt, "tls-port=%u,", tlsPort);
> +hasSecure = true;
>  }
>  
>  if (port > 0 || tlsPort > 0) {
> @@ -7561,17 +7565,30 @@ 
> qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>  !cfg->spicePassword)
>  virBufferAddLit(&opt, "disable-ticketing,");
>  
> -if (tlsPort > 0)
> +if (hasSecure)
>  virBufferAsprintf(&opt, "x509-dir=%s,", cfg->spiceTLSx509certdir);
>  
> -switch (defaultMode) {
> +switch (graphics->data.spice.defaultMode) {
>  case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE:
> +if (!hasSecure) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("spice defaultMode secure requested in XML "
> + "configuration, but TLS is not available"));
> +goto error;
> +}
>  virBufferAddLit(&opt, "tls-channel=default,");
>  break;
>  case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE:
> +if (!hasInsecure) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("spice defaultMode insecure requested in XML "
> + "configuration, but plaintext is not 
> available"));
> +goto error;
> +}
>  virBufferAddLit(&opt, "plaintext-channel=default,");
>  break;
>  case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY:
> +case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_LAST:
>  /* nothing */
>  break;
>  }
> @@ -7579,10 +7596,10 @@ 
> qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>  for (i = 0; i < VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST; i++) {
>  switch (graphics->data.spice.channels[i]) {
>  case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE:
> -if (tlsPort <= 0) {
> +if (!hasSecure) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -   _("spice secure channels set in XML 
> configuration, "
> - "but TLS port is not provided"));
> +   _("spice secure channels set in XML "
> + "configuration, but TLS is not available"));
>  goto error;
>  }

I kinda prefer the original messages mentioning the lack of port as the
culprit. So maybe plaintext port and TLS port? If I saw the 'plaintext' error
as is I know I'd be confused and go digging into the code to figure out what
was triggering it.

ACK otherwise. IMO these first 8 patches are worth pushing and we work out any
additional stuff like the type='network' weirdness, and additional test cases,
on top of these cleanups

- Cole

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

Re: [libvirt] [PATCH v4 07/14] qemu_process: separate graphics socket and address generation

On 05/19/2016 07:35 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_process.c | 93 
> +++--
>  1 file changed, 59 insertions(+), 34 deletions(-)
> 

ACK

- Cole

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

Re: [libvirt] [PATCH v4 06/14] graphics: resolve address for listen type network in qemu_process

On 05/19/2016 04:50 PM, Cole Robinson wrote:
> On 05/19/2016 07:35 AM, Pavel Hrdina wrote:
>> Both VNC and SPICE requires the same code to resolve address for listen
>> type network.  Remove code duplication and create a new function that
>> will be used in qemuProcessSetupGraphics().
>>
>> Signed-off-by: Pavel Hrdina 
>> ---
>>  src/qemu/qemu_command.c | 103 
>> ++--
>>  src/qemu/qemu_process.c |  47 +-
>>  2 files changed, 57 insertions(+), 93 deletions(-)

>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index f4bf6c1..75c8e53 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -4388,6 +4388,32 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr 
>> driver,
>>  
>>  
>>  static int
>> +qemuProcessGraphicsSetupNetworkAddress(virDomainGraphicsListenDefPtr 
>> glisten,
>> +   const char *listenAddr)
>> +{
>> +int rc;
>> +
>> +if (!glisten->network) {
>> +if (VIR_STRDUP(glisten->address, listenAddr) < 0)
>> +return -1;
>> +return 0;
>> +}
>> +
> 
> This is a logic change. Previously we accept this XML
> 
> 
>   
> 
> 
> and silently treat that as using vnc_listen/spice_listen. Now we stick that
> address in the XML like
> 
> 
>   
> 
> 
> Which at least is more explicit, but it is a logic change. Just shows that the
> address type='network' stuff needs more test coverage at least. I think at
> some point we should reject bare type='network' if it doesn't have a @network
> attribute
> 
> If that change was intentional it should be an additive patch after this
> cleanup, with a test case
> 

Hmm okay I see that it must be intentional, because the qemu_command code now
depends on glisten->address. So ACK to this as long as you add a todo item to
add some test cases for this stuff, and to figure out the bare  weirdness

Thanks,
Cole

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

Re: [libvirt] [PATCH 7/7] Do not mask QEMU_CAPS_DEVICE in qemuBuildDriveStr



On 05/19/2016 02:59 PM, Ján Tomko wrote:
> For some disk types (XEN, SD), we want to emit the syntax

Removed XEN in patch 4 I thought...

> we used for disks before -device was available even
> if QEMU supports -device.
> 
> Use the qemuDiskBusNeedsDeviceArg helper to figure out
> whether to use the old or new syntax.
> ---
>  src/qemu/qemu_command.c | 12 +++-
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 

The rest seems fine - although connecting the dots between CAPS_DEVICE
and bus != BUS_SD wasn't as obvious...  It's the double negative of
qemuDiskBusNeedsDeviceArg to set deviceFlagMasked that caused me to pause.


ACK series with a few touchups.

John
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index a6cc0c9..55326c3 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1106,6 +1106,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
>  char *source = NULL;
>  int actualType = virStorageSourceGetActualType(disk->src);
>  qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> +bool emitDeviceSyntax = qemuDiskBusNeedsDeviceArg(disk->bus);
>  
>  if (idx < 0) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -1246,7 +1247,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
>  }
>  VIR_FREE(source);
>  
> -if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))
> +if (emitDeviceSyntax)
>  virBufferAddLit(&opt, "if=none");
>  else
>  virBufferAsprintf(&opt, "if=%s", bus);
> @@ -1263,7 +1264,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
>  }
>  }
>  
> -if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> +if (emitDeviceSyntax) {
>  virBufferAsprintf(&opt, ",id=%s%s", QEMU_DRIVE_HOST_PREFIX, 
> disk->info.alias);
>  } else {
>  if (busid == -1 && unitid == -1) {
> @@ -1916,7 +1917,6 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
>  char *optstr;
>  unsigned int bootindex = 0;
>  virDomainDiskDefPtr disk = def->disks[i];
> -bool deviceFlagMasked = true;
>  
>  /* PowerPC pseries based VMs do not support floppy device */
>  if ((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) &&
> @@ -1945,15 +1945,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
>  
>  virCommandAddArg(cmd, "-drive");
>  
> -if (!qemuDiskBusNeedsDeviceArg(disk->bus)) {
> -virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE);
> -deviceFlagMasked = true;
> -}
>  optstr = qemuBuildDriveStr(disk,
> emitBootindex ? false : !!bootindex,
> qemuCaps);
> -if (deviceFlagMasked)
> -virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE);
>  if (!optstr)
>  return -1;
>  virCommandAddArg(cmd, optstr);
> 

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

Re: [libvirt] [PATCH 5/7] Assume QEMU_CAPS_DEVICE in qemuBuildDiskDriveCommandLine



On 05/19/2016 02:59 PM, Ján Tomko wrote:
> We no longer need to handle -usbdevice and the withDeviceArg
> logic becomes clearer.
> ---
>  src/qemu/qemu_command.c | 30 ++
>  1 file changed, 6 insertions(+), 24 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index bb40c17..0bde505 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1905,23 +1905,7 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
>  unsigned int bootindex = 0;
>  virDomainDiskDefPtr disk = def->disks[i];
>  bool withDeviceArg = false;
> -bool deviceFlagMasked = false;
> -
> -/* Unless we have -device, then USB disks need special
> -   handling */
> -if ((disk->bus == VIR_DOMAIN_DISK_BUS_USB) &&
> -!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> -if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> -virCommandAddArg(cmd, "-usbdevice");
> -virCommandAddArgFormat(cmd, "disk:%s", disk->src->path);
> -} else {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("unsupported usb disk type for '%s'"),
> -   disk->src->path);
> -return -1;
> -}
> -continue;
> -}
> +bool deviceFlagMasked = true;

I know this goes away by patch 7, but there's nothing later on that sets
this to false - so it seems it should be kept as false for now...

John
>  
>  /* PowerPC pseries based VMs do not support floppy device */
>  if ((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) &&
> @@ -1955,13 +1939,11 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
> devices. Fortunately, those don't need
> static PCI addresses, so we don't really
> care that we can't use -device */
> -if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> -if (disk->bus != VIR_DOMAIN_DISK_BUS_SD) {
> -withDeviceArg = true;
> -} else {
> -virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE);
> -deviceFlagMasked = true;
> -}
> +if (disk->bus != VIR_DOMAIN_DISK_BUS_SD) {
> +withDeviceArg = true;
> +} else {
> +virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE);
> +deviceFlagMasked = true;
>  }
>  optstr = qemuBuildDriveStr(disk,
> emitBootindex ? false : !!bootindex,
> 

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

Re: [libvirt] [PATCH 2/7] qemu: process: Drop !QEMU_CAPS_DEVICE code



On 05/19/2016 02:59 PM, Ján Tomko wrote:
> From: Cole Robinson 
> 
> Nowadays we only support qemu 0.12.0+ which provides QEMU_CAPS_DEVICE,
> so this is all dead code.
> ---
>  src/qemu/qemu_process.c | 53 
> ++---
>  1 file changed, 24 insertions(+), 29 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 40c238b..b7c8f25 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c

[...]

> @@ -5981,11 +5978,9 @@ int qemuProcessAttach(virConnectPtr conn 
> ATTRIBUTE_UNUSED,
>   * we also need to populate the PCI address set cache for later
>   * use in hotplug
>   */
> -if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> -VIR_DEBUG("Assigning domain PCI addresses");
> -if ((qemuDomainAssignAddresses(vm->def, priv->qemuCaps, vm)) < 0)
> +VIR_DEBUG("Assigning domain PCI addresses");
> +if ((qemuDomainAssignAddresses(vm->def, priv->qemuCaps, vm)) < 0)
>  goto error;

There's 4 extra spaces on indention for goto error.

John

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

Re: [libvirt] [PATCH 0/6] auto-assign addresses when is specified


On 05/19/2016 01:23 PM, Ján Tomko wrote:

On Thu, May 19, 2016 at 01:15:28PM -0400, Cole Robinson wrote:

On 05/18/2016 03:23 PM, Laine Stump wrote:

This is an alternative to Cole's series that permits  to force assignment of a PCI address, which is
particularly useful on platforms that could connect the same device in
different ways (e.g. aarch64/virt).

Here is Cole's last iteration of the series:

   https://www.redhat.com/archives/libvir-list/2016-May/msg01088.html

I had expressed a dislike of the "auto_allocate" flag that his series
temporarily adds to the address (while simultaneously changing the
address type to NONE) and suggested just changing all the necessary
places to check for a valid PCI address instead of just checking the
address type. He replied that this wasn't as simple as it looked, so I
decided to try it; turns out he's right. But I still like this method
better because it's not playing tricks with the address type, or
adding a temporary private attribute to what should be pure config
data.

Your opinion may vary though :-)


I like this series more than counting XML elements and temporarily
changing the types.


Note that patch 5/6 incorporates the same test case that Cole used in
his penultimate patch, and I've added his patch to check the case of
aarch64 at the end as well.


ACK series, but it's missing formatdomain.html.in changes. You can grab the
check from my patch #2

I'm fine with this approach but I'll just list the downsides

- Less generalizable, for adding additional address types in the future, but
that's hypothetical at this point
- We don't raise an explicit error for drivers that don't support this type of
address allocation, like libxl. If it matters we could add a domain XML parse
feature to throw an explicit error though
- checking info->type == DEVICE_ADDRESS_TYPE_PCI is now a suspect pattern and
needs to be considered carefully in other parts of the code

upsides:
- less magic
- I think it will make allocating address at hotplug time simpler as well


Yes, qemu_hotplug.c has a few of those places using == DEVICE_ADDRESS_TYPE_PCI
untouched by this series.


Since they happen after parse is finished, it should be safe.

And anyway, looking at those uses, I think what most of them are doing 
(calling virDomainPCIAddressEnsureAddr()) is 100% unnecessary, since 
it's now already done when addresses are assigned in 
qemuDomainDefAssignAddresses(), which has already been called. (Back in 
Jan 2010 when the calls to qemuDomainPCIAddressEnsureAddr() were added 
(commit d8acc4), this was not the case - they were needed in order for 
the new devices to get an address assigned, but a lot has changed since 
then - even before Cole put in the postparse callback address assignment 
stuff and called it when attaching a device, we had already been 
assigning addresses during the higher level qemuDomainAttachDeviceConfig 
for a long time (since commit f5dd58a in June 2012; long enough that the 
calls to qemuDomainCCWAddressAssign() that are also sprinkled throughout 
qemu_hotplug.c in the same vicinity as the calls to 
qemuDomainPCIAddressEnsureAddr() weren't needed *even at the time they 
were added!* (commit f94646, March 2013). This was purely cargo-cult 
coding, caused by commit f5dd58a failing to remove the calls to 
...EnsureAddr(). The interesting bit is that both of these commits were 
put in in support of s390 virtio devices.).


(Well, *that* was a nice trip down git "memory lane" (aka "blame")

So, the result is that most of the code in qemu_hotplug that requires 
checking for address type is unneeded, and I'm going to send a patch to 
remove it.


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

Re: [libvirt] [PATCH v4 06/14] graphics: resolve address for listen type network in qemu_process

On 05/19/2016 07:35 AM, Pavel Hrdina wrote:
> Both VNC and SPICE requires the same code to resolve address for listen
> type network.  Remove code duplication and create a new function that
> will be used in qemuProcessSetupGraphics().
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_command.c | 103 
> ++--
>  src/qemu/qemu_process.c |  47 +-
>  2 files changed, 57 insertions(+), 93 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index e5847f7..ee43e21 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7384,10 +7384,7 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr 
> cfg,
>  {
>  virBuffer opt = VIR_BUFFER_INITIALIZER;
>  virDomainGraphicsListenDefPtr glisten = NULL;
> -const char *listenAddr = NULL;
> -char *netAddr = NULL;
>  bool escapeAddr;
> -int ret;
>  
>  if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC)) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -7395,6 +7392,8 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr 
> cfg,
>  goto error;
>  }
>  
> +glisten = virDomainGraphicsGetListen(graphics, 0);
> +
>  if (graphics->data.vnc.socket || cfg->vncAutoUnixSocket) {
>  if (!graphics->data.vnc.socket) {
>  if (virAsprintf(&graphics->data.vnc.socket,
> @@ -7416,52 +7415,15 @@ 
> qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
>  goto error;
>  }
>  
> -if ((glisten = virDomainGraphicsGetListen(graphics, 0))) {
> -
> -switch (glisten->type) {
> -case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
> -listenAddr = glisten->address;
> -break;
> -
> -case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
> -if (!glisten->network)
> -break;
> -
> -ret = networkGetNetworkAddress(glisten->network, &netAddr);
> -if (ret <= -2) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -   "%s", _("network-based listen not 
> possible, "
> -   "network driver not present"));
> -goto error;
> -}
> -if (ret < 0)
> -goto error;
> -
> -listenAddr = netAddr;
> -/* store the address we found in the  element so it
> - * will show up in status. */
> -if (VIR_STRDUP(glisten->address, netAddr) < 0)
> -goto error;
> -break;
> -
> -case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
> -case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST:
> -break;
> -}
> +if (glisten && glisten->address) {
> +escapeAddr = strchr(glisten->address, ':') != NULL;
> +if (escapeAddr)
> +virBufferAsprintf(&opt, "[%s]", glisten->address);
> +else
> +virBufferAdd(&opt, glisten->address, -1);
>  }
> -
> -if (!listenAddr)
> -listenAddr = cfg->vncListen;
> -

This bit being dropped kinda confused me, but I see that this is taken care of
at the new SetupNetworkAddress callers already

> -escapeAddr = strchr(listenAddr, ':') != NULL;
> -if (escapeAddr)
> -virBufferAsprintf(&opt, "[%s]", listenAddr);
> -else
> -virBufferAdd(&opt, listenAddr, -1);
>  virBufferAsprintf(&opt, ":%d",
>graphics->data.vnc.port - 5900);
> -
> -VIR_FREE(netAddr);
>  }
>  
>  if (!graphics->data.vnc.socket &&
> @@ -7525,7 +7487,6 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr 
> cfg,
>  return 0;
>  
>   error:
> -VIR_FREE(netAddr);
>  virBufferFreeAndReset(&opt);
>  return -1;
>  }
> @@ -7539,9 +7500,6 @@ 
> qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>  {
>  virBuffer opt = VIR_BUFFER_INITIALIZER;
>  virDomainGraphicsListenDefPtr glisten = NULL;
> -const char *listenAddr = NULL;
> -char *netAddr = NULL;
> -int ret;
>  int defaultMode = graphics->data.spice.defaultMode;
>  int port = graphics->data.spice.port;
>  int tlsPort = graphics->data.spice.tlsPort;
> @@ -7553,6 +7511,8 @@ 
> qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>  goto error;
>  }
>  
> +glisten = virDomainGraphicsGetListen(graphics, 0);
> +
>  if (port > 0)
>  virBufferAsprintf(&opt, "port=%u,", port);
>  
> @@ -7567,46 +7527,8 @@ 
> qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>  }
>  
>  if (port > 0 || tlsPort > 0) {
> -if ((glisten = virDomainGraphicsGetListen(graphics, 0))) {
> -
> -switch (glisten->type) {
> -case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
> -

[libvirt] [PATCH v5 4/6] qemu: Introduce qemuDomainSecretSetup

Currently just a shim to call qemuDomainSecretPlainSetup, but soon to be more

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_domain.c | 30 +-
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f038450..754536e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -835,6 +835,26 @@ qemuDomainSecretPlainSetup(virConnectPtr conn,
 }
 
 
+/* qemuDomainSecretSetup:
+ * @conn: Pointer to connection
+ * @secinfo: Pointer to secret info
+ * @protocol: Protocol for secret
+ * @authdef: Pointer to auth data
+ *
+ * A shim to call plain setup.
+ *
+ * Returns 0 on success, -1 on failure
+ */
+static int
+qemuDomainSecretSetup(virConnectPtr conn,
+  qemuDomainSecretInfoPtr secinfo,
+  virStorageNetProtocol protocol,
+  virStorageAuthDefPtr authdef)
+{
+return qemuDomainSecretPlainSetup(conn, secinfo, protocol, authdef);
+}
+
+
 /* qemuDomainSecretDiskDestroy:
  * @disk: Pointer to a disk definition
  *
@@ -880,8 +900,8 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
 if (VIR_ALLOC(secinfo) < 0)
 return -1;
 
-if (qemuDomainSecretPlainSetup(conn, secinfo, src->protocol,
-   src->auth) < 0)
+if (qemuDomainSecretSetup(conn, secinfo, src->protocol,
+  src->auth) < 0)
 goto error;
 
 diskPriv->secinfo = secinfo;
@@ -945,9 +965,9 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn,
 if (VIR_ALLOC(secinfo) < 0)
 return -1;
 
-if (qemuDomainSecretPlainSetup(conn, secinfo,
-   VIR_STORAGE_NET_PROTOCOL_ISCSI,
-   iscsisrc->auth) < 0)
+if (qemuDomainSecretSetup(conn, secinfo,
+  VIR_STORAGE_NET_PROTOCOL_ISCSI,
+  iscsisrc->auth) < 0)
 goto error;
 
 hostdevPriv->secinfo = secinfo;
-- 
2.5.5

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

[libvirt] [PATCH v5 1/6] tests: Add mock for virRandomBytes

Create a mock for virRandomBytes to generate a not so random value.
This should be usable by other tests that need a not so random number
to be generated by including the virrandommock at preload.

The "random number" generated is based upon the size of the expected
stream of bytes being returned where each byte in the result gets
the index of the array - hence a 4 byte array returns 0x00010203.

Signed-off-by: John Ferlan 
---
 tests/Makefile.am | 12 +++
 tests/virrandommock.c | 39 +++
 tests/virrandomtest.c | 86 +++
 3 files changed, 137 insertions(+)
 create mode 100644 tests/virrandommock.c
 create mode 100644 tests/virrandomtest.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index c7c9a03..98b8bb9 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -180,6 +180,7 @@ test_programs = virshtest sockettest \
virbitmaptest \
vircgrouptest \
vircryptotest \
+   virrandomtest \
virpcitest \
virendiantest \
virfiletest \
@@ -423,6 +424,7 @@ test_libraries = libshunload.la \
vircgroupmock.la \
virpcimock.la \
virnetdevmock.la \
+   virrandommock.la \
nodeinfomock.la \
nssmock.la \
$(NULL)
@@ -1080,6 +1082,10 @@ vircryptotest_SOURCES = \
vircryptotest.c testutils.h testutils.c
 vircryptotest_LDADD = $(LDADDS)
 
+virrandomtest_SOURCES = \
+   virrandomtest.c testutils.h testutils.c
+virrandomtest_LDADD = $(LDADDS)
+
 virhostdevtest_SOURCES = \
virhostdevtest.c testutils.h testutils.c
 virhostdevtest_LDADD = $(LDADDS)
@@ -1094,6 +1100,12 @@ virpcimock_la_CFLAGS = $(AM_CFLAGS)
 virpcimock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
 virpcimock_la_LIBADD = $(MOCKLIBS_LIBS)
 
+virrandommock_la_SOURCES = \
+   virrandommock.c
+virrandommock_la_CFLAGS = $(AM_CFLAGS)
+virrandommock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
+virrandommock_la_LIBADD = $(MOCKLIBS_LIBS)
+
 nodeinfomock_la_SOURCES = \
nodeinfomock.c
 nodeinfomock_la_CFLAGS = $(AM_CFLAGS)
diff --git a/tests/virrandommock.c b/tests/virrandommock.c
new file mode 100644
index 000..6df5e20
--- /dev/null
+++ b/tests/virrandommock.c
@@ -0,0 +1,39 @@
+/*
+ * Copyright (C) 2016 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ * Author: John Ferlan 
+ */
+
+#include 
+
+#include "internal.h"
+#include "virrandom.h"
+#include "virmock.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+int
+virRandomBytes(unsigned char *buf,
+   size_t buflen)
+{
+size_t i;
+
+for (i = 0; i < buflen; i++)
+buf[i] = i;
+
+return 0;
+}
diff --git a/tests/virrandomtest.c b/tests/virrandomtest.c
new file mode 100644
index 000..f76911f
--- /dev/null
+++ b/tests/virrandomtest.c
@@ -0,0 +1,86 @@
+/*
+ * Copyright (C) 2016 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ * Author: John Ferlan 
+ */
+
+#include 
+
+#include "internal.h"
+#include "viralloc.h"
+#include "virrandom.h"
+#include "testutils.h"
+
+#ifndef WIN32
+
+# define VIR_FROM_THIS VIR_FROM_NONE
+
+static int
+testRandomBytes(const void *unused ATTRIBUTE_UNUSED)
+{
+int ret = -1;
+size_t i;
+uint8_t *data;
+size_t datalen = 32;
+
+if (VIR_ALLOC_N(data, datalen) < 0)
+return -1;
+
+if (virRandomBytes(data, datalen) < 0) {
+fprintf(stderr, "Failed to generate random bytes");
+goto cleanup;
+}
+
+for (i = 0; i < datalen; i++) {
+if (data[i] != i) {
+fprintf(stderr,
+"virRandomBytes data[%zu]='%x' not in seqence\n",
+i, data[i]);
+

[libvirt] [PATCH v5 6/6] qemu: Utilize qemu secret objects for RBD auth/secret

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

If they're available and we need to pass secrets to qemu, then use the
qemu domain secret object in order to pass the secrets for RBD volumes
instead of passing the base64 encoded secret on the command line.

The goal is to make AES secrets the default and have no user interaction
required in order to allow using the AES mechanism. If the mechanism
is not available, then fall back to the current plain mechanism using
a base64 encoded secret.

New APIs:

qemu_domain.c:
  qemuDomainGetSecretAESAlias:
Generate/return the secret object alias for an AES Secret Info type.
This will be called from qemuDomainSecretAESSetup.

  qemuDomainSecretAESSetup: (private)
This API handles the details of the generation of the AES secret
and saves the pieces that need to be passed to qemu in order for
the secret to be decrypted. The encrypted secret based upon the
domain master key, an initialization vector (16 byte random value),
and the stored secret. Finally, the requirement from qemu is the IV
and encrypted secret are to be base64 encoded.

qemu_command.c:
  qemuBuildSecretInfoProps: (private)
Generate/return a JSON properties object for the AES secret to
be used by both the command building and eventually the hotplug
code in order to add the secret object. Code was designed so that
in the future perhaps hotplug could use it if it made sense.

  qemuBuildObjectSecretCommandLine (private)
Generate and add to the command line the -object secret for the
secret. This will be required for the subsequent RBD reference
to the object.

  qemuBuildDiskSecinfoCommandLine (private)
Handle adding the AES secret object.

Adjustments:

qemu_domain.c:
  The qemuDomainSecretSetup was altered to call either the AES or Plain
  Setup functions based upon whether AES secrets are possible (we have
  the encryption API) or not, we have secrets, and of course if the
  protocol source is RBD.

qemu_command.c:
  Adjust the qemuBuildRBDSecinfoURI API's in order to generate the
  specific command options for an AES secret, such as:

-object secret,id=$alias,keyid=$masterKey,data=$base64encodedencrypted,
format=base64
-drive file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\
   mon_host=mon1.example.org\:6321,password-secret=$alias,...

  where the 'id=' value is the secret object alias generated by
  concatenating the disk alias and "-aesKey0". The 'keyid= $masterKey'
  is the master key shared with qemu, and the -drive syntax will
  reference that alias as the 'password-secret'. For the -drive
  syntax, the 'id=myname' is kept to define the username, while the
  'key=$base64 encoded secret' is removed.

  While according to the syntax described for qemu commit '60390a21'
  or as seen in the email archive:

https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04083.html

  it is possible to pass a plaintext password via a file, the qemu
  commit 'ac1d8878' describes the more feature rich 'keyid=' option
  based upon the shared masterKey.

Add tests for checking/comparing output.

NB: For hotplug, since the hotplug code doesn't add command line
arguments, passing the encoded secret directly to the monitor
will suffice.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_alias.c  |  23 
 src/qemu/qemu_alias.h  |   2 +
 src/qemu/qemu_command.c| 117 ++-
 src/qemu/qemu_domain.c | 127 +++--
 ...muxml2argv-disk-drive-network-rbd-auth-AES.args |  31 +
 ...emuxml2argv-disk-drive-network-rbd-auth-AES.xml |  42 +++
 tests/qemuxml2argvmock.c   |  16 +++
 tests/qemuxml2argvtest.c   |   5 +-
 8 files changed, 354 insertions(+), 9 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.xml

diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index cb102ec..d624071 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -482,3 +482,26 @@ qemuDomainGetMasterKeyAlias(void)
 
 return alias;
 }
+
+
+/* qemuDomainGetSecretAESAlias:
+ *
+ * Generate and return an alias for the encrypted secret
+ *
+ * Returns NULL or a string containing the alias
+ */
+char *
+qemuDomainGetSecretAESAlias(const char *srcalias)
+{
+char *alias;
+
+if (!srcalias) {
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+   _("encrypted secret alias requires valid source 
alias"));
+return NULL;
+}
+
+ignore_value(virAsprintf(&alias, "%s-secret0", srcalias));
+
+return alias;
+}
diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
index 2d7bc9b..e328a9b 100644
--- a/src/qemu/qemu_alias.h
+++ b/src/qemu/qemu_alias.h
@@ -69

[libvirt] [PATCH v5 3/6] util: Introduce virCryptoGenerateRandom

Move the logic from qemuDomainGenerateRandomKey into this new
function, altering the comments, variable names, and error messages
to keep things more generic.

NB: Although perhaps more reasonable to add soemthing to virrandom.c.
The virrandom.c was included in the setuid_rpc_client, so I chose
placement in vircrypto.

Signed-off-by: John Ferlan 
---
 src/libvirt_private.syms |  1 +
 src/qemu/qemu_domain.c   | 53 ++--
 src/util/vircrypto.c | 41 +
 src/util/vircrypto.h |  2 ++
 4 files changed, 46 insertions(+), 51 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6c02b10..fb5b419 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1395,6 +1395,7 @@ virConfWriteMem;
 
 # util/vircrypto.h
 virCryptoEncryptData;
+virCryptoGenerateRandom;
 virCryptoHashString;
 virCryptoHaveCipher;
 
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 0cec340..f038450 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -45,15 +45,8 @@
 #include "virthreadjob.h"
 #include "viratomic.h"
 #include "virprocess.h"
-#include "virrandom.h"
+#include "vircrypto.h"
 #include "secret_util.h"
-#include "base64.h"
-#ifdef WITH_GNUTLS
-# include 
-# if HAVE_GNUTLS_CRYPTO_H
-#  include 
-# endif
-#endif
 #include "logging/log_manager.h"
 #include "locking/domain_lock.h"
 
@@ -630,48 +623,6 @@ qemuDomainMasterKeyReadFile(qemuDomainObjPrivatePtr priv)
 }
 
 
-/* qemuDomainGenerateRandomKey
- * @nbytes: Size in bytes of random key to generate
- *
- * Generate a random key of nbytes length and return it.
- *
- * Since the gnutls_rnd could be missing, provide an alternate less
- * secure mechanism to at least have something.
- *
- * Returns pointer memory containing key on success, NULL on failure
- */
-static uint8_t *
-qemuDomainGenerateRandomKey(size_t nbytes)
-{
-uint8_t *key;
-int ret;
-
-if (VIR_ALLOC_N(key, nbytes) < 0)
-return NULL;
-
-#if HAVE_GNUTLS_RND
-/* Generate a master key using gnutls_rnd() if possible */
-if ((ret = gnutls_rnd(GNUTLS_RND_RANDOM, key, nbytes)) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("failed to generate master key, ret=%d"), ret);
-VIR_FREE(key);
-return NULL;
-}
-#else
-/* If we don't have gnutls_rnd(), we will generate a less cryptographically
- * strong master key from /dev/urandom.
- */
-if ((ret = virRandomBytes(key, nbytes)) < 0) {
-virReportSystemError(ret, "%s", _("failed to generate master key"));
-VIR_FREE(key);
-return NULL;
-}
-#endif
-
-return key;
-}
-
-
 /* qemuDomainMasterKeyRemove:
  * @priv: Pointer to the domain private object
  *
@@ -718,7 +669,7 @@ qemuDomainMasterKeyCreate(virDomainObjPtr vm)
 return 0;
 
 if (!(priv->masterKey =
-  qemuDomainGenerateRandomKey(QEMU_DOMAIN_MASTER_KEY_LEN)))
+  virCryptoGenerateRandom(QEMU_DOMAIN_MASTER_KEY_LEN)))
 return -1;
 
 priv->masterKeyLen = QEMU_DOMAIN_MASTER_KEY_LEN;
diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index b8f5554..a2132bf 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -266,3 +266,44 @@ virCryptoEncryptData(virCryptoCipher algorithm,
_("algorithm=%d is not supported"), algorithm);
 return -1;
 }
+
+/* virCryptoGenerateRandom:
+ * @nbytes: Size in bytes of random byte stream to generate
+ *
+ * Generate a random stream of nbytes length and return it.
+ *
+ * Since the gnutls_rnd could be missing, provide an alternate less
+ * secure mechanism to at least have something.
+ *
+ * Returns pointer memory containing byte stream on success, NULL on failure
+ */
+uint8_t *
+virCryptoGenerateRandom(size_t nbytes)
+{
+uint8_t *buf;
+int ret;
+
+if (VIR_ALLOC_N(buf, nbytes) < 0)
+return NULL;
+
+#if HAVE_GNUTLS_RND
+/* Generate the byte stream using gnutls_rnd() if possible */
+if ((ret = gnutls_rnd(GNUTLS_RND_RANDOM, buf, nbytes)) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("failed to generate byte stream, ret=%d"), ret);
+VIR_FREE(buf);
+return NULL;
+}
+#else
+/* If we don't have gnutls_rnd(), we will generate a less cryptographically
+ * strong master buf from /dev/urandom.
+ */
+if ((ret = virRandomBytes(buf, nbytes)) < 0) {
+virReportSystemError(ret, "%s", _("failed to generate byte stream"));
+VIR_FREE(buf);
+return NULL;
+}
+#endif
+
+return buf;
+}
diff --git a/src/util/vircrypto.h b/src/util/vircrypto.h
index f0ec07b..1270414 100644
--- a/src/util/vircrypto.h
+++ b/src/util/vircrypto.h
@@ -55,4 +55,6 @@ int virCryptoEncryptData(virCryptoCipher algorithm,
 ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6)
 ATTRIBUTE_NONNULL(8) ATTRIBUTE_RETURN_CHECK;
 
+uint8_t *virCryptoGenerateRandom(size_t nbytes);
+
 #

[libvirt] [PATCH v5 2/6] util: Introduce encryption APIs

Introduce virCryptoHaveCipher and virCryptoEncryptData to handle
performing encryption.

 virCryptoHaveCipher:
   Boolean function to determine whether the requested cipher algorithm
   is available. It's expected this API will be called prior to
   virCryptoEncryptdata. It will return true/false.

 virCryptoEncryptData:
   Based on the requested cipher type, call the specific encryption
   API to encrypt the data.

Currently the only algorithm support is the AES 256 CBC encryption.

Adjust tests for the API's

Signed-off-by: John Ferlan 
---
 configure.ac |   1 +
 src/libvirt_private.syms |   2 +
 src/util/vircrypto.c | 192 ++-
 src/util/vircrypto.h |  20 -
 tests/vircryptotest.c| 100 +++-
 5 files changed, 312 insertions(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index 378069d..10fbd20 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1261,6 +1261,7 @@ if test "x$with_gnutls" != "xno"; then
   ]])
 
   AC_CHECK_FUNCS([gnutls_rnd])
+  AC_CHECK_FUNCS([gnutls_cipher_encrypt])
 
   CFLAGS="$old_CFLAGS"
   LIBS="$old_LIBS"
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 868bcfa..6c02b10 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1394,7 +1394,9 @@ virConfWriteMem;
 
 
 # util/vircrypto.h
+virCryptoEncryptData;
 virCryptoHashString;
+virCryptoHaveCipher;
 
 
 # util/virdbus.h
diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index 39a479a..b8f5554 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -1,7 +1,7 @@
 /*
  * vircrypto.c: cryptographic helper APIs
  *
- * Copyright (C) 2014 Red Hat, Inc.
+ * Copyright (C) 2014, 2016 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -21,11 +21,20 @@
 #include 
 
 #include "vircrypto.h"
+#include "virlog.h"
 #include "virerror.h"
 #include "viralloc.h"
 
 #include "md5.h"
 #include "sha256.h"
+#ifdef WITH_GNUTLS
+# include 
+# if HAVE_GNUTLS_CRYPTO_H
+#  include 
+# endif
+#endif
+
+VIR_LOG_INIT("util.crypto");
 
 #define VIR_FROM_THIS VIR_FROM_CRYPTO
 
@@ -76,3 +85,184 @@ virCryptoHashString(virCryptoHash hash,
 
 return 0;
 }
+
+
+/* virCryptoHaveCipher:
+ * @algorithm: Specific cipher algorithm desired
+ *
+ * Expected to be called prior to virCryptoEncryptData in order
+ * to determine whether the requested encryption option is available,
+ * so that "other" alternatives can be taken if the algorithm is
+ * not available.
+ *
+ * Returns true if we can support the encryption.
+ */
+bool
+virCryptoHaveCipher(virCryptoCipher algorithm)
+{
+switch (algorithm) {
+
+case VIR_CRYPTO_CIPHER_AES256CBC:
+#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT
+return true;
+#else
+return false;
+#endif
+
+case VIR_CRYPTO_CIPHER_NONE:
+case VIR_CRYPTO_CIPHER_LAST:
+break;
+};
+
+return false;
+}
+
+
+#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT
+/* virCryptoEncryptDataAESgntuls:
+ *
+ * Performs the AES gnutls encryption
+ *
+ * Same input as virCryptoEncryptData, except the algoritm is replaced
+ * by the specific gnutls algorithm.
+ *
+ * Encrypts the @data buffer using the @enckey and if available the @iv
+ *
+ * Returns 0 on success with the ciphertext being filled. It is the
+ * caller's responsibility to clear and free it. Returns -1 on failure
+ * w/ error set.
+ */
+static int
+virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t gnutls_enc_alg,
+  uint8_t *enckey,
+  size_t enckeylen,
+  uint8_t *iv,
+  size_t ivlen,
+  uint8_t *data,
+  size_t datalen,
+  uint8_t **ciphertextret,
+  size_t *ciphertextlenret)
+{
+int rc;
+size_t i;
+gnutls_cipher_hd_t handle = NULL;
+gnutls_datum_t enc_key;
+gnutls_datum_t iv_buf;
+uint8_t *ciphertext;
+size_t ciphertextlen;
+
+/* Allocate a padded buffer, copy in the data */
+ciphertextlen = VIR_ROUND_UP(datalen, 16);
+if (VIR_ALLOC_N(ciphertext, ciphertextlen) < 0)
+return -1;
+memcpy(ciphertext, data, datalen);
+
+ /* Fill in the padding of the buffer with the size of the padding
+  * which is required for decryption. */
+for (i = datalen; i < ciphertextlen; i++)
+ciphertext[i] = ciphertextlen - datalen;
+
+/* Initialize the gnutls cipher */
+enc_key.size = enckeylen;
+enc_key.data = enckey;
+if (iv) {
+iv_buf.size = ivlen;
+iv_buf.data = iv;
+}
+if ((rc = gnutls_cipher_init(&handle, gnutls_enc_alg,
+ &enc_key, &iv_buf)) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("failed to initialize cipher: '%s'"),
+   gnutls_strerror(rc)

[libvirt] [PATCH v5 5/6] tests: Allow comma separate list of libs to preload

Adjust the model to allow callers to provide a comma separated list
of libraries to be loaded. For those tests that may need more than
one mock library loaded.

Signed-off-by: John Ferlan 
---
 tests/testutils.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tests/testutils.c b/tests/testutils.c
index 9180e86..69cd77c 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -854,8 +854,16 @@ int virtTestMain(int argc,
 if (getenv("VIR_TEST_FILE_ACCESS"))
 VIRT_TEST_PRELOAD(TEST_MOCK);
 
-if (lib)
-VIRT_TEST_PRELOAD(lib);
+if (lib) {
+char **liblist;
+size_t count = 0;
+size_t i;
+
+liblist = virStringSplitCount(lib, ",", 0, &count);
+for (i = 0; i < count; i++)
+VIRT_TEST_PRELOAD(liblist[i]);
+virStringFreeListCount(liblist, count);
+}
 
 progname = last_component(argv[0]);
 if (STRPREFIX(progname, "lt-"))
-- 
2.5.5

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

[libvirt] [PATCH v5 0/6] Add AES Secret Object support (for RBD only)

This is a combination of two series... 

The first 2 patches are a followon to (v1 from yesterday):

http://www.redhat.com/archives/libvir-list/2016-May/msg01396.html

But there really were a offshoot of the original AES/IV Secret changes (v4):

http://www.redhat.com/archives/libvir-list/2016-May/msg01292.html

Hopefully I haven't forgotten anything along the way. There's been numerous
adjustments and changes along the way.

Patch 1 is a combination with adjustments of patches 1&2 from v1. This
should make virRandomBytes available in the virrandommock library
which then is used in later patches.

Patch 2 mostly adjust names, comments, adds #ifdef's for unavailable code
This patch will make use of the virrandommock library instead of
self-populating the enc_alg and iv_buf. Mainly because it's possible,
but also since it's the basis for later patches to utilize the same
virrandommock library.

Patch 3 splits out the existing qemuDomainGenerateRandomKey into a
vircrypto.c API.  The vircrypto.c was chosen over virrandom.c
because virrandom.c ends up being included in setuid_rpc_client
and it wasn't overly clear that it was desired to drag in all
of gnutls for just this one mock function.

Patch 4 splits out the qemuDomainSecretSetup as was suggested in one review

Patch 5 is new to handle the ability to have more than one mock library
to preload from a VIRT_TEST_MAIN_PRELOAD macro. As it turns out
the qemuxml2argvtest will need not only the qemuxml2argvmock, but
also the virrandommock libraries. I went with comma separated,
but a "space" separated list is fine with me too.

Patch 6 is the remainder of the v4 of the original series. Splitting it
up with ATTRIBUTE_UNUSED markers just no longer made sense. Lots
of changes here to keep up with the previous patches, but also
to adjust error messages, variable/API names, etc. Also changed
were the secret alias (leading to adjustments in each of the .args
file for the secret alias. Along the way I also had to adjust the
expected encoded ciphertext and iv since the mock algorithm changed
from all 0xff to an increasing sequence starting at 0x00 through
the length of the buffer.

John Ferlan (6):
  tests: Add mock for virRandomBytes
  util: Introduce encryption APIs
  util: Introduce virCryptoGenerateRandom
  qemu: Introduce qemuDomainSecretSetup
  tests: Allow comma separate list of libs to preload
  qemu: Utilize qemu secret objects for RBD auth/secret

 configure.ac   |   1 +
 src/libvirt_private.syms   |   3 +
 src/qemu/qemu_alias.c  |  23 ++
 src/qemu/qemu_alias.h  |   2 +
 src/qemu/qemu_command.c| 117 ++-
 src/qemu/qemu_domain.c | 200 +-
 src/util/vircrypto.c   | 233 -
 src/util/vircrypto.h   |  22 +-
 tests/Makefile.am  |  12 ++
 ...muxml2argv-disk-drive-network-rbd-auth-AES.args |  31 +++
 ...emuxml2argv-disk-drive-network-rbd-auth-AES.xml |  42 
 tests/qemuxml2argvmock.c   |  16 ++
 tests/qemuxml2argvtest.c   |   5 +-
 tests/testutils.c  |  12 +-
 tests/vircryptotest.c  | 100 -
 tests/virrandommock.c  |  39 
 tests/virrandomtest.c  |  86 
 17 files changed, 879 insertions(+), 65 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-AES.xml
 create mode 100644 tests/virrandommock.c
 create mode 100644 tests/virrandomtest.c

-- 
2.5.5

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

Re: [libvirt] [PATCH] tests: nodeinfotest: Remove virSaveLastError() usage



On 05/19/2016 03:27 PM, Cole Robinson wrote:
> It's overkill here, we can use virGetLast* instead
> ---
>  tests/nodeinfotest.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 

ACK

John

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

Re: [libvirt] [PATCH v4 05/14] qemu_command: move sasl parameter after port and addr definition

On 05/19/2016 07:35 AM, Pavel Hrdina wrote:
> This is required for following patches where new listen types will be
> introduced.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_command.c  | 20 
> ++--
>  .../qemuxml2argv-graphics-spice-sasl.args|  2 +-
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 

ACK

- Cole

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

Re: [libvirt] [PATCH v4 04/14] domain_conf: introduce virDomainGraphicsAddListenAddr

On 05/19/2016 07:35 AM, Pavel Hrdina wrote:
> Move code that decide whether we print the 'listen' attribute or not
> into virDomainGraphicsAddListenAddr() function.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/conf/domain_conf.c | 59 
> +-
>  1 file changed, 34 insertions(+), 25 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b1b2bb9..b3b60f1 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -21397,13 +21397,43 @@ virDomainGraphicsListenDefFormat(virBufferPtr buf,
>  }
>  
>  
> +/**
> + * virDomainGraphicsAddListenAddr:

The naming confused me, how about virDomainGraphicsListenDefFormatAddr? To
follow the previous function name of virDomainGraphicsListenDefFormat. Just
something with Format in it at least

Also this patch makes me realize we totally lack MIGRATABLE xml tests, but
that can be additive.

ACK

- Cole

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

Re: [libvirt] [PATCH 2/2] qemu_cgroup: allow access to /dev/dri/render*

2016-05-19 Thread Dave Airlie

On 20 May 2016 at 00:23, Daniel P. Berrange  wrote:
> On Thu, May 19, 2016 at 04:12:52PM +0200, Gerd Hoffmann wrote:
>>   Hi,
>>
>> > $ ls -lZ /dev/dri/
>> > total 0
>> > crw-rw+ 1 root video system_u:object_r:dri_device_t:s0 226,   0 May 18
>> > 19:17 card0
>> > crw---. 1 root video system_u:object_r:dri_device_t:s0 226,  64 May 18
>> > 19:17 controlD64
>> > crw-rw+ 1 root video system_u:object_r:dri_device_t:s0 226, 128 May 18
>> > 19:17 renderD128
>> >
>> > qemu itself loops over /dev/dri, grabs the first matching renderD* that it 
>> > can
>> > open, then does its magic. Some questions I have in general:
>> >
>> > - is there only ever one render node? or one per video card?
>>
>> One per video card.
>
> Is there a way to tell QEMU which video card to use ? If so we need to
> somehow represent this in libvirt.

We should probably add support for using an explicit path as the backing
for a particular virtio-gpu device. At the moment I think we just open the
first which may or may not be a great decision.

>
>> > - is it okay to use the same node for multiple VMs simultaneously?
>>
>> Yes.
>
> Presumably they're going to compete for execution time and potentially
> VRAM at least ? I assume they have 100% security isolation from each
> other though.  IOW, permissioning is really just there to prevent a
> rogue processes from doing denial of service  on the GPU resources,
> rather than actively compromising other users of the GPU ?

Securing 3D accelerated VM access to 100% is unlikely to ever be possible,
the GPU hardware just doesn't support this in some cases, later GPU
hardware is a lot better, but there will always be DoS and possible info
leaks through a GPU. I don't think VMware or anyone else do much
different here. What using a render node does is blocks you from deliberately
/accidentally accessing other users buffers using the defined API the
old drm API had a global namespace you could stumble through for
shared buffers.

>> > Maybe the long term fix is to have libvirt pass in a pre-opened fd for
>> > qemu:///system, since I don't know if it would be acceptable to chown
>> > qemu:qemu on the render node, but maybe we use setfacl instead.
>>
>> chown isn't a good idea I think.  But doesn't use libvirt setfacl anyway
>> for simliar things (i.e. /dev/bus/usb/... for usb pass-through) ?
>
> No, we exclusively switch access to QEMU only.
>
> Obviously the DRI stuff is different as we expected the host OS to
> have continued concurrent use of the video card.

chowning wouldn't be acceptable, adding an acl for qemu:qemu
would be fine though.

Dave.

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

Re: [libvirt] [PATCH v4 03/14] graphics: rename gListen to glisten

On 05/19/2016 07:35 AM, Pavel Hrdina wrote:
> We have both in the code.  Let's use only one format.
> 

ACK

- Cole

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

Re: [libvirt] [PATCH 0/6] auto-assign addresses when is specified

On 05/19/2016 03:27 PM, Laine Stump wrote:
> On 05/19/2016 01:15 PM, Cole Robinson wrote:
>> On 05/18/2016 03:23 PM, Laine Stump wrote:
>>> This is an alternative to Cole's series that permits >> type='pci'/> to force assignment of a PCI address, which is
>>> particularly useful on platforms that could connect the same device in
>>> different ways (e.g. aarch64/virt).
>>>
>>> Here is Cole's last iteration of the series:
>>>
>>>https://www.redhat.com/archives/libvir-list/2016-May/msg01088.html
>>>
>>> I had expressed a dislike of the "auto_allocate" flag that his series
>>> temporarily adds to the address (while simultaneously changing the
>>> address type to NONE) and suggested just changing all the necessary
>>> places to check for a valid PCI address instead of just checking the
>>> address type. He replied that this wasn't as simple as it looked, so I
>>> decided to try it; turns out he's right. But I still like this method
>>> better because it's not playing tricks with the address type, or
>>> adding a temporary private attribute to what should be pure config
>>> data.
>>>
>>> Your opinion may vary though :-)
>>>
>>> Note that patch 5/6 incorporates the same test case that Cole used in
>>> his penultimate patch, and I've added his patch to check the case of
>>> aarch64 at the end as well.
>>>
>> ACK series, but it's missing formatdomain.html.in changes. You can grab the
>> check from my patch #2
> 
> Right. I forgot that. I'll grab your doc bits and squash them in.
> 
>>
>> I'm fine with this approach but I'll just list the downsides
>>
>> - Less generalizable, for adding additional address types in the future, but
>> that's hypothetical at this point
> 
> Actually I was thinking the opposite :-) (not that it makes too much
> difference - how many different device types will we ever be able to actually
> choose between?)
> 
> 
>> - We don't raise an explicit error for drivers that don't support this type 
>> of
>> address allocation, like libxl.
> 
> Is that specific to my method of supporting  ? Since they
> don't use any of the common address assignment functions, I hadn't even looked
> at what they do.
> 

My patches had an explicit PostParse check in generic code that would throw an
error if  was never correctly filled in by the
hypervisor, so  
>> If it matters we could add a domain XML parse
>> feature to throw an explicit error though
>> - checking info->type == DEVICE_ADDRESS_TYPE_PCI is now a suspect pattern and
>> needs to be considered carefully in other parts of the code
> 
> Yes and no. I did check all uses of it. It really only needs extra
> qualification in code that is part of the parser or postparse callbacks (of
> course, in order to only check those parts, you need to know where/what they
> are!). Once address assignment is done, anything with ADDRESS_TYPE_PCI is
> guaranteed to have a valid PCI address.
> 

Agreed, my point was just that we need to be cognizant of the distinction
whenever new code is added

>>
>> upsides:
>> - less magic
>> - I think it will make allocating address at hotplug time simpler as well
> 
> 
> I hadn't thought about that yet -  more of the "90% of the coding effort going
> to 0.005% of the users" :-P
> 

Yeah it's certainly not a blocker for this series, but hotplug will be
relevant for aarch64 eventually

> Thanks for putting up with my opinionated opinions :-) (and for reviewing, and
> for the test cases, and ) I'll try to push it a bit later today.

No worries, thanks for implementing it :)

- Cole

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

Re: [libvirt] [PATCH 0/6] auto-assign addresses when is specified

On 05/19/2016 01:15 PM, Cole Robinson wrote:

On 05/18/2016 03:23 PM, Laine Stump wrote:

This is an alternative to Cole's series that permits to force assignment of a PCI address, which is
particularly useful on platforms that could connect the same device in
different ways (e.g. aarch64/virt).

Here is Cole's last iteration of the series:

https://www.redhat.com/archives/libvir-list/2016-May/msg01088.html

I had expressed a dislike of the "auto_allocate" flag that his series
temporarily adds to the address (while simultaneously changing the
address type to NONE) and suggested just changing all the necessary
places to check for a valid PCI address instead of just checking the
address type. He replied that this wasn't as simple as it looked, so I
decided to try it; turns out he's right. But I still like this method
better because it's not playing tricks with the address type, or
adding a temporary private attribute to what should be pure config
data.

Your opinion may vary though :-)

Note that patch 5/6 incorporates the same test case that Cole used in
his penultimate patch, and I've added his patch to check the case of
aarch64 at the end as well.

ACK series, but it's missing formatdomain.html.in changes. You can grab the
check from my patch #2

Right. I forgot that. I'll grab your doc bits and squash them in.

I'm fine with this approach but I'll just list the downsides

- Less generalizable, for adding additional address types in the future, but
that's hypothetical at this point

Actually I was thinking the opposite :-) (not that it makes too much
difference - how many different device types will we ever be able to
actually choose between?)

- We don't raise an explicit error for drivers that don't support this type of
address allocation, like libxl.

Is that specific to my method of supporting ?
Since they don't use any of the common address assignment functions, I
hadn't even looked at what they do.

If it matters we could add a domain XML parse
feature to throw an explicit error though
- checking info->type == DEVICE_ADDRESS_TYPE_PCI is now a suspect pattern and
needs to be considered carefully in other parts of the code

Yes and no. I did check all uses of it. It really only needs extra
qualification in code that is part of the parser or postparse callbacks
(of course, in order to only check those parts, you need to know
where/what they are!). Once address assignment is done, anything with
ADDRESS_TYPE_PCI is guaranteed to have a valid PCI address.

upsides:
- less magic
- I think it will make allocating address at hotplug time simpler as well

I hadn't thought about that yet - more of the "90% of the coding effort
going to 0.005% of the users" :-P

Thanks for putting up with my opinionated opinions :-) (and for
reviewing, and for the test cases, and ) I'll try to push it a bit
later today.

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

[libvirt] [PATCH] tests: nodeinfotest: Remove virSaveLastError() usage

It's overkill here, we can use virGetLast* instead
---
 tests/nodeinfotest.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c
index d8eace5..03500fb 100644
--- a/tests/nodeinfotest.c
+++ b/tests/nodeinfotest.c
@@ -44,10 +44,8 @@ linuxTestCompareFiles(char *sysfs_prefix,
 memset(&nodeinfo, 0, sizeof(nodeinfo));
 if (linuxNodeInfoCPUPopulate(sysfs_prefix, cpuinfo, arch, &nodeinfo) < 0) {
 if (virTestGetDebug()) {
-virErrorPtr error = virSaveLastError();
-if (error && error->code != VIR_ERR_OK)
-VIR_TEST_DEBUG("\n%s\n", error->message);
-virFreeError(error);
+if (virGetLastError())
+VIR_TEST_DEBUG("\n%s\n", virGetLastErrorMessage());
 }
 VIR_FORCE_FCLOSE(cpuinfo);
 goto fail;
-- 
2.7.4

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

Re: [libvirt] [PATCH v3 1/3] tests: nodeinfotest: Convert to virGetLastErrorMessage()

I pushed patches #2 and #3. Thanks!

On 05/19/2016 03:10 PM, Jovanka Gulicoska wrote:
> Remove unnecessary virSaveLastError() usage and convert to
> virGetLastErrorMessage()
> ---
>  tests/nodeinfotest.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c
> index d8eace5..7eeb297 100644
> --- a/tests/nodeinfotest.c
> +++ b/tests/nodeinfotest.c
> @@ -44,10 +44,10 @@ linuxTestCompareFiles(char *sysfs_prefix,
>  memset(&nodeinfo, 0, sizeof(nodeinfo));
>  if (linuxNodeInfoCPUPopulate(sysfs_prefix, cpuinfo, arch, &nodeinfo) < 
> 0) {
>  if (virTestGetDebug()) {
> -virErrorPtr error = virSaveLastError();
> -if (error && error->code != VIR_ERR_OK)
> -VIR_TEST_DEBUG("\n%s\n", error->message);
> -virFreeError(error);
> +const char *msg = virGetLastErrorMessage();
> +
> +if (msg)
> +VIR_TEST_DEBUG("\n%s\n", msg);
>  }
>  VIR_FORCE_FCLOSE(cpuinfo);
>  goto fail;
> 

doh, I screwed this up when I gave you the recommendation for using
virGetLastErrorMessage() here, since it basically never returns NULL, so that
check will always trigger. I'll send a patch to fix this case properly

Thanks,
Cole

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

[libvirt] [PATCH v3 1/3] tests: nodeinfotest: Convert to virGetLastErrorMessage()

Remove unnecessary virSaveLastError() usage and convert to
virGetLastErrorMessage()
---
 tests/nodeinfotest.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c
index d8eace5..7eeb297 100644
--- a/tests/nodeinfotest.c
+++ b/tests/nodeinfotest.c
@@ -44,10 +44,10 @@ linuxTestCompareFiles(char *sysfs_prefix,
 memset(&nodeinfo, 0, sizeof(nodeinfo));
 if (linuxNodeInfoCPUPopulate(sysfs_prefix, cpuinfo, arch, &nodeinfo) < 0) {
 if (virTestGetDebug()) {
-virErrorPtr error = virSaveLastError();
-if (error && error->code != VIR_ERR_OK)
-VIR_TEST_DEBUG("\n%s\n", error->message);
-virFreeError(error);
+const char *msg = virGetLastErrorMessage();
+
+if (msg)
+VIR_TEST_DEBUG("\n%s\n", msg);
 }
 VIR_FORCE_FCLOSE(cpuinfo);
 goto fail;
-- 
2.5.5

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

[libvirt] [PATCH v3 3/3] More usage of virGetLastErrorMessage

Convert to virGetLastErrorMessage() in the rest of the code
---
 daemon/libvirtd.c   |  8 ++--
 examples/object-events/event-test.c |  9 +++--
 src/bhyve/bhyve_driver.c|  3 +--
 src/conf/virsecretobj.c |  5 +
 src/libvirt.c   |  3 +--
 src/libxl/libxl_domain.c|  3 +--
 src/libxl/libxl_driver.c|  4 +---
 src/locking/lock_daemon.c   |  8 ++--
 src/logging/log_daemon.c|  8 ++--
 src/lxc/lxc_container.c |  9 +++--
 src/lxc/lxc_controller.c|  9 +++--
 src/lxc/lxc_domain.c|  4 ++--
 src/lxc/lxc_process.c   |  6 ++
 src/rpc/virnettlscontext.c  |  3 +--
 src/storage/storage_driver.c| 16 
 src/uml/uml_driver.c|  3 +--
 src/util/iohelper.c | 10 ++
 src/util/virhook.c  |  3 +--
 src/util/virhostdev.c   | 20 
 19 files changed, 41 insertions(+), 93 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index f24fb22..5617e42 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1315,12 +1315,8 @@ int main(int argc, char **argv) {
 /* Read the config file if it exists*/
 if (remote_config_file &&
 daemonConfigLoadFile(config, remote_config_file, implicit_conf) < 0) {
-virErrorPtr err = virGetLastError();
-if (err && err->message)
-VIR_ERROR(_("Can't load config file: %s: %s"),
-  err->message, remote_config_file);
-else
-VIR_ERROR(_("Can't load config file: %s"), remote_config_file);
+VIR_ERROR(_("Can't load config file: %s: %s"),
+  virGetLastErrorMessage(), remote_config_file);
 exit(EXIT_FAILURE);
 }
 
diff --git a/examples/object-events/event-test.c 
b/examples/object-events/event-test.c
index 2063536..c1ff4a7 100644
--- a/examples/object-events/event-test.c
+++ b/examples/object-events/event-test.c
@@ -917,9 +917,8 @@ main(int argc, char **argv)
 }
 
 if (virEventRegisterDefaultImpl() < 0) {
-virErrorPtr err = virGetLastError();
 fprintf(stderr, "Failed to register event implementation: %s\n",
-err && err->message ? err->message: "Unknown error");
+virGetLastErrorMessage());
 goto cleanup;
 }
 
@@ -972,17 +971,15 @@ main(int argc, char **argv)
 goto cleanup;
 
 if (virConnectSetKeepAlive(dconn, 5, 3) < 0) {
-virErrorPtr err = virGetLastError();
 fprintf(stderr, "Failed to start keepalive protocol: %s\n",
-err && err->message ? err->message : "Unknown error");
+virGetLastErrorMessage());
 run = 0;
 }
 
 while (run) {
 if (virEventRunDefaultImpl() < 0) {
-virErrorPtr err = virGetLastError();
 fprintf(stderr, "Failed to run event loop: %s\n",
-err && err->message ? err->message : "Unknown error");
+virGetLastErrorMessage());
 }
 }
 
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 441c666..c58286f 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -88,9 +88,8 @@ bhyveAutostartDomain(virDomainObjPtr vm, void *opaque)
 ret = virBhyveProcessStart(data->conn, data->driver, vm,
VIR_DOMAIN_RUNNING_BOOTED, 0);
 if (ret < 0) {
-virErrorPtr err = virGetLastError();
 VIR_ERROR(_("Failed to autostart VM '%s': %s"),
-  vm->def->name, err ? err->message : _("unknown error"));
+  vm->def->name, virGetLastErrorMessage());
 }
 }
 virObjectUnlock(vm);
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index 4babd31..c46d22c 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -990,11 +990,8 @@ virSecretLoadAllConfigs(virSecretObjListPtr secrets,
 continue;
 
 if (!(secret = virSecretLoad(secrets, de->d_name, path, configDir))) {
-virErrorPtr err = virGetLastError();
-
 VIR_ERROR(_("Error reading secret: %s"),
-  err != NULL ? err->message: _("unknown error"));
-virResetError(err);
+  virGetLastErrorMessage());
 VIR_FREE(path);
 continue;
 }
diff --git a/src/libvirt.c b/src/libvirt.c
index 114e88c..0e7e435 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -770,10 +770,9 @@ virStateInitialize(bool privileged,
 if (virStateDriverTab[i]->stateInitialize(privileged,
   callback,
   opaque) < 0) {
-virErrorPtr err = virGetLastError();
 VIR_ERROR(_("Initialization of %s state driver failed: %s"),

[libvirt] [PATCH v3 2/3] tests: More usage of virGetLastErrorMessage()

Use virGetLastErrorMessage() instead of virGetLastError() in tests
---
 tests/commandtest.c | 81 +++--
 tests/libvirtdconftest.c| 26 +++
 tests/openvzutilstest.c |  7 ++--
 tests/qemucapsprobe.c   |  6 ++--
 tests/securityselinuxtest.c |  6 ++--
 5 files changed, 46 insertions(+), 80 deletions(-)

diff --git a/tests/commandtest.c b/tests/commandtest.c
index cf5f44a..6430e20 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -178,8 +178,7 @@ static int test2(const void *unused ATTRIBUTE_UNUSED)
 int ret;
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 virCommandFree(cmd);
 return -1;
 }
@@ -190,8 +189,7 @@ static int test2(const void *unused ATTRIBUTE_UNUSED)
 }
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 virCommandFree(cmd);
 return -1;
 }
@@ -218,8 +216,7 @@ static int test3(const void *unused ATTRIBUTE_UNUSED)
  VIR_COMMAND_PASS_FD_CLOSE_PARENT);
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 goto cleanup;
 }
 
@@ -260,8 +257,7 @@ static int test4(const void *unused ATTRIBUTE_UNUSED)
 virCommandDaemonize(cmd);
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 goto cleanup;
 }
 
@@ -294,8 +290,7 @@ static int test5(const void *unused ATTRIBUTE_UNUSED)
 virCommandAddEnvPassCommon(cmd);
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 virCommandFree(cmd);
 return -1;
 }
@@ -318,8 +313,7 @@ static int test6(const void *unused ATTRIBUTE_UNUSED)
 virCommandAddEnvPassBlockSUID(cmd, "DOESNOTEXIST", NULL);
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 virCommandFree(cmd);
 return -1;
 }
@@ -343,8 +337,7 @@ static int test7(const void *unused ATTRIBUTE_UNUSED)
 virCommandAddEnvPassBlockSUID(cmd, "DOESNOTEXIST", NULL);
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 virCommandFree(cmd);
 return -1;
 }
@@ -368,8 +361,7 @@ static int test8(const void *unused ATTRIBUTE_UNUSED)
 virCommandAddEnvPair(cmd, "USER", "test");
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 virCommandFree(cmd);
 return -1;
 }
@@ -406,8 +398,7 @@ static int test9(const void *unused ATTRIBUTE_UNUSED)
 }
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 virCommandFree(cmd);
 return -1;
 }
@@ -432,8 +423,7 @@ static int test10(const void *unused ATTRIBUTE_UNUSED)
 virCommandAddArgSet(cmd, args);
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 virCommandFree(cmd);
 return -1;
 }
@@ -456,8 +446,7 @@ static int test11(const void *unused ATTRIBUTE_UNUSED)
 virCommandPtr cmd = virCommandNewArgs(args);
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 virCommandFree(cmd);
 return -1;
 }
@@ -478,8 +467,7 @@ static int test12(const void *unused ATTRIBUTE_UNUSED)
 virCommandSetInputBuffer(cmd, "Hello World\n");
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());

[libvirt] [PATCH v3 0/3] More usage of virGetLastErrorMessage()

Use virGetLastErrorMessage() instead of virGetLastError()
Link to task: 
http://wiki.libvirt.org/page/BiteSizedTasks#More_usage_of_virGetLastErrorMessage.28.29

Jovanka Gulicoska (3):
  tests: nodeinfotest: Convert to virGetLastErrorMessage()
  tests: More usage of virGetLastErrorMessage()
  More usage of virGetLastErrorMessage

 daemon/libvirtd.c   |  8 +---
 examples/object-events/event-test.c |  9 ++---
 src/bhyve/bhyve_driver.c|  3 +-
 src/conf/virsecretobj.c |  5 +--
 src/libvirt.c   |  3 +-
 src/libxl/libxl_domain.c|  3 +-
 src/libxl/libxl_driver.c|  4 +-
 src/locking/lock_daemon.c   |  8 +---
 src/logging/log_daemon.c|  8 +---
 src/lxc/lxc_container.c |  9 ++---
 src/lxc/lxc_controller.c|  9 ++---
 src/lxc/lxc_domain.c|  4 +-
 src/lxc/lxc_process.c   |  6 +--
 src/rpc/virnettlscontext.c  |  3 +-
 src/storage/storage_driver.c| 16 ++--
 src/uml/uml_driver.c|  3 +-
 src/util/iohelper.c | 10 +
 src/util/virhook.c  |  3 +-
 src/util/virhostdev.c   | 20 -
 tests/commandtest.c | 81 +
 tests/libvirtdconftest.c| 26 ++--
 tests/nodeinfotest.c|  8 ++--
 tests/openvzutilstest.c |  7 +---
 tests/qemucapsprobe.c   |  6 +--
 tests/securityselinuxtest.c |  6 +--
 25 files changed, 91 insertions(+), 177 deletions(-)

-- 
2.5.5

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

Re: [libvirt] [PATCH v4 02/14] tests: cleanup vnc auto socket test

On 05/19/2016 07:35 AM, Pavel Hrdina wrote:
> Commit 55320c23 introduced a new test for VNC to test if
> vnc_auto_unix_socket is set in qemu.conf, but forget to enable it in
> qemuxml2argvtest.c.
> 
> This patch also moves the code in qemuxml2xmltest.c next to other VNC
> tests and refactor the test so we also check the case for parsing active
> XML.
> 
> Signed-off-by: Pavel Hrdina 

ACK

- Cole

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

Re: [libvirt] [PATCH v4 01/14] qemu_domain: add a empty listen type address if we remove socket for VNC

On 05/19/2016 07:35 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_domain.c   | 12 
> +---
>  .../qemuxml2xmlout-graphics-vnc-autosocket.xml   |  4 +++-
>  2 files changed, 12 insertions(+), 4 deletions(-)

ACK

- Cole

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

[libvirt] [PATCH 5/7] Assume QEMU_CAPS_DEVICE in qemuBuildDiskDriveCommandLine

We no longer need to handle -usbdevice and the withDeviceArg
logic becomes clearer.
---
 src/qemu/qemu_command.c | 30 ++
 1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index bb40c17..0bde505 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1905,23 +1905,7 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
 unsigned int bootindex = 0;
 virDomainDiskDefPtr disk = def->disks[i];
 bool withDeviceArg = false;
-bool deviceFlagMasked = false;
-
-/* Unless we have -device, then USB disks need special
-   handling */
-if ((disk->bus == VIR_DOMAIN_DISK_BUS_USB) &&
-!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
-if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
-virCommandAddArg(cmd, "-usbdevice");
-virCommandAddArgFormat(cmd, "disk:%s", disk->src->path);
-} else {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("unsupported usb disk type for '%s'"),
-   disk->src->path);
-return -1;
-}
-continue;
-}
+bool deviceFlagMasked = true;
 
 /* PowerPC pseries based VMs do not support floppy device */
 if ((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) &&
@@ -1955,13 +1939,11 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
devices. Fortunately, those don't need
static PCI addresses, so we don't really
care that we can't use -device */
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
-if (disk->bus != VIR_DOMAIN_DISK_BUS_SD) {
-withDeviceArg = true;
-} else {
-virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE);
-deviceFlagMasked = true;
-}
+if (disk->bus != VIR_DOMAIN_DISK_BUS_SD) {
+withDeviceArg = true;
+} else {
+virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE);
+deviceFlagMasked = true;
 }
 optstr = qemuBuildDriveStr(disk,
emitBootindex ? false : !!bootindex,
-- 
2.7.3

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

Re: [libvirt] [PATCH v2 7/8] Move the detach of PCI device to the beginnging of live hotplug


On 05/19/2016 02:46 PM, Shivaprasad bhat wrote:



On Fri, May 20, 2016 at 12:05 AM, Laine Stump > wrote:


On 05/18/2016 05:35 PM, Shivaprasad G Bhat wrote:

The hostdevices are the only devices which have dependencies
outside of themselves such that, other functions of the PCI
card should also have been detached from host driver before
attempting the hotplug.


Are you saying that all the functions on a host device must be
detached from their host driver, even if you're only assigning one
or another of them to the guest?

Yes. All devices from same iommu group should be detached from the host.


Ah right, I forgot about that. But that is something we already don't 
deal with (currently you can assign a single device from an iommu group 
with many devices, but managed='yes' won't work for it. That would 
require some other sort of managed mode, like "managed='group'" or 
something, but more elaborate managed modes have already been NACKed on 
the list (I posted an RFC patch for "managed='detach'" at the request of 
someone else - it would detach the device from the host driver before 
assignment, but not reattach it afterwards. This was deemed 
inappropriate and unnecessary.)


You may wonder why we don't want to automatically detach all the other 
devices without any clue to do so in the config - it's because you can't 
expect the user to know which devices are in the same iommu group as the 
one you want to assign, and you don't want to just automatically/silent 
detach one that is essential for continued operation of the host.


So we really should only detach those devices that are going to be 
assigned to the guest.



Here, I hope the Card is independent. Otherwise, many cards can also
belong to same iommu group. In such case, manual the nodedev-detach 
for other card functions is necessary.



This patch moves the detach to the beginning of the hotplug
so that the following patch can detach all funtions first before
attempting to hotplug any.

We need not move the detach for net devices using SRIOV as
all SRIOV devices are single function devices.



I'm not sure why that makes any difference. In any case, you
should detach all the devices that are going to be assigned, then
assign them all, with the one going to function 0 being last.


There will be only function zero. So I felt its not necessary. Are you 
saying different SRIOV functions(all with function zero) be hotplugged as
different functions of single card in guest ? In that case, we would 
need to do that.


I couldn't parse your question, but I wasn't talking about SRIOV in 
particular, just saying that the way multifunction device assignment is 
done in qemu is to assign all the non-0 functions, then assign function 
0. But that was just an incidental part of the conversation. The 
important part is that you first detach from the host all devices to be 
assigned, then you assign them all.


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

[libvirt] [PATCH 1/7] Remove qemuProcessInitPCIAddresses with dependencies

It was only called for QEMUs without QEMU_CAPS_DEVICE,
which we no longer support.
---
 src/qemu/qemu_monitor.c  |  15 --
 src/qemu/qemu_monitor.h  |  10 --
 src/qemu/qemu_monitor_json.c |   9 -
 src/qemu/qemu_monitor_json.h |   3 -
 src/qemu/qemu_monitor_text.c | 126 -
 src/qemu/qemu_monitor_text.h |   3 -
 src/qemu/qemu_process.c  | 410 ---
 7 files changed, 576 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 1f402ee..7d1ca91 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2695,21 +2695,6 @@ qemuMonitorGetChardevInfo(qemuMonitorPtr mon,
 }
 
 
-int
-qemuMonitorGetAllPCIAddresses(qemuMonitorPtr mon,
-  qemuMonitorPCIAddress **addrs)
-{
-VIR_DEBUG("addrs=%p", addrs);
-
-QEMU_CHECK_MONITOR(mon);
-
-if (mon->json)
-return qemuMonitorJSONGetAllPCIAddresses(mon, addrs);
-else
-return qemuMonitorTextGetAllPCIAddresses(mon, addrs);
-}
-
-
 /**
  * qemuMonitorDriveDel:
  * @mon: monitor object
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 4d8f21f..6359314 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -677,16 +677,6 @@ int qemuMonitorAttachPCIDiskController(qemuMonitorPtr mon,
const char *bus,
virPCIDeviceAddress *guestAddr);
 
-typedef struct _qemuMonitorPCIAddress qemuMonitorPCIAddress;
-struct _qemuMonitorPCIAddress {
-unsigned int vendor;
-unsigned int product;
-virPCIDeviceAddress addr;
-};
-
-int qemuMonitorGetAllPCIAddresses(qemuMonitorPtr mon,
-  qemuMonitorPCIAddress **addrs);
-
 int qemuMonitorAddDevice(qemuMonitorPtr mon,
  const char *devicestr);
 
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 1790e4d..64a8765 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3568,15 +3568,6 @@ qemuMonitorJSONGetChardevInfo(qemuMonitorPtr mon,
 }
 
 
-int qemuMonitorJSONGetAllPCIAddresses(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
-  qemuMonitorPCIAddress **addrs 
ATTRIBUTE_UNUSED)
-{
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("query-pci not supported in JSON mode"));
-return -1;
-}
-
-
 int qemuMonitorJSONDelDevice(qemuMonitorPtr mon,
  const char *devalias)
 {
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index b207e3c..95bba69 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -215,9 +215,6 @@ int qemuMonitorJSONAttachPCIDiskController(qemuMonitorPtr 
mon,
const char *bus,
virPCIDeviceAddress *guestAddr);
 
-int qemuMonitorJSONGetAllPCIAddresses(qemuMonitorPtr mon,
-  qemuMonitorPCIAddress **addrs);
-
 int qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
  const char *devicestr);
 
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index ddf16b2..28a6e1b 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -1833,132 +1833,6 @@ int qemuMonitorTextGetChardevInfo(qemuMonitorPtr mon,
 }
 
 
-/*
- * The format we're after looks like this
- *
- *   (qemu) info pci
- *   Bus  0, device   0, function 0:
- * Host bridge: PCI device 8086:1237
- *   id ""
- *   Bus  0, device   1, function 0:
- * ISA bridge: PCI device 8086:7000
- *   id ""
- *   Bus  0, device   1, function 1:
- * IDE controller: PCI device 8086:7010
- *   BAR4: I/O at 0xc000 [0xc00f].
- *   id ""
- *   Bus  0, device   1, function 3:
- * Bridge: PCI device 8086:7113
- *   IRQ 9.
- *   id ""
- *   Bus  0, device   2, function 0:
- * VGA controller: PCI device 1013:00b8
- *   BAR0: 32 bit prefetchable memory at 0xf000 [0xf1ff].
- *   BAR1: 32 bit memory at 0xf200 [0xf2000fff].
- *   id ""
- *   Bus  0, device   3, function 0:
- * Ethernet controller: PCI device 8086:100e
- *  IRQ 11.
- *  BAR0: 32 bit memory at 0xf202 [0xf203].
- *  BAR1: I/O at 0xc040 [0xc07f].
- *   id ""
- *
- * Of this, we're interesting in the vendor/product ID
- * and the bus/device/function data.
- */
-#define CHECK_END(p) if (!(p)) break;
-#define SKIP_TO(p, lbl)\
-(p) = strstr((p), (lbl));  \
-if (p) \
-(p) += strlen(lbl);
-#define GET_INT(p, base, val)   \
-if (virStrToLong_ui((p), &(p), (base), &(val)) < 0) {   \
-virReportError(VIR_ERR_OPERATION_FAILED,   \
-   _("ca

[libvirt] [PATCH 2/7] qemu: process: Drop !QEMU_CAPS_DEVICE code

From: Cole Robinson 

Nowadays we only support qemu 0.12.0+ which provides QEMU_CAPS_DEVICE,
so this is all dead code.
---
 src/qemu/qemu_process.c | 53 ++---
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 40c238b..b7c8f25 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2406,31 +2406,29 @@ qemuProcessInitPasswords(virConnectPtr conn,
 goto cleanup;
 }
 
-if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
-for (i = 0; i < vm->def->ndisks; i++) {
-size_t secretLen;
+for (i = 0; i < vm->def->ndisks; i++) {
+size_t secretLen;
 
-if (!vm->def->disks[i]->src->encryption ||
-!virDomainDiskGetSource(vm->def->disks[i]))
-continue;
+if (!vm->def->disks[i]->src->encryption ||
+!virDomainDiskGetSource(vm->def->disks[i]))
+continue;
 
-VIR_FREE(secret);
-if (qemuProcessGetVolumeQcowPassphrase(conn,
-   vm->def->disks[i],
-   &secret, &secretLen) < 0)
-goto cleanup;
+VIR_FREE(secret);
+if (qemuProcessGetVolumeQcowPassphrase(conn,
+   vm->def->disks[i],
+   &secret, &secretLen) < 0)
+goto cleanup;
 
-VIR_FREE(alias);
-if (VIR_STRDUP(alias, vm->def->disks[i]->info.alias) < 0)
-goto cleanup;
-if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
-goto cleanup;
-ret = qemuMonitorSetDrivePassphrase(priv->mon, alias, secret);
-if (qemuDomainObjExitMonitor(driver, vm) < 0)
-ret = -1;
-if (ret < 0)
-goto cleanup;
-}
+VIR_FREE(alias);
+if (VIR_STRDUP(alias, vm->def->disks[i]->info.alias) < 0)
+goto cleanup;
+if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
+goto cleanup;
+ret = qemuMonitorSetDrivePassphrase(priv->mon, alias, secret);
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ret = -1;
+if (ret < 0)
+goto cleanup;
 }
 
  cleanup:
@@ -3290,9 +3288,8 @@ qemuProcessReconnect(void *opaque)
 goto cleanup;
 }
 
-if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE))
-if ((qemuDomainAssignAddresses(obj->def, priv->qemuCaps, obj)) < 0)
-goto error;
+if ((qemuDomainAssignAddresses(obj->def, priv->qemuCaps, obj)) < 0)
+goto error;
 
 /* if domain requests security driver we haven't loaded, report error, but
  * do not kill the domain
@@ -5981,11 +5978,9 @@ int qemuProcessAttach(virConnectPtr conn 
ATTRIBUTE_UNUSED,
  * we also need to populate the PCI address set cache for later
  * use in hotplug
  */
-if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
-VIR_DEBUG("Assigning domain PCI addresses");
-if ((qemuDomainAssignAddresses(vm->def, priv->qemuCaps, vm)) < 0)
+VIR_DEBUG("Assigning domain PCI addresses");
+if ((qemuDomainAssignAddresses(vm->def, priv->qemuCaps, vm)) < 0)
 goto error;
-}
 
 if ((timestamp = virTimeStringNow()) == NULL)
 goto error;
-- 
2.7.3

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

[libvirt] [PATCH 4/7] Remove DISK_BUS_XEN support from qemuBuildDiskDriveCommandLine

We have stopped supporting Xenner some time ago.
---
 src/qemu/qemu_command.c|  5 +--
 .../qemuargv2xmldata/qemuargv2xml-disk-xenvbd.args | 25 ---
 .../qemuargv2xmldata/qemuargv2xml-disk-xenvbd.xml  | 51 --
 tests/qemuargv2xmltest.c   |  1 -
 .../qemuxml2argvdata/qemuxml2argv-disk-xenvbd.args | 26 ---
 .../qemuxml2argvdata/qemuxml2argv-disk-xenvbd.xml  | 47 
 tests/qemuxml2argvtest.c   |  1 -
 .../qemuxml2xmlout-disk-xenvbd.xml | 51 --
 tests/qemuxml2xmltest.c|  1 -
 9 files changed, 2 insertions(+), 206 deletions(-)
 delete mode 100644 tests/qemuargv2xmldata/qemuargv2xml-disk-xenvbd.args
 delete mode 100644 tests/qemuargv2xmldata/qemuargv2xml-disk-xenvbd.xml
 delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-xenvbd.args
 delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-xenvbd.xml
 delete mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-xenvbd.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6c1bd8f..bb40c17 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1951,13 +1951,12 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
 virCommandAddArg(cmd, "-drive");
 
 /* Unfortunately it is not possible to use
-   -device for floppies, xen PV, or SD
+   -device for floppies, or SD
devices. Fortunately, those don't need
static PCI addresses, so we don't really
care that we can't use -device */
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
-if (disk->bus != VIR_DOMAIN_DISK_BUS_XEN &&
-disk->bus != VIR_DOMAIN_DISK_BUS_SD) {
+if (disk->bus != VIR_DOMAIN_DISK_BUS_SD) {
 withDeviceArg = true;
 } else {
 virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE);
diff --git a/tests/qemuargv2xmldata/qemuargv2xml-disk-xenvbd.args 
b/tests/qemuargv2xmldata/qemuargv2xml-disk-xenvbd.args
deleted file mode 100644
index 07fb4e4..000
--- a/tests/qemuargv2xmldata/qemuargv2xml-disk-xenvbd.args
+++ /dev/null
@@ -1,25 +0,0 @@
-LC_ALL=C \
-PATH=/bin \
-HOME=/home/test \
-USER=test \
-LOGNAME=test \
-QEMU_AUDIO_DRV=none \
-/usr/bin/qemu \
--name QEMUGuest1 \
--S \
--M pc \
--m 214 \
--smp 1 \
--uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
--nographic \
--monitor unix:/tmp/test-monitor,server,nowait \
--no-acpi \
--boot c \
--usb \
--drive file=/dev/HostVG/QEMUGuest1,format=raw,if=ide,bus=0,unit=0 \
--drive file=/dev/HostVG/QEMUGuest2,format=raw,if=ide,media=cdrom,bus=1,unit=0 \
--drive file=/tmp/data.img,format=raw,if=xen,index=0 \
--drive file=/tmp/logs.img,format=raw,if=xen,index=6 \
--net none \
--serial none \
--parallel none
diff --git a/tests/qemuargv2xmldata/qemuargv2xml-disk-xenvbd.xml 
b/tests/qemuargv2xmldata/qemuargv2xml-disk-xenvbd.xml
deleted file mode 100644
index 17c5e2c..000
--- a/tests/qemuargv2xmldata/qemuargv2xml-disk-xenvbd.xml
+++ /dev/null
@@ -1,51 +0,0 @@
-
-  QEMUGuest1
-  c7a5fdbd-edaf-9455-926a-d65c16db1809
-  219136
-  219136
-  1
-  
-hvm
-
-  
-  
-  destroy
-  restart
-  destroy
-  
-/usr/bin/qemu
-
-  
-  
-  
-  
-
-
-  
-  
-  
-  
-  
-
-
-  
-  
-  
-
-
-  
-  
-  
-
-
-  
-
-
-
-  
-
-
-
-
-  
-
diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c
index 48c83ea..73d3f5c 100644
--- a/tests/qemuargv2xmltest.c
+++ b/tests/qemuargv2xmltest.c
@@ -195,7 +195,6 @@ mymain(void)
 DO_TEST("disk-floppy");
 DO_TEST("disk-many");
 DO_TEST("disk-virtio");
-DO_TEST("disk-xenvbd");
 DO_TEST("disk-drive-boot-disk");
 DO_TEST("disk-drive-boot-cdrom");
 DO_TEST("disk-drive-fmt-qcow");
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-xenvbd.args 
b/tests/qemuxml2argvdata/qemuxml2argv-disk-xenvbd.args
deleted file mode 100644
index 04b9be6..000
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-xenvbd.args
+++ /dev/null
@@ -1,26 +0,0 @@
-LC_ALL=C \
-PATH=/bin \
-HOME=/home/test \
-USER=test \
-LOGNAME=test \
-QEMU_AUDIO_DRV=none \
-/usr/bin/qemu \
--name QEMUGuest1 \
--S \
--M pc \
--m 214 \
--smp 1 \
--uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
--nographic \
--nodefaults \
--monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
--no-acpi \
--boot c \
--usb \
--drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
--device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
--drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,media=cdrom,\
-id=drive-ide0-1-0 \
--device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 \
--drive file=/tmp/data.img,format=raw,if=xen,index=0 \
--drive file=/tmp/logs.img,format=raw,if=xen,index=6
diff --git a/tests/qemuxml2a

[libvirt] [PATCH 7/7] Do not mask QEMU_CAPS_DEVICE in qemuBuildDriveStr

For some disk types (XEN, SD), we want to emit the syntax
we used for disks before -device was available even
if QEMU supports -device.

Use the qemuDiskBusNeedsDeviceArg helper to figure out
whether to use the old or new syntax.
---
 src/qemu/qemu_command.c | 12 +++-
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index a6cc0c9..55326c3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1106,6 +1106,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
 char *source = NULL;
 int actualType = virStorageSourceGetActualType(disk->src);
 qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+bool emitDeviceSyntax = qemuDiskBusNeedsDeviceArg(disk->bus);
 
 if (idx < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1246,7 +1247,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
 }
 VIR_FREE(source);
 
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))
+if (emitDeviceSyntax)
 virBufferAddLit(&opt, "if=none");
 else
 virBufferAsprintf(&opt, "if=%s", bus);
@@ -1263,7 +1264,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
 }
 }
 
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
+if (emitDeviceSyntax) {
 virBufferAsprintf(&opt, ",id=%s%s", QEMU_DRIVE_HOST_PREFIX, 
disk->info.alias);
 } else {
 if (busid == -1 && unitid == -1) {
@@ -1916,7 +1917,6 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
 char *optstr;
 unsigned int bootindex = 0;
 virDomainDiskDefPtr disk = def->disks[i];
-bool deviceFlagMasked = true;
 
 /* PowerPC pseries based VMs do not support floppy device */
 if ((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) &&
@@ -1945,15 +1945,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
 
 virCommandAddArg(cmd, "-drive");
 
-if (!qemuDiskBusNeedsDeviceArg(disk->bus)) {
-virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE);
-deviceFlagMasked = true;
-}
 optstr = qemuBuildDriveStr(disk,
emitBootindex ? false : !!bootindex,
qemuCaps);
-if (deviceFlagMasked)
-virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE);
 if (!optstr)
 return -1;
 virCommandAddArg(cmd, optstr);
-- 
2.7.3

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

[libvirt] [PATCH 0/7] More QEMU_CAPS_DEVICE cleanups

While looking at Laine's patches I found uses of ADDRESS_TYPE_PCI
in functions that are only called on !QEMU_CAPS_DEVICE (i.e. never).

Then I cleaned up some more code until I remembered Cole already started
doing that. So I included the only patch I could apply easily.

Cole Robinson (1):
  qemu: process: Drop !QEMU_CAPS_DEVICE code

Ján Tomko (6):
  Remove qemuProcessInitPCIAddresses with dependencies
  qemu: always add -nodefaults
  Remove DISK_BUS_XEN support from qemuBuildDiskDriveCommandLine
  Assume QEMU_CAPS_DEVICE in qemuBuildDiskDriveCommandLine
  Introduce qemuDiskBusNeedsDeviceArg
  Do not mask QEMU_CAPS_DEVICE in qemuBuildDriveStr

 src/qemu/qemu_command.c|  90 ++--
 src/qemu/qemu_monitor.c|  15 -
 src/qemu/qemu_monitor.h|  10 -
 src/qemu/qemu_monitor_json.c   |   9 -
 src/qemu/qemu_monitor_json.h   |   3 -
 src/qemu/qemu_monitor_text.c   | 126 --
 src/qemu/qemu_monitor_text.h   |   3 -
 src/qemu/qemu_process.c| 463 ++---
 .../qemuargv2xmldata/qemuargv2xml-disk-xenvbd.args |  25 --
 .../qemuargv2xmldata/qemuargv2xml-disk-xenvbd.xml  |  51 ---
 tests/qemuargv2xmltest.c   |   1 -
 .../qemuxml2argvdata/qemuxml2argv-disk-xenvbd.args |  26 --
 .../qemuxml2argvdata/qemuxml2argv-disk-xenvbd.xml  |  47 ---
 tests/qemuxml2argvtest.c   |   1 -
 .../qemuxml2xmlout-disk-xenvbd.xml |  51 ---
 tests/qemuxml2xmltest.c|   1 -
 16 files changed, 48 insertions(+), 874 deletions(-)
 delete mode 100644 tests/qemuargv2xmldata/qemuargv2xml-disk-xenvbd.args
 delete mode 100644 tests/qemuargv2xmldata/qemuargv2xml-disk-xenvbd.xml
 delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-xenvbd.args
 delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-xenvbd.xml
 delete mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-xenvbd.xml

-- 
2.7.3

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

[libvirt] [PATCH 6/7] Introduce qemuDiskBusNeedsDeviceArg

Replace the two uses of the withDeviceArg bool in
qemuBuildDiskDriveCommandLine and allow this function to be reused in
qemuBuildDriveStr.
---
 src/qemu/qemu_command.c | 24 ++--
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0bde505..a6cc0c9 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1080,6 +1080,18 @@ qemuCheckFips(void)
 }
 
 
+/* Unfortunately it is not possible to use
+   -device for floppies, or SD
+   devices. Fortunately, those don't need
+   static PCI addresses, so we don't really
+   care that we can't use -device */
+static bool
+qemuDiskBusNeedsDeviceArg(int bus)
+{
+return bus != VIR_DOMAIN_DISK_BUS_SD;
+}
+
+
 char *
 qemuBuildDriveStr(virDomainDiskDefPtr disk,
   bool bootable,
@@ -1904,7 +1916,6 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
 char *optstr;
 unsigned int bootindex = 0;
 virDomainDiskDefPtr disk = def->disks[i];
-bool withDeviceArg = false;
 bool deviceFlagMasked = true;
 
 /* PowerPC pseries based VMs do not support floppy device */
@@ -1934,14 +1945,7 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
 
 virCommandAddArg(cmd, "-drive");
 
-/* Unfortunately it is not possible to use
-   -device for floppies, or SD
-   devices. Fortunately, those don't need
-   static PCI addresses, so we don't really
-   care that we can't use -device */
-if (disk->bus != VIR_DOMAIN_DISK_BUS_SD) {
-withDeviceArg = true;
-} else {
+if (!qemuDiskBusNeedsDeviceArg(disk->bus)) {
 virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE);
 deviceFlagMasked = true;
 }
@@ -1960,7 +1964,7 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
 else if (disk->info.bootIndex)
 bootindex = disk->info.bootIndex;
 
-if (withDeviceArg) {
+if (qemuDiskBusNeedsDeviceArg(disk->bus)) {
 if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) {
 if (virAsprintf(&optstr, "drive%c=drive-%s",
 disk->info.addr.drive.unit ? 'B' : 'A',
-- 
2.7.3

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

[libvirt] [PATCH 3/7] qemu: always add -nodefaults

Since we always asumme support of QEMU_CAPS_DEVICE.
---
 src/qemu/qemu_command.c | 37 -
 1 file changed, 8 insertions(+), 29 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4a2fa1d..6c1bd8f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4213,13 +4213,8 @@ qemuBuildVideoCommandLine(virCommandPtr cmd,
 size_t i;
 int primaryVideoType;
 
-if (!def->nvideos) {
-/* If we have -device, then we set -nodefaults already */
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE) &&
-virQEMUCapsGet(qemuCaps, QEMU_CAPS_VGA_NONE))
-virCommandAddArgList(cmd, "-vga", "none", NULL);
+if (!def->videos)
 return 0;
-}
 
 primaryVideoType = def->videos[0]->type;
 
@@ -8223,11 +8218,7 @@ qemuBuildNetCommandLine(virCommandPtr cmd,
 int last_good_net = -1;
 virErrorPtr originalError = NULL;
 
-if (!def->nnets) {
-/* If we have -device, then we set -nodefault already */
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))
-virCommandAddArgList(cmd, "-net", "none", NULL);
-} else {
+if (def->nnets) {
 unsigned int bootNet = 0;
 
 if (emitBootindex) {
@@ -8545,7 +8536,6 @@ qemuBuildSerialCommandLine(virLogManagerPtr logManager,
virQEMUCapsPtr qemuCaps)
 {
 size_t i;
-int actualSerials = 0;
 bool havespice = false;
 
 if (def->nserials) {
@@ -8582,13 +8572,8 @@ qemuBuildSerialCommandLine(virLogManagerPtr logManager,
 virCommandAddArg(cmd, devstr);
 VIR_FREE(devstr);
 }
-actualSerials++;
 }
 
-/* If we have -device, then we set -nodefaults already */
-if (!actualSerials && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))
-virCommandAddArgList(cmd, "-serial", "none", NULL);
-
 return 0;
 }
 
@@ -8601,10 +8586,6 @@ qemuBuildParallelsCommandLine(virLogManagerPtr 
logManager,
 {
 size_t i;
 
-/* If we have -device, then we set -nodefaults already */
-if (!def->nparallels && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))
-virCommandAddArgList(cmd, "-parallel", "none", NULL);
-
 for (i = 0; i < def->nparallels; i++) {
 virDomainChrDefPtr parallel = def->parallels[i];
 char *devstr;
@@ -9477,14 +9458,12 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
 virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none");
 }
 
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
-/* Disable global config files and default devices */
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_USER_CONFIG))
-virCommandAddArg(cmd, "-no-user-config");
-else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NODEFCONFIG))
-virCommandAddArg(cmd, "-nodefconfig");
-virCommandAddArg(cmd, "-nodefaults");
-}
+/* Disable global config files and default devices */
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_USER_CONFIG))
+virCommandAddArg(cmd, "-no-user-config");
+else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NODEFCONFIG))
+virCommandAddArg(cmd, "-nodefconfig");
+virCommandAddArg(cmd, "-nodefaults");
 
 if (qemuBuildSgaCommandLine(cmd, def, qemuCaps) < 0)
 goto error;
-- 
2.7.3

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

Re: [libvirt] [PATCH v2 7/8] Move the detach of PCI device to the beginnging of live hotplug


On 05/18/2016 05:35 PM, Shivaprasad G Bhat wrote:

The hostdevices are the only devices which have dependencies
outside of themselves such that, other functions of the PCI
card should also have been detached from host driver before
attempting the hotplug.


Are you saying that all the functions on a host device must be detached 
from their host driver, even if you're only assigning one or another of 
them to the guest?




This patch moves the detach to the beginning of the hotplug
so that the following patch can detach all funtions first before
attempting to hotplug any.

We need not move the detach for net devices using SRIOV as
all SRIOV devices are single function devices.



I'm not sure why that makes any difference. In any case, you should 
detach all the devices that are going to be assigned, then assign them 
all, with the one going to function 0 being last.




Signed-off-by: Shivaprasad G Bhat 
---
  src/qemu/qemu_driver.c  |   13 -
  src/qemu/qemu_hotplug.c |   18 +-
  2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 37d970e..9cff397 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8273,8 +8273,13 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, 
const char *xml,
   VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0)
  goto endjob;
  
-if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom)) < 0)

+if (dev_copy->type == VIR_DOMAIN_DEVICE_HOSTDEV &&
+dev_copy->data.hostdev->source.subsys.type == 
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
+qemuDomainAttachPCIHostDevicePrepare(driver,vm->def, 
dev_copy->data.hostdev, qemuCaps) < 0)
  goto endjob;
+
+if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom)) < 0)
+goto undoprepare;
  /*
   * update domain status forcibly because the domain status may be
   * changed even if we failed to attach the device. For example,
@@ -8309,6 +8314,12 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, 
const char *xml,
  virObjectUnref(cfg);
  virNWFilterUnlockFilterUpdates();
  return ret;
+
+  undoprepare:
+if (dev_copy->type == VIR_DOMAIN_DEVICE_HOSTDEV &&
+dev_copy->data.hostdev->source.subsys.type == 
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
+qemuHostdevReAttachPCIDevices(driver, vm->def->name, 
&dev_copy->data.hostdev, 1);
+goto endjob;
  }
  
  static int qemuDomainAttachDevice(virDomainPtr dom, const char *xml)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a2bcd89..bdfbafe 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -899,6 +899,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
  actualType = virDomainNetGetActualType(net);
  
  if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {

+virDomainHostdevDefPtr hostdev = virDomainNetGetActualHostdev(net);
  /* This is really a "smart hostdev", so it should be attached
   * as a hostdev (the hostdev code will reach over into the
   * netdev-specific code as appropriate), then also added to
@@ -907,8 +908,14 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
   * qemuDomainAttachHostDevice uses a connection to resolve
   * a SCSI hostdev secret, which is not this case, so pass NULL.
   */
-ret = qemuDomainAttachHostDevice(NULL, driver, vm,
- virDomainNetGetActualHostdev(net));
+if (qemuDomainAttachPCIHostDevicePrepare(driver, vm->def,
+ hostdev, priv->qemuCaps) < 0)
+goto cleanup;
+
+ret = qemuDomainAttachHostDevice(NULL, driver, vm, hostdev);
+if (!ret)
+qemuHostdevReAttachPCIDevices(driver, vm->def->name, &hostdev, 1);
+
  goto cleanup;
  }
  
@@ -1248,10 +1255,6 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,

  if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0)
  return -1;
  
-if (qemuDomainAttachPCIHostDevicePrepare(driver, vm->def,

- hostdev, priv->qemuCaps) < 0)
-return -1;
-
  backend = hostdev->source.subsys.u.pci.backend;
  
  /* Temporarily add the hostdev to the domain definition. This is needed

@@ -1330,13 +1333,10 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
  if (releaseaddr)
  qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL);
  
-qemuHostdevReAttachPCIDevices(driver, vm->def->name, &hostdev, 1);

-
  VIR_FREE(devstr);
  VIR_FREE(configfd_name);
  VIR_FORCE_CLOSE(configfd);
  
- cleanup:

  return -1;
  }
  


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



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

Re: [libvirt] [PATCH 1/6] conf: disk: Rename virDomainDiskDefValidate to virDomainDiskDefParseValidate

On 05/17/2016 10:25 AM, Peter Krempa wrote:
> Name the validation function distinctively since it's called in the
> parser. Later patches will add function that will validate disk
> definitions that are invalid but need to be parsed to avoid losing
> domains.
> ---
>  src/conf/domain_conf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b445469..12f9da2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6932,7 +6932,7 @@ virDomainDiskDefGeometryParse(virDomainDiskDefPtr def,
> 
> 
>  static int
> -virDomainDiskDefValidate(const virDomainDiskDef *def)
> +virDomainDiskDefParseValidate(const virDomainDiskDef *def)
>  {
>  if (def->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) {
>  if (def->event_idx != VIR_TRISTATE_SWITCH_ABSENT) {
> @@ -7537,7 +7537,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
>  goto error;
>  }
> 
> -if (virDomainDiskDefValidate(def) < 0)
> +if (virDomainDiskDefParseValidate(def) < 0)
>  goto error;
> 
>   cleanup:
> 

ACK, and could be pushed now

- Cole

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

Re: [libvirt] [PATCH 3/6] conf: Introduce infrastructure to add config validation to define time

On 05/18/2016 08:43 AM, Pavel Hrdina wrote:
> On Tue, May 17, 2016 at 04:25:39PM +0200, Peter Krempa wrote:
>> Recently there were a few patches adding checks to the post parse
>> callback infrastructure that rejected previously accepted configuration.
>> This unfortunately was not acceptable since we would fail to load the
>> configs from the disk and the VMs would vanish.
>>
>> This patch adds helpers which are called in the appropriate places after
>> the config is parsed when defining/starting a VM but not when reloading
>> the configs.
>>
>> This infrastructure is meant for invalid configurations that were
>> accepted previously and thus it's not executed when starting a VM from
>> any saved state. Any checks that are necessary to be executed at that
>> point still need to be added to qemuProcessValidateXML.
>> ---
>>  src/conf/domain_conf.c   | 67 
>> ++--
>>  src/conf/domain_conf.h   |  2 ++
>>  src/libvirt_private.syms |  1 +
>>  src/qemu/qemu_conf.h |  2 ++
>>  src/qemu/qemu_domain.c   | 21 +++
>>  src/qemu/qemu_driver.c   |  6 +
>>  src/qemu/qemu_process.c  | 22 +++-
>>  7 files changed, 100 insertions(+), 21 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> 
> [...]
> 
>> +/**
>> + * virDomainDefValidate:
>> + * @def: domain definition
>> + *
>> + * This validation function is designed to take checks of globally invalid
>> + * configurations that the parser needs to accept so that VMs don't vanish 
>> upon
>> + * daemon restart. Such definition can be rejected upon startup or define, 
>> where
>> + * this function shall be called.
>> + *
>> + * Returns 0 if domain definition is valid, -1 on error and reports an
>> + * appropriate message.
>> + */
>> +int
>> +virDomainDefValidate(const virDomainDef *def)
>> +{
>> +size_t i;
>> +
>> +/* check configuration of individual devices */
>> +VIR_DOMAIN_DEF_VALIDATE_DEVICES(disk, virDomainDiskDefValidate);
>> +
>> +return 0;
>> +}
>> +
>> +#undef VIR_DOMAIN_DEF_VALIDATE_DEVICES
> 
> [...]
> 
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> @@ -5343,3 +5343,24 @@ qemuDomainDefValidateDiskLunSource(const 
>> virStorageSource *src)
>>
>>  return 0;
>>  }
>> +
>> +
>> +/**
>> + * qemuDomainDefValidate:
>> + * @def: domain definition
>> + *
>> + * Validates that the domain definition is valid for the qemu driver. This 
>> check
>> + * is run when the domain is defined or started as new. This check is 
>> explicitly
>> + * not executed when starting a VM from snapshot/saved state/migration to 
>> allow
>> + * partially invalid configs to still run.
>> + *
>> + * Returns 0 on success, -1 on error.
>> + */
>> +int
>> +qemuDomainDefValidate(const virDomainDef *def)
>> +{
>> +if (virDomainDefValidate(def) < 0)
>> +return -1;
>> +
>> +return 0;
>> +}
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 7f8dfc8..e6d22ec 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -1799,6 +1799,9 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr 
>> conn,
>>  if (virDomainCreateXMLEnsureACL(conn, def) < 0)
>>  goto cleanup;
>>
>> +if (qemuDomainDefValidate(def) < 0)
>> +goto cleanup;
>> +
>>  if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, 
>> def->emulator)))
>>  goto cleanup;
>>
>> @@ -7275,6 +7278,9 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
>>  if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0)
>>  goto cleanup;
>>
>> +if (qemuDomainDefValidate(def) < 0)
>> +goto cleanup;
>> +
>>  if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, 
>> def->emulator)))
>>  goto cleanup;
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> 
> [...]
> 
>> @@ -4634,6 +4627,21 @@ qemuProcessStartValidateXML(virDomainObjPtr vm,
>>  return -1;
>>  }
>>
>> +/* checks below should not be executed when starting a qemu process for 
>> a
>> + * VM that was running before (migration, snapshots, save). It's more
>> + * important to start such VM than keep the configuration clean */
>> +if (!migration && !snapshot) {
>> +if (qemuDomainDefValidate(vm->def) < 0)
>> +return -1;
>> +
>> +if (virDomainDefCheckDuplicateDiskInfo(vm->def) < 0)
>> +return -1;
>> +}
>> +
>> +/* checks below validate config that would make qemu fail to start */
>> +if (qemuValidateCpuCount(vm->def, qemuCaps) < 0)
>> +return -1;
>> +
>>  return 0;
>>  }
> 
> I would suggest to do the opposite.  That would mean adding the
> virDomainDefValidate() after postParse callbacks and there you would call
> xmlopt->config.domainDefValidate().  Calling virDomainDefValidate() after
> postParse would be conditional based on a new VIR_DOMAIN_DEF_PARSE_... flag
> which would have to be set at the appropriate places so we woul

Re: [libvirt] [PATCH v2 7/8] Move the detach of PCI device to the beginnging of live hotplug

On Fri, May 20, 2016 at 12:05 AM, Laine Stump  wrote:

> On 05/18/2016 05:35 PM, Shivaprasad G Bhat wrote:
>
>> The hostdevices are the only devices which have dependencies
>> outside of themselves such that, other functions of the PCI
>> card should also have been detached from host driver before
>> attempting the hotplug.
>>
>
> Are you saying that all the functions on a host device must be detached
> from their host driver, even if you're only assigning one or another of
> them to the guest?
>
> Yes. All devices from same iommu group should be detached from the host.
Here, I hope the Card is independent. Otherwise, many cards can also
belong to same iommu group. In such case, manual the nodedev-detach for
other card functions is necessary.


>
>> This patch moves the detach to the beginning of the hotplug
>> so that the following patch can detach all funtions first before
>> attempting to hotplug any.
>>
>> We need not move the detach for net devices using SRIOV as
>> all SRIOV devices are single function devices.
>>
>
>
> I'm not sure why that makes any difference. In any case, you should detach
> all the devices that are going to be assigned, then assign them all, with
> the one going to function 0 being last.


There will be only function zero. So I felt its not necessary. Are you
saying different SRIOV functions(all with function zero) be hotplugged as
different functions of single card in guest ? In that case, we would need
to do that.


>
>
>
>> Signed-off-by: Shivaprasad G Bhat 
>> ---
>>   src/qemu/qemu_driver.c  |   13 -
>>   src/qemu/qemu_hotplug.c |   18 +-
>>   2 files changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 37d970e..9cff397 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -8273,8 +8273,13 @@ static int
>> qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
>>
>>  VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0)
>>   goto endjob;
>>   -if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom)) < 0)
>> +if (dev_copy->type == VIR_DOMAIN_DEVICE_HOSTDEV &&
>> +dev_copy->data.hostdev->source.subsys.type ==
>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
>> +qemuDomainAttachPCIHostDevicePrepare(driver,vm->def,
>> dev_copy->data.hostdev, qemuCaps) < 0)
>>   goto endjob;
>> +
>> +if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom)) < 0)
>> +goto undoprepare;
>>   /*
>>* update domain status forcibly because the domain status may
>> be
>>* changed even if we failed to attach the device. For example,
>> @@ -8309,6 +8314,12 @@ static int
>> qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
>>   virObjectUnref(cfg);
>>   virNWFilterUnlockFilterUpdates();
>>   return ret;
>> +
>> +  undoprepare:
>> +if (dev_copy->type == VIR_DOMAIN_DEVICE_HOSTDEV &&
>> +dev_copy->data.hostdev->source.subsys.type ==
>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
>> +qemuHostdevReAttachPCIDevices(driver, vm->def->name,
>> &dev_copy->data.hostdev, 1);
>> +goto endjob;
>>   }
>> static int qemuDomainAttachDevice(virDomainPtr dom, const char *xml)
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index a2bcd89..bdfbafe 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -899,6 +899,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>>   actualType = virDomainNetGetActualType(net);
>> if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
>> +virDomainHostdevDefPtr hostdev =
>> virDomainNetGetActualHostdev(net);
>>   /* This is really a "smart hostdev", so it should be attached
>>* as a hostdev (the hostdev code will reach over into the
>>* netdev-specific code as appropriate), then also added to
>> @@ -907,8 +908,14 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>>* qemuDomainAttachHostDevice uses a connection to resolve
>>* a SCSI hostdev secret, which is not this case, so pass NULL.
>>*/
>> -ret = qemuDomainAttachHostDevice(NULL, driver, vm,
>> -
>>  virDomainNetGetActualHostdev(net));
>> +if (qemuDomainAttachPCIHostDevicePrepare(driver, vm->def,
>> + hostdev,
>> priv->qemuCaps) < 0)
>> +goto cleanup;
>> +
>> +ret = qemuDomainAttachHostDevice(NULL, driver, vm, hostdev);
>> +if (!ret)
>> +qemuHostdevReAttachPCIDevices(driver, vm->def->name,
>> &hostdev, 1);
>> +
>>   goto cleanup;
>>   }
>>   @@ -1248,10 +1255,6 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr
>> driver,
>>   if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0)
>>   return -1;
>>   -if (qemuDomainAttachPCIHostDevicePrepare(driver, vm->def,
>> - ho

Re: [libvirt] [PATCH v2 5/8] Introduce virDomainPCIMultifunctionDeviceAddressAssign

On Thu, May 19, 2016 at 11:59 PM, Laine Stump  wrote:

> On 05/18/2016 05:32 PM, Shivaprasad G Bhat wrote:
>
>> This patch assigns multifunction pci addresses to devices in the devlist.
>> The pciaddrs passed in the argument is not altered so that the actual
>> call to
>> reserve the address using virDomainPCIAddressEnsureAddr() passes. The
>> function
>> focuses on user given address validation and also the auto-assign of
>> addresses.
>> The address auto-assignment works well for PPC64 and x86-i440fx machines.
>>
>
> Since you know that after hotplugging these devices into this slot, you
> won't be able to add any new devices to it, I think it's unnecessary to
> keep track of exactly which functions of the slot are occupied and which
> aren't. Effectively, they *all* are.
>
> So instead of copy-pasting the huge chunk of code and allocating one
> function at a time, how about just marking the entire slot in use at a
> higher level rather than trying to mark individual functions? (unless
> you're looking at these individual bits later for something *other* than
> just deciding which ones to free.
>
>
This was mainly for validation. If a user gives the same function number
for more than one device, I wanted to refuse that. Also, the ordering of
the user
specified slot numbers can be different. User can give different slot
numbers also. But now you point it out, I realize we need the function zero
to be at the last in the device list anyway. So, we can just force user
specified function numbers to be in decreasing order if specified. Hope
that is the right way?

Note that you'll need to determine the CONNECT_TYPE at the higher level
> too, and be sure to choose PCI_DEVICE or PCIE_DEVICE appropriately (and if
> there is any attempt to mix the two, I would say you should refuse to
> auto-assign an address (but allow it if they manually specify the address).
>
> Noted.

>
>
>> The q35 machines needs some level of logic here to get the correct next
>> valid
>> slot so that the hotplug go through fine.
>>
>
> Can you explain that? There should be no difference.
>
>
I somehow couldn't get it working. May be my setup. I'll need some help
when I try next time on this machine.


>
>
>> Signed-off-by: Shivaprasad G Bhat 
>> ---
>>   src/conf/domain_addr.c   |  199
>> ++
>>   src/conf/domain_addr.h   |4 +
>>   src/libvirt_private.syms |1
>>   3 files changed, 204 insertions(+)
>>
>> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
>> index 7ea9e4d..7c52f84 100644
>> --- a/src/conf/domain_addr.c
>> +++ b/src/conf/domain_addr.c
>> @@ -454,6 +454,205 @@
>> virDomainPCIAddressReserveSlot(virDomainPCIAddressSetPtr addrs,
>>   return virDomainPCIAddressReserveAddr(addrs, addr, flags, true,
>> false);
>>   }
>>   +static int
>> +virDomainPCIAddressGetNextFunctionAddr(uint8_t *slot,
>> +   virPCIDeviceAddressPtr addr)
>> +{
>> +size_t i = 0;
>> +
>> +for (i = 0; i < 8; i++) {
>> +if (!(*slot & 1 << i)) {
>> +addr->function = i;
>> +break;
>> +}
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +static int
>> +virDomainPCIAddressAssignFunctionAddr(virDomainPCIAddressSetPtr addrs,
>> +  uint8_t *slot,
>> +  virPCIDeviceAddressPtr addr,
>> +  virDomainPCIConnectFlags flags,
>> +  bool fromConfig)
>> +{
>> +int ret = -1;
>> +char *addrStr = NULL;
>> +virErrorNumber errType = (fromConfig
>> +  ? VIR_ERR_XML_ERROR :
>> VIR_ERR_INTERNAL_ERROR);
>> +
>> +if (!(addrStr = virDomainPCIAddressAsString(addr)))
>> +goto cleanup;
>> +
>> +/* Check that the requested bus exists, is the correct type, and we
>> + * are asking for a valid slot and function
>> + */
>> +if (!virDomainPCIAddressValidate(addrs, addr, addrStr, flags,
>> fromConfig))
>> +goto cleanup;
>> +
>> +if (*slot & (1 << addr->function)) {
>> +virReportError(errType,
>> +   _("Attempted double use of PCI Address %s"),
>> +   addrStr);
>> +goto cleanup;
>> +}
>> +*slot |= (1 << addr->function);
>> +if (addr->function == 0)
>> +addr->multi = VIR_TRISTATE_SWITCH_ON;
>> +VIR_DEBUG("Reserving PCI address %s", addrStr);
>> +
>> +ret = 0;
>> + cleanup:
>> +VIR_FREE(addrStr);
>> +return ret;
>> +}
>> +
>> +
>> +static int
>> +virDomainPCIAddressAssignSlotNextFunction(virDomainPCIAddressSetPtr
>> addrs,
>> +  uint8_t *slot,
>> +  virDomainDeviceInfoPtr dev,
>> +  virDomainPCIConnectFlags flags)
>> +{
>> +if (virDomainPCIAddressGetNextFunctionAddr(slot, &dev->addr.pci) < 0)
>> +return -1;
>> +
>> +

Re: [libvirt] [PATCH 2/6] qemu: driver: Fix function header alignment of some functions

On 05/17/2016 10:25 AM, Peter Krempa wrote:
> ---
>  src/qemu/qemu_driver.c | 17 -
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 

ACK, and could be pushed now

- Cole


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

Re: [libvirt] [PATCH v2 5/8] Introduce virDomainPCIMultifunctionDeviceAddressAssign


On 05/18/2016 05:32 PM, Shivaprasad G Bhat wrote:

This patch assigns multifunction pci addresses to devices in the devlist.
The pciaddrs passed in the argument is not altered so that the actual call to
reserve the address using virDomainPCIAddressEnsureAddr() passes. The function
focuses on user given address validation and also the auto-assign of addresses.
The address auto-assignment works well for PPC64 and x86-i440fx machines.


Since you know that after hotplugging these devices into this slot, you 
won't be able to add any new devices to it, I think it's unnecessary to 
keep track of exactly which functions of the slot are occupied and which 
aren't. Effectively, they *all* are.


So instead of copy-pasting the huge chunk of code and allocating one 
function at a time, how about just marking the entire slot in use at a 
higher level rather than trying to mark individual functions? (unless 
you're looking at these individual bits later for something *other* than 
just deciding which ones to free.


Note that you'll need to determine the CONNECT_TYPE at the higher level 
too, and be sure to choose PCI_DEVICE or PCIE_DEVICE appropriately (and 
if there is any attempt to mix the two, I would say you should refuse to 
auto-assign an address (but allow it if they manually specify the address).





The q35 machines needs some level of logic here to get the correct next valid
slot so that the hotplug go through fine.


Can you explain that? There should be no difference.




Signed-off-by: Shivaprasad G Bhat 
---
  src/conf/domain_addr.c   |  199 ++
  src/conf/domain_addr.h   |4 +
  src/libvirt_private.syms |1
  3 files changed, 204 insertions(+)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 7ea9e4d..7c52f84 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -454,6 +454,205 @@ virDomainPCIAddressReserveSlot(virDomainPCIAddressSetPtr 
addrs,
  return virDomainPCIAddressReserveAddr(addrs, addr, flags, true, false);
  }
  
+static int

+virDomainPCIAddressGetNextFunctionAddr(uint8_t *slot,
+   virPCIDeviceAddressPtr addr)
+{
+size_t i = 0;
+
+for (i = 0; i < 8; i++) {
+if (!(*slot & 1 << i)) {
+addr->function = i;
+break;
+}
+}
+
+return 0;
+}
+
+static int
+virDomainPCIAddressAssignFunctionAddr(virDomainPCIAddressSetPtr addrs,
+  uint8_t *slot,
+  virPCIDeviceAddressPtr addr,
+  virDomainPCIConnectFlags flags,
+  bool fromConfig)
+{
+int ret = -1;
+char *addrStr = NULL;
+virErrorNumber errType = (fromConfig
+  ? VIR_ERR_XML_ERROR : VIR_ERR_INTERNAL_ERROR);
+
+if (!(addrStr = virDomainPCIAddressAsString(addr)))
+goto cleanup;
+
+/* Check that the requested bus exists, is the correct type, and we
+ * are asking for a valid slot and function
+ */
+if (!virDomainPCIAddressValidate(addrs, addr, addrStr, flags, fromConfig))
+goto cleanup;
+
+if (*slot & (1 << addr->function)) {
+virReportError(errType,
+   _("Attempted double use of PCI Address %s"),
+   addrStr);
+goto cleanup;
+}
+*slot |= (1 << addr->function);
+if (addr->function == 0)
+addr->multi = VIR_TRISTATE_SWITCH_ON;
+VIR_DEBUG("Reserving PCI address %s", addrStr);
+
+ret = 0;
+ cleanup:
+VIR_FREE(addrStr);
+return ret;
+}
+
+
+static int
+virDomainPCIAddressAssignSlotNextFunction(virDomainPCIAddressSetPtr addrs,
+  uint8_t *slot,
+  virDomainDeviceInfoPtr dev,
+  virDomainPCIConnectFlags flags)
+{
+if (virDomainPCIAddressGetNextFunctionAddr(slot, &dev->addr.pci) < 0)
+return -1;
+
+if (virDomainPCIAddressAssignFunctionAddr(addrs, slot, &dev->addr.pci, flags, 
false) < 0)
+return -1;
+
+dev->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
+
+return 0;
+}
+
+
+static int
+virDomainPCIAddressAssignSlotAddr(virDomainPCIAddressSetPtr addrs,
+  uint8_t *slot,
+  virDomainDeviceInfoPtr dev)
+{
+int ret = -1;
+char *addrStr = NULL;
+virDomainPCIConnectFlags flags = (VIR_PCI_CONNECT_HOTPLUGGABLE |
+  VIR_PCI_CONNECT_TYPE_PCI_DEVICE);
+
+if (!(addrStr = virDomainPCIAddressAsString(&dev->addr.pci)))
+goto cleanup;
+
+if (dev->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
+if (((dev->addr.pci.function == 0) && (dev->addr.pci.multi == 
VIR_TRISTATE_SWITCH_ON)) ||
+dev->addr.pci.function != 0) {
+
+if (!virDomainPCIAddressValidate(addrs, &dev->addr.pci,
+

Re: [libvirt] [PATCH v2 2/8] Release address in function granularity than slot

On Thu, May 19, 2016 at 11:42 PM, Laine Stump  wrote:

> Since you have to unplug all the functions in a slot at the same time
> anyway, I don't see the point in reverting this commit - you'll just end up
> needing to call  it multiple times - once for each function that was in the
> slot.
>

I thought of going this way because, incomplete hotplugs can be attempted
towards completion if the hotplug reverts failed. Otherwise, we will have
the devices left in the xml with the slot marked free if the hotplug abort
failed by any means. This being for a negative test case, I am not sure if
recovery
of such sorts is possible.


>
> (just guessing without looking at the code - perhaps it's already being
> called from within lower level functions for each device, and each device
> gets its own notification from qemu that it's been detached? Or to ask a
> more specific question - exactly what happens with device detach? Do you
> send qemu a single detach command and it detaches all the functions as a
> single unit? Or do you send it multiple detach commands, with function 0
> being the last?)
>
>
Yes. There is an event for each function. On x86, any function device_del
would detach all functions of the card and events are sent for all
functions. On PPC64, each function should be device_del'ed and function 0
at last and on function 0 deletion alone all the events will come for each
device.

.

>
>
> On 05/18/2016 05:30 PM, Shivaprasad G Bhat wrote:
>
>> The commit 6fe678c is reverted. The code is moved around and cant revert
>> staright.
>>
>> Signed-off-by: Shivaprasad G Bhat 
>> ---
>>   src/conf/domain_addr.c |   22 +-
>>   src/libvirt_private.syms   |1 +
>>   src/qemu/qemu_domain_address.c |2 +-
>>   3 files changed, 11 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
>> index acd8ce6..35c7cd4 100644
>> --- a/src/conf/domain_addr.c
>> +++ b/src/conf/domain_addr.c
>> @@ -472,21 +472,17 @@
>> virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs,
>>   goto cleanup;
>> if (dev->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
>> -/* We do not support hotplug multi-function PCI device now, so
>> we should
>> - * reserve the whole slot. The function of the PCI device must
>> be 0.
>> - */
>> -if (dev->addr.pci.function != 0) {
>> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> -   _("Only PCI device addresses with function=0"
>> - " are supported"));
>> -goto cleanup;
>> -}
>> +if (((dev->addr.pci.function == 0) && (dev->addr.pci.multi ==
>> VIR_TRISTATE_SWITCH_ON)) ||
>> +dev->addr.pci.function != 0) {
>>   -if (!virDomainPCIAddressValidate(addrs, &dev->addr.pci,
>> - addrStr, flags, true))
>> -goto cleanup;
>> +if (!virDomainPCIAddressValidate(addrs, &dev->addr.pci,
>> + addrStr, flags, true))
>> +goto cleanup;
>>   -ret = virDomainPCIAddressReserveSlot(addrs, &dev->addr.pci,
>> flags);
>> +ret = virDomainPCIAddressReserveAddr(addrs, &dev->addr.pci,
>> flags, false, true);
>> +} else {
>> +ret = virDomainPCIAddressReserveSlot(addrs, &dev->addr.pci,
>> flags);
>> +}
>>   } else {
>>   ret = virDomainPCIAddressReserveNextSlot(addrs, dev, flags);
>>   }
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index fb24808..e4953b7 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -98,6 +98,7 @@ virDomainPCIAddressBusSetModel;
>>   virDomainPCIAddressEnsureAddr;
>>   virDomainPCIAddressFlagsCompatible;
>>   virDomainPCIAddressGetNextSlot;
>> +virDomainPCIAddressReleaseAddr;
>>   virDomainPCIAddressReleaseSlot;
>>   virDomainPCIAddressReserveAddr;
>>   virDomainPCIAddressReserveNextSlot;
>> diff --git a/src/qemu/qemu_domain_address.c
>> b/src/qemu/qemu_domain_address.c
>> index 9c8c262..1e7d98c 100644
>> --- a/src/qemu/qemu_domain_address.c
>> +++ b/src/qemu/qemu_domain_address.c
>> @@ -1682,7 +1682,7 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm,
>>NULLSTR(devstr));
>>   else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
>>virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
>> - virDomainPCIAddressReleaseSlot(priv->pciaddrs,
>> + virDomainPCIAddressReleaseAddr(priv->pciaddrs,
>>   &info->addr.pci) < 0)
>>   VIR_WARN("Unable to release PCI address on %s",
>>NULLSTR(devstr));
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
>>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listi

Re: [libvirt] [PATCH v2 3/8] Validate address in virDomainPCIAddressReleaseAddr


On 05/18/2016 05:30 PM, Shivaprasad G Bhat wrote:

This function was not used so far. Now, that we begin to use it, make sure to
check the address before actually releasing the address.

Signed-off-by: Shivaprasad G Bhat 
---
  src/conf/domain_addr.c |   18 +-
  1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 35c7cd4..7ea9e4d 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -497,8 +497,24 @@ int
  virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs,
 virPCIDeviceAddressPtr addr)
  {
+/* permit any kind of connection type in validation, since we
+ * already had it, and are giving it back.
+ */
+virDomainPCIConnectFlags flags = VIR_PCI_CONNECT_TYPES_MASK;
+int ret = -1;
+char *addrStr = NULL;
+
+if (!(addrStr = virDomainPCIAddressAsString(addr)))
+goto cleanup;
+
+if (!virDomainPCIAddressValidate(addrs, addr, addrStr, flags, false))
+goto cleanup;


I don't think this is necessary. virDomainPCIAddressValidate() is used 
to verify that it's okay to plug a particular device into a particular 
slot. But since the device is already in the slot, that's a moot point. 
The only thing that needs to be verified is that libvirt has information 
that this device is in the given slot (which I'm assuming is done at a 
higher level).



+
  addrs->buses[addr->bus].slots[addr->slot] &= ~(1 << addr->function);
-return 0;
+ret = 0;
+ cleanup:
+VIR_FREE(addrStr);
+return ret;
  }
  
  int


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



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

Re: [libvirt] [PATCH 0/3] More usage of virGetLastErrorMessage()

On 05/19/2016 01:07 PM, Jovanka Gulicoska wrote:
> v2 of patches

When sending a v2, use format-patch/send-email -v2 option, so the subject
looks like

[PATCH v2 0/3] ...

> Use virGetLastErrorMessage() insted of virGetLastError()

*instead, though spelling in cover letters isn't enforced or anything, but
this typo made it into one of the commit messages too where we try to be strict

> Link to task: 
> http://wiki.libvirt.org/page/BiteSizedTasks#More_usage_of_virGetLastErrorMessage.28.29

Code looks fine, but I'm being picky about some formatting things :) Bring on
the v3

Thanks,
Cole

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

Re: [libvirt] [PATCH v2 2/8] Release address in function granularity than slot

Since you have to unplug all the functions in a slot at the same time 
anyway, I don't see the point in reverting this commit - you'll just end 
up needing to call  it multiple times - once for each function that was 
in the slot.


(just guessing without looking at the code - perhaps it's already being 
called from within lower level functions for each device, and each 
device gets its own notification from qemu that it's been detached? Or 
to ask a more specific question - exactly what happens with device 
detach? Do you send qemu a single detach command and it detaches all the 
functions as a single unit? Or do you send it multiple detach commands, 
with function 0 being the last?)



On 05/18/2016 05:30 PM, Shivaprasad G Bhat wrote:

The commit 6fe678c is reverted. The code is moved around and cant revert
staright.

Signed-off-by: Shivaprasad G Bhat 
---
  src/conf/domain_addr.c |   22 +-
  src/libvirt_private.syms   |1 +
  src/qemu/qemu_domain_address.c |2 +-
  3 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index acd8ce6..35c7cd4 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -472,21 +472,17 @@ virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr 
addrs,
  goto cleanup;
  
  if (dev->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {

-/* We do not support hotplug multi-function PCI device now, so we 
should
- * reserve the whole slot. The function of the PCI device must be 0.
- */
-if (dev->addr.pci.function != 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Only PCI device addresses with function=0"
- " are supported"));
-goto cleanup;
-}
+if (((dev->addr.pci.function == 0) && (dev->addr.pci.multi == 
VIR_TRISTATE_SWITCH_ON)) ||
+dev->addr.pci.function != 0) {
  
-if (!virDomainPCIAddressValidate(addrs, &dev->addr.pci,

- addrStr, flags, true))
-goto cleanup;
+if (!virDomainPCIAddressValidate(addrs, &dev->addr.pci,
+ addrStr, flags, true))
+goto cleanup;
  
-ret = virDomainPCIAddressReserveSlot(addrs, &dev->addr.pci, flags);

+ret = virDomainPCIAddressReserveAddr(addrs, &dev->addr.pci, flags, 
false, true);
+} else {
+ret = virDomainPCIAddressReserveSlot(addrs, &dev->addr.pci, flags);
+}
  } else {
  ret = virDomainPCIAddressReserveNextSlot(addrs, dev, flags);
  }
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fb24808..e4953b7 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -98,6 +98,7 @@ virDomainPCIAddressBusSetModel;
  virDomainPCIAddressEnsureAddr;
  virDomainPCIAddressFlagsCompatible;
  virDomainPCIAddressGetNextSlot;
+virDomainPCIAddressReleaseAddr;
  virDomainPCIAddressReleaseSlot;
  virDomainPCIAddressReserveAddr;
  virDomainPCIAddressReserveNextSlot;
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 9c8c262..1e7d98c 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1682,7 +1682,7 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm,
   NULLSTR(devstr));
  else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
   virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
- virDomainPCIAddressReleaseSlot(priv->pciaddrs,
+ virDomainPCIAddressReleaseAddr(priv->pciaddrs,
  &info->addr.pci) < 0)
  VIR_WARN("Unable to release PCI address on %s",
   NULLSTR(devstr));

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



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

Re: [libvirt] [PATCH 3/3] virGetLastErrorMessage in nodeinfotest

I suggest:

  tests: nodeinfotest: Convert to virGetLastErrorMessage()

Also, make this patch #1. So patch one adjusts this specific case, then patch
#2 adjusts the rest of tests/, then patch #3 adjusts the rest of the code. It
cascades nicely :)

On 05/19/2016 01:07 PM, Jovanka Gulicoska wrote:
> Use virGetLastErrorMessage in nodeinfotest.c to preserve the behavior
> in previous patches

No need to mention the file in the commit body, it's in the subject and the
diff. I suggest

  Remove unnecessary virSaveLastError() usage and convert to
  virGetLastErrorMessage()

> ---
>  tests/nodeinfotest.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c
> index d8eace5..cc74ab5 100644
> --- a/tests/nodeinfotest.c
> +++ b/tests/nodeinfotest.c
> @@ -44,10 +44,10 @@ linuxTestCompareFiles(char *sysfs_prefix,
>  memset(&nodeinfo, 0, sizeof(nodeinfo));
>  if (linuxNodeInfoCPUPopulate(sysfs_prefix, cpuinfo, arch, &nodeinfo) < 
> 0) {
>  if (virTestGetDebug()) {
> -virErrorPtr error = virSaveLastError();
> -if (error && error->code != VIR_ERR_OK)
> -VIR_TEST_DEBUG("\n%s\n", error->message);
> -virFreeError(error);
> +char const *msg = virGetLastErrorMessage();
> +

We use 'const char *' in the code much more often than this format... they are
functionally identical but better to stick with conventions

Thanks,
Cole

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

Re: [libvirt] [PATCH v2 4/8] Introduce PCI Multifunction device parser


On 05/18/2016 05:31 PM, Shivaprasad G Bhat wrote:

This patch just introduces the parser function used by
the later patches. The parser disallows hostdevices to be
used with other virtio devices simultaneously.


Why? (and I think you mean "emulated" not "virtio")




Signed-off-by: Shivaprasad G Bhat 
---
  src/conf/domain_conf.c   |  236 ++
  src/conf/domain_conf.h   |   22 
  src/libvirt_private.syms |3 +
  3 files changed, 261 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ed0c471..e946147 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -860,6 +860,36 @@ virDomainXMLOptionClassDispose(void *obj)
  (xmlopt->config.privFree)(xmlopt->config.priv);
  }
  
+/* virDomainDeviceDefListAddCopy - add a *copy* of the device to this list */

+int
+virDomainDeviceDefListAddCopy(virDomainDeviceDefListPtr list,
+  virDomainDeviceDefPtr dev,
+  const virDomainDef *def,
+  virCapsPtr caps,
+  virDomainXMLOptionPtr xmlopt)
+{
+virDomainDeviceDefPtr copy = virDomainDeviceDefCopy(dev, def, caps, 
xmlopt);
+
+if (!copy)
+return -1;
+if (VIR_APPEND_ELEMENT(list->devs, list->count, copy) < 0) {
+virDomainDeviceDefFree(copy);
+return -1;
+}
+return 0;
+}
+
+void virDomainDeviceDefListDispose(virDomainDeviceDefListPtr list)
+{
+size_t i;
+
+if (!list)
+return;
+for (i = 0; i < list->count; i++)
+virDomainDeviceDefFree(list->devs[i]);
+VIR_FREE(list);
+}



This isn't a vir*Dispose() function, it is a vir*Free() function. the 
Dispose() functions are used for virObject-based objects, and take a 
void *obj as their argument.




+
  /**
   * virDomainKeyWrapCipherDefParseXML:
   *
@@ -24365,3 +24395,209 @@ virDomainObjGetShortName(virDomainObjPtr vm)
  
  return ret;

  }
+
+static int
+virDomainPCIMultifunctionDeviceDefParseXML(xmlXPathContextPtr ctxt,
+   const virDomainDef *def,
+   virDomainDeviceDefListPtr devlist,
+   virCapsPtr caps,
+   virDomainXMLOptionPtr xmlopt,
+   unsigned int flags)
+{


Instead of all these loops for each type of device. I think it would 
make more sense to separate virDomainDeviceDefParse() so that the 
original function was:


 virDomainDeviceDefParse()
 {
  ...
  if (!(xml = virXMLParseString(..)))
  return -1;
  node = ctxt->node;

 dev = virDomainDeviceDefParseXML(node, ctxt, def, caps, 
xmlopt, flags)))

 xmlFreeDoc(xml)
 xmlXPathFreeContext(ctxt);
 return dev;
}

with a new function virDomainDeviceDefParseXML() that contained all the 
rest of the insides of the original function. You could then call this 
new function for each node of the xmlDocPtr that you get after parsing 
the input to your new "parse multiple devices" function (which could 
maybe be called virDomainDeviceDefParseMany()). After all of the devices 
are parsed, then you can validate that they are all PCI devices, and 
each for a different function of the same slot (if an address has been 
assigned).



+size_t i, j;
+int n;
+virDomainDeviceDef device;
+xmlNodePtr *nodes = NULL;
+char *netprefix = NULL;
+int nhostdevs = 0;
+
+device.type = VIR_DOMAIN_DEVICE_DISK;
+
+if ((n = virXPathNodeSet("./disk", ctxt, &nodes)) < 0)
+goto error;
+
+for (i = 0; i < n; i++) {
+virDomainDiskDefPtr disk = virDomainDiskDefParseXML(xmlopt,
+nodes[i],
+ctxt,
+NULL,
+def->seclabels,
+def->nseclabels,
+flags);
+if (!disk)
+goto error;
+
+device.data.disk = disk;
+if (virDomainDeviceDefListAddCopy(devlist, &device, def, caps, xmlopt) 
< 0)
+goto error;
+VIR_FREE(disk);
+}
+VIR_FREE(nodes);
+
+device.type = VIR_DOMAIN_DEVICE_CONTROLLER;
+/* analysis of the controller devices */
+if ((n = virXPathNodeSet("./controller", ctxt, &nodes)) < 0)
+goto error;
+
+for (i = 0; i < n; i++) {
+virDomainControllerDefPtr controller = 
virDomainControllerDefParseXML(nodes[i],
+  
ctxt,
+  
flags);
+if (!controller)
+

Re: [libvirt] [PATCH 2/3] More usage of virGetLastErrorMessage()

Since this one touches lots of different drivers, lack of a prefix is fine IMO

On 05/19/2016 01:07 PM, Jovanka Gulicoska wrote:
> Relace virGetLastError() with virGetLastErrorMessage() in drivers.

*Replace

'in drivers' isn't entirely accurate for the files it is touching. I suggest

  Convert to virGetLastErrorMessage() in the rest of the code

- Cole

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

Re: [libvirt] [PATCH 1/3] virGetLastErrorMessage() in tests

I think the subject should be

  tests: Convert to virGetLastErrorMessage()

or

  tests: More usage of virGetLastErrorMessage()

Most commits subjects use a prefix describing what part of the code they are
touching

On 05/19/2016 01:07 PM, Jovanka Gulicoska wrote:
> Use virGetLastErrorMessage() insted of virGetLastError() in tests

*instead


Thanks,
Cole

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

Re: [libvirt] [PATCH v2 1/8] Revert "prevent hot unplugging multi function PCI device"

If you're going to say in the commit that you're reverting something, 
you should list the commit id of the patch you're reverting (even if, as 
in this case, the revert couldn't be done with "git revert").


Usually, though, a revert is done to remove something that shouldn't 
have been committed in the first place, while these bits were correct.


Of course I think that after review it would be more proper to squash 
this patch into the one that actually enables hot-unplug of 
multifunction devices. ACK on what it does, though.


On 05/18/2016 05:29 PM, Shivaprasad G Bhat wrote:

This has to go. The unlugging is going to be supported.

Signed-off-by: Shivaprasad G Bhat 
---
  src/qemu/qemu_hotplug.c |   60 ---
  1 file changed, 60 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index f40b34d..5b822f9 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2770,35 +2770,6 @@ qemuDomainChangeGraphics(virQEMUDriverPtr driver,
  return ret;
  }
  
-

-static int qemuComparePCIDevice(virDomainDefPtr def ATTRIBUTE_UNUSED,
-virDomainDeviceDefPtr device ATTRIBUTE_UNUSED,
-virDomainDeviceInfoPtr info1,
-void *opaque)
-{
-virDomainDeviceInfoPtr info2 = opaque;
-
-if (info1->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI ||
-info2->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
-return 0;
-
-if (info1->addr.pci.domain == info2->addr.pci.domain &&
-info1->addr.pci.bus == info2->addr.pci.bus &&
-info1->addr.pci.slot == info2->addr.pci.slot &&
-info1->addr.pci.function != info2->addr.pci.function)
-return -1;
-return 0;
-}
-
-static bool qemuIsMultiFunctionDevice(virDomainDefPtr def,
-  virDomainDeviceInfoPtr dev)
-{
-if (virDomainDeviceInfoIterate(def, qemuComparePCIDevice, dev) < 0)
-return true;
-return false;
-}
-
-
  static int
  qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
@@ -3407,13 +3378,6 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
  int ret = -1;
  qemuDomainObjPrivatePtr priv = vm->privateData;
  
-if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) {

-virReportError(VIR_ERR_OPERATION_FAILED,
-   _("cannot hot unplug multifunction PCI device: %s"),
-   detach->dst);
-goto cleanup;
-}
-
  if (qemuDomainMachineIsS390CCW(vm->def) &&
  virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) {
  if (!virDomainDeviceAddressIsValid(&detach->info,
@@ -3636,14 +3600,6 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr 
driver,
  goto cleanup;
  }
  
-if (detach->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&

-qemuIsMultiFunctionDevice(vm->def, &detach->info)) {
-virReportError(VIR_ERR_OPERATION_FAILED,
-   _("cannot hot unplug multifunction PCI device: %s"),
-   dev->data.disk->dst);
-goto cleanup;
-}
-
  if (qemuDomainControllerIsBusy(vm, detach)) {
  virReportError(VIR_ERR_OPERATION_FAILED, "%s",
 _("device cannot be detached: device is busy"));
@@ -3679,17 +3635,8 @@ qemuDomainDetachHostPCIDevice(virQEMUDriverPtr driver,
virDomainHostdevDefPtr detach)
  {
  qemuDomainObjPrivatePtr priv = vm->privateData;
-virDomainHostdevSubsysPCIPtr pcisrc = &detach->source.subsys.u.pci;
  int ret;
  
-if (qemuIsMultiFunctionDevice(vm->def, detach->info)) {

-virReportError(VIR_ERR_OPERATION_FAILED,
-   _("cannot hot unplug multifunction PCI device: 
%.4x:%.2x:%.2x.%.1x"),
-   pcisrc->addr.domain, pcisrc->addr.bus,
-   pcisrc->addr.slot, pcisrc->addr.function);
-return -1;
-}
-
  if (!virDomainDeviceAddressIsValid(detach->info,
 VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
  virReportError(VIR_ERR_OPERATION_FAILED,
@@ -3921,13 +3868,6 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
  "%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 (!detach->info.alias) {


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



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

Re: [libvirt] RFC: what are the requirements to domain id

On 19.05.2016 17:56, Cole Robinson wrote:
> On 05/19/2016 10:47 AM, Nikolay Shirokovskiy wrote:
>> Hi, all.
>>
> 
> 
> 
>>
>> A more profound question is what is this id for?  How it is supposed to be 
>> used?
>>
> 
> I think most of the 'why' behind it is just that 'thats what xen does', since
> domain ID has been there since day 1 in libvirt, which was largely a duplicate
> of xen interfaces at the time.

Yeah, different drivers use it differently. For instance, LXC use it to
report PID of container's init, QEMU use it to report sequential ID of
domain, etc.

Michal

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

Re: [libvirt] [PATCH 1/6] conf: move virDomainDeviceInfo definition from domain_conf.h to device_conf.h

On Wed, May 18, 2016 at 03:23:49PM -0400, Laine Stump wrote:
> Also moves all the subordinate structs. This is necessary due to a new
> inline function that will be defined in device_conf.h, and also makes
> sense, because it is the *device* info that's in the struct. (Actually
> a lot more stuff from domain_conf.h could move to this newer file, but
> I didn't want to disturb any more than necessary).

This would be easier to read as a pure code motion, with the typedef
style change being separate.

Jan

> ---
>  src/conf/device_conf.h | 111 +-
>  src/conf/domain_conf.h | 129 
> -
>  2 files changed, 110 insertions(+), 130 deletions(-)

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

Re: [libvirt] [PATCH 0/6] auto-assign addresses when is specified

On Thu, May 19, 2016 at 01:15:28PM -0400, Cole Robinson wrote:
> On 05/18/2016 03:23 PM, Laine Stump wrote:
> > This is an alternative to Cole's series that permits  > type='pci'/> to force assignment of a PCI address, which is
> > particularly useful on platforms that could connect the same device in
> > different ways (e.g. aarch64/virt).
> > 
> > Here is Cole's last iteration of the series:
> > 
> >   https://www.redhat.com/archives/libvir-list/2016-May/msg01088.html
> > 
> > I had expressed a dislike of the "auto_allocate" flag that his series
> > temporarily adds to the address (while simultaneously changing the
> > address type to NONE) and suggested just changing all the necessary
> > places to check for a valid PCI address instead of just checking the
> > address type. He replied that this wasn't as simple as it looked, so I
> > decided to try it; turns out he's right. But I still like this method
> > better because it's not playing tricks with the address type, or
> > adding a temporary private attribute to what should be pure config
> > data.
> > 
> > Your opinion may vary though :-)
> > 

I like this series more than counting XML elements and temporarily
changing the types.

> > Note that patch 5/6 incorporates the same test case that Cole used in
> > his penultimate patch, and I've added his patch to check the case of
> > aarch64 at the end as well.
> > 
> 
> ACK series, but it's missing formatdomain.html.in changes. You can grab the
> check from my patch #2
> 
> I'm fine with this approach but I'll just list the downsides
> 
> - Less generalizable, for adding additional address types in the future, but
> that's hypothetical at this point
> - We don't raise an explicit error for drivers that don't support this type of
> address allocation, like libxl. If it matters we could add a domain XML parse
> feature to throw an explicit error though
> - checking info->type == DEVICE_ADDRESS_TYPE_PCI is now a suspect pattern and
> needs to be considered carefully in other parts of the code
> 
> upsides:
> - less magic
> - I think it will make allocating address at hotplug time simpler as well
> 

Yes, qemu_hotplug.c has a few of those places using == DEVICE_ADDRESS_TYPE_PCI
untouched by this series.

Jan

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

Re: [libvirt] [PATCH 2/6] conf: new functions to check if PCI address is wanted/present

On Wed, May 18, 2016 at 03:23:50PM -0400, Laine Stump wrote:
> In order to allow  with no other attributes to
> mean "I want a PCI address, but any PCI address will do" (just as
> having no  at all usually indicates), we will need to change
> several places in the code from a simple "info->type == (or !=)
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_(PCI|NONE)" into something slightly
> more complex, this patch adds to new functions that take a
> virDomainDeviceInfoPtr and return true/false depending on 1) whether
> the current state of the info indicates that we "want" a PCI address
> for this device (virDeviceInfoPCIAddressWanted()) and 2) whether this
> device already has a valid PCI address
> (virDeviceInfoPCIAddressPresent()).
> 
> Both of these functions required the simpler check for whether a pci
> address is "empty" (i.e. all of its attributes are 0, which can never
> happen in a real PCI address, since slot 0 of bus 0 of domain 0 is
> always reserved), so that function is also added.
> ---
>  src/conf/device_conf.h | 21 +
>  1 file changed, 21 insertions(+)
> 
> diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
> index 46c720d..847564b 100644
> --- a/src/conf/device_conf.h
> +++ b/src/conf/device_conf.h
> @@ -184,6 +184,27 @@ typedef struct _virDomainDeviceInfo {
>  int virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr addr,
> bool report);
>  
> +static inline bool

I think inline keywords are pointless nowadays. Some compilers will
inline the functions even if you don't want them to.

Jan

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

Re: [libvirt] Process to upstream new driver for proprietary hypervisor?

On 19.05.2016 17:19, Scott Bissett wrote:
> Hello,
> 
> I am currently implementing a libvirt driver for my company's proprietary 
> custom hypervisor. I have used the VMware driver as a design template, since 
> it is interfacing a closed-source hypervisor as well.
> 
> Ultimately, as we approach a minimally-usable implementation, we would like 
> to upstream our driver mods to libvirt source. What is the process to do so? 
> This is all new to me and my organization.
> 

Hey, glad to hear you want to implement new driver.

Firstly, I guess you are familiar with our driver structure already:

http://libvirt.org/api.html

Then, all you need to do is to create new subdir: src/$hypervisor and
place all your files there. Ideally,
src/$hypervisor/$hypervisor_driver.c would be the entry point for the
driver as it would will virHypervisorDriver structure with implemented APIs.

It's okay to look around how other drivers do it and mimic their
approach. The fact that the code is merged says we were okay with their
approach once.

> For some brief background info, I am maintaining a git repo of our new code, 
> plus ed scripts to modify libvirt source files. I am using quilt to create 
> patches which are given to dpkg-buildpackage to create our own .deb packages. 
> (I am working on Ubuntu at the moment; soon I will try to address repackaging 
> rpms for CentOS).
> 

ed scripts? Wow, this may be even bigger mystery than esx driver where
half of the driver is auto generated :-)

What we prefer are patches sent to the list (generated by git
format-patch and send by git send-email). It's all covered here:

http://libvirt.org/hacking.html

Yes, some items there are outdated (e.g. nobody generates patches by
'diff'), but use your gut feeling and you should do good.

Also, please make sure that 'make syntax-check' and 'make check' passes
after each single patch of yours. Happy coding!

Michal

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

Re: [libvirt] [PATCH 0/6] auto-assign addresses when is specified

On 05/18/2016 03:23 PM, Laine Stump wrote:
> This is an alternative to Cole's series that permits  type='pci'/> to force assignment of a PCI address, which is
> particularly useful on platforms that could connect the same device in
> different ways (e.g. aarch64/virt).
> 
> Here is Cole's last iteration of the series:
> 
>   https://www.redhat.com/archives/libvir-list/2016-May/msg01088.html
> 
> I had expressed a dislike of the "auto_allocate" flag that his series
> temporarily adds to the address (while simultaneously changing the
> address type to NONE) and suggested just changing all the necessary
> places to check for a valid PCI address instead of just checking the
> address type. He replied that this wasn't as simple as it looked, so I
> decided to try it; turns out he's right. But I still like this method
> better because it's not playing tricks with the address type, or
> adding a temporary private attribute to what should be pure config
> data.
> 
> Your opinion may vary though :-)
> 
> Note that patch 5/6 incorporates the same test case that Cole used in
> his penultimate patch, and I've added his patch to check the case of
> aarch64 at the end as well.
> 

ACK series, but it's missing formatdomain.html.in changes. You can grab the
check from my patch #2

I'm fine with this approach but I'll just list the downsides

- Less generalizable, for adding additional address types in the future, but
that's hypothetical at this point
- We don't raise an explicit error for drivers that don't support this type of
address allocation, like libxl. If it matters we could add a domain XML parse
feature to throw an explicit error though
- checking info->type == DEVICE_ADDRESS_TYPE_PCI is now a suspect pattern and
needs to be considered carefully in other parts of the code

upsides:
- less magic
- I think it will make allocating address at hotplug time simpler as well

Thanks,
Cole

> 
> Cole Robinson (1):
>   tests: qemu: test  with aarch64
> 
> Laine Stump (5):
>   conf: move virDomainDeviceInfo definition from domain_conf.h to
> device_conf.h
>   conf: new functions to check if PCI address is wanted/present
>   conf: allow type='pci' addresses with no address attributes specified
>   bhyve: auto-assign addresses when  is specified
>   qemu: auto-assign addresses when  is specified
> 
>  docs/schemas/basictypes.rng|   8 +-
>  src/bhyve/bhyve_device.c   |  10 +-
>  src/conf/device_conf.c |   6 +-
>  src/conf/device_conf.h | 132 
> -
>  src/conf/domain_addr.c |   2 +-
>  src/conf/domain_conf.c |  13 +-
>  src/conf/domain_conf.h | 129 
>  src/qemu/qemu_domain_address.c |  64 +-
>  ...l2argv-aarch64-virtio-pci-manual-addresses.args |   4 +-
>  ...ml2argv-aarch64-virtio-pci-manual-addresses.xml |   5 +
>  .../qemuxml2argv-pci-autofill-addr.args|  25 
>  .../qemuxml2argv-pci-autofill-addr.xml |  35 ++
>  tests/qemuxml2argvtest.c   |   1 +
>  ...2xmlout-aarch64-virtio-pci-manual-addresses.xml |   5 +
>  .../qemuxml2xmlout-pci-autofill-addr.xml   |  41 +++
>  tests/qemuxml2xmltest.c|   1 +
>  16 files changed, 298 insertions(+), 183 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-autofill-addr.xml
> 

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

Re: [libvirt] [PATCH 0/8] lxc: add job support

On 05/19/2016 12:48 PM, Michal Privoznik wrote:
> On 18.05.2016 16:53, Michal Privoznik wrote:
>> On 16.05.2016 23:36, Katerina Koukiou wrote:
>>> This patch series adds job support to the lxc driver, using techiques from 
>>> the
>>> libxl driver. One benefit is no longer blocking get operations during long
>>> running modify operations.  E.g. with these patches 'vish dominfo dom' will
>>> work while 'virsh save dom ...' is in progress.
>>>
>>> The first patch adds the job support machinery, followed by several patches
>>> that make use of it.
>>> Although this might look not needed that much right now, it is preparing
>>> environment for future work.
>>>
>>> This patch series has been reviewed by Michal Privoznik who is my mentor
>>> for GSoC 2016.
>>>
>>>
>>> Katerina Koukiou (8):
>>>   lxc: Add job support to lxc driver
>>>   lxc: use job functions in lxcDomain{CreateXMLWithFiles,
>>> CreateWithFiles}
>>>   lxc: use job functions in lxcDomainSetMemoryFlags
>>>   lxc: use job functions in lxcDomain{Suspend, Resume}
>>>   lxc: use job functions in lxcDomain{AttachDeviceFlags,
>>> DetachDeviceFlags, UpdateDeviceFlags}
>>>   lxc: add job functions in lxcDomainSetAutostart
>>>   lxc: use job functions in lxcDomain* functions that do query
>>> operations.
>>>   lxc: use job functions in lxcDomain* functions that perform modify
>>> actions.
>>>
>>>  src/lxc/lxc_domain.c | 154 --
>>>  src/lxc/lxc_domain.h |  37 ++
>>>  src/lxc/lxc_driver.c | 351 
>>> ---
>>>  3 files changed, 428 insertions(+), 114 deletions(-)
>>>
>>
>> ACK series, although, I'll leave others time to chime in before pushing.
> 
> Okay, nobody objected, so I've pushed these. Congratulations on your
> first libvirt contribution!
> 

I had glanced over them and they looked quite good to me. Nice work Katerina!

- Cole

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

[libvirt] [PATCH 1/3] virGetLastErrorMessage() in tests

Use virGetLastErrorMessage() insted of virGetLastError() in tests
---
 tests/commandtest.c | 81 +++--
 tests/libvirtdconftest.c| 26 +++
 tests/openvzutilstest.c |  7 ++--
 tests/qemucapsprobe.c   |  6 ++--
 tests/securityselinuxtest.c |  6 ++--
 5 files changed, 46 insertions(+), 80 deletions(-)

diff --git a/tests/commandtest.c b/tests/commandtest.c
index cf5f44a..6430e20 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -178,8 +178,7 @@ static int test2(const void *unused ATTRIBUTE_UNUSED)
 int ret;
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 virCommandFree(cmd);
 return -1;
 }
@@ -190,8 +189,7 @@ static int test2(const void *unused ATTRIBUTE_UNUSED)
 }
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 virCommandFree(cmd);
 return -1;
 }
@@ -218,8 +216,7 @@ static int test3(const void *unused ATTRIBUTE_UNUSED)
  VIR_COMMAND_PASS_FD_CLOSE_PARENT);
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 goto cleanup;
 }
 
@@ -260,8 +257,7 @@ static int test4(const void *unused ATTRIBUTE_UNUSED)
 virCommandDaemonize(cmd);
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 goto cleanup;
 }
 
@@ -294,8 +290,7 @@ static int test5(const void *unused ATTRIBUTE_UNUSED)
 virCommandAddEnvPassCommon(cmd);
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 virCommandFree(cmd);
 return -1;
 }
@@ -318,8 +313,7 @@ static int test6(const void *unused ATTRIBUTE_UNUSED)
 virCommandAddEnvPassBlockSUID(cmd, "DOESNOTEXIST", NULL);
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 virCommandFree(cmd);
 return -1;
 }
@@ -343,8 +337,7 @@ static int test7(const void *unused ATTRIBUTE_UNUSED)
 virCommandAddEnvPassBlockSUID(cmd, "DOESNOTEXIST", NULL);
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 virCommandFree(cmd);
 return -1;
 }
@@ -368,8 +361,7 @@ static int test8(const void *unused ATTRIBUTE_UNUSED)
 virCommandAddEnvPair(cmd, "USER", "test");
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 virCommandFree(cmd);
 return -1;
 }
@@ -406,8 +398,7 @@ static int test9(const void *unused ATTRIBUTE_UNUSED)
 }
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 virCommandFree(cmd);
 return -1;
 }
@@ -432,8 +423,7 @@ static int test10(const void *unused ATTRIBUTE_UNUSED)
 virCommandAddArgSet(cmd, args);
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 virCommandFree(cmd);
 return -1;
 }
@@ -456,8 +446,7 @@ static int test11(const void *unused ATTRIBUTE_UNUSED)
 virCommandPtr cmd = virCommandNewArgs(args);
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());
 virCommandFree(cmd);
 return -1;
 }
@@ -478,8 +467,7 @@ static int test12(const void *unused ATTRIBUTE_UNUSED)
 virCommandSetInputBuffer(cmd, "Hello World\n");
 
 if (virCommandRun(cmd, NULL) < 0) {
-virErrorPtr err = virGetLastError();
-printf("Cannot run child %s\n", err->message);
+printf("Cannot run child %s\n", virGetLastErrorMessage());

[libvirt] [PATCH 3/3] virGetLastErrorMessage in nodeinfotest

Use virGetLastErrorMessage in nodeinfotest.c to preserve the behavior
in previous patches
---
 tests/nodeinfotest.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c
index d8eace5..cc74ab5 100644
--- a/tests/nodeinfotest.c
+++ b/tests/nodeinfotest.c
@@ -44,10 +44,10 @@ linuxTestCompareFiles(char *sysfs_prefix,
 memset(&nodeinfo, 0, sizeof(nodeinfo));
 if (linuxNodeInfoCPUPopulate(sysfs_prefix, cpuinfo, arch, &nodeinfo) < 0) {
 if (virTestGetDebug()) {
-virErrorPtr error = virSaveLastError();
-if (error && error->code != VIR_ERR_OK)
-VIR_TEST_DEBUG("\n%s\n", error->message);
-virFreeError(error);
+char const *msg = virGetLastErrorMessage();
+
+if (msg)
+VIR_TEST_DEBUG("\n%s\n", msg);
 }
 VIR_FORCE_FCLOSE(cpuinfo);
 goto fail;
-- 
2.5.5

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

[libvirt] [PATCH 2/3] More usage of virGetLastErrorMessage()

Relace virGetLastError() with virGetLastErrorMessage() in drivers.
---
 daemon/libvirtd.c   |  8 ++--
 examples/object-events/event-test.c |  9 +++--
 src/bhyve/bhyve_driver.c|  3 +--
 src/conf/virsecretobj.c |  5 +
 src/libvirt.c   |  3 +--
 src/libxl/libxl_domain.c|  3 +--
 src/libxl/libxl_driver.c|  4 +---
 src/locking/lock_daemon.c   |  8 ++--
 src/logging/log_daemon.c|  8 ++--
 src/lxc/lxc_container.c |  9 +++--
 src/lxc/lxc_controller.c|  9 +++--
 src/lxc/lxc_domain.c|  4 ++--
 src/lxc/lxc_process.c   |  6 ++
 src/rpc/virnettlscontext.c  |  3 +--
 src/storage/storage_driver.c| 16 
 src/uml/uml_driver.c|  3 +--
 src/util/iohelper.c | 10 ++
 src/util/virhook.c  |  3 +--
 src/util/virhostdev.c   | 20 
 19 files changed, 41 insertions(+), 93 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index f24fb22..5617e42 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1315,12 +1315,8 @@ int main(int argc, char **argv) {
 /* Read the config file if it exists*/
 if (remote_config_file &&
 daemonConfigLoadFile(config, remote_config_file, implicit_conf) < 0) {
-virErrorPtr err = virGetLastError();
-if (err && err->message)
-VIR_ERROR(_("Can't load config file: %s: %s"),
-  err->message, remote_config_file);
-else
-VIR_ERROR(_("Can't load config file: %s"), remote_config_file);
+VIR_ERROR(_("Can't load config file: %s: %s"),
+  virGetLastErrorMessage(), remote_config_file);
 exit(EXIT_FAILURE);
 }
 
diff --git a/examples/object-events/event-test.c 
b/examples/object-events/event-test.c
index 2063536..c1ff4a7 100644
--- a/examples/object-events/event-test.c
+++ b/examples/object-events/event-test.c
@@ -917,9 +917,8 @@ main(int argc, char **argv)
 }
 
 if (virEventRegisterDefaultImpl() < 0) {
-virErrorPtr err = virGetLastError();
 fprintf(stderr, "Failed to register event implementation: %s\n",
-err && err->message ? err->message: "Unknown error");
+virGetLastErrorMessage());
 goto cleanup;
 }
 
@@ -972,17 +971,15 @@ main(int argc, char **argv)
 goto cleanup;
 
 if (virConnectSetKeepAlive(dconn, 5, 3) < 0) {
-virErrorPtr err = virGetLastError();
 fprintf(stderr, "Failed to start keepalive protocol: %s\n",
-err && err->message ? err->message : "Unknown error");
+virGetLastErrorMessage());
 run = 0;
 }
 
 while (run) {
 if (virEventRunDefaultImpl() < 0) {
-virErrorPtr err = virGetLastError();
 fprintf(stderr, "Failed to run event loop: %s\n",
-err && err->message ? err->message : "Unknown error");
+virGetLastErrorMessage());
 }
 }
 
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 441c666..c58286f 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -88,9 +88,8 @@ bhyveAutostartDomain(virDomainObjPtr vm, void *opaque)
 ret = virBhyveProcessStart(data->conn, data->driver, vm,
VIR_DOMAIN_RUNNING_BOOTED, 0);
 if (ret < 0) {
-virErrorPtr err = virGetLastError();
 VIR_ERROR(_("Failed to autostart VM '%s': %s"),
-  vm->def->name, err ? err->message : _("unknown error"));
+  vm->def->name, virGetLastErrorMessage());
 }
 }
 virObjectUnlock(vm);
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index 4babd31..c46d22c 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -990,11 +990,8 @@ virSecretLoadAllConfigs(virSecretObjListPtr secrets,
 continue;
 
 if (!(secret = virSecretLoad(secrets, de->d_name, path, configDir))) {
-virErrorPtr err = virGetLastError();
-
 VIR_ERROR(_("Error reading secret: %s"),
-  err != NULL ? err->message: _("unknown error"));
-virResetError(err);
+  virGetLastErrorMessage());
 VIR_FREE(path);
 continue;
 }
diff --git a/src/libvirt.c b/src/libvirt.c
index 114e88c..0e7e435 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -770,10 +770,9 @@ virStateInitialize(bool privileged,
 if (virStateDriverTab[i]->stateInitialize(privileged,
   callback,
   opaque) < 0) {
-virErrorPtr err = virGetLastError();
 VIR_ERROR(_("Initialization of %s state driver failed: %s"),

[libvirt] [PATCH 0/3] More usage of virGetLastErrorMessage()

v2 of patches
Use virGetLastErrorMessage() insted of virGetLastError()
Link to task: 
http://wiki.libvirt.org/page/BiteSizedTasks#More_usage_of_virGetLastErrorMessage.28.29

Jovanka Gulicoska (3):
  virGetLastErrorMessage() in tests
  More usage of virGetLastErrorMessage()
  virGetLastErrorMessage in nodeinfotest

 daemon/libvirtd.c   |  8 +---
 examples/object-events/event-test.c |  9 ++---
 src/bhyve/bhyve_driver.c|  3 +-
 src/conf/virsecretobj.c |  5 +--
 src/libvirt.c   |  3 +-
 src/libxl/libxl_domain.c|  3 +-
 src/libxl/libxl_driver.c|  4 +-
 src/locking/lock_daemon.c   |  8 +---
 src/logging/log_daemon.c|  8 +---
 src/lxc/lxc_container.c |  9 ++---
 src/lxc/lxc_controller.c|  9 ++---
 src/lxc/lxc_domain.c|  4 +-
 src/lxc/lxc_process.c   |  6 +--
 src/rpc/virnettlscontext.c  |  3 +-
 src/storage/storage_driver.c| 16 ++--
 src/uml/uml_driver.c|  3 +-
 src/util/iohelper.c | 10 +
 src/util/virhook.c  |  3 +-
 src/util/virhostdev.c   | 20 -
 tests/commandtest.c | 81 +
 tests/libvirtdconftest.c| 26 ++--
 tests/nodeinfotest.c|  8 ++--
 tests/openvzutilstest.c |  7 +---
 tests/qemucapsprobe.c   |  6 +--
 tests/securityselinuxtest.c |  6 +--
 25 files changed, 91 insertions(+), 177 deletions(-)

-- 
2.5.5

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

[libvirt] xsave-related CPU features implementation

2016-05-19 Thread Alexander Burluka

Hello,

I would like to add support for the next CPU features which presents in
QEMU - xsaveopt, xsavec, xgetbv1, xsaves. These features were introduced
in QEMU commit 0bb0b2d2fe7f645ddaf1f0ff40ac669c9feb4aa1.
I was confused by comment in src/cpu/cpu_map.xml:
...

...
Is this comment is still actual? This feature set needs ecx 0x1.
I can see that it was a temporary solution in commit by Peter Krempa
012f9b19ef3812884e207dc431571502de4cebce. Does these features addtion
require CPU detection improvement? Or can I simply put them into 
src/cpu/cpu_map.xml?

Thank you

-- 
Regards,
Alexander Burluka

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

[libvirt] [PATCH] tests: Add forgotten backslash

While introducing virtestmock.la, I've forgotten to add '\' at
the end of one line leaving our Makefile.am mangled. Fortunately,
the only thing that comes after is '$(NULL)' so nothing is
terribly broken.

Signed-off-by: Michal Privoznik 
---

Pushed under trivial rule.

 tests/Makefile.am | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 238f6da..c7c9a03 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -447,7 +447,7 @@ endif WITH_DBUS
 if WITH_LINUX
 test_libraries += virusbmock.la \
virnetdevbandwidthmock.la \
-   virtestmock.la
+   virtestmock.la \
$(NULL)
 endif WITH_LINUX
 
-- 
2.8.1

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

Re: [libvirt] [PATCH 0/8] lxc: add job support

On 18.05.2016 16:53, Michal Privoznik wrote:
> On 16.05.2016 23:36, Katerina Koukiou wrote:
>> This patch series adds job support to the lxc driver, using techiques from 
>> the
>> libxl driver. One benefit is no longer blocking get operations during long
>> running modify operations.  E.g. with these patches 'vish dominfo dom' will
>> work while 'virsh save dom ...' is in progress.
>>
>> The first patch adds the job support machinery, followed by several patches
>> that make use of it.
>> Although this might look not needed that much right now, it is preparing
>> environment for future work.
>>
>> This patch series has been reviewed by Michal Privoznik who is my mentor
>> for GSoC 2016.
>>
>>
>> Katerina Koukiou (8):
>>   lxc: Add job support to lxc driver
>>   lxc: use job functions in lxcDomain{CreateXMLWithFiles,
>> CreateWithFiles}
>>   lxc: use job functions in lxcDomainSetMemoryFlags
>>   lxc: use job functions in lxcDomain{Suspend, Resume}
>>   lxc: use job functions in lxcDomain{AttachDeviceFlags,
>> DetachDeviceFlags, UpdateDeviceFlags}
>>   lxc: add job functions in lxcDomainSetAutostart
>>   lxc: use job functions in lxcDomain* functions that do query
>> operations.
>>   lxc: use job functions in lxcDomain* functions that perform modify
>> actions.
>>
>>  src/lxc/lxc_domain.c | 154 --
>>  src/lxc/lxc_domain.h |  37 ++
>>  src/lxc/lxc_driver.c | 351 
>> ---
>>  3 files changed, 428 insertions(+), 114 deletions(-)
>>
> 
> ACK series, although, I'll leave others time to chime in before pushing.

Okay, nobody objected, so I've pushed these. Congratulations on your
first libvirt contribution!

Michal

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

Re: [libvirt] [PATCH 0/4] Remove need for STATIC_ANALYSIS in viralloc.h

On Wed, May 18, 2016 at 08:07:52AM -0400, John Ferlan wrote:
> Not sure which release exactly, but the coverity analysis bug I filed that
> was the reason for commit id 'c9a85af31' has been fixed, so it's time to
> remove that... In doing so uncovered another issue - it seems the fix for
> Coverity addressed the primary problem we've seen, but when there's functions
> returning allocated strings by reference that can also VIR_FREE(*string),
> whatever was added to address the main issue doesn't seem to recognize this
> other usage, resulinting in a false positive resource_leak. Luckily there's
> only two instances in our code for that. One is addressed by patch 3 and
> the other in virPCIDeviceGetDriverPathAndName which would require quite
> a few more changes to address (essentially split up the function - it'll
> be on my "to do" list).
> 
> Patch 1:
> Addresses an issue seen once patches 2-4 were applied - that there's a
> real problem with the error path.  It's a simple fix.
> 
> Patches 2 & 3:
> Address a false positive resource leak even with patch 4 applied.
> 
> Patch 4:
> Remove the rather ugly !STATIC_ANALYSIS within the VIR_FREE and VIR_DISPOSE*
> macros. As I found with a build after the fact, VIR_DISPOSE_STRING was missing
> an argument anyway (it should have been ": 0, 1, NULL" instead of ": 1, 
> NULL").
> 

Nice to see it go.

> NB: Patch 4 has been run through the private coverity server...
> 
> John Ferlan (4):
>   util: Fix error path for virPCIGetVirtualFunctions
>   util: Remove need for ret in virPCIGetPhysicalFunction
>   util: Adjust return for virPCIGetDeviceAddressFromSysfsLink
>   util: Remove need for STATIC_ANALYSIS check
> 
>  src/util/viralloc.h | 34 ++
>  src/util/virpci.c   | 39 +++
>  2 files changed, 25 insertions(+), 48 deletions(-)
> 

ACK series

Jan

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

Re: [libvirt] RFC: what are the requirements to domain id

On 05/19/2016 10:47 AM, Nikolay Shirokovskiy wrote:
> Hi, all.
> 

> 
> A more profound question is what is this id for?  How it is supposed to be 
> used?
> 

I think most of the 'why' behind it is just that 'thats what xen does', since
domain ID has been there since day 1 in libvirt, which was largely a duplicate
of xen interfaces at the time.

- Cole

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

[libvirt] Process to upstream new driver for proprietary hypervisor?

2016-05-19 Thread Scott Bissett

Hello,

I am currently implementing a libvirt driver for my company's proprietary 
custom hypervisor. I have used the VMware driver as a design template, since it 
is interfacing a closed-source hypervisor as well.

Ultimately, as we approach a minimally-usable implementation, we would like to 
upstream our driver mods to libvirt source. What is the process to do so? This 
is all new to me and my organization.

For some brief background info, I am maintaining a git repo of our new code, 
plus ed scripts to modify libvirt source files. I am using quilt to create 
patches which are given to dpkg-buildpackage to create our own .deb packages. 
(I am working on Ubuntu at the moment; soon I will try to address repackaging 
rpms for CentOS).

I appreciate any tips, pointers, suggestions, etc.

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

Re: [libvirt] RFC: what are the requirements to domain id

On Thu, May 19, 2016 at 05:47:05PM +0300, Nikolay Shirokovskiy wrote:
> Hi, all.
> 
>   Domain id is somewhat required feature for hypervisors: virsh list prints
> them by default and active/inactive distinction is based on this id value
> analysys. But other than -1 is reserved for inactive domains there is no
> further requirements for it, one can happily keep them 0 for active domains.
> 
>   Looks like some drivers use process pids for this purpose. Qemu driver use
> its own incrementing counter and domain ids endure libvirtd restart. Vz do not
> provide a suitable value so the question arises can some simple id
> implementation be good enough? By simple I mean to keep counter as long as
> daemon lives and do not introduce any additional persistent state. As I said
> before there is basically no option to not to support it.
> 
> A more profound question is what is this id for?  How it is supposed to be 
> used?

I guess it isn't formally specified but the requirements are thus:

 * -1: all inactive domains must report this value
 * 0: only permitted for control plane domain (eg Xen Domain-0 or equiv)
 * > 0: must be unique for each running domain on the hypervisor connection

There is no rule about how domain ID values must be assigned to guests,
and the value must only persist for as long as the guest is running. Next
time it boots, it is free to have a different ID value, or the same value,
as desired by the hypervisor impl. Similarly you can assign then incrementally
from 0, or based on some other ID like the PID.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

[libvirt] RFC: what are the requirements to domain id

2016-05-19 Thread Nikolay Shirokovskiy

Hi, all.

  Domain id is somewhat required feature for hypervisors: virsh list prints
them by default and active/inactive distinction is based on this id value
analysys. But other than -1 is reserved for inactive domains there is no
further requirements for it, one can happily keep them 0 for active domains.

  Looks like some drivers use process pids for this purpose. Qemu driver use
its own incrementing counter and domain ids endure libvirtd restart. Vz do not
provide a suitable value so the question arises can some simple id
implementation be good enough? By simple I mean to keep counter as long as
daemon lives and do not introduce any additional persistent state. As I said
before there is basically no option to not to support it.

A more profound question is what is this id for?  How it is supposed to be used?

Nikolay

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

Re: [libvirt] [PATCH 2/3] tests: Need help... Trying to add virRandomBytes mock



On 05/19/2016 10:18 AM, Peter Krempa wrote:
> On Wed, May 18, 2016 at 19:52:31 -0400, John Ferlan wrote:
>> What I'd like to do is merge this and the previous patch together;
>> however, I'm missing something (hopefully obvious) with the building
>> of the mocked function.
>>
>> Since this won't be a qemu specific function, the test wouldn't belong
>> in qemuxml2argvmock - rather a separate mock would seemingly have to be
>> created and then a separate virrandomtest; however, I'm lost in the
>> maze of Makefile magic - so hopefully someone can help... The current
>> code works, but generates a nefarious:
>>
>> *** Warning: Linking the executable virrandomtest against the loadable module
>> *** virrandommock.so is not portable!
>>
>> during the build...
> 
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 6ba6c5b..9d1a2ea 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -1085,7 +1085,6 @@ vircryptotest_LDADD = $(LDADDS)
>  virrandomtest_SOURCES = \
> virrandomtest.c testutils.h testutils.c
>  virrandomtest_LDADD = \
> -   virrandommock.la \
> ../src/libvirt.la \
> ../src/libvirt_util.la \
> $(GNULIB_LIBS)
 diff --git a/tests/virrandomtest.c b/tests/virrandomtest.c
> index 76e187c..bf1f46e 100644
> --- a/tests/virrandomtest.c
> +++ b/tests/virrandomtest.c
> @@ -73,7 +73,7 @@ mymain(void)
>  return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
>  }
> 
> -VIRT_TEST_MAIN(mymain)
> +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virrandommock.so")
> 

Ah - I see - thanks... I also see that means the Makefile.am can just be
$(LDADDS), too

>

Next problem - how does this then get "into" qemuxml2argvtest?  It
already pulls in qemuxml2argvmock.

I tried messing with virtTestRun to no avail.

I tried to add virrandommock.la in the qemuxml2argvtests LDADD list to
no avail.

Without the not so random number, the xml2argv test I've added fails.

This did work with the virRandomBytes and gnutls_rnd in qemuxml2argvmock

Tks -

John

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

Re: [libvirt] [PATCH v2 0/7] cpuGetModels: Create a NULL-terminated list

2016-05-19 Thread Pavel Hrdina

On Thu, May 19, 2016 at 01:07:01PM +0200, Jiri Denemark wrote:
> The list of CPU models is freed using virStringFreeList, which expects
> the list to by NULL-terminated.
> 
> Jiri Denemark (7):
>   cpu_x86: Use array of models in CPU map
>   cpu_x86: Use array of vendors in CPU map
>   cpu_x86: Use array of features in CPU map
>   cpu_ppc64: Use array of vendors in CPU map
>   cpu_ppc64: Use array of models in CPU map
>   cpu: Rework CPU map loading
>   cpu: Fix documentation of cpuGetModels

ACK series

Pavel

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

Re: [libvirt] hotplug support for "filesystem" devices?

On 05/18/2016 12:20 PM, Snyder, Emile wrote:
> Hello all,
> 
> I'm a newbie to libvirt, coming to it because I'm trying to understand how to 
> provide the ability to attach/detach network filesystem shares to openstack 
> guest instances in something analogous to the way block devices can be now.
> 
> While playing with the existing virtfs/9p support for qemu, I found that I 
> can't use virsh to attach a "filesystem" device while the VM is running. For 
> example, I create a snippet of XML like
> 
> 
>   
>   
> 
> 
> and then try:
> stack@devstack-8366:~/devstack$ virsh list
>  IdName   State
> 
>  3 instance-0002  running
>  5 instance-0004  running
>  8 instance-0007  running
> 
> stack@devstack-8366:~/devstack$ virsh attach-device 8 share.xml
> error: Failed to attach device from share.xml
> error: Operation not supported: live attach of device 'filesystem' is not 
> supported
> 
> 
> I see that error coming from src/qemu/qemu_driver.c in the 
> qemuDomainAttachDeviceLive function.
> 
> Can anyone tell me if this is something that is going to be hard or easy to 
> make possible? Pointers to where I should look in the source, or in qemu? Is 
> anyone working it already?
> 

Unfortunately qemu currently lacks the support for virtio 9pfs hotplug AFAICT.
There's two aspects: there's the filesystem source bit, which is the qemu
-fsdev option, and then there's the guest facing -device virtio-9p*.

The latter bit is hotpluggable, there were qemu patches that said as much in
the last year. But there's no infrastructure for hotplugging fsdevs. So any
work to enable this in libvirt would have to start there.

- Cole

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

Re: [libvirt] [PATCH 2/2] qemu_cgroup: allow access to /dev/dri/render*

On Thu, May 19, 2016 at 04:12:52PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > $ ls -lZ /dev/dri/
> > total 0
> > crw-rw+ 1 root video system_u:object_r:dri_device_t:s0 226,   0 May 18
> > 19:17 card0
> > crw---. 1 root video system_u:object_r:dri_device_t:s0 226,  64 May 18
> > 19:17 controlD64
> > crw-rw+ 1 root video system_u:object_r:dri_device_t:s0 226, 128 May 18
> > 19:17 renderD128
> > 
> > qemu itself loops over /dev/dri, grabs the first matching renderD* that it 
> > can
> > open, then does its magic. Some questions I have in general:
> > 
> > - is there only ever one render node? or one per video card?
> 
> One per video card.

Is there a way to tell QEMU which video card to use ? If so we need to
somehow represent this in libvirt.

> > - is it okay to use the same node for multiple VMs simultaneously?
> 
> Yes.

Presumably they're going to compete for execution time and potentially
VRAM at least ? I assume they have 100% security isolation from each
other though.  IOW, permissioning is really just there to prevent a
rogue processes from doing denial of service  on the GPU resources,
rather than actively compromising other users of the GPU ?

> > Maybe the long term fix is to have libvirt pass in a pre-opened fd for
> > qemu:///system, since I don't know if it would be acceptable to chown
> > qemu:qemu on the render node, but maybe we use setfacl instead.
> 
> chown isn't a good idea I think.  But doesn't use libvirt setfacl anyway
> for simliar things (i.e. /dev/bus/usb/... for usb pass-through) ?

No, we exclusively switch access to QEMU only.

Obviously the DRI stuff is different as we expected the host OS to
have continued concurrent use of the video card.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH 2/3] tests: Need help... Trying to add virRandomBytes mock

2016-05-19 Thread Peter Krempa

On Wed, May 18, 2016 at 19:52:31 -0400, John Ferlan wrote:
> What I'd like to do is merge this and the previous patch together;
> however, I'm missing something (hopefully obvious) with the building
> of the mocked function.
> 
> Since this won't be a qemu specific function, the test wouldn't belong
> in qemuxml2argvmock - rather a separate mock would seemingly have to be
> created and then a separate virrandomtest; however, I'm lost in the
> maze of Makefile magic - so hopefully someone can help... The current
> code works, but generates a nefarious:
> 
> *** Warning: Linking the executable virrandomtest against the loadable module
> *** virrandommock.so is not portable!
> 
> during the build...

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 6ba6c5b..9d1a2ea 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1085,7 +1085,6 @@ vircryptotest_LDADD = $(LDADDS)
 virrandomtest_SOURCES = \
virrandomtest.c testutils.h testutils.c
 virrandomtest_LDADD = \
-   virrandommock.la \
../src/libvirt.la \
../src/libvirt_util.la \
$(GNULIB_LIBS)
diff --git a/tests/virrandomtest.c b/tests/virrandomtest.c
index 76e187c..bf1f46e 100644
--- a/tests/virrandomtest.c
+++ b/tests/virrandomtest.c
@@ -73,7 +73,7 @@ mymain(void)
 return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }

-VIRT_TEST_MAIN(mymain)
+VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virrandommock.so")

 #else



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

Re: [libvirt] [PATCH 2/2] qemu_cgroup: allow access to /dev/dri/render*

2016-05-19 Thread Gerd Hoffmann

  Hi,

> $ ls -lZ /dev/dri/
> total 0
> crw-rw+ 1 root video system_u:object_r:dri_device_t:s0 226,   0 May 18
> 19:17 card0
> crw---. 1 root video system_u:object_r:dri_device_t:s0 226,  64 May 18
> 19:17 controlD64
> crw-rw+ 1 root video system_u:object_r:dri_device_t:s0 226, 128 May 18
> 19:17 renderD128
> 
> qemu itself loops over /dev/dri, grabs the first matching renderD* that it can
> open, then does its magic. Some questions I have in general:
> 
> - is there only ever one render node? or one per video card?

One per video card.

> - is it okay to use the same node for multiple VMs simultaneously?

Yes.

> Maybe the long term fix is to have libvirt pass in a pre-opened fd for
> qemu:///system, since I don't know if it would be acceptable to chown
> qemu:qemu on the render node, but maybe we use setfacl instead.

chown isn't a good idea I think.  But doesn't use libvirt setfacl anyway
for simliar things (i.e. /dev/bus/usb/... for usb pass-through) ?

> I certainly agree with that at least WRT UI tools... after playing with this
> stuff a bit I won't be adding UI clicky 'enable gl' in virt-manager in the
> short term, there's just too many operational caveats. But advertisement in
> the form of blog posts with all the caveats listed will probably save us bug
> reports, and maybe a command line virt-xml one liner to turn it on for an
> existing VM. And of course have virt-manager work with it correctly on the
> viewer side, patches in git and fedora build coming soon

Agree, we should first get things going smooth before adding a big
"enable gl" switch in the UI.

cheers,
  Gerd


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

Re: [libvirt] [PATCH v2 4/8] Introduce PCI Multifunction device parser

On Thu, May 19, 2016 at 3:01 AM, Shivaprasad G Bhat <
shivaprasadb...@gmail.com> wrote:

> This patch just introduces the parser function used by
> the later patches. The parser disallows hostdevices to be
> used with other virtio devices simultaneously.
>
> Signed-off-by: Shivaprasad G Bhat 
> ---
>  src/conf/domain_conf.c   |  236
> ++
>  src/conf/domain_conf.h   |   22 
>  src/libvirt_private.syms |3 +
>  3 files changed, 261 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ed0c471..e946147 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -860,6 +860,36 @@ virDomainXMLOptionClassDispose(void *obj)
>  (xmlopt->config.privFree)(xmlopt->config.priv);
>  }
>
> +/* virDomainDeviceDefListAddCopy - add a *copy* of the device to this
> list */
> +int
> +virDomainDeviceDefListAddCopy(virDomainDeviceDefListPtr list,
> +  virDomainDeviceDefPtr dev,
> +  const virDomainDef *def,
> +  virCapsPtr caps,
> +  virDomainXMLOptionPtr xmlopt)
> +{
> +virDomainDeviceDefPtr copy = virDomainDeviceDefCopy(dev, def, caps,
> xmlopt);
> +
> +if (!copy)
> +return -1;
> +if (VIR_APPEND_ELEMENT(list->devs, list->count, copy) < 0) {
> +virDomainDeviceDefFree(copy);
> +return -1;
> +}
> +return 0;
> +}
> +
> +void virDomainDeviceDefListDispose(virDomainDeviceDefListPtr list)
> +{
> +size_t i;
> +
> +if (!list)
> +return;
> +for (i = 0; i < list->count; i++)
> +virDomainDeviceDefFree(list->devs[i]);
> +VIR_FREE(list);
> +}
> +
>  /**
>   * virDomainKeyWrapCipherDefParseXML:
>   *
> @@ -24365,3 +24395,209 @@ virDomainObjGetShortName(virDomainObjPtr vm)
>
>  return ret;
>  }
> +
> +static int
> +virDomainPCIMultifunctionDeviceDefParseXML(xmlXPathContextPtr ctxt,
> +   const virDomainDef *def,
> +   virDomainDeviceDefListPtr
> devlist,
> +   virCapsPtr caps,
> +   virDomainXMLOptionPtr xmlopt,
> +   unsigned int flags)
> +{
> +size_t i, j;
> +int n;
> +virDomainDeviceDef device;
> +xmlNodePtr *nodes = NULL;
> +char *netprefix = NULL;
> +int nhostdevs = 0;
> +
> +device.type = VIR_DOMAIN_DEVICE_DISK;
> +
> +if ((n = virXPathNodeSet("./disk", ctxt, &nodes)) < 0)
> +goto error;
> +
> +for (i = 0; i < n; i++) {
> +virDomainDiskDefPtr disk = virDomainDiskDefParseXML(xmlopt,
> +nodes[i],
> +ctxt,
> +NULL,
> +
> def->seclabels,
> +
> def->nseclabels,
> +flags);
> +if (!disk)
> +goto error;
> +
> +device.data.disk = disk;
> +if (virDomainDeviceDefListAddCopy(devlist, &device, def, caps,
> xmlopt) < 0)
> +goto error;
> +VIR_FREE(disk);
> +}
> +VIR_FREE(nodes);
> +
> +device.type = VIR_DOMAIN_DEVICE_CONTROLLER;
> +/* analysis of the controller devices */
> +if ((n = virXPathNodeSet("./controller", ctxt, &nodes)) < 0)
> +goto error;
> +
> +for (i = 0; i < n; i++) {
> +virDomainControllerDefPtr controller =
> virDomainControllerDefParseXML(nodes[i],
> +
> ctxt,
> +
> flags);
> +if (!controller)
> +goto error;
> +
> +device.data.controller = controller;
> +if (virDomainDeviceDefListAddCopy(devlist, &device, def, caps,
> xmlopt) < 0)
> +goto error;
> +VIR_FREE(controller);
> +}
> +VIR_FREE(nodes);
> +
> +device.type = VIR_DOMAIN_DEVICE_NET;
> +if ((n = virXPathNodeSet("./interface", ctxt, &nodes)) < 0)
> +goto error;
> +
> +netprefix = caps->host.netprefix;
> +for (i = 0; i < n; i++) {
> +virDomainNetDefPtr net = virDomainNetDefParseXML(xmlopt,
> + nodes[i],
> + ctxt,
> + NULL,
> + netprefix,
> + flags);
> +if (!net)
> +goto error;
> +
> +device.data.net = net;
> +if (virDomainDeviceDefListAddCopy(devlist, &device, def, caps,
> xmlopt) < 0)
> +goto error;
> +VIR_FREE(net);
> +}
> +VIR_FREE(nodes);
> +
> +/* analysis of the host devices */
> +device.type = VIR_DOMAIN_DEVICE_HOSTDEV;
> +if ((nhostdevs = virXPathNodeSe

Re: [libvirt] [PATCH v3 1/1] perf: add support to perf event for MBM

2016-05-19 Thread Peter Krempa

On Fri, May 13, 2016 at 12:26:07 +0800, Qiaowei Ren wrote:
> Some Intel processor families (e.g. the Intel Xeon processor E5 v3
> family) introduced some RDT (Resource Director Technology) features
> to monitor or control shared resource. Among these features, MBM
> (Memory Bandwidth Monitoring), which is build on the CMT (Cache
> Monitoring Technology) infrastructure, provides OS/VMM a way to
> monitor bandwidth from one level of cache to another.
> 
> With current perf framework, this patch adds support to perf event
> for MBM.
> 
> Signed-off-by: Qiaowei Ren 
> ---
>  include/libvirt/libvirt-domain.h | 26 -
>  src/libvirt-domain.c | 12 
>  src/qemu/qemu_driver.c   | 41 +++---
>  src/util/virperf.c   | 63 
> 
>  src/util/virperf.h   |  2 ++
>  5 files changed, 108 insertions(+), 36 deletions(-)

[...]

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index c4c4968..670f620 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c

[...]

> @@ -19494,24 +19496,38 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
>  
>  #undef QEMU_ADD_COUNT_PARAM
>  
> +#define QEMU_ADD_PERF_PARAM_ULL(record, maxparams, name, value) \
> +do { \
> +char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> +snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> + "perf.%s", name); \
> +if (virTypedParamsAddULLong(&(record)->params, \
> +&(record)->nparams, \
> +maxparams, \
> +param_name, \
> +value) < 0) \
> +goto cleanup; \
> +} while (0)

This macro is used once so it's not really necessary.

> +
>  static int
> -qemuDomainGetStatsPerfCmt(virPerfPtr perf,
> +qemuDomainGetStatsPerfRdt(virPerfPtr perf,
> +  virPerfEventType type,
>virDomainStatsRecordPtr record,
>int *maxparams)
>  {
> -uint64_t cache = 0;
> +uint64_t value = 0;
>  
> -if (virPerfReadEvent(perf, VIR_PERF_EVENT_CMT, &cache) < 0)
> +if (virPerfReadEvent(perf, type, &value) < 0)
>  return -1;
>  
> -if (virTypedParamsAddULLong(&record->params,
> -&record->nparams,
> -maxparams,
> -"perf.cache",
> -cache) < 0)
> -return -1;
> +QEMU_ADD_PERF_PARAM_ULL(record, maxparams,
> +virPerfEventTypeToString(type),
> +value);

Otherwise looks good. Thanks for tweaking the documentation.

I'll push this with the macro dropped in a while.

Peter


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

Re: [libvirt] [PATCH 2/2] qemu_cgroup: allow access to /dev/dri/render*

On 05/19/2016 08:52 AM, Daniel P. Berrange wrote:
> On Thu, May 19, 2016 at 08:36:35AM -0400, Cole Robinson wrote:
>> On 05/19/2016 08:21 AM, Daniel P. Berrange wrote:
>>> On Thu, May 19, 2016 at 01:29:07PM +0200, Ján Tomko wrote:
 Allow access to /dev/dri/render* devices for domains
 using  with 

 https://bugzilla.redhat.com/show_bug.cgi?id=1337290
>>>
>>> Ignoring cgroups for a minute, how exactly does QEMU get access to
>>> the /dev/dri/render* devices in general ?  ie when QEMU is running
>>> as the 'qemu:qemu' user/group account, with selinux enforcing I
>>> don't see how it can possibly open these files, as we're not granting
>>> access to them in any of the security drivers. Given this, allowing
>>> them in cgroups seems like the least of our problems.
>>>
>>
>> The svirt bits can at least be temporarily worked around with chmod 666
>> /dev/dri/render* and setenforce 0. The cgroup bit requires duplicating the
>> entire cgroup_device_acl block in qemu.conf which is less friendly and not
>> very future proof. Seems like an easy win
> 
> There's a potential issue though with going down a path now which is not
> viable long term, which we then get stuck supporting for upgradability.
> eg if we start granting permission to use these devices to multiple QEMUs
> concurrently will we regret doing that later and have to break people's
> deployments to fix it properly.
> 

Hmm, I see. CCing gl guys

How this works on my f24 host:

$ ls -lZ /dev/dri/
total 0
crw-rw+ 1 root video system_u:object_r:dri_device_t:s0 226,   0 May 18
19:17 card0
crw---. 1 root video system_u:object_r:dri_device_t:s0 226,  64 May 18
19:17 controlD64
crw-rw+ 1 root video system_u:object_r:dri_device_t:s0 226, 128 May 18
19:17 renderD128

qemu itself loops over /dev/dri, grabs the first matching renderD* that it can
open, then does its magic. Some questions I have in general:

- is there only ever one render node? or one per video card?
- is it okay to use the same node for multiple VMs simultaneously?

Maybe the long term fix is to have libvirt pass in a pre-opened fd for
qemu:///system, since I don't know if it would be acceptable to chown
qemu:qemu on the render node, but maybe we use setfacl instead.

> Without sVirt integration though I'd suggest we don't really advertize
> this to users, as telling them to chmod / setenforce is not really a
> supportable strategy for usage in any case.
>

I certainly agree with that at least WRT UI tools... after playing with this
stuff a bit I won't be adding UI clicky 'enable gl' in virt-manager in the
short term, there's just too many operational caveats. But advertisement in
the form of blog posts with all the caveats listed will probably save us bug
reports, and maybe a command line virt-xml one liner to turn it on for an
existing VM. And of course have virt-manager work with it correctly on the
viewer side, patches in git and fedora build coming soon

>> But yes, there needs to be a larger discussion about how to correctly handle
>> this WRT svirt for both qemu:///system and qemu:///session. selinux bug here:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1337333
> 
> Looks like we'd need to consider those separately - as in the session
> case, even libvirtd won't have the option to fix permissioning. It is
> something that would have to be done at the OS level to grant access.
> Once granting access to just an unprivileged QEMU you might as well
> just grant access to all a user's processes, since there's no separation
> stopping other processes in the user session getting access to the devices
> via QEMU. IOW, if you want qemu:///session mode to have access you end up
> with a chmod 666 world, where everyone has access. I don't know enough about
> it to know if that's reasonable or not.

Actually qemu:///session DAC permissions are fine, because the logged in user
has ACL access to the render node already, like /dev/snd/* for example. It's
just the svirt selinux policy that is rejecting access

The DAC permissions are an issue with qemu:qemu on qemu:///system though

- Cole

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

Re: [libvirt] [PATCH 2/2] qemu_cgroup: allow access to /dev/dri/render*

On Thu, May 19, 2016 at 08:36:35AM -0400, Cole Robinson wrote:
> On 05/19/2016 08:21 AM, Daniel P. Berrange wrote:
> > On Thu, May 19, 2016 at 01:29:07PM +0200, Ján Tomko wrote:
> >> Allow access to /dev/dri/render* devices for domains
> >> using  with 
> >>
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1337290
> > 
> > Ignoring cgroups for a minute, how exactly does QEMU get access to
> > the /dev/dri/render* devices in general ?  ie when QEMU is running
> > as the 'qemu:qemu' user/group account, with selinux enforcing I
> > don't see how it can possibly open these files, as we're not granting
> > access to them in any of the security drivers. Given this, allowing
> > them in cgroups seems like the least of our problems.
> > 
> 
> The svirt bits can at least be temporarily worked around with chmod 666
> /dev/dri/render* and setenforce 0. The cgroup bit requires duplicating the
> entire cgroup_device_acl block in qemu.conf which is less friendly and not
> very future proof. Seems like an easy win

There's a potential issue though with going down a path now which is not
viable long term, which we then get stuck supporting for upgradability.
eg if we start granting permission to use these devices to multiple QEMUs
concurrently will we regret doing that later and have to break people's
deployments to fix it properly.

Without sVirt integration though I'd suggest we don't really advertize
this to users, as telling them to chmod / setenforce is not really a
supportable strategy for usage in any case.

> But yes, there needs to be a larger discussion about how to correctly handle
> this WRT svirt for both qemu:///system and qemu:///session. selinux bug here:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1337333

Looks like we'd need to consider those separately - as in the session
case, even libvirtd won't have the option to fix permissioning. It is
something that would have to be done at the OS level to grant access.
Once granting access to just an unprivileged QEMU you might as well
just grant access to all a user's processes, since there's no separation
stopping other processes in the user session getting access to the devices
via QEMU. IOW, if you want qemu:///session mode to have access you end up
with a chmod 666 world, where everyone has access. I don't know enough about
it to know if that's reasonable or not.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH 3/3] qemu: bulk stats: Don't access possibly blocked storage

2016-05-19 Thread Jiri Denemark

On Wed, May 18, 2016 at 15:14:29 +0200, Peter Krempa wrote:
> When libvirt is gathering stats for block devices in the bulk stats API
> it would use the fallback code that accesses the files directly in
> libvirt both if the VM was offline and if qemu didn't return the stats
> at all.

It took me a while to understand what you wanted to say here. It would
be nice if you could reword it :-)

> If qemu is not cooperating due to being stuck on an inaccessible NFS
> share we would then attempt to read the files and get stuck too with
> the VM object locked. All other APIs would get eventually get stuck

s/ get / /

> waiting on the VM lock.
> 
> Avoid this problem by skipping the block stats if the VM is online but
> the monitor did not provide any stats.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1337073
> ---
>  src/qemu/qemu_driver.c | 13 +++--
>  1 file changed, 11 insertions(+), 2 deletions(-)

ACK

Jirka

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

Re: [libvirt] [PATCH 2/2] qemu_cgroup: allow access to /dev/dri/render*

On 05/19/2016 08:21 AM, Daniel P. Berrange wrote:
> On Thu, May 19, 2016 at 01:29:07PM +0200, Ján Tomko wrote:
>> Allow access to /dev/dri/render* devices for domains
>> using  with 
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1337290
> 
> Ignoring cgroups for a minute, how exactly does QEMU get access to
> the /dev/dri/render* devices in general ?  ie when QEMU is running
> as the 'qemu:qemu' user/group account, with selinux enforcing I
> don't see how it can possibly open these files, as we're not granting
> access to them in any of the security drivers. Given this, allowing
> them in cgroups seems like the least of our problems.
> 

The svirt bits can at least be temporarily worked around with chmod 666
/dev/dri/render* and setenforce 0. The cgroup bit requires duplicating the
entire cgroup_device_acl block in qemu.conf which is less friendly and not
very future proof. Seems like an easy win

But yes, there needs to be a larger discussion about how to correctly handle
this WRT svirt for both qemu:///system and qemu:///session. selinux bug here:

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

- Cole

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

Re: [libvirt] [PATCH 2/3] qemu: driver: Separate bulk stats worker for block devices

2016-05-19 Thread Jiri Denemark

On Wed, May 18, 2016 at 15:14:28 +0200, Peter Krempa wrote:
> Extract the fallback path that reloads the stats from disk into a
> separate function.
> ---
>  src/qemu/qemu_driver.c | 58 
> --
>  1 file changed, 37 insertions(+), 21 deletions(-)

ACK

Jirka

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

Re: [libvirt] [PATCH v2 2/2] qemu: parse: Handle suffixes for -m memory

2016-05-19 Thread Nishith Shah

On Thu, May 19, 2016 at 5:40 PM, Cole Robinson  wrote:

> On 05/18/2016 02:36 AM, Nishith Shah wrote:
> > diff --git a/src/qemu/qemu_parse_command.c
> b/src/qemu/qemu_parse_command.c
> > index 334dcf8..f027d51 100644
> > --- a/src/qemu/qemu_parse_command.c
> > +++ b/src/qemu/qemu_parse_command.c
> > @@ -1638,14 +1638,22 @@ qemuParseCommandLineMem(virDomainDefPtr dom,
> >  {
> >  unsigned long long mem;
> >  char *end;
> > +
>
> This whitespace change should be part of patch #1
>
> Otherwise the patches look good to me, bring on v3 :)
>
> Thanks,
> Cole
>
Working on it :)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] qemu_cgroup: allow access to /dev/dri/render*

On Thu, May 19, 2016 at 01:29:07PM +0200, Ján Tomko wrote:
> Allow access to /dev/dri/render* devices for domains
> using  with 
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1337290

Ignoring cgroups for a minute, how exactly does QEMU get access to
the /dev/dri/render* devices in general ?  ie when QEMU is running
as the 'qemu:qemu' user/group account, with selinux enforcing I
don't see how it can possibly open these files, as we're not granting
access to them in any of the security drivers. Given this, allowing
them in cgroups seems like the least of our problems.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH v2 1/2] qemu: parse: Use qemuParseCommandLineMem for -m memory

On Thu, May 19, 2016 at 08:08:39AM -0400, Cole Robinson wrote:
> On 05/18/2016 02:36 AM, Nishith Shah wrote:
> > +virDomainDefSetMemoryTotal(dom, mem * 1024);
> > +dom->mem.cur_balloon = mem * 1024;
> > +
> > +return 0;
> > +
> > + error:
> > +return -1;
> > +}
> > +
> 
> Typically when we use the 'goto error' pattern, what we do is:
> 
> int ret = -1;
> 
> if (condition)
> goto error;
> 
> ret = 0;
>  error:
> return ret;
> 
> So there's only one 'return'

When the path is taken both on error and success, 'cleanup' is
the preferred label name:  http://libvirt.org/hacking.html#goto

Jan

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

Re: [libvirt] [PATCH v2 1/2] qemu: parse: Use qemuParseCommandLineMem for -m memory

On 05/19/2016 08:20 AM, Ján Tomko wrote:
> On Thu, May 19, 2016 at 08:08:39AM -0400, Cole Robinson wrote:
>> On 05/18/2016 02:36 AM, Nishith Shah wrote:
>>> +virDomainDefSetMemoryTotal(dom, mem * 1024);
>>> +dom->mem.cur_balloon = mem * 1024;
>>> +
>>> +return 0;
>>> +
>>> + error:
>>> +return -1;
>>> +}
>>> +
>>
>> Typically when we use the 'goto error' pattern, what we do is:
>>
>> int ret = -1;
>>
>> if (condition)
>> goto error;
>>
>> ret = 0;
>>  error:
>> return ret;
>>
>> So there's only one 'return'
> 
> When the path is taken both on error and success, 'cleanup' is
> the preferred label name:  http://libvirt.org/hacking.html#goto
> 

Thanks for the correction, I need to make a note about that...

- Cole

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

Re: [libvirt] [PATCH] Introduce gnutls_priority config option