Re: [libvirt] [PATCH hooks 1/1] Add check for Signed-off-by in commit messages

[Erik Skultety]

On Mon, Jan 22, 2018 at 08:58:52AM -0500, John Ferlan wrote:
>
>
> On 01/22/2018 07:05 AM, Daniel P. Berrange wrote:
> > This extends the update hook so that it enforces a requirement to have a
> > Signed-off-by line in every commit message. This can be optionally
> > turned off in individual repos by setting the "hooks.allowmissingsob"
> > git config variable.
> >
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  update | 16 +++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >

I've been using it since there was the first mention of it on the list and
because it didn't add any burden to my daily work and couldn't honestly be
simpler to configure, I don't have any objections to this move whatsoever.

>
> Count me in as in favor of this.  I have only ever had to add:
>
> [format]
>   signoff = true
>
> to my $HOME/.gitconfig once and it's been in place ever since.  This is
> far less than the:
>
> [user] and [sendemail] sections and the same amount of additions to/for
> the [push] section.  One extra line.
>
> It's also less than anything I've had to add to my repository specific
> .git/config files.
>
> It's also easier than the :
>
> Reviewed-by: John Ferlan 
>
> line that I keep in a "cheats" file that I always have open so that I
> don't have to add it for patches I review.  Of course, I could also just
> say ACK, but the R-b seems so much more authoritative.  Now if I could
> only remember or figure out a way to add it to any patches I push
> without having to remember to go back and rebase --interactive to add it

You're looking for git interpret-trailers...Ironically, I haven't been using it
even though I've known about it for quite some while, oh sigh, now's the right
time to do it.

Erik

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

Re: [libvirt] [PATCH 0/9] Resolve libvirtd hang on termination with connected long running client

[Nikolay Shirokovskiy]

Hi, John.

Thank you for driving this issue! Unfortunately I have little time now
to take participation in reviweing etc.

Nikolay

On 19.01.2018 20:23, John Ferlan wrote:
> RFC:
> https://www.redhat.com/archives/libvir-list/2018-January/msg00318.html
> 
> Adjustments since RFC...
> 
> Patches 1&2: No change, were already R-B'd
> Patch 3: Removed code as noted in code review, update commit message
> Patch 4: From old series removed, see below for more details
> Patch 9: no change
> NB: Patches 5-8 and 10 from Nikolay Shirokovskiy 
> are removed as they seemed to not be necessary
> 
> Replaced the former patch 4 with series of patches to (slowly) provide
> support to disable new connections, handle removing waiting jobs, causing
> the waiting workers to quit, and allow any running jobs to complete.
> 
> As it turns out, waiting for running jobs to complete cannot be done
> from the virNetServerClose callbacks because that means the event loop
> processing done during virNetServerRun will not allow any currently
> long running worker job thread a means to complete.
> 
> So when virNetDaemonQuit is called as a result of the libvirtd signal
> handlers for SIG{QUIT|INT|TERM}, instead of just causing virNetServerRun
> to quit immediately, alter to using a quitRequested flag and then use
> that quitRequested flag to check for long running worker threads before
> causing the event loop to quit resulting in libvirtd being able to run
> through the virNetDaemonClose processing.
> 
> John Ferlan (9):
>   libvirtd: Alter refcnt processing for domain server objects
>   libvirtd: Alter refcnt processing for server program objects
>   netserver: Remove ServiceToggle during ServerDispose
>   util: Introduce virThreadPoolDrain
>   rpc: Introduce virNetServerQuitRequested
>   rpc: Introduce virNetServerWorkerCount
>   rpc: Alter virNetDaemonQuit processing
>   docs: Add news article for libvirtd issue
>   APPLY ONLY FOR TESTING PURPOSES
> 
>  daemon/libvirtd.c| 43 +++-
>  docs/news.xml| 12 +
>  src/libvirt_private.syms |  1 +
>  src/libvirt_remote.syms  |  2 ++
>  src/qemu/qemu_driver.c   |  5 
>  src/rpc/virnetdaemon.c   | 45 +-
>  src/rpc/virnetserver.c   | 52 ---
>  src/rpc/virnetserver.h   |  4 +++
>  src/util/virthreadpool.c | 64 
> 
>  src/util/virthreadpool.h |  2 ++
>  10 files changed, 204 insertions(+), 26 deletions(-)
> 

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

[libvirt] [PATCH v3 1/5] qemu: Add some more details for hotplug errors when device not found

[Chen Hanxiao]

From: Chen Hanxiao 

More proper/detail error messages updated.

Signed-off-by: Chen Hanxiao 
---
v3:
  subject changed.
  some description of logs changed.

 src/libvirt_private.syms |  2 ++
 src/qemu/qemu_hotplug.c  | 35 +++
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index bc8cc1fba..70a91e5fd 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -203,6 +203,7 @@ virDomainChrConsoleTargetTypeToString;
 virDomainChrDefForeach;
 virDomainChrDefFree;
 virDomainChrDefNew;
+virDomainChrDeviceTypeToString;
 virDomainChrEquals;
 virDomainChrFind;
 virDomainChrGetDomainPtrs;
@@ -427,6 +428,7 @@ virDomainMemoryDefFree;
 virDomainMemoryFindByDef;
 virDomainMemoryFindInactiveByDef;
 virDomainMemoryInsert;
+virDomainMemoryModelTypeToString;
 virDomainMemoryRemove;
 virDomainMemorySourceTypeFromString;
 virDomainMemorySourceTypeToString;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 6b245bd6a..1f67ab74a 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3514,8 +3514,9 @@ qemuDomainChangeGraphics(virQEMUDriverPtr driver,
 int ret = -1;
 
 if (!olddev) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("cannot find existing graphics device to modify"));
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("cannot find existing graphics device to modify of "
+ "type '%s'"), type);
 goto cleanup;
 }
 
@@ -5101,8 +5102,10 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
 qemuDomainObjPrivatePtr priv = vm->privateData;
 
 if ((idx = virDomainShmemDefFind(vm->def, dev)) < 0) {
-virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-   _("device not present in domain configuration"));
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _("model '%s' shmem device not present "
+ "in domain configuration"),
+   virDomainShmemModelTypeToString(dev->model));
 return -1;
 }
 
@@ -5158,8 +5161,10 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
   watchdog->model == dev->model &&
   watchdog->action == dev->action &&
   virDomainDeviceInfoAddressIsEqual(>info, >info))) {
-virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-   _("watchdog device not present in domain 
configuration"));
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _("model '%s' watchdog device not present "
+ "in domain configuration"),
+   virDomainWatchdogModelTypeToString(watchdog->model));
 return -1;
 }
 
@@ -5429,8 +5434,10 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
 char *devstr = NULL;
 
 if (!(tmpChr = virDomainChrFind(vmdef, chr))) {
-virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-   _("device not present in domain configuration"));
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _("chr type '%s' device not present "
+ "in domain configuration"),
+   virDomainChrDeviceTypeToString(chr->deviceType));
 goto cleanup;
 }
 
@@ -5476,8 +5483,10 @@ qemuDomainDetachRNGDevice(virQEMUDriverPtr driver,
 int ret = -1;
 
 if ((idx = virDomainRNGFind(vm->def, rng)) < 0) {
-virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-   _("device not present in domain configuration"));
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _("model '%s' RNG device not present "
+ "in domain configuration"),
+   virDomainRNGBackendTypeToString(rng->model));
 return -1;
 }
 
@@ -5519,8 +5528,10 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver,
 qemuDomainMemoryDeviceAlignSize(vm->def, memdef);
 
 if ((idx = virDomainMemoryFindByDef(vm->def, memdef)) < 0) {
-virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-   _("device not present in domain configuration"));
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _("model '%s' memory device not present "
+ "in the domain configuration"),
+   virDomainMemoryModelTypeToString(memdef->model));
 return -1;
 }
 
-- 
2.14.3

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

[libvirt] [PATCH v3 4/5] qemu: Use VIR_ERR_DEVICE_MISSING for various DetachDeviceConfig messages

[Chen Hanxiao]

From: Chen Hanxiao 

Modify OPERATION_FAILED error codes to use DEVICE_MISSING instead.

