Re: [libvirt] [PATCH] add support of iSER transport type in qemu with libiscsi

[Charles Kelimod]

Hello John,

I have questions:

>>This and the Parse makes no sense when compared to the 3 possible
options.  That is, it's possible to have "tcp", "rdma", and "iser", but
you only care about "iser".

This is because for other types are optional, there was no this line here,
therefor I added this and only care iser.

>Consider using a local @transport... I didn't check if src->hosts could
be NULL here...

This is used for iSCSI only, in this case, from my other
place modification I should make sure src->hosts won't be NULL, and can it
be local?


For those samples like "auth", I don't think those are all necessary, but
if I don't add those, the tests won't pass. Because iser is some strange,
just part of iSCSI, it is all same as iscsi except transport..

>Why do both  and  get the transport='iser'?
>From qemu command line and code architecture I believe both place are need,
do you think we should use "iser" in only source or host section? I can do
dig search.



Best Regards,
Charles.


On Wed, Jan 17, 2018 at 7:10 AM, John Ferlan  wrote:

>
>
> On 01/16/2018 03:52 AM, lichs...@gmail.com wrote:
> > From: zhangshengyu 
> >
>
> This needs to be split up better into multiple patches - there are many
> examples of how to do that. Just see how patches were done the last time
> someone added a new transport type. Typically the docs/schemas,
> src/conf, src/util, xml2xml tests would go in one patch while the
> src/qemu, xml2argv patches in a second patch.  Each patch would be
> compile-able and testable.
>
> As pointed out before, you're still not completely following the
> contributor guidelines w/r/t commit message. Furthermore, when you
> "update" your patch and post a followup version, you should make a "v2"
> or "v3" or "v4" in your header and then either in the cover letter or
> after the '---' describe what changed since the previous patch.
>
> The first patch would also need to update docs/formatdomain.html.in and
> the commit message should also describe the new XML being added - there
> are many examples.
>
> > ---
> >  docs/schemas/domaincommon.rng  | 28 +
> >  src/conf/domain_conf.c |  8 
> >  src/qemu/qemu_block.c  | 24 ++-
> >  src/qemu/qemu_command.c|  3 ++
> >  src/qemu/qemu_parse_command.c  | 10 -
> >  src/storage/storage_backend_gluster.c  |  1 +
> >  src/util/virstoragefile.c  |  3 +-
> >  src/util/virstoragefile.h  |  1 +
> >  .../disk-drive-network-iser-auth.args  | 25 
> >  .../disk-drive-network-iser-auth.xml   | 44
> 
> >  .../qemuargv2xmldata/disk-drive-network-iser.args  | 25 
> >  tests/qemuargv2xmldata/disk-drive-network-iser.xml | 41
> +++
> >  tests/qemuargv2xmltest.c   |  2 +
> >  ...-drive-network-iser-auth-secrettype-invalid.xml | 33 +++
> >  ...sk-drive-network-iser-auth-wrong-secrettype.xml | 33 +++
> >  .../disk-drive-network-iser-auth.args  | 31 ++
> >  .../disk-drive-network-iser-auth.xml   | 43
> 
> >  .../disk-drive-network-iser-lun.args   | 27 +
> >  .../disk-drive-network-iser-lun.xml| 28 +
> >  .../qemuxml2argvdata/disk-drive-network-iser.args  | 29 +
> >  tests/qemuxml2argvdata/disk-drive-network-iser.xml | 37
> +
> >  tests/qemuxml2argvtest.c   |  7 
> >  .../disk-drive-network-iser-auth.xml   | 47
> ++
> >  .../qemuxml2xmloutdata/disk-drive-network-iser.xml | 41
> +++
> >  tests/qemuxml2xmltest.c|  2 +
> >  25 files changed, 569 insertions(+), 4 deletions(-)
> >  create mode 100644 tests/qemuargv2xmldata/disk-
> drive-network-iser-auth.args
> >  create mode 100644 tests/qemuargv2xmldata/disk-
> drive-network-iser-auth.xml
> >  create mode 100644 tests/qemuargv2xmldata/disk-drive-network-iser.args
> >  create mode 100644 tests/qemuargv2xmldata/disk-drive-network-iser.xml
> >  create mode 100644 tests/qemuxml2argvdata/disk-drive-network-iser-auth-
> secrettype-invalid.xml
> >  create mode 100644 tests/qemuxml2argvdata/disk-
> drive-network-iser-auth-wrong-secrettype.xml
> >  create mode 100644 tests/qemuxml2argvdata/disk-
> drive-network-iser-auth.args
> >  create mode 100644 tests/qemuxml2argvdata/disk-
> drive-network-iser-auth.xml
> >  create mode 100644 tests/qemuxml2argvdata/disk-
> drive-network-iser-lun.args
> >  create mode 100644 tests/qemuxml2argvdata/disk-
> drive-network-iser-lun.xml
> >  create mode 100644 tests/qemuxml2argvdata/disk-drive-network-iser.args
> >  create mode 100644 tests/qemuxml2argvdata/disk-drive-network-iser.xml
> >  

Re: [libvirt] [PATCH RESEND v1 1/2] i386: Add Intel Processor Trace feature support

[Kang, Luwei]

