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

[Michael S. Tsirkin]

On Fri, Sep 08, 2017 at 10:48:10AM -0500, Brijesh Singh wrote:
> > >  > 11. GO verifies the measurement and if measurement matches then it 
> > > may
> > >  >  give a secret blob -- which must be injected into the guest before
> > >  >  libvirt starts the VM. If verification failed, GO will request 
> > > cloud
> > >  >  provider to destroy the VM.

I realised I'm missing something here: how does GO limit the
secret to the specific VM? For example, what prevents hypervisor
from launching two VMs with the same GO's DH, getting measurement
from 1st one but injecting the secret into the second one?

Thanks,

-- 
MST

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

Re: [libvirt] [PATCH] qemu: Move qemuFreeKeywords into qemu_parse_command.c

[John Ferlan]

On 10/16/2017 04:20 PM, Kothapally Madhu Pavan wrote:
> Move qemuFreeKeywords into qemu_parse_command.c as
> qemuKeywordsFree and call it rather than inline code
> in multiple places.

To be consistent, the new API would be qemuParseKeywordsFree - API's in
qemu_parse_command.c start with qemuParse

I've fixed that up as well as this description and pushed.

Thanks for doing this quickly.

Reviewed-by: John Ferlan 

(and pushed)

John

> 
> Signed-off-by: Kothapally Madhu Pavan 
> ---
>  src/qemu/qemu_monitor_json.c  | 15 ++---
>  src/qemu/qemu_parse_command.c | 52 
> ---
>  src/qemu/qemu_parse_command.h |  5 +
>  3 files changed, 26 insertions(+), 46 deletions(-)
> 

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

Re: [libvirt] [PATCH 3/3] util: storagefile: Track whether a virStorageSource was auto-detected

[John Ferlan]

On 10/12/2017 03:13 PM, Peter Krempa wrote:
> When formatting an inactive or migratable XML we will need to suppress
> backing chain members which were detected from the disk to keep
> semantics straight. This means we need to record, whether a
> virStorageSource originates from autodetection.
> ---
>  src/util/virstoragefile.c | 3 +++
>  src/util/virstoragefile.h | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index dd4494940..df6a547b0 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -2042,6 +2042,7 @@ virStorageSourceCopy(const virStorageSource *src,
>  ret->haveTLS = src->haveTLS;
>  ret->tlsFromConfig = src->tlsFromConfig;
>  ret->tlsVerify = src->tlsVerify;
> +ret->detected = src->detected;
> 
>  /* storage driver metadata are not copied */
>  ret->drv = NULL;
> @@ -3418,6 +3419,8 @@ virStorageSourceNewFromBacking(virStorageSourcePtr 
> parent)
>  goto error;
>  }
> 
> +ret->detected = true;
> +

Dang it... Looked at the wrong tab when checking build results, compiler
returns:

util/virstoragefile.c: In function 'virStorageSourceNewFromBacking':
util/virstoragefile.c:3440:19: error: potential null pointer dereference
[-Werror=null-dereference]
 ret->detected = true;

here for me.

That needs to be fixed...

John

>  return ret;
> 
>   error:
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index 74dee10f2..5181d6cb9 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -295,6 +295,8 @@ struct _virStorageSource {
>  char *tlsAlias;
>  char *tlsCertdir;
>  bool tlsVerify;
> +
> +bool detected; /* true if this entry was not provided by the user */
>  };
> 
> 

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

Re: [libvirt] [PATCH 0/3] misc fixes and improvements for backing chain and block devices (blockdev-add saga)

[John Ferlan]

On 10/12/2017 03:13 PM, Peter Krempa wrote:
> See individual patches please.
> 
> Peter Krempa (3):
>   qemu: command: Separate wrapping of disk backend props to 'file'
> object
>   qemu: block: Add support for file/block/dir storage to JSON disk src
> generator
>   util: storagefile: Track whether a virStorageSource was auto-detected
> 
>  src/qemu/qemu_block.c | 20 +---
>  src/qemu/qemu_command.c   | 27 ++-
>  src/util/virstoragefile.c |  3 +++
>  src/util/virstoragefile.h |  2 ++
>  4 files changed, 40 insertions(+), 12 deletions(-)
> 

Reviewed-by: John Ferlan 
(series)

John

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

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

[John Ferlan]

ping?

Thanks -

John

On 10/06/2017 04:59 PM, John Ferlan wrote:
> Patches should hopefully speak for themselves.
> 
> John Ferlan (4):
>   storage: Extract out the LVCREATE
>   storage: Introduce virStorageBackendCreateVolUsingQemuImg
>   storage: Allow creation of a LUKS using logical volume
>   docs: Add news article
> 
>  docs/news.xml | 13 ++
>  src/storage/storage_backend_logical.c | 74 
> +++
>  src/storage/storage_util.c| 42 
>  src/storage/storage_util.h|  8 
>  4 files changed, 104 insertions(+), 33 deletions(-)
> 

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

Re: [libvirt] [PATCH 0/5] Properly resize a local LUKS encrypted volume

[John Ferlan]

ping?

Thanks

John

On 10/06/2017 02:13 PM, John Ferlan wrote:
> The patches hopefully speak for themselves.
> 
> John Ferlan (5):
>   storage: Alter args to storageBackendResizeQemuImg
>   storage: Add error path for virStorageBackendCreateQemuImgCmdFromVol
>   storage: Alter storageBackendCreateQemuImgSecretObject args
>   storage: Properly resize a local volume using LUKS
>   docs: Add news article for bug fix
> 
>  docs/news.xml  |   8 +++
>  src/storage/storage_util.c | 133 
> -
>  2 files changed, 115 insertions(+), 26 deletions(-)
> 

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

Re: [libvirt] [PATCH 3/3] docs: Fix multiUser/replaceUser in RDP display doc.

[John Ferlan]

On 10/10/2017 05:52 PM, Dawid Zamirski wrote:
> The original description got it backwards.
> ---
>  docs/formatdomain.html.in | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

>From commit id '24e0171b' !!  and subsequently just carried on by
multiple people without regard to reading the wording ;-)

Reviewed-by: John Ferlan 

I'll push this patch shortly.

John

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

Re: [libvirt] [PATCH 2/3] vbox: Read runtime RDP port and handle autoport.

[John Ferlan]

On 10/10/2017 05:52 PM, Dawid Zamirski wrote:
> VirutalBox has a IVRDEServerInfo sturcture available that
> gives the effective runtime port that the VM is using when it's
> running. This is useful when the "TCP/Ports" VBox property was set to
> port range (e.g. via autoport = "yes" or via VBoxManage) in which
> case it would be impossible to get the "active" port otherwise.
> ---
>  src/vbox/vbox_common.c|   3 +-
>  src/vbox/vbox_tmpl.c  | 135 
> +-
>  src/vbox/vbox_uniformed_api.h |   2 +-
>  3 files changed, 97 insertions(+), 43 deletions(-)
> 

Is there more than one change going on here?

The removal of VBOX_SESSION_{OPEN|CLOSE} could seemingly be a separately
explained patch...



> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index 92ee37164..d542f2b76 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -3326,7 +3326,8 @@ vboxDumpDisplay(virDomainDefPtr def, vboxDriverPtr 
> data, IMachine *machine)
>  if (VIR_ALLOC(graphics) < 0)
>  goto cleanup;
>  
> -gVBoxAPI.UIVRDEServer.GetPorts(data, VRDEServer, graphics);
> +gVBoxAPI.UIVRDEServer.GetPorts(data, VRDEServer, machine, graphics);
> +gVBoxAPI.UISession.Close(data->vboxSession);
>  
>  graphics->type = VIR_DOMAIN_GRAPHICS_TYPE_RDP;
>  
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index 8e47d90d6..ff69cf39c 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c
> @@ -144,12 +144,6 @@ if (strUtf16) {\
>(unsigned)(iid)->m3[7]);\
>  }\
>  
> -#define VBOX_SESSION_OPEN(/* unused */ iid_value, /* in */ machine) \
> -machine->vtbl->LockMachine(machine, data->vboxSession, LockType_Write)
> -
> -#define VBOX_SESSION_CLOSE() \
> -data->vboxSession->vtbl->UnlockMachine(data->vboxSession)
> -
>  #define VBOX_IID_INITIALIZER { NULL, true }
>  
>  /* default RDP port range to use for auto-port setting */
> @@ -227,29 +221,6 @@ _vboxIIDFromArrayItem(vboxDriverPtr data, vboxIID *iid,
>  _vboxIIDFromArrayItem(data, iid, array, idx)
>  #define DEBUGIID(msg, strUtf16) DEBUGPRUnichar(msg, strUtf16)
>  
> -/**
> - * Converts Utf-16 string to int
> - */
> -static int PRUnicharToInt(PCVBOXXPCOM pFuncs, PRUnichar *strUtf16)
> -{
> -char *strUtf8 = NULL;
> -int ret = 0;
> -
> -if (!strUtf16)
> -return -1;
> -
> -pFuncs->pfnUtf16ToUtf8(strUtf16, );
> -if (!strUtf8)
> -return -1;
> -
> -if (virStrToLong_i(strUtf8, NULL, 10, ) < 0)
> -ret = -1;
> -
> -pFuncs->pfnUtf8Free(strUtf8);
> -
> -return ret;
> -}
> -
>  /**
>   * Converts int to Utf-16 string
>   */
> @@ -286,6 +257,56 @@ static virDomainState _vboxConvertState(PRUint32 state)
>  }
>  }
>  
> +static int
> +vboxGetActiveVRDEServerPort(ISession *session, IMachine *machine)
> +{
> +nsresult rc;
> +PRInt32 port = -1;
> +IVRDEServerInfo *vrdeInfo = NULL;
> +IConsole *console = NULL;
> +int locked = 0;
> +
> +rc = machine->vtbl->LockMachine(machine, session, LockType_Shared);
> +if (NS_FAILED(rc)) {
> +VIR_WARN("Could not obtain shared lock on VBox VM, rc=%08x", rc);
> +goto cleanup;

Why not just return -1 here since cleanup wouldn't need to clean up
@console or @vrdeInfo

That way @locked isn't necessary either.

> +} else {
> +locked = 1;
> +}
> +
> +rc = session->vtbl->GetConsole(session, );
> +if (NS_FAILED(rc)) {
> +VIR_WARN("Could not get VBox session console, rc=%08x", rc);
> +goto cleanup;
> +}
> +
> +/* it may be null if VM is not running */
> +if (!console)
> +goto cleanup;
> +
> +rc = console->vtbl->GetVRDEServerInfo(console, );
> +
> +if (NS_FAILED(rc) || !vrdeInfo) {
> +VIR_WARN("Could not get VBox VM VRDEServerInfo, rc=%08x", rc);
> +goto cleanup;
> +}
> +
> +rc = vrdeInfo->vtbl->GetPort(vrdeInfo, );
> +
> +if (NS_FAILED(rc)) {
> +VIR_WARN("Could not read port from VRDEServerInfo, rc=%08x", rc);
> +goto cleanup;
> +}
> +
> + cleanup:
> +VBOX_RELEASE(console);
> +VBOX_RELEASE(vrdeInfo);
> +if (locked)
> +session->vtbl->UnlockMachine(session);
> +
> +return port;
> +}
> +
>  static int
>  _vboxDomainSnapshotRestore(virDomainPtr dom,
>IMachine *machine,
> @@ -326,7 +347,7 @@ _vboxDomainSnapshotRestore(virDomainPtr dom,
>  goto cleanup;
>  }
>  
> -rc = VBOX_SESSION_OPEN(domiid.value, machine);
> +rc = machine->vtbl->LockMachine(machine, data->vboxSession, 
> LockType_Write);
>  #if VBOX_API_VERSION < 500
>  if (NS_SUCCEEDED(rc))
>  rc = data->vboxSession->vtbl->GetConsole(data->vboxSession, 
> );
> @@ -371,7 +392,7 @@ _vboxDomainSnapshotRestore(virDomainPtr dom,
>  #if VBOX_API_VERSION < 500
>  VBOX_RELEASE(console);
>  #endif /*VBOX_API_VERSION < 500*/
> -VBOX_SESSION_CLOSE();
> +

Re: [libvirt] [PATCH 1/3] vbox: Make autoport set RDP port range.

[John Ferlan]

On 10/10/2017 05:52 PM, Dawid Zamirski wrote:
> Originally autoport in vbox driver was setting the port to default
> value (3389) which caused mutiple VM instances use the same port.
> Since libvirt XML does not allow to set port ranges, this patch changes
> the "autoport" behavior to set VBox's "TCP/Ports" property to an
> arbitraty port range (3389-3689) to avoid that issue.

arbitrary

> ---
>  src/vbox/vbox_tmpl.c | 16 +---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index dffeabde0..8e47d90d6 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c
> @@ -152,6 +152,9 @@ if (strUtf16) {\
>  
>  #define VBOX_IID_INITIALIZER { NULL, true }
>  
> +/* default RDP port range to use for auto-port setting */
> +#define VBOX_RDP_AUTOPORT_RANGE "3389-3689"
> +
>  static void
>  _vboxIIDUnalloc(vboxDriverPtr data, vboxIID *iid)
>  {
> @@ -1601,20 +1604,27 @@ _vrdeServerGetPorts(vboxDriverPtr data 
> ATTRIBUTE_UNUSED,
>  }
>  
>  static nsresult
> -_vrdeServerSetPorts(vboxDriverPtr data ATTRIBUTE_UNUSED,
> -IVRDEServer *VRDEServer, virDomainGraphicsDefPtr 
> graphics)
> +_vrdeServerSetPorts(vboxDriverPtr data, IVRDEServer *VRDEServer,
> +virDomainGraphicsDefPtr graphics)
>  {
>  nsresult rc = 0;
>  PRUnichar *VRDEPortsKey = NULL;
>  PRUnichar *VRDEPortsValue = NULL;
>  
>  VBOX_UTF8_TO_UTF16("TCP/Ports", );
> -VRDEPortsValue = PRUnicharFromInt(data->pFuncs, graphics->data.rdp.port);
> +
> +if (graphics->data.rdp.port)

So one of my pet peeves in libvirt here and it's perhaps a latent or
day1 bug...

Looking through history - I'm not quite sure autoport ever quite worked
right because domain_conf would allow rdp.port == -1 in order to set
rdp.autoport = 1 (or true).

If rdp.port == -1, then this test passes which would set the "TCP/Ports"
property to -1. Now maybe that works, but I'm assuming it doesn't and an
error is thrown. Perhaps something you could test - modify the XML to
" and see what happens.

Since I went and dug a bit...  Looking at various commits in this space:

Commit to add version 4000 support: 8d2e24d6

Commit to add version 3001 support (< 3001, >= 3001) : 834d654

Original commit: 10d1650

It seems >= 3001 support totally ignored autoport setting


So my initial suggestion is alter the order of the checks.  That is:

   if (...rdp.autoport)
  set port range
   else
  set port to provided value

That way we won't have to worry about -1.

Also, please modify the formatdomain.html.in page in order to describe
that autoport will by default select a port in the range of 3389-3689.
Yes previously at some point in distant history 3389 was set to the
default because a 0 was allowed to be used to define that by the SetPort
API.

Secondary to that if you really feel a bit adventurous, you could add
attributes to the  element in order to define a port range
(min, max) in order to allow the autoport to select from outside your
default selection. Not required and hopefully 300 ports are enough, but
one never quite knows.

> +VRDEPortsValue = PRUnicharFromInt(data->pFuncs,
> +  graphics->data.rdp.port);
> +else if (graphics->data.rdp.autoport)
> +VBOX_UTF8_TO_UTF16(VBOX_RDP_AUTOPORT_RANGE, );
> +
>  rc = VRDEServer->vtbl->SetVRDEProperty(VRDEServer, VRDEPortsKey,
> VRDEPortsValue);
>  VBOX_UTF16_FREE(VRDEPortsKey);
>  VBOX_UTF16_FREE(VRDEPortsValue);
>  
> +

Spurious empty line

John
>  return rc;
>  }
>  
> 

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

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

[Dawid Zamirski]

On Tue, 2017-10-17 at 15:44 -0400, John Ferlan wrote:
> 
> On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
> > From: Dawid Zamirski 
> > 
> > When registering a VM we call OpenMedium on each disk image which
> > adds it
> > to vbox's global media registry. Therefore, we should make sure to
> > call
> > Close when unregistering VM so we cleanup the media registry
> > entries
> > after ourselves - this does not remove disk image files. This
> > follows
> > the behaviour of the VBoxManage unregistervm command.
> > ---
> >  src/vbox/vbox_tmpl.c | 24 +++-
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> > 
> 
> I'm not a vbox expert by any stretch, looking at this mostly because
> it's been sitting on list unreviewed...

Thank you :-)

> 
> > diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> > index dffeabde0..ac3b8fa00 100644
> > --- a/src/vbox/vbox_tmpl.c
> > +++ b/src/vbox/vbox_tmpl.c
> > @@ -378,6 +378,8 @@ _unregisterMachine(vboxDriverPtr data, vboxIID
> > *iid, IMachine **machine)
> >  {
> >  nsresult rc;
> >  vboxArray media = VBOX_ARRAY_INITIALIZER;
> > +size_t i;
> > +
> >  rc = data->vboxObj->vtbl->FindMachine(data->vboxObj, iid-
> > >value, machine);
> >  if (NS_FAILED(rc)) {
> >  virReportError(VIR_ERR_NO_DOMAIN, "%s",
> > @@ -385,12 +387,24 @@ _unregisterMachine(vboxDriverPtr data,
> > vboxIID *iid, IMachine **machine)
> >  return rc;
> >  }
> >  
> > -/* We're not interested in the array returned by the
> > Unregister method,
> > - * but in the side effect of unregistering the virtual
> > machine. In order
> > - * to call the Unregister method correctly we need to use the
> > vboxArray
> > - * wrapper here. */
> 
> I think you should keep the second sentence here - "In order to
> call..."
>  - IDC either way, but that would seem to still be important.
> 

Since this patch adds a code that loops over this array to close each
IMedium instance it's self-documenting so no there's need for code
comments here.

> >  rc = vboxArrayGetWithUintArg(, *machine, (*machine)-
> > >vtbl->Unregister,
> > - CleanupMode_DetachAllReturnNone);
> > + CleanupMode_DetachAllReturnHardDi
> > sksOnly);
> > +
> > +if (NS_FAILED(rc))
> > +goto cleanup;
> > +
> > +/* close each medium attached to VM to remove from media
> > registry */
> > +for (i = 0; i < media.count; i++) {
> > +IMedium *medium = media.items[i];
> > +
> > +if (!medium)
> > +continue;
> > +
> 
> So the vboxSnapshotRedefine and vboxDomainSnapshotDeleteMetadataOnly
> APIs that use CleanupMode_DetachAllReturnHardDisksOnly seem to go
> through some great pain to determine whether it's a "fake" disk or
> not -
> would this code need the same kind of logic?
> 
> /me wonders how much the existing code could be made more common -
> beyond the "isCurrent" in the latter function the code appears to be
> the
> same. If this code needs it, then it might be worth the effort to
> pass
> 'isCurrent' to some common helper and for the other caller, just pass
> true instead... Which would be similar if this code needed that.

I intentionally tried to stay away from touching the snapshot related
functions even though it seems that they could share some code. Since
I've been working on this on and off between projects at work, at
initial stages it seemed like a couple of lines of code "here and
there" would let me set/change vbox storage controller models via
libvirtxml. Unfortunately, as I dug deeper and tested all sorts of
combinations more bugs (even in master branch) seemed to pop out and
the series grew to be much bigger than I originally intended. However,
since I'll have to reorganize the commits for V2 anyway, I'll take a
look at those functions and see what I can do.

> 
> > +/* it's ok to ignore failure here - e.g. it may be used by
> > another VM */
> > +medium->vtbl->Close(medium);
> 
> You could place an ignore_value(); around this. Again see the two
> API's
> above they have a detailed explanation.
> 
> 
> John
> 
> > +}
> > +
> > + cleanup:
> >  vboxArrayUnalloc();
> >  return rc;
> >  }
> > 
> 
> --
> 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] qemu: Parse CPU stepping from query-cpu-model-expansion

[Jiri Denemark]

On Tue, Oct 17, 2017 at 16:23:49 -0400, John Ferlan wrote:
> 
> 
> On 10/10/2017 09:14 AM, Jiri Denemark wrote:
> > Even though only family and model are used for matching CPUID data with
> > CPU models from cpu_map.xml, stepping is used by x86DataFilterTSX which
> > is supposed to disable TSX on CPU models with broken TSX support.
> > 
> > This series applies on top of "Fix host-model if the chosen CPU model
> > has more features in QEMU compared to our cpu_map.xml" series I sent a
> > week ago.
> > 
> > Jiri Denemark (3):
> >   qemu: Parse CPU stepping from query-cpu-model-expansion
> >   cputest: Update Xeon-E7-8890 data
> >   cputest: Add query-cpu-definitions reply for Xeon-E7-8890
> > 
> >  src/cpu/cpu_x86.c  |  16 +-
> >  src/cpu/cpu_x86.h  |   3 +-
> >  src/qemu/qemu_capabilities.c   |   5 +-
> >  tests/cputest.c|   2 +-
> >  .../x86_64-cpuid-Xeon-E7-8890-disabled.xml |   6 +
> >  .../x86_64-cpuid-Xeon-E7-8890-enabled.xml  |   9 +
> >  .../cputestdata/x86_64-cpuid-Xeon-E7-8890-json.xml |  14 +
> >  tests/cputestdata/x86_64-cpuid-Xeon-E7-8890.json   | 521 
> > +
> >  tests/cputestdata/x86_64-cpuid-Xeon-E7-8890.xml|   6 +-
> >  9 files changed, 569 insertions(+), 13 deletions(-)
> >  create mode 100644 tests/cputestdata/x86_64-cpuid-Xeon-E7-8890-disabled.xml
> >  create mode 100644 tests/cputestdata/x86_64-cpuid-Xeon-E7-8890-enabled.xml
> >  create mode 100644 tests/cputestdata/x86_64-cpuid-Xeon-E7-8890-json.xml
> >  create mode 100644 tests/cputestdata/x86_64-cpuid-Xeon-E7-8890.json
> > 
> 
> Reviewed-by: John Ferlan 
> (series)

Pushed, thanks.

Jirka

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

Re: [libvirt] [PATCH 0/3] qemu: Parse CPU stepping from query-cpu-model-expansion

[John Ferlan]

On 10/10/2017 09:14 AM, Jiri Denemark wrote:
> Even though only family and model are used for matching CPUID data with
> CPU models from cpu_map.xml, stepping is used by x86DataFilterTSX which
> is supposed to disable TSX on CPU models with broken TSX support.
> 
> This series applies on top of "Fix host-model if the chosen CPU model
> has more features in QEMU compared to our cpu_map.xml" series I sent a
> week ago.
> 
> Jiri Denemark (3):
>   qemu: Parse CPU stepping from query-cpu-model-expansion
>   cputest: Update Xeon-E7-8890 data
>   cputest: Add query-cpu-definitions reply for Xeon-E7-8890
> 
>  src/cpu/cpu_x86.c  |  16 +-
>  src/cpu/cpu_x86.h  |   3 +-
>  src/qemu/qemu_capabilities.c   |   5 +-
>  tests/cputest.c|   2 +-
>  .../x86_64-cpuid-Xeon-E7-8890-disabled.xml |   6 +
>  .../x86_64-cpuid-Xeon-E7-8890-enabled.xml  |   9 +
>  .../cputestdata/x86_64-cpuid-Xeon-E7-8890-json.xml |  14 +
>  tests/cputestdata/x86_64-cpuid-Xeon-E7-8890.json   | 521 
> +
>  tests/cputestdata/x86_64-cpuid-Xeon-E7-8890.xml|   6 +-
>  9 files changed, 569 insertions(+), 13 deletions(-)
>  create mode 100644 tests/cputestdata/x86_64-cpuid-Xeon-E7-8890-disabled.xml
>  create mode 100644 tests/cputestdata/x86_64-cpuid-Xeon-E7-8890-enabled.xml
>  create mode 100644 tests/cputestdata/x86_64-cpuid-Xeon-E7-8890-json.xml
>  create mode 100644 tests/cputestdata/x86_64-cpuid-Xeon-E7-8890.json
> 

Reviewed-by: John Ferlan 
(series)

John

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

Re: [libvirt] [PATCH] conf: fix use of uninitialized variable

[John Ferlan]

On 10/17/2017 09:56 AM, Nikolay Shirokovskiy wrote:
> If same boot order is specified twice (or more) in domain xml
> we call free for uninitiaziled loadparm on cleanup in 
> virDomainDeviceBootParseXML
> and SIGABRT (or similar) as a result.
> ---
>  src/conf/domain_conf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: John Ferlan 

(and pushed) - Tks -

John

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

Re: [libvirt] [PATCH 3/6] docs: Add info about ide model attribute.

[John Ferlan]

On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
> From: Dawid Zamirski 
> 
> ---
>  docs/formatdomain.html.in | 3 +++
>  1 file changed, 3 insertions(+)
> 

This would be merged into patch 2...

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index c0e3c2221..ddcc9f1b1 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3596,6 +3596,9 @@
>   Since 1.3.5, USB controllers accept a
>   ports attribute to configure how many devices can be
>   connected to the controller.
> +ide
> +An ide controller has an optional attribute
> +model, which is one of "piix3", "piix4" or "ich6".

Once this is integrated, you'd need to add a Since
x.x.x tag in order to indicate which release the change went into.

John
>
>  
>  
> 

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

Re: [libvirt] [PATCH 6/6] vbox: Update XML dump of storage devices.

[John Ferlan]

On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
> From: Dawid Zamirski 
> 
> * added vboxDumpStorageControllers
> * replaced vboxDumpIDEHDDs with vboxDumpDisks which has been refectored
>   quite a bit to handle removable devices, the new SAS controller
>   support etc.
> * align the logic in vboxSnapshotGetReadWriteDisks and
>   vboxSnapshotGetReadOnlyDisks to more closely resemble vboxDumpDisks.
> * vboxGenerateMediumName was simplified to no longer "compute" the
>   device name value as deviePort/deviceSlot and their upper bound
>   values are no longer enough to do this accurately due to the added
>   SAS bus support which does not "map" directly into libvirt XML
>   semantics
> * vboxGetMaxPortSlotValues is now removed as it's no longer used
> ---
>  src/vbox/vbox_common.c | 691 
> -
>  1 file changed, 393 insertions(+), 298 deletions(-)
> 
Nny way to break this up into smaller more reviewably manageable chunks?

Certainly as I pointed out before, separating out the SAS into its own
patch could include whatever changes would happen here to support it.

It's too bad vboxDumpIDEHDDs couldn't be more easily split using the
current logic then updates applied to use the controller fetches.

I've kind of lost steam and think there's enough up to this point that
needs to be redone that I'll wait for the followup to look more in
depth. I noted a couple of things below.

> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index 7645b29a0..dd876645a 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -290,126 +290,44 @@ static int openSessionForMachine(vboxDriverPtr data, 
> const unsigned char *dom_uu
>  return 0;
>  }
>  
> -/**
> - * function to get the values for max port per
> - * instance and max slots per port for the devices
> - *
> - * @returns true on Success, false on failure.
> - * @param   vboxInput IVirtualBox pointer
> - * @param   maxPortPerInst  Output array of max port per instance
> - * @param   maxSlotPerPort  Output array of max slot per port
> - *
> - */
> -
> -static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
> - PRUint32 *maxPortPerInst,
> - PRUint32 *maxSlotPerPort)
> -{
> -ISystemProperties *sysProps = NULL;
> -
> -if (!vbox)
> -return false;
> -
> -gVBoxAPI.UIVirtualBox.GetSystemProperties(vbox, );
> -
> -if (!sysProps)
> -return false;
> -
> -gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
> - StorageBus_IDE,
> - 
> [StorageBus_IDE]);
> -gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
> - StorageBus_SATA,
> - 
> [StorageBus_SATA]);
> -gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
> - StorageBus_SCSI,
> - 
> [StorageBus_SCSI]);
> -gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
> - StorageBus_SAS,
> - 
> [StorageBus_SAS]);
> -gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
> - 
> StorageBus_Floppy,
> - 
> [StorageBus_Floppy]);
> -
> -gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
> -  
> StorageBus_IDE,
> -  
> [StorageBus_IDE]);
> -gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
> -  
> StorageBus_SATA,
> -  
> [StorageBus_SATA]);
> -gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
> -  
> StorageBus_SCSI,
> -  
> [StorageBus_SCSI]);
> -gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
> -  
> StorageBus_SAS,
> -  
> [StorageBus_SAS]);
> -gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
> -  
> StorageBus_Floppy,
> -

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