Signed-off-by: Chen Hanxiao 
---
 src/qemu/qemu_driver.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a203c9297..10eebd61c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8172,7 +8172,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
 case VIR_DOMAIN_DEVICE_HOSTDEV: {
 hostdev = dev->data.hostdev;
 if ((idx = virDomainHostdevFind(vmdef, hostdev, _hostdev)) < 0) {
-virReportError(VIR_ERR_INVALID_ARG, "%s",
+virReportError(VIR_ERR_DEVICE_MISSING, "%s",
_("device not present in domain configuration"));
 return -1;
 }
@@ -8184,7 +8184,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
 case VIR_DOMAIN_DEVICE_LEASE:
 lease = dev->data.lease;
 if (!(det_lease = virDomainLeaseRemove(vmdef, lease))) {
-virReportError(VIR_ERR_INVALID_ARG,
+virReportError(VIR_ERR_DEVICE_MISSING,
_("Lease %s in lockspace %s does not exist"),
lease->key, NULLSTR(lease->lockspace));
 return -1;
@@ -8196,7 +8196,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
 cont = dev->data.controller;
 if ((idx = virDomainControllerFind(vmdef, cont->type,
cont->idx)) < 0) {
-virReportError(VIR_ERR_INVALID_ARG, "%s",
+virReportError(VIR_ERR_DEVICE_MISSING, "%s",
_("device not present in domain configuration"));
 return -1;
 }
@@ -8218,7 +8218,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
 fs = dev->data.fs;
 idx = virDomainFSIndexByName(vmdef, fs->dst);
 if (idx < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+virReportError(VIR_ERR_DEVICE_MISSING, "%s",
_("no matching filesystem device was found"));
 return -1;
 }
@@ -8229,7 +8229,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
 
 case VIR_DOMAIN_DEVICE_RNG:
 if ((idx = virDomainRNGFind(vmdef, dev->data.rng)) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+virReportError(VIR_ERR_DEVICE_MISSING, "%s",
_("no matching RNG device was found"));
 return -1;
 }
@@ -8240,7 +8240,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
 case VIR_DOMAIN_DEVICE_MEMORY:
 if ((idx = virDomainMemoryFindInactiveByDef(vmdef,
 dev->data.memory)) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+virReportError(VIR_ERR_DEVICE_MISSING, "%s",
_("matching memory device was not found"));
 return -1;
 }
@@ -8252,7 +8252,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
 case VIR_DOMAIN_DEVICE_REDIRDEV:
 if ((idx = virDomainRedirdevDefFind(vmdef,
 dev->data.redirdev)) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+virReportError(VIR_ERR_DEVICE_MISSING, "%s",
_("no matching redirdev was not found"));
 return -1;
 }
@@ -8262,7 +8262,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
 
 case VIR_DOMAIN_DEVICE_SHMEM:
 if ((idx = virDomainShmemDefFind(vmdef, dev->data.shmem)) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+virReportError(VIR_ERR_DEVICE_MISSING, "%s",
_("matching shmem device was not found"));
 return -1;
 }
@@ -8273,7 +8273,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
 
 case VIR_DOMAIN_DEVICE_WATCHDOG:
 if (!vmdef->watchdog) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+virReportError(VIR_ERR_DEVICE_MISSING, "%s",
_("domain has no watchdog"));
 return -1;
 }
@@ -8283,7 +8283,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
 
 case VIR_DOMAIN_DEVICE_INPUT:
 if ((idx = virDomainInputDefFind(vmdef, dev->data.input)) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+virReportError(VIR_ERR_DEVICE_MISSING, "%s",
_("matching input device not found"));
 return -1;
 }
-- 
2.14.3

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

[libvirt] [PATCH v3 2/5] qemu: Introduce VIR_ERR_DEVICE_MISSING

[Chen Hanxiao]

From: Chen Hanxiao 

Add new error code to be able to allow consumers (such as Nova) to be
able to key of a specific error code rather than needing to search the
error message."

Signed-off-by: Chen Hanxiao 
---
v3:
  commit message updated

 include/libvirt/virterror.h | 1 +
 src/util/virerror.c | 6 ++
 2 files changed, 7 insertions(+)

diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
index 91ba29784..3e7c7a02c 100644
--- a/include/libvirt/virterror.h
+++ b/include/libvirt/virterror.h
@@ -320,6 +320,7 @@ typedef enum {
 VIR_ERR_AGENT_UNSYNCED = 97,/* guest agent replies with wrong id
to guest-sync command (DEPRECATED)*/
 VIR_ERR_LIBSSH = 98,/* error in libssh transport driver */
+VIR_ERR_DEVICE_MISSING = 99,/* fail to find the desired device */
 } virErrorNumber;
 
 /**
diff --git a/src/util/virerror.c b/src/util/virerror.c
index 562c3bc61..c000b0043 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -1453,6 +1453,12 @@ virErrorMsg(virErrorNumber error, const char *info)
 else
 errmsg = _("libssh transport error: %s");
 break;
+case VIR_ERR_DEVICE_MISSING:
+if (info == NULL)
+errmsg = _("device not found");
+else
+errmsg = _("device not found: %s");
+break;
 }
 return errmsg;
 }
-- 
2.14.3

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

[libvirt] [PATCH v3 0/5] Introduce VIR_ERR_DEVICE_MISSING

[Chen Hanxiao]

Use Introduce VIR_ERR_DEVICE_MISSING for hotplug and detach device
error message.

Chen Hanxiao (5):
  qemu: Add some more details for hotplug errors when device not found
  qemu: Introduce VIR_ERR_DEVICE_MISSING
  qemu: Use VIR_ERR_DEVICE_MISSING for various hotplug messages
  qemu: Use VIR_ERR_DEVICE_MISSING for various DetachDeviceConfig
messages
  news: Add VIR_ERR_DEVICE_MISSING change as improvements

 docs/news.xml   |  5 +
 include/libvirt/virterror.h |  1 +
 src/conf/domain_conf.c  |  8 
 src/libvirt_private.syms|  2 ++
 src/qemu/qemu_driver.c  | 20 +--
 src/qemu/qemu_hotplug.c | 47 -
 src/util/virerror.c |  6 ++
 7 files changed, 57 insertions(+), 32 deletions(-)

-- 
2.14.3

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

[libvirt] [PATCH v3 3/5] qemu: Use VIR_ERR_DEVICE_MISSING for various hotplug messages

[Chen Hanxiao]

From: Chen Hanxiao 

Modify OPERATION_FAILED error codes to use DEVICE_MISSING instead.

Signed-off-by: Chen Hanxiao 
---
v3:
  modify virDomainNetFindIdx to use VIR_ERR_DEVICE_MISSING

 src/conf/domain_conf.c  |  8 
 src/qemu/qemu_hotplug.c | 24 
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a1c25060f..b81aeed59 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16228,7 +16228,7 @@ virDomainNetFindIdx(virDomainDefPtr def, 
virDomainNetDefPtr net)
 
 if (matchidx < 0) {
 if (MACAddrSpecified && PCIAddrSpecified) {
-virReportError(VIR_ERR_OPERATION_FAILED,
+virReportError(VIR_ERR_DEVICE_MISSING,
_("no device matching MAC address %s found on "
  "%.4x:%.2x:%.2x.%.1x"),
virMacAddrFormat(>mac, mac),
@@ -16237,18 +16237,18 @@ virDomainNetFindIdx(virDomainDefPtr def, 
virDomainNetDefPtr net)
net->info.addr.pci.slot,
net->info.addr.pci.function);
 } else if (PCIAddrSpecified) {
-virReportError(VIR_ERR_OPERATION_FAILED,
+virReportError(VIR_ERR_DEVICE_MISSING,
_("no device found on %.4x:%.2x:%.2x.%.1x"),
net->info.addr.pci.domain,
net->info.addr.pci.bus,
net->info.addr.pci.slot,
net->info.addr.pci.function);
 } else if (MACAddrSpecified) {
-virReportError(VIR_ERR_OPERATION_FAILED,
+virReportError(VIR_ERR_DEVICE_MISSING,
_("no device matching MAC address %s found"),
virMacAddrFormat(>mac, mac));
 } else {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+virReportError(VIR_ERR_DEVICE_MISSING, "%s",
_("no matching device found"));
 }
 }
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 1f67ab74a..12198ec8f 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3514,7 +3514,7 @@ qemuDomainChangeGraphics(virQEMUDriverPtr driver,
 int ret = -1;
 
 if (!olddev) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
+virReportError(VIR_ERR_DEVICE_MISSING,
_("cannot find existing graphics device to modify of "
  "type '%s'"), type);
 goto cleanup;
@@ -4809,7 +4809,7 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr 
driver,
 if ((idx = virDomainControllerFind(vm->def,
dev->data.controller->type,
dev->data.controller->idx)) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED,
+virReportError(VIR_ERR_DEVICE_MISSING,
_("controller %s:%d not found"),

virDomainControllerTypeToString(dev->data.controller->type),
dev->data.controller->idx);
@@ -5038,18 +5038,18 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
 if (idx < 0) {
 switch (subsys->type) {
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
-virReportError(VIR_ERR_OPERATION_FAILED,
+virReportError(VIR_ERR_DEVICE_MISSING,
_("host pci device %.4x:%.2x:%.2x.%.1x not found"),
pcisrc->addr.domain, pcisrc->addr.bus,
pcisrc->addr.slot, pcisrc->addr.function);
 break;
 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
 if (usbsrc->bus && usbsrc->device) {
-virReportError(VIR_ERR_OPERATION_FAILED,
+virReportError(VIR_ERR_DEVICE_MISSING,
_("host usb device %03d.%03d not found"),
usbsrc->bus, usbsrc->device);
 } else {
-virReportError(VIR_ERR_OPERATION_FAILED,
+virReportError(VIR_ERR_DEVICE_MISSING,
_("host usb device vendor=0x%.4x product=0x%.4x 
not found"),
usbsrc->vendor, usbsrc->product);
 }
@@ -5058,13 +5058,13 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
 if (scsisrc->protocol ==
 VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
 virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = 
>u.iscsi;
-virReportError(VIR_ERR_OPERATION_FAILED,
+virReportError(VIR_ERR_DEVICE_MISSING,
_("host scsi iSCSI path %s not found"),
iscsisrc->src->path);
 } else {
  virDomainHostdevSubsysSCSIHostPtr scsihostsrc =
   

[libvirt] [PATCH v3 5/5] news: Add VIR_ERR_DEVICE_MISSING change as improvements

[Chen Hanxiao]

From: Chen Hanxiao 

Signed-off-by: Chen Hanxiao 
---
 docs/news.xml | 5 +
 1 file changed, 5 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index b4d980624..5798f42d8 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -37,6 +37,11 @@
 
 
 
+  
+
+  qemu: Use VIR_ERR_DEVICE_MISSING for various hotplug/detach messages.
+
+  
 
 
 
-- 
2.14.3

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

Re: [libvirt] [PATCH v2] virsh: Checking the volume capacity before uploading a new file.

[Julio Faracco]

Hi Peter,

I agree about "length" parameter.
What I didn't understand is this error (check my test case):

# virsh vol-create-as --pool disk-loop loop0 --capacity 20M

# virsh vol-info loop0 disk-loop
Name:   loop0
Type:   file
Capacity:   20.00 MiB
Allocation: 20.00 MiB

# dd if=/dev/zero of=/tmp/test bs=10M count=1

# ls -l /tmp/test
-rw-rw-r-- 1 julio julio 10485760 Jan 23 00:31 /tmp/test

Now, let's upload 5M (partial):

# virsh vol-upload loop0 /tmp/test --pool disk-loop --length 5242880
error: cannot send data to volume loop0
error: Library function returned error but did not set virError

But, if I set a length with 15M, no errors occurred:

# virsh vol-upload loop0 /tmp/test --pool disk-loop --length 15728640

2018-01-22 12:44 GMT-02:00 Peter Krempa :
> On Sun, Jan 21, 2018 at 21:51:05 -0200, Julio Faracco wrote:
>> The current command 'vol-upload' is not checking if the volume accepts
>> a file bigger than its capacity. It can cause an interrupt of the
>> upload stream. This commit adds a check that fails before starting to
>> send new file to volume if the file is bigger.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1529059
>>
>> Signed-off-by: Julio Faracco 
>> ---
>>  tools/virsh-volume.c | 25 +
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
>> index 8265a39..04e480c 100644
>> --- a/tools/virsh-volume.c
>> +++ b/tools/virsh-volume.c
>> @@ -672,6 +672,7 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd)
>>  {
>>  const char *file = NULL;
>>  virStorageVolPtr vol = NULL;
>> +virStorageVolInfo volumeInfo;
>>  bool ret = false;
>>  int fd = -1;
>>  virStreamPtr st = NULL;
>> @@ -679,6 +680,7 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd)
>>  unsigned long long offset = 0, length = 0;
>>  virshControlPtr priv = ctl->privData;
>>  unsigned int flags = 0;
>> +off_t fileLen = -1;
>>  virshStreamCallbackData cbData;
>>
>>  if (vshCommandOptULongLong(ctl, cmd, "offset", ) < 0)
>> @@ -701,6 +703,29 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd)
>>  cbData.ctl = ctl;
>>  cbData.fd = fd;
>>
>> +if (virStorageVolGetInfo(vol, ) < 0)
>> +goto cleanup;
>> +
>> +if ((fileLen = virFileLength(file, fd)) < 0) {
>> +vshError(ctl, _("cannot get the file %s length"), file);
>
> This is wrong since you might want to upload contents of a block device,
> in which case you'd report this error.
>
>> +goto cleanup;
>> +}
>> +
>> +if (length < fileLen) {
>
> I think this is wrong. The 'length' parameter was designed to allow
> upload of a partial file. Adding this would break it.
>
>> +vshError(ctl, _("length parameter is smaller than file size"));
>> +goto cleanup;
>> +}

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

Re: [libvirt] a libvirt event question

[Chen, Xiaogang]

Hello:

We found the causes for this issue:  QEMU does not connect the socket for 
sending event until enters into its main_loop if libvirt uses "nowait" to 
create the chardev for monitor; And qemu cannot send event until it receives 
the first qmp cmd: qmp_capabilities. So We cannot send event from qemu before 
these conditions met.

To overcome these restrictions I think we either:

1: Change qemu implementation to send the event after receives qmp_capabilities 
cmd and socket connection is setup. This will require some major changes on 
qemu.

Or:

2: Implement a new qmp cmd that libvirt will use to query sev_measurement from 
qemu after issues qmp_capabilities cmd, but before resume VM. Sev_measurement 
value will be sent back from qemu as response for this new qmp cmd.


We would like to know what do you think about these two methods? Which method 
do you prefer? Thank you.

Regards
Xiaogang

From: Chen, Xiaogang
Sent: Thursday, January 18, 2018 2:01 PM
To: 'libvir-list@redhat.com' 
Cc: Singh, Brijesh ; Lendacky, Thomas 

Subject: a libvirt event question

Hello:


We are adding libvirt functions to launch SEV VM. When start VM I put it at 
"pause" state. During that time qemu sends an event "sev_measurement". We hope 
libvirtd  get this event before we resume VM. But I did not see libvirtd 
receive it. If I send this event at  qemu during resume time libvirtd can get 
this event successfully. I traced qemuMonitorJSONIOProcessEvent function for 
events received, did not see any event coming before resume VM.



Is there something I missed to get this event during pause state? Thank you for 
your help.



Regards

Xiaogang

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

Re: [libvirt] [PATCH 9/8] docs: Mention bash completion feature

[John Ferlan]

On 01/12/2018 10:30 AM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  docs/news.xml | 11 +++
>  1 file changed, 11 insertions(+)
> 

I think you were planning to change this one anyway considering 4.0.0
got some of this added already.

John

> diff --git a/docs/news.xml b/docs/news.xml
> index 064b9ae83..e5ed89504 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -47,6 +47,17 @@
>qemu: Add support for hot unplugging redirdev device
>  
>
> +  
> +
> +  virsh: Enhance bash completion
> +
> +
> +  New bash completion script is introduced to enable completion even
> +  for non-interactive virsh. At the same time, virsh offers 
> completion
> +  of some basic libvirt objects like domains, networks, storage 
> pools,
> +  etc. to virsh commands accepting them.
> +
> +  
>  
>  
>  
> 

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

Re: [libvirt] [PATCH 8/8] virsh: Introduce virshSnapshotNameCompleter

[John Ferlan]

On 01/12/2018 09:37 AM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  tools/virsh-completer.c | 51 
> +
>  tools/virsh-completer.h |  4 
>  tools/virsh-snapshot.c  | 21 +---
>  3 files changed, 69 insertions(+), 7 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 7/8] virsh: Introduce virshSecretUUIDCompleter

[John Ferlan]

On 01/12/2018 09:37 AM, Michal Privoznik wrote:
> This is a slight change from previous patches since virSecret
> does not have a name only UUID strings.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  tools/virsh-completer.c | 46 ++
>  tools/virsh-completer.h |  4 
>  tools/virsh-secret.c| 15 ++-
>  3 files changed, 60 insertions(+), 5 deletions(-)
> 

Reviewed-by: John Ferlan 

John


[...]

>  };
> @@ -667,7 +671,8 @@ static const vshCmdInfo info_secret_event[] = {
>  static const vshCmdOptDef opts_secret_event[] = {
>  {.name = "secret",
>   .type = VSH_OT_STRING,
> - .help = N_("filter by secret name or uuid")
> + .help = N_("filter by secret name or uuid"),

Wonder what a secret name is? ;-)  I know separate issue...  This and
the virsh.pod should be adjusted.

> + .completer = virshSecretUUIDCompleter,
>  },
>  {.name = "event",
>   .type = VSH_OT_STRING,
> 

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

Re: [libvirt] [PATCH 6/8] virsh: Introduce virshNWFilterNameCompleter

[John Ferlan]

On 01/12/2018 09:37 AM, Michal Privoznik wrote:
> The virConnectListAllNWFilters() has no extra flags yet, which
> simplifies things a bit.
> 

CONNECT flags wouldn't make sense here either especially for undefine
where things either are or aren't.


> Signed-off-by: Michal Privoznik 
> ---
>  tools/virsh-completer.c | 45 +
>  tools/virsh-completer.h |  4 
>  tools/virsh-nwfilter.c  |  9 ++---
>  3 files changed, 55 insertions(+), 3 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 5/8] virsh: Introduce virshNodeDeviceNameCompleter

[John Ferlan]

On 01/12/2018 09:37 AM, Michal Privoznik wrote:
> Yet again, we don't need listing by device capabilities, so flags
> are unused.

The CONNECT flags wouldn't make sense anyway ;-)

> 
> Signed-off-by: Michal Privoznik 
> ---
>  tools/virsh-completer.c | 45 +
>  tools/virsh-completer.h |  4 
>  tools/virsh-nodedev.c   | 16 +++-
>  3 files changed, 60 insertions(+), 5 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

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

[Eduardo Habkost]

On Wed, Jan 17, 2018 at 09:04:26AM +0100, Kashyap Chamarthy wrote:
> On Tue, Jan 16, 2018 at 03:35:20PM -0200, Eduardo Habkost wrote:
> > On Fri, Jan 12, 2018 at 08:31:16PM +0100, Kashyap Chamarthy wrote:
> > > Currently, the CPU feature 'name' XML attribute, as in:
> 
> [...]
> 
> > > ---
> > >  docs/formatdomain.html.in | 17 +
> > >  1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > > index d272cc1ba..e717fb3aa 100644
> > > --- a/docs/formatdomain.html.in
> > > +++ b/docs/formatdomain.html.in
> > > @@ -1454,6 +1454,23 @@
> > >  
> > >  Since 0.8.5 the policy
> > >  attribute can be omitted and will default to 
> > > require.
> > > +
> > > +Individual CPU feature names can be specified as part of the
> > > +name attribute.
> > 
> > Isn't this "should" instead of "can"?  Does it make sense to have
> > a 'feature' element without a 'name' attribute?
> 
> Good catch.  Near as I see, it doesn't.  So I'll: s/can/should.
> 
> > 
> > >   The list of known CPU feature
> > > +names (e.g. 'vmx', 'cmt', et cetera) can be found in the same
> > > +file as CPU models -- cpu_map.xml. For example,
> > > +to explicitly specify the 'pcid' feature with Intel IvyBridge
> > > +CPU model:
> > 
> > Another paragraph above already says "The list of known feature
> > names can be found in the same file as CPU models".   If you think the
> > existing paragraph is not enough, I suggest rewriting it so the
> > document won't repeat exactly the same thing.
> 
> True.  How about this rewrite: 
> 
> "Once you choose a feature (e.g. 'pcid') from the `cpu_map.xml`, to
> specify it explicitly with the Intel IvyBridge CPU model [...]"

"Once you choose a feature (e.g. 'pcid') from the `cpu_map.xml`"
doesn't seem to convey any additional information that wasn't
mentioned before.  What about just "For example, to explicitly
specify the 'pcid' feature with Intel IvyBridge CPU model:"?

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

Maybe this would be too much information.  It's harmless to set a feature
explicitly to 'require' if the CPU model already contains the feature.

-- 
Eduardo

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

Re: [libvirt] [PATCH 4/8] virsh: Introduce virshNetworkNameCompleter

[John Ferlan]

On 01/12/2018 09:37 AM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  tools/virsh-completer.c | 51 
> +
>  tools/virsh-completer.h |  4 
>  tools/virsh-network.c   | 24 ---
>  3 files changed, 68 insertions(+), 11 deletions(-)
> 


For as much as I recall there being complaints when I added the various
VIRSH macros for various comments, I would think that made this task easier!

> diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
> index f5b1e4261..2c0d4f640 100644
> --- a/tools/virsh-completer.c
> +++ b/tools/virsh-completer.c
> @@ -297,3 +297,54 @@ virshInterfaceNameCompleter(vshControl *ctl,
>  VIR_FREE(ret);
>  return NULL;
>  }
> +
> +
> +char **
> +virshNetworkNameCompleter(vshControl *ctl,
> +  const vshCmd *cmd ATTRIBUTE_UNUSED,
> +  unsigned int flags)
> +{
> +virshControlPtr priv = ctl->privData;
> +virNetworkPtr *nets = NULL;
> +int nnets = 0;
> +size_t i = 0;
> +char **ret = NULL;
> +
> +virCheckFlags(VIR_CONNECT_LIST_NETWORKS_INACTIVE |
> +  VIR_CONNECT_LIST_NETWORKS_ACTIVE |
> +  VIR_CONNECT_LIST_NETWORKS_PERSISTENT |
> +  VIR_CONNECT_LIST_NETWORKS_TRANSIENT |
> +  VIR_CONNECT_LIST_NETWORKS_AUTOSTART |
> +  VIR_CONNECT_LIST_NETWORKS_NO_AUTOSTART,
> +  NULL);

Only 0, ACTIVE, and INACTIVE are used, but I think PERSISTENT needs to
be added (per below).

> +
> +if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
> +return NULL;
> +
> +if ((nnets = virConnectListAllNetworks(priv->conn, , flags)) < 0)
> +return NULL;
> +
> +if (VIR_ALLOC_N(ret, nnets + 1) < 0)
> +goto error;
> +
> +for (i = 0; i < nnets; i++) {
> +const char *name = virNetworkGetName(nets[i]);
> +
> +if (VIR_STRDUP(ret[i], name) < 0)
> +goto error;
> +
> +virNetworkFree(nets[i]);
> +}
> +VIR_FREE(nets);
> +
> +return ret;
> +
> + error:
> +for (; i < nnets; i++)
> +virNetworkFree(nets[i]);
> +VIR_FREE(nets);
> +for (i = 0; i < nnets; i++)
> +VIR_FREE(ret[i]);
> +VIR_FREE(ret);
> +return NULL;
> +}
> diff --git a/tools/virsh-completer.h b/tools/virsh-completer.h
> index 2323aaba3..20ba4cb55 100644
> --- a/tools/virsh-completer.h
> +++ b/tools/virsh-completer.h
> @@ -50,4 +50,8 @@ char ** virshInterfaceNameCompleter(vshControl *ctl,
>  const vshCmd *cmd,
>  unsigned int flags);
>  
> +char ** virshNetworkNameCompleter(vshControl *ctl,
> +  const vshCmd *cmd,
> +  unsigned int flags);
> +
>  #endif
> diff --git a/tools/virsh-network.c b/tools/virsh-network.c
> index cd55e384f..3b472ea67 100644
> --- a/tools/virsh-network.c
> +++ b/tools/virsh-network.c
> @@ -34,11 +34,13 @@
>  #include "virtime.h"
>  #include "conf/network_conf.h"
>  
> -#define VIRSH_COMMON_OPT_NETWORK \
> +#define VIRSH_COMMON_OPT_NETWORK(cflags) \
>  {.name = "network", \
>   .type = VSH_OT_DATA, \
>   .flags = VSH_OFLAG_REQ, \
> - .help = N_("network name or uuid") \
> + .help = N_("network name or uuid"), \
> + .completer = virshNetworkNameCompleter, \
> + .completer_flags = cflags, \
>  }
>  
>  virNetworkPtr
> @@ -93,7 +95,7 @@ static const vshCmdInfo info_network_autostart[] = {
>  };
>  
>  static const vshCmdOptDef opts_network_autostart[] = {
> -VIRSH_COMMON_OPT_NETWORK,
> +VIRSH_COMMON_OPT_NETWORK(0),

Light dawns... Would only be reasonable for PERSISTENT networks right?
Not going to autostart a transient one...  Perhaps this should be true
for previous ones already pushed and reviewed... That is I won't go back
and I won't bring it up again.

>  {.name = "disable",
>   .type = VSH_OT_BOOL,
>   .help = N_("disable autostarting")
> @@ -240,7 +242,7 @@ static const vshCmdInfo info_network_destroy[] = {
>  };
>  
>  static const vshCmdOptDef opts_network_destroy[] = {
> -VIRSH_COMMON_OPT_NETWORK,
> +VIRSH_COMMON_OPT_NETWORK(VIR_CONNECT_LIST_NETWORKS_ACTIVE),
>  {.name = NULL}
>  };
>  
> @@ -279,7 +281,7 @@ static const vshCmdInfo info_network_dumpxml[] = {
>  };
>  
>  static const vshCmdOptDef opts_network_dumpxml[] = {
> -VIRSH_COMMON_OPT_NETWORK,
> +VIRSH_COMMON_OPT_NETWORK(0),
>  {.name = "inactive",
>   .type = VSH_OT_BOOL,
>   .help = N_("show inactive defined XML")
> @@ -330,7 +332,7 @@ static const vshCmdInfo info_network_info[] = {
>  };
>  
>  static const vshCmdOptDef opts_network_info[] = {
> -VIRSH_COMMON_OPT_NETWORK,
> +VIRSH_COMMON_OPT_NETWORK(0),
>  {.name = NULL}
>  };
>  
> @@ -779,7 +781,7 @@ static const vshCmdInfo info_network_start[] = {
>  };
>  
>  static const vshCmdOptDef 

Re: [libvirt] [PATCH 3/8] virsh: Introduce virshInterfaceNameCompleter

[John Ferlan]

On 01/12/2018 09:37 AM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  tools/virsh-completer.c | 47 +++
>  tools/virsh-completer.h |  4 
>  tools/virsh-interface.c | 16 +---
>  3 files changed, 60 insertions(+), 7 deletions(-)
> 

Noted a couple of patches ago about the oddness with
virshDomainInterfaceCompleter...  Doesn't stop this, but it may alter
this one if the other interface is changed.

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 2/8] virsh: Introduce virshStorageVolNameCompleter

[John Ferlan]

On 01/12/2018 09:37 AM, Michal Privoznik wrote:
> This one is a bit simpler since virStoragePoolListAllVolumes()
> has no flags yet.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  tools/virsh-completer.c | 52 
> +
>  tools/virsh-completer.h |  4 
>  tools/virsh-volume.c|  3 ++-
>  3 files changed, 58 insertions(+), 1 deletion(-)
> 

Reviewed-by: John Ferlan 


John

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

Re: [libvirt] [PATCH 1/8] virsh: Introduce virshStoragePoolNameCompleter

[John Ferlan]

On 01/12/2018 09:37 AM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  tools/virsh-completer.c | 51 
> +
>  tools/virsh-completer.h |  4 
>  tools/virsh-pool.c  | 28 +--
>  tools/virsh-volume.c| 42 +---
>  tools/virsh.h   |  6 --
>  5 files changed, 95 insertions(+), 36 deletions(-)
> 
> diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
> index 48dd9fbc2..8ca2fffd9 100644
> --- a/tools/virsh-completer.c
> +++ b/tools/virsh-completer.c
> @@ -147,3 +147,54 @@ virshDomainInterfaceCompleter(vshControl *ctl,
>  virStringListFree(ret);
>  return NULL;
>  }
> +
> +
> +char **
> +virshStoragePoolNameCompleter(vshControl *ctl,
> +  const vshCmd *cmd ATTRIBUTE_UNUSED,
> +  unsigned int flags)
> +{
> +virshControlPtr priv = ctl->privData;
> +virStoragePoolPtr *pools = NULL;
> +int npools = 0;
> +size_t i = 0;
> +char **ret = NULL;
> +
> +virCheckFlags(VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE |
> +  VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE |
> +  VIR_CONNECT_LIST_STORAGE_POOLS_PERSISTENT |
> +  VIR_CONNECT_LIST_STORAGE_POOLS_TRANSIENT |
> +  VIR_CONNECT_LIST_STORAGE_POOLS_AUTOSTART |
> +  VIR_CONNECT_LIST_STORAGE_POOLS_NO_AUTOSTART,
> +  NULL);

So for this and other patches, is this list right? For various storage
consumers, all I see necessary is 0, ACTIVE, INACTIVE, and PERSISTENT.

For that matter VIRSH_COMMON_OPT_DOMAIN_FULL doesn't seem to use the
full set from virshDomainNameCompleter either... I see 0, ACTIVE,
RUNNING, OTHER, PERSISTENT, PAUSED, and INACTIVE.

Also, it seems there's a dual usage model where either various CONNECT
flags are used or the Completer function could "roll it's own"?  That is
the virshDomainInterfaceCompleter reads the XML files and checks flags
against VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC (e.g. 1 << 0) which is the
same as VIR_CONNECT_LIST_INTERFACES_INACTIVE.  A few patches later we
have virshInterfaceNameCompleter that's using the same CONNECT flags.
It could be construed as confusing... Took me a few to realize the usage.

FWIW: in the IIRC the "odd" thing about MAC's is that they could be
added once the vNIC is active - that is they'll eventually show up. So
just because it's not in the config/inactive XML doesn't mean it won't
be present. So should the code be only reading the XML? Maybe it'd be
better to add a CONNECT flag that returns when a MAC is present and then
let this code choose the active/inactive + new connect mac flag.  I
dunno. I know separate issue, patches welcome, yadda yadda, but figured
I'd bring it up.



> +
> +if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
> +return NULL;

Since this same check is copied everywhere, why not move it to the
caller (e.g. vshReadlineParse) ?

> +
> +if ((npools = virConnectListAllStoragePools(priv->conn, , flags)) 
> < 0)
> +return NULL;
> +
> +if (VIR_ALLOC_N(ret, npools + 1) < 0)
> +goto error;
> +
> +for (i = 0; i < npools; i++) {
> +const char *name = virStoragePoolGetName(pools[i]);
> +
> +if (VIR_STRDUP(ret[i], name) < 0)
> +goto error;
> +
> +virStoragePoolFree(pools[i]);
> +}
> +VIR_FREE(pools);
> +
> +return ret;
> +
> + error:
> +for (; i < npools; i++)
> +virStoragePoolFree(pools[i]);
> +VIR_FREE(pools);
> +for (i = 0; i < npools; i++)
> +VIR_FREE(ret[i]);
> +VIR_FREE(ret);
> +return NULL;
> +}
> diff --git a/tools/virsh-completer.h b/tools/virsh-completer.h
> index 1a2dd685f..249e793b9 100644
> --- a/tools/virsh-completer.h
> +++ b/tools/virsh-completer.h
> @@ -38,4 +38,8 @@ char ** virshDomainInterfaceCompleter(vshControl *ctl,
>const vshCmd *cmd,
>unsigned int flags);
>  
> +char ** virshStoragePoolNameCompleter(vshControl *ctl,
> +  const vshCmd *cmd,
> +  unsigned int flags);
> +
>  #endif
> diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
> index 094874b64..cea4cfc12 100644
> --- a/tools/virsh-pool.c
> +++ b/tools/virsh-pool.c
> @@ -34,8 +34,8 @@
>  #include "virstring.h"
>  #include "virtime.h"
>  
> -#define VIRSH_COMMON_OPT_POOL_FULL \
> -VIRSH_COMMON_OPT_POOL(N_("pool name or uuid"))
> +#define VIRSH_COMMON_OPT_POOL_FULL(cflags) \
> +VIRSH_COMMON_OPT_POOL(N_("pool name or uuid"), cflags)
>  
>  #define VIRSH_COMMON_OPT_POOL_BUILD \
>  {.name = "build", \
> @@ -182,7 +182,7 @@ static const vshCmdInfo info_pool_autostart[] = {
>  };
>  
>  static const vshCmdOptDef opts_pool_autostart[] = {
> -VIRSH_COMMON_OPT_POOL_FULL,
> +

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

[Daniel P. Berrange]

On Mon, Jan 22, 2018 at 10:25:38AM -0700, Jim Fehlig wrote:
> On 01/17/2018 08:34 AM, Guido Günther wrote:
> > Otherwise stopping domains with qemu://session fails like
> > 
> > [164012.338157] audit: type=1400 audit(1516202208.784:99): 
> > apparmor="DENIED" operation="signal" profile="/usr/sbin/libvirtd" pid=18835 
> > comm="libvirtd" requested_mask="send" denied_mask="send" signal=term 
> > peer="unconfined"
> > ---
> >   examples/apparmor/usr.sbin.libvirtd | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/examples/apparmor/usr.sbin.libvirtd 
> > b/examples/apparmor/usr.sbin.libvirtd
> > index 0ddec3f6e2..be4fabf905 100644
> > --- a/examples/apparmor/usr.sbin.libvirtd
> > +++ b/examples/apparmor/usr.sbin.libvirtd
> > @@ -63,7 +63,7 @@
> > signal (send) peer=/usr/sbin/dnsmasq,
> > signal (read, send) peer=libvirt-*,
> > -  signal (send) set=("kill") peer=unconfined,
> > +  signal (send) set=("kill", "term") peer=unconfined,
> 
> Is "hup" needed here as well?

Shouldn't be, libvirt starts by using 'term' to kill QEMU and if that
doesn't work, falls back to "kill". It shouldn't ever use "hup"


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

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

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

[Jim Fehlig]

On 01/17/2018 08:34 AM, Guido Günther wrote:

Otherwise stopping domains with qemu://session fails like

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

diff --git a/examples/apparmor/usr.sbin.libvirtd 
b/examples/apparmor/usr.sbin.libvirtd
index 0ddec3f6e2..be4fabf905 100644
--- a/examples/apparmor/usr.sbin.libvirtd
+++ b/examples/apparmor/usr.sbin.libvirtd
@@ -63,7 +63,7 @@
  
signal (send) peer=/usr/sbin/dnsmasq,

signal (read, send) peer=libvirt-*,
-  signal (send) set=("kill") peer=unconfined,
+  signal (send) set=("kill", "term") peer=unconfined,


Is "hup" needed here as well?

Regards,
Jim

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

[libvirt] [dbus RFC 10/11] main: implement multiple connection within one daemon

[Pavel Hrdina]

This effectively removes --connect parameter from daemon, for now we
will support only local connection.  Instead of specifying URI for
different drivers we will use connect object which allows using
multiple drivers from one daemon.  In the future if that feature will
be requested we can extend libvirt-dbus to have some configuration
file where we will have an option to specify remote host instead of
local connection.

This change also defines the difference between session and system
bus, and limits the list of drivers available for that specific bus
because not all drivers support session mode.  Currently only QEMU
driver has session mode.  For test purposes we can allow TEST driver
to be available for session bus.

Signed-off-by: Pavel Hrdina 
---
 src/connect.c   | 17 +++--
 src/connect.h   |  4 +++-
 src/main.c  | 55 ++---
 test/libvirttest.py |  4 ++--
 4 files changed, 55 insertions(+), 25 deletions(-)

diff --git a/src/connect.c b/src/connect.c
index 5249eba..878cf91 100644
--- a/src/connect.c
+++ b/src/connect.c
@@ -238,7 +238,8 @@ static const sd_bus_vtable virt_connect_vtable[] = {
 int
 virtDBusConnectNew(virtDBusConnect **connectp,
sd_bus *bus,
-   const char *uri)
+   const char *uri,
+   const char *connectPath)
 {
 _cleanup_(virtDBusConnectFreep) virtDBusConnect *connect = NULL;
 int r;
@@ -249,7 +250,7 @@ virtDBusConnectNew(virtDBusConnect **connectp,
 
 connect->bus = sd_bus_ref(bus);
 connect->uri = uri;
-connect->connectPath = "/org/libvirt/Connect";
+connect->connectPath = connectPath;
 
 connect->enumerateDomains = virtDBusConnectEnumarateDomains;
 
@@ -293,3 +294,15 @@ virtDBusConnectFreep(virtDBusConnect **connectp)
 if (*connectp)
 virtDBusConnectFree(*connectp);
 }
+
+void
+virtDBusConnectListFree(virtDBusConnect ***connectList)
+{
+if (!*connectList)
+return;
+
+for (int i = 0; (*connectList)[i]; i += 1)
+virtDBusConnectFree((*connectList)[i]);
+
+free(*connectList);
+}
diff --git a/src/connect.h b/src/connect.h
index bed2c7e..46e8c9a 100644
--- a/src/connect.h
+++ b/src/connect.h
@@ -20,6 +20,8 @@ typedef struct virtDBusConnect virtDBusConnect;
 
 int virtDBusConnectNew(virtDBusConnect **connectp,
sd_bus *bus,
-   const char *uri);
+   const char *uri,
+   const char *connectPath);
 virtDBusConnect *virtDBusConnectFree(virtDBusConnect *connect);
 void virtDBusConnectFreep(virtDBusConnect **connectp);
+void virtDBusConnectListFree(virtDBusConnect ***connectList);
diff --git a/src/main.c b/src/main.c
index 95522f5..14c7c18 100644
--- a/src/main.c
+++ b/src/main.c
@@ -79,6 +79,21 @@ virtDBusHandleBusEvent(int watch,
 virEventUpdateHandle(watch, virtDBusGetLibvirtEvents(bus));
 }
 
+struct virtDBusDriver {
+const char *uri;
+const char *object;
+};
+
+static const struct virtDBusDriver sessionDrivers[] = {
+{ "qemu:///session","/org/libvirt/qemu" },
+{ "test:///default","/org/libvirt/test" },
+};
+
+static const struct virtDBusDriver systemDrivers[] = {
+{ "qemu:///system", "/org/libvirt/qemu" },
+{ "test:///default","/org/libvirt/test" },
+};
+
 int
 main(int argc, char *argv[])
 {
@@ -89,16 +104,16 @@ main(int argc, char *argv[])
 
 static const struct option options[] = {
 { "help",no_argument,   NULL, 'h' },
-{ "connect", required_argument, NULL, 'c' },
 { "system",  no_argument,   NULL, ARG_SYSTEM },
 { "session", no_argument,   NULL, ARG_SESSION },
 {}
 };
 
 bool system_bus;
-const char *uri = NULL;
+const struct virtDBusDriver *drivers = NULL;
+int ndrivers = 0;
 
-_cleanup_(virtDBusConnectFreep) virtDBusConnect *connect = NULL;
+_cleanup_(virtDBusConnectListFree) virtDBusConnect **connect = NULL;
 _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL;
 _cleanup_(virtDBusUtilClosep) int signal_fd = -1;
 _cleanup_(virtDBusVirEventRemoveHandlep) int bus_watch = -1;
@@ -121,15 +136,10 @@ main(int argc, char *argv[])
 printf("Provide a D-Bus interface to a libvirtd.\n");
 printf("\n");
 printf("  -h, --helpDisplay this help text and 
exit\n");
-printf("  -c, --connect URI Connect to the specified libvirt 
URI\n");
 printf("  --session Connect to the session bus\n");
 printf("  --system  Connect to the system bus\n");
 return 0;
 
-case 'c':
-uri = optarg;
-break;
-
 case ARG_SYSTEM:
 system_bus = true;
 break;
@@ -143,14 +153,6 @@ main(int argc, char *argv[])
 }
 }
 
-if (uri == NULL) {
-if 

[libvirt] [dbus RFC 09/11] domain: derive domain path from connect path

[Pavel Hrdina]

Each connection have its own domains so we must make the domain path
as a sub-path of the specific connection.

Signed-off-by: Pavel Hrdina 
---
 src/connect.c | 12 +++
 src/connect.h |  1 +
 src/domain.c  | 64 ---
 src/events.c  | 10 +-
 src/util.c| 10 ++
 src/util.h|  6 --
 6 files changed, 68 insertions(+), 35 deletions(-)

diff --git a/src/connect.c b/src/connect.c
index 3aec99e..5249eba 100644
--- a/src/connect.c
+++ b/src/connect.c
@@ -100,7 +100,8 @@ virtDBusConnectEnumarateDomains(sd_bus *bus VIR_ATTR_UNUSED,
 paths = calloc(n_domains + 1, sizeof(char *));
 
 for (int i = 0; i < n_domains; i += 1)
-paths[i] = virtDBusUtilBusPathForVirDomain(domains[i]);
+paths[i] = virtDBusUtilBusPathForVirDomain(domains[i],
+   connect->domainPath);
 
 *nodes = paths;
 paths = NULL;
@@ -142,7 +143,8 @@ virtDBusConnectListDomains(sd_bus_message *message,
 for (int i = 0; domains[i] != NULL; i += 1) {
 _cleanup_(virtDBusUtilFreep) char *path = NULL;
 
-path = virtDBusUtilBusPathForVirDomain(domains[i]);
+path = virtDBusUtilBusPathForVirDomain(domains[i],
+   connect->domainPath);
 
 r = sd_bus_message_append(reply, "o", path);
 if (r < 0)
@@ -180,7 +182,7 @@ virtDBusConnectCreateXML(sd_bus_message *message,
 if (!domain)
 return virtDBusUtilSetLastVirtError(error);
 
-path = virtDBusUtilBusPathForVirDomain(domain);
+path = virtDBusUtilBusPathForVirDomain(domain, connect->domainPath);
 
 return sd_bus_reply_method_return(message, "o", path);
 }
@@ -208,7 +210,7 @@ virtDBusConnectDefineXML(sd_bus_message *message,
 if (!domain)
 return virtDBusUtilSetLastVirtError(error);
 
-path = virtDBusUtilBusPathForVirDomain(domain);
+path = virtDBusUtilBusPathForVirDomain(domain, connect->domainPath);
 
 return sd_bus_reply_method_return(message, "o", path);
 }
@@ -278,6 +280,8 @@ virtDBusConnectFree(virtDBusConnect *connect)
 if (connect->connection)
 virtDBusConnectClose(connect, true);
 
+free(connect->domainPath);
+
 free(connect);
 
 return NULL;
diff --git a/src/connect.h b/src/connect.h
index 489ce84..bed2c7e 100644
--- a/src/connect.h
+++ b/src/connect.h
@@ -9,6 +9,7 @@ struct virtDBusConnect {
 sd_bus *bus;
 const char *uri;
 const char *connectPath;
+char *domainPath;
 virConnectPtr connection;
 
 sd_bus_node_enumerator_t enumerateDomains;
diff --git a/src/domain.c b/src/domain.c
index bc465b0..33ed2b3 100644
--- a/src/domain.c
+++ b/src/domain.c
@@ -1,6 +1,9 @@
+#define _GNU_SOURCE
 #include "domain.h"
 #include "util.h"
 
+#include 
+
 static int
 virtDBusDomainGetName(sd_bus *bus VIR_ATTR_UNUSED,
   const char *path,
@@ -14,7 +17,8 @@ virtDBusDomainGetName(sd_bus *bus VIR_ATTR_UNUSED,
 _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL;
 const char *name = "";
 
-domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path);
+domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path,
+  connect->domainPath);
 if (domain == NULL)
 return sd_bus_message_append(reply, "s", "");
 
@@ -38,7 +42,8 @@ virtDBusDomainGetUUID(sd_bus *bus VIR_ATTR_UNUSED,
 _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL;
 char uuid[VIR_UUID_STRING_BUFLEN] = "";
 
-domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path);
+domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path,
+  connect->domainPath);
 if (domain == NULL)
 return sd_bus_message_append(reply, "s", "");
 
@@ -59,7 +64,8 @@ virtDBusDomainGetId(sd_bus *bus VIR_ATTR_UNUSED,
 virtDBusConnect *connect = userdata;
 _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL;
 
-domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path);
+domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path,
+  connect->domainPath);
 if (domain == NULL)
 return sd_bus_message_append(reply, "u", 0);
 
@@ -78,7 +84,8 @@ virtDBusDomainGetVcpus(sd_bus *bus VIR_ATTR_UNUSED,
 virtDBusConnect *connect = userdata;
 _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL;
 
-domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path);
+domain = virtDBusUtilVirDomainFromBusPath(connect->connection, path,
+  connect->domainPath);
 if (domain == NULL)
 return sd_bus_message_append(reply, "u", 0);
 
@@ -98,7 +105,8 @@ virtDBusDomainGetOsType(sd_bus *bus VIR_ATTR_UNUSED,
 _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr 

[libvirt] [dbus RFC 01/11] util: introduce VIRT_ARRAY_CARDINALITY macro

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---
 src/util.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/util.h b/src/util.h
index 0c9f3d7..6d99841 100644
--- a/src/util.h
+++ b/src/util.h
@@ -8,6 +8,8 @@
 #define _cleanup_(_x) __attribute__((__cleanup__(_x)))
 #define VIR_ATTR_UNUSED __attribute__((__unused__))
 
+#define VIRT_ARRAY_CARDINALITY(array) (sizeof(array) / sizeof(*(array)))
+
 
 int
 virtDBusUtilMessageAppendTypedParameters(sd_bus_message *message,
-- 
2.14.3

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

[libvirt] [dbus RFC 06/11] connect: implement reconnect functionality to libvirt

[Pavel Hrdina]

If the connection dies for some reason between D-Bus calls we need
to properly reconnect because the current connection is not usable
anymore.  Without this the only solution would be to restart the
libvirt-dbus daemon.

Signed-off-by: Pavel Hrdina 
---
 src/connect.c | 37 +++--
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/src/connect.c b/src/connect.c
index 9de764c..10183f3 100644
--- a/src/connect.c
+++ b/src/connect.c
@@ -4,6 +4,7 @@
 #include "util.h"
 
 #include 
+#include 
 #include 
 
 static int virtDBusConnectCredType[] = {
@@ -34,12 +35,34 @@ static virConnectAuth virtDBusConnectAuth = {
 NULL,
 };
 
+static void
+virtDBusConnectClose(virtDBusConnect *connect,
+ bool deregisterEvents)
+{
+
+for (int i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i += 1) {
+if (connect->callback_ids[i] >= 0) {
+if (deregisterEvents) {
+virConnectDomainEventDeregisterAny(connect->connection,
+   connect->callback_ids[i]);
+}
+connect->callback_ids[i] = -1;
+}
+}
+
+virConnectClose(connect->connection);
+}
+
 static int
 virtDBusConnectOpen(virtDBusConnect *connect,
 sd_bus_error *error)
 {
-if (connect->connection)
-return 0;
+if (connect->connection) {
+if (virConnectIsAlive(connect->connection))
+return 0;
+else
+virtDBusConnectClose(connect, false);
+}
 
 virtDBusConnectAuth.cbdata = error;
 
@@ -254,14 +277,8 @@ virtDBusConnectFree(virtDBusConnect *connect)
 if (connect->bus)
 sd_bus_unref(connect->bus);
 
-if (connect->connection) {
-for (int i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i += 1) {
-if (connect->callback_ids[i] >= 0)
-virConnectDomainEventDeregisterAny(connect->connection, 
connect->callback_ids[i]);
-}
-
-virConnectClose(connect->connection);
-}
+if (connect->connection)
+virtDBusConnectClose(connect, true);
 
 free(connect);
 
-- 
2.14.3

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

[libvirt] [dbus RFC 02/11] util: introduce virtDBusUtilSetError helper

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---
 src/util.c | 7 +++
 src/util.h | 4 
 2 files changed, 11 insertions(+)

diff --git a/src/util.c b/src/util.c
index 2445ce0..6636d0a 100644
--- a/src/util.c
+++ b/src/util.c
@@ -69,6 +69,13 @@ virtDBusUtilSetLastVirtError(sd_bus_error *error)
 return sd_bus_error_set(error, "org.libvirt.Error", vir_error->message);
 }
 
+int
+virtDBusUtilSetError(sd_bus_error *error,
+ const char *message)
+{
+return sd_bus_error_set(error, "org.libvirt.Error", message);
+}
+
 char *
 virtDBusUtilBusPathForVirDomain(virDomainPtr domain)
 {
diff --git a/src/util.h b/src/util.h
index 6d99841..cb8447d 100644
--- a/src/util.h
+++ b/src/util.h
@@ -19,6 +19,10 @@ virtDBusUtilMessageAppendTypedParameters(sd_bus_message 
*message,
 int
 virtDBusUtilSetLastVirtError(sd_bus_error *error);
 
+int
+virtDBusUtilSetError(sd_bus_error *error,
+ const char *message);
+
 char *
 virtDBusUtilBusPathForVirDomain(virDomainPtr domain);
 
-- 
2.14.3

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

[libvirt] [dbus RFC 04/11] connect: implement lazy connection

[Pavel Hrdina]

Open a connection to libvirt only when it is required.

Signed-off-by: Pavel Hrdina 
---
 src/connect.c | 41 +++--
 src/connect.h |  1 +
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/src/connect.c b/src/connect.c
index cb19c39..8d958c2 100644
--- a/src/connect.c
+++ b/src/connect.c
@@ -6,6 +6,23 @@
 #include 
 #include 
 
+static int
+virtDBusConnectOpen(virtDBusConnect *connect,
+sd_bus_error *error)
+{
+if (connect->connection)
+return 0;
+
+connect->connection = virConnectOpenAuth(connect->uri,
+ virConnectAuthPtrDefault, 0);
+if (!connect->connection)
+return virtDBusUtilSetLastVirtError(error);
+
+virtDBusEventsRegister(connect);
+
+return 0;
+}
+
 static int
 virtDBusConnectEnumarateDomains(sd_bus *bus VIR_ATTR_UNUSED,
 const char *path VIR_ATTR_UNUSED,
@@ -17,6 +34,11 @@ virtDBusConnectEnumarateDomains(sd_bus *bus VIR_ATTR_UNUSED,
 _cleanup_(virtDBusUtilVirDomainListFreep) virDomainPtr *domains = NULL;
 _cleanup_(virtDBusUtilStrvFreep) char **paths = NULL;
 int n_domains;
+int r;
+
+r = virtDBusConnectOpen(connect, error);
+if (r < 0)
+return r;
 
 n_domains = virConnectListAllDomains(connect->connection, , 0);
 if (n_domains < 0)
@@ -44,6 +66,10 @@ virtDBusConnectListDomains(sd_bus_message *message,
 uint32_t flags;
 int r;
 
+r = virtDBusConnectOpen(connect, error);
+if (r < 0)
+return r;
+
 r = sd_bus_message_read(message, "u", );
 if (r < 0)
 return r;
@@ -89,6 +115,10 @@ virtDBusConnectCreateXML(sd_bus_message *message,
 _cleanup_(virtDBusUtilFreep) char *path = NULL;
 int r;
 
+r = virtDBusConnectOpen(connect, error);
+if (r < 0)
+return r;
+
 r = sd_bus_message_read(message, "su", , );
 if (r < 0)
 return r;
@@ -113,6 +143,10 @@ virtDBusConnectDefineXML(sd_bus_message *message,
 _cleanup_(virtDBusUtilFreep) char *path = NULL;
 int r;
 
+r = virtDBusConnectOpen(connect, error);
+if (r < 0)
+return r;
+
 r = sd_bus_message_read(message, "s", );
 if (r < 0)
 return r;
@@ -159,12 +193,7 @@ virtDBusConnectNew(virtDBusConnect **connectp,
 connect->callback_ids[i] = -1;
 
 connect->bus = sd_bus_ref(bus);
-
-connect->connection = virConnectOpenAuth(uri, virConnectAuthPtrDefault, 0);
-if (!connect->connection)
-return -EINVAL;
-
-virtDBusEventsRegister(connect);
+connect->uri = uri;
 
 r = sd_bus_add_object_vtable(connect->bus,
  NULL,
diff --git a/src/connect.h b/src/connect.h
index 5d64a10..52e8279 100644
--- a/src/connect.h
+++ b/src/connect.h
@@ -7,6 +7,7 @@
 
 struct virtDBusConnect {
 sd_bus *bus;
+const char *uri;
 virConnectPtr connection;
 
 int callback_ids[VIR_DOMAIN_EVENT_ID_LAST];
-- 
2.14.3

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

[libvirt] [dbus RFC 08/11] connect: store connect path in connect structure

[Pavel Hrdina]

This prepares for multiple connection objects since each object will
have different path based on the driver name.

Signed-off-by: Pavel Hrdina 
---
 src/connect.c | 3 ++-
 src/connect.h | 1 +
 src/events.c  | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/connect.c b/src/connect.c
index 314277f..3aec99e 100644
--- a/src/connect.c
+++ b/src/connect.c
@@ -247,12 +247,13 @@ virtDBusConnectNew(virtDBusConnect **connectp,
 
 connect->bus = sd_bus_ref(bus);
 connect->uri = uri;
+connect->connectPath = "/org/libvirt/Connect";
 
 connect->enumerateDomains = virtDBusConnectEnumarateDomains;
 
 r = sd_bus_add_object_vtable(connect->bus,
  NULL,
- "/org/libvirt/Connect",
+ connect->connectPath,
  "org.libvirt.Connect",
  virt_connect_vtable,
  connect);
diff --git a/src/connect.h b/src/connect.h
index de1aae7..489ce84 100644
--- a/src/connect.h
+++ b/src/connect.h
@@ -8,6 +8,7 @@
 struct virtDBusConnect {
 sd_bus *bus;
 const char *uri;
+const char *connectPath;
 virConnectPtr connection;
 
 sd_bus_node_enumerator_t enumerateDomains;
diff --git a/src/events.c b/src/events.c
index 92f280d..d2d7f6b 100644
--- a/src/events.c
+++ b/src/events.c
@@ -53,7 +53,7 @@ virtDBusEventsDomainLifecycle(virConnectPtr connection 
VIR_ATTR_UNUSED,
 
 r = sd_bus_message_new_signal(connect->bus,
   ,
-  "/org/libvirt/Connect",
+  connect->connectPath,
   "org.libvirt.connect",
   signal);
 if (r < 0)
-- 
2.14.3

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

[libvirt] [dbus RFC 11/11] main: add support for all libvirt drivers

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---
 src/main.c | 26 ++
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/src/main.c b/src/main.c
index 14c7c18..6421919 100644
--- a/src/main.c
+++ b/src/main.c
@@ -85,13 +85,31 @@ struct virtDBusDriver {
 };
 
 static const struct virtDBusDriver sessionDrivers[] = {
-{ "qemu:///session","/org/libvirt/qemu" },
-{ "test:///default","/org/libvirt/test" },
+{ "qemu:///session","/org/libvirt/qemu" },
+{ "test:///default","/org/libvirt/test" },
+{ "uml:///session", "/org/libvirt/uml" },
+{ "vbox:///session","/org/libvirt/vbox" },
+{ "vmwarefusion:///session","/org/libvirt/vmwarefusion" },
+{ "vmwareplayer:///session","/org/libvirt/vmwareplayer" },
+{ "vmwarews:///session","/org/libvirt/vmwarews" },
 };
 
 static const struct virtDBusDriver systemDrivers[] = {
-{ "qemu:///system", "/org/libvirt/qemu" },
-{ "test:///default","/org/libvirt/test" },
+{ "XenApi://localhost/","/org/libvirt/XenApi" },
+{ "bhyve:///system","/org/libvirt/bhyve" },
+{ "esx://localhost/",   "/org/libvirt/esx" },
+{ "gsx://localhost/",   "/org/libvirt/gsx" },
+{ "hyperv://localhost/","/org/libvirt/hyperv" },
+{ "lxc:///","/org/libvirt/lxc" },
+{ "openvz:///system",   "/org/libvirt/openvz" },
+{ "phyp://localhost/",  "/org/libvirt/phyp" },
+{ "qemu:///system", "/org/libvirt/qemu" },
+{ "test:///default","/org/libvirt/test" },
+{ "uml:///system",  "/org/libvirt/uml" },
+{ "vbox:///system", "/org/libvirt/vbox" },
+{ "vpx://localhost/",   "/org/libvirt/vpx" },
+{ "vz:///system",   "/org/libvirt/vz" },
+{ "xen:///","/org/libvirt/xen" },
 };
 
 int
-- 
2.14.3

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

[libvirt] [dbus RFC 03/11] src: rename manager to connect

[Pavel Hrdina]

Manager was a good name for the original design.  However, the design
will be changed that for now libvirt-dbus will be using only local
connection and will support multiple drivers.

Signed-off-by: Pavel Hrdina 
---
 src/Makefile.am   |  2 +-
 src/{manager.c => connect.c}  | 93 ---
 src/{manager.h => connect.h}  | 10 ++--
 src/domain.c  | 80 +-
 src/domain.h  |  4 +-
 src/events.c  | 54 +-
 src/events.h  |  4 +-
 src/main.c|  8 +--
 test/Makefile.am  |  2 +-
 test/libvirttest.py   |  4 +-
 test/{test_manager.py => test_connect.py} | 12 ++--
 test/test_domain.py   |  6 +-
 12 files changed, 140 insertions(+), 139 deletions(-)
 rename src/{manager.c => connect.c} (64%)
 rename src/{manager.h => connect.h} (53%)
 rename test/{test_manager.py => test_connect.py} (79%)

diff --git a/src/Makefile.am b/src/Makefile.am
index 5d4cb04..1a5b50b 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -3,7 +3,7 @@ AM_CPPFLAGS = \
 
 DAEMON_SOURCES = \
main.c \
-   manager.c manager.h \
+   connect.c connect.h \
util.c util.h \
domain.c domain.h \
events.c events.h
diff --git a/src/manager.c b/src/connect.c
similarity index 64%
rename from src/manager.c
rename to src/connect.c
index f289a7a..cb19c39 100644
--- a/src/manager.c
+++ b/src/connect.c
@@ -1,24 +1,24 @@
 #include "domain.h"
 #include "events.h"
-#include "manager.h"
+#include "connect.h"
 #include "util.h"
 
 #include 
 #include 
 
 static int
-virtDBusManagerEnumarateDomains(sd_bus *bus VIR_ATTR_UNUSED,
+virtDBusConnectEnumarateDomains(sd_bus *bus VIR_ATTR_UNUSED,
 const char *path VIR_ATTR_UNUSED,
 void *userdata,
 char ***nodes,
 sd_bus_error *error)
 {
-virtDBusManager *manager = userdata;
+virtDBusConnect *connect = userdata;
 _cleanup_(virtDBusUtilVirDomainListFreep) virDomainPtr *domains = NULL;
 _cleanup_(virtDBusUtilStrvFreep) char **paths = NULL;
 int n_domains;
 
-n_domains = virConnectListAllDomains(manager->connection, , 0);
+n_domains = virConnectListAllDomains(connect->connection, , 0);
 if (n_domains < 0)
 return virtDBusUtilSetLastVirtError(error);
 
@@ -34,11 +34,11 @@ virtDBusManagerEnumarateDomains(sd_bus *bus VIR_ATTR_UNUSED,
 }
 
 static int
-virtDBusManagerListDomains(sd_bus_message *message,
+virtDBusConnectListDomains(sd_bus_message *message,
void *userdata,
sd_bus_error *error)
 {
-virtDBusManager *manager = userdata;
+virtDBusConnect *connect = userdata;
 _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL;
 _cleanup_(virtDBusUtilVirDomainListFreep) virDomainPtr *domains = NULL;
 uint32_t flags;
@@ -48,7 +48,7 @@ virtDBusManagerListDomains(sd_bus_message *message,
 if (r < 0)
 return r;
 
-r = virConnectListAllDomains(manager->connection, , flags);
+r = virConnectListAllDomains(connect->connection, , flags);
 if (r < 0)
 return virtDBusUtilSetLastVirtError(error);
 
@@ -78,11 +78,11 @@ virtDBusManagerListDomains(sd_bus_message *message,
 }
 
 static int
-virtDBusManagerCreateXML(sd_bus_message *message,
+virtDBusConnectCreateXML(sd_bus_message *message,
  void *userdata,
  sd_bus_error *error)
 {
-virtDBusManager *manager = userdata;
+virtDBusConnect *connect = userdata;
 const char *xml;
 uint32_t flags;
 _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL;
@@ -93,7 +93,7 @@ virtDBusManagerCreateXML(sd_bus_message *message,
 if (r < 0)
 return r;
 
-domain = virDomainCreateXML(manager->connection, xml, flags);
+domain = virDomainCreateXML(connect->connection, xml, flags);
 if (!domain)
 return virtDBusUtilSetLastVirtError(error);
 
@@ -103,11 +103,11 @@ virtDBusManagerCreateXML(sd_bus_message *message,
 }
 
 static int
-virtDBusManagerDefineXML(sd_bus_message *message,
+virtDBusConnectDefineXML(sd_bus_message *message,
  void *userdata,
  sd_bus_error *error)
 {
-virtDBusManager *manager = userdata;
+virtDBusConnect *connect = userdata;
 const char *xml;
 _cleanup_(virtDBusUtilVirDomainFreep) virDomainPtr domain = NULL;
 _cleanup_(virtDBusUtilFreep) char *path = NULL;
@@ -117,7 +117,7 @@ virtDBusManagerDefineXML(sd_bus_message *message,
 if (r < 0)
 return r;
 
-domain = virDomainDefineXML(manager->connection, xml);
+domain = virDomainDefineXML(connect->connection, xml);
 if 

[libvirt] [dbus RFC 07/11] connect: move domain related code to domain.c

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---
 src/connect.c | 7 ++-
 src/connect.h | 2 ++
 src/domain.c  | 7 +++
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/connect.c b/src/connect.c
index 10183f3..314277f 100644
--- a/src/connect.c
+++ b/src/connect.c
@@ -248,6 +248,8 @@ virtDBusConnectNew(virtDBusConnect **connectp,
 connect->bus = sd_bus_ref(bus);
 connect->uri = uri;
 
+connect->enumerateDomains = virtDBusConnectEnumarateDomains;
+
 r = sd_bus_add_object_vtable(connect->bus,
  NULL,
  "/org/libvirt/Connect",
@@ -257,11 +259,6 @@ virtDBusConnectNew(virtDBusConnect **connectp,
 if (r < 0)
 return r;
 
-r = sd_bus_add_node_enumerator(bus, NULL, "/org/libvirt/domain",
-   virtDBusConnectEnumarateDomains, connect);
-if (r < 0)
-return r;
-
 if ((r = virtDBusDomainRegister(connect, bus) < 0))
 return r;
 
diff --git a/src/connect.h b/src/connect.h
index 52e8279..de1aae7 100644
--- a/src/connect.h
+++ b/src/connect.h
@@ -10,6 +10,8 @@ struct virtDBusConnect {
 const char *uri;
 virConnectPtr connection;
 
+sd_bus_node_enumerator_t enumerateDomains;
+
 int callback_ids[VIR_DOMAIN_EVENT_ID_LAST];
 };
 typedef struct virtDBusConnect virtDBusConnect;
diff --git a/src/domain.c b/src/domain.c
index a3e1d0b..bc465b0 100644
--- a/src/domain.c
+++ b/src/domain.c
@@ -539,6 +539,13 @@ int
 virtDBusDomainRegister(virtDBusConnect *connect,
sd_bus *bus)
 {
+int r;
+
+r = sd_bus_add_node_enumerator(bus, NULL, "/org/libvirt/domain",
+   connect->enumerateDomains, connect);
+if (r < 0)
+return r;
+
 return sd_bus_add_fallback_vtable(bus,
   NULL,
   "/org/libvirt/domain",
-- 
2.14.3

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

[libvirt] [dbus RFC 05/11] connect: don't use default libvirt authentication callback

[Pavel Hrdina]

We need to implement our own authentication callback because the
default one ask for credentials using STDIO.  This is not suitable
behavior for daemon.

For now we will require usage of client configuration file for libvirt
to provide credentials for drivers that require authentication [1].

[1] 

Signed-off-by: Pavel Hrdina 
---
 src/connect.c | 32 +++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/src/connect.c b/src/connect.c
index 8d958c2..9de764c 100644
--- a/src/connect.c
+++ b/src/connect.c
@@ -6,6 +6,34 @@
 #include 
 #include 
 
+static int virtDBusConnectCredType[] = {
+VIR_CRED_AUTHNAME,
+VIR_CRED_ECHOPROMPT,
+VIR_CRED_REALM,
+VIR_CRED_PASSPHRASE,
+VIR_CRED_NOECHOPROMPT,
+VIR_CRED_EXTERNAL,
+};
+
+static int
+virtDBusConnectAuthCallback(virConnectCredentialPtr cred VIR_ATTR_UNUSED,
+unsigned int ncred VIR_ATTR_UNUSED,
+void *cbdata)
+{
+sd_bus_error *error = cbdata;
+
+return virtDBusUtilSetError(error,
+"Interactive authentication is not supported. "
+"Use client configuration file for libvirt.");
+}
+
+static virConnectAuth virtDBusConnectAuth = {
+virtDBusConnectCredType,
+VIRT_ARRAY_CARDINALITY(virtDBusConnectCredType),
+virtDBusConnectAuthCallback,
+NULL,
+};
+
 static int
 virtDBusConnectOpen(virtDBusConnect *connect,
 sd_bus_error *error)
@@ -13,8 +41,10 @@ virtDBusConnectOpen(virtDBusConnect *connect,
 if (connect->connection)
 return 0;
 
+virtDBusConnectAuth.cbdata = error;
+
 connect->connection = virConnectOpenAuth(connect->uri,
- virConnectAuthPtrDefault, 0);
+ , 0);
 if (!connect->connection)
 return virtDBusUtilSetLastVirtError(error);
 
-- 
2.14.3

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

[libvirt] [dbus RFC 00/11] improve libvirt connections handling

[Pavel Hrdina]

This patch series is a proposal to how to implement libvirt connections
in the libvirt D-Bus binding.  The design is based on a discussion with
Daniel and it should follow D-Bus convention.

Each connection is represented as existing object where the object
path contains the driver name:

/org/libvirt/qemu

and there is no need to call any connect API but simply use the existing
object directly to operate with that connection.

This patch series also implements strict difference between system and
user bus.  On the user bus only drivers with session capability are
available and on the system bus only drivers with system capability
are available.


Pavel Hrdina (11):
  util: introduce VIRT_ARRAY_CARDINALITY macro
  util: introduce virtDBusUtilSetError helper
  src: rename manager to connect
  connect: implement lazy connection
  connect: don't use default libvirt authentication callback
  connect: implement reconnect functionality to libvirt
  connect: move domain related code to domain.c
  connect: store connect path in connect structure
  domain: derive domain path from connect path
  main: implement multiple connection within one daemon
  main: add support for all libvirt drivers

 src/Makefile.am   |   2 +-
 src/connect.c | 308 ++
 src/connect.h |  27 +++
 src/domain.c  | 131 -
 src/domain.h  |   4 +-
 src/events.c  |  64 +++
 src/events.h  |   4 +-
 src/main.c|  75 ++--
 src/manager.c | 216 -
 src/manager.h |  20 --
 src/util.c|  17 +-
 src/util.h|  12 +-
 test/Makefile.am  |   2 +-
 test/libvirttest.py   |   6 +-
 test/{test_manager.py => test_connect.py} |  12 +-
 test/test_domain.py   |   6 +-
 16 files changed, 543 insertions(+), 363 deletions(-)
 create mode 100644 src/connect.c
 create mode 100644 src/connect.h
 delete mode 100644 src/manager.c
 delete mode 100644 src/manager.h
 rename test/{test_manager.py => test_connect.py} (79%)

-- 
2.14.3

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

Re: [libvirt] [PATCH v3] libvirtd: clarify the TLS conf default value setting

[John Ferlan]

On 01/21/2018 09:39 AM, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
> Provide more details related to the requirement that setting one
> of the values requires setting all of them.
> 
> Signed-off-by: Chen Hanxiao 
> 
> ---
> v3:
>   description updated follow John's comments
> v2:
>   fix a typo
> 
>  daemon/libvirtd.conf | 14 ++
>  1 file changed, 14 insertions(+)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH] libvirtd: Explicit dependency on systemd-machined

[Cedric Bosdonnat]

It's now pushed!
Thanks a lot Michal for your contribution, feel free to submit other ones in 
the future ;)

--
Cedric

On Wed, 2018-01-10 at 23:06 +0100, Michal Koutný wrote:
> The libvirtd daemon uses systemd-machined D-Bus API when manipulating
> domains. The systemd-machined is D-Bus activated on demand.
> 
> However, during system shutdown systemd-machined is stopped concurrently
> with libvirtd and virsh users also doing their final cleanup may
> transitively fail due to unavailability of systemd-machined. Example
> error message
> 
> > libvirtd[1390]: 2017-12-20 18:55:56.182+: 32700: error : 
> > virSystemdTerminateMachine:503 : Refusing activation,
> > D-Bus is shutting down.
> 
> To circumvent this we need to explicitly specify both ordering and
> requirement dependency (to avoid late D-Bus activation) on
> systemd-machined. See [1] for the dependency debate.
> 
> [1] 
> https://lists.freedesktop.org/archives/systemd-devel/2018-January/040095.html
> ---
> 
> The Wants= dependency is for the case when systemd-machined wasn't started
> neither D-Bus activated anytime before the shutdown transaction. AFAICS this 
> is
> very unlikely so for the sake of lazy activation, the Wants= hunk can be
> dropped.
> 
>  daemon/libvirtd.service.in | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/daemon/libvirtd.service.in b/daemon/libvirtd.service.in
> index c189f5e65..769702ea7 100644
> --- a/daemon/libvirtd.service.in
> +++ b/daemon/libvirtd.service.in
> @@ -7,6 +7,7 @@
>  Description=Virtualization daemon
>  Requires=virtlogd.socket
>  Requires=virtlockd.socket
> +Wants=systemd-machined.service
>  Before=libvirt-guests.service
>  After=network.target
>  After=dbus.service
> @@ -14,6 +15,7 @@ After=iscsid.service
>  After=apparmor.service
>  After=local-fs.target
>  After=remote-fs.target
> +After=systemd-machined.service
>  Documentation=man:libvirtd(8)
>  Documentation=https://libvirt.org
>  

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

Re: [libvirt] [PATCH 0/4] Add admin protocol support for virtlogd/virtlockd

[Daniel P. Berrange]

On Mon, Jan 22, 2018 at 04:44:10PM +0100, Michal Privoznik wrote:
> On 01/19/2018 06:09 PM, Daniel P. Berrange wrote:
> > The initial admin protocol support was only integrated into libvirtd.
> > This series extracts that code so that it is reusable with all the
> > daemons we have (and more than we'll get).
> > 
> > Daniel P. Berrange (4):
> >   admin: move admins server impl/dispatch into src/admin directory
> >   util: add virGetUNIXSocketPath helper
> >   logd: add support for admin protocol in virtlogd
> >   lockd: add support for admin protocol in virtlockd
> > 
> >  .gitignore |   1 +
> >  cfg.mk |   3 +-
> >  daemon/Makefile.am |  33 +
> >  daemon/libvirtd.c  |   2 +-
> >  daemon/libvirtd.h  |  10 --
> >  po/POTFILES.in |   4 +-
> >  src/Makefile.am|  33 -
> >  {daemon => src/admin}/admin_server.c   |   4 +-
> >  {daemon => src/admin}/admin_server.h   |   6 +-
> >  .../admin.c => src/admin/admin_server_dispatch.c   |  21 ++--
> >  .../admin.h => src/admin/admin_server_dispatch.h   |   9 +-
> >  src/libvirt-admin.c|  23 +++-
> >  src/locking/lock_daemon.c  | 132 
> > +++-
> >  src/locking/lock_daemon_config.c   |   3 +
> >  src/locking/lock_daemon_config.h   |   1 +
> >  src/locking/test_virtlockd.aug.in  |   4 +
> >  src/locking/virtlockd-admin.socket.in  |  10 ++
> >  src/locking/virtlockd.aug  |   1 +
> >  src/locking/virtlockd.conf |   6 +
> >  src/locking/virtlockd.service.in   |   1 +
> >  src/logging/log_daemon.c   | 135 
> > +++--
> >  src/logging/log_daemon_config.c|   3 +
> >  src/logging/log_daemon_config.h|   1 +
> >  src/logging/test_virtlogd.aug.in   |   4 +
> >  src/logging/virtlogd-admin.socket.in   |  10 ++
> >  src/logging/virtlogd.aug   |   1 +
> >  src/logging/virtlogd.service.in|   1 +
> >  src/util/virutil.c |  45 +++
> >  src/util/virutil.h |   1 +
> >  29 files changed, 371 insertions(+), 137 deletions(-)
> >  rename {daemon => src/admin}/admin_server.c (99%)
> >  rename {daemon => src/admin}/admin_server.h (96%)
> >  rename daemon/admin.c => src/admin/admin_server_dispatch.c (96%)
> >  rename daemon/admin.h => src/admin/admin_server_dispatch.h (83%)
> >  create mode 100644 src/locking/virtlockd-admin.socket.in
> >  create mode 100644 src/logging/virtlogd-admin.socket.in
> > 
> 
> ACK if you fix all the nits I've found.

I'm going to repost due to the fact that exec-restart is fubar in this
version.

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

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

Re: [libvirt] [PATCH 3/4] logd: add support for admin protocol in virtlogd

[Daniel P. Berrange]

On Mon, Jan 22, 2018 at 04:44:13PM +0100, Michal Privoznik wrote:
> On 01/19/2018 06:09 PM, Daniel P. Berrange wrote:
> > Add a virtlogd-admin-sock can serves the admin protocol for the virtlogd
> > daemon and define a virtlogd:///{system,session}  URI scheme for
> > connecting to it.
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  src/Makefile.am  |   1 +
> >  src/libvirt-admin.c  |  20 --
> >  src/logging/log_daemon.c | 135 
> > ++-
> >  src/logging/log_daemon_config.c  |   3 +
> >  src/logging/log_daemon_config.h  |   1 +
> >  src/logging/test_virtlogd.aug.in |   4 ++
> >  src/logging/virtlogd-admin.socket.in |  10 +++
> >  src/logging/virtlogd.aug |   1 +
> >  src/logging/virtlogd.service.in  |   1 +
> >  9 files changed, 136 insertions(+), 40 deletions(-)
> >  create mode 100644 src/logging/virtlogd-admin.socket.in
> > 
> 
> 
> > diff --git a/src/logging/log_daemon_config.c 
> > b/src/logging/log_daemon_config.c
> > index cf58e6230e..3226b2c484 100644
> > --- a/src/logging/log_daemon_config.c
> > +++ b/src/logging/log_daemon_config.c
> > @@ -73,6 +73,7 @@ virLogDaemonConfigNew(bool privileged ATTRIBUTE_UNUSED)
> >  return NULL;
> >  
> >  data->max_clients = 1024;
> > +data->admin_max_clients = 5000;
> >  data->max_size = 1024 * 1024 * 2;
> >  data->max_backups = 3;
> 
> 5000 seems like huge default. Perhaps 5 is enough? Or 10 so that we
> match the value from the aug test. Which brigns up interesting question
> - how come we don't need src/logging/virtlogd.conf change too? And also,
> how can it be that the aug test doesn't test virtlogd.conf but some
> crafted input? We should have something similar to qemu.conf test.

5000 was blindly copied from libvirtd - it is odd that it doesn't match
the default listed in the ocnfig file there too.

I'll see about fixing the aug test to work like libvirtd's test does.


BTW, other flaw in this patch is that I've broken exec-restarts

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

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

Re: [libvirt] [PATCH 0/4] Add admin protocol support for virtlogd/virtlockd

[Michal Privoznik]

On 01/19/2018 06:09 PM, Daniel P. Berrange wrote:
> The initial admin protocol support was only integrated into libvirtd.
> This series extracts that code so that it is reusable with all the
> daemons we have (and more than we'll get).
> 
> Daniel P. Berrange (4):
>   admin: move admins server impl/dispatch into src/admin directory
>   util: add virGetUNIXSocketPath helper
>   logd: add support for admin protocol in virtlogd
>   lockd: add support for admin protocol in virtlockd
> 
>  .gitignore |   1 +
>  cfg.mk |   3 +-
>  daemon/Makefile.am |  33 +
>  daemon/libvirtd.c  |   2 +-
>  daemon/libvirtd.h  |  10 --
>  po/POTFILES.in |   4 +-
>  src/Makefile.am|  33 -
>  {daemon => src/admin}/admin_server.c   |   4 +-
>  {daemon => src/admin}/admin_server.h   |   6 +-
>  .../admin.c => src/admin/admin_server_dispatch.c   |  21 ++--
>  .../admin.h => src/admin/admin_server_dispatch.h   |   9 +-
>  src/libvirt-admin.c|  23 +++-
>  src/locking/lock_daemon.c  | 132 +++-
>  src/locking/lock_daemon_config.c   |   3 +
>  src/locking/lock_daemon_config.h   |   1 +
>  src/locking/test_virtlockd.aug.in  |   4 +
>  src/locking/virtlockd-admin.socket.in  |  10 ++
>  src/locking/virtlockd.aug  |   1 +
>  src/locking/virtlockd.conf |   6 +
>  src/locking/virtlockd.service.in   |   1 +
>  src/logging/log_daemon.c   | 135 
> +++--
>  src/logging/log_daemon_config.c|   3 +
>  src/logging/log_daemon_config.h|   1 +
>  src/logging/test_virtlogd.aug.in   |   4 +
>  src/logging/virtlogd-admin.socket.in   |  10 ++
>  src/logging/virtlogd.aug   |   1 +
>  src/logging/virtlogd.service.in|   1 +
>  src/util/virutil.c |  45 +++
>  src/util/virutil.h |   1 +
>  29 files changed, 371 insertions(+), 137 deletions(-)
>  rename {daemon => src/admin}/admin_server.c (99%)
>  rename {daemon => src/admin}/admin_server.h (96%)
>  rename daemon/admin.c => src/admin/admin_server_dispatch.c (96%)
>  rename daemon/admin.h => src/admin/admin_server_dispatch.h (83%)
>  create mode 100644 src/locking/virtlockd-admin.socket.in
>  create mode 100644 src/logging/virtlogd-admin.socket.in
> 

ACK if you fix all the nits I've found.

Michal

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

Re: [libvirt] [PATCH 4/4] lockd: add support for admin protocol in virtlockd

[Michal Privoznik]

On 01/19/2018 06:09 PM, Daniel P. Berrange wrote:
> Add a virtlockd-admin-sock can serves the admin protocol for the virtlockd
> daemon and define a virtlockd:///{system,session}  URI scheme for
> connecting to it.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  cfg.mk|   1 -
>  src/Makefile.am   |   1 +
>  src/libvirt-admin.c   |   3 +
>  src/locking/lock_daemon.c | 132 
> +-
>  src/locking/lock_daemon_config.c  |   3 +
>  src/locking/lock_daemon_config.h  |   1 +
>  src/locking/test_virtlockd.aug.in |   4 ++
>  src/locking/virtlockd-admin.socket.in |  10 +++
>  src/locking/virtlockd.aug |   1 +
>  src/locking/virtlockd.conf|   6 ++
>  src/locking/virtlockd.service.in  |   1 +
>  11 files changed, 129 insertions(+), 34 deletions(-)
>  create mode 100644 src/locking/virtlockd-admin.socket.in
> 


> diff --git a/src/locking/lock_daemon_config.c 
> b/src/locking/lock_daemon_config.c
> index 20824b870c..efa5655a30 100644
> --- a/src/locking/lock_daemon_config.c
> +++ b/src/locking/lock_daemon_config.c
> @@ -72,6 +72,7 @@ virLockDaemonConfigNew(bool privileged ATTRIBUTE_UNUSED)
>  return NULL;
>  
>  data->max_clients = 1024;
> +data->admin_max_clients = 5000;

Again, looks like too much. 5 would match value in the config file.

>  
>  return data;
>  }
> @@ -100,6 +101,8 @@ virLockDaemonConfigLoadOptions(virLockDaemonConfigPtr 
> data,
>  return -1;
>  if (virConfGetValueUInt(conf, "max_clients", >max_clients) < 0)
>  return -1;
> +if (virConfGetValueUInt(conf, "admin_max_clients", 
> >admin_max_clients) < 0)
> +return -1;
>  
>  return 0;
>  }
> diff --git a/src/locking/lock_daemon_config.h 
> b/src/locking/lock_daemon_config.h
> index 6ab84c6a0a..3e642208f5 100644
> --- a/src/locking/lock_daemon_config.h
> +++ b/src/locking/lock_daemon_config.h
> @@ -34,6 +34,7 @@ struct _virLockDaemonConfig {
>  char *log_filters;
>  char *log_outputs;
>  unsigned int max_clients;
> +unsigned int admin_max_clients;
>  };
>  
>  
> diff --git a/src/locking/test_virtlockd.aug.in 
> b/src/locking/test_virtlockd.aug.in
> index 799818e5d1..2d69816b5c 100644
> --- a/src/locking/test_virtlockd.aug.in
> +++ b/src/locking/test_virtlockd.aug.in
> @@ -3,6 +3,8 @@ module Test_virtlockd =
>  log_filters=\"3:remote 4:event\"
>  log_outputs=\"3:syslog:libvirtd\"
>  log_buffer_size = 64
> +max_clients = 10
> +admin_max_clients = 10
>  "
>  
> test Virtlockd.lns get conf =
> @@ -10,3 +12,5 @@ log_buffer_size = 64
>  { "log_filters" = "3:remote 4:event" }
>  { "log_outputs" = "3:syslog:libvirtd" }
>  { "log_buffer_size" = "64" }
> + { "max_clients" = "10" }
> + { "admin_max_clients" = "10" }

Please expand the TABs.

Michal

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

Re: [libvirt] [PATCH 5/9] resctrl: Add functions to work with resctrl allocations

[Martin Kletzander]

On Thu, Jan 11, 2018 at 03:58:42PM +0100, Pavel Hrdina wrote:

On Wed, Jan 03, 2018 at 04:05:38PM -0500, John Ferlan wrote:



On 12/13/2017 10:39 AM, Martin Kletzander wrote:
> With this commit we finally have a way to read and manipulate basic resctrl
> settings.  Locking is done only on exposed functions that read/write from/to
> resctrlfs.  Not in functions that are exposed in virresctrlpriv.h as those are
> only supposed to be used from tests.
>
> Signed-off-by: Martin Kletzander 
> ---
>  src/Makefile.am   |2 +-
>  src/libvirt_private.syms  |   11 +
>  src/util/virresctrl.c | 1041 
-
>  src/util/virresctrl.h |   62 +++
>  src/util/virresctrlpriv.h |   27 ++
>  5 files changed, 1141 insertions(+), 2 deletions(-)
>  create mode 100644 src/util/virresctrlpriv.h
>

Geez, as referenced by the cover letter, I'm glad this is the non way
too complicated parts of the code (tongue firmly planted in my cheek
with the obligatory eye roll emoji).

So, IIUC virResctrlInfoPtr is the "host" world and virResctrlAllocPtr is
the "guest" world, right?  If so might be something to add to the commit
messages to make things just slightly more clear.

Lots of code here - hopefully another set of eyes can look at this too -
I'm sure I'll miss something as it's been very difficult to review this
in one (long) session...

Note to self, no sense in grep-ing for "alloc" or "free" any more after
this code is pushed as they're liberally used.  Kind of funny to see
"alloc_free" in the same line ;-)



Feel free to suggest different naming.  My head is so full of this that
I couldn't think of anything more sensible.


The other usage that was really confusing is @cache w/ @[n]masks and
@[n]sizes.  It seems to be used as the array index for both and it's
never really crystal clear over the relationship between masks and sizes.



There's a lot info that I could add in the comments.  And I will for the
next version.


> diff --git a/src/Makefile.am b/src/Makefile.am
> index 4c022d1e44b9..9b4fd0c1d597 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -167,7 +167,7 @@ UTIL_SOURCES = \
>util/virprocess.c util/virprocess.h \
>util/virqemu.c util/virqemu.h \
>util/virrandom.h util/virrandom.c \
> -  util/virresctrl.h util/virresctrl.c \
> +  util/virresctrl.h util/virresctrl.c util/virresctrlpriv.h \
>util/virrotatingfile.h util/virrotatingfile.c \
>util/virscsi.c util/virscsi.h \
>util/virscsihost.c util/virscsihost.h \
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 1a4f304f5e1f..a8009d913669 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2548,6 +2548,17 @@ virRandomInt;
>  # util/virresctrl.h
>  virCacheTypeFromString;
>  virCacheTypeToString;
> +virResctrlAllocAddPID;
> +virResctrlAllocCreate;
> +virResctrlAllocForeachSize;
> +virResctrlAllocFormat;
> +virResctrlAllocGetFree;
> +virResctrlAllocNew;
> +virResctrlAllocRemove;
> +virResctrlAllocSetID;
> +virResctrlAllocSetSize;
> +virResctrlAllocUpdateMask;
> +virResctrlAllocUpdateSize;
>  virResctrlGetInfo;
>  virResctrlInfoGetCache;
>  virResctrlInfoNew;
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index a681322905be..32ab84b6f121 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -18,8 +18,12 @@
>
>  #include 
>
> -#include "virresctrl.h"
> +#include 
> +#include 
> +#include 
> +#include 
>
> +#include "virresctrlpriv.h"
>  #include "c-ctype.h"
>  #include "count-one-bits.h"
>  #include "viralloc.h"
> @@ -151,6 +155,156 @@ virResctrlInfoNew(void)
>  }
>
>
> +/* Alloc-related definitions and AllocClass-related functions */
> +typedef struct _virResctrlAllocPerType virResctrlAllocPerType;
> +typedef virResctrlAllocPerType *virResctrlAllocPerTypePtr;
> +struct _virResctrlAllocPerType {
> +/* There could be bool saying whether this is set or not, but since 
everything
> + * in virResctrlAlloc (and most of libvirt) goes with pointer arrays we 
would
> + * have to have one more level of allocation anyway, so this stays 
faithful to
> + * the concept */
> +unsigned long long **sizes;
> +size_t nsizes;

Perhaps something I should have noted in patch 2 - these @sizes are what
exactly?  Is it a page size or just a "max size" in ?bytes for something
stored in the cache?


It's an array that stores allocation size per CPU cache, since there
can be multiple CPUs in the host system.


> +
> +/* Mask for each cache */
> +virBitmapPtr *masks;
> +size_t nmasks;

And of course, what does the mask represent?


The mask represents how the cache allocation is written into the
schemata file and it is based on the cache allocation size.
Let's say that you have L3 cache with size 15 MiB where granularity
of cache allocation is 768 KiB but minimal allocation is 1536 KiB.
This means that you have 

Re: [libvirt] [PATCH 3/4] logd: add support for admin protocol in virtlogd

[Michal Privoznik]

On 01/19/2018 06:09 PM, Daniel P. Berrange wrote:
> Add a virtlogd-admin-sock can serves the admin protocol for the virtlogd
> daemon and define a virtlogd:///{system,session}  URI scheme for
> connecting to it.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/Makefile.am  |   1 +
>  src/libvirt-admin.c  |  20 --
>  src/logging/log_daemon.c | 135 
> ++-
>  src/logging/log_daemon_config.c  |   3 +
>  src/logging/log_daemon_config.h  |   1 +
>  src/logging/test_virtlogd.aug.in |   4 ++
>  src/logging/virtlogd-admin.socket.in |  10 +++
>  src/logging/virtlogd.aug |   1 +
>  src/logging/virtlogd.service.in  |   1 +
>  9 files changed, 136 insertions(+), 40 deletions(-)
>  create mode 100644 src/logging/virtlogd-admin.socket.in
> 


> diff --git a/src/logging/log_daemon_config.c b/src/logging/log_daemon_config.c
> index cf58e6230e..3226b2c484 100644
> --- a/src/logging/log_daemon_config.c
> +++ b/src/logging/log_daemon_config.c
> @@ -73,6 +73,7 @@ virLogDaemonConfigNew(bool privileged ATTRIBUTE_UNUSED)
>  return NULL;
>  
>  data->max_clients = 1024;
> +data->admin_max_clients = 5000;
>  data->max_size = 1024 * 1024 * 2;
>  data->max_backups = 3;

5000 seems like huge default. Perhaps 5 is enough? Or 10 so that we
match the value from the aug test. Which brigns up interesting question
- how come we don't need src/logging/virtlogd.conf change too? And also,
how can it be that the aug test doesn't test virtlogd.conf but some
crafted input? We should have something similar to qemu.conf test.

>  
> @@ -103,6 +104,8 @@ virLogDaemonConfigLoadOptions(virLogDaemonConfigPtr data,
>  return -1;
>  if (virConfGetValueUInt(conf, "max_clients", >max_clients) < 0)
>  return -1;
> +if (virConfGetValueUInt(conf, "admin_max_clients", 
> >admin_max_clients) < 0)
> +return -1;
>  if (virConfGetValueSizeT(conf, "max_size", >max_size) < 0)
>  return -1;
>  if (virConfGetValueSizeT(conf, "max_backups", >max_backups) < 0)
> diff --git a/src/logging/log_daemon_config.h b/src/logging/log_daemon_config.h
> index 72d77d5e6f..53101b0610 100644
> --- a/src/logging/log_daemon_config.h
> +++ b/src/logging/log_daemon_config.h
> @@ -34,6 +34,7 @@ struct _virLogDaemonConfig {
>  char *log_filters;
>  char *log_outputs;
>  unsigned int max_clients;
> +unsigned int admin_max_clients;
>  
>  size_t max_backups;
>  size_t max_size;
> diff --git a/src/logging/test_virtlogd.aug.in 
> b/src/logging/test_virtlogd.aug.in
> index 3e6888fd48..ee3fae5cde 100644
> --- a/src/logging/test_virtlogd.aug.in
> +++ b/src/logging/test_virtlogd.aug.in
> @@ -2,6 +2,8 @@ module Test_virtlogd =
>let conf = "log_level = 3
>  log_filters=\"3:remote 4:event\"
>  log_outputs=\"3:syslog:virtlogd\"
> +max_clients = 10
> +admin_max_clients = 10
>  max_size = 131072
>  max_backups = 3
>  "
> @@ -10,5 +12,7 @@ max_backups = 3
>  { "log_level" = "3" }
>  { "log_filters" = "3:remote 4:event" }
>  { "log_outputs" = "3:syslog:virtlogd" }
> + { "max_clients" = "10" }
> + { "admin_max_clients" = "10" }

Expand the TABs please.

Michal

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

Re: [libvirt] [PATCH 1/4] admin: move admins server impl/dispatch into src/admin directory

[Michal Privoznik]

On 01/19/2018 06:09 PM, Daniel P. Berrange wrote:
> The admin server functionality is a generic concept that should be wired
> up into all libvirt daemons, but is currently integrated with the
> libvirtd code. Move it all into the src/admin directory to prepare for
> broader reuse.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  .gitignore |  1 +
>  cfg.mk |  2 +-
>  daemon/Makefile.am | 33 
> +-
>  daemon/libvirtd.c  |  2 +-
>  daemon/libvirtd.h  | 10 ---
>  po/POTFILES.in |  4 +--
>  src/Makefile.am| 31 +++-
>  {daemon => src/admin}/admin_server.c   |  4 +--
>  {daemon => src/admin}/admin_server.h   |  6 ++--
>  .../admin.c => src/admin/admin_server_dispatch.c   | 21 +-
>  .../admin.h => src/admin/admin_server_dispatch.h   |  9 +++---
>  11 files changed, 60 insertions(+), 63 deletions(-)
>  rename {daemon => src/admin}/admin_server.c (99%)
>  rename {daemon => src/admin}/admin_server.h (96%)
>  rename daemon/admin.c => src/admin/admin_server_dispatch.c (96%)
>  rename daemon/admin.h => src/admin/admin_server_dispatch.h (83%)

Missed couple of occurrences:

diff --git i/cfg.mk w/cfg.mk
index 4635f9f77..4743f48f3 100644
--- i/cfg.mk
+++ w/cfg.mk
@@ -1119,7 +1119,7 @@ sc_po_check: \
$(srcdir)/daemon/remote_dispatch.h \
$(srcdir)/daemon/qemu_dispatch.h \
$(srcdir)/src/remote/remote_client_bodies.h \
-   $(srcdir)/daemon/admin_dispatch.h \
+   $(srcdir)/src/admin/admin_server_dispatch_stubs.h \
$(srcdir)/src/admin/admin_client.h
 $(srcdir)/daemon/remote_dispatch.h: $(srcdir)/src/remote/remote_protocol.x
$(MAKE) -C daemon remote_dispatch.h
@@ -1127,8 +1127,8 @@ $(srcdir)/daemon/qemu_dispatch.h: 
$(srcdir)/src/remote/qemu_protocol.x
$(MAKE) -C daemon qemu_dispatch.h
 $(srcdir)/src/remote/remote_client_bodies.h: 
$(srcdir)/src/remote/remote_protocol.x
$(MAKE) -C src remote/remote_client_bodies.h
-$(srcdir)/daemon/admin_dispatch.h: $(srcdir)/src/admin/admin_protocol.x
-   $(MAKE) -C daemon admin_dispatch.h
+   $(srcdir)/src/admin/admin_server_dispatch_stubs.h: 
$(srcdir)/src/admin/admin_protocol.x
+   $(MAKE) -C src admin/admin_server_dispatch_stubs.h
 $(srcdir)/src/admin/admin_client.h: $(srcdir)/src/admin/admin_protocol.x
$(MAKE) -C src admin/admin_client.h
 
diff --git i/po/POTFILES.in w/po/POTFILES.in
index 4c0abb18a..106b26277 100644
--- i/po/POTFILES.in
+++ w/po/POTFILES.in
@@ -1,4 +1,3 @@
-daemon/admin_dispatch.h
 daemon/libvirtd-config.c
 daemon/libvirtd.c
 daemon/qemu_dispatch.h



Michal

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

Re: [libvirt] [PATCH 2/4] util: add virGetUNIXSocketPath helper

[Michal Privoznik]

On 01/19/2018 06:09 PM, Daniel P. Berrange wrote:
> When receiving multiple socket FDs from systemd, it is critical to know
> what socket address each corresponds to so we can setup the right
> protocols on each.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/util/virutil.c | 45 +
>  src/util/virutil.h |  1 +
>  2 files changed, 46 insertions(+)

Don't forget to actually expose the symbol:

diff --git i/src/libvirt_private.syms w/src/libvirt_private.syms
index bc8cc1fba..54e3b9130 100644
--- i/src/libvirt_private.syms
+++ w/src/libvirt_private.syms
@@ -2962,6 +2962,7 @@ virGetListenFDs;
 virGetSelfLastChanged;
 virGetSystemPageSize;
 virGetSystemPageSizeKB;
+virGetUNIXSocketPath;
 virGetUnprivSGIOSysfsPath;
 virGetUserCacheDirectory;
 virGetUserConfigDirectory;


Michal

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

Re: [libvirt] [PATCH v2] virsh: Checking the volume capacity before uploading a new file.

[Peter Krempa]

On Sun, Jan 21, 2018 at 21:51:05 -0200, Julio Faracco wrote:
> The current command 'vol-upload' is not checking if the volume accepts
> a file bigger than its capacity. It can cause an interrupt of the
> upload stream. This commit adds a check that fails before starting to
> send new file to volume if the file is bigger.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1529059
> 
> Signed-off-by: Julio Faracco 
> ---
>  tools/virsh-volume.c | 25 +
>  1 file changed, 25 insertions(+)
> 
> diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
> index 8265a39..04e480c 100644
> --- a/tools/virsh-volume.c
> +++ b/tools/virsh-volume.c
> @@ -672,6 +672,7 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd)
>  {
>  const char *file = NULL;
>  virStorageVolPtr vol = NULL;
> +virStorageVolInfo volumeInfo;
>  bool ret = false;
>  int fd = -1;
>  virStreamPtr st = NULL;
> @@ -679,6 +680,7 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd)
>  unsigned long long offset = 0, length = 0;
>  virshControlPtr priv = ctl->privData;
>  unsigned int flags = 0;
> +off_t fileLen = -1;
>  virshStreamCallbackData cbData;
>  
>  if (vshCommandOptULongLong(ctl, cmd, "offset", ) < 0)
> @@ -701,6 +703,29 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd)
>  cbData.ctl = ctl;
>  cbData.fd = fd;
>  
> +if (virStorageVolGetInfo(vol, ) < 0)
> +goto cleanup;
> +
> +if ((fileLen = virFileLength(file, fd)) < 0) {
> +vshError(ctl, _("cannot get the file %s length"), file);

This is wrong since you might want to upload contents of a block device,
in which case you'd report this error.

> +goto cleanup;
> +}
> +
> +if (length < fileLen) {

I think this is wrong. The 'length' parameter was designed to allow
upload of a partial file. Adding this would break it.

> +vshError(ctl, _("length parameter is smaller than file size"));
> +goto cleanup;
> +}


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

Re: [libvirt] [PATCH] libvirtd: Explicit dependency on systemd-machined

[Michal Privoznik]

On 01/22/2018 11:10 AM, Cedric Bosdonnat wrote:
> The patch looks good to me, though I'm not a systemd expert.
> If no one vetoes it I'll push your patch by the end of the day.

Yup ACK.

Sorry, I had this marked for review but got buried under some other
stuff. Cedric feel free to push and don't forget to congratulate Michal
on this first contribution ;-)

Michal

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

Re: [libvirt] [PATCH 2/9] util: Add virResctrlInfo

[Martin Kletzander]

On Tue, Jan 02, 2018 at 03:08:30PM -0500, John Ferlan wrote:



On 12/13/2017 10:39 AM, Martin Kletzander wrote:

This will make the current functions obsolete and it will provide more
information to the virresctrl module so that it can be used later.

Signed-off-by: Martin Kletzander 
---
 po/POTFILES.in   |   1 +
 src/libvirt_private.syms |   3 +
 src/util/virresctrl.c| 301 +++
 src/util/virresctrl.h|  19 +++
 4 files changed, 324 insertions(+)

diff --git a/po/POTFILES.in b/po/POTFILES.in
index c1fa23427eff..8382ee633621 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -252,6 +252,7 @@ src/util/virportallocator.c
 src/util/virprocess.c
 src/util/virqemu.c
 src/util/virrandom.c
+src/util/virresctrl.c
 src/util/virrotatingfile.c
 src/util/virscsi.c
 src/util/virscsihost.c
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index de4ec4d442c9..75be612a2f13 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2550,6 +2550,9 @@ virCacheTypeFromString;
 virCacheTypeToString;
 virResctrlGetCacheControlType;
 virResctrlGetCacheInfo;
+virResctrlGetInfo;
+virResctrlInfoGetCache;
+virResctrlInfoNew;


 # util/virrotatingfile.h
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 050a08178e24..6fd1ceb587cf 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -25,12 +25,15 @@
 #include "viralloc.h"
 #include "virfile.h"
 #include "virlog.h"
+#include "virobject.h"
 #include "virstring.h"

 #define VIR_FROM_THIS VIR_FROM_RESCTRL

 VIR_LOG_INIT("util.virresctrl")

+
+/* Common definitions */
 #define SYSFS_RESCTRL_PATH "/sys/fs/resctrl"

 /* Resctrl is short for Resource Control.  It might be implemented for various
@@ -55,6 +58,304 @@ VIR_ENUM_IMPL(virResctrl, VIR_CACHE_TYPE_LAST,
   "CODE",
   "DATA")

+
+/* Info-related definitions and InfoClass-related functions */
+typedef struct _virResctrlInfoPerType virResctrlInfoPerType;
+typedef virResctrlInfoPerType *virResctrlInfoPerTypePtr;
+struct _virResctrlInfoPerType {
+/* Kernel-provided information */
+char *cbm_mask;
+unsigned int min_cbm_bits;
+
+/* Our computed information from the above */
+unsigned int bits;
+unsigned int max_cache_id;
+
+/* In order to be self-sufficient we need size information per cache.
+ * Funnily enough, one of the outcomes of the resctrlfs design is that it
+ * does not account for different sizes per cache on the same level.  So
+ * for the sake of easiness, let's copy that, for now. */
+unsigned long long size;
+
+/* Information that we will return upon request (this is public struct) as
+ * until now all the above is internal to this module */
+virResctrlInfoPerCache control;
+};
+
+typedef struct _virResctrlInfoPerLevel virResctrlInfoPerLevel;
+typedef virResctrlInfoPerLevel *virResctrlInfoPerLevelPtr;
+struct _virResctrlInfoPerLevel {
+virResctrlInfoPerTypePtr *types;
+};
+
+struct _virResctrlInfo {
+virObject parent;
+
+virResctrlInfoPerLevelPtr *levels;
+size_t nlevels;
+};
+
+static virClassPtr virResctrlInfoClass;
+
+static void
+virResctrlInfoDispose(void *obj)
+{
+size_t i = 0;
+size_t j = 0;
+
+virResctrlInfoPtr resctrl = obj;
+
+for (i = 0; i < resctrl->nlevels; i++) {
+virResctrlInfoPerLevelPtr level = resctrl->levels[i];
+
+if (!level)
+continue;
+
+if (level->types) {
+for (j = 0; j < VIR_CACHE_TYPE_LAST; j++) {
+if (level->types[j])
+VIR_FREE(level->types[j]->cbm_mask);
+VIR_FREE(level->types[j]);
+}
+}
+VIR_FREE(level->types);
+VIR_FREE(level);
+}
+
+VIR_FREE(resctrl->levels);
+}
+
+
+static int virResctrlInfoOnceInit(void)


static int
virRes


+{
+if (!(virResctrlInfoClass = virClassNew(virClassForObject(),
+"virResctrlInfo",
+sizeof(virResctrlInfo),
+virResctrlInfoDispose)))
+return -1;
+
+return 0;
+}
+
+
+VIR_ONCE_GLOBAL_INIT(virResctrlInfo)
+
+
+virResctrlInfoPtr
+virResctrlInfoNew(void)
+{
+if (virResctrlInfoInitialize() < 0)
+return NULL;
+
+return virObjectNew(virResctrlInfoClass);
+}
+
+
+/* Info-related functions */
+bool
+virResctrlInfoIsEmpty(virResctrlInfoPtr resctrl)
+{
+size_t i = 0;
+size_t j = 0;
+
+if (!resctrl)
+return true;
+
+for (i = 0; i < resctrl->nlevels; i++) {
+virResctrlInfoPerLevelPtr i_level = resctrl->levels[i];
+
+if (!i_level)
+continue;
+
+for (j = 0; j < VIR_CACHE_TYPE_LAST; j++) {
+if (i_level->types[j])
+return false;
+}
+}
+
+return true;
+}
+


Two blank lines


+int
+virResctrlGetInfo(virResctrlInfoPtr resctrl)
+{
+

Re: [libvirt] [PATCH] qemu: Don't initialize struct utsname

[Daniel P. Berrange]

On Mon, Jan 22, 2018 at 03:12:46PM +0100, Jiri Denemark wrote:
> On Mon, Jan 22, 2018 at 14:01:26 +, Daniel P. Berrange wrote:
> > On Mon, Jan 22, 2018 at 02:54:42PM +0100, Jiri Denemark wrote:
> > > It breaks the build and it is not really useful for anything.
> > 
> > Why does that break the build ?   "{ 0 }" is a valid initializer for
> > any struct according to the C standards.
> 
> Yeah, it seems -Wmissing-braces falsely reports missing braces in there.
> It worked just fine on my host, by our Jenkins builders all failed on
> it. Anyway, I didn't bother looking at details as it's easier to drop
> the initializer. And doing so makes usage of struct utsname in libvirt
> consistent.

Looks like it generates the bogus warning when the first field of the
struct is itself another struct.

eg

  struct Base {
 int a;
  }

  struct One {
 int b;
 struct base c;
  };

  struct Two {
 struct base c;
 int b;
  };


  struct One one = {0};
  struct Two two = {0};


The 'two' initializer generates a warning but the 'one' initializer
does not. So that explains why most of our {0} initalizers are working
ok at least.

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

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

Re: [libvirt] [PATCH] qemu: Don't initialize struct utsname

[Jiri Denemark]

On Mon, Jan 22, 2018 at 14:01:26 +, Daniel P. Berrange wrote:
> On Mon, Jan 22, 2018 at 02:54:42PM +0100, Jiri Denemark wrote:
> > It breaks the build and it is not really useful for anything.
> 
> Why does that break the build ?   "{ 0 }" is a valid initializer for
> any struct according to the C standards.

Yeah, it seems -Wmissing-braces falsely reports missing braces in there.
It worked just fine on my host, by our Jenkins builders all failed on
it. Anyway, I didn't bother looking at details as it's easier to drop
the initializer. And doing so makes usage of struct utsname in libvirt
consistent.

Jirka

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

Re: [libvirt] [PATCH] Raise the frame limit for tests

[Michal Privoznik]

On 01/22/2018 02:34 PM, Daniel P. Berrange wrote:
> On Mon, Jan 22, 2018 at 02:28:42PM +0100, Martin Kletzander wrote:
>> On Mon, Jan 22, 2018 at 12:57:31PM +, Daniel P. Berrange wrote:
>>> On Mon, Jan 22, 2018 at 01:54:28PM +0100, Martin Kletzander wrote:
 On Mon, Jan 22, 2018 at 01:47:24PM +0100, Michal Privoznik wrote:
> On 01/22/2018 01:22 PM, Daniel P. Berrange wrote:
>> On Mon, Jan 22, 2018 at 12:49:12PM +0100, Martin Kletzander wrote:
>>> On Thu, Jan 18, 2018 at 10:16:55AM +0100, Ján Tomko wrote:
 After the latest CPU additions, the build fails with clang:
 cputest.c:905:1: error: stack frame size of 26136 bytes
  in function 'mymain' [-Werror,-Wframe-larger-than=]

 Raise the relaxed limit which is used for tests.
 ---
 m4/virt-compile-warnings.m4 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

 Pushed as a build breaker fix

 diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4
 index f18a08a8f..b9c974842 100644
 --- a/m4/virt-compile-warnings.m4
 +++ b/m4/virt-compile-warnings.m4
 @@ -200,7 +200,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
 # but using 1024 bytes sized buffers (mostly for virStrerror)
 # stops us from going down further
 gl_WARN_ADD([-Wframe-larger-than=4096], 
 [STRICT_FRAME_LIMIT_CFLAGS])
 -gl_WARN_ADD([-Wframe-larger-than=25600], 
 [RELAXED_FRAME_LIMIT_CFLAGS])
 +gl_WARN_ADD([-Wframe-larger-than=32768], 
 [RELAXED_FRAME_LIMIT_CFLAGS])

>>>
>>> Remind me again why don't we do -Wno-frame-larger-than (or something to 
>>> that
>>> effect) for tests?  Is it just because "We should fix it at some 
>>> point"?  I
>>> can't really recall the reasoning behind that (and if it is still 
>>> valid) even
>>> though I already asked for it.
>>
>> I don't think there's a strong reason, given the way we currently write
>> tests with huge amounts of stack variables.
>>
>> For -Wframe-larger-than to be useful, we'd need to move all the big data
>> blobs to be static, global variables.
> Or simply use compiler that honours variable lifetime. If a variable is
> defined only in a block, compiler should be able to just reuse the
> stack. I mean for the following case:
>
> do {
>  int x;
> } while (0);
>
> do {
>  int y;
> } while (0);
>
> I don't see any compelling reason for compiler to reserve two ints on
> the stack. Or if it does, count it as one when comparing agains
> -Wframe-larger-than.
>

 We can do that ourselves, even though it's not really great thing to do.  
 Just
 reset the one struct and reuse it.  I added it (and future research) as an 
 idea
 to GSoC ideas.  Let's see if someone rewrites that.
>>>
>>> Is it really worth the effort though?  It is important for the core library
>>> because we have a unimaginable set of code paths that are hard to validate,
>>> so keeping stack use low is key to minimize risk fo stack exhaustion. In the
>>> test suite, however, we have basically 1-3 call frames and stack exhaustion
>>> is a non-issue - the test would merely crash & not have any bad 
>>> consequences.
>>>
>>
>> There are two points for this.  1) It can drive someone to start 
>> contributing to
>> libvirt by starting off easily, and 2) it can then help with assessing ways 
>> how
>> we can make the library frame sizes smaller.
>>
>> So if there is no point in this for tests, as you said, we're back to my
>> original question.  Why to have this when we just randomly increase it?
> 
> As I said, I don't see any real point it in - we might as well just use
> the -Wno-frame-larger-than flag.
> 

Agreed. tests/ shouldn't use -Wframe-lager-than. Nor examples/ perhaps
(although those are so small that they are never going to hit the limit).

Michal

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

Re: [libvirt] [PATCH hooks 1/1] Add check for Signed-off-by in commit messages

[Michal Privoznik]

On 01/22/2018 02:20 PM, Peter Krempa wrote:
> On Mon, Jan 22, 2018 at 13:06:28 +, Daniel Berrange wrote:
>> On Mon, Jan 22, 2018 at 01:20:12PM +0100, Peter Krempa wrote:
>>> On Mon, Jan 22, 2018 at 12:05:19 +, Daniel Berrange wrote:
 This extends the update hook so that it enforces a requirement to have a
 Signed-off-by line in every commit message. This can be optionally
 turned off in individual repos by setting the "hooks.allowmissingsob"
 git config variable.

 Signed-off-by: Daniel P. Berrange 
 ---
  update | 16 +++-
  1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> NACK, I don't like signoffs and I don't really think they achieve
>>> anything.
>>
>> A signoff with no documented meaning attached by the project is fairly
>> weak, as you would have to argue there was some commonly accepted signifance
>> to it across the community. A signoff which is explicitly associated with a
>> statement of intent has benefit, hence why I also sent a patch to clarify
>> that the signoff asserts compliance with the DCO. Adding these signoffs has
> 
> You can do the same by declaring that all patches have to comply to that
> and not mandating adding a line which will become eventually pointless
> since every patch will need to have it.
> 
> Also since there's no way to check that it's actually true, anybody can
> declare anything.

Sure, but then the burden is on you because you declared that you comply
with something by including that line while you cannot. I am no lawyer,
but if it can help us be stronger against law suits then so be it.

> Also the check itself can be fooled easily, so I think
> it's pointless altogether.
> 
>> little to no time burden on long term developers own work on a daily basis.
>> Just needs adding -s to "git commit" which quickly becomes engrained in
>> muscle memory such that you'll end up doing it for every project you find
> 
> Or you can defeat it entirely by adding it into your commit message
> template. I don't care about the added burden though. I don't really
> think it achieves anything.
> 
>> yourself contributing to. Many of our "drive by" contributors already do
>> this as habit, we'll just need to remind those that forget periodically.
> 
> I presonally signed-off < 5 commits in libvirt. So the last statement
> is untrue. Some people don't do it on purpose.
> 
> The sign-off by itself (whithout cryptographic signature) is just
> pointless. Validity with a cryptographic signature from drive-by
> contributors can still be unproven, but at least you don't get
> impersonation.
> 
> If everything is signed off, nothing really is.

Not true. I view DCO as an agreement between Libvirt entity (whomever
represented by) and individual contributor that they are allowed by they
employer (or copyright holder) to send the patch. And thus, if the
agreement is signed more than once, it doesn't matter (although,
employees, occupancies, company cultures change through time). It's
still valid. But if it is not signed at all that's the problem.