> > > > > > On Mon, Jan 15, 2018 at 12:04:55 -0200, Eduardo Habkost wrote:
> > > > > > > CCing libvirt developers.
> > > > > > ...
> > > > > > > This case is slightly more problematic, however: the new
> > > > > > > feature is actually migratable (under very controlled
> > > > > > > circumstances) because of patch 2/2, but it is not
> > > > > > > migration-safe[1].  This means libvirt shouldn't include it
> > > > > > > in "host-model" expansion (which uses the
> > > > > > > query-cpu-model-expansion QMP command) until we make the feature 
> > > > > > > migration-safe.
> > > > > > >
> > > > > > > For QEMU, this means the feature shouldn't be returned by
> > > > > > > "query-cpu-model-expansion type=static model=max" (but it
> > > > > > > can be returned by "query-cpu-model-expansion type=full 
> > > > > > > model=max").
> > > > > > >
> > > > > > > Jiri, it looks like libvirt uses type=full on
> > > > > > > query-cpu-model-expansion on x86.  It needs to use
> > > > > > > type=static[2], or it will have no way to find out if a
> > > > > > > feature is migration-safe or not.
> > > > > > ...
> > > > > > > [2] It looks like libvirt uses type=full because it wants to get
> > > > > > > all QOM property aliases returned.  In this case, one
> > > > > > > solution for libvirt is to use:
> > > > > > >
> > > > > > > static_expansion = query_cpu_model_expansion(type=static, 
> > > > > > > model)
> > > > > > > all_props = query_cpu_model_expansion(type=full,
> > > > > > > static_expansion)
> > > > > >
> > > > > > This is exactly what libvirt is doing (with model = "host")
> > > > > > ever since query-cpu-model-expansion support was implemented for 
> > > > > > x86.
> > > > >
> > > > > Oh, now I see that the x86 code uses
> > > > > QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC_FULL and not just 
> > > > > QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL.
> > > Nice!
> > > > >
> > > >
> > > > So, I need to add Intel PT feature in "X86CPUDefinition
> > > > builtin_x86_defs[]" so that we can get this CPUID in specific CPU
> > > > model not only "-cpu host". Is that right?
> > >
> > > The problem is that you won't be able to add intel-pt to any CPU
> > > model unless the feature is made migration-safe (by not calling
> > > kvm_arch_get_supported_cpuid() from cpu_x86_cpuid()).
> >
> > Hi Eduardo,
> > Have some question need you help clear. What is
> > "migration-safe" feature mean? I find all the feature which
> > included in CPU model (builtin_x86_defs[]) will make
> > "xcc->migration_safe = true;" in
> > x86_cpu_cpudef_class_init(). If create a Skylake guest on
> > Skylake HW and live migrate this guest to another machine
> > with old HW(e.g Haswell). Can we think the new feature or
> > cpu model(Skylake guest) which only supported in Skylake HW
> > is safe?
> 
> I mean matching the requirements so we can say the feature is migration-safe, 
> that means exposing the same CPUID data to the
> guest on any host, if the machine-type + command-line is the same.  The data 
> on CPUID[7] is OK (it changes according to the
> command-line options only), but the data exposed on CPUID[14h] on this patch 
> is not migration-safe (it changes depending on the
> host it is running).
> 

Many thanks for your clarification.  I think I have understood. But CPUID[14h] 
is depend on CPUID[7].EBX[bit25] and it would be zero if CPUID[7].EBX[bit25] is 
not set. Or what you concerned is make live migration on two different HW which 
all support Intel PT virtualization but have different  CPUID[14h] result? This 
may need to think about.

Thanks,
Luwei Kang

> 
> >
> > >
> > > What's missing here is to either: (a) make cpu_x86_cpuid() return
> > > host-independent data (it can be constant, or it can be configurable on 
> > > the command-line); or (b) add a mechanism to skip intel-
> pt from "query-cpu-model-expansion type=static".
> >
> > ==> it can be constant:
> > I think it shouldn't be constant because Intel PT virtualization can 
> > only supported in Ice Lake hardware now. Intel PT cpuid info
> would be mask off on old platform.
> > ==> or it can be configurable on the command-line:
> > Because of I didn't add this feature in any CPU model. We only can 
> > enable this feature by "-cpu Skylake-Server,+intel-pt" at
> present.
> 
> Note that I'm talking about CPUID[14h], not CPUID[7].  The CPUID[7] bits are 
> safe because they are set according to the CPU model
> + command-line options only.  The bits on CPUID[14h] change depending on the 
> host and are not migration-safe.  CPUID[7].EBX[bit
> 25] is just the (already configurable) bit that enables the migration-unsafe 
> code for CPUID[14h].
> 
> >
> > What about add a new cpu model name "Icelake" and add PT in this. Is that 
> > would be migration safe?
> 
> It won't, because of the CPUID[14h] code that makes it unsafe to migrate 
> between hosts with different Intel-PT capabilities (i.e.
> different data on CPUID[14h]).
> 
> 
> >
> > Thanks,
> > 

Re: [libvirt] [PATCH RESEND v1 1/2] i386: Add Intel Processor Trace feature support

[Eduardo Habkost]

On Wed, Jan 17, 2018 at 10:32:56AM +, Kang, Luwei wrote:
> > > > > On Mon, Jan 15, 2018 at 12:04:55 -0200, Eduardo Habkost wrote:
> > > > > > CCing libvirt developers.
> > > > > ...
> > > > > > This case is slightly more problematic, however: the new feature
> > > > > > is actually migratable (under very controlled circumstances)
> > > > > > because of patch 2/2, but it is not migration-safe[1].  This
> > > > > > means libvirt shouldn't include it in "host-model" expansion
> > > > > > (which uses the query-cpu-model-expansion QMP command) until we
> > > > > > make the feature migration-safe.
> > > > > >
> > > > > > For QEMU, this means the feature shouldn't be returned by
> > > > > > "query-cpu-model-expansion type=static model=max" (but it can be
> > > > > > returned by "query-cpu-model-expansion type=full model=max").
> > > > > >
> > > > > > Jiri, it looks like libvirt uses type=full on
> > > > > > query-cpu-model-expansion on x86.  It needs to use
> > > > > > type=static[2], or it will have no way to find out if a feature
> > > > > > is migration-safe or not.
> > > > > ...
> > > > > > [2] It looks like libvirt uses type=full because it wants to get
> > > > > > all QOM property aliases returned.  In this case, one
> > > > > > solution for libvirt is to use:
> > > > > >
> > > > > > static_expansion = query_cpu_model_expansion(type=static, model)
> > > > > > all_props = query_cpu_model_expansion(type=full,
> > > > > > static_expansion)
> > > > >
> > > > > This is exactly what libvirt is doing (with model = "host") ever
> > > > > since query-cpu-model-expansion support was implemented for x86.
> > > >
> > > > Oh, now I see that the x86 code uses
> > > > QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC_FULL and not just 
> > > > QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL.
> > Nice!
> > > >
> > >
> > > So, I need to add Intel PT feature in "X86CPUDefinition
> > > builtin_x86_defs[]" so that we can get this CPUID in specific CPU
> > > model not only "-cpu host". Is that right?
> > 
> > The problem is that you won't be able to add intel-pt to any CPU model 
> > unless the feature is made migration-safe (by not calling
> > kvm_arch_get_supported_cpuid() from cpu_x86_cpuid()).
> 
> Hi Eduardo,
> Have some question need you help clear. What is
> "migration-safe" feature mean? I find all the feature which
> included in CPU model (builtin_x86_defs[]) will make
> "xcc->migration_safe = true;" in
> x86_cpu_cpudef_class_init(). If create a Skylake guest on
> Skylake HW and live migrate this guest to another machine
> with old HW(e.g Haswell). Can we think the new feature or
> cpu model(Skylake guest) which only supported in Skylake HW
> is safe?

I mean matching the requirements so we can say the feature is migration-safe,
that means exposing the same CPUID data to the guest on any host, if the
machine-type + command-line is the same.  The data on CPUID[7] is OK (it
changes according to the command-line options only), but the data exposed on
CPUID[14h] on this patch is not migration-safe (it changes depending on the
host it is running).


> 
> > 
> > What's missing here is to either: (a) make cpu_x86_cpuid() return 
> > host-independent data (it can be constant, or it can be
> > configurable on the command-line); or (b) add a mechanism to skip intel-pt 
> > from "query-cpu-model-expansion type=static".
> 
> ==> it can be constant:
> I think it shouldn't be constant because Intel PT virtualization can only 
> supported in Ice Lake hardware now. Intel PT cpuid info would be mask off on 
> old platform.
> ==> or it can be configurable on the command-line:
> Because of I didn't add this feature in any CPU model. We only can enable 
> this feature by "-cpu Skylake-Server,+intel-pt" at present.

Note that I'm talking about CPUID[14h], not CPUID[7].  The CPUID[7] bits are
safe because they are set according to the CPU model + command-line options
only.  The bits on CPUID[14h] change depending on the host and are not
migration-safe.  CPUID[7].EBX[bit 25] is just the (already configurable) bit
that enables the migration-unsafe code for CPUID[14h].

> 
> What about add a new cpu model name "Icelake" and add PT in this. Is that 
> would be migration safe?

It won't, because of the CPUID[14h] code that makes it unsafe to migrate
between hosts with different Intel-PT capabilities (i.e. different data on
CPUID[14h]).


> 
> Thanks,
> Luwei Kang
> 
> > Probably
> > (a) is easier to implement, and it also makes the feature more useful (by 
> > making it migration-safe).
> > 
> > >
> > > Intel PT is first supported in Intel Core M and 5th generation Intel
> > > Core processors that are based on the Intel micro-architecture code
> > > name Broadwell but Intel PT use EPT is first supported in Ice Lake.
> > > Intel PT virtualization depend on PT use EPT.  I will add Intel PT to
> > > "Broadwell" CPU model and later to make sure a "Broadwell" guest can
> > > use Intel PT if the host is Ice 

Re: [libvirt] [PATCH 2/3] nodedev: update mdev_types caps before dumpxml

[Wuzongyong (Euler Dept)]

Would you push this two patches before release of 4.0.0?

Thanks,
Zongyong Wu


> -Original Message-
> From: Erik Skultety [mailto:eskul...@redhat.com]
> Sent: Thursday, January 11, 2018 6:07 PM
> To: Wuzongyong (Euler Dept) 
> Cc: libvir-list@redhat.com; weijinfen ; Wanzongshun
> (Vincent) 
> Subject: Re: [PATCH 2/3] nodedev: update mdev_types caps before dumpxml
> 
> On Wed, Jan 10, 2018 at 08:14:50PM +0800, Wu Zongyong wrote:
> > In current implemention, mdev_types caps keep constant all the time.
> > But, it is possible that a device capable of mdev_types sometime(for
> > example:bind to proper driver) and incapable of mdev_types at other
> > times(for example: unbind from its driver).
> > We should keep the info of xml dumped out consistent with real status
> > of the device.
> >
> > Signed-off-by: Wu Zongyong 
> 
> For patches 1 and 2 (I'll tune the commit message for this one a bit
> before
> pushing):
> 
> Reviewed-by: Erik Skultety 

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

Re: [libvirt] [RFC PATCH 06/10] qemu: implement state driver shutdown function

[John Ferlan]

On 01/10/2018 01:05 PM, Daniel P. Berrange wrote:
> On Wed, Jan 10, 2018 at 12:23:31PM -0500, John Ferlan wrote:
>> From: Nikolay Shirokovskiy 
>>
>> Shutdown function should help API calls to finish when
>> event loop is not running anymore. For this reason let's
>> close agent and qemu monitors. These function will unblock
>> API calls wating for response from qemu process or qemu agent.
>>
>> Closing agent monitor and setting priv->agent to NULL when
>> waiting for response is normal usecase (handling EOF from
>> agent is handled the same way for example).
>>
>> However we can not do the same for qemu monitor. This monitor is normally
>> closed and unrefed during qemuProcessStop under destroy job so other threads
>> do not deal with priv->mon. But if we take extra reference to monitor
>> we are good. This can lead to double close but this function looks like to
>> be idempotent.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/qemu/qemu_driver.c | 39 +++
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index a203c9297..1de236cb5 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -1093,6 +1093,44 @@ qemuStateStop(void)
>>  return ret;
>>  }
>>  
>> +
>> +static int
>> +qemuDomainDisconnect(virDomainObjPtr vm, void *opaque ATTRIBUTE_UNUSED)
>> +{
>> +
>> +qemuDomainObjPrivatePtr priv;
>> +
>> +virObjectLock(vm);
>> +priv = vm->privateData;
>> +
>> +if (priv->mon) {
>> +/* Take extra reference to monitor so it won't be disposed
>> + * by qemuMonitorClose last unref. */
>> +virObjectRef(priv->mon);
>> +qemuMonitorClose(priv->mon);
>> +}
>> +
>> +if (priv->agent) {
>> +/* Other threads are ready for priv->agent to became NULL meanwhile 
>> */
>> +qemuAgentClose(priv->agent);
>> +priv->agent = NULL;
>> +}
>> +
>> +virObjectUnlock(vm);
>> +return 0;
>> +}
>> +
>> +
>> +static void
>> +qemuStateShutdown(void)
>> +{
>> +if (!qemu_driver)
>> +return;
>> +
>> +virDomainObjListForEach(qemu_driver->domains, qemuDomainDisconnect, 
>> NULL);
>> +}
>> +
>> +
>>  /**
>>   * qemuStateCleanup:
>>   *
>> @@ -21357,6 +21395,7 @@ static virStateDriver qemuStateDriver = {
>>  .stateCleanup = qemuStateCleanup,
>>  .stateReload = qemuStateReload,
>>  .stateStop = qemuStateStop,
>> +.stateShutdown = qemuStateShutdown,
> 
> I'm curious why we need this separate  .stateShutdown hook. It feels like
> this code could just be placed at the start of qemuStateStop and achieve
> the same result.
> 
> 

This path is only run when compiled "WITH_DBUS" and it only matters when
(!virNetDaemonIsPrivileged(dmn)).

Also since qemuStateStop will suspend and save to disk all active
domains, it doesn't seem that's what may be desired in this case where
libvirtd is stopped.


John

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

Re: [libvirt] [RFC PATCH 04/10] qemu: Introduce virTheadPoolDrain

[John Ferlan]

On 01/15/2018 11:57 AM, Daniel P. Berrange wrote:
> On Mon, Jan 15, 2018 at 05:51:28PM +0100, Erik Skultety wrote:
>> On Wed, Jan 10, 2018 at 12:23:29PM -0500, John Ferlan wrote:
>>> Split up virThreadPoolFree to create a Drain function which will
>>> be called from virNetServerClose in order to ensure the various
>>> worker threads are removed during the close rather than waiting
>>> for the dispose function.
>>>
>>> Signed-off-by: John Ferlan 
>>> ---
>>
>> I think that Dan had a bit different idea about the virThreadPoolDrain (I 
>> think
>> that the name should have been something like virThreadPoolJobsPurge) 
>> function.
>> https://www.redhat.com/archives/libvir-list/2017-December/msg00802.html
> 
> Yep, this impl in John's patch is more akin to a StopWorkers()
> method, than a Drain() method. To me "Drain" implies the workers
> are still available to process further jobs
> 
> Regards,
> Daniel
> 

OK - right... there's so many links and thoughts with various
terminology in the former series... I of course read one path about
splitting up PoolFree and just ran with that, but had the more recent
stuff in the front of the brain so the Drain name stuck, but the concept
of Drain certainly wasn't what Dan described.

Let's see what I can come up with for a Drain function. Still trying to
wrap my head around the totality of this code - so many nooks and
crannies. Still not 100% clear how to handle the what happens if some
worker/job is truly stuck and the TERM is attempted.

John

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

Re: [libvirt] [RFC PATCH 03/10] netserver: Toggle service off during close

[John Ferlan]

On 01/15/2018 11:35 AM, Erik Skultety wrote:
> On Wed, Jan 10, 2018 at 12:23:28PM -0500, John Ferlan wrote:
>> Rather than waiting until virNetServerDispose to toggle the service
>> to off, let's do that when virNetServerServiceClose is called such
>> as during virNetServerClose.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/rpc/virnetserver.c| 3 ---
>>  src/rpc/virnetserverservice.c | 2 ++
>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
>> index 77a4c0b8d..7bab11efb 100644
>> --- a/src/rpc/virnetserver.c
>> +++ b/src/rpc/virnetserver.c
>> @@ -805,9 +805,6 @@ void virNetServerDispose(void *obj)
>>
>>  VIR_FREE(srv->name);
>>
>> -for (i = 0; i < srv->nservices; i++)
>> -virNetServerServiceToggle(srv->services[i], false);
>> -
> 
> ^This hunk would suffice.
> 
>>  virThreadPoolFree(srv->workers);
>>
>>  for (i = 0; i < srv->nservices; i++)
>> diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c
>> index 4e5426ffe..636c5be4e 100644
>> --- a/src/rpc/virnetserverservice.c
>> +++ b/src/rpc/virnetserverservice.c
>> @@ -525,4 +525,6 @@ void virNetServerServiceClose(virNetServerServicePtr svc)
>>  virNetSocketClose(svc->socks[i]);
>>  virObjectUnref(svc);
>>  }
>> +
>> +virNetServerServiceToggle(svc, false);
> 
> ^This is a NOP, since all the sockets have been closed already (in the loop
> which precedes the call) and the IO callback handle removed with watch reset 
> to
> -1.

oh right ... and it was discussed multiple times in various
threads in the "other" series that I pulled this from...

I was more focused on trying to put together 3 or 4 disjoint series and
discussions into one pile and really wasn't thinking beyond the take
existing code or words and generate patches.

So, I'll drop the second hunk and change the commit message to:

netserver: Remove ServiceToggle during ServerDispose

No sense in calling ServiceToggle for all nservices during
ServiceDispose since ServerClose calls ServiceClose which
removes the IOCallback that's being toggled via ServiceToggle.


Tks -

John



> 
> Reviewed-by: Erik Skultety 
> 

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

Re: [libvirt] [PATCH V2] nodedev: Fix failing to parse PCI address for non-PCI network devices

[Jim Fehlig]

On 01/09/2018 04:35 AM, Erik Skultety wrote:

On Mon, Jan 08, 2018 at 10:08:59AM -0700, Jim Fehlig wrote:

Based loosely on a patch from Fei Li .

Commit 8708ca01c added virNetDevSwitchdevFeature() to check if a network
device has Switchdev capabilities. virNetDevSwitchdevFeature() attempts
to retrieve the PCI device associated with the network device, ignoring
non-PCI devices. It does so via the following call chain

   virNetDevSwitchdevFeature()->virNetDevGetPCIDevice()->
   virPCIGetDeviceAddressFromSysfsLink()

For non-PCI network devices (qeth, Xen vif, etc),
virPCIGetDeviceAddressFromSysfsLink() will report an error when
virPCIDeviceAddressParse() fails. virPCIDeviceAddressParse() also
logs an error. After commit 8708ca01c there are now two errors reported
for each non-PCI network device even though the errors are harmless.

To avoid changing virPCIGetDeviceAddressFromSysfsLink(),
virPCIDeviceAddressParse() and related code that has quite a few
call sites, add a function to check if a network device is a
PCI device and use it in virNetDevSwitchdevFeature() before attempting
to retrieve the PCI device.
---

Suggestions welcome on a better name for virPCIIsPCIDevice, or even
a better approach to solving this issue. Currently virPCIIsPCIDevice
assumes the device is PCI if its subsystem starts with 'pci'. I'd be
happy to hear if there is a better way to determine if a network
device is PCI.


Overall I like this approach, but see my comments below. I'm also curious about
some points I raised.



  src/libvirt_private.syms |  1 +
  src/util/virnetdev.c | 25 ++---
  src/util/virpci.c| 32 
  src/util/virpci.h|  3 +++
  4 files changed, 58 insertions(+), 3 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a705fa846..bdf98ded1 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2458,6 +2458,7 @@ virPCIGetVirtualFunctionInfo;
  virPCIGetVirtualFunctions;
  virPCIHeaderTypeFromString;
  virPCIHeaderTypeToString;
+virPCIIsPCIDevice;
  virPCIIsVirtualFunction;
  virPCIStubDriverTypeFromString;
  virPCIStubDriverTypeToString;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index eb2d119bf..a9af08797 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1148,6 +1148,21 @@ virNetDevSysfsDeviceFile(char **pf_sysfs_device_link, 
const char *ifname,
  }


+static bool
+virNetDevIsPCIDevice(const char *devName)
+{
+char *vfSysfsDevicePath = NULL;
+bool ret;
+
+if (virNetDevSysfsFile(, devName, "device/subsystem") < 
0)
+return false;


See below for ^this.


+
+ret = virPCIIsPCIDevice(vfSysfsDevicePath);
+VIR_FREE(vfSysfsDevicePath);
+return ret;
+}
+
+
  static virPCIDevicePtr
  virNetDevGetPCIDevice(const char *devName)
  {
@@ -3236,14 +3251,18 @@ virNetDevSwitchdevFeature(const char *ifname,
  if (is_vf == 1 && virNetDevGetPhysicalFunction(ifname, ) < 0)
  goto cleanup;

-pci_device_ptr = pfname ? virNetDevGetPCIDevice(pfname) :
-  virNetDevGetPCIDevice(ifname);


I'd probably leave this as is, virNetDevGetPCIDevice already calls the
virNetDevSysfsFile, then you'd have something like virNetDevIsPCIDevice instead
of virPCIisPCIDevice (since those methods already assume a PCI device which we
don't know at this point). Something like this:

virNetDevIsPCIDevice(const char *device)
{
 char *subsystem_link;

 virAsprintf(_link, "%s/subsystem", device);

 
}

virNetDevGetPCIDevice
{
 if (virNetDevSysfsFile(, devname, "device") < 0)
 goto cleanup;

 if (virNetDevIsPCIDevice(vfSysfsDevicePath) < 0)
 goto cleanup;

 
}


Yes, good point about functions in virpci.c already assuming a PCI device. I've 
changed the code as you suggested.





+if (pfname == NULL && VIR_STRDUP(pfname, ifname) < 0)
+goto cleanup;
+


^This hunk would be unnecessary then.


  /* No PCI device, then no feature bit to check/add */