[John Ferlan]

On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
> From: Dawid Zamirski 
> 
> Until now the vbox driver was completely ignoring  element

ignoring the 

> for storage in the XML definition. This patch adds support for
> interpretting  element for storage devices. With this the

interpreting the 

> following other changes were made to the whole storage attachment code:
> 
> * vboxAttachDrives no longer "computes" deviceSlot and devicePort
>   values based on the disk device name. This was causing the driver to
>   ignore the values set by  element of the  device. If
>   that element is omitted in XML, the values produced by default by
>   virDomainDiskDefAssign address should work well.
> * if any part of storage attachment code fails, i.e wherever we call
>   virReportError, we also fail the DefineXML caller rather than ignoring
>   those issues and moving on.

This particular code (checking vboxAttachDrives status) probably could
have been pulled out and made into it's own patch.

> * the DefineXML cleanup part of the code was changed to make sure that
>   any critical failure in device attachment code does not leave any
>   partially defined VM behind.
> * do not require disk source for removable drives so that "empty"
>   drives can be created for cdrom and floppy types.


What's "StorageBus_SAS" and why was it not mentioned?  Seems to be
something that should be separated into a patch just before this one in
order to reduce the number of changes in this patch.

> ---
>  src/vbox/vbox_common.c | 535 
> ++---
>  src/vbox/vbox_common.h |   8 +
>  src/vbox/vbox_tmpl.c   |  43 ++--
>  3 files changed, 310 insertions(+), 276 deletions(-)
> 
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index 92ee37164..7645b29a0 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -324,6 +324,9 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
>  gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
>   StorageBus_SCSI,
>   
> [StorageBus_SCSI]);
> +gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
> + StorageBus_SAS,
> + 
> [StorageBus_SAS]);
>  gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
>   
> StorageBus_Floppy,
>   
> [StorageBus_Floppy]);
> @@ -337,6 +340,9 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
>  gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
>
> StorageBus_SCSI,
>
> [StorageBus_SCSI]);
> +gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
> +  
> StorageBus_SAS,
> +  
> [StorageBus_SAS]);
>  gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
>
> StorageBus_Floppy,
>
> [StorageBus_Floppy]);
> @@ -346,68 +352,6 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
>  return true;
>  }
>  
> -/**
> - * function to get the StorageBus, Port number
> - * and Device number for the given devicename
> - * e.g: hda has StorageBus = IDE, port = 0,
> - *  device = 0
> - *
> - * @returns true on Success, false on failure.
> - * @param   deviceName  Input device name
> - * @param   aMaxPortPerInst Input array of max port per device instance
> - * @param   aMaxSlotPerPort Input array of max slot per device port
> - * @param   storageBus  Input storage bus type
> - * @param   deviceInst  Output device instance number
> - * @param   devicePort  Output port number
> - * @param   deviceSlot  Output slot number
> - *
> - */
> -static bool vboxGetDeviceDetails(const char *deviceName,
> - PRUint32 *aMaxPortPerInst,
> - PRUint32 *aMaxSlotPerPort,
> - PRUint32 storageBus,
> - PRInt32 *deviceInst,
> - PRInt32 *devicePort,
> - PRInt32 *deviceSlot)
> -{
> -int total = 0;
> -PRUint32 maxPortPerInst = 0;
> -PRUint32 maxSlotPerPort = 0;
> -
> -if (!deviceName ||
> -!deviceInst ||
> -!devicePort ||
> -!deviceSlot ||
> -

Re: [libvirt] [PATCH 4/6] vbox: Add more IStorageController API mappings.

[John Ferlan]

On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
> From: Dawid Zamirski 
> 
> This patch 'maps' additonal methods for VBOX API to the libvirt unified

"for the VBOX API"

additional

> vbox API to deal with IStorageController. The 'mapped' methods are:
> 
> * IStorageController->GetStorageControllerType()
> * IStorageController->SetStorageControllerType()
> * IMachine->GetStorageControllers()
> ---
>  src/vbox/vbox_common.h| 13 +
>  src/vbox/vbox_tmpl.c  | 20 
>  src/vbox/vbox_uniformed_api.h |  3 +++
>  3 files changed, 36 insertions(+)
> 

This seems reasonable Even though there's some style differences
with how we generally like to see libvirt code - it follows the style of
other vbox code and to me that perhaps more important.

Reviewed-by: John Ferlan 

John

> diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h
> index c6da8929d..b08ad1e3e 100644
> --- a/src/vbox/vbox_common.h
> +++ b/src/vbox/vbox_common.h
> @@ -235,6 +235,19 @@ enum StorageBus
>  StorageBus_SAS = 5
>  };
>  
> +enum StorageControllerType
> +{
> +StorageControllerType_Null = 0,
> +StorageControllerType_LsiLogic = 1,
> +StorageControllerType_BusLogic = 2,
> +StorageControllerType_IntelAhci = 3,
> +StorageControllerType_PIIX3 = 4,
> +StorageControllerType_PIIX4 = 5,
> +StorageControllerType_ICH6 = 6,
> +StorageControllerType_I82078 = 7,
> +StorageControllerType_LsiLogicSas = 8
> +};
> +
>  enum AccessMode
>  {
>  AccessMode_ReadOnly = 1,
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index ac3b8fa00..6592cbd63 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c
> @@ -550,6 +550,11 @@ static void* _handleUSBGetDeviceFilters(IUSBCommon 
> *USBCommon)
>  return USBCommon->vtbl->GetDeviceFilters;
>  }
>  
> +static void* _handleMachineGetStorageControllers(IMachine *machine)
> +{
> +return machine->vtbl->GetStorageControllers;
> +}
> +
>  static void* _handleMachineGetMediumAttachments(IMachine *machine)
>  {
>  return machine->vtbl->GetMediumAttachments;
> @@ -1919,6 +1924,18 @@ _storageControllerGetBus(IStorageController 
> *storageController, PRUint32 *bus)
>  return storageController->vtbl->GetBus(storageController, bus);
>  }
>  
> +static nsresult
> +_storageControllerGetControllerType(IStorageController *storageController, 
> PRUint32 *controllerType)
> +{
> +return storageController->vtbl->GetControllerType(storageController, 
> controllerType);
> +}
> +
> +static nsresult
> +_storageControllerSetControllerType(IStorageController *storageController, 
> PRUint32 controllerType)
> +{
> +return storageController->vtbl->SetControllerType(storageController, 
> controllerType);
> +}
> +
>  static nsresult
>  _sharedFolderGetHostPath(ISharedFolder *sharedFolder, PRUnichar **hostPath)
>  {
> @@ -2268,6 +2285,7 @@ static vboxUniformedArray _UArray = {
>  .handleGetMachines = _handleGetMachines,
>  .handleGetHardDisks = _handleGetHardDisks,
>  .handleUSBGetDeviceFilters = _handleUSBGetDeviceFilters,
> +.handleMachineGetStorageControllers = 
> _handleMachineGetStorageControllers,
>  .handleMachineGetMediumAttachments = _handleMachineGetMediumAttachments,
>  .handleMachineGetSharedFolders = _handleMachineGetSharedFolders,
>  .handleSnapshotGetChildren = _handleSnapshotGetChildren,
> @@ -2499,6 +2517,8 @@ static vboxUniformedIMediumAttachment 
> _UIMediumAttachment = {
>  
>  static vboxUniformedIStorageController _UIStorageController = {
>  .GetBus = _storageControllerGetBus,
> +.GetControllerType = _storageControllerGetControllerType,
> +.SetControllerType = _storageControllerSetControllerType,
>  };
>  
>  static vboxUniformedISharedFolder _UISharedFolder = {
> diff --git a/src/vbox/vbox_uniformed_api.h b/src/vbox/vbox_uniformed_api.h
> index 2ccaf43e8..dc0b391b2 100644
> --- a/src/vbox/vbox_uniformed_api.h
> +++ b/src/vbox/vbox_uniformed_api.h
> @@ -135,6 +135,7 @@ typedef struct {
>  void* (*handleGetMachines)(IVirtualBox *vboxObj);
>  void* (*handleGetHardDisks)(IVirtualBox *vboxObj);
>  void* (*handleUSBGetDeviceFilters)(IUSBCommon *USBCommon);
> +void* (*handleMachineGetStorageControllers)(IMachine *machine);
>  void* (*handleMachineGetMediumAttachments)(IMachine *machine);
>  void* (*handleMachineGetSharedFolders)(IMachine *machine);
>  void* (*handleSnapshotGetChildren)(ISnapshot *snapshot);
> @@ -410,6 +411,8 @@ typedef struct {
>  /* Functions for IStorageController */
>  typedef struct {
>  nsresult (*GetBus)(IStorageController *storageController, PRUint32 *bus);
> +nsresult (*SetControllerType)(IStorageController *storageController, 
> PRUint32 controllerType);
> +nsresult (*GetControllerType)(IStorageController *storageController, 
> PRUint32 *controllerType);
>  } vboxUniformedIStorageController;
>  
>  /* Functions for ISharedFolder */
> 

--
libvir-list 

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

[John Ferlan]

On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
> From: Dawid Zamirski 
> 
> The optional values are 'piix3', 'piix4' or 'ich6'. Those will be
> needed to allow setting IDE controller model in VirtualBox driver.
> ---
>  docs/schemas/domaincommon.rng | 18 --
>  src/conf/domain_conf.c|  9 +
>  src/conf/domain_conf.h|  9 +
>  src/libvirt_private.syms  |  2 ++
>  4 files changed, 36 insertions(+), 2 deletions(-)
> 

Patches 2 & 3 should be combined and you'll need to provide an xml2xml
example here, modifying tests/qemuxml2xmltest.c in order to add your
test. Search for controller type='scsi' and model= being set in any of
the tests/*/*.xml files.  Then generate your test that would have
controller type='ide' ... model='xxx' (just one example is fine).

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

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

> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 4dbda6932..c3f1557f0 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1927,12 +1927,11 @@
>
>  
>  
> -  
> +  
>
>  
>
>  fdc
> -ide
>  sata
>  ccid
>
> @@ -1993,6 +1992,21 @@
>
>  
>
> +  
> +  
> +
> +  ide
> +
> +
> +  
> +
> +  piix3
> +  piix4
> +  ich6
> +
> +  
> +
> +  
>
>
>  
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 54be9028d..493bf83ff 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -378,6 +378,11 @@ VIR_ENUM_IMPL(virDomainControllerModelUSB, 
> VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST,
>"qemu-xhci",
>"none")
>  
> +VIR_ENUM_IMPL(virDomainControllerModelIDE, 
> VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST,
> +  "piix3",
> +  "piix4",
> +  "ich6")
> +
>  VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST,
>"mount",
>"block",
> @@ -9467,6 +9472,8 @@ virDomainControllerModelTypeFromString(const 
> virDomainControllerDef *def,
>  return virDomainControllerModelUSBTypeFromString(model);
>  else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
>  return virDomainControllerModelPCITypeFromString(model);
> +else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> +return virDomainControllerModelIDETypeFromString(model);
>  
>  return -1;
>  }
> @@ -9482,6 +9489,8 @@ 
> virDomainControllerModelTypeToString(virDomainControllerDefPtr def,
>  return virDomainControllerModelUSBTypeToString(model);
>  else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
>  return virDomainControllerModelPCITypeToString(model);
> +else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> +return virDomainControllerModelIDETypeToString(model);
>  
>  return NULL;
>  }
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index a42efcfa6..d7f4c3f1e 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -748,6 +748,14 @@ typedef enum {
>  VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST
>  } virDomainControllerModelUSB;
>  
> +typedef enum {
> +VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3,
> +VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4,
> +VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6,
> +
> +VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST
> +} virDomainControllerModelIDE;
> +
>  # define IS_USB2_CONTROLLER(ctrl) \
>  (((ctrl)->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) && \
>   ((ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1 || \
> @@ -3231,6 +3239,7 @@ VIR_ENUM_DECL(virDomainControllerModelPCI)
>  VIR_ENUM_DECL(virDomainControllerPCIModelName)
>  VIR_ENUM_DECL(virDomainControllerModelSCSI)
>  VIR_ENUM_DECL(virDomainControllerModelUSB)
> +VIR_ENUM_DECL(virDomainControllerModelIDE)
>  VIR_ENUM_DECL(virDomainFS)
>  VIR_ENUM_DECL(virDomainFSDriver)
>  VIR_ENUM_DECL(virDomainFSAccessMode)
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 9243c5591..616b14f82 100644
> --- 

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

[John Ferlan]

On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
> From: Dawid Zamirski 
> 
> When registering a VM we call OpenMedium on each disk image which adds it
> to vbox's global media registry. Therefore, we should make sure to call
> Close when unregistering VM so we cleanup the media registry entries
> after ourselves - this does not remove disk image files. This follows
> the behaviour of the VBoxManage unregistervm command.
> ---
>  src/vbox/vbox_tmpl.c | 24 +++-
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 

I'm not a vbox expert by any stretch, looking at this mostly because
it's been sitting on list unreviewed...

> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index dffeabde0..ac3b8fa00 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c
> @@ -378,6 +378,8 @@ _unregisterMachine(vboxDriverPtr data, vboxIID *iid, 
> IMachine **machine)
>  {
>  nsresult rc;
>  vboxArray media = VBOX_ARRAY_INITIALIZER;
> +size_t i;
> +
>  rc = data->vboxObj->vtbl->FindMachine(data->vboxObj, iid->value, 
> machine);
>  if (NS_FAILED(rc)) {
>  virReportError(VIR_ERR_NO_DOMAIN, "%s",
> @@ -385,12 +387,24 @@ _unregisterMachine(vboxDriverPtr data, vboxIID *iid, 
> IMachine **machine)
>  return rc;
>  }
>  
> -/* We're not interested in the array returned by the Unregister method,
> - * but in the side effect of unregistering the virtual machine. In order
> - * to call the Unregister method correctly we need to use the vboxArray
> - * wrapper here. */

I think you should keep the second sentence here - "In order to call..."
 - IDC either way, but that would seem to still be important.

>  rc = vboxArrayGetWithUintArg(, *machine, 
> (*machine)->vtbl->Unregister,
> - CleanupMode_DetachAllReturnNone);
> + CleanupMode_DetachAllReturnHardDisksOnly);
> +
> +if (NS_FAILED(rc))
> +goto cleanup;
> +
> +/* close each medium attached to VM to remove from media registry */
> +for (i = 0; i < media.count; i++) {
> +IMedium *medium = media.items[i];
> +
> +if (!medium)
> +continue;
> +

So the vboxSnapshotRedefine and vboxDomainSnapshotDeleteMetadataOnly
APIs that use CleanupMode_DetachAllReturnHardDisksOnly seem to go
through some great pain to determine whether it's a "fake" disk or not -
would this code need the same kind of logic?

/me wonders how much the existing code could be made more common -
beyond the "isCurrent" in the latter function the code appears to be the
same. If this code needs it, then it might be worth the effort to pass
'isCurrent' to some common helper and for the other caller, just pass
true instead... Which would be similar if this code needed that.

> +/* it's ok to ignore failure here - e.g. it may be used by another 
> VM */
> +medium->vtbl->Close(medium);

You could place an ignore_value(); around this. Again see the two API's
above they have a detailed explanation.


John

> +}
> +
> + cleanup:
>  vboxArrayUnalloc();
>  return rc;
>  }
> 

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

Re: [libvirt] [PATCH 0/2] vz: build fixes

[Maxim Nestratov]

17-Oct-17 20:40, Jiri Denemark пишет:


On Tue, Oct 17, 2017 at 16:55:13 +0300, Nikolay Shirokovskiy wrote:

Nikolay Shirokovskiy (2):
   vz: missing pieces for fd885a06 for vz driver
   vz: fix typo for 0d3d020b

  src/vz/vz_driver.c | 4 ++--
  src/vz/vz_sdk.c| 2 +-
  2 files changed, 3 insertions(+), 3 deletions(-)

ACK series

Jirka

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


Thank you. Pushed both.

Maxim

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

Re: [libvirt] [PATCH 0/2] vz: build fixes

[Jiri Denemark]

On Tue, Oct 17, 2017 at 16:55:13 +0300, Nikolay Shirokovskiy wrote:
> Nikolay Shirokovskiy (2):
>   vz: missing pieces for fd885a06 for vz driver
>   vz: fix typo for 0d3d020b
> 
>  src/vz/vz_driver.c | 4 ++--
>  src/vz/vz_sdk.c| 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)

ACK series

Jirka

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

Re: [libvirt] [libvirt-jenkins-ci PATCH 1/5] ansible: Remove bootstrap phase

[Andrea Bolognani]

On Tue, 2017-10-17 at 18:16 +0200, Pavel Hrdina wrote:
> > Could we just generate a random root password, but install SSH public
> > keys and set SSH to only permit public key auth. 
> 
> That's the idea, having the root password stored on the host is just
> if something goes wrong and you need to use serial console.

Yeah, I like it. I'll figure out something.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [libvirt-jenkins-ci PATCH 3/5] ansible: Add unattended installation support

[Andrea Bolognani]

On Tue, 2017-10-17 at 17:32 +0100, Daniel P. Berrange wrote:
> Yeah, I think it is reasonable to have Ubuntu support. Travis is somewhat
> performance limited

Not to mention the embarassingly outdated OS offering. But hey,
it's worth every penny we spend on it! :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [libvirt-jenkins-ci PATCH 3/5] ansible: Add unattended installation support

[Daniel P. Berrange]

On Tue, Oct 17, 2017 at 06:24:47PM +0200, Andrea Bolognani wrote:
> On Tue, 2017-10-17 at 17:57 +0200, Pavel Hrdina wrote:
> > I wouldn't include Ubuntu related things.  Yes, we use travis where they
> > have Ubuntu nodes, but this is jenkins-ci repository where we don't use
> > Ubuntu at all.
> 
> While a sane CI setup is definitely the primary reason why this work
> is happening, a secondary goal is making it possible for (potential)
> developers to ensure portability and debug build issues on platforms
> that they don't already have easy access to. The overlap between the
> two goals is basically 99% anyway.
> 
> Moreover, if some build job fails on Travis, it would be nice to
> quickly reproduce the failure locally[1] instead of doing multiple
> round-trips to Travis. So I vote for keeping the Ubuntu bits,
> especially considering that the overhead is literally 9 lines :)

Yeah, I think it is reasonable to have Ubuntu support. Travis is somewhat
performance limited, so if we ever get more CI hardware of our own I would
expect us to move Ubuntu jobs off Travis. The only key unique thing about
Travis is ability to build on OS-X - we can't easily replicate that ourselves
due to OS-X restrictive licensing.


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] [libvirt-jenkins-ci PATCH 3/5] ansible: Add unattended installation support

[Andrea Bolognani]

On Tue, 2017-10-17 at 17:57 +0200, Pavel Hrdina wrote:
> I wouldn't include Ubuntu related things.  Yes, we use travis where they
> have Ubuntu nodes, but this is jenkins-ci repository where we don't use
> Ubuntu at all.

While a sane CI setup is definitely the primary reason why this work
is happening, a secondary goal is making it possible for (potential)
developers to ensure portability and debug build issues on platforms
that they don't already have easy access to. The overlap between the
two goals is basically 99% anyway.

Moreover, if some build job fails on Travis, it would be nice to
quickly reproduce the failure locally[1] instead of doing multiple
round-trips to Travis. So I vote for keeping the Ubuntu bits,
especially considering that the overhead is literally 9 lines :)

> > +install_disk_size: 10
> 
> Currently we have 15 GiB per guest and in some cases we are able to run
> out of space.  Let's use 15 GiB.

Sure.

> > +case "$INSTALL_CONFIG" in
> > +*kickstart*|*ks*) EXTRA_ARGS="ks=file:/${INSTALL_CONFIG##*/}" ;;
> > +esac
> 
> I would add "console=ttyS0" into EXTRA_ARGS to get serial console
> working.