Also, we will surely never experience this at Red Hat, but I've heard
many urban legends when an employee was not allowed to send a patch to
the upstream (because of some corporate politics or whatever). And I
think those are the cases we should protect us from.

Michal

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

Re: [libvirt] [PATCH] qemu: Don't initialize struct utsname

[Daniel P. Berrange]

On Mon, Jan 22, 2018 at 02:54:42PM +0100, Jiri Denemark wrote:
> It breaks the build and it is not really useful for anything.

Why does that break the build ?   "{ 0 }" is a valid initializer for
any struct according to the C standards.

> 
> Signed-off-by: Jiri Denemark 
> ---
> 
> Pushed under the build breaker rule.
> 
>  src/qemu/qemu_capabilities.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 2c573827f1..5e03447baa 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -5431,7 +5431,7 @@ virQEMUCapsCacheNew(const char *libDir,
>  char *capsCacheDir = NULL;
>  virFileCachePtr cache = NULL;
>  virQEMUCapsCachePrivPtr priv = NULL;
> -struct utsname uts = { 0 };
> +struct utsname uts;
>  
>  if (virAsprintf(, "%s/capabilities", cacheDir) < 0)
>  goto error;
> -- 
> 2.16.0
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

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

Re: [libvirt] [PATCH hooks 1/1] Add check for Signed-off-by in commit messages

[John Ferlan]

On 01/22/2018 07:05 AM, Daniel P. Berrange wrote:
> This extends the update hook so that it enforces a requirement to have a
> Signed-off-by line in every commit message. This can be optionally
> turned off in individual repos by setting the "hooks.allowmissingsob"
> git config variable.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  update | 16 +++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 

Count me in as in favor of this.  I have only ever had to add:

[format]
signoff = true

to my $HOME/.gitconfig once and it's been in place ever since.  This is
far less than the:

[user] and [sendemail] sections and the same amount of additions to/for
the [push] section.  One extra line.

It's also less than anything I've had to add to my repository specific
.git/config files.

It's also easier than the :

Reviewed-by: John Ferlan 

line that I keep in a "cheats" file that I always have open so that I
don't have to add it for patches I review.  Of course, I could also just
say ACK, but the R-b seems so much more authoritative.  Now if I could
only remember or figure out a way to add it to any patches I push
without having to remember to go back and rebase --interactive to add it
(which I rarely ever do).

John

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

Re: [libvirt] [PATCH hooks 1/1] Add check for Signed-off-by in commit messages

[Daniel P. Berrange]

On Mon, Jan 22, 2018 at 02:20:52PM +0100, Peter Krempa wrote:
> On Mon, Jan 22, 2018 at 13:06:28 +, Daniel Berrange wrote:
> > On Mon, Jan 22, 2018 at 01:20:12PM +0100, Peter Krempa wrote:
> > > On Mon, Jan 22, 2018 at 12:05:19 +, Daniel Berrange wrote:
> > > > This extends the update hook so that it enforces a requirement to have a
> > > > Signed-off-by line in every commit message. This can be optionally
> > > > turned off in individual repos by setting the "hooks.allowmissingsob"
> > > > git config variable.
> > > > 
> > > > Signed-off-by: Daniel P. Berrange 
> > > > ---
> > > >  update | 16 +++-
> > > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > NACK, I don't like signoffs and I don't really think they achieve
> > > anything.
> > 
> > A signoff with no documented meaning attached by the project is fairly
> > weak, as you would have to argue there was some commonly accepted signifance
> > to it across the community. A signoff which is explicitly associated with a
> > statement of intent has benefit, hence why I also sent a patch to clarify
> > that the signoff asserts compliance with the DCO. Adding these signoffs has
> 
> You can do the same by declaring that all patches have to comply to that
> and not mandating adding a line which will become eventually pointless
> since every patch will need to have it.
> 
> Also since there's no way to check that it's actually true, anybody can
> declare anything. Also the check itself can be fooled easily, so I think
> it's pointless altogether.

The git hook isn't expected to be flawless. Whomever is pushing the patches
to git is still actually reviewing the submissions. If someone puts garbage
there it will still likely be caught just as if someone puts garbage in the
actual code.

> > little to no time burden on long term developers own work on a daily basis.
> > Just needs adding -s to "git commit" which quickly becomes engrained in
> > muscle memory such that you'll end up doing it for every project you find
> 
> Or you can defeat it entirely by adding it into your commit message
> template. I don't care about the added burden though. I don't really
> think it achieves anything.

So we can weigh up the possible outcomes. It it adopt it and it turns
out to not do anything, then we've merely suffered insigifincant burden
of adding it.  If we don't adopt it, and it does turn out to be important
the consequences may we quite severe.  It makes no prudent sense to
not adopt it, given the minimal burden it has and the potentially significant
upside.

> > yourself contributing to. Many of our "drive by" contributors already do
> > this as habit, we'll just need to remind those that forget periodically.
> 
> I presonally signed-off < 5 commits in libvirt. So the last statement
> is untrue. Some people don't do it on purpose.
> 
> The sign-off by itself (whithout cryptographic signature) is just
> pointless. Validity with a cryptographic signature from drive-by
> contributors can still be unproven, but at least you don't get
> impersonation.

I think these are two different axis. The sob isn't trying to address the
question of impersonation. It obviously has as a starting point that you
accept the identity of the submitter to some degree. I accept that if you
have cryptographically signed patches, that would give a stronger
validation of identity, but there's never any absolutes. So not having
a crypto signature doesn't make the sob invalid.

> If everything is signed off, nothing really is.

I don't really see that.

> NACK still stands.

You are nacking something that you've accepted above will have no negative
impact on your work, but has potentially significant upside to the project.
That is very disappointing.

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

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

[libvirt] [PATCH] qemu: Don't initialize struct utsname

[Jiri Denemark]

It breaks the build and it is not really useful for anything.

Signed-off-by: Jiri Denemark 
---

Pushed under the build breaker rule.

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

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 2c573827f1..5e03447baa 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -5431,7 +5431,7 @@ virQEMUCapsCacheNew(const char *libDir,
 char *capsCacheDir = NULL;
 virFileCachePtr cache = NULL;
 virQEMUCapsCachePrivPtr priv = NULL;
-struct utsname uts = { 0 };
+struct utsname uts;
 
 if (virAsprintf(, "%s/capabilities", cacheDir) < 0)
 goto error;
-- 
2.16.0

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

Re: [libvirt] [PATCH hooks 1/1] Add check for Signed-off-by in commit messages

[Daniel P. Berrange]

On Mon, Jan 22, 2018 at 02:24:40PM +0100, Ján Tomko wrote:
> On Mon, Jan 22, 2018 at 12:42:47PM +, Daniel P. Berrange wrote:
> > On Mon, Jan 22, 2018 at 01:22:01PM +0100, Ján Tomko wrote:
> > > On Mon, Jan 22, 2018 at 12:05:19PM +, Daniel P. Berrange wrote:
> > > > This extends the update hook so that it enforces a requirement to have a
> > > > Signed-off-by line in every commit message. This can be optionally
> > > > turned off in individual repos by setting the "hooks.allowmissingsob"
> > > > git config variable.
> > > >
> > > > Signed-off-by: Daniel P. Berrange 
> > > > ---
> > > > update | 16 +++-
> > > > 1 file changed, 15 insertions(+), 1 deletion(-)
> > > >
> > > 
> > > Given that signed-off-by lines are pointless for patches authored and
> > > committed by the same person,
> > 
> > They are not pointless. They provide an explicit assertion that the author
> > is acknowledged they are permitted to make the contribution under the 
> > project's
> > license. This is distinct from the Author/Committer info in the commit, 
> > because
> > that is added automatically my git with no thought required by the 
> > developer.
> > 
> 
> I refuse to believe that a group of programmers is incapable of automating 
> such
> mundane process.
> 
> Also, adding -s to the command line by muscle memory is by definition a
> no-thought process.

Those who are regular contributors to open source with sob understand what
it means. The infrequent or first time contributors would be those who are
making an explicit thought aobut it.

> > > NACK unless the hooks.allowmissingsob will be set in the main
> > > libvirt.git (I don't really care about other repos).
> > 
> > I'm intending it to be set in *every* repository include libvirt.git. There
> > is no real world burden for developers to add a signed-off-by line to the
> > commits they contribute to the project,
> 
> It adds visual clutter and one unnecessary step.

There's no real extra step here - its just a tweak to the existing commit
step, so doesn't add any burden. The "visual clutter" is not a relevant
point imho, not least because it is already widely present across our
commits.

> > and it puts us in a stronger legal
> > position going forward. It is already commonplace across countless open
> > source projects, including many that we interact & build on in libvirt.
> > 
> 
> Yes, and the amount of red tape required to contribute is off-putting.
> It saddens me to see libvirt go that route.

I think this is really overstating the impact of the change. Compared to the
extensive code style guidelines that users must follow when writing code,
this change will have no measurable negative impact on work involved in
contributing to libvirt.

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

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

Re: [libvirt] [PATCH] Raise the frame limit for tests

[Daniel P. Berrange]

On Mon, Jan 22, 2018 at 02:28:42PM +0100, Martin Kletzander wrote:
> On Mon, Jan 22, 2018 at 12:57:31PM +, Daniel P. Berrange wrote:
> > On Mon, Jan 22, 2018 at 01:54:28PM +0100, Martin Kletzander wrote:
> > > On Mon, Jan 22, 2018 at 01:47:24PM +0100, Michal Privoznik wrote:
> > > > On 01/22/2018 01:22 PM, Daniel P. Berrange wrote:
> > > > > On Mon, Jan 22, 2018 at 12:49:12PM +0100, Martin Kletzander wrote:
> > > > > > On Thu, Jan 18, 2018 at 10:16:55AM +0100, Ján Tomko wrote:
> > > > > > > After the latest CPU additions, the build fails with clang:
> > > > > > > cputest.c:905:1: error: stack frame size of 26136 bytes
> > > > > > >  in function 'mymain' [-Werror,-Wframe-larger-than=]
> > > > > > >
> > > > > > > Raise the relaxed limit which is used for tests.
> > > > > > > ---
> > > > > > > m4/virt-compile-warnings.m4 | 2 +-
> > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > Pushed as a build breaker fix
> > > > > > >
> > > > > > > diff --git a/m4/virt-compile-warnings.m4 
> > > > > > > b/m4/virt-compile-warnings.m4
> > > > > > > index f18a08a8f..b9c974842 100644
> > > > > > > --- a/m4/virt-compile-warnings.m4
> > > > > > > +++ b/m4/virt-compile-warnings.m4
> > > > > > > @@ -200,7 +200,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
> > > > > > > # but using 1024 bytes sized buffers (mostly for virStrerror)
> > > > > > > # stops us from going down further
> > > > > > > gl_WARN_ADD([-Wframe-larger-than=4096], 
> > > > > > > [STRICT_FRAME_LIMIT_CFLAGS])
> > > > > > > -gl_WARN_ADD([-Wframe-larger-than=25600], 
> > > > > > > [RELAXED_FRAME_LIMIT_CFLAGS])
> > > > > > > +gl_WARN_ADD([-Wframe-larger-than=32768], 
> > > > > > > [RELAXED_FRAME_LIMIT_CFLAGS])
> > > > > > >
> > > > > >
> > > > > > Remind me again why don't we do -Wno-frame-larger-than (or 
> > > > > > something to that
> > > > > > effect) for tests?  Is it just because "We should fix it at some 
> > > > > > point"?  I
> > > > > > can't really recall the reasoning behind that (and if it is still 
> > > > > > valid) even
> > > > > > though I already asked for it.
> > > > >
> > > > > I don't think there's a strong reason, given the way we currently 
> > > > > write
> > > > > tests with huge amounts of stack variables.
> > > > >
> > > > > For -Wframe-larger-than to be useful, we'd need to move all the big 
> > > > > data
> > > > > blobs to be static, global variables.
> > > > Or simply use compiler that honours variable lifetime. If a variable is
> > > > defined only in a block, compiler should be able to just reuse the
> > > > stack. I mean for the following case:
> > > >
> > > > do {
> > > >  int x;
> > > > } while (0);
> > > >
> > > > do {
> > > >  int y;
> > > > } while (0);
> > > >
> > > > I don't see any compelling reason for compiler to reserve two ints on
> > > > the stack. Or if it does, count it as one when comparing agains
> > > > -Wframe-larger-than.
> > > >
> > > 
> > > We can do that ourselves, even though it's not really great thing to do.  
> > > Just
> > > reset the one struct and reuse it.  I added it (and future research) as 
> > > an idea
> > > to GSoC ideas.  Let's see if someone rewrites that.
> > 
> > Is it really worth the effort though?  It is important for the core library
> > because we have a unimaginable set of code paths that are hard to validate,
> > so keeping stack use low is key to minimize risk fo stack exhaustion. In the
> > test suite, however, we have basically 1-3 call frames and stack exhaustion
> > is a non-issue - the test would merely crash & not have any bad 
> > consequences.
> > 
> 
> There are two points for this.  1) It can drive someone to start contributing 
> to
> libvirt by starting off easily, and 2) it can then help with assessing ways 
> how
> we can make the library frame sizes smaller.
> 
> So if there is no point in this for tests, as you said, we're back to my
> original question.  Why to have this when we just randomly increase it?