-if (pci_device_ptr == NULL) {
+if (!virNetDevIsPCIDevice(pfname)) {


^This one too...


  ret = 0;
  goto cleanup;
  }

+if ((pci_device_ptr = virNetDevGetPCIDevice(pfname)) == NULL)
+goto cleanup;
+
  if (!(nl_msg = nlmsg_alloc_simple(family_id,
NLM_F_REQUEST | NLM_F_ACK))) {
  virReportOOMError();
diff --git a/src/util/virpci.c b/src/util/virpci.c
index fe57bef32..f22d89cd7 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -2651,6 +2651,38 @@ virPCIGetDeviceAddressFromSysfsLink(const char 
*device_link)
  return bdf;
  }

+/**
+ * virPCIIsPCIDevice:
+ * @device_link: sysfs path for the device
+ *
+ * Returns true if the device specified in @device_link sysfs
+ * path is a PCI device, otherwise returns false.
+ */
+bool
+virPCIIsPCIDevice(const char *device_link)


This could still be part of the virnetdev module and 

[libvirt] [PATCH V3] nodedev: Fix failing to parse PCI address for non-PCI network devices

[Jim Fehlig]

Commit 8708ca01c added virNetDevSwitchdevFeature() to check if a network
device has Switchdev capabilities. virNetDevSwitchdevFeature() attempts
to retrieve the PCI device associated with the network device, ignoring
non-PCI devices. It does so via the following call chain

  virNetDevSwitchdevFeature()->virNetDevGetPCIDevice()->
  virPCIGetDeviceAddressFromSysfsLink()

For non-PCI network devices (qeth, Xen vif, etc),
virPCIGetDeviceAddressFromSysfsLink() will report an error when
virPCIDeviceAddressParse() fails. virPCIDeviceAddressParse() also
logs an error. After commit 8708ca01c there are now two errors reported
for each non-PCI network device even though the errors are harmless.

To avoid the errors, introduce virNetDevIsPCIDevice() and use it in
virNetDevGetPCIDevice() before attempting to retrieve the associated
PCI device. virNetDevIsPCIDevice() uses the 'subsystem' property of the
device to determine if it is PCI. See the sysfs rules in kernel
documentation for more details

https://www.kernel.org/doc/html/latest/admin-guide/sysfs-rules.html
---

V2: https://www.redhat.com/archives/libvir-list/2018-January/msg00233.html

Changes in V3:
Implement checking if netdev is PCI in virnetdev.c instead of virpci.c

Addressed other comments from eskultet

 src/util/virnetdev.c | 46 ++
 1 file changed, 46 insertions(+)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index eb2d119bf..baf4a71fe 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -22,6 +22,7 @@
 
 #include 
 
+#include "dirname.h"
 #include "virnetdev.h"
 #include "virnetlink.h"
 #include "virmacaddr.h"
@@ -1147,6 +1148,48 @@ virNetDevSysfsDeviceFile(char **pf_sysfs_device_link, 
const char *ifname,
 return 0;
 }
 
+/**
+ * Determine if the device path specified in devpath is a PCI Device
+ * by resolving the 'subsystem'-link in devpath and looking for
+ * 'pci' in the last component. For more information see the rules
+ * for accessing sysfs in the kernel docs
+ *
+ * https://www.kernel.org/doc/html/latest/admin-guide/sysfs-rules.html
+ *
+ * Returns true if devpath's susbsystem is pci, false otherwise.
+ */
+static bool
+virNetDevIsPCIDevice(const char *devpath)
+{
+char *subsys_link = NULL;
+char *abs_path = NULL;
+char *subsys = NULL;
+bool ret = false;
+
+if (virAsprintf(_link, "%s/subsystem", devpath) < 0)
+return false;
+
+if (!virFileExists(subsys_link))
+goto cleanup;
+
+if (virFileIsLink(subsys_link) != 1)
+goto cleanup;
+
+if (virFileResolveLink(subsys_link, _path) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Unable to resolve device subsystem symlink %s"),
+   subsys_link);
+goto cleanup;
+}
+
+subsys = last_component(abs_path);
+ret = STRPREFIX(subsys, "pci");
+
+ cleanup:
+VIR_FREE(subsys_link);
+VIR_FREE(abs_path);
+return ret;
+}
 
 static virPCIDevicePtr
 virNetDevGetPCIDevice(const char *devName)