I prefer serial console to graphical console too, but for some reason
I thought that would not be appropriate for the CI setup... Guess I
should just have asked ;)

> > +--ram "$INSTALL_MEMORY_SIZE" \
> 
> Don't use --ram, that is deprecated, --memory should be used instead.

TIL! Consider it done.

> > +--disk 
> > "size=$INSTALL_DISK_SIZE,pool=$INSTALL_STORAGE_POOL,bus=virtio" \
> > +--network "network=$INSTALL_NETWORK,model=virtio" \
> > +--initrd-inject "$INSTALL_CONFIG" \
> > +--extra-args "$EXTRA_ARGS"
> 
> and we might add:
> 
> --cpu host-passthrough  // we will not migrate the guest
> --graphics none // we use only ssh to the CI host
> --serial pty// if something is really wrong with the guest
> --autostart

These all look sensible, I'll add them.


[1] I almost had a success story I could use to prove my point today:
Martin ran into a Travis build failure while testing out some new
stuff, and thanks to the Ubuntu configs we were able to bring up
a fully-configured Ubuntu 14.04 guest in around 10 minutes.
Unfortunately the failure ended up not reproducing in the guest,
which is quite annoying and something that requires more
investigation, but the point about the usefulness of having this
ability still stands IMHO.
-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [libvirt-jenkins-ci PATCH 3/5] ansible: Add unattended installation support

[Daniel P. Berrange]

On Tue, Oct 17, 2017 at 06:14:35PM +0200, Pavel Hrdina wrote:
> On Tue, Oct 17, 2017 at 05:04:57PM +0100, Daniel P. Berrange wrote:
> > On Tue, Oct 17, 2017 at 05:57:30PM +0200, Pavel Hrdina wrote:
> > > On Mon, Oct 16, 2017 at 06:02:06PM +0200, Andrea Bolognani wrote:
> > > > The 'manage' tool can now be used to install most known guests
> > > > without requiring user interaction.
> > > > 
> > > > Signed-off-by: Andrea Bolognani 
> > > > ---
> > > >  ansible/group_vars/all/install.yml | 10 +++
> > > >  ansible/host_vars/libvirt-centos-6/install.yml |  3 +
> > > >  ansible/host_vars/libvirt-centos-7/install.yml |  3 +
> > > >  ansible/host_vars/libvirt-debian-8/install.yml |  3 +
> > > >  ansible/host_vars/libvirt-debian-9/install.yml |  3 +
> > > >  ansible/host_vars/libvirt-fedora-25/install.yml|  3 +
> > > >  ansible/host_vars/libvirt-fedora-26/install.yml|  3 +
> > > >  .../host_vars/libvirt-fedora-rawhide/install.yml   |  3 +
> > > >  ansible/host_vars/libvirt-ubuntu-12/install.yml|  3 +
> > > >  ansible/host_vars/libvirt-ubuntu-14/install.yml|  3 +
> > > >  ansible/host_vars/libvirt-ubuntu-16/install.yml|  3 +
> > > >  ansible/kickstart.cfg  | 60 +++
> > > >  ansible/manage | 74 
> > > > +++
> > > >  ansible/preseed.cfg| 85 
> > > > ++
> > > >  14 files changed, 259 insertions(+)
> > > >  create mode 100644 ansible/group_vars/all/install.yml
> > > >  create mode 100644 ansible/host_vars/libvirt-centos-6/install.yml
> > > >  create mode 100644 ansible/host_vars/libvirt-centos-7/install.yml
> > > >  create mode 100644 ansible/host_vars/libvirt-debian-8/install.yml
> > > >  create mode 100644 ansible/host_vars/libvirt-debian-9/install.yml
> > > >  create mode 100644 ansible/host_vars/libvirt-fedora-25/install.yml
> > > >  create mode 100644 ansible/host_vars/libvirt-fedora-26/install.yml
> > > >  create mode 100644 ansible/host_vars/libvirt-fedora-rawhide/install.yml
> > > 
> > > I wouldn't include Ubuntu related things.  Yes, we use travis where they
> > > have Ubuntu nodes, but this is jenkins-ci repository where we don't use
> > > Ubuntu at all.
> > > 
> > > >  create mode 100644 ansible/host_vars/libvirt-ubuntu-12/install.yml
> > > >  create mode 100644 ansible/host_vars/libvirt-ubuntu-14/install.yml
> > > >  create mode 100644 ansible/host_vars/libvirt-ubuntu-16/install.yml
> > > >  create mode 100644 ansible/kickstart.cfg
> > > >  create mode 100644 ansible/preseed.cfg
> > > > 
> > > > diff --git a/ansible/group_vars/all/install.yml 
> > > > b/ansible/group_vars/all/install.yml
> > > > new file mode 100644
> > > > index 000..714328e
> > > > --- /dev/null
> > > > +++ b/ansible/group_vars/all/install.yml
> > > > @@ -0,0 +1,10 @@
> > > > +---
> > > > +# Sizes are in GiB
> > > > +install_virt_type: kvm
> > > > +install_arch: x86_64
> > > > +install_machine: pc
> > > > +install_vcpus: 2
> > > > +install_memory_size: 2
> > > > +install_disk_size: 10
> > > 
> > > Currently we have 15 GiB per guest and in some cases we are able to run
> > > out of space.  Let's use 15 GiB.
> > 
> > We used to run out of space periodically because our RPM build jobs were
> > creating RPMs in $HOME/rpmbuild which jenkins never purged. I reconfigured
> > Jenkins to use the GIT workspace as the RPM build dir, so jenkins always
> > purges RPMs. Since then we've not run out of space again AFAIK. None the
> > less, I agree that 15 GiB is probably the min we need
> 
> Mainly because some of the guests have only 10 GiB disk.
> 
> > > > diff --git a/ansible/kickstart.cfg b/ansible/kickstart.cfg
> > > > new file mode 100644
> > > > index 000..c28f275
> > > > --- /dev/null
> > > > +++ b/ansible/kickstart.cfg
> > > > @@ -0,0 +1,60 @@
> > > > +# Installer configuration
> > > > +#
> > > > +# Perform a text based installation followed by a reboot, and disable
> > > > +# the first boot assistant
> > > > +text
> > > > +install
> > > > +reboot
> > > > +firstboot --disable
> > > > +
> > > > +
> > > > +# Environment configuration
> > > > +#
> > > > +# Locale, keyboard and timezone. All these will be configured again
> > > > +# later with Ansible, but they're required information so we must
> > > > +# provide them
> > > > +lang en_US.UTF-8
> > > > +keyboard us
> > > > +timezone --utc Europe/Prague
> > 
> > How about UTC as a neutral option for timezone ;-)
> 
> +1
> 
> > 
> > 
> > > > +
> > > > +
> > > > +# User creation
> > > > +#
> > > > +# We don't create any user except for root. We can use a very insecure
> > > > +# root password because the guest will not be exposed to the Internet:
> > > > +# it will only be accessible from the host itself
> > > > +authconfig --enableshadow --passalgo=sha512
> > > > +rootpw --plaintext root
> > > > +
> > > > +
> > > > +# Partition disk
> > > > +#
> > > > +# Erase everything and set 

Re: [libvirt] [libvirt-jenkins-ci PATCH 1/5] ansible: Remove bootstrap phase

[Pavel Hrdina]

On Tue, Oct 17, 2017 at 05:11:40PM +0100, Daniel P. Berrange wrote:
> On Tue, Oct 17, 2017 at 06:05:23PM +0200, Pavel Hrdina wrote:
> > On Mon, Oct 16, 2017 at 06:02:04PM +0200, Andrea Bolognani wrote:
> > > Having to bootstrap the guest as a separate phase is annoying and
> > > can be avoided by assuming the root password is well-known.
> > 
> > I'm not sure about this.  Yes the password will be well known for us
> > but I would rather have it generated and stored somewhere on the host.
> > 
> > The guests are hidden from internet but they are still connected to
> > jenkins and are executing commands provided by jenkins.  Maybe I'm
> > just too paranoid :).
> 
> Could we just generate a random root password, but install SSH public
> keys and set SSH to only permit public key auth. 

That's the idea, having the root password stored on the host is just
if something goes wrong and you need to use serial console.

Pavel


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

Re: [libvirt] [libvirt-jenkins-ci PATCH 3/5] ansible: Add unattended installation support

[Pavel Hrdina]