As I said, I don't see any real point it in - we might as well just use
the -Wno-frame-larger-than flag.

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

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

Re: [libvirt] [PATCH] Raise the frame limit for tests

[Martin Kletzander]

On Mon, Jan 22, 2018 at 02:01:27PM +0100, Michal Privoznik wrote:

On 01/22/2018 01:54 PM, Martin Kletzander wrote:

On Mon, Jan 22, 2018 at 01:47:24PM +0100, Michal Privoznik wrote:

On 01/22/2018 01:22 PM, Daniel P. Berrange wrote:

On Mon, Jan 22, 2018 at 12:49:12PM +0100, Martin Kletzander wrote:

On Thu, Jan 18, 2018 at 10:16:55AM +0100, Ján Tomko wrote:

After the latest CPU additions, the build fails with clang:
cputest.c:905:1: error: stack frame size of 26136 bytes
 in function 'mymain' [-Werror,-Wframe-larger-than=]

Raise the relaxed limit which is used for tests.
---
m4/virt-compile-warnings.m4 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Pushed as a build breaker fix

diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4
index f18a08a8f..b9c974842 100644
--- a/m4/virt-compile-warnings.m4
+++ b/m4/virt-compile-warnings.m4
@@ -200,7 +200,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
    # but using 1024 bytes sized buffers (mostly for virStrerror)
    # stops us from going down further
    gl_WARN_ADD([-Wframe-larger-than=4096],
[STRICT_FRAME_LIMIT_CFLAGS])
-    gl_WARN_ADD([-Wframe-larger-than=25600],
[RELAXED_FRAME_LIMIT_CFLAGS])
+    gl_WARN_ADD([-Wframe-larger-than=32768],
[RELAXED_FRAME_LIMIT_CFLAGS])