@@ -1158,6 +1201,9 @@ virNetDevGetPCIDevice(const char *devName)
 if (virNetDevSysfsFile(, devName, "device") < 0)
 goto cleanup;
 
+if (!virNetDevIsPCIDevice(vfSysfsDevicePath))
+goto cleanup;
+
 vfPCIAddr = virPCIGetDeviceAddressFromSysfsLink(vfSysfsDevicePath);
 if (!vfPCIAddr)
 goto cleanup;
-- 
2.15.1

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

[libvirt] [PATCH v2 2/2] qemu: add support for generating SMBIOS OEM strings command line

[Daniel P. Berrange]

This wires up the previously added OEM strings XML schema to be able to
generate comamnd line args for QEMU. This requires QEMU >= 2.12 release
containing this patch:

  commit 2d6dcbf93fb01b4a7f45a93d276d4d74b16392dd
  Author: Daniel P. Berrange 
  Date:   Sat Oct 28 21:51:36 2017 +0100

smbios: support setting OEM strings table

Signed-off-by: Daniel P. Berrange 
---
 src/qemu/qemu_command.c | 28 
 tests/qemuxml2argvdata/smbios.args  |  2 ++
 tests/qemuxml2argvdata/smbios.xml   |  5 +
 tests/qemuxml2xmloutdata/smbios.xml |  5 +
 4 files changed, 40 insertions(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b8aede32d2..96bd0ad8ee 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6142,6 +6142,26 @@ qemuBuildSmbiosBaseBoardStr(virSysinfoBaseBoardDefPtr 
def)
 }
 
 
+static char *
+qemuBuildSmbiosOEMStringsStr(virSysinfoOEMStringsDefPtr def)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+size_t i;
+
+if (!def)
+return NULL;
+
+virBufferAddLit(, "type=11");
+
+for (i = 0; i < def->nvalues; i++) {
+virBufferAddLit(, ",value=");
+virQEMUBuildBufferEscapeComma(, def->values[i]);
+}
+
+return virBufferContentAndReset();
+}
+
+
 static int
 qemuBuildSmbiosCommandLine(virCommandPtr cmd,
virQEMUDriverPtr driver,
@@ -6212,6 +6232,14 @@ qemuBuildSmbiosCommandLine(virCommandPtr cmd,
 virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL);
 VIR_FREE(smbioscmd);
 }
+
+if (source->oemStrings) {
+if (!(smbioscmd = 
qemuBuildSmbiosOEMStringsStr(source->oemStrings)))
+return -1;
+
+virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL);
+VIR_FREE(smbioscmd);
+}
 }
 
 return 0;
diff --git a/tests/qemuxml2argvdata/smbios.args 
b/tests/qemuxml2argvdata/smbios.args
index 3d94a109f9..d27d436a3a 100644
--- a/tests/qemuxml2argvdata/smbios.args
+++ b/tests/qemuxml2argvdata/smbios.args
@@ -17,6 +17,8 @@ serial=32dfcb37-5af1-552b-357c-be8c3aa38310,\
 uuid=c7a5fdbd-edaf-9455-926a-d65c16db1809,sku=1234567890,family=Red Hat' \
 -smbios 'type=2,manufacturer=Hewlett-Packard,product=0B4Ch,version=D,\
 serial=CZC1065993,asset=CZC1065993,location=Upside down' \
+-smbios 'type=11,value=Hello,value=World,value=This is,,\
+ more tricky value=escaped' \
 -nographic \
 -nodefaults \
 -chardev 
socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
diff --git a/tests/qemuxml2argvdata/smbios.xml 
b/tests/qemuxml2argvdata/smbios.xml
index c12477dcb5..319bdf61df 100644
--- a/tests/qemuxml2argvdata/smbios.xml
+++ b/tests/qemuxml2argvdata/smbios.xml
@@ -26,6 +26,11 @@
   CZC1065993
   Upside down
 
+
+  Hello
+  World
+  This is, more tricky value=escaped
+
   
   
 hvm
diff --git a/tests/qemuxml2xmloutdata/smbios.xml 
b/tests/qemuxml2xmloutdata/smbios.xml
index d21f6275f0..cbe616c7da 100644
--- a/tests/qemuxml2xmloutdata/smbios.xml
+++ b/tests/qemuxml2xmloutdata/smbios.xml
@@ -26,6 +26,11 @@
   CZC1065993
   Upside down
 
+
+  Hello
+  World
+  This is, more tricky value=escaped
+
   
   
 hvm
-- 
2.14.3

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

[libvirt] [PATCH v2 0/2] Support SMBIOS OEM strings

[Daniel P. Berrange]

A followup to

  https://www.redhat.com/archives/libvir-list/2017-November/msg00720.html

The QEMU patch is now merged (for next 2.12 release).

Since v2:

 - Remove redundant error message report
 - Split QEMU from XML parts of patch

Daniel P. Berrange (2):
  conf: add support for setting OEM strings SMBIOS data fields
  qemu: add support for generating SMBIOS OEM strings command line

 docs/formatdomain.html.in   | 13 ++
 docs/schemas/domaincommon.rng   |  9 +++
 src/conf/domain_conf.c  | 47 +
 src/qemu/qemu_command.c | 28 ++
 src/util/virsysinfo.c   | 33 ++
 src/util/virsysinfo.h   | 10 
 tests/qemuxml2argvdata/smbios.args  |  2 ++
 tests/qemuxml2argvdata/smbios.xml   |  5 
 tests/qemuxml2xmloutdata/smbios.xml |  5 
 9 files changed, 152 insertions(+)

-- 
2.14.3

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

[libvirt] [PATCH v2 1/2] conf: add support for setting OEM strings SMBIOS data fields

[Daniel P. Berrange]

The OEM strings table in SMBIOS allows the vendor to pass arbitrary
strings into the guest OS. This can be used as a way to pass data to an
application like cloud-init, or potentially as an alternative to the
kernel command line for OS installers where you can't modify the install
ISO image to change the kernel args.

As an example, consider if cloud-init and anaconda supported OEM strings
you could use something like


  cloud-init:ds=nocloud-net;s=http://10.10.0.1:8000/
  
anaconda:method=http://dl.fedoraproject.org/pub/fedora/linux/releases/25/x86_64/os


use of a application specific prefix as illustrated above is
recommended, but not mandated, so that an app can reliably identify
which of the many OEM strings are targetted at it.

Signed-off-by: Daniel P. Berrange 
---
 docs/formatdomain.html.in | 13 
 docs/schemas/domaincommon.rng |  9 +
 src/conf/domain_conf.c| 47 +++
 src/util/virsysinfo.c | 33 ++
 src/util/virsysinfo.h | 10 +
 5 files changed, 112 insertions(+)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index d272cc1ba6..6af2d26209 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -411,6 +411,10 @@
 entry name='version'0B98401 Pro/entry
 entry name='serial'W1KS427111E/entry
   /baseBoard
+  oemStrings
+entrymyappname:some arbitrary data/entry
+entryotherappname:more arbitrary data/entry
+  /oemStrings
 /sysinfo
 ...
 
@@ -498,6 +502,15 @@
 validation and date format checking, all values are
 passed as strings to the hypervisor driver.
   
+  oemStrings
+  
+This is block 11 of SMBIOS. This element should appear once and
+can have multiple entry child elements, each providing
+arbitrary string data. There are no restrictions on what data can
+be provided in the entries, however, if the data is intended to be
+consumed by an application in the guest, it is recommended to use
+the application name as a prefix in the string.
+  
 
   
 
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index f22c932f6c..a154b5a462 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4857,6 +4857,15 @@
 
   
 
+
+  
+
+  
+
+  
+
+  
+
   
 
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a1c25060f9..6c0ad1ed7d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14461,6 +14461,42 @@ virSysinfoBaseBoardParseXML(xmlXPathContextPtr ctxt,
 return ret;
 }
 