On Tue, Oct 17, 2017 at 05:04:57PM +0100, Daniel P. Berrange wrote:
> On Tue, Oct 17, 2017 at 05:57:30PM +0200, Pavel Hrdina wrote:
> > On Mon, Oct 16, 2017 at 06:02:06PM +0200, Andrea Bolognani wrote:
> > > The 'manage' tool can now be used to install most known guests
> > > without requiring user interaction.
> > > 
> > > Signed-off-by: Andrea Bolognani 
> > > ---
> > >  ansible/group_vars/all/install.yml | 10 +++
> > >  ansible/host_vars/libvirt-centos-6/install.yml |  3 +
> > >  ansible/host_vars/libvirt-centos-7/install.yml |  3 +
> > >  ansible/host_vars/libvirt-debian-8/install.yml |  3 +
> > >  ansible/host_vars/libvirt-debian-9/install.yml |  3 +
> > >  ansible/host_vars/libvirt-fedora-25/install.yml|  3 +
> > >  ansible/host_vars/libvirt-fedora-26/install.yml|  3 +
> > >  .../host_vars/libvirt-fedora-rawhide/install.yml   |  3 +
> > >  ansible/host_vars/libvirt-ubuntu-12/install.yml|  3 +
> > >  ansible/host_vars/libvirt-ubuntu-14/install.yml|  3 +
> > >  ansible/host_vars/libvirt-ubuntu-16/install.yml|  3 +
> > >  ansible/kickstart.cfg  | 60 +++
> > >  ansible/manage | 74 
> > > +++
> > >  ansible/preseed.cfg| 85 
> > > ++
> > >  14 files changed, 259 insertions(+)
> > >  create mode 100644 ansible/group_vars/all/install.yml
> > >  create mode 100644 ansible/host_vars/libvirt-centos-6/install.yml
> > >  create mode 100644 ansible/host_vars/libvirt-centos-7/install.yml
> > >  create mode 100644 ansible/host_vars/libvirt-debian-8/install.yml
> > >  create mode 100644 ansible/host_vars/libvirt-debian-9/install.yml
> > >  create mode 100644 ansible/host_vars/libvirt-fedora-25/install.yml
> > >  create mode 100644 ansible/host_vars/libvirt-fedora-26/install.yml
> > >  create mode 100644 ansible/host_vars/libvirt-fedora-rawhide/install.yml
> > 
> > I wouldn't include Ubuntu related things.  Yes, we use travis where they
> > have Ubuntu nodes, but this is jenkins-ci repository where we don't use
> > Ubuntu at all.
> > 
> > >  create mode 100644 ansible/host_vars/libvirt-ubuntu-12/install.yml
> > >  create mode 100644 ansible/host_vars/libvirt-ubuntu-14/install.yml
> > >  create mode 100644 ansible/host_vars/libvirt-ubuntu-16/install.yml
> > >  create mode 100644 ansible/kickstart.cfg
> > >  create mode 100644 ansible/preseed.cfg
> > > 
> > > diff --git a/ansible/group_vars/all/install.yml 
> > > b/ansible/group_vars/all/install.yml
> > > new file mode 100644
> > > index 000..714328e
> > > --- /dev/null
> > > +++ b/ansible/group_vars/all/install.yml
> > > @@ -0,0 +1,10 @@
> > > +---
> > > +# Sizes are in GiB
> > > +install_virt_type: kvm
> > > +install_arch: x86_64
> > > +install_machine: pc
> > > +install_vcpus: 2
> > > +install_memory_size: 2
> > > +install_disk_size: 10
> > 
> > Currently we have 15 GiB per guest and in some cases we are able to run
> > out of space.  Let's use 15 GiB.
> 
> We used to run out of space periodically because our RPM build jobs were
> creating RPMs in $HOME/rpmbuild which jenkins never purged. I reconfigured
> Jenkins to use the GIT workspace as the RPM build dir, so jenkins always
> purges RPMs. Since then we've not run out of space again AFAIK. None the
> less, I agree that 15 GiB is probably the min we need

Mainly because some of the guests have only 10 GiB disk.

> > > diff --git a/ansible/kickstart.cfg b/ansible/kickstart.cfg
> > > new file mode 100644
> > > index 000..c28f275
> > > --- /dev/null
> > > +++ b/ansible/kickstart.cfg
> > > @@ -0,0 +1,60 @@
> > > +# Installer configuration
> > > +#
> > > +# Perform a text based installation followed by a reboot, and disable
> > > +# the first boot assistant
> > > +text
> > > +install
> > > +reboot
> > > +firstboot --disable
> > > +
> > > +
> > > +# Environment configuration
> > > +#
> > > +# Locale, keyboard and timezone. All these will be configured again
> > > +# later with Ansible, but they're required information so we must
> > > +# provide them
> > > +lang en_US.UTF-8
> > > +keyboard us
> > > +timezone --utc Europe/Prague
> 
> How about UTC as a neutral option for timezone ;-)

+1

> 
> 
> > > +
> > > +
> > > +# User creation
> > > +#
> > > +# We don't create any user except for root. We can use a very insecure
> > > +# root password because the guest will not be exposed to the Internet:
> > > +# it will only be accessible from the host itself
> > > +authconfig --enableshadow --passalgo=sha512
> > > +rootpw --plaintext root
> > > +
> > > +
> > > +# Partition disk
> > > +#
> > > +# Erase everything and set up a 2 GiB swap partition, then assign all
> > > +# remaining space to the root partition
> > > +ignoredisk --only-use=vda
> > > +zerombr
> > > +clearpart --none
> > > +part / --fstype=ext4 --size=2048 --grow
> > > +part swap --fstype=swap --size=2048
> 
> We shouldn't need 2 GiB swap - any 

Re: [libvirt] [libvirt-jenkins-ci PATCH 1/5] ansible: Remove bootstrap phase

[Daniel P. Berrange]

On Tue, Oct 17, 2017 at 06:05:23PM +0200, Pavel Hrdina wrote:
> On Mon, Oct 16, 2017 at 06:02:04PM +0200, Andrea Bolognani wrote:
> > Having to bootstrap the guest as a separate phase is annoying and
> > can be avoided by assuming the root password is well-known.
> 
> I'm not sure about this.  Yes the password will be well known for us
> but I would rather have it generated and stored somewhere on the host.
> 
> The guests are hidden from internet but they are still connected to
> jenkins and are executing commands provided by jenkins.  Maybe I'm
> just too paranoid :).

Could we just generate a random root password, but install SSH public
keys and set SSH to only permit public key auth. 

That way if there is compromised code that we build for whatever
reasons, it can't use 'su' to escalate to root in the build VMs.

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] [libvirt-jenkins-ci PATCH 1/5] ansible: Remove bootstrap phase

[Pavel Hrdina]

On Mon, Oct 16, 2017 at 06:02:04PM +0200, Andrea Bolognani wrote:
> Having to bootstrap the guest as a separate phase is annoying and
> can be avoided by assuming the root password is well-known.

I'm not sure about this.  Yes the password will be well known for us
but I would rather have it generated and stored somewhere on the host.

The guests are hidden from internet but they are still connected to
jenkins and are executing commands provided by jenkins.  Maybe I'm
just too paranoid :).

Pavel


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

Re: [libvirt] [libvirt-jenkins-ci PATCH 3/5] ansible: Add unattended installation support

[Daniel P. Berrange]

On Tue, Oct 17, 2017 at 05:57:30PM +0200, Pavel Hrdina wrote:
> On Mon, Oct 16, 2017 at 06:02:06PM +0200, Andrea Bolognani wrote:
> > The 'manage' tool can now be used to install most known guests
> > without requiring user interaction.
> > 
> > Signed-off-by: Andrea Bolognani 
> > ---
> >  ansible/group_vars/all/install.yml | 10 +++
> >  ansible/host_vars/libvirt-centos-6/install.yml |  3 +
> >  ansible/host_vars/libvirt-centos-7/install.yml |  3 +
> >  ansible/host_vars/libvirt-debian-8/install.yml |  3 +
> >  ansible/host_vars/libvirt-debian-9/install.yml |  3 +
> >  ansible/host_vars/libvirt-fedora-25/install.yml|  3 +
> >  ansible/host_vars/libvirt-fedora-26/install.yml|  3 +
> >  .../host_vars/libvirt-fedora-rawhide/install.yml   |  3 +
> >  ansible/host_vars/libvirt-ubuntu-12/install.yml|  3 +
> >  ansible/host_vars/libvirt-ubuntu-14/install.yml|  3 +
> >  ansible/host_vars/libvirt-ubuntu-16/install.yml|  3 +
> >  ansible/kickstart.cfg  | 60 +++
> >  ansible/manage | 74 +++
> >  ansible/preseed.cfg| 85 
> > ++
> >  14 files changed, 259 insertions(+)
> >  create mode 100644 ansible/group_vars/all/install.yml
> >  create mode 100644 ansible/host_vars/libvirt-centos-6/install.yml
> >  create mode 100644 ansible/host_vars/libvirt-centos-7/install.yml
> >  create mode 100644 ansible/host_vars/libvirt-debian-8/install.yml
> >  create mode 100644 ansible/host_vars/libvirt-debian-9/install.yml
> >  create mode 100644 ansible/host_vars/libvirt-fedora-25/install.yml
> >  create mode 100644 ansible/host_vars/libvirt-fedora-26/install.yml
> >  create mode 100644 ansible/host_vars/libvirt-fedora-rawhide/install.yml
> 
> I wouldn't include Ubuntu related things.  Yes, we use travis where they
> have Ubuntu nodes, but this is jenkins-ci repository where we don't use
> Ubuntu at all.
> 
> >  create mode 100644 ansible/host_vars/libvirt-ubuntu-12/install.yml
> >  create mode 100644 ansible/host_vars/libvirt-ubuntu-14/install.yml
> >  create mode 100644 ansible/host_vars/libvirt-ubuntu-16/install.yml
> >  create mode 100644 ansible/kickstart.cfg
> >  create mode 100644 ansible/preseed.cfg
> > 
> > diff --git a/ansible/group_vars/all/install.yml 
> > b/ansible/group_vars/all/install.yml
> > new file mode 100644
> > index 000..714328e
> > --- /dev/null
> > +++ b/ansible/group_vars/all/install.yml
> > @@ -0,0 +1,10 @@
> > +---
> > +# Sizes are in GiB
> > +install_virt_type: kvm
> > +install_arch: x86_64
> > +install_machine: pc
> > +install_vcpus: 2
> > +install_memory_size: 2
> > +install_disk_size: 10
> 
> Currently we have 15 GiB per guest and in some cases we are able to run
> out of space.  Let's use 15 GiB.

We used to run out of space periodically because our RPM build jobs were
creating RPMs in $HOME/rpmbuild which jenkins never purged. I reconfigured
Jenkins to use the GIT workspace as the RPM build dir, so jenkins always
purges RPMs. Since then we've not run out of space again AFAIK. None the
less, I agree that 15 GiB is probably the min we need


> > diff --git a/ansible/kickstart.cfg b/ansible/kickstart.cfg
> > new file mode 100644
> > index 000..c28f275
> > --- /dev/null
> > +++ b/ansible/kickstart.cfg
> > @@ -0,0 +1,60 @@
> > +# Installer configuration
> > +#
> > +# Perform a text based installation followed by a reboot, and disable
> > +# the first boot assistant
> > +text
> > +install
> > +reboot
> > +firstboot --disable
> > +
> > +
> > +# Environment configuration
> > +#
> > +# Locale, keyboard and timezone. All these will be configured again
> > +# later with Ansible, but they're required information so we must
> > +# provide them
> > +lang en_US.UTF-8
> > +keyboard us
> > +timezone --utc Europe/Prague

How about UTC as a neutral option for timezone ;-)


> > +
> > +
> > +# User creation
> > +#
> > +# We don't create any user except for root. We can use a very insecure
> > +# root password because the guest will not be exposed to the Internet:
> > +# it will only be accessible from the host itself
> > +authconfig --enableshadow --passalgo=sha512
> > +rootpw --plaintext root
> > +
> > +
> > +# Partition disk
> > +#
> > +# Erase everything and set up a 2 GiB swap partition, then assign all
> > +# remaining space to the root partition
> > +ignoredisk --only-use=vda
> > +zerombr
> > +clearpart --none
> > +part / --fstype=ext4 --size=2048 --grow
> > +part swap --fstype=swap --size=2048

We shouldn't need 2 GiB swap - any builder that uses even a few 100 MB
of swap is doomed.  I'd give it 300 MB swap max.



> > +virt-install \
> > +--name "$GUEST" \
> > +--location "$INSTALL_URL" \
> > +--virt-type "$INSTALL_VIRT_TYPE" \
> > +--arch "$INSTALL_ARCH" \
> > +--machine "$INSTALL_MACHINE"  \
> > +--vcpus "$INSTALL_VCPUS" \
> > +

Re: [libvirt] [libvirt-jenkins-ci PATCH 3/5] ansible: Add unattended installation support

[Pavel Hrdina]

On Mon, Oct 16, 2017 at 06:02:06PM +0200, Andrea Bolognani wrote:
> The 'manage' tool can now be used to install most known guests
> without requiring user interaction.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  ansible/group_vars/all/install.yml | 10 +++
>  ansible/host_vars/libvirt-centos-6/install.yml |  3 +
>  ansible/host_vars/libvirt-centos-7/install.yml |  3 +
>  ansible/host_vars/libvirt-debian-8/install.yml |  3 +
>  ansible/host_vars/libvirt-debian-9/install.yml |  3 +
>  ansible/host_vars/libvirt-fedora-25/install.yml|  3 +
>  ansible/host_vars/libvirt-fedora-26/install.yml|  3 +
>  .../host_vars/libvirt-fedora-rawhide/install.yml   |  3 +
>  ansible/host_vars/libvirt-ubuntu-12/install.yml|  3 +
>  ansible/host_vars/libvirt-ubuntu-14/install.yml|  3 +
>  ansible/host_vars/libvirt-ubuntu-16/install.yml|  3 +
>  ansible/kickstart.cfg  | 60 +++
>  ansible/manage | 74 +++
>  ansible/preseed.cfg| 85 
> ++
>  14 files changed, 259 insertions(+)
>  create mode 100644 ansible/group_vars/all/install.yml
>  create mode 100644 ansible/host_vars/libvirt-centos-6/install.yml
>  create mode 100644 ansible/host_vars/libvirt-centos-7/install.yml
>  create mode 100644 ansible/host_vars/libvirt-debian-8/install.yml
>  create mode 100644 ansible/host_vars/libvirt-debian-9/install.yml
>  create mode 100644 ansible/host_vars/libvirt-fedora-25/install.yml
>  create mode 100644 ansible/host_vars/libvirt-fedora-26/install.yml
>  create mode 100644 ansible/host_vars/libvirt-fedora-rawhide/install.yml

I wouldn't include Ubuntu related things.  Yes, we use travis where they
have Ubuntu nodes, but this is jenkins-ci repository where we don't use
Ubuntu at all.

>  create mode 100644 ansible/host_vars/libvirt-ubuntu-12/install.yml
>  create mode 100644 ansible/host_vars/libvirt-ubuntu-14/install.yml
>  create mode 100644 ansible/host_vars/libvirt-ubuntu-16/install.yml
>  create mode 100644 ansible/kickstart.cfg
>  create mode 100644 ansible/preseed.cfg
> 
> diff --git a/ansible/group_vars/all/install.yml 
> b/ansible/group_vars/all/install.yml
> new file mode 100644
> index 000..714328e
> --- /dev/null
> +++ b/ansible/group_vars/all/install.yml
> @@ -0,0 +1,10 @@
> +---
> +# Sizes are in GiB
> +install_virt_type: kvm
> +install_arch: x86_64
> +install_machine: pc
> +install_vcpus: 2
> +install_memory_size: 2
> +install_disk_size: 10

Currently we have 15 GiB per guest and in some cases we are able to run
out of space.  Let's use 15 GiB.

> +install_storage_pool: default
> +install_network: default
> diff --git a/ansible/host_vars/libvirt-centos-6/install.yml 
> b/ansible/host_vars/libvirt-centos-6/install.yml
> new file mode 100644
> index 000..3a9459b
> --- /dev/null
> +++ b/ansible/host_vars/libvirt-centos-6/install.yml
> @@ -0,0 +1,3 @@
> +---
> +install_url: http://mirror.centos.org/centos/6/os/x86_64/
> +install_config: kickstart.cfg
> diff --git a/ansible/host_vars/libvirt-centos-7/install.yml 
> b/ansible/host_vars/libvirt-centos-7/install.yml
> new file mode 100644
> index 000..f003b89
> --- /dev/null
> +++ b/ansible/host_vars/libvirt-centos-7/install.yml
> @@ -0,0 +1,3 @@
> +---
> +install_url: http://mirror.centos.org/centos/7/os/x86_64/
> +install_config: kickstart.cfg
> diff --git a/ansible/host_vars/libvirt-debian-8/install.yml 
> b/ansible/host_vars/libvirt-debian-8/install.yml
> new file mode 100644
> index 000..a2c8341
> --- /dev/null
> +++ b/ansible/host_vars/libvirt-debian-8/install.yml
> @@ -0,0 +1,3 @@
> +---
> +install_url: http://deb.debian.org/debian/dists/jessie/main/installer-amd64/
> +install_config: preseed.cfg
> diff --git a/ansible/host_vars/libvirt-debian-9/install.yml 
> b/ansible/host_vars/libvirt-debian-9/install.yml
> new file mode 100644
> index 000..5b1da76
> --- /dev/null
> +++ b/ansible/host_vars/libvirt-debian-9/install.yml
> @@ -0,0 +1,3 @@
> +---
> +install_url: http://deb.debian.org/debian/dists/stretch/main/installer-amd64/
> +install_config: preseed.cfg
> diff --git a/ansible/host_vars/libvirt-fedora-25/install.yml 
> b/ansible/host_vars/libvirt-fedora-25/install.yml
> new file mode 100644
> index 000..bb4bde3
> --- /dev/null
> +++ b/ansible/host_vars/libvirt-fedora-25/install.yml
> @@ -0,0 +1,3 @@
> +---
> +install_url: 
> https://download.fedoraproject.org/pub/fedora/linux/releases/25/Server/x86_64/os
> +install_config: kickstart.cfg
> diff --git a/ansible/host_vars/libvirt-fedora-26/install.yml 
> b/ansible/host_vars/libvirt-fedora-26/install.yml
> new file mode 100644
> index 000..eff160d
> --- /dev/null
> +++ b/ansible/host_vars/libvirt-fedora-26/install.yml
> @@ -0,0 +1,3 @@
> +---
> +install_url: 
> https://download.fedoraproject.org/pub/fedora/linux/releases/26/Server/x86_64/os
> +install_config: kickstart.cfg
> diff --git 

[libvirt] [PATCH 0/2] Clean up some forgotten tabs from our source files

[Erik Skultety]

I don't think a reliable syntax-check rule checking tab spacing can be used,
but I can of course be proven wrong.