Remind me again why don't we do -Wno-frame-larger-than (or something
to that
effect) for tests?  Is it just because "We should fix it at some
point"?  I
can't really recall the reasoning behind that (and if it is still
valid) even
though I already asked for it.


I don't think there's a strong reason, given the way we currently write
tests with huge amounts of stack variables.

For -Wframe-larger-than to be useful, we'd need to move all the big data
blobs to be static, global variables.

Or simply use compiler that honours variable lifetime. If a variable is
defined only in a block, compiler should be able to just reuse the
stack. I mean for the following case:

do {
 int x;
} while (0);

do {
 int y;
} while (0);

I don't see any compelling reason for compiler to reserve two ints on
the stack. Or if it does, count it as one when comparing agains
-Wframe-larger-than.



We can do that ourselves, even though it's not really great thing to
do.  Just
reset the one struct and reuse it.  I added it (and future research) as
an idea
to GSoC ideas.  Let's see if someone rewrites that.


I don't think that's a good idea. It's working around broken compiler.
Just like the original patch (which we unfortunately have to have).



Really?  Well, looks like you're not the only one, so feel free to remove that.
I just thought that would be a really easy starting point for someone.


Michal

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


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

Re: [libvirt] [PATCH] docs: document requirement to provide Signed-off-by lines for DCO