+static int
+virSysinfoOEMStringsParseXML(xmlNodePtr node,
+ xmlXPathContextPtr ctxt,
+ virSysinfoOEMStringsDefPtr *oem)
+{
+int ret = -1;
+virSysinfoOEMStringsDefPtr def;
+xmlNodePtr *strings = NULL;
+int nstrings;
+size_t i;
+
+nstrings = virXPathNodeSet("./entry", ctxt, );
+if (nstrings < 0)
+return -1;
+if (nstrings == 0)
+return 0;
+
+if (VIR_ALLOC(def) < 0)
+goto cleanup;
+
+if (VIR_ALLOC_N(def->values, nstrings) < 0)
+goto cleanup;
+
+def->nvalues = nstrings;
+for (i = 0; i < nstrings; i++)
+def->values[i] = virXMLNodeContentString(strings[i]);
+
+*oem = def;
+def = NULL;
+ret = 0;
+ cleanup:
+VIR_FREE(strings);
+virSysinfoOEMStringsDefFree(def);
+return ret;
+}
+
 static virSysinfoDefPtr
 virSysinfoParseXML(xmlNodePtr node,
   xmlXPathContextPtr ctxt,
@@ -14519,6 +14555,17 @@ virSysinfoParseXML(xmlNodePtr node,
 if (virSysinfoBaseBoardParseXML(ctxt, >baseBoard, >nbaseBoard) < 
0)
 goto error;
 
+/* Extract system related metadata */
+if ((tmpnode = virXPathNode("./oemStrings[1]", ctxt)) != NULL) {
+oldnode = ctxt->node;
+ctxt->node = tmpnode;
+if (virSysinfoOEMStringsParseXML(tmpnode, ctxt, >oemStrings) < 0) 
{
+ctxt->node = oldnode;
+goto error;
+}
+ctxt->node = oldnode;
+}
+
  cleanup:
 VIR_FREE(type);
 return def;
diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c
index 1fbdd778f9..60765c38de 100644
--- a/src/util/virsysinfo.c
+++ b/src/util/virsysinfo.c
@@ -108,6 +108,18 @@ void virSysinfoBaseBoardDefClear(virSysinfoBaseBoardDefPtr 
def)
 VIR_FREE(def->location);
 }
 
+void virSysinfoOEMStringsDefFree(virSysinfoOEMStringsDefPtr def)
+{
+size_t i;
+
+if (def == NULL)
+return;
+
+for (i = 0; i < def->nvalues; i++)
+VIR_FREE(def->values[i]);
+VIR_FREE(def->values);
+}
+
 /**
  * virSysinfoDefFree:
  * @def: a sysinfo structure
@@ -157,6 +169,8 @@ void 

Re: [libvirt] [PATCH] news: Update for 4.0.0

[Michal Privoznik]

On 01/17/2018 05:19 PM, Andrea Bolognani wrote:
> On Wed, 2018-01-17 at 17:12 +0100, Michal Privoznik wrote:
>> On 01/17/2018 05:01 PM, Andrea Bolognani wrote:
>>>
>>>  
>>> +  
>>> +
>>> +  tools: Provide bash completion support
>>> +
>>> +
>>> +  Both virsh and virt-admin now implement
>>> +  basic bash completion support.
>>> +
>>> +  
>>
>> .. I wanted to advertise this once these are merged:
>>
>> https://www.redhat.com/archives/libvir-list/2018-January/msg00436.html
>>
>> so that users can have full experience.
> 
> I know that there are going to be more features down the line,
> that's why I mentioned it's "basic" support ;)
> 
> I can pull that from the release notes, if you'd prefer, but it
> makes sense to me that we would both list basic bash completion
> as a new feature for this release, and list much improved bash
> completion as an improvement next release. It's also more accurate
> from the "code archeology" point of view, assuming we care about
> that at all.
> 

Okay, fair enough. Leave that in and once the patches are merged we can
advertise "enhanced support" :-)

Michal

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

Re: [libvirt] [PATCH 01/17] cpu: add CPU features for indirect branch prediction protection

[Eric Blake]

On 01/09/2018 04:45 PM, Jiri Denemark wrote:
> From: Paolo Bonzini 
> 
> Added in QEMU commits TBD and TBD.

I'm assuming the TBD will be resolved before you push?

> 
> Signed-off-by: Paolo Bonzini 
> Signed-off-by: Jiri Denemark 
> ---
>  src/cpu/cpu_map.xml | 8 
>  1 file changed, 8 insertions(+)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



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

Re: [libvirt] [PATCH] qemu: avoid denial of service reading from QEMU monitor (CVE-2018-xxxx)

[Eric Blake]

On 01/17/2018 10:13 AM, Michal Privoznik wrote:
> On 01/16/2018 06:01 PM, Daniel P. Berrange wrote:
>> We read from QEMU until seeing a \r\n pair to indicate a completed reply
>> or event. To avoid memory denial-of-service though, we must have a size
>> limit on amount of data we buffer. 10 MB is large enough that it ought
>> to cope with normal QEMU replies, and small enough that we're not
>> consuming unreasonable mem.
>>

>>
> 
> ACK, although is this really a CVE? Doesn't look that harmful to me. I
> mean, owning qemu is not that easy, is it?

We treat qemu as untrusted, in case a guest escapes qemu due to some
other CVE.  If a guest really did cause qemu to emit unbounded QMP text,
and it starves libvirtd, then that guest has mounted a denial of service
against anything else libvirtd is starved from doing.  So yes, in my
opinion it is a CVE, even if it is an unlikely case because it won't
trigger without a flaw in more than one layer.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



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

Re: [libvirt] [PATCH] news: Update for 4.0.0

[Andrea Bolognani]

On Wed, 2018-01-17 at 17:12 +0100, Michal Privoznik wrote:
> On 01/17/2018 05:01 PM, Andrea Bolognani wrote:
> >
> >  
> > +  
> > +
> > +  tools: Provide bash completion support
> > +
> > +
> > +  Both virsh and virt-admin now implement
> > +  basic bash completion support.
> > +
> > +  
> 
> .. I wanted to advertise this once these are merged:
> 
> https://www.redhat.com/archives/libvir-list/2018-January/msg00436.html
> 
> so that users can have full experience.

I know that there are going to be more features down the line,
that's why I mentioned it's "basic" support ;)

I can pull that from the release notes, if you'd prefer, but it
makes sense to me that we would both list basic bash completion
as a new feature for this release, and list much improved bash
completion as an improvement next release. It's also more accurate
from the "code archeology" point of view, assuming we care about
that at all.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] qemu: avoid denial of service reading from QEMU monitor (CVE-2018-xxxx)

[Daniel P. Berrange]

On Wed, Jan 17, 2018 at 05:13:06PM +0100, Michal Privoznik wrote:
> On 01/16/2018 06:01 PM, Daniel P. Berrange wrote:
> > We read from QEMU until seeing a \r\n pair to indicate a completed reply
> > or event. To avoid memory denial-of-service though, we must have a size
> > limit on amount of data we buffer. 10 MB is large enough that it ought
> > to cope with normal QEMU replies, and small enough that we're not
> > consuming unreasonable mem.
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  src/qemu/qemu_monitor.c | 15 +++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> > index 046caf001c..85c7d68a13 100644
> > --- a/src/qemu/qemu_monitor.c
> > +++ b/src/qemu/qemu_monitor.c
> > @@ -55,6 +55,15 @@ VIR_LOG_INIT("qemu.qemu_monitor");
> >  #define DEBUG_IO 0
> >  #define DEBUG_RAW_IO 0
> >  
> > +/* We read from QEMU until seeing a \r\n pair to indicate a
> > + * completed reply or event. To avoid memory denial-of-service
> > + * though, we must have a size limit on amount of data we
> > + * buffer. 10 MB is large enough that it ought to cope with
> > + * normal QEMU replies, and small enough that we're not
> > + * consuming unreasonable mem.
> > + */
> > +#define QEMU_MONITOR_MAX_RESPONSE (10 * 1024 * 1024)
> > +
> >  struct _qemuMonitor {
> >  virObjectLockable parent;
> >  
> > @@ -575,6 +584,12 @@ qemuMonitorIORead(qemuMonitorPtr mon)
> >  int ret = 0;
> >  
> >  if (avail < 1024) {
> > +if (mon->bufferLength >= QEMU_MONITOR_MAX_RESPONSE) {
> > +virReportSystemError(ERANGE,
> > + _("No complete monitor response found in 
> > %d bytes"),
> > + QEMU_MONITOR_MAX_RESPONSE);
> > +return -1;
> > +}
> >  if (VIR_REALLOC_N(mon->buffer,
> >mon->bufferLength + 1024) < 0)
> >  return -1;
> > 
> 
> ACK, although is this really a CVE? Doesn't look that harmful to me. I
> mean, owning qemu is not that easy, is it?

There are a never ending stream of CVEs in QEMU guest device models.
These are mitigated by SELinux preventing a compromised QEMU from attacking
resources on the host it isn't granted access to. So attention would naturally
focus on attacking things it already has access to like the libvirt monitor
connection. Memory denial of service in libvirt is not too serious, but
still a CVE bug. Worse would be if libvirtd has buffer overflow/crash
in parsing a JSON response...

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH] qemu: avoid denial of service reading from QEMU monitor (CVE-2018-xxxx)

[Michal Privoznik]

On 01/16/2018 06:01 PM, Daniel P. Berrange wrote:
> We read from QEMU until seeing a \r\n pair to indicate a completed reply
> or event. To avoid memory denial-of-service though, we must have a size
> limit on amount of data we buffer. 10 MB is large enough that it ought
> to cope with normal QEMU replies, and small enough that we're not
> consuming unreasonable mem.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/qemu/qemu_monitor.c | 15 +++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 046caf001c..85c7d68a13 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -55,6 +55,15 @@ VIR_LOG_INIT("qemu.qemu_monitor");
>  #define DEBUG_IO 0
>  #define DEBUG_RAW_IO 0
>  
> +/* We read from QEMU until seeing a \r\n pair to indicate a
> + * completed reply or event. To avoid memory denial-of-service
> + * though, we must have a size limit on amount of data we
> + * buffer. 10 MB is large enough that it ought to cope with
> + * normal QEMU replies, and small enough that we're not
> + * consuming unreasonable mem.
> + */
> +#define QEMU_MONITOR_MAX_RESPONSE (10 * 1024 * 1024)
> +
>  struct _qemuMonitor {
>  virObjectLockable parent;
>  
> @@ -575,6 +584,12 @@ qemuMonitorIORead(qemuMonitorPtr mon)
>  int ret = 0;
>  
>  if (avail < 1024) {
> +if (mon->bufferLength >= QEMU_MONITOR_MAX_RESPONSE) {
> +virReportSystemError(ERANGE,
> + _("No complete monitor response found in %d 
> bytes"),
> + QEMU_MONITOR_MAX_RESPONSE);
> +return -1;
> +}
>  if (VIR_REALLOC_N(mon->buffer,
>mon->bufferLength + 1024) < 0)
>  return -1;
> 

ACK, although is this really a CVE? Doesn't look that harmful to me. I
mean, owning qemu is not that easy, is it?

Michal

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

Re: [libvirt] [PATCH] qemu: qemuDomainNamespaceUnlinkPaths: Return 0 in case of success

[Michal Privoznik]

On 01/17/2018 04:47 PM, Marc Hartmayer wrote:
> Commit 7a931a4204af refactored the code and probably forgot to add
> this line.
> 
> Signed-off-by: Marc Hartmayer 
> Reviewed-by: Boris Fiuczynski 
> ---
>  src/qemu/qemu_domain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 5c171e4cbd6c..441bf5935b14 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -10212,7 +10212,7 @@ qemuDomainNamespaceUnlinkPaths(virDomainObjPtr vm,
>  goto cleanup;
>  }
>  
> -
> +ret = 0;
>   cleanup:
>  virStringListFreeCount(devMountsPath, ndevMountsPath);
>  virObjectUnref(cfg);
> 

Ooops. Yes. ACKed and pushed. Definitely a release material.

Michal

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

Re: [libvirt] [PATCH] news: Update for 4.0.0

[Michal Privoznik]

On 01/17/2018 05:01 PM, Andrea Bolognani wrote:
> As usual, a bunch of changes slipped through the cracks during the
> development cycle. Update the release notes to include at least the
> most notable.
> 
> Signed-off-by: Andrea Bolognani 
> ---
> I'll push this tomorrow morning under the "can't possibly be worse
> than leaving it alone" rule, so that it makes it into the release,
> unless I get feedback earlier.
> 
>  docs/news.xml | 50 ++
>  1 file changed, 50 insertions(+)


ACK, although ..
> 
> diff --git a/docs/news.xml b/docs/news.xml
> index 064b9ae83..d034be99a 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -35,8 +35,48 @@
>  
>
>  
> +  
> +
> +  tools: Provide bash completion support
> +
> +
> +  Both virsh and virt-admin now implement
> +  basic bash completion support.
> +
> +  

.. I wanted to advertise this once these are merged:

https://www.redhat.com/archives/libvir-list/2018-January/msg00436.html

so that users can have full experience.

Michal

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

Re: [libvirt] [PATCH 00/17] CPU models and features for Spectre, CVE-2017-5715

[Jiri Denemark]

On Wed, Jan 10, 2018 at 10:52:29 +0100, Pavel Hrdina wrote:
> On Tue, Jan 09, 2018 at 11:45:13PM +0100, Jiri Denemark wrote:
> > This is the libvirt's part of the changes related to CVE-2017-5715. The
> > new models can be used to pass the protective CPU features to guests.
> > But remember, the host CPU microcode, host kernel, QEMU, and libvirt all
> > need to be updated for this to be any useful.
> > 
> > Based on a patch from Paolo Bonzini.
> > 
> > See QEMU patches from Eduardo for more details:
> > https://patchew.org/QEMU/20180109154519.25634-1-ehabk...@redhat.com/
> 
> I guess that you will wait with pushing until the QEMU patches are
> accepted and pushed as well.
> 
> Reviewed-by: Pavel Hrdina 

Thanks. All QEMU patches except for EPYC-IBPB CPU model are queued in
Eduardo's x86-next and a pull request is coming soon. I pushed the first
16 patches, i.e., without EPYC-IBPB.

Jirka

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

[libvirt] [PATCH] news: Update for 4.0.0

[Andrea Bolognani]

As usual, a bunch of changes slipped through the cracks during the
development cycle. Update the release notes to include at least the
most notable.

Signed-off-by: Andrea Bolognani 
---
I'll push this tomorrow morning under the "can't possibly be worse
than leaving it alone" rule, so that it makes it into the release,
unless I get feedback earlier.

 docs/news.xml | 50 ++
 1 file changed, 50 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 064b9ae83..d034be99a 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -35,8 +35,48 @@
 
   
 
+  
+
+  tools: Provide bash completion support
+
+
+  Both virsh and virt-admin now implement
+  basic bash completion support.
+
+  
+  
+
+  qemu: Refresh capabilities on host microcode update
+
+
+  A microcode update can cause the CPUID bits to change; therefore,
+  the capabilities cache should be rebuilt when such an update is
+  detected on the host.
+
+  
+  
+
+  lxc: Set hostname based on container name
+
+  
 
 
+  
+
+  CPU frequency reporting improvements
+
+
+  The CPU frequency will now be reported by virsh nodeinfo
+  and other tools for s390 hosts; at the same time; CPU frequency has
+  been disabled on aarch64 hosts because there's no way to detect it
+  reliably.
+
+  
+  
+
+  libxl: Mark domain0 as persistent
+
+  
   
 
   Xen: Add support for multiple IP addresses on interface devices
@@ -49,6 +89,16 @@
   
 
 
+  
+
+  qemu: Enforce vCPU hotplug granularity constraints
+
+
+  QEMU 2.7 and newer don't allow guests to start unless the initial
+  vCPUs count is a multiple of the vCPU hotplug granularity, so
+  validate it and report an error if needed.
+
+  
 
   
   
-- 
2.14.3

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

[libvirt] [PATCH] qemu: qemuDomainNamespaceUnlinkPaths: Return 0 in case of success

[Marc Hartmayer]

Commit 7a931a4204af refactored the code and probably forgot to add
this line.

Signed-off-by: Marc Hartmayer 
Reviewed-by: Boris Fiuczynski 
---
 src/qemu/qemu_domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 5c171e4cbd6c..441bf5935b14 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -10212,7 +10212,7 @@ qemuDomainNamespaceUnlinkPaths(virDomainObjPtr vm,
 goto cleanup;
 }
 
-
+ret = 0;
  cleanup:
 virStringListFreeCount(devMountsPath, ndevMountsPath);
 virObjectUnref(cfg);
-- 
2.13.4

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

[libvirt] [PATCH] apparmor: allow libvirt to send term signal to unconfined

[Guido Günther]

Otherwise stopping domains with qemu://session fails like

[164012.338157] audit: type=1400 audit(1516202208.784:99): apparmor="DENIED" 
operation="signal" profile="/usr/sbin/libvirtd" pid=18835 comm="libvirtd" 
requested_mask="send" denied_mask="send" signal=term peer="unconfined"
---
 examples/apparmor/usr.sbin.libvirtd | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/examples/apparmor/usr.sbin.libvirtd 
b/examples/apparmor/usr.sbin.libvirtd
index 0ddec3f6e2..be4fabf905 100644
--- a/examples/apparmor/usr.sbin.libvirtd
+++ b/examples/apparmor/usr.sbin.libvirtd
@@ -63,7 +63,7 @@
 
   signal (send) peer=/usr/sbin/dnsmasq,
   signal (read, send) peer=libvirt-*,
-  signal (send) set=("kill") peer=unconfined,
+  signal (send) set=("kill", "term") peer=unconfined,
 
   # Very lenient profile for libvirtd since we want to first focus on confining
   # the guests. Guests will have a very restricted profile.
-- 
2.15.1

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

Re: [libvirt] [PATCH] AppArmor: Allow libvirtd to kill unconfined processes

[Guido Günther]

Hi,
On Mon, Jan 15, 2018 at 07:43:56AM +0100, intrigeri wrote:
> Christian Ehrhardt:
> > I recently had spotted this issue and discussed on IRC but couldn't
> > recreate after a while when I wanted to debug.
> 
> I've seen it the last few times I've started libvirtd.service on two
> different Debian sid ("unstable") systems.
> 
> > But the reason and the rule totally make sense.
> 
> > It is a bit unfortunate as it allows random kills essentially, so if
> > anyone is up to we might be better up to spawn those capability
> > probers with a named profile that we can refer to here.
> 
> Yes, this would be ideal. Sadly, this requires C programming skills so
> it's out of my league.
> 
> > But until then the rule here is required to not get into awkward situations.
> 
> > +1 from me, thanks intrigeri
> 
> Thanks :)

Pushed now.
 -- Guido

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

Re: [libvirt] [PATCH libvirt] qemu: Fix segmentation fault when attaching a non iSCSI host device

[Marc Hartmayer]

On Wed, Jan 17, 2018 at 02:39 PM +0100, John Ferlan  wrote:
> On 01/17/2018 07:26 AM, Marc Hartmayer wrote:
>> Add a check if it's a iSCSI hostdev and if it's not then don't use the
>> union member 'iscsi'. The segmentation fault occured when accessing
>> secinfo->type, but this can vary from case to case.
>>
>> Signed-off-by: Marc Hartmayer 
>> Reviewed-by: Bjoern Walk 
>> Reviewed-by: Boris Fiuczynski 
>> ---
>>  src/qemu/qemu_hotplug.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>
> Thanks - thought I got this with '6050affb7', but right it must be
> protocol iSCSI .
>
> Reviewed-by: John Ferlan 
>
> (and safe for freeze - I'll push shortly),

Thanks.

>
> John
>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 6dc16a1054af..83d0e1c71a8e 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -2343,8 +2343,7 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
>>  bool secobjAdded = false;
>>  virJSONValuePtr secobjProps = NULL;
>>  virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
>> -virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = >u.iscsi;
>> -qemuDomainStorageSourcePrivatePtr srcPriv;
>> +qemuDomainStorageSourcePrivatePtr srcPriv = NULL;
>>  qemuDomainSecretInfoPtr secinfo = NULL;
>>
>>  if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) {
>> @@ -2386,7 +2385,8 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
>>  if (qemuDomainSecretHostdevPrepare(conn, priv, hostdev) < 0)
>>  goto cleanup;
>>
>> -srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src);
>> +if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)
>> +srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(scsisrc->u.iscsi.src);
>>  if (srcPriv)
>>  secinfo = srcPriv->secinfo;
>
> Probably could combine into one if statement...

Yep.

>
>
>>  if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
>>
>
--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


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

Re: [libvirt] [PATCH] remove bogus casts of arg to g_object_ref

[Daniel P. Berrange]

On Wed, Jan 17, 2018 at 02:43:17PM +, Daniel P. Berrange wrote:
> Latest version of glib uses typeof() magic to cast the
> return value of g_object_ref to match its argument,
> instead of returning a 'void *'. A few places in the
> code were casting the arg to G_OBJECT() which was then
> incompatible with the variable we assigned the result
> to. The parameter casts were always redundant so just
> remove them.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  libvirt-gconfig/libvirt-gconfig-object.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Opps, should have said, I pushed this as a build fix...

> 
> diff --git a/libvirt-gconfig/libvirt-gconfig-object.c 
> b/libvirt-gconfig/libvirt-gconfig-object.c
> index 851e35c..ca2c6e6 100644
> --- a/libvirt-gconfig/libvirt-gconfig-object.c
> +++ b/libvirt-gconfig/libvirt-gconfig-object.c
> @@ -572,7 +572,7 @@ gvir_config_object_set_node_content(GVirConfigObject 
> *object,
>  node = gvir_config_object_replace_child(object, node_name);
>  g_return_if_fail(node != NULL);
>  } else {
> -node = g_object_ref(G_OBJECT(object));
> +node = g_object_ref(object);
>  }
>  encoded_data = xmlEncodeEntitiesReentrant(node->priv->node->doc,
>(xmlChar *)value);
> @@ -896,7 +896,7 @@ gvir_config_object_attach(GVirConfigObject *parent, 
> GVirConfigObject *child, gbo
>  child->priv->doc = NULL;
>  }
>  if (parent->priv->doc != NULL) {
> -child->priv->doc = g_object_ref(G_OBJECT(parent->priv->doc));
> +child->priv->doc = g_object_ref(parent->priv->doc);
>  }
>  }
>  
> -- 
> 2.14.3
> 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [PATCH] remove bogus casts of arg to g_object_ref

[Daniel P. Berrange]

Latest version of glib uses typeof() magic to cast the
return value of g_object_ref to match its argument,
instead of returning a 'void *'. A few places in the
code were casting the arg to G_OBJECT() which was then
incompatible with the variable we assigned the result
to. The parameter casts were always redundant so just
remove them.

Signed-off-by: Daniel P. Berrange 
---
 libvirt-gconfig/libvirt-gconfig-object.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libvirt-gconfig/libvirt-gconfig-object.c 
b/libvirt-gconfig/libvirt-gconfig-object.c
index 851e35c..ca2c6e6 100644
--- a/libvirt-gconfig/libvirt-gconfig-object.c
+++ b/libvirt-gconfig/libvirt-gconfig-object.c
@@ -572,7 +572,7 @@ gvir_config_object_set_node_content(GVirConfigObject 
*object,
 node = gvir_config_object_replace_child(object, node_name);
 g_return_if_fail(node != NULL);
 } else {
-node = g_object_ref(G_OBJECT(object));
+node = g_object_ref(object);
 }
 encoded_data = xmlEncodeEntitiesReentrant(node->priv->node->doc,
   (xmlChar *)value);
@@ -896,7 +896,7 @@ gvir_config_object_attach(GVirConfigObject *parent, 
GVirConfigObject *child, gbo
 child->priv->doc = NULL;
 }
 if (parent->priv->doc != NULL) {
-child->priv->doc = g_object_ref(G_OBJECT(parent->priv->doc));
+child->priv->doc = g_object_ref(parent->priv->doc);
 }
 }
 
-- 
2.14.3

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

Re: [libvirt] [PATCH libvirt] qemu: Fix segmentation fault when attaching a non iSCSI host device

[John Ferlan]

On 01/17/2018 07:26 AM, Marc Hartmayer wrote:
> Add a check if it's a iSCSI hostdev and if it's not then don't use the
> union member 'iscsi'. The segmentation fault occured when accessing
> secinfo->type, but this can vary from case to case.
> 
> Signed-off-by: Marc Hartmayer 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Boris Fiuczynski 
> ---
>  src/qemu/qemu_hotplug.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 

Thanks - thought I got this with '6050affb7', but right it must be
protocol iSCSI .

Reviewed-by: John Ferlan 

(and safe for freeze - I'll push shortly),

John

> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 6dc16a1054af..83d0e1c71a8e 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2343,8 +2343,7 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
>  bool secobjAdded = false;
>  virJSONValuePtr secobjProps = NULL;
>  virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
> -virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = >u.iscsi;
> -qemuDomainStorageSourcePrivatePtr srcPriv;
> +qemuDomainStorageSourcePrivatePtr srcPriv = NULL;
>  qemuDomainSecretInfoPtr secinfo = NULL;
>  
>  if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) {
> @@ -2386,7 +2385,8 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
>  if (qemuDomainSecretHostdevPrepare(conn, priv, hostdev) < 0)
>  goto cleanup;
>  
> -srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src);
> +if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)
> +srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(scsisrc->u.iscsi.src);
>  if (srcPriv)
>  secinfo = srcPriv->secinfo;

Probably could combine into one if statement...


>  if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
> 

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

[libvirt] [PATCH libvirt] qemu: Fix segmentation fault when attaching a non iSCSI host device

[Marc Hartmayer]

Add a check if it's a iSCSI hostdev and if it's not then don't use the
union member 'iscsi'. The segmentation fault occured when accessing
secinfo->type, but this can vary from case to case.

Signed-off-by: Marc Hartmayer 
Reviewed-by: Bjoern Walk 
Reviewed-by: Boris Fiuczynski 
---
 src/qemu/qemu_hotplug.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 6dc16a1054af..83d0e1c71a8e 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2343,8 +2343,7 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
 bool secobjAdded = false;
 virJSONValuePtr secobjProps = NULL;
 virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
-virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = >u.iscsi;
-qemuDomainStorageSourcePrivatePtr srcPriv;
+qemuDomainStorageSourcePrivatePtr srcPriv = NULL;
 qemuDomainSecretInfoPtr secinfo = NULL;
 
 if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) {
@@ -2386,7 +2385,8 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
 if (qemuDomainSecretHostdevPrepare(conn, priv, hostdev) < 0)
 goto cleanup;
 
-srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src);
+if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)
+srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(scsisrc->u.iscsi.src);
 if (srcPriv)
 secinfo = srcPriv->secinfo;
 if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