Erik Skultety (2):
  maint: Replace tabs with spaces in all source files in repo
  maint: Remove not-so-much informative block commentaries

 daemon/remote.c |   7 -
 docs/search.php.code.in | 350 ++--
 include/libvirt/virterror.h | 278 +--
 src/bhyve/bhyve_monitor.c   |   2 +-
 src/bhyve/bhyve_process.c   |   2 +-
 src/conf/node_device_conf.h |  52 +++
 src/conf/nwfilter_conf.h|   2 +-
 src/conf/virnodedeviceobj.c |   2 +-
 src/conf/virnodedeviceobj.h |   4 +-
 src/network/bridge_driver.c |   2 -
 src/qemu/qemu_domain.h  |   8 +-
 src/qemu/qemu_monitor.h |   2 +-
 src/util/viraudit.h |   4 +-
 src/util/virconf.c  |  35 +
 src/util/virconf.h  |  10 +-
 src/util/virerror.h |   5 -
 src/util/virsysinfo.c   |   2 +-
 src/util/virxml.c   |   8 +-
 src/vbox/vbox_XPCOMCGlue.c  |   9 --
 src/xen/xen_driver.h|   2 +-
 src/xen/xen_hypervisor.c| 168 ++---
 src/xen/xen_hypervisor.h|   2 +-
 src/xen/xend_internal.c |  23 +--
 src/xen/xend_internal.h |   8 +-
 src/xen/xs_internal.c   |  18 +--
 src/xen/xs_internal.h   |  14 +-
 src/xenconfig/xen_sxpr.c|   5 -
 27 files changed, 468 insertions(+), 556 deletions(-)

--
2.13.6

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

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

[Erik Skultety]

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

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

diff --git a/docs/search.php.code.in b/docs/search.php.code.in
index 3c66f94ca..01a6a64d2 100644
--- a/docs/search.php.code.in
+++ b/docs/search.php.code.in
@@ -27,199 +27,199 @@
  $rb) ? -1 : 1;
+list($ra,$ta,$ma,$na,$da) = $a;
+list($rb,$tb,$mb,$nb,$db) = $b;
+if ($ra == $rb) return 0;
+return ($ra > $rb) ? -1 : 1;
 }
 if (($query) && (strlen($query) <= 50)) {
-   $link = mysql_connect ("localhost", "nobody");
-   if (!$link) {
-   echo " Could not connect to the database: ", mysql_error();
-   } else {
-   mysql_select_db("libvir", $link);
-   $list = explode (" ", $query);
-   $results = array();
-   $number = 0;
-   for ($number = 0;$number < count($list);$number++) {
+$link = mysql_connect ("localhost", "nobody");
+if (!$link) {
+echo " Could not connect to the database: ", mysql_error();
+} else {
+mysql_select_db("libvir", $link);
+$list = explode (" ", $query);
+$results = array();
+$number = 0;
+for ($number = 0;$number < count($list);$number++) {
 
-   $word = $list[$number];
-   if (($scope == 'any') || ($scope == 'API')) {
-   list($result, $j) = queryWord($word);
-   if ($j > 0) {
-   for ($i = 0; $i < $j; $i++) {
-   $relevance = mysql_result($result, $i, 0);
-   $name = mysql_result($result, $i, 1);
-   $type = mysql_result($result, $i, 2);
-   $module = mysql_result($result, $i, 3);
-   $desc = mysql_result($result, $i, 4);
-   if (array_key_exists($name, $results)) {
-   list($r,$t,$m,$d,$w,$u) = $results[$name];
-   $results[$name] = array(($r + $relevance) * 2,
-   $t,$m,$d,$w,$u);
-   } else {
-   $id = $name;
-   $m = strtolower($module);
-   $url = "html/libvirt-$module.html#$id";
-   $results[$name] = array($relevance,$type,
-   $module, $desc, $name, $url);
-   }
-   }
-   mysql_free_result($result);
-   }
-   }
-   if (($scope == 'any') || ($scope == 'DOCS')) {
-   list($result, $k) = queryHTMLWord($word);
-   if ($k > 0) {
-   for ($i = 0; $i < $k; $i++) {
-   $relevance = mysql_result($result, $i, 0);
-   $name = mysql_result($result, $i, 1);
-   $id = mysql_result($result, $i, 2);
-   $module = mysql_result($result, $i, 3);
-   $desc = mysql_result($result, $i, 4);
-   $url = $module;
-   if ($id != "") {
-   $url = $url + "#$id";
-   }
-   $results["$name _html_ $number _ $i"] =
- array($relevance, "XML docs",
-   $module, $desc, 

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

[Erik Skultety]

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

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

diff --git a/daemon/remote.c b/daemon/remote.c
index fd8542120..3f7d2d344 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -3750,10 +3750,6 @@ remoteDispatchAuthPolkit(virNetServerPtr server,
 }
 
 
-/***
- * NODE INFO APIS
- **/
-
 static int
 remoteDispatchNodeDeviceGetParent(virNetServerPtr server ATTRIBUTE_UNUSED,
   virNetServerClientPtr client 
ATTRIBUTE_UNUSED,
@@ -3864,9 +3860,6 @@ 
remoteDispatchConnectUnregisterCloseCallback(virNetServerPtr server ATTRIBUTE_UN
 return rv;
 }
 
-/***
- * Register / deregister events
- ***/
 static int
 remoteDispatchConnectDomainEventRegister(virNetServerPtr server 
ATTRIBUTE_UNUSED,
  virNetServerClientPtr client,
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 530d00ff3..fcaa66df9 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -4286,8 +4286,6 @@ networkRegister(void)
 }
 
 
-//
-
 /* A unified function to log network connections and disconnections */
 
 static void
diff --git a/src/util/virconf.c b/src/util/virconf.c
index a88cc9901..5822ee45e 100644
--- a/src/util/virconf.c
+++ b/src/util/virconf.c
@@ -44,12 +44,6 @@
 
 VIR_LOG_INIT("util.conf");
 
-/
- *  *
- *  Structures and macros used by the mini parser   *
- *  *
- /
-
 typedef struct _virConfParserCtxt virConfParserCtxt;
 typedef virConfParserCtxt *virConfParserCtxtPtr;
 
@@ -75,12 +69,6 @@ struct _virConfParserCtxt {
   do { while ((ctxt->cur < ctxt->end) && (c_isblank(CUR)))  \
   ctxt->cur++; } while (0)
 
-/
- *  *
- *  Structures used by configuration data   *
- *  *
- /
-
 VIR_ENUM_IMPL(virConf, VIR_CONF_LAST,
   "*unexpected*",
   "long",
@@ -133,12 +121,6 @@ virConfErrorHelper(const char *file, const char *func, 
size_t line,
 }
 
 
-/
- *  *
- *  Structures allocations and deallocations*
- *  *
- /
-
 /**
  * virConfFreeList:
  * @list: the list to free
@@ -260,11 +242,6 @@ virConfAddEntry(virConfPtr conf, char *name, 
virConfValuePtr value, char *comm)
 return ret;
 }
 
-/
- *  *
- *  Serialization   *
- *  *
- /
 
 /**
  * virConfSaveValue:
@@ -353,11 +330,6 @@ virConfSaveEntry(virBufferPtr buf, virConfEntryPtr cur)
 return 0;
 }
 
-/
- *  *
- *  The parser core *
- *  *
- /
 
 /**
  * virConfParseLong:
@@ -754,11 +726,6 @@ virConfParse(const char *filename, const char *content, 
int len,
 return NULL;
 }
 

[libvirt] [PATCH 10/12] Introduce qemuBuildInputDevStr

[Ján Tomko]

A function that builds the -device string for input devices.
---
 src/qemu/qemu_command.c | 42 +-
 src/qemu/qemu_command.h |  7 +++
 2 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 138bbdf1a..7bdff85fc 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4303,6 +4303,27 @@ qemuBuildUSBInputDevStr(const virDomainDef *def,
 }
 
 
+int
+qemuBuildInputDevStr(char **devstr,
+ const virDomainDef *def,
+ virDomainInputDefPtr input,
+ virQEMUCapsPtr qemuCaps)
+{
+switch (input->bus) {
+case VIR_DOMAIN_INPUT_BUS_USB:
+if (!(*devstr = qemuBuildUSBInputDevStr(def, input, qemuCaps)))
+return -1;
+break;
+
+case VIR_DOMAIN_INPUT_BUS_VIRTIO:
+if (!(*devstr = qemuBuildVirtioInputDevStr(def, input, qemuCaps)))
+return -1;
+break;
+}
+return 0;
+}
+
+
 static int
 qemuBuildInputCommandLine(virCommandPtr cmd,
   const virDomainDef *def,
@@ -4312,22 +4333,17 @@ qemuBuildInputCommandLine(virCommandPtr cmd,
 
 for (i = 0; i < def->ninputs; i++) {
 virDomainInputDefPtr input = def->inputs[i];
+char *devstr = NULL;
 
-if (input->bus == VIR_DOMAIN_INPUT_BUS_USB) {
-char *optstr;
-virCommandAddArg(cmd, "-device");
-if (!(optstr = qemuBuildUSBInputDevStr(def, input, qemuCaps)))
-return -1;
-virCommandAddArg(cmd, optstr);
-VIR_FREE(optstr);
-} else if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO) {
-char *optstr;
+if (qemuBuildInputDevStr(, def, input, qemuCaps) < 0)
+return -1;
+
+if (devstr) {
 virCommandAddArg(cmd, "-device");
-if (!(optstr = qemuBuildVirtioInputDevStr(def, input, qemuCaps)))
-return -1;
-virCommandAddArg(cmd, optstr);
-VIR_FREE(optstr);
+virCommandAddArg(cmd, devstr);
 }
+
+VIR_FREE(devstr);
 }
 
 return 0;
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 1254ad4df..0961ec8cb 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -204,4 +204,11 @@ char *qemuBuildWatchdogDevStr(const virDomainDef *def,
   virDomainWatchdogDefPtr dev,
   virQEMUCapsPtr qemuCaps);
 
+int qemuBuildInputDevStr(char **devstr,
+ const virDomainDef *def,
+ virDomainInputDefPtr input,
+ virQEMUCapsPtr qemuCaps)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ATTRIBUTE_NONNULL(4);
+
 #endif /* __QEMU_COMMAND_H__*/
-- 
2.13.0

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

[libvirt] [PATCH 11/12] qemu: implement input device hotplug

[Ján Tomko]

For both virtio input devices and USB input devices.

https://bugzilla.redhat.com/show_bug.cgi?id=1379603
---
 src/libvirt_private.syms |  2 ++
 src/qemu/qemu_driver.c   |  9 +-
 src/qemu/qemu_hotplug.c  | 73 
 src/qemu/qemu_hotplug.h  |  5 
 4 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7713ecc01..f5cdbeb66 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -157,6 +157,7 @@ virDomainAuditDisk;
 virDomainAuditFS;
 virDomainAuditHostdev;
 virDomainAuditInit;
+virDomainAuditInput;
 virDomainAuditIOThread;
 virDomainAuditMemory;
 virDomainAuditNet;
@@ -385,6 +386,7 @@ virDomainHubTypeFromString;
 virDomainHubTypeToString;
 virDomainHypervTypeFromString;
 virDomainHypervTypeToString;
+virDomainInputBusTypeToString;
 virDomainInputDefFind;
 virDomainInputDefFree;
 virDomainIOMMUModelTypeFromString;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 092df673c..75a0e42aa 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7664,9 +7664,16 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
 }
 break;
 
+case VIR_DOMAIN_DEVICE_INPUT:
+ret = qemuDomainAttachInputDevice(driver, vm, dev->data.input);
+if (ret == 0) {
+alias = dev->data.input->info.alias;
+dev->data.input = NULL;
+}
+break;
+
 case VIR_DOMAIN_DEVICE_NONE:
 case VIR_DOMAIN_DEVICE_FS:
-case VIR_DOMAIN_DEVICE_INPUT:
 case VIR_DOMAIN_DEVICE_SOUND:
 case VIR_DOMAIN_DEVICE_VIDEO:
 case VIR_DOMAIN_DEVICE_GRAPHICS:
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c65e7e500..b32acb71e 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2905,6 +2905,79 @@ qemuDomainAttachWatchdog(virQEMUDriverPtr driver,
 }
 
 
+int
+qemuDomainAttachInputDevice(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+virDomainInputDefPtr input)
+{
+int ret = -1;
+char *devstr = NULL;
+qemuDomainObjPrivatePtr priv = vm->privateData;
+virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_INPUT,
+   { .input = input } };
+bool releaseaddr = false;
+
+if (input->bus != VIR_DOMAIN_INPUT_BUS_USB &&
+input->bus != VIR_DOMAIN_INPUT_BUS_VIRTIO) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("input device on bus '%s' cannot be hot plugged."),
+   virDomainInputBusTypeToString(input->bus));
+return -1;
+}
+
+if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO) {
+if (qemuDomainEnsureVirtioAddress(, vm, , "input") < 0)
+return -1;
+} else if (input->bus == VIR_DOMAIN_INPUT_BUS_USB) {
+if (priv->usbaddrs) {
+if (virDomainUSBAddressEnsure(priv->usbaddrs, >info) < 0)
+goto cleanup;
+releaseaddr = true;
+}
+}
+
+if (qemuAssignDeviceInputAlias(vm->def, input, -1) < 0)
+goto cleanup;
+
+if (qemuBuildInputDevStr(, vm->def, input, priv->qemuCaps) < 0)
+goto cleanup;
+
+if (VIR_REALLOC_N(vm->def->inputs, vm->def->ninputs + 1) < 0)
+goto cleanup;
+
+qemuDomainObjEnterMonitor(driver, vm);
+if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
+goto exit_monitor;
+
+if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+releaseaddr = false;
+goto cleanup;
+}
+
+VIR_APPEND_ELEMENT_COPY_INPLACE(vm->def->inputs, vm->def->ninputs, input);
+
+ret = 0;
+releaseaddr = false;
+
+ audit:
+virDomainAuditInput(vm, input, "attach", ret == 0);
+
+ cleanup:
+if (releaseaddr)
+qemuDomainReleaseDeviceAddress(vm, >info, NULL);
+
+VIR_FREE(devstr);
+return ret;
+
+ exit_monitor:
+if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+releaseaddr = false;
+goto cleanup;
+}
+goto audit;
+}
+
+
 static int
 qemuDomainChangeNetBridge(virDomainObjPtr vm,
   virDomainNetDefPtr olddev,
diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
index 3455832d6..985b7495b 100644
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -125,6 +125,11 @@ int qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
 int qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
  virDomainWatchdogDefPtr watchdog);
+
+int qemuDomainAttachInputDevice(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+virDomainInputDefPtr input);
+
 int qemuDomainAttachLease(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
   virDomainLeaseDefPtr lease);
-- 
2.13.0

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

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

[Ján Tomko]

Allow unplugging USB and virtio USB devices.

https://bugzilla.redhat.com/show_bug.cgi?id=1379603
---
 src/qemu/qemu_driver.c  |  4 ++-
 src/qemu/qemu_hotplug.c | 76 +
 src/qemu/qemu_hotplug.h |  3 ++
 3 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 75a0e42aa..a9d3ba778 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7764,9 +7764,11 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
 case VIR_DOMAIN_DEVICE_WATCHDOG:
 ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog);
 break;
+case VIR_DOMAIN_DEVICE_INPUT:
+ret = qemuDomainDetachInputDevice(vm, dev->data.input);
+break;
 
 case VIR_DOMAIN_DEVICE_FS:
-case VIR_DOMAIN_DEVICE_INPUT:
 case VIR_DOMAIN_DEVICE_SOUND:
 case VIR_DOMAIN_DEVICE_VIDEO:
 case VIR_DOMAIN_DEVICE_GRAPHICS:
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index b32acb71e..85faa2a46 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -4430,6 +4430,31 @@ qemuDomainRemoveWatchdog(virQEMUDriverPtr driver,
 }
 
 
+static int
+qemuDomainRemoveInputDevice(virDomainObjPtr vm,
+virDomainInputDefPtr dev)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+virQEMUDriverPtr driver = priv->driver;
+virObjectEventPtr event = NULL;
+size_t i;
+
+VIR_DEBUG("Removing input device %s from domain %p %s",
+  dev->info.alias, vm, vm->def->name);
+
+event = virDomainEventDeviceRemovedNewFromObj(vm, dev->info.alias);
+qemuDomainEventQueue(driver, event);
+for (i = 0; i < vm->def->ninputs; i++) {
+if (vm->def->inputs[i] == dev)
+break;
+}
+qemuDomainReleaseDeviceAddress(vm, >info, NULL);
+virDomainInputDefFree(vm->def->inputs[i]);
+VIR_DELETE_ELEMENT(vm->def->inputs, i, vm->def->ninputs);
+return 0;
+}
+
+
 int
 qemuDomainRemoveDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
@@ -6184,3 +6209,54 @@ qemuDomainSetVcpuInternal(virQEMUDriverPtr driver,
 virObjectUnref(cfg);
 return ret;
 }
+
+
+int
+qemuDomainDetachInputDevice(virDomainObjPtr vm,
+virDomainInputDefPtr def)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+virQEMUDriverPtr driver = priv->driver;
+virDomainInputDefPtr input;
+int ret = -1;
+int idx;
+
+if ((idx = virDomainInputDefFind(vm->def, def)) < 0) {
+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+   _("matching input device not found"));
+return -1;
+}
+input = vm->def->inputs[idx];
+
+switch ((virDomainInputBus) input->bus) {
+case VIR_DOMAIN_INPUT_BUS_PS2:
+case VIR_DOMAIN_INPUT_BUS_XEN:
+case VIR_DOMAIN_INPUT_BUS_PARALLELS:
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("input device on bus '%s' cannot be detached"),
+   virDomainInputBusTypeToString(input->bus));
+return -1;
+
+case VIR_DOMAIN_INPUT_BUS_LAST:
+case VIR_DOMAIN_INPUT_BUS_USB:
+case VIR_DOMAIN_INPUT_BUS_VIRTIO:
+break;
+}
+
+qemuDomainMarkDeviceForRemoval(vm, >info);
+
+qemuDomainObjEnterMonitor(driver, vm);
+if (qemuMonitorDelDevice(priv->mon, input->info.alias)) {
+ignore_value(qemuDomainObjExitMonitor(driver, vm));
+goto cleanup;
+}
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
+goto cleanup;
+
+if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
+ret = qemuDomainRemoveInputDevice(vm, input);
+
+ cleanup:
+qemuDomainResetDeviceRemoval(vm);
+return ret;
+}
diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
index 985b7495b..3e0d618e0 100644
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -184,4 +184,7 @@ int qemuDomainSetVcpuInternal(virQEMUDriverPtr driver,
   virBitmapPtr vcpus,
   bool state);
 
+int qemuDomainDetachInputDevice(virDomainObjPtr vm,
+virDomainInputDefPtr def);
+
 #endif /* __QEMU_HOTPLUG_H__ */
-- 
2.13.0

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

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

[Ján Tomko]

Move assignment of input device alias into a separate function,
for reuse on hotplug.
---
 src/qemu/qemu_alias.c | 24 +++-
 src/qemu/qemu_alias.h |  3 +++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index 72df1083f..737fc2fda 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -412,6 +412,28 @@ qemuAssignDeviceWatchdogAlias(virDomainWatchdogDefPtr 
watchdog)
 
 if (VIR_STRDUP(watchdog->info.alias, "watchdog0") < 0)
 return -1;