[Andrea Bolognani]

On Mon, 2018-01-22 at 12:40 +, Daniel P. Berrange wrote:
> Document that contributors are required to assert compliance with the
> Developers Certification of Origin 1.1, by providing Signed-off-by tags

Both here...

> for all commit messages. The DCO is formally stating what we have long
> implicitly expected of contributors in terms of their legal rights to
> make the contribution. This puts the project in a stronger position
> should any questions around contributions be raised going forward in the
> future.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  docs/hacking.html.in | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/hacking.html.in b/docs/hacking.html.in
> index 6cadfb8343..98a24d785c 100644
> --- a/docs/hacking.html.in
> +++ b/docs/hacking.html.in
> @@ -108,12 +108,18 @@
>of the bug number is useful; but also summarize the issue
>rather than making all readers follow the link.  You can use
>'git shortlog -30' to get an idea of typical summary lines.
> -  Libvirt does not currently attach any meaning to
> -  Signed-off-by: lines, so it is up to you if you want to
> -  include or omit them in the commit message.
>  
>
>  
> +  Contributors to libvirt projects must
> +  assert that they are in compliance with the
> +  https://developercertificate.org/;>Developers
> +Certificate of Origin 1.1. This is achieved by adding

... and here, use either "Developer Certificate of Origin" or
"Developer's Certificate of Origin" - the DCO website irritatingly
uses both wordings :(

> +  a "Signed-off-by" line to every commit message. The presence
> +  of this line attests that the contributor has read the
> +  above lined DCO and agrees with its statements.
> +  

The indentation is off in the above paragraph.

Other than that it looks good, so

  Reviewed-by: Andrea Bolognani 

but I'm wondering if we need some sort of vote or agreement at the
community level before this can be formalized and enforced.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] Raise the frame limit for tests

[Martin Kletzander]

On Mon, Jan 22, 2018 at 12:57:31PM +, Daniel P. Berrange wrote:

On Mon, Jan 22, 2018 at 01:54:28PM +0100, Martin Kletzander wrote:

On Mon, Jan 22, 2018 at 01:47:24PM +0100, Michal Privoznik wrote:
> On 01/22/2018 01:22 PM, Daniel P. Berrange wrote:
> > On Mon, Jan 22, 2018 at 12:49:12PM +0100, Martin Kletzander wrote:
> > > On Thu, Jan 18, 2018 at 10:16:55AM +0100, Ján Tomko wrote:
> > > > After the latest CPU additions, the build fails with clang:
> > > > cputest.c:905:1: error: stack frame size of 26136 bytes
> > > >  in function 'mymain' [-Werror,-Wframe-larger-than=]
> > > >
> > > > Raise the relaxed limit which is used for tests.
> > > > ---
> > > > m4/virt-compile-warnings.m4 | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > Pushed as a build breaker fix
> > > >
> > > > diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4
> > > > index f18a08a8f..b9c974842 100644
> > > > --- a/m4/virt-compile-warnings.m4
> > > > +++ b/m4/virt-compile-warnings.m4
> > > > @@ -200,7 +200,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
> > > > # but using 1024 bytes sized buffers (mostly for virStrerror)
> > > > # stops us from going down further
> > > > gl_WARN_ADD([-Wframe-larger-than=4096], [STRICT_FRAME_LIMIT_CFLAGS])
> > > > -gl_WARN_ADD([-Wframe-larger-than=25600], 
[RELAXED_FRAME_LIMIT_CFLAGS])
> > > > +gl_WARN_ADD([-Wframe-larger-than=32768], 
[RELAXED_FRAME_LIMIT_CFLAGS])
> > > >
> > >
> > > Remind me again why don't we do -Wno-frame-larger-than (or something to 
that
> > > effect) for tests?  Is it just because "We should fix it at some point"?  
I
> > > can't really recall the reasoning behind that (and if it is still valid) 
even
> > > though I already asked for it.
> >
> > I don't think there's a strong reason, given the way we currently write
> > tests with huge amounts of stack variables.
> >
> > For -Wframe-larger-than to be useful, we'd need to move all the big data
> > blobs to be static, global variables.
> Or simply use compiler that honours variable lifetime. If a variable is
> defined only in a block, compiler should be able to just reuse the
> stack. I mean for the following case:
>
> do {
>  int x;
> } while (0);
>
> do {
>  int y;
> } while (0);
>
> I don't see any compelling reason for compiler to reserve two ints on
> the stack. Or if it does, count it as one when comparing agains
> -Wframe-larger-than.
>

We can do that ourselves, even though it's not really great thing to do.  Just
reset the one struct and reuse it.  I added it (and future research) as an idea
to GSoC ideas.  Let's see if someone rewrites that.


Is it really worth the effort though?  It is important for the core library
because we have a unimaginable set of code paths that are hard to validate,
so keeping stack use low is key to minimize risk fo stack exhaustion. In the
test suite, however, we have basically 1-3 call frames and stack exhaustion
is a non-issue - the test would merely crash & not have any bad consequences.



There are two points for this.  1) It can drive someone to start contributing to
libvirt by starting off easily, and 2) it can then help with assessing ways how
we can make the library frame sizes smaller.

So if there is no point in this for tests, as you said, we're back to my
original question.  Why to have this when we just randomly increase it?


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


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

Re: [libvirt] [PATCH hooks 1/1] Add check for Signed-off-by in commit messages

[Ján Tomko]

On Mon, Jan 22, 2018 at 12:42:47PM +, Daniel P. Berrange wrote:

On Mon, Jan 22, 2018 at 01:22:01PM +0100, Ján Tomko wrote:

On Mon, Jan 22, 2018 at 12:05:19PM +, Daniel P. Berrange wrote:
> This extends the update hook so that it enforces a requirement to have a
> Signed-off-by line in every commit message. This can be optionally
> turned off in individual repos by setting the "hooks.allowmissingsob"
> git config variable.
>
> Signed-off-by: Daniel P. Berrange 
> ---
> update | 16 +++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>