-- 
2.13.4

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

[libvirt] New GIT repos for libvirt wiki & virt tools planet

[Daniel P. Berrange]

Hi Folks,

Just let you all know that I've moved various libvirt related websites over
to hosting on Red Hat's  OpenShift v3 infrastructure. With the new v3 that
is based on Kubernetes, I'm able to publish the GIT repos containing the
core content & software setup.

For wiki.libvirt.org:

  https://libvirt.org/git/?p=libvirt-wiki.git;a=summary

For planet.virt-tools.org:

  https://libvirt.org/git/?p=virttools-planet.git;a=summary

For www.virt-tools.org:

  https://libvirt.org/git/?p=virttools-web.git;a=summary

These are setup to mirror to github, and with a github webhook installed.
So if when changes to the *content*, it will get automatically deployed
to the live site by OpenShift.

NB, auto-deploy doesn't apply to the actual OpenShift configuration,
only the static config. I still need to manualy deploy OpenShift config
changes for security reasons :-)

The key benefit here is that adding people to the blog aggregator on
planet.virt-tools.org is now easier. Just send a patch for this file:

https://libvirt.org/git/?p=virttools-planet.git;a=blob;f=updater/virt-tools/config.ini;hb=HEAD

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH] rpm: updates wrt min required fedora version