+
+return 0;
+}
+
+int
+qemuAssignDeviceInputAlias(virDomainDefPtr def,
+   virDomainInputDefPtr input,
+   int idx)
+{
+if (idx == -1) {
+int thisidx;
+size_t i;
+
+for (i = 0; i < def->ninputs; i++) {
+if ((thisidx = qemuDomainDeviceAliasIndex(>inputs[i]->info, 
"input")) >= idx)
+idx = thisidx + 1;
+}
+}
+
+if (virAsprintf(>info.alias, "input%d", idx) < 0)
+return -1;
+
 return 0;
 }
 
@@ -461,7 +483,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr 
qemuCaps)
 return -1;
 }
 for (i = 0; i < def->ninputs; i++) {
-if (virAsprintf(>inputs[i]->info.alias, "input%zu", i) < 0)
+if (qemuAssignDeviceInputAlias(def, def->inputs[i], i) < 0)
 return -1;
 }
 for (i = 0; i < def->nparallels; i++) {
diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
index 652ffea0c..35424829f 100644
--- a/src/qemu/qemu_alias.h
+++ b/src/qemu/qemu_alias.h
@@ -66,6 +66,9 @@ int qemuAssignDeviceShmemAlias(virDomainDefPtr def,
int idx);
 
 int qemuAssignDeviceWatchdogAlias(virDomainWatchdogDefPtr watchdog);
+int qemuAssignDeviceInputAlias(virDomainDefPtr def,
+   virDomainInputDefPtr input,
+   int idx);
 
 int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps);
 
-- 
2.13.0

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

[libvirt] [PATCH 06/12] Use qemuDomainEnsureVirtioAddress where possible

[Ján Tomko]

There are two more cases where we set an S390/CCW/PCI address
type based on the machine type.

Reuse qemuDomainEnsureVirtioAddress to reduce repetition.
---
 src/qemu/qemu_hotplug.c | 56 -
 1 file changed, 4 insertions(+), 52 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 177c8a01d..c65e7e500 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -498,7 +498,6 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr 
driver,
 qemuDomainObjPrivatePtr priv = vm->privateData;
 virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_CONTROLLER,
{ .controller = controller } };
-virDomainCCWAddressSetPtr ccwaddrs = NULL;
 bool releaseaddr = false;
 
 if (controller->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
@@ -523,30 +522,9 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr 
driver,
 return -1;
 }
 
-if (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
-if (qemuDomainIsS390CCW(vm->def) &&
-virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW))
-controller->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
-else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390))
-controller->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390;
-} else {
-if (!qemuCheckCCWS390AddressSupport(vm->def, controller->info,
-priv->qemuCaps, "controller"))
-goto cleanup;
-}
+if (qemuDomainEnsureVirtioAddress(, vm, , "controller") < 
0)
+return -1;
 
-if (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
-controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
-if (qemuDomainEnsurePCIAddress(vm, , driver) < 0)
-goto cleanup;
-} else if (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
-if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def)))
-goto cleanup;
-if (virDomainCCWAddressAssign(>info, ccwaddrs,
-  !controller->info.addr.ccw.assigned) < 0)
-goto cleanup;
-}
-releaseaddr = true;
 if (qemuAssignDeviceControllerAlias(vm->def, priv->qemuCaps, controller) < 
0)
 goto cleanup;
 
@@ -575,7 +553,6 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr 
driver,
 qemuDomainReleaseDeviceAddress(vm, >info, NULL);
 
 VIR_FREE(devstr);
-virDomainCCWAddressSetFree(ccwaddrs);
 return ret;
 }
 
@@ -2115,7 +2092,6 @@ qemuDomainAttachRNGDevice(virConnectPtr conn,
 bool chardevAdded = false;
 bool objAdded = false;
 virJSONValuePtr props = NULL;
-virDomainCCWAddressSetPtr ccwaddrs = NULL;
 const char *type;
 int ret = -1;
 int rv;
@@ -2127,31 +2103,8 @@ qemuDomainAttachRNGDevice(virConnectPtr conn,
 if (VIR_REALLOC_N(vm->def->rngs, vm->def->nrngs + 1) < 0)
 goto cleanup;
 
-if (rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
-if (qemuDomainIsS390CCW(vm->def) &&
-virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) {
-rng->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
-} else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390)) {
-rng->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390;
-}
-} else {
-if (!qemuCheckCCWS390AddressSupport(vm->def, rng->info, priv->qemuCaps,
-"rng"))
-goto cleanup;
-}
-
-if (rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
-rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
-if (qemuDomainEnsurePCIAddress(vm, , driver) < 0)
-goto cleanup;
-} else if (rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
-if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def)))
-goto cleanup;
-if (virDomainCCWAddressAssign(>info, ccwaddrs,
-  !rng->info.addr.ccw.assigned) < 0)
-goto cleanup;
-}
-releaseaddr = true;
+if (qemuDomainEnsureVirtioAddress(, vm, , "rng") < 0)
+return -1;
 
 if (qemuDomainNamespaceSetupRNG(driver, vm, rng) < 0)
 goto cleanup;
@@ -2226,7 +2179,6 @@ qemuDomainAttachRNGDevice(virConnectPtr conn,
 VIR_FREE(charAlias);
 VIR_FREE(objAlias);
 VIR_FREE(devstr);
-virDomainCCWAddressSetFree(ccwaddrs);
 return ret;
 
  exit_monitor:
-- 
2.13.0

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

[libvirt] [PATCH 07/12] qemu: allow coldplugging input devices

[Ján Tomko]

https://bugzilla.redhat.com/show_bug.cgi?id=1379603
---
 src/qemu/qemu_driver.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fb4d72236..050a41524 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8060,6 +8060,10 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef,
 break;
 
 case VIR_DOMAIN_DEVICE_INPUT:
+if (VIR_APPEND_ELEMENT(vmdef->inputs, vmdef->ninputs, dev->data.input) 
< 0)
+return -1;
+break;
+
 case VIR_DOMAIN_DEVICE_SOUND:
 case VIR_DOMAIN_DEVICE_VIDEO:
 case VIR_DOMAIN_DEVICE_GRAPHICS:
-- 
2.13.0

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

[libvirt] [PATCH 02/12] qemuDomainAttachControllerDevice: remove dead code

[Ján Tomko]

After a successful attach, the device address has already been set.
Remove the pointless assignment.
---
 src/qemu/qemu_hotplug.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 0288986d8..2d13c2891 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -590,11 +590,8 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr 
driver,
 goto cleanup;
 }
 
-if (ret == 0) {
-if (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
-controller->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
+if (ret == 0)
 virDomainControllerInsertPreAlloced(vm->def, controller);
-}
 
  cleanup:
 if (ret != 0 && releaseaddr)
-- 
2.13.0

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

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

[Ján Tomko]

Let it be reused in qemu_domain_address.
---
 src/qemu/qemu_command.c | 40 
 src/qemu/qemu_command.h |  5 -
 src/qemu/qemu_domain.c  | 40 
 src/qemu/qemu_domain.h  |  5 +
 4 files changed, 45 insertions(+), 45 deletions(-)

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

[libvirt] [PATCH 03/12] qemuDomainAttachRNGDevice: do not access source.file randomly

[Ján Tomko]

We pass the source.file to qemuCheckCCWS390AddressSupport for
the purpose of reporting an error message without actually checking
that the rng device is of type VIR_DOMAIN_RNG_BACKEND_RANDOM.

Change it to a hardcoded "rng" string, which also avoids
referring to the device by a host-side attribute.
---
 src/qemu/qemu_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 2d13c2891..1e7931a84 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2159,7 +2159,7 @@ qemuDomainAttachRNGDevice(virConnectPtr conn,
 }
 } else {
 if (!qemuCheckCCWS390AddressSupport(vm->def, rng->info, priv->qemuCaps,
-rng->source.file))
+"rng"))
 goto cleanup;
 }
 
-- 
2.13.0

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

[libvirt] [PATCH 08/12] qemu: allow cold unplugging of input devices

[Ján Tomko]

https://bugzilla.redhat.com/show_bug.cgi?id=1379603
---
 src/conf/domain_conf.c   | 37 +
 src/conf/domain_conf.h   |  3 +++
 src/libvirt_private.syms |  1 +
 src/qemu/qemu_driver.c   |  8 
 4 files changed, 49 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 516f9fa06..495d85f90 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16556,6 +16556,43 @@ virDomainShmemDefRemove(virDomainDefPtr def,
 }
 
 
+static bool
+virDomainInputDefEquals(const virDomainInputDef *a,
+const virDomainInputDef *b)
+{
+if (a->type != b->type)
+return false;
+
+if (a->bus != b->bus)
+return false;
+
+if (a->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH &&
+STRNEQ_NULLABLE(a->source.evdev, b->source.evdev))
+return false;
+
+if (a->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+!virDomainDeviceInfoAddressIsEqual(>info, >info))
+return false;
+
+return true;
+}
+
+
+ssize_t
+virDomainInputDefFind(const virDomainDef *def,
+  const virDomainInputDef *input)
+{
+size_t i;
+
+for (i = 0; i < def->ninputs; i++) {
+if (virDomainInputDefEquals(input, def->inputs[i]))
+return i;
+}
+
+return -1;
+}
+
+
 char *
 virDomainDefGetDefaultEmulator(virDomainDefPtr def,
virCapsPtr caps)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a42efcfa6..c210bd418 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3204,6 +3204,9 @@ ssize_t virDomainShmemDefFind(virDomainDefPtr def, 
virDomainShmemDefPtr shmem)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
 virDomainShmemDefPtr virDomainShmemDefRemove(virDomainDefPtr def, size_t idx)
 ATTRIBUTE_NONNULL(1);
+ssize_t virDomainInputDefFind(const virDomainDef *def,
+  const virDomainInputDef *input)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
 
 VIR_ENUM_DECL(virDomainTaint)
 VIR_ENUM_DECL(virDomainVirt)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7bd21ae23..7713ecc01 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -385,6 +385,7 @@ virDomainHubTypeFromString;
 virDomainHubTypeToString;
 virDomainHypervTypeFromString;
 virDomainHypervTypeToString;
+virDomainInputDefFind;
 virDomainInputDefFree;
 virDomainIOMMUModelTypeFromString;
 virDomainIOMMUModelTypeToString;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 050a41524..092df673c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8239,6 +8239,14 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
 break;
 
 case VIR_DOMAIN_DEVICE_INPUT:
+if ((idx = virDomainInputDefFind(vmdef, dev->data.input)) < 0) {
+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+   _("matching input device not found"));
+return -1;
+}
+VIR_DELETE_ELEMENT(vmdef->inputs, idx, vmdef->ninputs);
+break;
+
 case VIR_DOMAIN_DEVICE_SOUND:
 case VIR_DOMAIN_DEVICE_VIDEO:
 case VIR_DOMAIN_DEVICE_GRAPHICS:
-- 
2.13.0

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

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

[Ján Tomko]

Split out the common code responsible for reserving/assigning
PCI/CCW addresses for virtio disks into a helper function
for reuse by other virtio devices.
---
 src/qemu/qemu_domain_address.c | 45 ++
 src/qemu/qemu_domain_address.h |  4 
 src/qemu/qemu_hotplug.c| 29 +++
 3 files changed, 52 insertions(+), 26 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index c4485d4ab..ebe9eb861 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -2904,3 +2904,48 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm,
 VIR_WARN("Unable to release USB address on %s",
  NULLSTR(devstr));
 }
+
+
+int
+qemuDomainEnsureVirtioAddress(bool *releaseAddr,
+  virDomainObjPtr vm,
+  virDomainDeviceDefPtr dev,
+  const char *devicename)
+{
+virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev);
+qemuDomainObjPrivatePtr priv = vm->privateData;
+virDomainCCWAddressSetPtr ccwaddrs = NULL;
+virQEMUDriverPtr driver = priv->driver;
+int ret = -1;
+
+if (!info->type) {
+if (qemuDomainIsS390CCW(vm->def) &&
+virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW))
+info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
+else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390))
+info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390;
+} else {
+if (!qemuCheckCCWS390AddressSupport(vm->def, *info, priv->qemuCaps,
+devicename))
+return -1;
+}
+
+if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
+if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def)))
+goto cleanup;
+if (virDomainCCWAddressAssign(info, ccwaddrs,
+  !info->addr.ccw.assigned) < 0)
+goto cleanup;
+} else if (!info->type ||
+   info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
+if (qemuDomainEnsurePCIAddress(vm, dev, driver) < 0)
+goto cleanup;
+*releaseAddr = true;
+}
+
+ret = 0;
+
+ cleanup:
+virDomainCCWAddressSetFree(ccwaddrs);
+return ret;
+}
diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h
index b5644fa9c..e951a4c88 100644
--- a/src/qemu/qemu_domain_address.h
+++ b/src/qemu/qemu_domain_address.h
@@ -59,6 +59,10 @@ qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def)
 int qemuDomainAssignMemoryDeviceSlot(virDomainDefPtr def,
  virDomainMemoryDefPtr mem);
 
+int qemuDomainEnsureVirtioAddress(bool *releaseAddr,
+  virDomainObjPtr vm,
+  virDomainDeviceDefPtr dev,
+  const char *devicename);
 
 # define __QEMU_DOMAIN_ADDRESS_H__
 
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 1e7931a84..177c8a01d 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -363,7 +363,6 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
 bool driveAdded = false;
 bool secobjAdded = false;
 bool encobjAdded = false;
-virDomainCCWAddressSetPtr ccwaddrs = NULL;
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 const char *src = virDomainDiskGetSource(disk);
 virJSONValuePtr secobjProps = NULL;
@@ -372,33 +371,12 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
 qemuDomainSecretInfoPtr secinfo;
 qemuDomainSecretInfoPtr encinfo;
 
-if (!disk->info.type) {
-if (qemuDomainIsS390CCW(vm->def) &&
-virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW))
-disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
-else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390))
-disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390;
-} else {
-if (!qemuCheckCCWS390AddressSupport(vm->def, disk->info, 
priv->qemuCaps,
-disk->dst))
-goto cleanup;
-}
-
 if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0)
 goto cleanup;
 
-if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
-if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def)))
-goto error;
-if (virDomainCCWAddressAssign(>info, ccwaddrs,
-  !disk->info.addr.ccw.assigned) < 0)
-goto error;
-} else if (!disk->info.type ||
-disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
-if (qemuDomainEnsurePCIAddress(vm, , driver) < 0)
-goto error;
-}
-releaseaddr = true;
+if (qemuDomainEnsureVirtioAddress(, vm, , disk->dst) < 0)
+goto error;
+
 if 

[libvirt] [PATCH 01/12] conf: audit passthrough input devices at domain startup

[Ján Tomko]

Introduce virDomainAuditInput and use it to log the evdev passed
to the guest.
---
 src/conf/domain_audit.c | 44 
 src/conf/domain_audit.h |  5 +
 2 files changed, 49 insertions(+)

diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
index 4afc22019..723c73736 100644
--- a/src/conf/domain_audit.c
+++ b/src/conf/domain_audit.c
@@ -868,6 +868,9 @@ virDomainAuditStart(virDomainObjPtr vm, const char *reason, 
bool success)
 for (i = 0; i < vm->def->nshmems; i++)
 virDomainAuditShmem(vm, vm->def->shmems[i], "start", true);
 
+for (i = 0; i < vm->def->ninputs; i++)
+virDomainAuditInput(vm, vm->def->inputs[i], "start", true);
+
 virDomainAuditMemory(vm, 0, virDomainDefGetMemoryTotal(vm->def),
  "start", true);
 virDomainAuditVcpu(vm, 0, virDomainDefGetVcpus(vm->def), "start", true);
@@ -983,3 +986,44 @@ virDomainAuditShmem(virDomainObjPtr vm,
 VIR_FREE(shmpath);
 return;
 }
+
+
+void
+virDomainAuditInput(virDomainObjPtr vm,
+virDomainInputDefPtr input,
+const char *reason,
+bool success)
+{
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+char *vmname;
+const char *virt = virDomainVirtTypeToString(vm->def->virtType);
+
+virUUIDFormat(vm->def->uuid, uuidstr);
+
+if (!(vmname = virAuditEncode("vm", vm->def->name)))
+goto no_memory;
+
+switch ((virDomainInputType) input->type) {
+case VIR_DOMAIN_INPUT_TYPE_MOUSE:
+case VIR_DOMAIN_INPUT_TYPE_TABLET:
+case VIR_DOMAIN_INPUT_TYPE_KBD:
+break;
+
+case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH:
+VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success,
+  "virt=%s resrc=evdev reason=%s %s uuid=%s path=%s",
+  virt, reason, vmname, uuidstr, 
VIR_AUDIT_STR(input->source.evdev));
+break;
+
+case VIR_DOMAIN_INPUT_TYPE_LAST:
+break;
+}
+
+ cleanup:
+VIR_FREE(vmname);
+return;
+
+ no_memory:
+VIR_WARN("OOM while encoding audit message");
+goto cleanup;
+}
diff --git a/src/conf/domain_audit.h b/src/conf/domain_audit.h
index 8cb585dc7..474ccb6b8 100644
--- a/src/conf/domain_audit.h
+++ b/src/conf/domain_audit.h
@@ -133,6 +133,11 @@ void virDomainAuditShmem(virDomainObjPtr vm,
  virDomainShmemDefPtr def,
  const char *reason, bool success)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
+void virDomainAuditInput(virDomainObjPtr vm,
+ virDomainInputDefPtr input,
+ const char *reason,
+ bool success)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
 
 
 #endif /* __VIR_DOMAIN_AUDIT_H__ */
-- 
2.13.0

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

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

[Ján Tomko]

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

Ján Tomko (12):
  conf: audit passthrough input devices at domain startup
  qemuDomainAttachControllerDevice: remove dead code
  qemuDomainAttachRNGDevice: do not access source.file randomly
  Move qemuCheckCCWS390AddressSupport to qemu_domain
  Split out qemuDomainEnsureVirtioAddress
  Use qemuDomainEnsureVirtioAddress where possible
  qemu: allow coldplugging input devices
  qemu: allow cold unplugging of input devices
  split out qemuAssignDeviceInputAlias
  Introduce qemuBuildInputDevStr
  qemu: implement input device hotplug
  qemu: implement input device hotunplug

 src/conf/domain_audit.c|  44 
 src/conf/domain_audit.h|   5 +
 src/conf/domain_conf.c |  37 +++
 src/conf/domain_conf.h |   3 +
 src/libvirt_private.syms   |   3 +
 src/qemu/qemu_alias.c  |  24 -
 src/qemu/qemu_alias.h  |   3 +
 src/qemu/qemu_command.c|  82 +-
 src/qemu/qemu_command.h|  12 ++-
 src/qemu/qemu_domain.c |  40 +++
 src/qemu/qemu_domain.h |   5 +
 src/qemu/qemu_domain_address.c |  45 
 src/qemu/qemu_domain_address.h |   4 +
 src/qemu/qemu_driver.c |  25 -
 src/qemu/qemu_hotplug.c| 239 +++--
 src/qemu/qemu_hotplug.h|   8 ++
 16 files changed, 436 insertions(+), 143 deletions(-)

-- 
2.13.0

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