Given that signed-off-by lines are pointless for patches authored and
committed by the same person,


They are not pointless. They provide an explicit assertion that the author
is acknowledged they are permitted to make the contribution under the project's
license. This is distinct from the Author/Committer info in the commit, because
that is added automatically my git with no thought required by the developer.



I refuse to believe that a group of programmers is incapable of automating such
mundane process.

Also, adding -s to the command line by muscle memory is by definition a
no-thought process.


NACK unless the hooks.allowmissingsob will be set in the main
libvirt.git (I don't really care about other repos).


I'm intending it to be set in *every* repository include libvirt.git. There
is no real world burden for developers to add a signed-off-by line to the
commits they contribute to the project,


It adds visual clutter and one unnecessary step.


and it puts us in a stronger legal
position going forward. It is already commonplace across countless open
source projects, including many that we interact & build on in libvirt.



Yes, and the amount of red tape required to contribute is off-putting.
It saddens me to see libvirt go that route.

Jan


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


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

Re: [libvirt] [PATCH hooks 1/1] Add check for Signed-off-by in commit messages

[Peter Krempa]

On Mon, Jan 22, 2018 at 13:06:28 +, Daniel Berrange wrote:
> On Mon, Jan 22, 2018 at 01:20:12PM +0100, Peter Krempa wrote:
> > On Mon, Jan 22, 2018 at 12:05:19 +, Daniel Berrange wrote:
> > > This extends the update hook so that it enforces a requirement to have a
> > > Signed-off-by line in every commit message. This can be optionally
> > > turned off in individual repos by setting the "hooks.allowmissingsob"
> > > git config variable.
> > > 
> > > Signed-off-by: Daniel P. Berrange 
> > > ---
> > >  update | 16 +++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > NACK, I don't like signoffs and I don't really think they achieve
> > anything.
> 
> A signoff with no documented meaning attached by the project is fairly
> weak, as you would have to argue there was some commonly accepted signifance
> to it across the community. A signoff which is explicitly associated with a
> statement of intent has benefit, hence why I also sent a patch to clarify
> that the signoff asserts compliance with the DCO. Adding these signoffs has

You can do the same by declaring that all patches have to comply to that
and not mandating adding a line which will become eventually pointless
since every patch will need to have it.

Also since there's no way to check that it's actually true, anybody can
declare anything. Also the check itself can be fooled easily, so I think
it's pointless altogether.

> little to no time burden on long term developers own work on a daily basis.
> Just needs adding -s to "git commit" which quickly becomes engrained in
> muscle memory such that you'll end up doing it for every project you find

Or you can defeat it entirely by adding it into your commit message
template. I don't care about the added burden though. I don't really
think it achieves anything.

> yourself contributing to. Many of our "drive by" contributors already do
> this as habit, we'll just need to remind those that forget periodically.

I presonally signed-off < 5 commits in libvirt. So the last statement
is untrue. Some people don't do it on purpose.

The sign-off by itself (whithout cryptographic signature) is just
pointless. Validity with a cryptographic signature from drive-by
contributors can still be unproven, but at least you don't get
impersonation.

If everything is signed off, nothing really is.

NACK still stands.


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

Re: [libvirt] [PATCH] Raise the frame limit for tests

[Michal Privoznik]

On 01/22/2018 01:54 PM, Martin Kletzander wrote:
> On Mon, Jan 22, 2018 at 01:47:24PM +0100, Michal Privoznik wrote:
>> On 01/22/2018 01:22 PM, Daniel P. Berrange wrote:
>>> On Mon, Jan 22, 2018 at 12:49:12PM +0100, Martin Kletzander wrote:
 On Thu, Jan 18, 2018 at 10:16:55AM +0100, Ján Tomko wrote:
> After the latest CPU additions, the build fails with clang:
> cputest.c:905:1: error: stack frame size of 26136 bytes
>  in function 'mymain' [-Werror,-Wframe-larger-than=]
>
> Raise the relaxed limit which is used for tests.
> ---
> m4/virt-compile-warnings.m4 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Pushed as a build breaker fix
>
> diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4
> index f18a08a8f..b9c974842 100644
> --- a/m4/virt-compile-warnings.m4
> +++ b/m4/virt-compile-warnings.m4
> @@ -200,7 +200,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
>     # but using 1024 bytes sized buffers (mostly for virStrerror)
>     # stops us from going down further
>     gl_WARN_ADD([-Wframe-larger-than=4096],
> [STRICT_FRAME_LIMIT_CFLAGS])
> -    gl_WARN_ADD([-Wframe-larger-than=25600],
> [RELAXED_FRAME_LIMIT_CFLAGS])
> +    gl_WARN_ADD([-Wframe-larger-than=32768],
> [RELAXED_FRAME_LIMIT_CFLAGS])
>

 Remind me again why don't we do -Wno-frame-larger-than (or something
 to that
 effect) for tests?  Is it just because "We should fix it at some
 point"?  I
 can't really recall the reasoning behind that (and if it is still
 valid) even
 though I already asked for it.
>>>
>>> I don't think there's a strong reason, given the way we currently write
>>> tests with huge amounts of stack variables.
>>>
>>> For -Wframe-larger-than to be useful, we'd need to move all the big data
>>> blobs to be static, global variables.
>> Or simply use compiler that honours variable lifetime. If a variable is
>> defined only in a block, compiler should be able to just reuse the
>> stack. I mean for the following case:
>>
>> do {
>>  int x;
>> } while (0);
>>
>> do {
>>  int y;
>> } while (0);
>>
>> I don't see any compelling reason for compiler to reserve two ints on
>> the stack. Or if it does, count it as one when comparing agains
>> -Wframe-larger-than.
>>
> 
> We can do that ourselves, even though it's not really great thing to
> do.  Just
> reset the one struct and reuse it.  I added it (and future research) as
> an idea
> to GSoC ideas.  Let's see if someone rewrites that.

I don't think that's a good idea. It's working around broken compiler.
Just like the original patch (which we unfortunately have to have).

Michal

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

Re: [libvirt] [PATCH hooks 1/1] Add check for Signed-off-by in commit messages

[Daniel P. Berrange]

On Mon, Jan 22, 2018 at 01:20:12PM +0100, Peter Krempa wrote:
> On Mon, Jan 22, 2018 at 12:05:19 +, Daniel Berrange wrote:
> > This extends the update hook so that it enforces a requirement to have a
> > Signed-off-by line in every commit message. This can be optionally
> > turned off in individual repos by setting the "hooks.allowmissingsob"
> > git config variable.
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  update | 16 +++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> NACK, I don't like signoffs and I don't really think they achieve
> anything.

A signoff with no documented meaning attached by the project is fairly
weak, as you would have to argue there was some commonly accepted signifance
to it across the community. A signoff which is explicitly associated with a
statement of intent has benefit, hence why I also sent a patch to clarify
that the signoff asserts compliance with the DCO. Adding these signoffs has
little to no time burden on long term developers own work on a daily basis.
Just needs adding -s to "git commit" which quickly becomes engrained in
muscle memory such that you'll end up doing it for every project you find
yourself contributing to. Many of our "drive by" contributors already do
this as habit, we'll just need to remind those that forget periodically.

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

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

Re: [libvirt] [PATCH] Raise the frame limit for tests

[Daniel P. Berrange]

On Mon, Jan 22, 2018 at 01:54:28PM +0100, Martin Kletzander wrote:
> On Mon, Jan 22, 2018 at 01:47:24PM +0100, Michal Privoznik wrote:
> > On 01/22/2018 01:22 PM, Daniel P. Berrange wrote:
> > > On Mon, Jan 22, 2018 at 12:49:12PM +0100, Martin Kletzander wrote:
> > > > On Thu, Jan 18, 2018 at 10:16:55AM +0100, Ján Tomko wrote:
> > > > > After the latest CPU additions, the build fails with clang:
> > > > > cputest.c:905:1: error: stack frame size of 26136 bytes
> > > > >  in function 'mymain' [-Werror,-Wframe-larger-than=]
> > > > > 
> > > > > Raise the relaxed limit which is used for tests.
> > > > > ---
> > > > > m4/virt-compile-warnings.m4 | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > Pushed as a build breaker fix
> > > > > 
> > > > > diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4
> > > > > index f18a08a8f..b9c974842 100644
> > > > > --- a/m4/virt-compile-warnings.m4
> > > > > +++ b/m4/virt-compile-warnings.m4
> > > > > @@ -200,7 +200,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
> > > > > # but using 1024 bytes sized buffers (mostly for virStrerror)
> > > > > # stops us from going down further
> > > > > gl_WARN_ADD([-Wframe-larger-than=4096], 
> > > > > [STRICT_FRAME_LIMIT_CFLAGS])
> > > > > -gl_WARN_ADD([-Wframe-larger-than=25600], 
> > > > > [RELAXED_FRAME_LIMIT_CFLAGS])
> > > > > +gl_WARN_ADD([-Wframe-larger-than=32768], 
> > > > > [RELAXED_FRAME_LIMIT_CFLAGS])
> > > > > 
> > > > 
> > > > Remind me again why don't we do -Wno-frame-larger-than (or something to 
> > > > that
> > > > effect) for tests?  Is it just because "We should fix it at some 
> > > > point"?  I
> > > > can't really recall the reasoning behind that (and if it is still 
> > > > valid) even
> > > > though I already asked for it.
> > > 
> > > I don't think there's a strong reason, given the way we currently write
> > > tests with huge amounts of stack variables.
> > > 
> > > For -Wframe-larger-than to be useful, we'd need to move all the big data
> > > blobs to be static, global variables.
> > Or simply use compiler that honours variable lifetime. If a variable is
> > defined only in a block, compiler should be able to just reuse the
> > stack. I mean for the following case:
> > 
> > do {
> >  int x;
> > } while (0);
> > 
> > do {
> >  int y;
> > } while (0);
> > 
> > I don't see any compelling reason for compiler to reserve two ints on
> > the stack. Or if it does, count it as one when comparing agains
> > -Wframe-larger-than.
> > 
> 
> We can do that ourselves, even though it's not really great thing to do.  Just
> reset the one struct and reuse it.  I added it (and future research) as an 
> idea
> to GSoC ideas.  Let's see if someone rewrites that.

Is it really worth the effort though?  It is important for the core library
because we have a unimaginable set of code paths that are hard to validate,
so keeping stack use low is key to minimize risk fo stack exhaustion. In the
test suite, however, we have basically 1-3 call frames and stack exhaustion
is a non-issue - the test would merely crash & not have any bad consequences.

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

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

Re: [libvirt] [PATCH] Raise the frame limit for tests

[Martin Kletzander]

On Mon, Jan 22, 2018 at 01:47:24PM +0100, Michal Privoznik wrote:

On 01/22/2018 01:22 PM, Daniel P. Berrange wrote:

On Mon, Jan 22, 2018 at 12:49:12PM +0100, Martin Kletzander wrote:

On Thu, Jan 18, 2018 at 10:16:55AM +0100, Ján Tomko wrote:

After the latest CPU additions, the build fails with clang:
cputest.c:905:1: error: stack frame size of 26136 bytes
 in function 'mymain' [-Werror,-Wframe-larger-than=]

Raise the relaxed limit which is used for tests.
---
m4/virt-compile-warnings.m4 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Pushed as a build breaker fix

diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4
index f18a08a8f..b9c974842 100644
--- a/m4/virt-compile-warnings.m4
+++ b/m4/virt-compile-warnings.m4
@@ -200,7 +200,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
# but using 1024 bytes sized buffers (mostly for virStrerror)
# stops us from going down further
gl_WARN_ADD([-Wframe-larger-than=4096], [STRICT_FRAME_LIMIT_CFLAGS])
-gl_WARN_ADD([-Wframe-larger-than=25600], [RELAXED_FRAME_LIMIT_CFLAGS])
+gl_WARN_ADD([-Wframe-larger-than=32768], [RELAXED_FRAME_LIMIT_CFLAGS])



Remind me again why don't we do -Wno-frame-larger-than (or something to that
effect) for tests?  Is it just because "We should fix it at some point"?  I
can't really recall the reasoning behind that (and if it is still valid) even
though I already asked for it.


I don't think there's a strong reason, given the way we currently write
tests with huge amounts of stack variables.

For -Wframe-larger-than to be useful, we'd need to move all the big data
blobs to be static, global variables.

Or simply use compiler that honours variable lifetime. If a variable is
defined only in a block, compiler should be able to just reuse the
stack. I mean for the following case:

do {
 int x;
} while (0);

do {
 int y;
} while (0);

I don't see any compelling reason for compiler to reserve two ints on
the stack. Or if it does, count it as one when comparing agains
-Wframe-larger-than.



We can do that ourselves, even though it's not really great thing to do.  Just
reset the one struct and reuse it.  I added it (and future research) as an idea
to GSoC ideas.  Let's see if someone rewrites that.


Michal

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


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

Re: [libvirt] [PATCH] Raise the frame limit for tests

[Michal Privoznik]

On 01/22/2018 01:22 PM, Daniel P. Berrange wrote:
> On Mon, Jan 22, 2018 at 12:49:12PM +0100, Martin Kletzander wrote:
>> On Thu, Jan 18, 2018 at 10:16:55AM +0100, Ján Tomko wrote:
>>> After the latest CPU additions, the build fails with clang:
>>> cputest.c:905:1: error: stack frame size of 26136 bytes
>>>  in function 'mymain' [-Werror,-Wframe-larger-than=]
>>>
>>> Raise the relaxed limit which is used for tests.
>>> ---
>>> m4/virt-compile-warnings.m4 | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> Pushed as a build breaker fix
>>>
>>> diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4
>>> index f18a08a8f..b9c974842 100644
>>> --- a/m4/virt-compile-warnings.m4
>>> +++ b/m4/virt-compile-warnings.m4
>>> @@ -200,7 +200,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
>>> # but using 1024 bytes sized buffers (mostly for virStrerror)
>>> # stops us from going down further
>>> gl_WARN_ADD([-Wframe-larger-than=4096], [STRICT_FRAME_LIMIT_CFLAGS])
>>> -gl_WARN_ADD([-Wframe-larger-than=25600], [RELAXED_FRAME_LIMIT_CFLAGS])
>>> +gl_WARN_ADD([-Wframe-larger-than=32768], [RELAXED_FRAME_LIMIT_CFLAGS])
>>>
>>
>> Remind me again why don't we do -Wno-frame-larger-than (or something to that
>> effect) for tests?  Is it just because "We should fix it at some point"?  I
>> can't really recall the reasoning behind that (and if it is still valid) even
>> though I already asked for it.
> 
> I don't think there's a strong reason, given the way we currently write
> tests with huge amounts of stack variables.
> 
> For -Wframe-larger-than to be useful, we'd need to move all the big data
> blobs to be static, global variables.
Or simply use compiler that honours variable lifetime. If a variable is
defined only in a block, compiler should be able to just reuse the
stack. I mean for the following case:

do {
  int x;
} while (0);

do {
  int y;
} while (0);

I don't see any compelling reason for compiler to reserve two ints on
the stack. Or if it does, count it as one when comparing agains
-Wframe-larger-than.

Michal

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

[libvirt] [PATCH] docs: document requirement to provide Signed-off-by lines for DCO

[Daniel P. Berrange]

Document that contributors are required to assert compliance with the
Developers Certification of Origin 1.1, by providing Signed-off-by tags
for all commit messages. The DCO is formally stating what we have long
implicitly expected of contributors in terms of their legal rights to
make the contribution. This puts the project in a stronger position
should any questions around contributions be raised going forward in the
future.

Signed-off-by: Daniel P. Berrange 
---
 docs/hacking.html.in | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/docs/hacking.html.in b/docs/hacking.html.in
index 6cadfb8343..98a24d785c 100644
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -108,12 +108,18 @@
   of the bug number is useful; but also summarize the issue
   rather than making all readers follow the link.  You can use
   'git shortlog -30' to get an idea of typical summary lines.
-  Libvirt does not currently attach any meaning to
-  Signed-off-by: lines, so it is up to you if you want to
-  include or omit them in the commit message.
 
   
 
+  Contributors to libvirt projects must
+  assert that they are in compliance with the
+  https://developercertificate.org/;>Developers
+Certificate of Origin 1.1. This is achieved by adding
+  a "Signed-off-by" line to every commit message. The presence
+  of this line attests that the contributor has read the
+  above lined DCO and agrees with its statements.
+  
+
   Split large changes into a series of smaller patches,
 self-contained if possible, with an explanation of each patch
 and an explanation of how the sequence of patches fits
-- 
2.14.3

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

Re: [libvirt] [PATCH] qemu: Refresh caps cache after booting a different kernel

[Daniel P. Berrange]

On Mon, Jan 22, 2018 at 01:31:21PM +0100, Jiri Denemark wrote:
> On Mon, Jan 22, 2018 at 10:57:57 +, Daniel P. Berrange wrote:
> > On Mon, Jan 22, 2018 at 11:46:14AM +0100, Jiri Denemark wrote:
> > > Whenever a different kernel is booted, some capabilities related to KVM
> > > (such as CPUID bits) may change. We need to refresh the cache to see the
> > > changes.
> > > 
> > > Signed-off-by: Jiri Denemark 
> > > ---
> > > 
> > > Notes:
> > > The capabilities may also change if a parameter passed to a kvm module
> > > changes (kvm_intel.nested is a good example) so this is not a complete
> > > solution, but we're hopefully getting closer to it :-)
> > 
> > You mean getting closer to a situation where we are effectively storing the
> > cache on tmpfs, because we invalidate it on every reboot !
> 
> Well, that's a possible result, yes. Although it's both incomplete and
> invalidating the cache too often at the same time. It's possible we
> won't be able to come up with anything more clever anyway.
> 
> > I think sometime soon we're going to need to consider if our cache 
> > invalidation
> > approach is fundamentally broken.  We have a huge amount of stuff we query 
> > from
> > QEMU, but only a tiny amount is dependant on host kernel / microcode / kvm 
> > mod
> > options. Should we go back to invalidating only when libvirt/qemu binary 
> > changes
> > but then do partial invalidation of specific data items for kernel/microcode
> > changes.
> 
> On the other hand, while we have QEMU running, probing for all
> capabilities vs just a limited set which depend on the host shouldn't be
> a big difference. I haven't actually measured it though. However, we
> only invalidate the cache more often for KVM, which makes it pretty
> limited already since we only invalidate the capabilities for a single
> binary.

Oh true, I didn't notice you'd only done invalidation for the KVM code
path. That should avoid the major pain that GNOME Boxes saw where we
spend ages probing 20 QEMU binaries on every startup

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

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

Re: [libvirt] [PATCH hooks 1/1] Add check for Signed-off-by in commit messages

[Daniel P. Berrange]

On Mon, Jan 22, 2018 at 01:22:01PM +0100, Ján Tomko wrote:
> On Mon, Jan 22, 2018 at 12:05:19PM +, Daniel P. Berrange wrote:
> > This extends the update hook so that it enforces a requirement to have a
> > Signed-off-by line in every commit message. This can be optionally
> > turned off in individual repos by setting the "hooks.allowmissingsob"
> > git config variable.
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> > update | 16 +++-
> > 1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> 
> Given that signed-off-by lines are pointless for patches authored and
> committed by the same person,

They are not pointless. They provide an explicit assertion that the author
is acknowledged they are permitted to make the contribution under the project's
license. This is distinct from the Author/Committer info in the commit, because
that is added automatically my git with no thought required by the developer.

> NACK unless the hooks.allowmissingsob will be set in the main
> libvirt.git (I don't really care about other repos).

I'm intending it to be set in *every* repository include libvirt.git. There
is no real world burden for developers to add a signed-off-by line to the
commits they contribute to the project, and it puts us in a stronger legal
position going forward. It is already commonplace across countless open
source projects, including many that we interact & build on in libvirt.

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

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

Re: [libvirt] [PATCH] qemu: Refresh caps cache after booting a different kernel

[Jiri Denemark]

On Mon, Jan 22, 2018 at 10:57:57 +, Daniel P. Berrange wrote:
> On Mon, Jan 22, 2018 at 11:46:14AM +0100, Jiri Denemark wrote:
> > Whenever a different kernel is booted, some capabilities related to KVM
> > (such as CPUID bits) may change. We need to refresh the cache to see the
> > changes.
> > 
> > Signed-off-by: Jiri Denemark 
> > ---
> > 
> > Notes:
> > The capabilities may also change if a parameter passed to a kvm module
> > changes (kvm_intel.nested is a good example) so this is not a complete
> > solution, but we're hopefully getting closer to it :-)
> 
> You mean getting closer to a situation where we are effectively storing the
> cache on tmpfs, because we invalidate it on every reboot !

Well, that's a possible result, yes. Although it's both incomplete and
invalidating the cache too often at the same time. It's possible we
won't be able to come up with anything more clever anyway.

> I think sometime soon we're going to need to consider if our cache 
> invalidation
> approach is fundamentally broken.  We have a huge amount of stuff we query 
> from
> QEMU, but only a tiny amount is dependant on host kernel / microcode / kvm mod
> options. Should we go back to invalidating only when libvirt/qemu binary 
> changes
> but then do partial invalidation of specific data items for kernel/microcode
> changes.

On the other hand, while we have QEMU running, probing for all
capabilities vs just a limited set which depend on the host shouldn't be
a big difference. I haven't actually measured it though. However, we
only invalidate the cache more often for KVM, which makes it pretty
limited already since we only invalidate the capabilities for a single
binary.

Jirka

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

Re: [libvirt] [PATCH] qemu: auto-add generic xhci rather than NEC xhci to Q35 domains

[Pavel Hrdina]

On Sat, Jan 20, 2018 at 09:55:14PM -0500, Laine Stump wrote:
> We recently added a generic XHCI USB3 controller to QEMU, and libvirt
> supports adding that controller rather than the NEC XHCI USB3
> controller, but when auto-adding a USB controller to Q35 domains we
> were still adding the vendor-specific NEC controller. This patch
> changes to add the generic controller instead, if it's available in
> the QEMU binary that will be used.
> ---
>  src/qemu/qemu_domain.c | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)

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] Raise the frame limit for tests

[Daniel P. Berrange]

On Mon, Jan 22, 2018 at 12:49:12PM +0100, Martin Kletzander wrote:
> On Thu, Jan 18, 2018 at 10:16:55AM +0100, Ján Tomko wrote:
> > After the latest CPU additions, the build fails with clang:
> > cputest.c:905:1: error: stack frame size of 26136 bytes
> >  in function 'mymain' [-Werror,-Wframe-larger-than=]
> > 
> > Raise the relaxed limit which is used for tests.
> > ---
> > m4/virt-compile-warnings.m4 | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > Pushed as a build breaker fix
> > 
> > diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4
> > index f18a08a8f..b9c974842 100644
> > --- a/m4/virt-compile-warnings.m4
> > +++ b/m4/virt-compile-warnings.m4
> > @@ -200,7 +200,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
> > # but using 1024 bytes sized buffers (mostly for virStrerror)
> > # stops us from going down further
> > gl_WARN_ADD([-Wframe-larger-than=4096], [STRICT_FRAME_LIMIT_CFLAGS])
> > -gl_WARN_ADD([-Wframe-larger-than=25600], [RELAXED_FRAME_LIMIT_CFLAGS])
> > +gl_WARN_ADD([-Wframe-larger-than=32768], [RELAXED_FRAME_LIMIT_CFLAGS])
> > 
> 
> Remind me again why don't we do -Wno-frame-larger-than (or something to that
> effect) for tests?  Is it just because "We should fix it at some point"?  I
> can't really recall the reasoning behind that (and if it is still valid) even
> though I already asked for it.

I don't think there's a strong reason, given the way we currently write
tests with huge amounts of stack variables.