[Andrea Bolognani]

On Thu, 2018-01-11 at 16:31 +, Daniel P. Berrange wrote:
> Update the min fedora to 25. Use a macro to record the min versions so that 
> the
> later error message is always in sync with the earlier version check. Clarify
> the comment that refers to guessing of dist which does not actually happen.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  libvirt.spec.in | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 7e1b6a27d3..713961573a 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1,10 +1,12 @@
>  # -*- rpm-spec -*-
>  
>  # This spec file assumes you are building on a Fedora or RHEL version
> -# that's still supported by the vendor: that means Fedora 23 or newer,
> -# or RHEL 6 or newer. It may need some tweaks for other distros.
> -# If neither fedora nor rhel was defined, try to guess them from dist
> -%if (0%{?fedora} && 0%{?fedora} >= 23) || (0%{?rhel} && 0%{?rhel} >= 6)
> +# that's still supported by the vendor. It may work on other distros
> +# or versions, but no effort will be made to ensure that going forward.
> +%define min_rhel 6
> +%define min_fedora 25

This should be 26 - Fedora 25 has been EOL'd already. Same in the
commit message, of course.

With that fixed,

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH RESEND v1 1/2] i386: Add Intel Processor Trace feature support

[Kang, Luwei]

> > > > On Mon, Jan 15, 2018 at 12:04:55 -0200, Eduardo Habkost wrote:
> > > > > CCing libvirt developers.
> > > > ...
> > > > > This case is slightly more problematic, however: the new feature
> > > > > is actually migratable (under very controlled circumstances)
> > > > > because of patch 2/2, but it is not migration-safe[1].  This
> > > > > means libvirt shouldn't include it in "host-model" expansion
> > > > > (which uses the query-cpu-model-expansion QMP command) until we
> > > > > make the feature migration-safe.
> > > > >
> > > > > For QEMU, this means the feature shouldn't be returned by
> > > > > "query-cpu-model-expansion type=static model=max" (but it can be
> > > > > returned by "query-cpu-model-expansion type=full model=max").
> > > > >
> > > > > Jiri, it looks like libvirt uses type=full on
> > > > > query-cpu-model-expansion on x86.  It needs to use
> > > > > type=static[2], or it will have no way to find out if a feature
> > > > > is migration-safe or not.
> > > > ...
> > > > > [2] It looks like libvirt uses type=full because it wants to get
> > > > > all QOM property aliases returned.  In this case, one
> > > > > solution for libvirt is to use:
> > > > >
> > > > > static_expansion = query_cpu_model_expansion(type=static, model)
> > > > > all_props = query_cpu_model_expansion(type=full,
> > > > > static_expansion)
> > > >
> > > > This is exactly what libvirt is doing (with model = "host") ever
> > > > since query-cpu-model-expansion support was implemented for x86.
> > >
> > > Oh, now I see that the x86 code uses
> > > QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC_FULL and not just 
> > > QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL.
> Nice!
> > >
> >
> > So, I need to add Intel PT feature in "X86CPUDefinition
> > builtin_x86_defs[]" so that we can get this CPUID in specific CPU
> > model not only "-cpu host". Is that right?
> 
> The problem is that you won't be able to add intel-pt to any CPU model unless 
> the feature is made migration-safe (by not calling
> kvm_arch_get_supported_cpuid() from cpu_x86_cpuid()).