[libvirt] [PATCH 1/2] vz: missing pieces for fd885a06 for vz driver

[Nikolay Shirokovskiy]

---
 src/vz/vz_driver.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index c339622..3864d43 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -134,7 +134,7 @@ vzBuildCapabilities(void)
 goto error;
 
 if (!(caps->host.cpu = virCPUGetHost(caps->host.arch, VIR_CPU_TYPE_HOST,
- , NULL, 0)))
+ , NULL)))
 goto error;
 
 if (virCapabilitiesAddHostMigrateTransport(caps, "vzmigr") < 0)
@@ -957,7 +957,7 @@ vzConnectBaselineCPU(virConnectPtr conn,
 if (!(cpus = virCPUDefListParse(xmlCPUs, ncpus, VIR_CPU_TYPE_HOST)))
 goto cleanup;
 
-if (!(cpu = cpuBaseline(cpus, ncpus, NULL, 0, false)))
+if (!(cpu = cpuBaseline(cpus, ncpus, NULL, false)))
 goto cleanup;
 
 if ((flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) &&
-- 
1.8.3.1

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

[libvirt] [PATCH 2/2] vz: fix typo for 0d3d020b

[Nikolay Shirokovskiy]

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

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 5f37714..52bd0fc 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -4495,7 +4495,7 @@ prlsdkGetNetStats(PRL_HANDLE sdkstats, PRL_HANDLE sdkdom, 
const char *device,
 virMacAddr mac;
 
 if (virMacAddrParse(device, ) == 0)
-net = prlsdkFindNetByMAC(sdkdom, device);
+net = prlsdkFindNetByMAC(sdkdom, );
 else
 net = prlsdkFindNetByPath(sdkdom, device);
 
-- 
1.8.3.1

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

[libvirt] [PATCH] conf: fix use of uninitialized variable

[Nikolay Shirokovskiy]

If same boot order is specified twice (or more) in domain xml
we call free for uninitiaziled loadparm on cleanup in 
virDomainDeviceBootParseXML
and SIGABRT (or similar) as a result.
---
 src/conf/domain_conf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 516f9fa..25d48f9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6188,7 +6188,7 @@ virDomainDeviceBootParseXML(xmlNodePtr node,
 virHashTablePtr bootHash)
 {
 char *order;
-char *loadparm;
+char *loadparm = NULL;
 int ret = -1;
 
 if (!(order = virXMLPropString(node, "order"))) {
-- 
1.8.3.1

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

[libvirt] [PATCH 0/2] vz: build fixes

[Nikolay Shirokovskiy]

Nikolay Shirokovskiy (2):
  vz: missing pieces for fd885a06 for vz driver
  vz: fix typo for 0d3d020b

 src/vz/vz_driver.c | 4 ++--
 src/vz/vz_sdk.c| 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
1.8.3.1

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

Re: [libvirt] [PATCH] docs: remove duplicate https links

[Daniel P. Berrange]

On Tue, Oct 17, 2017 at 09:32:43AM +0200, Ján Tomko wrote:
> Commit e371b3b changed all the links to libvirt.org to use https.
> Remove the leftover 'http' links from downloads page, since they
> point to https anyway.
> ---
> Pushed as trivial.

/me wonders why on earth I added both https & http links in
the first place !

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] [libvirt-jenkins-ci PATCH 2/5] ansible: Introduce the 'manage' tool

[Andrea Bolognani]

On Tue, 2017-10-17 at 15:21 +0200, Pavel Hrdina wrote:
> > +do_list() {
> > +INVENTORY=$(grep "^inventory\\s*=" ansible.cfg 2>/dev/null | sed 
> > "s/^.*=\\s*//g")
> > +INVENTORY=${INVENTORY:-/etc/ansible/hosts}
> 
> I don't think that there is a need to include system-wide inventory
> since we have our own inventory and we don't use the system-wide
> inventory at all.

Good point. I turned the absence of the local inventory file into
an error, and...

> > +grep -v '^\[' "$INVENTORY" | sort -u

... improved the regular expression so that empty lines and comments
will be ignored in addition to group names.

> > +case "$1" in
> > +list)   do_list ;;
> > +prepare|update) do_prepare "$2" ;;
> > +*help)  do_help ;;
> > +*)  die "Usage: $PROGRAM_NAME ACTION [OPTIONS]" ;;
> 
> How about grouping *help) and *) together?  I was pretending to be a
> basic user and I've run this script without any option at all and this
> was the only output.  So it would be nice to print the help itself
> or mention that there is some "--help" option.

I went one step further and dropped the command-specific error
message in favor of displaying the help text. One less string, and
it's actually more informative :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [libvirt-jenkins-ci PATCH 2/5] ansible: Introduce the 'manage' tool

[Pavel Hrdina]

On Mon, Oct 16, 2017 at 06:02:05PM +0200, Andrea Bolognani wrote:
> This script replaces the existing Makefile, and will be extended
> to provide more functionality in future commits.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  ansible/Makefile |  9 -
>  ansible/manage   | 57 
> 
>  2 files changed, 57 insertions(+), 9 deletions(-)
>  delete mode 100644 ansible/Makefile
>  create mode 100755 ansible/manage
> 
> diff --git a/ansible/Makefile b/ansible/Makefile
> deleted file mode 100644
> index 6af7ae3..000
> --- a/ansible/Makefile
> +++ /dev/null
> @@ -1,9 +0,0 @@
> -all:
> -
> -site:
> - @ansible-playbook site.yml
> -
> -clean:
> - @rm -f *.retry log
> -
> -.PHONY: all site clean
> diff --git a/ansible/manage b/ansible/manage
> new file mode 100755
> index 000..46bec6c
> --- /dev/null
> +++ b/ansible/manage
> @@ -0,0 +1,57 @@
> +#!/bin/sh
> +
> +PROGRAM_NAME="$0"
> +
> +# ---
> +#  Utility functions
> +# ---
> +
> +# die MESSAGE
> +#
> +# Abort the program after displaying $MESSAGE on standard error.
> +die() {
> +echo "$1" >&2
> +exit 1
> +}
> +
> +# --
> +#  User-visible actions
> +# --
> +
> +do_help() {
> +echo "\
> +Usage: $PROGRAM_NAME ACTION [OPTIONS]
> +
> +Actions:
> +  list List known guests
> +  prepare GUESTPrepare or update GUEST. Can be run as many times as 
> needed
> +  update GUEST Alias for prepare
> +  help Display this help"
> +}
> +
> +do_list() {
> +INVENTORY=$(grep "^inventory\\s*=" ansible.cfg 2>/dev/null | sed 
> "s/^.*=\\s*//g")
> +INVENTORY=${INVENTORY:-/etc/ansible/hosts}

I don't think that there is a need to include system-wide inventory
since we have our own inventory and we don't use the system-wide
inventory at all.

> +
> +grep -v '^\[' "$INVENTORY" | sort -u
> +}
> +
> +do_prepare() {
> +GUEST="$1"
> +
> +test "$GUEST" || {
> +die "Usage: $PROGRAM_NAME prepare GUEST"
> +}
> +do_list | grep -q "$GUEST" || {
> +die "$PROGRAM_NAME: $GUEST: Unknown guest"
> +}
> +
> +ansible-playbook -l "$GUEST" site.yml
> +}
> +
> +case "$1" in
> +list)   do_list ;;
> +prepare|update) do_prepare "$2" ;;
> +*help)  do_help ;;
> +*)  die "Usage: $PROGRAM_NAME ACTION [OPTIONS]" ;;

How about grouping *help) and *) together?  I was pretending to be a
basic user and I've run this script without any option at all and this
was the only output.  So it would be nice to print the help itself
or mention that there is some "--help" option.

> +esac

Pavel


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

Re: [libvirt] [libvirt-jenkins-ci PATCH 0/2] ansible: Improve service management

[Pavel Hrdina]

On Wed, Oct 11, 2017 at 10:52:16AM +0200, Andrea Bolognani wrote:
> Rely on built-in functionality instead of reinventing the wheel.
> 
> Andrea Bolognani (2):
>   ansible: Use systemd module
>   ansible: Use built-in init system detection

Reviewed-by: Pavel Hrdina 


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

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

[John Ferlan]

On 10/17/2017 03:40 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 16.10.2017 15:47, John Ferlan wrote:
>>
>>
>> On 09/27/2017 08:45 AM, Nikolay Shirokovskiy wrote:
>>> Current daemon shutdown can cause crashes. The problem is that threads
>>> serving client request are joined on daemon dispose after drivers already
>>> cleaned up. But this threads typically uses drivers and thus crashes come.
>>> We need to join threads before virStateCleanup. virNetDaemonClose is
>>> a good candidate.
>>>
>>> But it turns out that we can hang on join. The problem is that at this
>>> moment event loop is not functional and for example threads waiting for
>>> qemu response will never finish. Let's introduce extra shutdown step
>>> for drivers so that they can signal any API calls in progress to finish.
>>> ---
>>>  daemon/libvirtd.c|  2 ++
>>>  src/driver-state.h   |  4 
>>>  src/libvirt.c| 18 ++
>>>  src/libvirt_internal.h   |  1 +
>>>  src/libvirt_private.syms |  1 +
>>>  src/rpc/virnetserver.c   |  5 +++--
>>>  6 files changed, 29 insertions(+), 2 deletions(-)
>>>
>>
>> So - first off - this patch is doing 2 things:
>>
>>  1. Introduce a new driver State function - "Shutdown"
>>
>>  2. Moving virThreadPoolFree from virNetServerDispose to virNetServerClose
>>
>> The two do not seem to be related so, they'd need to be separated.
>>
>> It appears the motivation behind the StateShutdown is to "force" the
>> close the qemu monitor and agent, but it doesn't seem that's really
>> related to the virNet{Daemon|Server}* timing issue. So let's consider
>> that separately...  and I'm not considering it now...
>>
>> Focusing more on #2 - the move of the virThreadPoolFree would seem to
>> hasten the eventual free. Or more to the point ensure it happens. While
>> that does resolve the problem, I don't think it's the best or actual fix
>> to what it appears the real problem is.
> 
> The problem is related to the fact that virThreadPoolFree does 2 things.
> 
> 1. finishes all worker threads
> 2. do actual free
> 
> What I want is to hasten #1 not actual free.
> 

Right - but because your fix just calls virThreadPoolFree and sets
srv->workers = NULL - that caused me to wonder why we had to do that
when it "should" be done once we're done using the servers hash table
entry... IOW: Something should Unref sooner.

BTW: Your 1 and 2 are all the same to me - the Unref being the more key
component because we know virObjectUnref(srv) should be the "last"
reference to @srv.  It's one of those ordering things. It should be do
A, then B to allocate/start and then undo B and undo A on cleanup. In
this case we partially undo B, then undo A, and eventually complete the
undo B much later on.

>>
>> Taking it from the Start/Alloc/Ref side - @srv gets a Ref at
>> virNetServerNew and then again at virNetDaemonAddServer. So each @srv
>> has two refs, which means in order for virNetServerDispose to be called,
>> there would need to be two Unref's; however, I can only find one.
>>
>> During cleanup: @srv is Unref'd after virNetDaemonClose, but I'm not
>> finding the other one. Do you recall where you may have seen it? I'm
> 
> When the daemon object is unref'd at the end of daemon main function
> servers are unrefed as part of daemon dispose.>

OK - right, virHashFree does end up making that free call, but as you
note, much too late.

> 
>> assuming the answer is no, there wasn't one and hence why you moved that
>> virThreadPoolFree call.
> 
> Not entirely true. I moved this call so that all client request
> are finished before we clean up drivers state. Otherwise client requests
> will see disposed objects in the middele of operation and crashes occur.
> 

So while the move "works" it does so because it's bypassing the 'proper'
way to resolve this by Unref() the table elements when the decision is
made by the the code that it is done with them...

>>
>> Since at virNetDaemonAddServer we add a Ref to @srv, then during
>> virNetDaemonClose I'd expect that for each server on dmn->servers
>> there'd be the virNetServerClose and a removal from the HashTable and
>> Unref of the @srv object which I'm not seeing. If that was there, then
>> the virNetServerDispose would call virThreadPoolFree right when the
>> Unref was done on @srv. The better fix, I believe is a call to
>> virHashRemoveAll in virNetDaemonClose which does that removal of @srv
>> from the dmn->servers hash table. NB this would fix a memory leak since
>> the eventual virHashFree(dmn->servers) doesn't do free the elements of
>> the hash table when virNetDaemonDispose is called as a result of the
>> virObjectUnref(dmn) at the bottom of main() in daemon/libvirtd.c.
> 
> Servers hash table created with free function virObjectFreeHashData
> which will unref servers when hash table is freed.
> 
> As to clearing servers hash table at virNetDaemonClose. To me daemon
> has close and dispose function and looks like dispose function is
> more suitable for freeing 

Re: [libvirt] [PATCH 4/4] qemu: Fix CPU model broken by older libvirt

[Pavel Hrdina]

On Wed, Oct 11, 2017 at 12:11:17PM +0200, Jiri Denemark wrote:
> When libvirt older than 3.9.0 reconnected to a running domain started by
> old libvirt it could have messed up the expansion of host-model by
> adding features QEMU does not support (such as cmt). Thus whenever we
> reconnect to a running domain, revert to an active snapshot, or restore
> a saved domain we need to check the guest CPU model and remove the
> CPU features unknown to QEMU. We can do this because we know the domain
> was successfully started, which means the CPU did not contain the
> features when libvirt started the domain.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1495171
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_domain.c  | 76 
> +
>  src/qemu/qemu_domain.h  |  4 +++
>  src/qemu/qemu_driver.c  | 14 +
>  src/qemu/qemu_process.c |  9 ++
>  4 files changed, 103 insertions(+)

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [PATCH 3/4] qemu: Filter CPU features when using host CPU

[Pavel Hrdina]

On Tue, Oct 17, 2017 at 01:15:43PM +0200, Jiri Denemark wrote:
> On Tue, Oct 17, 2017 at 12:57:10 +0200, Pavel Hrdina wrote:
> > On Wed, Oct 11, 2017 at 12:11:16PM +0200, Jiri Denemark wrote:
> > > The host CPU definition from host capabilities may contain features
> > > unknown to QEMU. Thus whenever we want to use this CPU definition, we
> > > have to filter the unknown features.
> > 
> > Might be nice to explicitly mention in the commit message that this
> > fixes the issue while reconnecting to QEMU process started by old
> > libvirt that keeps "host-model" in the status XML.  Took me some time
> > to figure that out :).
> 
> Yeah, sorry about it. What about the following commit message?
> 
> When reconnecting to a domain started with a host-model CPU which was
> started by old libvirt that did not replace host-model with the real CPU
> definition, libvirt replaces the host-model CPU with the CPU from
> capabilities (because this is what the old libvirt did when it started
> the domain). Without this patch libvirt could use features unknown to
> QEMU in the CPU definition which replaced the original host-model CPU.
> Such domain would keep running just fine, but any attempt to migrate it
> will fail and once the domain is saved or snapshotted, restoring it
> would fail too.
> 
> In other words whenever we want to use this CPU definition, we have to
> filter the unknown features.

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [PATCH 3/4] qemu: Filter CPU features when using host CPU

[Jiri Denemark]

On Tue, Oct 17, 2017 at 12:57:10 +0200, Pavel Hrdina wrote:
> On Wed, Oct 11, 2017 at 12:11:16PM +0200, Jiri Denemark wrote:
> > The host CPU definition from host capabilities may contain features
> > unknown to QEMU. Thus whenever we want to use this CPU definition, we
> > have to filter the unknown features.
> 
> Might be nice to explicitly mention in the commit message that this
> fixes the issue while reconnecting to QEMU process started by old
> libvirt that keeps "host-model" in the status XML.  Took me some time
> to figure that out :).

Yeah, sorry about it. What about the following commit message?

When reconnecting to a domain started with a host-model CPU which was
started by old libvirt that did not replace host-model with the real CPU
definition, libvirt replaces the host-model CPU with the CPU from
capabilities (because this is what the old libvirt did when it started
the domain). Without this patch libvirt could use features unknown to
QEMU in the CPU definition which replaced the original host-model CPU.
Such domain would keep running just fine, but any attempt to migrate it
will fail and once the domain is saved or snapshotted, restoring it
would fail too.

In other words whenever we want to use this CPU definition, we have to
filter the unknown features.

Jirka

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

Re: [libvirt] [PATCH 3/4] qemu: Filter CPU features when using host CPU

[Pavel Hrdina]

On Wed, Oct 11, 2017 at 12:11:16PM +0200, Jiri Denemark wrote:
> The host CPU definition from host capabilities may contain features
> unknown to QEMU. Thus whenever we want to use this CPU definition, we
> have to filter the unknown features.

Might be nice to explicitly mention in the commit message that this
fixes the issue while reconnecting to QEMU process started by old
libvirt that keeps "host-model" in the status XML.  Took me some time
to figure that out :).

> https://bugzilla.redhat.com/show_bug.cgi?id=1495171
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_process.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [PATCH] qemu: Check QEMU error on failed migration

[Jiri Denemark]

On Mon, Oct 16, 2017 at 17:18:58 +0200, Pavel Hrdina wrote:
> On Thu, Oct 12, 2017 at 03:48:29PM +0200, Jiri Denemark wrote:
> > When migration fails, QEMU may provide a description of the error in
> > the reply to query-migrate QMP command. We can fetch this error and use
> > it instead of the generic "unexpectedly failed" message.
> > 
> > Signed-off-by: Jiri Denemark 
...
> > -if (qemuMonitorJSONGetMigrationStats(qemuMonitorTestGetMonitor(test), 
> > ) < 0)
> > +if (qemuMonitorJSONGetMigrationStats(qemuMonitorTestGetMonitor(test),
> > + , ) < 0)
> >  goto cleanup;
> >  
> > -if (memcmp(, , sizeof(stats)) != 0) {
> > +if (memcmp(, , sizeof(stats)) != 0 || error) {
> >  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > -   "Invalid migration status");
> > +   "Invalid migration statistics");
> > +goto cleanup;
> > +}
> 
> Do we need to pass the "" for the first call of
> qemuMonitorJSONGetMigrationStats() since we know the answer?

Well, this is true for all tests. This is just testing that error stays
unset if there's no error reported by QEMU.

> 
> > +
> > +memset(, 0, sizeof(stats));
> > +if (qemuMonitorJSONGetMigrationStats(qemuMonitorTestGetMonitor(test),
> > + , ) < 0)
> > +goto cleanup;
> > +
> > +if (stats.status != QEMU_MONITOR_MIGRATION_STATUS_ERROR ||
> > +STRNEQ_NULLABLE(error, "It's broken")) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +   "Invalid failed migration status");
> >  goto cleanup;
> >  }
> >  
> >  ret = 0;
> >   cleanup:
> >  qemuMonitorTestFree(test);
> > +VIR_FREE(error);
> >  return ret;
> >  }
> 
> Reviewed-by: Pavel Hrdina 

Thanks, pushed.

Jirka

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

[libvirt] [libvirt-jenkins-ci PATCH 6/5] fixup: Allow 'manage ACTION all'

[Andrea Bolognani]