For -Wframe-larger-than to be useful, we'd need to move all the big data
blobs to be static, global variables.

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

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

Re: [libvirt] [PATCH hooks 1/1] Add check for Signed-off-by in commit messages

[Ján Tomko]

On Mon, Jan 22, 2018 at 12:05:19PM +, Daniel P. Berrange wrote:

This extends the update hook so that it enforces a requirement to have a
Signed-off-by line in every commit message. This can be optionally
turned off in individual repos by setting the "hooks.allowmissingsob"
git config variable.

Signed-off-by: Daniel P. Berrange 
---
update | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)



Given that signed-off-by lines are pointless for patches authored and
committed by the same person,

NACK unless the hooks.allowmissingsob will be set in the main
libvirt.git (I don't really care about other repos).

Jan


diff --git a/update b/update
index 6a8edcb..b7cfe7f 100755
--- a/update
+++ b/update
@@ -248,7 +248,21 @@ if [ $check_diff = yes ]; then
if [ "$allow_bad_whitespace" != "true" ]; then
test "$oldrev" = $zero \
&& exit 0
-   exec git diff --check $oldrev $newrev --
+   git diff --check $oldrev $newrev --
+   test $? != 0 && exit 1
+   fi
+
+   allow_missing_sob=$(git config --bool hooks.allowmissingsob)
+   if [ "$allow_missing_sob" != "true" ]; then
+   for rev in `git log --format=%h $oldrev..$newrev`
+   do
+   git show $rev | grep Signed-off-by >/dev/null 2>&1
+   if test $? != 0
+   then
+   echo "*** Update hook: missing Signed-off-by tag in 
$rev" >&2
+   exit 1
+   fi
+   done
fi
fi

--
2.14.3

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


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

Re: [libvirt] [PATCH hooks 1/1] Add check for Signed-off-by in commit messages

[Peter Krempa]

On Mon, Jan 22, 2018 at 12:05:19 +, Daniel Berrange wrote:
> This extends the update hook so that it enforces a requirement to have a
> Signed-off-by line in every commit message. This can be optionally
> turned off in individual repos by setting the "hooks.allowmissingsob"
> git config variable.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  update | 16 +++-
>  1 file changed, 15 insertions(+), 1 deletion(-)

NACK, I don't like signoffs and I don't really think they achieve
anything.


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

[libvirt] [PATCH hooks 1/1] Add check for Signed-off-by in commit messages

[Daniel P. Berrange]

This extends the update hook so that it enforces a requirement to have a
Signed-off-by line in every commit message. This can be optionally
turned off in individual repos by setting the "hooks.allowmissingsob"
git config variable.

Signed-off-by: Daniel P. Berrange 
---
 update | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/update b/update
index 6a8edcb..b7cfe7f 100755
--- a/update
+++ b/update
@@ -248,7 +248,21 @@ if [ $check_diff = yes ]; then
if [ "$allow_bad_whitespace" != "true" ]; then
test "$oldrev" = $zero \
&& exit 0
-   exec git diff --check $oldrev $newrev --
+   git diff --check $oldrev $newrev --
+   test $? != 0 && exit 1
+   fi
+
+   allow_missing_sob=$(git config --bool hooks.allowmissingsob)
+   if [ "$allow_missing_sob" != "true" ]; then
+   for rev in `git log --format=%h $oldrev..$newrev`
+   do
+   git show $rev | grep Signed-off-by >/dev/null 2>&1
+   if test $? != 0
+   then
+   echo "*** Update hook: missing Signed-off-by 
tag in $rev" >&2
+   exit 1
+   fi
+   done
fi
 fi
 
-- 
2.14.3

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

[libvirt] [PATCH hooks 0/1] Enforce requirement for Signed-off-by line

[Daniel P. Berrange]

As a general rule most libvirt commits in recent history have have a
valid Signed-off-by line present. There are still cases, however, where
this is being forgotten. It is time to put some checks on git pushes to
detect missinged Signed-off-by lines and reject the push.

I created a new git repository that contains the original git repository
update hook Eric provided years back. This commit then augments that
hook to add the Signed-off-by check. The updated hook gets automatically
installed on the server when we push to this repo.

Daniel P. Berrange (1):
  Add check for Signed-off-by in commit messages

 update | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

-- 
2.14.3

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

Re: [libvirt] [PATCH] Raise the frame limit for tests

[Martin Kletzander]

On Thu, Jan 18, 2018 at 10:16:55AM +0100, Ján Tomko wrote:

After the latest CPU additions, the build fails with clang:
cputest.c:905:1: error: stack frame size of 26136 bytes
 in function 'mymain' [-Werror,-Wframe-larger-than=]

Raise the relaxed limit which is used for tests.
---
m4/virt-compile-warnings.m4 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Pushed as a build breaker fix

diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4
index f18a08a8f..b9c974842 100644
--- a/m4/virt-compile-warnings.m4
+++ b/m4/virt-compile-warnings.m4
@@ -200,7 +200,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
# but using 1024 bytes sized buffers (mostly for virStrerror)
# stops us from going down further
gl_WARN_ADD([-Wframe-larger-than=4096], [STRICT_FRAME_LIMIT_CFLAGS])
-gl_WARN_ADD([-Wframe-larger-than=25600], [RELAXED_FRAME_LIMIT_CFLAGS])
+gl_WARN_ADD([-Wframe-larger-than=32768], [RELAXED_FRAME_LIMIT_CFLAGS])



Remind me again why don't we do -Wno-frame-larger-than (or something to that
effect) for tests?  Is it just because "We should fix it at some point"?  I
can't really recall the reasoning behind that (and if it is still valid) even
though I already asked for it.


# Extra special flags
dnl -fstack-protector stuff passes gl_WARN_ADD with gcc
--
2.13.6

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


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

Re: [libvirt] [PATCH v2] libvirtd: clarify the TLS conf default vaule setting

[Kashyap Chamarthy]

On Fri, Jan 19, 2018 at 05:20:10PM -0500, John Ferlan wrote:

[...]

> More simply stated:
> 
> Provide more details related to the requirement that setting one
> of the values requires setting all of them.

Sounds clearer.

[...]

> How about this instead:
> 
> # Use of TLS requires that x509 certificates be issued. The default locations
> # for the certificate files is as follows:
> #
> #   /etc/pki/CA/cacert.pem - The CA master certificate
> #   /etc/pki/libvirt/servercert.pem- The server certificate signed 
> with
> #the cacert.pem
> #   /etc/pki/libvirt/private/serverkey.pem - The server private key
> #
> # It is possible to override the default locations by altering the 'key_file',
> # 'cert_file', and 'ca_file' values and uncommenting them below.
> #
> # NB, overriding the default of one location requires uncommenting and
> # possibly additionally overriding the other settings.
> #

Noticed this change randomly.  The above looks much better to me.  So,
if we go with the above: 

Reviewed-by: Kashyap Chamarthy 

-- 
/kashyap

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

Re: [libvirt] [PATCH] qemu: Refresh caps cache after booting a different kernel

[Daniel P. Berrange]

On Mon, Jan 22, 2018 at 11:46:14AM +0100, Jiri Denemark wrote:
> Whenever a different kernel is booted, some capabilities related to KVM
> (such as CPUID bits) may change. We need to refresh the cache to see the
> changes.
> 
> Signed-off-by: Jiri Denemark 
> ---
> 
> Notes:
> The capabilities may also change if a parameter passed to a kvm module
> changes (kvm_intel.nested is a good example) so this is not a complete
> solution, but we're hopefully getting closer to it :-)

You mean getting closer to a situation where we are effectively storing the
cache on tmpfs, because we invalidate it on every reboot !

I think sometime soon we're going to need to consider if our cache invalidation
approach is fundamentally broken.  We have a huge amount of stuff we query from
QEMU, but only a tiny amount is dependant on host kernel / microcode / kvm mod
options. Should we go back to invalidating only when libvirt/qemu binary changes
but then do partial invalidation of specific data items for kernel/microcode
changes.

It might require QEMU help to give us "promise" of which QMP commands return
data that may be impacted by KVM / kernel.

>  src/qemu/qemu_capabilities.c | 57 
> +---
>  src/qemu/qemu_capspriv.h |  1 +
>  tests/qemucapsprobe.c|  2 +-
>  3 files changed, 50 insertions(+), 10 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index ab0ea8ec0d..2c573827f1 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -52,6 +52,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define VIR_FROM_THIS VIR_FROM_QEMU
>  
> @@ -510,6 +511,7 @@ struct _virQEMUCaps {
>  unsigned int libvirtVersion;
>  unsigned int microcodeVersion;
>  char *package;
> +char *kernelVersion;
>  
>  virArch arch;
>  
> @@ -2303,6 +2305,9 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr 
> qemuCaps)
>  if (VIR_STRDUP(ret->package, qemuCaps->package) < 0)
>  goto error;
>  
> +if (VIR_STRDUP(ret->kernelVersion, qemuCaps->kernelVersion) < 0)
> +goto error;
> +
>  ret->arch = qemuCaps->arch;
>  
>  if (qemuCaps->kvmCPUModels) {
> @@ -2363,6 +2368,7 @@ void virQEMUCapsDispose(void *obj)
>  virBitmapFree(qemuCaps->flags);
>  
>  VIR_FREE(qemuCaps->package);
> +VIR_FREE(qemuCaps->kernelVersion);
>  VIR_FREE(qemuCaps->binary);
>  
>  VIR_FREE(qemuCaps->gicCapabilities);
> @@ -3834,6 +3840,7 @@ struct _virQEMUCapsCachePriv {
>  gid_t runGid;
>  virArch hostArch;
>  unsigned int microcodeVersion;
> +char *kernelVersion;
>  };
>  typedef struct _virQEMUCapsCachePriv virQEMUCapsCachePriv;
>  typedef virQEMUCapsCachePriv *virQEMUCapsCachePrivPtr;
> @@ -3845,6 +3852,7 @@ virQEMUCapsCachePrivFree(void *privData)
>  virQEMUCapsCachePrivPtr priv = privData;
>  
>  VIR_FREE(priv->libDir);
> +VIR_FREE(priv->kernelVersion);
>  VIR_FREE(priv);
>  }
>  
> @@ -3970,6 +3978,12 @@ virQEMUCapsLoadCache(virArch hostArch,
>  goto cleanup;
>  }
>  
> +if (virXPathBoolean("boolean(./kernelVersion)", ctxt) > 0) {
> +qemuCaps->kernelVersion = virXPathString("string(./kernelVersion)", 
> ctxt);
> +if (!qemuCaps->kernelVersion)
> +goto cleanup;
> +}
> +
>  if (!(str = virXPathString("string(./arch)", ctxt))) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("missing arch in QEMU capabilities cache"));
> @@ -4248,6 +4262,10 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps)
>  virBufferAsprintf(, "%s\n",
>qemuCaps->package);
>  
> +if (qemuCaps->kernelVersion)
> +virBufferAsprintf(, "%s\n",
> +  qemuCaps->kernelVersion);
> +
>  virBufferAsprintf(, "%s\n",
>virArchToString(qemuCaps->arch));
>  
> @@ -4385,14 +4403,24 @@ virQEMUCapsIsValid(void *data,
>  return false;
>  }
>  
> -if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
> -priv->microcodeVersion != qemuCaps->microcodeVersion) {
> -VIR_DEBUG("Outdated capabilities for '%s': microcode version changed 
> "
> -  "(%u vs %u)",
> -  qemuCaps->binary,
> -  priv->microcodeVersion,
> -  qemuCaps->microcodeVersion);
> -return false;
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
> +if (priv->microcodeVersion != qemuCaps->microcodeVersion) {
> +VIR_DEBUG("Outdated capabilities for '%s': microcode version "
> +  "changed (%u vs %u)",
> +  qemuCaps->binary,
> +  priv->microcodeVersion,
> +  qemuCaps->microcodeVersion);
> +return false;
> +}
> +
> +if (STRNEQ_NULLABLE(priv->kernelVersion, qemuCaps->kernelVersion)) {
> +

[libvirt] [PATCH] qemu: Refresh caps cache after booting a different kernel

[Jiri Denemark]

Whenever a different kernel is booted, some capabilities related to KVM
(such as CPUID bits) may change. We need to refresh the cache to see the
changes.

Signed-off-by: Jiri Denemark 
---

Notes:
The capabilities may also change if a parameter passed to a kvm module
changes (kvm_intel.nested is a good example) so this is not a complete
solution, but we're hopefully getting closer to it :-)

 src/qemu/qemu_capabilities.c | 57 +---
 src/qemu/qemu_capspriv.h |  1 +
 tests/qemucapsprobe.c|  2 +-
 3 files changed, 50 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index ab0ea8ec0d..2c573827f1 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -52,6 +52,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define VIR_FROM_THIS VIR_FROM_QEMU
 
@@ -510,6 +511,7 @@ struct _virQEMUCaps {
 unsigned int libvirtVersion;
 unsigned int microcodeVersion;
 char *package;
+char *kernelVersion;
 
 virArch arch;
 
@@ -2303,6 +2305,9 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps)
 if (VIR_STRDUP(ret->package, qemuCaps->package) < 0)
 goto error;
 
+if (VIR_STRDUP(ret->kernelVersion, qemuCaps->kernelVersion) < 0)
+goto error;
+
 ret->arch = qemuCaps->arch;
 
 if (qemuCaps->kvmCPUModels) {
@@ -2363,6 +2368,7 @@ void virQEMUCapsDispose(void *obj)
 virBitmapFree(qemuCaps->flags);
 
 VIR_FREE(qemuCaps->package);
+VIR_FREE(qemuCaps->kernelVersion);
 VIR_FREE(qemuCaps->binary);
 
 VIR_FREE(qemuCaps->gicCapabilities);
@@ -3834,6 +3840,7 @@ struct _virQEMUCapsCachePriv {
 gid_t runGid;
 virArch hostArch;
 unsigned int microcodeVersion;
+char *kernelVersion;
 };
 typedef struct _virQEMUCapsCachePriv virQEMUCapsCachePriv;
 typedef virQEMUCapsCachePriv *virQEMUCapsCachePrivPtr;
@@ -3845,6 +3852,7 @@ virQEMUCapsCachePrivFree(void *privData)
 virQEMUCapsCachePrivPtr priv = privData;
 
 VIR_FREE(priv->libDir);
+VIR_FREE(priv->kernelVersion);
 VIR_FREE(priv);
 }
 
@@ -3970,6 +3978,12 @@ virQEMUCapsLoadCache(virArch hostArch,
 goto cleanup;
 }
 
+if (virXPathBoolean("boolean(./kernelVersion)", ctxt) > 0) {
+qemuCaps->kernelVersion = virXPathString("string(./kernelVersion)", 
ctxt);
+if (!qemuCaps->kernelVersion)
+goto cleanup;
+}
+
 if (!(str = virXPathString("string(./arch)", ctxt))) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("missing arch in QEMU capabilities cache"));
@@ -4248,6 +4262,10 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps)
 virBufferAsprintf(, "%s\n",
   qemuCaps->package);
 
+if (qemuCaps->kernelVersion)
+virBufferAsprintf(, "%s\n",
+  qemuCaps->kernelVersion);
+
 virBufferAsprintf(, "%s\n",
   virArchToString(qemuCaps->arch));
 
@@ -4385,14 +4403,24 @@ virQEMUCapsIsValid(void *data,
 return false;
 }
 
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
-priv->microcodeVersion != qemuCaps->microcodeVersion) {
-VIR_DEBUG("Outdated capabilities for '%s': microcode version changed "
-  "(%u vs %u)",
-  qemuCaps->binary,
-  priv->microcodeVersion,
-  qemuCaps->microcodeVersion);
-return false;
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
+if (priv->microcodeVersion != qemuCaps->microcodeVersion) {
+VIR_DEBUG("Outdated capabilities for '%s': microcode version "
+  "changed (%u vs %u)",
+  qemuCaps->binary,
+  priv->microcodeVersion,
+  qemuCaps->microcodeVersion);
+return false;
+}
+
+if (STRNEQ_NULLABLE(priv->kernelVersion, qemuCaps->kernelVersion)) {
+VIR_DEBUG("Outdated capabilities for '%s': kernel version changed "
+  "('%s' vs '%s')",
+  qemuCaps->binary,
+  priv->kernelVersion,
+  qemuCaps->kernelVersion);
+return false;
+}
 }
 
 return true;
@@ -5228,6 +5256,7 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch,
 uid_t runUid,
 gid_t runGid,
 unsigned int microcodeVersion,
+const char *kernelVersion,
 bool qmpOnly)
 {
 virQEMUCapsPtr qemuCaps;
@@ -5284,9 +5313,13 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch,
 virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
 virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_QEMU);
 
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
+if 

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

[Kang, Luwei]

> > On 18/01/2018 14:24, Eduardo Habkost wrote:
> > > However, if there's a simple way to make it possible to migrate
> > > between hosts with different CPUID[14h] data, it would be even
> > > better.  With the current KVM intel-pt implementation, what happens
> > > if the CPUID[14h] data seen by the guest doesn't match exactly the
> > > CPUID[14h] leaves from the host?
> >
> > Some bits in there can be treated as CPU features (e.g. EBX bit 0 "CR3
> > filtering support").  Probably we should handle these in KVM right now.
> > KVM needs to compute a mask of valid 1 bits for IA32_RTIT_CTL based on
> > CPUID, and apply it when the MSR is written.
> 
> Does this mean QEMU can't set CPUID values that won't match the host with the 
> existing implementation, or this won't matter for
> well-behaved guests that don't try to set reserved bits on the MSRs?
> 
> 
> >   It also needs to
> > whitelist bits like we do for other feature words.  These include:
> >
> > - CPUID[EAX=14h,ECX=0].EBX
> >
> > - CPUID[EAX=14h,ECX=0].ECX except bit 31
> >
> > - CPUID[EAX=14h,ECX=1].EAX bits 16:31 (if
> > CPUID[EAX=14h,ECX=0].EBX[3]=1)
> >
> > - CPUID[EAX=14h,ECX=1].EBX (if CPUID[EAX=14h,ECX=0].EBX[1]=1)
> 
> What do you mean by whitelist?
> 
> 
> >
> > Others, currently only CPUID[EAX=14h,ECX=0].ECX[31] must match, there
> > is no way to emulate the "wrong" value.
> 
> In this case we could make it configurable but require the host and guest 
> value to always match.
> 
> This might be an obstacle to enabling intel-pt by default (because it could 
> make VMs not migratable to newer hosts), but may allow
> the feature to be configured in a predictable way.
> 
> 
> >
> > Others, currently only CPUID[EAX=14h,ECX=1].EAX[2:0] are numeric
> > values, and it's possible to emulate a lower value than the one in the 
> > processor.
> 
> This could be handled by QEMU.  There's no requirement that all 
> GET_SUPPORTED_CPUID values should be validated by simple bit
> masking.

So, we can get a lower value on the numeric of CPUID[EAX=14h,ECX=1].EAX[2:0] 
between different host. How about other bits of CPUID[14] ? Can we do like this 
as well?

Thanks,
Luwei Kang
> 
> 
> >
> > CPUID[EAX=14h,ECX=0].EAX is the maximum subleaf.  It should be
> > (barring guest bugs) okay to always present leaf 1.
> >
> > Paolo
> 
> --
> Eduardo

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

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

[Kang, Luwei]

> > On Thu, Jan 18, 2018 at 03:44:49PM +0100, Paolo Bonzini wrote:
> >> On 18/01/2018 15:37, Eduardo Habkost wrote:
> >>> On Thu, Jan 18, 2018 at 02:39:57PM +0100, Paolo Bonzini wrote:
>  On 18/01/2018 14:24, Eduardo Habkost wrote:
> > However, if there's a simple way to make it possible to migrate
> > between hosts with different CPUID[14h] data, it would be even
> > better.  With the current KVM intel-pt implementation, what
> > happens if the CPUID[14h] data seen by the guest doesn't match
> > exactly the CPUID[14h] leaves from the host?
> 
>  Some bits in there can be treated as CPU features (e.g. EBX bit 0
>  "CR3 filtering support").  Probably we should handle these in KVM right 
>  now.
>  KVM needs to compute a mask of valid 1 bits for IA32_RTIT_CTL based
>  on CPUID, and apply it when the MSR is written.
> >>>
> >>> Does this mean QEMU can't set CPUID values that won't match the host
> >>> with the existing implementation, or this won't matter for
> >>> well-behaved guests that don't try to set reserved bits on the MSRs?
> >>
> >> All the features could be handled exactly like regular feature bits.
> >> If QEMU sets them incorrectly and "enforce" is not used, bad things
> >> happen but it's the user's fault.
> >
> > Oh, I mean setting the bit to 0 when it's 1 on the host (if it's
> > 0 on the host, QEMU would never set it anyway).  Is it safe to do it
> > with the current KVM intel-pt implementation?
> 
> It's not, but it's (very) easy to fix.

Hi Paolo,
Do you mean there need to add some check before setting IA32_RTIT_CTL MSR 
in KVM because some bits of this MSR is depend on the result of CPUID[14]. Any 
attempts to change these reserved bit should cause a #GP.

> >
> >>
> >>>
>    It also needs to
>  whitelist bits like we do for other feature words.  These include:
> 
>  - CPUID[EAX=14h,ECX=0].EBX
> 
>  - CPUID[EAX=14h,ECX=0].ECX except bit 31
> 
>  - CPUID[EAX=14h,ECX=1].EAX bits 16:31 (if
>  CPUID[EAX=14h,ECX=0].EBX[3]=1)
> 
>  - CPUID[EAX=14h,ECX=1].EBX (if CPUID[EAX=14h,ECX=0].EBX[1]=1)
> >>>
> >>> What do you mean by whitelist?
> >>
> >> KVM needs to tell QEMU the bits it knows about.

I think kvm_arch_get_supported_cpuid() function can get the result of CPUID[14] 
from KVM. Is this the whitelist what you mentioned?

Thanks,
Luwei Kang

> >
> > So KVM isn't currently doing it on GET_SUPPORTED_CPUID?  Oops.
> >
> >
> >>
>  Others, currently only CPUID[EAX=14h,ECX=0].ECX[31] must match,
>  there is no way to emulate the "wrong" value.
> >>>
> >>> In this case we could make it configurable but require the host and
> >>> guest value to always match.
> >>>
> >>> This might be an obstacle to enabling intel-pt by default (because
> >>> it could make VMs not migratable to newer hosts), but may allow the
> >>> feature to be configured in a predictable way.
> >>
> >> Yeah, but consider that virtualized PT anyway would only be enabled
> >> on Ice Lake processors.  It's a few years away anyway!
> >>
>  Others, currently only CPUID[EAX=14h,ECX=1].EAX[2:0] are numeric
>  values, and it's possible to emulate a lower value than the one in the 
>  processor.
> >>>
> >>> This could be handled by QEMU.  There's no requirement that all
> >>> GET_SUPPORTED_CPUID values should be validated by simple bit
> >>> masking.
> >>
> >> Good!
> >>
> >> Paolo
> >


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

Re: [libvirt] [PATCH] libvirtd: Explicit dependency on systemd-machined

[Cedric Bosdonnat]

The patch looks good to me, though I'm not a systemd expert.
If no one vetoes it I'll push your patch by the end of the day.

--
Cedric

On Mon, 2018-01-22 at 09:10 +0100, Michal Koutný wrote:
> Hello list.
> 
> On 01/10/2018 11:06 PM, Michal Koutný  wrote:
> > [...]
> 
> I see there's no response to the patch. Do you have any comments on this
> patch?
> 
> Thanks,
> Michal
> 
> --
> 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] libvirtd: Explicit dependency on systemd-machined

[Michal Koutný]

Hello list.

On 01/10/2018 11:06 PM, Michal Koutný  wrote:
> [...]
I see there's no response to the patch. Do you have any comments on this
patch?

Thanks,
Michal



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