Hi Eduardo,
Have some question need you help clear. What is "migration-safe" feature 
mean? I find all the feature which included in CPU model (builtin_x86_defs[]) 
will make "xcc->migration_safe = true;" in x86_cpu_cpudef_class_init(). If 
create a Skylake guest on Skylake HW and live migrate this guest to another 
machine with old HW(e.g Haswell). Can we think the new feature or cpu 
model(Skylake guest) which only supported in Skylake HW is safe?

> 
> What's missing here is to either: (a) make cpu_x86_cpuid() return 
> host-independent data (it can be constant, or it can be
> configurable on the command-line); or (b) add a mechanism to skip intel-pt 
> from "query-cpu-model-expansion type=static".

==> it can be constant:
I think it shouldn't be constant because Intel PT virtualization can only 
supported in Ice Lake hardware now. Intel PT cpuid info would be mask off on 
old platform.
==> or it can be configurable on the command-line:
Because of I didn't add this feature in any CPU model. We only can enable 
this feature by "-cpu Skylake-Server,+intel-pt" at present.

What about add a new cpu model name "Icelake" and add PT in this. Is that would 
be migration safe?

Thanks,
Luwei Kang

> Probably
> (a) is easier to implement, and it also makes the feature more useful (by 
> making it migration-safe).
> 
> >
> > Intel PT is first supported in Intel Core M and 5th generation Intel
> > Core processors that are based on the Intel micro-architecture code
> > name Broadwell but Intel PT use EPT is first supported in Ice Lake.
> > Intel PT virtualization depend on PT use EPT.  I will add Intel PT to
> > "Broadwell" CPU model and later to make sure a "Broadwell" guest can
> > use Intel PT if the host is Ice Lake.
> 
> The "if the host is Ice Lake" part is problematic.  On migration-safe CPU 
> models (all of them except "max" and "host"), the data seen
> on CPUID can't depend on the host at all.  It should depend only on the 
> machine-type + command-line options.
> 
> --
> Eduardo

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

Re: [libvirt] [PATCH] spec: enable bash completion only on new enough distros

[Daniel P. Berrange]

On Wed, Jan 17, 2018 at 10:40:19AM +0100, Pavel Hrdina wrote:
> RHEL-6 doesn't have bash-completion package by default, it has to be
> installed from EPEL.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  libvirt.spec.in | 10 ++
>  1 file changed, 10 insertions(+)

Reviewed-by: Daniel P. Berrange 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [jenkins-ci PATCH] guests: install bash-completion when building libvirt

[Daniel P. Berrange]

On Wed, Jan 17, 2018 at 10:42:41AM +0100, Pavel Hrdina wrote:
> Libvirt recently added bash-completion support.  On CentOS6 the
> package is available only from EPEL repositories.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  guests/vars/mappings.yml | 4 
>  guests/vars/projects/libvirt.yml | 1 +
>  2 files changed, 5 insertions(+)

Reviewed-by: Daniel P. Berrange 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [jenkins-ci PATCH] guests: install bash-completion when building libvirt

[Pavel Hrdina]

Libvirt recently added bash-completion support.  On CentOS6 the
package is available only from EPEL repositories.

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

diff --git a/guests/vars/mappings.yml b/guests/vars/mappings.yml
index e669b6c..440123c 100644
--- a/guests/vars/mappings.yml
+++ b/guests/vars/mappings.yml
@@ -53,6 +53,10 @@ mappings:
 pkg: avahi
 rpm: avahi-devel
 
+  bash-completion:
+default: bash-completion
+CentOS6:
+
   ccache:
 default: ccache
 CentOS:
diff --git a/guests/vars/projects/libvirt.yml b/guests/vars/projects/libvirt.yml
index 598dfc4..9f027f8 100644
--- a/guests/vars/projects/libvirt.yml
+++ b/guests/vars/projects/libvirt.yml
@@ -3,6 +3,7 @@ packages:
   - apparmor
   - augeas
   - avahi
+  - bash-completion
   - cyrus-sasl
   - device-mapper
   - dnsmasq
-- 
2.14.3

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

[libvirt] [PATCH] spec: enable bash completion only on new enough distros

[Pavel Hrdina]

RHEL-6 doesn't have bash-completion package by default, it has to be
installed from EPEL.

Signed-off-by: Pavel Hrdina 
---
 libvirt.spec.in | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index d4ef116b2d..ef96888d09 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -87,6 +87,7 @@
 %define with_libssh2   0%{!?_without_libssh2:0}
 %define with_wireshark 0%{!?_without_wireshark:0}
 %define with_libssh0%{!?_without_libssh:0}
+%define with_bash_completion  0%{!?_without_bash_completion:0}
 %define with_pm_utils  1
 
 # Finally set the OS / architecture specific special cases
@@ -190,6 +191,11 @@
 %define with_libssh 0%{!?_without_libssh:1}
 %endif
 
+# Enable bash-completion for new enough distros
+%if 0%{?fedora} || 0%{?rhel} >= 7
+%define with_bash_completion  0%{!?_without_bash_completion:1}
+%endif
+
 
 %if %{with_qemu} || %{with_lxc} || %{with_uml}
 # numad is used to manage the CPU and memory placement dynamically,
@@ -306,7 +312,9 @@ BuildRequires: xen-devel
 BuildRequires: libxml2-devel
 BuildRequires: libxslt
 BuildRequires: readline-devel
+%if %{with_bash_completion}
 BuildRequires: bash-completion >= 2.0
+%endif
 BuildRequires: ncurses-devel
 BuildRequires: gettext
 BuildRequires: libtasn1-devel
@@ -2048,7 +2056,9 @@ exit 0
 %{_datadir}/systemtap/tapset/libvirt_qemu_probes*.stp
 %{_datadir}/systemtap/tapset/libvirt_functions.stp
 
+%if %{with_bash_completion}
 %{_datadir}/bash-completion/completions/vsh
+%endif
 
 
 %if %{with_systemd}
-- 
2.14.3

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

[libvirt] Availability of libvirt-4.0.0 Release Candidate 2

[Daniel Veillard]

  I'm late but it is now tagged in git and signed tarball and rpms are pushed
to the usual place:

   ftp://libvirt.org/libvirt/


  This seems to work fine for me, I think I heard that the issue on MacOS
is fixed, so with a bit of luck we can get through and make the release.

  If everything looks good I will then push the 4.0.0 release tomorrow
evening my time, but please try and give some feedback on rc2 if you see any
issue !

  thanks in adavance,

Daniel

-- 
Daniel Veillard  | Red Hat Developers Tools http://developer.redhat.com/
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

[libvirt] [PATCHv4 RESEND] vhost-user: add support reconnect for vhost-user ports

[ZhiPeng Lu]

For vhost-user ports, Open vSwitch acts as the server and QEMU the client.
When OVS crashes or restarts, the QEMU process should be reconnected to
OVS.

Signed-off-by: ZhiPeng Lu 
Signed-off-by: Michal Privoznik 
---
v1->v2:
 - modify xml format
v2->v3:
 - fix commit message syntax
 - reimplemente functions and the struct about reconnect
v3->v4:
 - revert reimplemente functions and the struct about reconnect
---
 docs/formatdomain.html.in|   7 +-
 docs/schemas/domaincommon.rng|  26 ++--
 src/conf/domain_conf.c   | 158 +--
 tests/qemuxml2argvdata/net-vhostuser-multiq.args |  12 +-
 tests/qemuxml2argvdata/net-vhostuser-multiq.xml  |  11 +-
 5 files changed, 127 insertions(+), 87 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index d272cc1..f0f9f4b 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -5832,7 +5832,9 @@ qemu-kvm -net nic,model=? /dev/null
   /interface
   interface type='vhostuser'
 mac address='52:54:00:3b:83:1b'/
-source type='unix' path='/tmp/vhost2.sock' mode='client'/
+source type='unix' path='/tmp/vhost2.sock' mode='client'
+  reconnect enabled='yes' timeout='10'/
+/source 
 model type='virtio'/
 driver queues='5'/
   /interface
@@ -5848,6 +5850,9 @@ qemu-kvm -net nic,model=? /dev/null
   are supported.
   vhost-user requires the virtio model type, thus the
   model element is mandatory.
+  Since 3.10.0 the element has an optional
+  attribute reconnect which configures reconnect timeout
+  (in seconds) if the connection is lost.
 
 
 Traffic filtering with NWFilter
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index f22c932..9258c7d 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2399,6 +2399,18 @@
   
 
   
+  
+
+  
+
+  
+  
+
+  
+
+  
+
+  
 
   

Re: [libvirt] [PATCH] docs: formatdomain: Document the CPU feature 'name' attribute

[Kashyap Chamarthy]

On Tue, Jan 16, 2018 at 03:35:20PM -0200, Eduardo Habkost wrote:
> On Fri, Jan 12, 2018 at 08:31:16PM +0100, Kashyap Chamarthy wrote:
> > Currently, the CPU feature 'name' XML attribute, as in:

[...]

> > ---
> >  docs/formatdomain.html.in | 17 +
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index d272cc1ba..e717fb3aa 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -1454,6 +1454,23 @@
> >  
> >  Since 0.8.5 the policy
> >  attribute can be omitted and will default to require.
> > +
> > +Individual CPU feature names can be specified as part of the
> > +name attribute.
> 
> Isn't this "should" instead of "can"?  Does it make sense to have
> a 'feature' element without a 'name' attribute?

Good catch.  Near as I see, it doesn't.  So I'll: s/can/should.

> 
> >   The list of known CPU feature
> > +names (e.g. 'vmx', 'cmt', et cetera) can be found in the same
> > +file as CPU models -- cpu_map.xml. For example,
> > +to explicitly specify the 'pcid' feature with Intel IvyBridge
> > +CPU model:
> 
> Another paragraph above already says "The list of known feature
> names can be found in the same file as CPU models".   If you think the
> existing paragraph is not enough, I suggest rewriting it so the
> document won't repeat exactly the same thing.

True.  How about this rewrite: 

"Once you choose a feature (e.g. 'pcid') from the `cpu_map.xml`, to
specify it explicitly with the Intel IvyBridge CPU model [...]"

I'll consider whether to also add a note that before specifying extra
CPU feature flags, one should check if the named CPU models provided by
libvirt already include the said flags.

[...]

-- 
/kashyap

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