When running the manage tool as part of scheduled maintainance, eg.
in a cron job, it would be annoying to have to loop over the output
of 'manage list' and call 'manage update' on each of the guests; it
would also mean not taking advantage of Ansible's parallel execution
capabilities, which can speed up things considerably.

Solve the problem by allowing 'all' to be used as a guest name.

Signed-off-by: Andrea Bolognani 
---
This will not be its own separate commit when pushing, but it will
be squashed into commit 2/5, with a small adjustment required in
commit 3/5 to deal with merge conflicts in the help text.

 guests/manage | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/guests/manage b/guests/manage
index d84b8b0..5927d62 100755
--- a/guests/manage
+++ b/guests/manage
@@ -50,11 +50,11 @@ do_help() {
 Usage: $PROGRAM_NAME ACTION [OPTIONS]
 
 Actions:
-  list List known guests
-  install GUESTInstall GUEST
-  prepare GUESTPrepare or update GUEST. Can be run as many times as needed
-  update GUEST Alias for prepare
-  help Display this help"
+  list List known guests
+  install GUESTInstall GUEST
+  prepare GUEST|allPrepare or update GUEST. Can be run multiple times
+  update  GUEST|allAlias for prepare
+  help Display this help"
 }
 
 do_list() {
@@ -113,9 +113,9 @@ do_prepare() {
 GUEST="$1"
 
 test "$GUEST" || {
-die "Usage: $PROGRAM_NAME prepare GUEST"
+die "Usage: $PROGRAM_NAME prepare GUEST|all"
 }
-do_list | grep -q "$GUEST" || {
+do_list | grep -q "$GUEST" || test "$GUEST" = all || {
 die "$PROGRAM_NAME: $GUEST: Unknown guest"
 }
 
-- 
2.13.6

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

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

[Nikolay Shirokovskiy]

On 16.10.2017 15:47, John Ferlan wrote:
> 
> 
> On 09/27/2017 08:45 AM, Nikolay Shirokovskiy wrote:
>> Current daemon shutdown can cause crashes. The problem is that threads
>> serving client request are joined on daemon dispose after drivers already
>> cleaned up. But this threads typically uses drivers and thus crashes come.
>> We need to join threads before virStateCleanup. virNetDaemonClose is
>> a good candidate.
>>
>> But it turns out that we can hang on join. The problem is that at this
>> moment event loop is not functional and for example threads waiting for
>> qemu response will never finish. Let's introduce extra shutdown step
>> for drivers so that they can signal any API calls in progress to finish.
>> ---
>>  daemon/libvirtd.c|  2 ++
>>  src/driver-state.h   |  4 
>>  src/libvirt.c| 18 ++
>>  src/libvirt_internal.h   |  1 +
>>  src/libvirt_private.syms |  1 +
>>  src/rpc/virnetserver.c   |  5 +++--
>>  6 files changed, 29 insertions(+), 2 deletions(-)
>>
> 
> So - first off - this patch is doing 2 things:
> 
>  1. Introduce a new driver State function - "Shutdown"
> 
>  2. Moving virThreadPoolFree from virNetServerDispose to virNetServerClose
> 
> The two do not seem to be related so, they'd need to be separated.
> 
> It appears the motivation behind the StateShutdown is to "force" the
> close the qemu monitor and agent, but it doesn't seem that's really
> related to the virNet{Daemon|Server}* timing issue. So let's consider
> that separately...  and I'm not considering it now...
> 
> Focusing more on #2 - the move of the virThreadPoolFree would seem to
> hasten the eventual free. Or more to the point ensure it happens. While
> that does resolve the problem, I don't think it's the best or actual fix
> to what it appears the real problem is.

The problem is related to the fact that virThreadPoolFree does 2 things.

1. finishes all worker threads
2. do actual free

What I want is to hasten #1 not actual free.

> 
> Taking it from the Start/Alloc/Ref side - @srv gets a Ref at
> virNetServerNew and then again at virNetDaemonAddServer. So each @srv
> has two refs, which means in order for virNetServerDispose to be called,
> there would need to be two Unref's; however, I can only find one.
> 
> During cleanup: @srv is Unref'd after virNetDaemonClose, but I'm not
> finding the other one. Do you recall where you may have seen it? I'm

When the daemon object is unref'd at the end of daemon main function
servers are unrefed as part of daemon dispose.


> assuming the answer is no, there wasn't one and hence why you moved that
> virThreadPoolFree call.

Not entirely true. I moved this call so that all client request
are finished before we clean up drivers state. Otherwise client requests
will see disposed objects in the middele of operation and crashes occur.

> 
> Since at virNetDaemonAddServer we add a Ref to @srv, then during
> virNetDaemonClose I'd expect that for each server on dmn->servers
> there'd be the virNetServerClose and a removal from the HashTable and
> Unref of the @srv object which I'm not seeing. If that was there, then
> the virNetServerDispose would call virThreadPoolFree right when the
> Unref was done on @srv. The better fix, I believe is a call to
> virHashRemoveAll in virNetDaemonClose which does that removal of @srv
> from the dmn->servers hash table. NB this would fix a memory leak since
> the eventual virHashFree(dmn->servers) doesn't do free the elements of
> the hash table when virNetDaemonDispose is called as a result of the
> virObjectUnref(dmn) at the bottom of main() in daemon/libvirtd.c.

Servers hash table created with free function virObjectFreeHashData
which will unref servers when hash table is freed.

As to clearing servers hash table at virNetDaemonClose. To me daemon
has close and dispose function and looks like dispose function is
more suitable for freeing servers for the sake of functional groupping.
Why we distinct close/dispose functions - I guess one can potentially
close/open daemon and reuse daemon object but there is no such usage
currently.

> 
> As an aside (and I think something else that needs to be fixed), there's
> virNetDaemonAddServerPostExec which adds a @srv to dmn->services, but
> never does the virObjectRef after the virHashAddEntry call. That would
> need to be a patch before the patch that adds the virHashRemoveAll.

Agree. By the way virtlogd and virtlockd treat differently 
virNetDaemonAddServerPostExec.
The first stores reference to server object and unref it on daemon dispose as 
if 
virNetDaemonAddServerPostExec return server with extra reference. The second 
just
drops the returned server object as if there is no extra reference. So if we 
add extra reference we need to patch virtlockd as well.

The original server reference owner is servers table I mean.

> 
> Make sense?   This is a very interesting/good catch to a problem - let's
> just get the right fix!
> 
> Tks -
> 
> John
> 
>> diff 

Re: [libvirt] [PATCH v2 2/3] hyperv: Escape WQL queries

[Ladi Prosek]

On Mon, Oct 16, 2017 at 3:58 PM, John Ferlan  wrote:
>
>
> On 10/06/2017 02:47 AM, Ladi Prosek wrote:
>> The code was vulnerable to SQL injection. Likely not a security issue due to
>> WMI SQL and other constraints but still lame. For example:
>>
>>   virsh # dominfo \"
>>   error: failed to get domain '"'
>>   error: internal error: SOAP fault during enumeration: code 's:Sender', 
>> subcode
>>   'n:CannotProcessFilter', reason 'The data source could not process the 
>> filter.
>>   The filter might be missing or it might be invalid. Change the filter and 
>> try
>>   the request again.  ', detail 'The WS-Management service cannot process the
>>   request. The WQL query is invalid. '
>>
>> This commit fixes the Hyper-V driver by escaping all WMI SQL string 
>> parameters.
>>
>> The same command with the fix:
>>
>>   virsh # dominfo \"
>>   error: failed to get domain '"'
>>   error: Domain not found: No domain with name "
>>
>> Signed-off-by: Ladi Prosek 
>> ---
>>  src/hyperv/hyperv_driver.c | 96 
>> +++---
>>  src/hyperv/hyperv_wmi.c|  2 +-
>>  src/util/virbuffer.c   | 18 +
>>  src/util/virbuffer.h   |  3 ++
>>  4 files changed, 70 insertions(+), 49 deletions(-)
>>
>
> Surprised to a degree this worked correctly without adding
> 'virBufferEscapeSQL' to src/libvirt_private.syms

Interesting, I followed instructions at
https://libvirt.org/compiling.html#building and didn't see any
warnings or indication that something was amiss.

> In any case, I'll add before pushing...

Thank you!

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

[libvirt] [PATCH] docs: remove duplicate https links

[Ján Tomko]

Commit e371b3b changed all the links to libvirt.org to use https.
Remove the leftover 'http' links from downloads page, since they
point to https anyway.
---
Pushed as trivial.

 docs/downloads.html.in | 17 -
 1 file changed, 17 deletions(-)

diff --git a/docs/downloads.html.in b/docs/downloads.html.in
index 640b51480..6657e21d0 100644
--- a/docs/downloads.html.in
+++ b/docs/downloads.html.in
@@ -28,7 +28,6 @@
   libvirt
   
 ftp://libvirt.org/libvirt/;>ftp
-https://libvirt.org/sources/;>http
 https://libvirt.org/sources/;>https
   
   
@@ -50,7 +49,6 @@
   C#
   
 ftp://libvirt.org/libvirt/csharp/;>ftp
-https://libvirt.org/sources/csharp/;>http
 https://libvirt.org/sources/csharp/;>https
   
   
@@ -66,7 +64,6 @@
   Go
   
 ftp://libvirt.org/libvirt/go/;>ftp
-https://libvirt.org/sources/go/;>http
 https://libvirt.org/sources/go/;>https
   
   
@@ -84,7 +81,6 @@
   Java
   
 ftp://libvirt.org/libvirt/java/;>ftp
-https://libvirt.org/sources/java/;>http
 https://libvirt.org/sources/java/;>https
   
   
@@ -100,7 +96,6 @@
   OCaml
   
 ftp://libvirt.org/libvirt/ocaml/;>ftp
-https://libvirt.org/sources/ocaml/;>http
 https://libvirt.org/sources/ocaml/;>https
   
   
@@ -133,7 +128,6 @@
   PHP
   
 ftp://libvirt.org/libvirt/php/;>ftp
-https://libvirt.org/sources/php/;>http
 https://libvirt.org/sources/php/;>https
   
   
@@ -149,7 +143,6 @@
   Python
   
 ftp://libvirt.org/libvirt/python/;>ftp
-https://libvirt.org/sources/python/;>http
 https://libvirt.org/sources/python/;>https
 https://pypi.python.org/pypi/libvirt-python;>pypi
   
@@ -166,7 +159,6 @@
   Ruby
   
 ftp://libvirt.org/libvirt/ruby/;>ftp
-https://libvirt.org/sources/ruby/;>http
 https://libvirt.org/sources/ruby/;>https
   
   
@@ -182,7 +174,6 @@
   Rust
   
 ftp://libvirt.org/libvirt/rust/;>ftp
-https://libvirt.org/sources/rust/;>http
 https://libvirt.org/sources/rust/;>https
   
   
@@ -201,7 +192,6 @@
   GLib / GConfig / GObject
   
 ftp://libvirt.org/libvirt/glib/;>ftp
-https://libvirt.org/sources/glib/;>http
 https://libvirt.org/sources/glib/;>https
   
   
@@ -217,7 +207,6 @@
   Go XML
   
 ftp://libvirt.org/libvirt/go/;>ftp
-https://libvirt.org/sources/go/;>http
 https://libvirt.org/sources/go/;>https
   
   
@@ -235,7 +224,6 @@
   Console Proxy
   
 ftp://libvirt.org/libvirt/consoleproxy/;>ftp
-https://libvirt.org/sources/consoleproxy/;>http
 https://libvirt.org/sources/consoleproxy/;>https
   
   
@@ -251,7 +239,6 @@
   CIM provider
   
 ftp://libvirt.org/libvirt/CIM/;>ftp
-https://libvirt.org/sources/CIM/;>http
 https://libvirt.org/sources/CIM/;>https
   
   
@@ -267,7 +254,6 @@
   CIM utils
   
 ftp://libvirt.org/libvirt/CIM/;>ftp
-https://libvirt.org/sources/CIM/;>http
 https://libvirt.org/sources/CIM/;>https
   
   
@@ -283,7 +269,6 @@
   SNMP
   
 ftp://libvirt.org/libvirt/snmp/;>ftp
-https://libvirt.org/sources/snmp/;>http
 https://libvirt.org/sources/snmp/;>https
   
   
@@ -299,7 +284,6 @@
   Application Sandbox
   
 ftp://libvirt.org/libvirt/sandbox/;>ftp
-https://libvirt.org/sources/sandbox/;>http
 https://libvirt.org/sources/sandbox/;>https
   
   
@@ -318,7 +302,6 @@
   TCK
   
 ftp://libvirt.org/libvirt/tck/;>ftp
-https://libvirt.org/sources/tck/;>http
 https://libvirt.org/sources/tck/;>https
   
   
-- 
2.13.0

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

Re: [libvirt] [PATCH v2 1/3] hyperv: Fix hypervInitConnection error reporting

[Ladi Prosek]

On Mon, Oct 16, 2017 at 3:58 PM, John Ferlan  wrote:
>
>
> On 10/06/2017 02:47 AM, Ladi Prosek wrote:
>> "%s is not a Hyper-V server" is not a correct generalization of all possible
>> error conditions of hypervEnumAndPull. For example:
>>
>>   $ virsh --connect hyperv://localhost/?transport=http
>>   Enter username for localhost [administrator]:
>>   Enter administrator's password for localhost: 
>>   error: failed to connect to the hypervisor
>>   error: internal error: localhost is not a Hyper-V server
>>
>> This commit removes the general virReportError from hypervInitConnection and
>> also the "Invalid query" virReportError from hypervSerializeEprParam, which
>> does not correctly describe the error either (virBufferCheckError has
>> already set a meaningful error message at that point).
>>
>> The same scenario with the fix:
>>
>>   $ virsh --connect hyperv://localhost/?transport=http
>>   Enter username for localhost [administrator]:
>>   Enter administrator's password for localhost: 
>>   error: failed to connect to the hypervisor
>>   error: internal error: Transport error during enumeration: User, password 
>> or
>>   similar was not accepted (26)
>>
>> Signed-off-by: Ladi Prosek 
>> ---
>>  src/hyperv/hyperv_driver.c | 2 --
>>  src/hyperv/hyperv_wmi.c| 1 -
>>  2 files changed, 3 deletions(-)
>>
>
> Seems reasonable -
>> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
>> index 09912610c..52274239c 100644
>> --- a/src/hyperv/hyperv_driver.c
>> +++ b/src/hyperv/hyperv_driver.c
>> @@ -105,8 +105,6 @@ hypervInitConnection(virConnectPtr conn, hypervPrivate 
>> *priv,
>>  priv->wmiVersion = HYPERV_WMI_VERSION_V1;
>>
>>  if (hypervEnumAndPull(priv, , ) < 0) {
>> -virReportError(VIR_ERR_INTERNAL_ERROR,
>> -   _("%s is not a Hyper-V server"), 
>> conn->uri->server);
>>  goto cleanup;
>>  }
>
> Before posting be sure to "make check syntax-check" which would have
> told you that since there's only one line (cleanup;) in the condition
> that the { } are not necessary.

Will do, sorry about that.

> I've removed them locally before pushing...

Thank you!

> Reviewed-by: John Ferlan 
>
>
> John
>>  }
>> diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c
>> index 980a00e28..0b9431bfa 100644
>> --- a/src/hyperv/hyperv_wmi.c
>> +++ b/src/hyperv/hyperv_wmi.c
>> @@ -514,7 +514,6 @@ hypervSerializeEprParam(hypervParamPtr p, hypervPrivate 
>> *priv,
>>  /* Get query and create filter based on it */
>>  if (virBufferCheckError(p->epr.query) < 0) {
>>  virBufferFreeAndReset(p->epr.query);
>> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid query"));
>>  goto cleanup;
>>  }
>>  query_string = virBufferContentAndReset(p->epr.query);
>>

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

Re: [libvirt] [PATCH 1/4] virt-aa-helper: fix paths for usb hostdevs

[Christian Ehrhardt]

On Fri, Sep 29, 2017 at 4:58 PM, Michal Privoznik 
wrote:

> On 09/20/2017 04:59 PM, Christian Ehrhardt wrote:
> > If users only specified vendor (the common case) then parsing
> > the xml via virDomainHostdevSubsysUSBDefParseXML would only set these.
> > Bus and Device would much later be added when the devices are prepared
> > to be added.
> >
> > Due to that a hot-add of a usb hostdev works as the device is prepared
> > and virt-aa-helper processes the new internal xml. But on an initial
> > guest start at the time virt-aa-helper renders the apparmor rules the
> > bus/device id's are not set yet:
> >
> > p ctl->def->hostdevs[0]->source.subsys.u.usb
> > $12 = {autoAddress = false, bus = 0, device = 0, vendor = 1921, product
> > = 21888}
> >
> > That causes rules to be wrong:
> >   "/dev/bus/usb/000/000" rw,
> >
> > The fix calls virHostdevFindUSBDevice after reading the XML from
> > irt-aa-helper to only add apparmor rules for devices that could be found
> > and now are fully known to be able to write the rule correctly.
> >
> > It uncondtionally sets virHostdevFindUSBDevice mandatory attribute as
> > adding an apparmor rule for a device not found makes no sense no matter
> > what startup policy it has set.
> >
> > Signed-off-by: Christian Ehrhardt 
> > ---
> >  src/security/virt-aa-helper.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/src/security/virt-aa-helper.c
> b/src/security/virt-aa-helper.c
> > index 7944dc1..d1518ea 100644
> > --- a/src/security/virt-aa-helper.c
> > +++ b/src/security/virt-aa-helper.c
> > @@ -55,6 +55,7 @@
> >  #include "virrandom.h"
> >  #include "virstring.h"
> >  #include "virgettext.h"
> > +#include "virhostdev.h"
> >
> >  #include "storage/storage_source.h"
> >
> > @@ -1069,6 +1070,9 @@ get_files(vahControl * ctl)
> >  if (usb == NULL)
> >  continue;
> >
> > +if (virHostdevFindUSBDevice(dev, true, ) < 0)
> > +continue;
> > +
>
> Shouldn't we rather fail in this case? Or, what happens if startupPolicy
> of the device is set to 'optional'? I think we need to error out here
> (although, we've probably errored out earlier in the process).
>

Hi,
sorry for the late reply, but I was finally getting some time off for a few
days.
I intentionally decided not to error out to avoid a new "source" of issues.
Compare the two options we have:
1. continue if not finding the device
  1.1 likely case we found it, rule will be correct - good
  1.2 we don't find it (for whatever reason) - we are "as bad" as before
this fix, but not worse
2. error out if not finding the device
  2.1 likely case we found it, rule will be correct - good
  2.2 we don't find it (for whatever reason) - we are now failing completely

What I don't like about 2.2 is that there might be cases things would have
been kind of ok, depsite whatever dark usb magic hit some special setup.
In those cases if we error out we add a new chance to fail.
And as there are often too many unknowns, so I chose the safer option.


> ACK to the rest of the patches (after some typo clean up, esp. in the
> commit messages).
>

Thanks,
do you want me to clean up commit messages or will somebody do (to his
preferred style) on accepting the commit?


> Michal
>



-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list