Re: [libvirt] NBD TLS support in QEMU
On Fri, Sep 05, 2014 at 08:23:17AM +0200, Michal Privoznik wrote: > On 03.09.2014 18:44, Stefan Hajnoczi wrote: > >Hi, > >QEMU offers both NBD client and server functionality. The NBD protocol > >runs unencrypted, which is a problem when the client and server > >communicate over an untrusted network. > > This is not problem for NBD only, but for the rest of data that qemu sends > over network, i.e. migration stream, VNC/SPICE, ... We already have TLS support for VNC and SPICE. We do need it for NBD, migration and the chardev TCP backend. I'd suggest that it is likely possible to add support for NBD, migration and chardev all at the same time by doing a general purpose TLS socket wrapper in QEMU that all those areas can use. > >The particular use case that prompted this mail is storage migration in > >OpenStack. The goal is to encrypt the NBD connection between source and > >destination hosts during storage migration. > > > >I think we can integrate TLS into the NBD protocol as an optional flag. > >A quick web search does not reveal existing open source SSL/TLS NBD > >implementations. I do see a VMware NBDSSL protocol but there is no > >specification so I guess it is proprietary. > > In case of libvirt, we have so called tunnelled migration (both spelled & > misspelled :P) in which libvirt opens a local ports on both src & dst side > and then sets up secured forwarding pipe to the other side. Or a insecured > one if user wishes so. The only problem is that when I adapted libvirt for > NBD, I intentionally forbade NBD in tunnelled migration as it requires one > more data stream for which libvirt migration protocol is not ready yet. > Having saidy that, I feel that libvirt is the show stopper here, not QEMU. While tunnelled migration via libvirt is doable, it is very much sub-optimal as it involves a great many data copies which is bad for performance of migration. This is the main reason that having direct TLS support in the QEMU migration and NBD data channels is desired. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] NBD TLS support in QEMU
On Fri, Sep 05, 2014 at 12:02:18AM +0200, Wouter Verhelst wrote: > [Cc: to nbd-general list added] > > On Wed, Sep 03, 2014 at 05:44:17PM +0100, Stefan Hajnoczi wrote: > > Hi, > > QEMU offers both NBD client and server functionality. The NBD protocol > > runs unencrypted, which is a problem when the client and server > > communicate over an untrusted network. > > > > The particular use case that prompted this mail is storage migration in > > OpenStack. The goal is to encrypt the NBD connection between source and > > destination hosts during storage migration. > > I've never given encrypted NBD high priority, since I don't think > encryption without authentication serves much purpose -- and I haven't > gotten around to adding authentication yet (for which I have plans; but > other things have priority). While have an authentication layer like SASL wired into the NBD protocol would be nice, it shouldn't be considered a blocker / pre-requisite. It is pretty straightforward for the server to require x509 certificates from the client and validate those as a means of authentication. We've used that as an authentication mechanism in libvirt and VNC with success, though we did later add SASL integration as an option too. > > I think we can integrate TLS into the NBD protocol as an optional flag. > > A quick web search does not reveal existing open source SSL/TLS NBD > > implementations. I do see a VMware NBDSSL protocol but there is no > > specification so I guess it is proprietary. > > > > The NBD protocol starts with a negotiation phase. This would be the > > appropriate place to indicate that TLS will be used. After client and > > server complete TLS setup the connection can continue as normal. > > > > Besides QEMU, the userspace NBD tools (http://nbd.sf.net/) can also be > > extended to support TLS. In this case the kernel needs a localhost > > socket and userspace handles TLS. > > That introduces a possibility for a deadlock, since now your network > socket isn't on the PF_MEMALLOC-protected socket anymore, which will > cause the kernel to throw away packets which are needed for your nbd > connection, in hopes of clearing some memory. > > I suppose you could theoretically do the encryption in kernel space. > Not convinced that trying TLS in kernel space is a good idea, though. > > I have heard of people using stunnel or the likes to pipe the NBD > protocol over a secure channel, with various levels of success. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 04/18] blockjob: split out block info monitor handling
On 09/04/14 21:30, Eric Blake wrote:
> On 09/04/2014 09:39 AM, Peter Krempa wrote:
>> On 08/31/14 06:02, Eric Blake wrote:
>>> Another layer of overly-multiplexed code that deserves to be
>>> split into obviously separate paths for query vs. modify.
>>> This continues the cleanup started in the previous patch.
>>>
>>> In the process, make some tweaks to simplify the logic when
>>> parsing the JSON reply. There should be no user-visible
>>> semantic changes.
>>>
>
>>> +int qemuMonitorBlockJobInfo(qemuMonitorPtr mon,
>>> +const char *device,
>>> +virDomainBlockJobInfoPtr info)
>>> +ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>>
>> Implementation of this function doesn't check for presence of the @info
>> structure and dereferences it always. You should mark argument 3 nonnull
>> too.
>
> Good catch.
>
>
>>> -for (i = 0; i < nr_results; i++) {
>>> +for (i = 0, ret = 0; i < nr_results && ret == 0; i++) {
>>> virJSONValuePtr entry = virJSONValueArrayGet(data, i);
>>> if (!entry) {
>>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>> _("missing array element"));
>>> -return -1;
>>> +ret = -1;
>>> +goto cleanup;
>>> }
>>> -if (qemuMonitorJSONGetBlockJobInfoOne(entry, device, info) == 0)
>>> -return 1;
>>> +ret = qemuMonitorJSONGetBlockJobInfoOne(entry, device, info);
>>
>> This doesn't break the loop, thus if the device info isn't last in the
>> structure you'd overwrite the return code. I presume the clients don't
>> check that far but you've documented that semantics.
>
> Actually, the loop DOES break, just on the next iteration. I added '&&
> ret == 0' to the loop continuation condition. So the semantics are:
>
> InfoOne fails and returns -1, the next iteration is aborted and overall
> ret is -1
>
> InfoOne returns 0 because the requested device didn't match, so the loop
> continues to look at the next array member. If all array members are
> exhausted, ret remains 0 and overall return is 0 for job not found.
>
> InfoOne succeeds and returns 1, the next iteration is aborted and the
> overall ret is 1 for info populated.
I actually figured out that you probably changed the loop condition when
I walked out of the office :D Unfortunately I wasn't around a computer
to verify and correct myself.
ACK to the original patch if you add ATTRIBUTE_NONNUL to the correct places.
Peter
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH-RFC] qemu: Add network bandwidth setting for ethernet interfaces
On Thu, Sep 04, 2014 at 03:02:54PM -0700, Anirban Chakraborty wrote:
ethernet interfaces in libvirt currently do not support bandwidth setting.
For example, following xml file for an interface will not apply these
settings to corresponding qdiscs.
-
This patch fixes the behavior.
Although this doesn't confuse git, it might confuse something else.
Leaving it without '' is ok.
Please review it and if it appears ok, please apply.
thanks,
Anirban Chakraborty
No need to have this in the commit message, it's kept in the log then.
Signed-off-by: Anirban Chakraborty
---
src/qemu/qemu_command.c | 5 +
src/qemu/qemu_hotplug.c | 3 +++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2184caa..258c6a7 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7251,6 +7251,11 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
if (tapfd[0] < 0)
goto cleanup;
}
+ /* Configure network bandwidth for ethernet type network interfaces */
+ if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)
+ if (virNetDevBandwidthSet(net->ifname,
+ virDomainNetGetActualBandwidth(net), false) < 0)
+ goto cleanup;
In libvirt, we indent by spaces. This would be caught by running
'make syntax-check'. Anyway, running 'make syntax-check check' is a
good practice to follow before formatting the patches.
if ((actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a364c52..aeb53c5 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -940,6 +940,9 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize)
< 0)
goto cleanup;
} else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) {
+ if (virNetDevBandwidthSet(net->ifname,
+ virDomainNetGetActualBandwidth(net), false) < 0)
+ goto cleanup;
Same here.
We need to clear the bandwidth settings when shutting down the domain
or unplugging the device.
Looking forward to v2,
Martin
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Don't include non-migratable features in host-model
Commit fba6bc4 introduced supoprt for the 'invtsc' feature,
which blocks migration. We should not include it in the
host-model CPU by default, because it's intended to be used
with migration.
https://bugzilla.redhat.com/show_bug.cgi?id=1138221
---
src/cpu/cpu_map.xml | 2 +-
src/cpu/cpu_x86.c | 63 -
2 files changed, 59 insertions(+), 6 deletions(-)
diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml
index 12987a0..18c7b0d 100644
--- a/src/cpu/cpu_map.xml
+++ b/src/cpu/cpu_map.xml
@@ -328,7 +328,7 @@
-
+
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index af2e08e..21a7007 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -89,6 +89,7 @@ struct x86_map {
struct x86_vendor *vendors;
struct x86_feature *features;
struct x86_model *models;
+struct x86_feature *migrate_blockers;
};
static struct x86_map* virCPUx86Map = NULL;
@@ -592,6 +593,28 @@ x86FeatureFree(struct x86_feature *feature)
static struct x86_feature *
+x86FeatureCopy(const struct x86_feature *src)
+{
+struct x86_feature *feature;
+
+if (VIR_ALLOC(feature) < 0)
+return NULL;
+
+if (VIR_STRDUP(feature->name, src->name) < 0)
+goto error;
+
+if ((feature->data = x86DataCopy(src->data)) == NULL)
+goto error;
+
+return feature;
+
+ error:
+x86FeatureFree(feature);
+return NULL;
+}
+
+
+static struct x86_feature *
x86FeatureFind(const struct x86_map *map,
const char *name)
{
@@ -677,6 +700,9 @@ x86FeatureLoad(xmlXPathContextPtr ctxt,
int ret = 0;
size_t i;
int n;
+char *str = NULL;
+bool migratable = true;
+struct x86_feature *migrate_blocker = NULL;
if (!(feature = x86FeatureNew()))
goto error;
@@ -694,6 +720,10 @@ x86FeatureLoad(xmlXPathContextPtr ctxt,
goto ignore;
}
+str = virXPathString("string(@migratable)", ctxt);
+if (STREQ_NULLABLE(str, "no"))
+migratable = false;
+
n = virXPathNodeSet("./cpuid", ctxt, &nodes);
if (n < 0)
goto ignore;
@@ -710,6 +740,14 @@ x86FeatureLoad(xmlXPathContextPtr ctxt,
goto error;
}
+if (!migratable) {
+if ((migrate_blocker = x86FeatureCopy(feature)) == NULL)
+goto error;
+
+migrate_blocker->next = map->migrate_blockers;
+map->migrate_blockers = migrate_blocker;
+}
+
if (map->features == NULL) {
map->features = feature;
} else {
@@ -720,6 +758,7 @@ x86FeatureLoad(xmlXPathContextPtr ctxt,
out:
ctxt->node = ctxt_node;
VIR_FREE(nodes);
+VIR_FREE(str);
return ret;
@@ -728,6 +767,7 @@ x86FeatureLoad(xmlXPathContextPtr ctxt,
ignore:
x86FeatureFree(feature);
+x86FeatureFree(migrate_blocker);
goto out;
}
@@ -1093,6 +1133,12 @@ x86MapFree(struct x86_map *map)
x86VendorFree(vendor);
}
+while (map->migrate_blockers != NULL) {
+struct x86_feature *migrate_blocker = map->migrate_blockers;
+map->migrate_blockers = migrate_blocker->next;
+x86FeatureFree(migrate_blocker);
+}
+
VIR_FREE(map);
}
@@ -2025,16 +2071,15 @@ x86UpdateHostModel(virCPUDefPtr guest,
const virCPUDef *host)
{
virCPUDefPtr oldguest = NULL;
+const struct x86_map *map;
+const struct x86_feature *feat;
size_t i;
int ret = -1;
guest->match = VIR_CPU_MATCH_EXACT;
-/* no updates are required */
-if (guest->nfeatures == 0) {
-virCPUDefFreeModel(guest);
-return virCPUDefCopyModel(guest, host, true);
-}
+if (!(map = virCPUx86GetMap()))
+goto cleanup;
/* update the host model according to the desired configuration */
if (!(oldguest = virCPUDefCopy(guest)))
@@ -2044,6 +2089,14 @@ x86UpdateHostModel(virCPUDefPtr guest,
if (virCPUDefCopyModel(guest, host, true) < 0)
goto cleanup;
+/* Remove non-migratable features by default */
+for (i = 0; i < guest->nfeatures; i++) {
+for (feat = map->migrate_blockers; feat; feat = feat->next) {
+if (STREQ(feat->name, guest->features[i].name))
+VIR_DELETE_ELEMENT_INPLACE(guest->features, i,
guest->nfeatures);
+}
+}
+
for (i = 0; i < oldguest->nfeatures; i++) {
if (virCPUDefUpdateFeature(guest,
oldguest->features[i].name,
--
1.8.5.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 09/18] blockjob: add new --bytes flag to virsh blockjob
On 08/31/14 06:02, Eric Blake wrote: > Expose the new flag just added to virDomainGetBlockJobInfo. > With --raw, the presence or absence of --bytes determines which > flag to use in the single API call. Without --raw, the use of > --bytes forces an error if the server doesn't support it, > otherwise, the code tries to silently fall back to scaling the > MiB/s value. > > My goal is to eventually also support --bytes in bandwidth mode; > but that's a bit further down the road (and needs a new API flag > added in libvirt.h first). > > This changes the human output, but the previous patch added > raw output precisely so that we can have flexibility with the > human output. For this commit, I used qemu-monitor-command to > force an unusual bandwidth, but the same will be possible once > qemu implements virDomainBlockCopy: > > Before: > Block Copy: [100 %]Bandwidth limit: 2 MiB/s > After: > Block Copy: [100 %]Bandwidth limit: 1048577 bytes/s (1.000 MiB/s) > > The cache avoids having to repeatedly attempt a flag probe when > talking to an older server, when multiple blockjob commands are > issued during a batch session and the user is manually polling > for job completion. > > * tools/virsh.h (_vshControl): Add a cache. > * tools/virsh.c (cmdConnect, vshReconnect): Initialize the cache. > * tools/virsh-domain.c (opts_block_job): Add --bytes. > * tools/virsh.pod (blockjob): Document this. > > Signed-off-by: Eric Blake > --- > tools/virsh-domain.c | 64 > ++-- > tools/virsh.c| 2 ++ > tools/virsh.h| 2 ++ > tools/virsh.pod | 8 +-- > 4 files changed, 67 insertions(+), 9 deletions(-) > ACK Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 10/18] blockcopy: allow block device destination
On 08/31/14 06:02, Eric Blake wrote: > To date, anyone performing a block copy and pivot ends up with > the destination being treated as . While this > works for data access for a block device, it has at least one > noticeable shortcoming: virDomainGetBlockInfo() reports allocation > differently for block devices visited as files (the size of the > device) than for block devices visited as > (the maximum sector used, as reported by qemu); and this difference > is significant when trying to manage qcow2 format on block devices > that can be grown as needed. > > Of course, the more powerful virDomainBlockCopy() API can already > express the ability to set the type. But a new API can't > be backported, while a new flag to an existing API can; and it is > also rather inconvenient to have to resort to the full power of > generating XML when just adding a flag to the older call will do > the trick. So this patch enhances blockcopy to let the user flag > when the resulting XML after the copy must list the device as > type='block'. > > * include/libvirt/libvirt.h.in (VIR_DOMAIN_BLOCK_REBASE_COPY_DEV): > New flag. > * src/libvirt.c (virDomainBlockRebase): Document it. > * tools/virsh-domain.c (opts_block_copy, blockJobImpl): Add > --blockdev option. > * tools/virsh.pod (blockcopy): Document it. > * src/qemu/qemu_driver.c (qemuDomainBlockRebase): Allow new flag. > (qemuDomainBlockCopy): Remember the flag, and make sure it is only > used on actual block devices. > * tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml: Test it. > > Signed-off-by: Eric Blake > --- > include/libvirt/libvirt.h.in | 2 ++ > src/libvirt.c | 8 +++-- > src/qemu/qemu_driver.c | 36 > ++ > .../qemuxml2argvdata/qemuxml2argv-disk-mirror.xml | 4 +-- > tools/virsh-domain.c | 6 > tools/virsh.pod| 7 +++-- > 6 files changed, 45 insertions(+), 18 deletions(-) > ACK Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 11/18] blockcopy: split out virsh implementation
On 08/31/14 06:02, Eric Blake wrote:
> I'm about to extend the capabilities of blockcopy. Hiding a few
> common lines of implementation gets in the way of the new required
> logic, and putting the new logic in the common implementation won't
> benefit any of the other blockjob operations. Therefore, it is
> simpler to just do the work inline. There should be no semantic
> change in this patch.
>
> * tools/virsh-domain.c (blockJobImpl): Drop unused variable. Move
> block copy guts...
> (cmdBlockCopy): ...into their lone caller.
>
> Signed-off-by: Eric Blake
> ---
> tools/virsh-domain.c | 45 ++---
> 1 file changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 813d8e0..b92b2eb 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -1907,6 +1894,9 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
> bool quit = false;
> int abort_flags = 0;
>
> +if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0)
> +return false;
> +
> blocking |= vshCommandOptBool(cmd, "timeout") || pivot || finish;
> if (blocking) {
> if (pivot && finish) {
...
> @@ -1935,7 +1923,26 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
> return false;
> }
>
> -if (!blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_COPY, &dom))
> +if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
> +goto cleanup;
> +
> +if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) {
> +vshError(ctl, "%s", _("bandwidth must be a number"));
> +goto cleanup;
> +}
> +
> +if (vshCommandOptBool(cmd, "shallow"))
> +flags |= VIR_DOMAIN_BLOCK_REBASE_SHALLOW;
> +if (vshCommandOptBool(cmd, "reuse-external"))
> +flags |= VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT;
> +if (vshCommandOptBool(cmd, "raw"))
> +flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_RAW;
> +if (vshCommandOptBool(cmd, "blockdev"))
> +flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_DEV;
> +if (vshCommandOptStringReq(ctl, cmd, "dest", &dest) < 0)
> +goto cleanup;
> +
You could extract the flags and string at the top where you extract
@path to be consistent, but that's more bikeshedding than useful.
> +if (virDomainBlockRebase(dom, path, dest, bandwidth, flags) < 0)
> goto cleanup;
>
> if (!blocking) {
>
ACK as-is.
Peter
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 11/18] blockcopy: split out virsh implementation
On 09/05/14 11:29, Peter Krempa wrote:
> On 08/31/14 06:02, Eric Blake wrote:
>> I'm about to extend the capabilities of blockcopy. Hiding a few
>> common lines of implementation gets in the way of the new required
>> logic, and putting the new logic in the common implementation won't
>> benefit any of the other blockjob operations. Therefore, it is
>> simpler to just do the work inline. There should be no semantic
>> change in this patch.
>>
>> * tools/virsh-domain.c (blockJobImpl): Drop unused variable. Move
>> block copy guts...
>> (cmdBlockCopy): ...into their lone caller.
>>
>> Signed-off-by: Eric Blake
>> ---
>> tools/virsh-domain.c | 45 ++---
>> 1 file changed, 26 insertions(+), 19 deletions(-)
>>
>> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>> index 813d8e0..b92b2eb 100644
>> --- a/tools/virsh-domain.c
>> +++ b/tools/virsh-domain.c
>
>
>> @@ -1907,6 +1894,9 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
>> bool quit = false;
>> int abort_flags = 0;
>>
>> +if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0)
>> +return false;
>> +
>> blocking |= vshCommandOptBool(cmd, "timeout") || pivot || finish;
>> if (blocking) {
>> if (pivot && finish) {
>
> ...
>
>> @@ -1935,7 +1923,26 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
>> return false;
>> }
>>
>> -if (!blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_COPY, &dom))
>> +if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>> +goto cleanup;
>> +
>> +if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) {
>> +vshError(ctl, "%s", _("bandwidth must be a number"));
>> +goto cleanup;
>> +}
>> +
>> +if (vshCommandOptBool(cmd, "shallow"))
>> +flags |= VIR_DOMAIN_BLOCK_REBASE_SHALLOW;
>> +if (vshCommandOptBool(cmd, "reuse-external"))
>> +flags |= VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT;
>> +if (vshCommandOptBool(cmd, "raw"))
>> +flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_RAW;
>> +if (vshCommandOptBool(cmd, "blockdev"))
>> +flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_DEV;
>> +if (vshCommandOptStringReq(ctl, cmd, "dest", &dest) < 0)
>> +goto cleanup;
>> +
>
> You could extract the flags and string at the top where you extract
> @path to be consistent, but that's more bikeshedding than useful.
>
Hmmm, next patch is moving it to the destination I've suggested. It'd be
better to move it to the correct position right away then ...
>> +if (virDomainBlockRebase(dom, path, dest, bandwidth, flags) < 0)
>> goto cleanup;
>>
>> if (!blocking) {
>>
>
> ACK as-is.
>
> Peter
>
>
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Don't include non-migratable features in host-model
On Fri, Sep 05, 2014 at 11:04:30 +0200, Jano Tomko wrote:
> Commit fba6bc4 introduced supoprt for the 'invtsc' feature,
s/supoprt/support/
> which blocks migration. We should not include it in the
> host-model CPU by default, because it's intended to be used
> with migration.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1138221
> ---
> src/cpu/cpu_map.xml | 2 +-
> src/cpu/cpu_x86.c | 63
> -
> 2 files changed, 59 insertions(+), 6 deletions(-)
>
> diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml
> index 12987a0..18c7b0d 100644
> --- a/src/cpu/cpu_map.xml
> +++ b/src/cpu/cpu_map.xml
> @@ -328,7 +328,7 @@
>
>
>
> -
> +
>
>
>
> diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
> index af2e08e..21a7007 100644
> --- a/src/cpu/cpu_x86.c
> +++ b/src/cpu/cpu_x86.c
...
> @@ -2044,6 +2089,14 @@ x86UpdateHostModel(virCPUDefPtr guest,
> if (virCPUDefCopyModel(guest, host, true) < 0)
> goto cleanup;
>
> +/* Remove non-migratable features by default */
> +for (i = 0; i < guest->nfeatures; i++) {
> +for (feat = map->migrate_blockers; feat; feat = feat->next) {
> +if (STREQ(feat->name, guest->features[i].name))
> +VIR_DELETE_ELEMENT_INPLACE(guest->features, i,
> guest->nfeatures);
> +}
> +}
> +
OK, this should be working as long as no CPU model contains invtsc
directly. Since this is currently true, I can live with this simple
solution for now. However, I think it's worth mentioning this limitation
in the comment above.
> for (i = 0; i < oldguest->nfeatures; i++) {
> if (virCPUDefUpdateFeature(guest,
> oldguest->features[i].name,
However, a new test should be added to cputest{data,.c} :-)
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 11/18] blockcopy: split out virsh implementation
On 09/05/14 11:30, Peter Krempa wrote:
> On 09/05/14 11:29, Peter Krempa wrote:
>> On 08/31/14 06:02, Eric Blake wrote:
>>> I'm about to extend the capabilities of blockcopy. Hiding a few
>>> common lines of implementation gets in the way of the new required
>>> logic, and putting the new logic in the common implementation won't
>>> benefit any of the other blockjob operations. Therefore, it is
>>> simpler to just do the work inline. There should be no semantic
>>> change in this patch.
>>>
>>> * tools/virsh-domain.c (blockJobImpl): Drop unused variable. Move
>>> block copy guts...
>>> (cmdBlockCopy): ...into their lone caller.
>>>
>>> Signed-off-by: Eric Blake
>>> ---
>>> tools/virsh-domain.c | 45 ++---
>>> 1 file changed, 26 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>>> index 813d8e0..b92b2eb 100644
>>> --- a/tools/virsh-domain.c
>>> +++ b/tools/virsh-domain.c
>>
>>
>>> @@ -1907,6 +1894,9 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
>>> bool quit = false;
>>> int abort_flags = 0;
>>>
>>> +if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0)
>>> +return false;
>>> +
>>> blocking |= vshCommandOptBool(cmd, "timeout") || pivot || finish;
>>> if (blocking) {
>>> if (pivot && finish) {
>>
>> ...
>>
>>> @@ -1935,7 +1923,26 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
>>> return false;
>>> }
>>>
>>> -if (!blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_COPY, &dom))
>>> +if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>>> +goto cleanup;
>>> +
>>> +if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) {
>>> +vshError(ctl, "%s", _("bandwidth must be a number"));
>>> +goto cleanup;
>>> +}
>>> +
>>> +if (vshCommandOptBool(cmd, "shallow"))
>>> +flags |= VIR_DOMAIN_BLOCK_REBASE_SHALLOW;
>>> +if (vshCommandOptBool(cmd, "reuse-external"))
>>> +flags |= VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT;
>>> +if (vshCommandOptBool(cmd, "raw"))
>>> +flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_RAW;
>>> +if (vshCommandOptBool(cmd, "blockdev"))
>>> +flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_DEV;
>>> +if (vshCommandOptStringReq(ctl, cmd, "dest", &dest) < 0)
>>> +goto cleanup;
>>> +
>>
>> You could extract the flags and string at the top where you extract
>> @path to be consistent, but that's more bikeshedding than useful.
>>
>
> Hmmm, next patch is moving it to the destination I've suggested. It'd be
> better to move it to the correct position right away then ...
>
Actually the code is not moved but refactored. Never mind the comment
above :) sorry for the noise.
Peter
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 12/18] blockcopy: expose new API in virsh
On 08/31/14 06:02, Eric Blake wrote:
> Expose the new power of virDomainBlockCopy through virsh (well,
> all but the finer-grained bandwidth, as that is its own can of
> worms for a later patch). Continue to use the older API where
> possible, for maximum compatibility.
>
> The command now requires either --dest (with optional --format
> and --blockdev), to directly describe the file destination, or
> --xml, to name a file that contains an XML description such as:
>
>
>
>
>
>
>
>
> Non-zero option parameters are converted into virTypedParameters,
> and if anything requires the new API, the command can synthesize
> appropriate XML even if the --dest option was used instead of --xml.
>
> The existing --raw flag remains for back-compat, but the preferred
> spelling is now --format=raw, since the new API now allows us
> to specify all formats rather than just a boolean raw to suppress
> probing.
>
> I hope I did justice in describing the effects of granularity and
> buf-size on how they get passed through to qemu.
>
> * tools/virsh-domain.c (cmdBlockCopy): Add new options --xml,
> --granularity, --buf-size, --format. Make --raw an alias for
> --format=raw. Call new API if new parameters are in use.
> * tools/virsh.pod (blockcopy): Document new options.
>
> Signed-off-by: Eric Blake
> ---
> tools/virsh-domain.c | 145
> +--
> tools/virsh.pod | 45 +---
> 2 files changed, 156 insertions(+), 34 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index b92b2eb..16d7f05 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -1893,6 +1912,23 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
> const char *path = NULL;
> bool quit = false;
> int abort_flags = 0;
> +const char *xml = NULL;
> +char *xmlstr = NULL;
> +virTypedParameterPtr params = NULL;
> +int nparams = 0;
> +
> +if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0)
> +return false;
> +if (vshCommandOptString(cmd, "dest", &dest) < 0)
> +return false;
> +if (vshCommandOptString(cmd, "xml", &xml) < 0)
> +return false;
> +if (vshCommandOptString(cmd, "format", &format) < 0)
> +return false;
> +
> +VSH_EXCLUSIVE_OPTIONS_VAR(dest, xml);
These were designed to work on booleans, but pointers are fortunately
sharing the semantics :)
> +VSH_EXCLUSIVE_OPTIONS_VAR(format, xml);
> +VSH_EXCLUSIVE_OPTIONS_VAR(blockdev, xml);
>
> if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0)
> return false;
> @@ -1926,24 +1962,101 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
> if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
> goto cleanup;
>
> +/* XXX: Parse bandwidth as scaled input, rather than forcing
> + * MiB/s, and either reject negative input or treat it as 0 rather
> + * than trying to guess which value will work well across both
> + * APIs with their different sizes and scales. */
> if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) {
> vshError(ctl, "%s", _("bandwidth must be a number"));
> goto cleanup;
> }
> +if (vshCommandOptUInt(cmd, "granularity", &granularity) < 0) {
> +vshError(ctl, "%s", _("granularity must be a number"));
> +goto cleanup;
> +}
> +if (vshCommandOptULongLong(cmd, "buf-size", &buf_size) < 0) {
> +vshError(ctl, "%s", _("buf-size must be a number"));
> +goto cleanup;
> +}
>
> +if (xml) {
> +if (virFileReadAll(xml, 8192, &xmlstr) < 0) {
> +vshReportError(ctl);
> +goto cleanup;
> +}
> +} else if (!dest) {
> +vshError(ctl, "%s", _("need either --dest or --xml"));
> +goto cleanup;
> +}
> +
> +/* Exploit that VIR_DOMAIN_BLOCK_REBASE_* and
> + * VIR_DOMAIN_BLOCK_COPY_* flags have the same values. */
> if (vshCommandOptBool(cmd, "shallow"))
> flags |= VIR_DOMAIN_BLOCK_REBASE_SHALLOW;
> if (vshCommandOptBool(cmd, "reuse-external"))
> flags |= VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT;
> -if (vshCommandOptBool(cmd, "raw"))
> -flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_RAW;
> -if (vshCommandOptBool(cmd, "blockdev"))
> -flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_DEV;
> -if (vshCommandOptStringReq(ctl, cmd, "dest", &dest) < 0)
> -goto cleanup;
> -
> -if (virDomainBlockRebase(dom, path, dest, bandwidth, flags) < 0)
> -goto cleanup;
> +
> +if (granularity || buf_size || (format && STRNEQ(format, "raw")) || xml)
> {
> +/* New API */
> +if (bandwidth || granularity || buf_size) {
> +params = vshCalloc(ctl, 3, sizeof(*params));
> +if (bandwidth) {
> +/* bandwidth is ulong MiB/s, but the typed parameter is
> + * ullong bytes/s; make sure we don't overflow */
> +if (bandwidt
Re: [libvirt] [PATCH v3 13/18] blockcopy: remote implementation for new API
On 08/31/14 06:02, Eric Blake wrote: > Fairly straightforward - I got lucky that the generated functions > worked out of the box :) > > * src/remote/remote_protocol.x (remote_domain_block_copy_args): > New struct. > (REMOTE_PROC_DOMAIN_BLOCK_COPY): New RPC. > * src/remote/remote_driver.c (remote_driver): Wire it up. > * src/remote_protocol-structs: Regenerate. > > Signed-off-by: Eric Blake > --- > src/remote/remote_driver.c | 1 + > src/remote/remote_protocol.x | 18 +- > src/remote_protocol-structs | 11 +++ > 3 files changed, 29 insertions(+), 1 deletion(-) > ACK, Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC] qemu: make time adjustment persistent if RTC changes in guest
On 2014/9/5 1:40, Laine Stump wrote:
> I do point out the existence of exactly the problem that you are fixing.
> However, there is a problem with the way you're fixing it if the clock
> for the domain is set like this:
>
>
>
> The problem is that when the domain was started, its *state* (but not
> persistent config) was modified to convert the localtime adjustment into
> an adjustment relative to UTC (and the basis type in the state was of
> course also changed from VIR_DOMAIN_BASIS_TYPE_LOCALTIME to
> VIR_DOMAIN_BASIS_TYPE_UTC). See this commit for details:
>
>
> http://libvirt.org/git/?p=libvirt.git;a=commit;h=cde8ca2dfda6b69eebb70695e5a42b0cc6bd2c38
Thanks to your reply.
I think I understand what you mean. If basis is 'localtime', the active config
is modified to basis='utc' and of course 'adjustment' is adjusted to a new
offset
which is relative to UTC.
But I met the opposite result.
My host's timezone was CST(UTC +8:00). Clock configuration is
.
I started the VM and found the unexpected result.
In the active config(/var/run/libvirt/qemu/*.xml), basis was utc,
but adjustment was 120(which was expected to 120 + 8*3600).
In the persistent config, basis was localtime and adjustment was 120.
Use gdb and set breakpoint to qemuProcessHandleRTCChange. Adjust the VM's
hardware clock. The result also shows me the state(active config) is not
right.
(gdb) p vm->def->clock.data
$3 = {utc_reset = 120, variable = {adjustment = 120, basis = 0, adjustment0 =
120}, timezone = 0x78 }
(gdb) p vm->newDef->clock.data
$4 = {utc_reset = 120, variable = {adjustment = 120, basis = 1, adjustment0 =
0}, timezone = 0x78 }
The difference between vm->def and vm->newDef is there
clock.data.variable.basis.
Your patch[1] modified the basis to utc, but it seems to only take effect to
active config.
[1]http://libvirt.org/git/?p=libvirt.git;a=commit;h=cde8ca2dfda6b69eebb70695e5a42b0cc6bd2c38
> (there is also some discussion of why this was done in the mailing list
> archives for that patch and for others in the same series).
>
> So if you just copy that UTC-based adjustment into the persistent
> config, it will now be incorrect by the difference between UTC and
> localtime. Even doing the math to "adjust the adjustment" by that amount
> would end up with an incorrect result if the domain passed through a
> daylight time to normal time boundary while it was running (including
> migrating from a host in one timezone to a host in another timezone that
> currently had a different setting for DST). And you can't modify the
> basis type in the persistent config because... well, just *because* :-)
> (seriously, I think that would be taking too many liberties with the
> user-supplied config).
>
> On the other hand, the documentation for
> specifically says that a change to the RTC will be preserved across
> domain restarts (well, it says "reboots", which technically is different
> as it doesn't involve terminating and restarting the qemu process, but I
> think we all know what the author intended to say there).
If my result above it true(if I don't miss anything), I think we should
resolve the issue of active config not the persistent one.
And fortunately my patch seems to do the right thing because of the
localtime-basis adjustment in persistent config.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2] Don't include non-migratable features in host-model
Commit fba6bc4 introduced support for the 'invtsc' feature,
which blocks migration. We should not include it in the
host-model CPU by default, because it's intended to be used
with migration.
https://bugzilla.redhat.com/show_bug.cgi?id=1138221
---
v2: added tests and comment, fixed a typo
src/cpu/cpu_map.xml | 2 +-
src/cpu/cpu_x86.c| 65 ++--
tests/cputest.c | 1 +
tests/cputestdata/x86-host-invtsc+host-model.xml | 22
tests/cputestdata/x86-host-invtsc.xml| 27 ++
5 files changed, 111 insertions(+), 6 deletions(-)
create mode 100644 tests/cputestdata/x86-host-invtsc+host-model.xml
create mode 100644 tests/cputestdata/x86-host-invtsc.xml
diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml
index 12987a0..18c7b0d 100644
--- a/src/cpu/cpu_map.xml
+++ b/src/cpu/cpu_map.xml
@@ -328,7 +328,7 @@
-
+
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index af2e08e..7571f16 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -89,6 +89,7 @@ struct x86_map {
struct x86_vendor *vendors;
struct x86_feature *features;
struct x86_model *models;
+struct x86_feature *migrate_blockers;
};
static struct x86_map* virCPUx86Map = NULL;
@@ -592,6 +593,28 @@ x86FeatureFree(struct x86_feature *feature)
static struct x86_feature *
+x86FeatureCopy(const struct x86_feature *src)
+{
+struct x86_feature *feature;
+
+if (VIR_ALLOC(feature) < 0)
+return NULL;
+
+if (VIR_STRDUP(feature->name, src->name) < 0)
+goto error;
+
+if ((feature->data = x86DataCopy(src->data)) == NULL)
+goto error;
+
+return feature;
+
+ error:
+x86FeatureFree(feature);
+return NULL;
+}
+
+
+static struct x86_feature *
x86FeatureFind(const struct x86_map *map,
const char *name)
{
@@ -677,6 +700,9 @@ x86FeatureLoad(xmlXPathContextPtr ctxt,
int ret = 0;
size_t i;
int n;
+char *str = NULL;
+bool migratable = true;
+struct x86_feature *migrate_blocker = NULL;
if (!(feature = x86FeatureNew()))
goto error;
@@ -694,6 +720,10 @@ x86FeatureLoad(xmlXPathContextPtr ctxt,
goto ignore;
}
+str = virXPathString("string(@migratable)", ctxt);
+if (STREQ_NULLABLE(str, "no"))
+migratable = false;
+
n = virXPathNodeSet("./cpuid", ctxt, &nodes);
if (n < 0)
goto ignore;
@@ -710,6 +740,14 @@ x86FeatureLoad(xmlXPathContextPtr ctxt,
goto error;
}
+if (!migratable) {
+if ((migrate_blocker = x86FeatureCopy(feature)) == NULL)
+goto error;
+
+migrate_blocker->next = map->migrate_blockers;
+map->migrate_blockers = migrate_blocker;
+}
+
if (map->features == NULL) {
map->features = feature;
} else {
@@ -720,6 +758,7 @@ x86FeatureLoad(xmlXPathContextPtr ctxt,
out:
ctxt->node = ctxt_node;
VIR_FREE(nodes);
+VIR_FREE(str);
return ret;
@@ -728,6 +767,7 @@ x86FeatureLoad(xmlXPathContextPtr ctxt,
ignore:
x86FeatureFree(feature);
+x86FeatureFree(migrate_blocker);
goto out;
}
@@ -1093,6 +1133,12 @@ x86MapFree(struct x86_map *map)
x86VendorFree(vendor);
}
+while (map->migrate_blockers != NULL) {
+struct x86_feature *migrate_blocker = map->migrate_blockers;
+map->migrate_blockers = migrate_blocker->next;
+x86FeatureFree(migrate_blocker);
+}
+
VIR_FREE(map);
}
@@ -2025,16 +2071,15 @@ x86UpdateHostModel(virCPUDefPtr guest,
const virCPUDef *host)
{
virCPUDefPtr oldguest = NULL;
+const struct x86_map *map;
+const struct x86_feature *feat;
size_t i;
int ret = -1;
guest->match = VIR_CPU_MATCH_EXACT;
-/* no updates are required */
-if (guest->nfeatures == 0) {
-virCPUDefFreeModel(guest);
-return virCPUDefCopyModel(guest, host, true);
-}
+if (!(map = virCPUx86GetMap()))
+goto cleanup;
/* update the host model according to the desired configuration */
if (!(oldguest = virCPUDefCopy(guest)))
@@ -2044,6 +2089,16 @@ x86UpdateHostModel(virCPUDefPtr guest,
if (virCPUDefCopyModel(guest, host, true) < 0)
goto cleanup;
+/* Remove non-migratable features by default
+ * Note: this only works as long as no CPU model contains non-migratable
+ * features directly */
+for (i = 0; i < guest->nfeatures; i++) {
+for (feat = map->migrate_blockers; feat; feat = feat->next) {
+if (STREQ(feat->name, guest->features[i].name))
+VIR_DELETE_ELEMENT_INPLACE(guest->features, i,
guest->nfeatures);
+}
+}
+
for (i = 0; i < oldguest->nfeatures; i++) {
if (virCPUDefUpdateFeature(guest,
oldguest->features[i].name,
diff --git a/tests/cputest.c b/tests/c
Re: [libvirt] [PATCH v2] selinux: Avoid label reservations for type = none
On Thu, Sep 04, 2014 at 02:42:32PM +0530, Shivaprasad G Bhat wrote: For security type='none' libvirt according to the docs should not generate seclabel be it for selinux or any model. So, skip the reservation of labels when type is none. I wrapped the commit message. Signed-off-by: Shivaprasad G Bhat --- src/security/security_selinux.c |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e8c13db..c21e4fe 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -731,7 +731,9 @@ virSecuritySELinuxReserveSecurityLabel(virSecurityManagerPtr mgr, virSecurityLabelDefPtr seclabel; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); -if (!seclabel || seclabel->type == VIR_DOMAIN_SECLABEL_STATIC) +if (!seclabel || +seclabel->type == VIR_DOMAIN_SECLABEL_NONE || +seclabel->type == VIR_DOMAIN_SECLABEL_STATIC) return 0; ACK, and apparmor does handle this already. I'll push in a minute. Martin signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv3.5 05.5/18] blockjob: add new monitor json conversions
The previous patch hoisted some bounds checks to the callers;
but someone that is not aware of the hoisted check could now
try passing an integer between LLONG_MAX and ULLONG_MAX. As a
safety measure, add new json conversion modes that let libvirt
error out early instead of pass bad numbers to qemu, if the
caller ever makes a mistake due to later refactoring.
Convert the various blockjob QMP calls to use the new modes,
and switch some of them to be optional (QMP has always supported
an omitted "speed" the same as "speed":0, for everything except
block-job-set-speed).
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONMakeCommandRaw):
Add 'j'/'y' and 'J'/'Y' to error out on negative input.
(qemuMonitorJSONDriveMirror, qemuMonitorJSONBlockCommit)
(qemuMonitorJSONBlockJob): Use it.
Signed-off-by: Eric Blake
---
This should address Peter's review concerns on 5/18; I could
push it either first or second (although I worded the commit
message to push it second).
src/qemu/qemu_monitor_json.c | 34 --
1 file changed, 28 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 68ba084..13274ea 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -457,10 +457,14 @@ qemuMonitorJSONMakeCommandRaw(bool wrap, const char
*cmdname, ...)
* S: string value, omitted if null
*
* i: signed integer value
+ * j: signed integer value, error if negative
* z: signed integer value, omitted if zero
+ * y: signed integer value, omitted if zero, error if negative
*
* I: signed long integer value
+ * J: signed long integer value, error if negative
* Z: signed long integer value, omitted if zero
+ * Y: signed long integer value, omitted if zero, error if negative
*
* u: unsigned integer value
* p: unsigned integer value, omitted if zero
@@ -500,10 +504,19 @@ qemuMonitorJSONMakeCommandRaw(bool wrap, const char
*cmdname, ...)
} break;
case 'z':
+case 'y':
+case 'j':
case 'i': {
int val = va_arg(args, int);
-if (!val && type == 'z')
+if (val < 0 && (type == 'j' || type == 'y')) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("argument key '%s' must not be negative"),
+ key);
+goto error;
+}
+
+if (!val && (type == 'z' || type == 'y'))
continue;
ret = virJSONValueObjectAppendNumberInt(jargs, key, val);
@@ -520,10 +533,19 @@ qemuMonitorJSONMakeCommandRaw(bool wrap, const char
*cmdname, ...)
} break;
case 'Z':
+case 'Y':
+case 'J':
case 'I': {
long long val = va_arg(args, long long);
-if (!val && type == 'Z')
+if (val < 0 && (type == 'J' || type == 'Y')) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("argument key '%s' must not be negative"),
+ key);
+goto error;
+}
+
+if (!val && (type == 'Z' || type == 'Y'))
continue;
ret = virJSONValueObjectAppendNumberLong(jargs, key, val);
@@ -3408,7 +3430,7 @@ qemuMonitorJSONDriveMirror(qemuMonitorPtr mon,
cmd = qemuMonitorJSONMakeCommand("drive-mirror",
"s:device", device,
"s:target", file,
- "U:speed", speed,
+ "Y:speed", speed,
"s:sync", shallow ? "top" : "full",
"s:mode", reuse ? "existing" :
"absolute-paths",
"S:format", format,
@@ -3474,7 +3496,7 @@ qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char
*device,
cmd = qemuMonitorJSONMakeCommand("block-commit",
"s:device", device,
- "U:speed", speed,
+ "Y:speed", speed,
"S:top", top,
"S:base", base,
"S:backing-file", backingName,
@@ -3850,7 +3872,7 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
cmd_name = modern ? "block-job-set-speed" : "block_job_set_speed";
cmd = qemuMonitorJSONMakeCommand(cmd_name,
"s:device", device,
- modern ? "U:speed" : "U:value", speed,
+ modern ? "J:speed" : "J:value", speed,
NULL);
break;
@@ -3858,7 +3880,7 @@ qemuMonitorJSONBlockJob(qemuMonitorP
Re: [libvirt] [PATCH 1/2] conf: Disallow nonexistent NUMA nodes for hugepages
On Tue, Sep 02, 2014 at 05:12:01PM +0200, Michal Privoznik wrote: As of 136ad4974 it is possible to specify different huge pages per guest NUMA node. However, there's no check if nodeset specified in ./hugepages/page contains only those guest NUMA nodes that exist. In other words with current code it is possible to define meaningless combination: 4 Notice the node 4 in ? And with such XML, the domain will disappear after libvirt updates to a version with the patch below. We need to VIR_WARN() about that and then just delete it from the definition. Or something similar. So NACK to this version. Martin signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] Don't include non-migratable features in host-model
On 09/05/2014 04:48 AM, Ján Tomko wrote: > Commit fba6bc4 introduced support for the 'invtsc' feature, > which blocks migration. We should not include it in the > host-model CPU by default, because it's intended to be used > with migration. > > https://bugzilla.redhat.com/show_bug.cgi?id=1138221 > --- > v2: added tests and comment, fixed a typo > > src/cpu/cpu_map.xml | 2 +- Hmm - I guess we don't have an .rng file that validates this, do we. ACK -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] Don't include non-migratable features in host-model
On 09/05/14 13:55, Eric Blake wrote: > On 09/05/2014 04:48 AM, Ján Tomko wrote: >> Commit fba6bc4 introduced support for the 'invtsc' feature, >> which blocks migration. We should not include it in the >> host-model CPU by default, because it's intended to be used >> with migration. >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1138221 >> --- >> v2: added tests and comment, fixed a typo >> >> src/cpu/cpu_map.xml | 2 +- > > Hmm - I guess we don't have an .rng file that validates this, do we. Nope, the map is considered internal. Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3.5 05.5/18] blockjob: add new monitor json conversions
On 09/05/14 13:36, Eric Blake wrote: > The previous patch hoisted some bounds checks to the callers; > but someone that is not aware of the hoisted check could now > try passing an integer between LLONG_MAX and ULLONG_MAX. As a > safety measure, add new json conversion modes that let libvirt > error out early instead of pass bad numbers to qemu, if the > caller ever makes a mistake due to later refactoring. > > Convert the various blockjob QMP calls to use the new modes, > and switch some of them to be optional (QMP has always supported > an omitted "speed" the same as "speed":0, for everything except > block-job-set-speed). > > * src/qemu/qemu_monitor_json.c (qemuMonitorJSONMakeCommandRaw): > Add 'j'/'y' and 'J'/'Y' to error out on negative input. > (qemuMonitorJSONDriveMirror, qemuMonitorJSONBlockCommit) > (qemuMonitorJSONBlockJob): Use it. > > Signed-off-by: Eric Blake > --- > > This should address Peter's review concerns on 5/18; I could > push it either first or second (although I worded the commit > message to push it second). Either way I'm fine with it. I just wanted to make sure that we fail rather than pass garbage to qemu. > > src/qemu/qemu_monitor_json.c | 34 -- > 1 file changed, 28 insertions(+), 6 deletions(-) > Exactly what I had in mind. ACK. Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] NBD TLS support in QEMU
On Fri, Sep 05, 2014 at 12:54:45AM +0200, Benoît Canet wrote: > The Friday 05 Sep 2014 à 00:07:04 (+0200), Wouter Verhelst wrote : > > On Thu, Sep 04, 2014 at 04:19:17PM +0200, Benoît Canet wrote: > > > The Wednesday 03 Sep 2014 à 17:44:17 (+0100), Stefan Hajnoczi wrote : > > > > Hi, > > > > QEMU offers both NBD client and server functionality. The NBD protocol > > > > runs unencrypted, which is a problem when the client and server > > > > communicate over an untrusted network. > > > > > > > > The particular use case that prompted this mail is storage migration in > > > > OpenStack. The goal is to encrypt the NBD connection between source and > > > > destination hosts during storage migration. > > > > > > I agree this would be usefull. > > > > > > > > > > > I think we can integrate TLS into the NBD protocol as an optional flag. > > > > A quick web search does not reveal existing open source SSL/TLS NBD > > > > implementations. I do see a VMware NBDSSL protocol but there is no > > > > specification so I guess it is proprietary. > > > > > > > > The NBD protocol starts with a negotiation phase. This would be the > > > > appropriate place to indicate that TLS will be used. After client and > > > > server complete TLS setup the connection can continue as normal. > > > > > > Prenegociating TLS look like we will accidentaly introduce some security > > > hole. > > I was thinking of the fallback to cleartext case. If the server or client were given a nbds:// URL then don't allow downgrading to unencrypted. I think we can implement this without a security hole. Stefan pgp8Fx5mtuiIh.pgp Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2] qemu: numatune/domiftune no support in session mode
Tuning NUMA or network interface parameters require root
privileges to manage cgroups, thus an attempt to set some of these
parameters in session mode on a running domain should be invalid
followed by an error.
As an example might be memory tuning which raises an error in such case.
Following behavior in session mode will be present after applying
this patch:
tuning | SET | GET |
|---||
numatune| shut off only | always |
memtune | never | never |
domiftune | never | always |
Resolves https://bugzilla.redhat.com/show_bug.cgi?id=1126762
---
src/qemu/qemu_command.c | 14 +-
src/qemu/qemu_driver.c | 35 +--
2 files changed, 38 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 1ca98fb..5d4838e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7442,7 +7442,7 @@ qemuBuildCommandLine(virConnectPtr conn,
emulator = def->emulator;
if (!cfg->privileged) {
-/* If we have no cgroups than we can have no tunings that
+/* If we have no cgroups then we can have no tunings that
* require them */
if (def->mem.hard_limit || def->mem.soft_limit ||
@@ -7465,6 +7465,18 @@ qemuBuildCommandLine(virConnectPtr conn,
_("CPU tuning is not available in session mode"));
goto error;
}
+
+virDomainNetDefPtr *nets = def->nets;
+size_t nnets = def->nnets;
+for (i = 0; i < nnets; i++) {
+if (nets[i]->bandwidth) {
+if (nets[i]->bandwidth->in || nets[i]->bandwidth->out) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Network bandwidth tuning is not
available in session mode"));
+goto error;
+}
+}
+}
}
for (i = 0; i < def->ngraphics; ++i) {
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3c04678..ad02e68 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8786,6 +8786,13 @@ qemuDomainSetNumaParameters(virDomainPtr dom,
&persistentDef) < 0)
goto cleanup;
+if (!cfg->privileged &&
+flags & VIR_DOMAIN_AFFECT_LIVE) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("NUMA tuning is not available in session mode"));
+goto cleanup;
+}
+
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
if (!virCgroupHasController(priv->cgroup,
VIR_CGROUP_CONTROLLER_CPUSET)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -8870,6 +8877,7 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
size_t i;
virDomainObjPtr vm = NULL;
virDomainDefPtr persistentDef = NULL;
+virQEMUDriverConfigPtr cfg = NULL;
char *nodeset = NULL;
int ret = -1;
virCapsPtr caps = NULL;
@@ -,6 +8896,7 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
return -1;
priv = vm->privateData;
+cfg = virQEMUDriverGetConfig(driver);
if (virDomainGetNumaParametersEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
@@ -8905,14 +8914,6 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
goto cleanup;
}
-if (flags & VIR_DOMAIN_AFFECT_LIVE) {
-if (!virCgroupHasController(priv->cgroup,
VIR_CGROUP_CONTROLLER_MEMORY)) {
-virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cgroup memory controller is not mounted"));
-goto cleanup;
-}
-}
-
for (i = 0; i < QEMU_NB_NUMA_PARAM && i < *nparams; i++) {
virMemoryParameterPtr param = ¶ms[i];
@@ -8935,9 +8936,16 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
if (!nodeset)
goto cleanup;
} else {
-if (virCgroupGetCpusetMems(priv->cgroup, &nodeset) < 0)
-goto cleanup;
+if (!virCgroupHasController(priv->cgroup,
+VIR_CGROUP_CONTROLLER_MEMORY) ||
+virCgroupGetCpusetMems(priv->cgroup, &nodeset) < 0) {
+nodeset = virDomainNumatuneFormatNodeset(vm->def->numatune,
+ NULL, -1);
+if (!nodeset)
+goto cleanup;
+}
}
+
if (virTypedParameterAssign(param, VIR_DOMAIN_NUMA_NODESET,
VIR_TYPED_PARAM_STRING, nodeset) < 0)
goto cleanup;
@@ -8962,6 +8970,7 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
if (vm)
virObjectUnlock(vm);
virObjectUnref(caps);
+virObjectUnref(cfg);
return ret;
}
@@ -9889,6 +9898,12 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
if (virDomai
Re: [libvirt] [Qemu-devel] NBD TLS support in QEMU
On Fri, Sep 05, 2014 at 12:02:18AM +0200, Wouter Verhelst wrote: > [Cc: to nbd-general list added] > > On Wed, Sep 03, 2014 at 05:44:17PM +0100, Stefan Hajnoczi wrote: > > Besides QEMU, the userspace NBD tools (http://nbd.sf.net/) can also be > > extended to support TLS. In this case the kernel needs a localhost > > socket and userspace handles TLS. > > That introduces a possibility for a deadlock, since now your network > socket isn't on the PF_MEMALLOC-protected socket anymore, which will > cause the kernel to throw away packets which are needed for your nbd > connection, in hopes of clearing some memory. Understood but there are plenty of use cases where this doesn't matter. Stefan pgpRJaBmwqA2f.pgp Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] NBD TLS support in QEMU
On Fri, Sep 05, 2014 at 09:46:18AM +0100, Hani Benhabiles wrote: > On Wed, Sep 03, 2014 at 05:44:17PM +0100, Stefan Hajnoczi wrote: > Also, so mean of verification is required (otherwise, back to point 0 being > vulnerable to sslstrip style attacks) either that the server's cert is signed > with a certain (self-generated) CA certificate or that it matches a certain > fingerprint. Doing it similarly on the server-side would allow hitting a 2nd > bird (authentication.) Yes, client and server side certificates are needed. Here are the SPICE TLS options in QEMU: tls-port= Set the TCP port spice is listening on for encrypted channels. x509-dir= Set the x509 file directory. Expects same filenames as -vnc $display,x509=$dir x509-key-file= x509-key-password= x509-cert-file= x509-cacert-file= x509-dh-key-file= The x509 file names can also be configured individually. tls-ciphers= Specify which ciphers to use. I guess NBD would need similar options althoug I haven't investigated TLS in depth yet. Stefan pgpxavPP1ofza.pgp Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] Don't include non-migratable features in host-model
On Fri, Sep 05, 2014 at 12:48:00 +0200, Jano Tomko wrote: > Commit fba6bc4 introduced support for the 'invtsc' feature, > which blocks migration. We should not include it in the > host-model CPU by default, because it's intended to be used > with migration. > > https://bugzilla.redhat.com/show_bug.cgi?id=1138221 > --- > v2: added tests and comment, fixed a typo Thanks, ACK. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 14/18] blockcopy: tweak how rebase calls into copy
On 08/31/14 06:02, Eric Blake wrote:
> In order to implement the new virDomainBlockCopy, the existing
> block copy internal implementation needs to be adjusted. The
> new function will parse XML into a storage source, and parse
> typed parameters into integers, then call into the same common
> backend. For now, it's easier to keep the same implementation
> limits that only local file destinations are suported, but now
> the check needs to be explicit.
>
> * src/qemu/qemu_driver.c (qemuDomainBlockCopy): Rename...
> (qemuDomainBlockCopyCommon): ...and adjust parameters.
> (qemuDomainBlockRebase): Adjust caller.
>
> Signed-off-by: Eric Blake
> ---
> src/qemu/qemu_driver.c | 108
> ++---
> 1 file changed, 58 insertions(+), 50 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 946c384..d3f1042 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -15351,11 +15351,12 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, const
> char *path,
>
> /* bandwidth in bytes/s */
> static int
> -qemuDomainBlockCopy(virDomainObjPtr vm,
> -virConnectPtr conn,
> -const char *path,
> -const char *dest, const char *format,
> -unsigned long long bandwidth, unsigned int flags)
You should mention that this function consumes @dest.
> +qemuDomainBlockCopyCommon(virDomainObjPtr vm,
> + virConnectPtr conn,
> + const char *path,
> + virStorageSourcePtr dest,
> + unsigned long long bandwidth,
> + unsigned int flags)
> {
> virQEMUDriverPtr driver = conn->privateData;
> qemuDomainObjPrivatePtr priv;
> @@ -15367,11 +15368,12 @@ qemuDomainBlockCopy(virDomainObjPtr vm,
> bool need_unlink = false;
> virStorageSourcePtr mirror = NULL;
> virQEMUDriverConfigPtr cfg = NULL;
> +const char *format = NULL;
> +int desttype = virStorageSourceGetActualType(dest);
>
> /* Preliminaries: find the disk we are editing, sanity checks */
> -virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW |
> - VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT |
> - VIR_DOMAIN_BLOCK_REBASE_COPY_DEV, -1);
> +virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW |
> + VIR_DOMAIN_BLOCK_COPY_REUSE_EXT, -1);
>
> priv = vm->privateData;
> cfg = virQEMUDriverGetConfig(driver);
> @@ -15416,8 +15418,8 @@ qemuDomainBlockCopy(virDomainObjPtr vm,
> if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0)
> goto endjob;
>
> -if ((flags & VIR_DOMAIN_BLOCK_REBASE_SHALLOW) &&
> -STREQ_NULLABLE(format, "raw") &&
> +if ((flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW) &&
> +dest->format == VIR_STORAGE_FILE_RAW &&
> disk->src->backingStore->path) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("disk '%s' has backing file, so raw shallow copy "
> @@ -15427,72 +15429,68 @@ qemuDomainBlockCopy(virDomainObjPtr vm,
> }
>
> /* Prepare the destination file. */
> -if (stat(dest, &st) < 0) {
> +/* XXX Allow non-file mirror destinations */
> +if (!virStorageSourceIsLocalStorage(dest)) {
> +virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> + _("non-file destination not supported yet"));
> +}
> +if (stat(dest->path, &st) < 0) {
> if (errno != ENOENT) {
> virReportSystemError(errno, _("unable to stat for disk %s: %s"),
> - disk->dst, dest);
> + disk->dst, dest->path);
> goto endjob;
> -} else if (flags & (VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT |
> -VIR_DOMAIN_BLOCK_REBASE_COPY_DEV)) {
> +} else if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT ||
> + desttype == VIR_STORAGE_TYPE_BLOCK) {
> virReportSystemError(errno,
> _("missing destination file for disk %s:
> %s"),
> - disk->dst, dest);
> + disk->dst, dest->path);
> goto endjob;
> }
> } else if (!S_ISBLK(st.st_mode)) {
> -if (st.st_size && !(flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) {
> +if (st.st_size && !(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("external destination file for disk %s already "
> "exists and is not a block device: %s"),
> - disk->dst, dest);
> + disk->dst, dest->path);
> goto endjob;
> }
> -if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_DEV) {
> +if (desttype == VIR_STORAGE_TYPE_BLOCK) {
> v
Re: [libvirt] [PATCH v3 15/18] blockcopy: add a way to parse disk source
On 08/31/14 06:02, Eric Blake wrote:
> The new blockcopy API wants to reuse only a subset of the disk
> hotplug parser - namely, we only care about the embedded
> virStorageSourcePtr inside a XML. Strange as it may
> seem, it was easier to just parse an entire disk definition,
> then throw away everything but the embedded source, than it
> was to disentangle the source parsing code from the rest of
> the overall disk parsing function. All that I needed was a
> couple of tweaks and a new internal flag that determines
> whether the normally-mandatory target element can be
> gracefully skipped, since everything else was already optional.
>
> * src/conf/domain_conf.h (virDomainDiskSourceParse): New
> prototype.
> * src/conf/domain_conf.c (VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE):
> New flag.
> (virDomainDiskDefParseXML): Honor flag to make target optional.
> (virDomainDiskSourceParse): New function.
>
> Signed-off-by: Eric Blake
> ---
> src/conf/domain_conf.c | 107
> ++-
> src/conf/domain_conf.h | 4 ++
> src/libvirt_private.syms | 1 +
> 3 files changed, 82 insertions(+), 30 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 53ef694..8723008 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
\
> @@ -5951,7 +5955,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
> } else {
> if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
> def->bus = VIR_DOMAIN_DISK_BUS_FDC;
> -} else {
> +} else if (!(flags & VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE)) {
> if (STRPREFIX(target, "hd"))
> def->bus = VIR_DOMAIN_DISK_BUS_IDE;
> else if (STRPREFIX(target, "sd"))
> @@ -6186,12 +6190,14 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
> }
> }
>
> -if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE
> -&& virDomainDiskDefAssignAddress(xmlopt, def) < 0)
> -goto error;
> +if (!(flags & VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE)) {
> +if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE
> +&& virDomainDiskDefAssignAddress(xmlopt, def) < 0)
> +goto error;
>
> -if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0)
> -goto error;
> +if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0)
> +goto error;
This actually might be useful for source of block copy and others some
time in the future.
> +}
>
> cleanup:
> VIR_FREE(bus);
ACK, thanks for dropping the refactor part.
Peter
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 16/18] blockcopy: add qemu implementation of new API
On 08/31/14 06:02, Eric Blake wrote:
> The hard part of managing the disk copy is already coded; all
> this had to do was convert the XML and virTypedParameters into
> the internal representation.
>
> With this patch, all blockcopy operations that used the old
> API should also work via the new API. Additional extensions,
> such as supporting the granularity tunable or a network rather
> than file destination, will be added as later patches.
>
> * src/qemu/qemu_driver.c (qemuDomainBlockCopy): New function.
>
> Signed-off-by: Eric Blake
> ---
> src/qemu/qemu_driver.c | 85
> ++
> 1 file changed, 85 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d3f1042..d93e184 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -15611,6 +15611,90 @@ qemuDomainBlockRebase(virDomainPtr dom, const char
> *path, const char *base,
> return ret;
> }
>
> +
> +static int
> +qemuDomainBlockCopy(virDomainPtr dom, const char *disk, const char *destxml,
> +virTypedParameterPtr params, int nparams,
> +unsigned int flags)
> +{
> +virQEMUDriverPtr driver = dom->conn->privateData;
> +virDomainObjPtr vm;
> +int ret = -1;
> +unsigned long long bandwidth = 0;
> +unsigned int granularity = 0;
> +unsigned long long buf_size = 0;
> +virStorageSourcePtr dest = NULL;
> +size_t i;
> +
> +virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW |
> + VIR_DOMAIN_BLOCK_COPY_REUSE_EXT, -1);
> +if (virTypedParamsValidate(params, nparams,
> + VIR_DOMAIN_BLOCK_COPY_BANDWIDTH,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_COPY_GRANULARITY,
> + VIR_TYPED_PARAM_UINT,
> + VIR_DOMAIN_BLOCK_COPY_BUF_SIZE,
> + VIR_TYPED_PARAM_ULLONG,
> + NULL) < 0)
> +return -1;
> +
> +if (!(vm = qemuDomObjFromDomain(dom)))
> +return -1;
> +
> +if (virDomainBlockCopyEnsureACL(dom->conn, vm->def) < 0)
> +goto cleanup;
> +
> +for (i = 0; i < nparams; i++) {
> +virTypedParameterPtr param = ¶ms[i];
> +
> +/* Typed params (wisely) refused to expose unsigned long, but
> + * back-compat demands that we stick with a maximum of
> + * unsigned long bandwidth in MiB/s, while our value is
> + * unsigned long long in bytes/s. Hence, we have to do
> + * overflow detection if this is a 32-bit server handling a
> + * 64-bit client. */
> +if (STREQ(param->field, VIR_DOMAIN_BLOCK_COPY_BANDWIDTH)) {
> +if (sizeof(unsigned long) < sizeof(bandwidth) &&
> +param->value.ul > ULONG_MAX * (1ULL << 20)) {
> +virReportError(VIR_ERR_OVERFLOW,
> + _("bandwidth must be less than %llu bytes"),
> + ULONG_MAX * (1ULL << 20));
> +goto cleanup;
Sigh. I'd rather not see such checks, but unfortunately we don't want to
break output of the blockJobInfo ...
> +}
> +bandwidth = param->value.ul;
> +} else if (STREQ(param->field, VIR_DOMAIN_BLOCK_COPY_GRANULARITY)) {
> +granularity = param->value.ui;
> +} else if (STREQ(param->field, VIR_DOMAIN_BLOCK_COPY_BUF_SIZE)) {
> +buf_size = param->value.ul;
> +}
> +}
> +if (granularity) {
> +virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> + _("granularity tuning not supported yet"));
> +goto cleanup;
> +}
> +if (buf_size) {
> +virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> + _("buffer size tuning not supported yet"));
> +goto cleanup;
> +}
> +
> +if (!(dest = virDomainDiskDefSourceParse(destxml, vm->def,
> driver->xmlopt,
> + VIR_DOMAIN_XML_INACTIVE)))
> +goto cleanup;
> +
> +ret = qemuDomainBlockCopyCommon(vm, dom->conn, disk, dest,
> +bandwidth, flags);
> +vm = NULL;
> +
> + cleanup:
> +virStorageSourceFree(dest);
> +if (vm)
> +virObjectUnlock(vm);
> +return ret;
> +}
> +
> +
> static int
> qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long
> bandwidth,
> unsigned int flags)
> @@ -17683,6 +17767,7 @@ static virDriver qemuDriver = {
> .domainBlockJobSetSpeed = qemuDomainBlockJobSetSpeed, /* 0.9.4 */
> .domainBlockPull = qemuDomainBlockPull, /* 0.9.4 */
> .domainBlockRebase = qemuDomainBlockRebase, /* 0.9.10 */
> +.domainBlockCopy = qemuDomainBlockCopy, /* 1.2.8 */
> .domainBlockCommit = qemuDomainBlockCommit, /* 1.0.0 */
> .connectIsAlive = qemuConnectIsAlive, /* 0.9.8 */
>
Re: [libvirt] [PATCH v3 17/18] blockcopy: add qemu implementation of new tunables
On 08/31/14 06:02, Eric Blake wrote: > Upstream qemu 1.4 added some drive-mirror tunables not present > when it was first introduced in 1.3. Management apps may want > to set these in some cases (for example, without tuning > granularity down to sector size, a copy may end up occupying > more bytes than the original because an entire cluster is > copied even when only a sector within the cluster is dirty, > although tuning it down results in more CPU time to do the > copy). I haven't personally needed to use the parameters, but > since they exist, and since the new API supports virTypedParams, > we might as well expose them. > > Since the tuning parameters aren't often used, and omitted from > the QMP command when unspecified, I think it is safe to rely on > qemu 1.3 to issue an error about them being unsupported, rather > than trying to create a new capability bit in libvirt. > > Meanwhile, all versions of qemu from 1.4 to 2.1 have a bug where > a bad granularity (such as non-power-of-2) gives a poor message: > error: internal error: unable to execute QEMU command 'drive-mirror': Invalid > parameter 'drive-virtio-disk0' > > because of abuse of QERR_INVALID_PARAMETER (which is supposed to > name the parameter that was given a bad value, rather than the > value passed to some other parameter). I don't see that a > capability check will help, so we'll just live with it (and it > has since been improved in upstream qemu). > > * src/qemu/qemu_monitor.h (qemuMonitorDriveMirror): Add > parameters. > * src/qemu/qemu_monitor.c (qemuMonitorDriveMirror): Likewise. > * src/qemu/qemu_monitor_json.h (qemuMonitorJSONDriveMirror): > Likewise. > * src/qemu/qemu_monitor_json.c (qemuMonitorJSONDriveMirror): > Likewise. > * src/qemu/qemu_driver.c (qemuDomainBlockCopyCommon): Likewise. > (qemuDomainBlockRebase, qemuDomainBlockCopy): Adjust callers. > * src/qemu/qemu_migration.c (qemuMigrationDriveMirror): Likewise. > * tests/qemumonitorjsontest.c (qemuMonitorJSONDriveMirror): Likewise. > > Signed-off-by: Eric Blake > --- > src/qemu/qemu_driver.c | 18 +- > src/qemu/qemu_migration.c| 2 +- > src/qemu/qemu_monitor.c | 8 +--- > src/qemu/qemu_monitor.h | 2 ++ > src/qemu/qemu_monitor_json.c | 6 +- > src/qemu/qemu_monitor_json.h | 2 ++ > tests/qemumonitorjsontest.c | 2 +- > 7 files changed, 21 insertions(+), 19 deletions(-) > > ACK, Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 18/18] blockjob: allow finer bandwidth tuning for set speed
On 08/31/14 06:02, Eric Blake wrote: > We stupidly modeled block job bandwidth after migration > bandwidth, which in turn was an 'unsigned long' and therefore > subject to 32-bit vs. 64-bit interpretations. To work around > the fact that 10-gigabit interfaces are possible but don't fit > within 32 bits, the original interface took the number scaled > as MiB/sec. But this scaling is rather coarse, and it might > be nice to tune bandwidth finer than in megabyte chunks. > > Several of the block job calls that can set speed are fed > through a common interface, so it was easier to adjust them all > at once. Note that there is intentionally no flag for the new > virDomainBlockCopy; there, since the API already uses a 64-bit > type always, instead of a possible 32-bit type, and is brand > new, it was easier to just avoid scaling issues. As with the > previous patch that adjusted the query side, omitting the new > flag preserves old behavior, and the documentation now mentions > limits of what happens when a 32-bit machine is on either client > or server side. > > * include/libvirt/libvirt.h.in (virDomainBlockJobSetSpeedFlags) > (virDomainBlockPullFlags) > (VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES) > (VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES): New enums. > * src/libvirt.c (virDomainBlockJobSetSpeed, virDomainBlockPull) > (virDomainBlockRebase, virDomainBlockCommit): Document them. > * src/qemu/qemu_driver.c (qemuDomainBlockJobSetSpeed) > (qemuDomainBlockPull, qemuDomainBlockRebase) > (qemuDomainBlockCommit, qemuDomainBlockJobImpl): Support new flag. > > Signed-off-by: Eric Blake > --- > include/libvirt/libvirt.h.in | 31 + > src/libvirt.c| 83 > +++- > src/qemu/qemu_driver.c | 61 +++- > 3 files changed, 118 insertions(+), 57 deletions(-) > ACK, the better granularity may help today, while we are able to keep the huge range for future expansion. Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] IO accounting overhaul
Am 01.09.2014 um 13:41 hat Markus Armbruster geschrieben: > Benoît Canet writes: > > > The Monday 01 Sep 2014 à 11:52:00 (+0200), Markus Armbruster wrote : > >> Cc'ing libvirt following Stefan's lead. > >> > >> Benoît Canet writes: > >> > /* the following would compute latecies for slices of 1 seconds then > >> > toss the > >> > * result and start a new slice. A weighted sumation of the instant > >> > latencies > >> > * could help to implement this. > >> > */ > >> > 1s_read_average_latency > >> > 1s_write_average_latency > >> > 1s_flush_average_latency > >> > > >> > /* the former three numbers could be used to further compute a 1 > >> > minute slice value */ > >> > 1m_read_average_latency > >> > 1m_write_average_latency > >> > 1m_flush_average_latency > >> > > >> > /* the former three numbers could be used to further compute a 1 hours > >> > slice value */ > >> > 1h_read_average_latency > >> > 1h_write_average_latency > >> > 1h_flush_average_latency > >> > >> This is something like "what we added to total_FOO_time in the last > >> completed 1s / 1m / 1h time slice divided by the number of additions". > >> Just another way to accumulate the same raw data, thus no worries. > >> > >> > /* 1 second average number of requests in flight */ > >> > 1s_read_queue_depth > >> > 1s_write_queue_depth > >> > > >> > /* 1 minute average number of requests in flight */ > >> > 1m_read_queue_depth > >> > 1m_write_queue_depth > >> > > >> > /* 1 hours average number of requests in flight */ > >> > 1h_read_queue_depth > >> > 1h_write_queue_depth I don't think I agree with putting fixed time periods like 1 s/min/h into qemu. What you need there is policy and we should probably make it configurable. Do we need accounting for multiple time periods at the same time or would it be enough to have one and make its duration an option? > > Optionally collecting the same data for each BDS of the graph. > > If that's the case, keeping the shared infrastructure in the block layer > makes sense. > > BDS member acct then holds I/O stats for the BDS. We currently use it > for something else: I/O stats of the device model backed by this BDS. > That needs to move elsewhere. Two places come to mind: > > 1. BlockBackend, when it's available (I resumed working on it last week >for a bit). Superficially attractive, because it's close to what we >have now, but then we have to deal with what to do when the backend >gets disconnected from its device model, then connected to another >one. > > 2. The device models that actually implement I/O accounting. Since >query-blockstats names a backend rather than a device model, we need >a BlockDevOps callback to fetch the stats. Fetch fails when the >callback is null. Lets us distinguish "no stats yet" and "device >model can't do stats", thus permits a QMP interface that doesn't lie. > > Right now, I like (2) better. So let's say I have some block device, which is attached to a guest device for a while, but then I detach it and continue using it in a different place (maybe another guest device or a block job). Should we really reset all counters in query-blockstats to 0? I think as I user I would be surprised about this, because I still refer to it by the same name (the device_name, which will be in the BB), so it's the same thing for me and the total requests include everything that was ever issued against it. > > -API wize I think about adding > > bdrv_acct_invalid() and > > bdrv_acct_failed() and systematically issuing a bdrv_acct_start() asap. > > Complication: partial success. Example: > > 1. Guest requests a read of N sectors. > > 2. Device model calls >bdrv_acct_start(s->bs, &req->acct, N * BDRV_SECTOR_SIZE, BDRV_ACCT_READ) > > 3. Device model examines the request, and deems it valid. > > 4. Device model passes it to the block layer. > > 5. Block layer does its thing, but for some reason only M < N sectors >can be read. Block layer returns M. No, it returns -errno. > 6. What's the device model to do now? Both bdrv_acct_failed() and >bdrv_acct_done() would be wrong. > >Should the device model account for a read of size M? This ignores >the partial failure. > >Should it split the read into a successful and a failed part for >accounting purposes? This miscounts the number of reads. I think we should simply account it as a failed request because this is what it will look like for the guest. If you want the partial data that was internally issued, you need to look at different statistics (probably those of bs->file). Kevin -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] IO accounting overhaul
The Friday 05 Sep 2014 à 16:30:31 (+0200), Kevin Wolf wrote : > Am 01.09.2014 um 13:41 hat Markus Armbruster geschrieben: > > Benoît Canet writes: > > > > > The Monday 01 Sep 2014 à 11:52:00 (+0200), Markus Armbruster wrote : > > >> Cc'ing libvirt following Stefan's lead. > > >> > > >> Benoît Canet writes: > > >> > /* the following would compute latecies for slices of 1 seconds then > > >> > toss the > > >> > * result and start a new slice. A weighted sumation of the instant > > >> > latencies > > >> > * could help to implement this. > > >> > */ > > >> > 1s_read_average_latency > > >> > 1s_write_average_latency > > >> > 1s_flush_average_latency > > >> > > > >> > /* the former three numbers could be used to further compute a 1 > > >> > minute slice value */ > > >> > 1m_read_average_latency > > >> > 1m_write_average_latency > > >> > 1m_flush_average_latency > > >> > > > >> > /* the former three numbers could be used to further compute a 1 hours > > >> > slice value */ > > >> > 1h_read_average_latency > > >> > 1h_write_average_latency > > >> > 1h_flush_average_latency > > >> > > >> This is something like "what we added to total_FOO_time in the last > > >> completed 1s / 1m / 1h time slice divided by the number of additions". > > >> Just another way to accumulate the same raw data, thus no worries. > > >> > > >> > /* 1 second average number of requests in flight */ > > >> > 1s_read_queue_depth > > >> > 1s_write_queue_depth > > >> > > > >> > /* 1 minute average number of requests in flight */ > > >> > 1m_read_queue_depth > > >> > 1m_write_queue_depth > > >> > > > >> > /* 1 hours average number of requests in flight */ > > >> > 1h_read_queue_depth > > >> > 1h_write_queue_depth > > I don't think I agree with putting fixed time periods like 1 s/min/h > into qemu. What you need there is policy and we should probably make > it configurable. > > Do we need accounting for multiple time periods at the same time or > would it be enough to have one and make its duration an option? > > > > Optionally collecting the same data for each BDS of the graph. > > > > If that's the case, keeping the shared infrastructure in the block layer > > makes sense. > > > > BDS member acct then holds I/O stats for the BDS. We currently use it > > for something else: I/O stats of the device model backed by this BDS. > > That needs to move elsewhere. Two places come to mind: > > > > 1. BlockBackend, when it's available (I resumed working on it last week > >for a bit). Superficially attractive, because it's close to what we > >have now, but then we have to deal with what to do when the backend > >gets disconnected from its device model, then connected to another > >one. > > > > 2. The device models that actually implement I/O accounting. Since > >query-blockstats names a backend rather than a device model, we need > >a BlockDevOps callback to fetch the stats. Fetch fails when the > >callback is null. Lets us distinguish "no stats yet" and "device > >model can't do stats", thus permits a QMP interface that doesn't lie. > > > > Right now, I like (2) better. > > So let's say I have some block device, which is attached to a guest > device for a while, but then I detach it and continue using it in a > different place (maybe another guest device or a block job). Should we > really reset all counters in query-blockstats to 0? > > I think as I user I would be surprised about this, because I still refer > to it by the same name (the device_name, which will be in the BB), so > it's the same thing for me and the total requests include everything > that was ever issued against it. It all depends on where you are in the user food chain. If you are a cloud end user you are interested in the stats associated with the hardware of your /dev/vdx because that's what iostat allow you to see. > > > > -API wize I think about adding > > > bdrv_acct_invalid() and > > > bdrv_acct_failed() and systematically issuing a bdrv_acct_start() asap. > > > > Complication: partial success. Example: > > > > 1. Guest requests a read of N sectors. > > > > 2. Device model calls > >bdrv_acct_start(s->bs, &req->acct, N * BDRV_SECTOR_SIZE, BDRV_ACCT_READ) > > > > 3. Device model examines the request, and deems it valid. > > > > 4. Device model passes it to the block layer. > > > > 5. Block layer does its thing, but for some reason only M < N sectors > >can be read. Block layer returns M. > > No, it returns -errno. > > > 6. What's the device model to do now? Both bdrv_acct_failed() and > >bdrv_acct_done() would be wrong. > > > >Should the device model account for a read of size M? This ignores > >the partial failure. > > > >Should it split the read into a successful and a failed part for > >accounting purposes? This miscounts the number of reads. > > I think we should simply account it as a failed request because this is > what it will look like for the guest. If you want the partia
Re: [libvirt] [Qemu-devel] IO accounting overhaul
The Friday 05 Sep 2014 à 16:30:31 (+0200), Kevin Wolf wrote : > Am 01.09.2014 um 13:41 hat Markus Armbruster geschrieben: > > Benoît Canet writes: > > > > > The Monday 01 Sep 2014 à 11:52:00 (+0200), Markus Armbruster wrote : > > >> Cc'ing libvirt following Stefan's lead. > > >> > > >> Benoît Canet writes: > > >> > /* the following would compute latecies for slices of 1 seconds then > > >> > toss the > > >> > * result and start a new slice. A weighted sumation of the instant > > >> > latencies > > >> > * could help to implement this. > > >> > */ > > >> > 1s_read_average_latency > > >> > 1s_write_average_latency > > >> > 1s_flush_average_latency > > >> > > > >> > /* the former three numbers could be used to further compute a 1 > > >> > minute slice value */ > > >> > 1m_read_average_latency > > >> > 1m_write_average_latency > > >> > 1m_flush_average_latency > > >> > > > >> > /* the former three numbers could be used to further compute a 1 hours > > >> > slice value */ > > >> > 1h_read_average_latency > > >> > 1h_write_average_latency > > >> > 1h_flush_average_latency > > >> > > >> This is something like "what we added to total_FOO_time in the last > > >> completed 1s / 1m / 1h time slice divided by the number of additions". > > >> Just another way to accumulate the same raw data, thus no worries. > > >> > > >> > /* 1 second average number of requests in flight */ > > >> > 1s_read_queue_depth > > >> > 1s_write_queue_depth > > >> > > > >> > /* 1 minute average number of requests in flight */ > > >> > 1m_read_queue_depth > > >> > 1m_write_queue_depth > > >> > > > >> > /* 1 hours average number of requests in flight */ > > >> > 1h_read_queue_depth > > >> > 1h_write_queue_depth > > I don't think I agree with putting fixed time periods like 1 s/min/h > into qemu. What you need there is policy and we should probably make > it configurable. I agree. > > Do we need accounting for multiple time periods at the same time or > would it be enough to have one and make its duration an option? I don't know yet. > > > > Optionally collecting the same data for each BDS of the graph. > > > > If that's the case, keeping the shared infrastructure in the block layer > > makes sense. > > > > BDS member acct then holds I/O stats for the BDS. We currently use it > > for something else: I/O stats of the device model backed by this BDS. > > That needs to move elsewhere. Two places come to mind: > > > > 1. BlockBackend, when it's available (I resumed working on it last week > >for a bit). Superficially attractive, because it's close to what we > >have now, but then we have to deal with what to do when the backend > >gets disconnected from its device model, then connected to another > >one. > > > > 2. The device models that actually implement I/O accounting. Since > >query-blockstats names a backend rather than a device model, we need > >a BlockDevOps callback to fetch the stats. Fetch fails when the > >callback is null. Lets us distinguish "no stats yet" and "device > >model can't do stats", thus permits a QMP interface that doesn't lie. > > > > Right now, I like (2) better. > > So let's say I have some block device, which is attached to a guest > device for a while, but then I detach it and continue using it in a > different place (maybe another guest device or a block job). Should we > really reset all counters in query-blockstats to 0? > > I think as I user I would be surprised about this, because I still refer > to it by the same name (the device_name, which will be in the BB), so > it's the same thing for me and the total requests include everything > that was ever issued against it. > > > > -API wize I think about adding > > > bdrv_acct_invalid() and > > > bdrv_acct_failed() and systematically issuing a bdrv_acct_start() asap. > > > > Complication: partial success. Example: > > > > 1. Guest requests a read of N sectors. > > > > 2. Device model calls > >bdrv_acct_start(s->bs, &req->acct, N * BDRV_SECTOR_SIZE, BDRV_ACCT_READ) > > > > 3. Device model examines the request, and deems it valid. > > > > 4. Device model passes it to the block layer. > > > > 5. Block layer does its thing, but for some reason only M < N sectors > >can be read. Block layer returns M. > > No, it returns -errno. > > > 6. What's the device model to do now? Both bdrv_acct_failed() and > >bdrv_acct_done() would be wrong. > > > >Should the device model account for a read of size M? This ignores > >the partial failure. > > > >Should it split the read into a successful and a failed part for > >accounting purposes? This miscounts the number of reads. > > I think we should simply account it as a failed request because this is > what it will look like for the guest. If you want the partial data that > was internally issued, you need to look at different statistics > (probably those of bs->file). > > Kevin > > -- > libvir-list mailing list > libvir-list@redhat.c
Re: [libvirt] [Qemu-devel] NBD TLS support in QEMU
On Fri, Sep 05, 2014 at 12:54:45AM +0200, Benoît Canet wrote:
> The Friday 05 Sep 2014 à 00:07:04 (+0200), Wouter Verhelst wrote :
> > On Thu, Sep 04, 2014 at 04:19:17PM +0200, Benoît Canet wrote:
> > > Prenegociating TLS look like we will accidentaly introduce some security
> > > hole.
>
> I was thinking of the fallback to cleartext case.
>
> As a regular developper I am afraid of doing something creative with
> cryptography.
STARTTLS-like schemes is not being "creative with cryptography", it's an
accepted way of doing things. Yes, there are pitfalls, but those always
exist; that doesn't mean you should fall into the trap of remaking the
errors HTTP made with HTTPS. It's taken years for HTTPS to be able to
deal with the fact that you couldn't have multiple HTTPS sites on the
same IP; I don't want to go there.
"fallback to cleartext" is a problem, but it should not be too hard to
have crypto be enabled by way of a tri-state variable ("disabled",
"available if client wants it", "required").
--
Home is where you have to wash the dishes.
-- #debian-devel, Freenode, 2004-09-22
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] NBD TLS support in QEMU
On Fri, Sep 05, 2014 at 09:46:18AM +0100, Hani Benhabiles wrote: > On Wed, Sep 03, 2014 at 05:44:17PM +0100, Stefan Hajnoczi wrote: > > Hi, > > QEMU offers both NBD client and server functionality. The NBD protocol > > runs unencrypted, which is a problem when the client and server > > communicate over an untrusted network. > > > > The particular use case that prompted this mail is storage migration in > > OpenStack. The goal is to encrypt the NBD connection between source and > > destination hosts during storage migration. > > > > I think we can integrate TLS into the NBD protocol as an optional flag. > > A quick web search does not reveal existing open source SSL/TLS NBD > > implementations. I do see a VMware NBDSSL protocol but there is no > > specification so I guess it is proprietary. > > Not sold on this because: > - It requires (unnecessary) changes to the NBD specification. They may not be necessary from a libvirt/qemu point of view, but I've had requests to implement encryption from other people, too. So while they're not very high on my priority list, I do think the changes are necessary. > - It would still be vulnerable to a protocol downgrade attack: ie. An attacker > could strip the TLS flag from the server's response resulting in either: > * Connection fallback to cleartext, if both ends don't force TLS. > * Loss of backward compatibility, if one of the ends forces TLS, making the > reason for which such a flag is added moot IIUC. Tunneling the entire protocol inside an SSL connection doesn't fix that; if an attacker is able to hijack your TCP connections and change flags, then this attacker is also able to hijack your TCP connection and redirect it to a decrypting/encrypting proxy. I agree that preventing a possible SSL downgrade attack (and other forms of MITM) should be high on the priority list, but "tunnel the whole thing in SSL" doesn't do that. [...] -- Home is where you have to wash the dishes. -- #debian-devel, Freenode, 2004-09-22 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] NBD TLS support in QEMU
On Wed, Sep 03, 2014 at 05:44:17PM +0100, Stefan Hajnoczi wrote: > Hi, > QEMU offers both NBD client and server functionality. The NBD protocol > runs unencrypted, which is a problem when the client and server > communicate over an untrusted network. > > The particular use case that prompted this mail is storage migration in > OpenStack. The goal is to encrypt the NBD connection between source and > destination hosts during storage migration. > > I think we can integrate TLS into the NBD protocol as an optional flag. > A quick web search does not reveal existing open source SSL/TLS NBD > implementations. I do see a VMware NBDSSL protocol but there is no > specification so I guess it is proprietary. Not sold on this because: - It requires (unnecessary) changes to the NBD specification. - It would still be vulnerable to a protocol downgrade attack: ie. An attacker could strip the TLS flag from the server's response resulting in either: * Connection fallback to cleartext, if both ends don't force TLS. * Loss of backward compatibility, if one of the ends forces TLS, making the reason for which such a flag is added moot IIUC. IMO, it is more fool proof add some --use-tls flag to the client and server implementations to wrap the data in TLS, just like HTTPS does for instance. Also, so mean of verification is required (otherwise, back to point 0 being vulnerable to sslstrip style attacks) either that the server's cert is signed with a certain (self-generated) CA certificate or that it matches a certain fingerprint. Doing it similarly on the server-side would allow hitting a 2nd bird (authentication.) > > The NBD protocol starts with a negotiation phase. This would be the > appropriate place to indicate that TLS will be used. After client and > server complete TLS setup the connection can continue as normal. > > Besides QEMU, the userspace NBD tools (http://nbd.sf.net/) can also be > extended to support TLS. In this case the kernel needs a localhost > socket and userspace handles TLS. > > Thoughts? > > Stefan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] NBD TLS support in QEMU
On Fri, Sep 05, 2014 at 09:13:26AM +0100, Daniel P. Berrange wrote: > On Fri, Sep 05, 2014 at 12:02:18AM +0200, Wouter Verhelst wrote: > > [Cc: to nbd-general list added] > > > > On Wed, Sep 03, 2014 at 05:44:17PM +0100, Stefan Hajnoczi wrote: > > > Hi, > > > QEMU offers both NBD client and server functionality. The NBD protocol > > > runs unencrypted, which is a problem when the client and server > > > communicate over an untrusted network. > > > > > > The particular use case that prompted this mail is storage migration in > > > OpenStack. The goal is to encrypt the NBD connection between source and > > > destination hosts during storage migration. > > > > I've never given encrypted NBD high priority, since I don't think > > encryption without authentication serves much purpose -- and I haven't > > gotten around to adding authentication yet (for which I have plans; but > > other things have priority). > > While have an authentication layer like SASL wired into the NBD protocol > would be nice, it shouldn't be considered a blocker / pre-requisite. I'm not saying that. If someone were to come up with patches for TLS support in NBD, I'm not going to say no (provided they're sensible etc, yada yada). It's just that when I look at things to do for NBD and prioritize, I intend work on authentication (with SASL, indeed) before I work on TLS. -- Home is where you have to wash the dishes. -- #debian-devel, Freenode, 2004-09-22 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] IO accounting overhaul
The Friday 05 Sep 2014 à 16:30:31 (+0200), Kevin Wolf wrote : > Am 01.09.2014 um 13:41 hat Markus Armbruster geschrieben: > > Benoît Canet writes: > > > > > The Monday 01 Sep 2014 à 11:52:00 (+0200), Markus Armbruster wrote : > > >> Cc'ing libvirt following Stefan's lead. > > >> > > >> Benoît Canet writes: > > >> > /* the following would compute latecies for slices of 1 seconds then > > >> > toss the > > >> > * result and start a new slice. A weighted sumation of the instant > > >> > latencies > > >> > * could help to implement this. > > >> > */ > > >> > 1s_read_average_latency > > >> > 1s_write_average_latency > > >> > 1s_flush_average_latency > > >> > > > >> > /* the former three numbers could be used to further compute a 1 > > >> > minute slice value */ > > >> > 1m_read_average_latency > > >> > 1m_write_average_latency > > >> > 1m_flush_average_latency > > >> > > > >> > /* the former three numbers could be used to further compute a 1 hours > > >> > slice value */ > > >> > 1h_read_average_latency > > >> > 1h_write_average_latency > > >> > 1h_flush_average_latency > > >> > > >> This is something like "what we added to total_FOO_time in the last > > >> completed 1s / 1m / 1h time slice divided by the number of additions". > > >> Just another way to accumulate the same raw data, thus no worries. > > >> > > >> > /* 1 second average number of requests in flight */ > > >> > 1s_read_queue_depth > > >> > 1s_write_queue_depth > > >> > > > >> > /* 1 minute average number of requests in flight */ > > >> > 1m_read_queue_depth > > >> > 1m_write_queue_depth > > >> > > > >> > /* 1 hours average number of requests in flight */ > > >> > 1h_read_queue_depth > > >> > 1h_write_queue_depth > I asked some input from a cloud provider. > I don't think I agree with putting fixed time periods like 1 s/min/h > into qemu. What you need there is policy and we should probably make > it configurable. yes, baking policy in qemu is bad. > > Do we need accounting for multiple time periods at the same time or > would it be enough to have one and make its duration an option? Having multiple time periods (up to 3) at once would be cool. Having big knobs to turn in the configuration (one per period) would be preferable than a configuration nightmare of one or more setting per device. > > > > Optionally collecting the same data for each BDS of the graph. > > > > If that's the case, keeping the shared infrastructure in the block layer > > makes sense. > > > > BDS member acct then holds I/O stats for the BDS. We currently use it > > for something else: I/O stats of the device model backed by this BDS. > > That needs to move elsewhere. Two places come to mind: > > > > 1. BlockBackend, when it's available (I resumed working on it last week > >for a bit). Superficially attractive, because it's close to what we > >have now, but then we have to deal with what to do when the backend > >gets disconnected from its device model, then connected to another > >one. > > > > 2. The device models that actually implement I/O accounting. Since > >query-blockstats names a backend rather than a device model, we need > >a BlockDevOps callback to fetch the stats. Fetch fails when the > >callback is null. Lets us distinguish "no stats yet" and "device > >model can't do stats", thus permits a QMP interface that doesn't lie. > > > > Right now, I like (2) better. > > So let's say I have some block device, which is attached to a guest > device for a while, but then I detach it and continue using it in a > different place (maybe another guest device or a block job). Should we > really reset all counters in query-blockstats to 0? > > I think as I user I would be surprised about this, because I still refer > to it by the same name (the device_name, which will be in the BB), so > it's the same thing for me and the total requests include everything > that was ever issued against it. This particular cloud provider think that associating the stats with the emulated hardware is the less worisome to manage. So now we need to discuss futher in the community about this. Best regards Benoît > > > > -API wize I think about adding > > > bdrv_acct_invalid() and > > > bdrv_acct_failed() and systematically issuing a bdrv_acct_start() asap. > > > > Complication: partial success. Example: > > > > 1. Guest requests a read of N sectors. > > > > 2. Device model calls > >bdrv_acct_start(s->bs, &req->acct, N * BDRV_SECTOR_SIZE, BDRV_ACCT_READ) > > > > 3. Device model examines the request, and deems it valid. > > > > 4. Device model passes it to the block layer. > > > > 5. Block layer does its thing, but for some reason only M < N sectors > >can be read. Block layer returns M. > > No, it returns -errno. > > > 6. What's the device model to do now? Both bdrv_acct_failed() and > >bdrv_acct_done() would be wrong. > > > >Should the device model account for a read of size M? This ignores > >the par
Re: [libvirt] [PATCH-RFC] qemu: Add network bandwidth setting for ethernet interfaces
On 09/05/2014 04:31 AM, Martin Kletzander wrote:
> On Thu, Sep 04, 2014 at 03:02:54PM -0700, Anirban Chakraborty wrote:
>> ethernet interfaces in libvirt currently do not support bandwidth
>> setting.
>> For example, following xml file for an interface will not apply these
>> settings to corresponding qdiscs.
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> -
>>
>> This patch fixes the behavior.
>
> Although this doesn't confuse git, it might confuse something else.
> Leaving it without '' is ok.
>
>> Please review it and if it appears ok, please apply.
>>
>> thanks,
>> Anirban Chakraborty
>>
>
> No need to have this in the commit message, it's kept in the log then.
>
>>
>> Signed-off-by: Anirban Chakraborty
>> ---
>> src/qemu/qemu_command.c | 5 +
>> src/qemu/qemu_hotplug.c | 3 +++
>> 2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 2184caa..258c6a7 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -7251,6 +7251,11 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>> if (tapfd[0] < 0)
>> goto cleanup;
>> }
>> +/* Configure network bandwidth for ethernet type network
>> interfaces */
>> +if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)
>> +if (virNetDevBandwidthSet(net->ifname,
>> +virDomainNetGetActualBandwidth(net), false) < 0)
>> +goto cleanup;
>>
>
> In libvirt, we indent by spaces. This would be caught by running
> 'make syntax-check'. Anyway, running 'make syntax-check check' is a
> good practice to follow before formatting the patches.
>
>> if ((actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
>> actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index a364c52..aeb53c5 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -940,6 +940,9 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>> if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd,
>> &vhostfdSize) < 0)
>> goto cleanup;
>> } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) {
>> +if (virNetDevBandwidthSet(net->ifname,
>> +virDomainNetGetActualBandwidth(net), false) < 0)
>> +goto cleanup;
>
> Same here.
>
> We need to clear the bandwidth settings when shutting down the domain
> or unplugging the device.
Aside from that, I would prefer if we could consolidate to *fewer* calls
to virNetDevBandwidthSet() rather than adding more special case warts on
the code. If the current calls to that function could be moved up in the
call stack from their current low-level locations that are specific to a
particular type of interface, perhaps they could all converge to a
single place. Then, that single place could make the call for all
interface types that supported bandwidth setting, and log an error
message for all interface types that didn't support it.
(as a matter of fact, I would eventually like to get all stuff like this
moved into the equivalent location to networkAllocateActualDevice(), so
that 1) a single call would take care of *everything* for a network
device, regardless of type, and 2) it would be possible to place that
single function behind a public API that could be callable from
session-mode libvirtd to enable unprivileged guests to have access to
all the different types of network connectivity.)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 04/18] blockjob: split out block info monitor handling
On 08/30/2014 10:02 PM, Eric Blake wrote:
> Another layer of overly-multiplexed code that deserves to be
> split into obviously separate paths for query vs. modify.
> This continues the cleanup started in the previous patch.
>
> In the process, make some tweaks to simplify the logic when
> parsing the JSON reply. There should be no user-visible
> semantic changes.
>
In addition to the ATTRIBUTE_NONNULL addition, I found a bug that needs
fixing:
> +int
> +qemuMonitorJSONBlockJobInfo(qemuMonitorPtr mon,
> +const char *device,
> +virDomainBlockJobInfoPtr info)
> {
> +virJSONValuePtr cmd = NULL;
> +virJSONValuePtr reply = NULL;
> virJSONValuePtr data;
> int nr_results;
> size_t i;
> +int ret;
>
> -if (!info)
> +cmd = qemuMonitorJSONMakeCommand("query-block-jobs", NULL);
> +if (!cmd)
> return -1;
> +ret = qemuMonitorJSONCommand(mon, cmd, &reply);
> +if (ret < 0)
> +goto cleanup;
If we haven't errored out yet, then ret is now 0...
>
> if ((data = virJSONValueObjectGet(reply, "return")) == NULL) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("reply was missing return data"));
> -return -1;
> +goto cleanup;
...and we have changed the return code from -1 to 0 here. Oops. I'm
squashing this in, before pushing:
diff --git i/src/qemu/qemu_monitor_json.c w/src/qemu/qemu_monitor_json.c
index 68ba084..2b58c78 100644
--- i/src/qemu/qemu_monitor_json.c
+++ w/src/qemu/qemu_monitor_json.c
@@ -3753,13 +3753,12 @@ qemuMonitorJSONBlockJobInfo(qemuMonitorPtr mon,
virJSONValuePtr data;
int nr_results;
size_t i;
-int ret;
+int ret = -1;
cmd = qemuMonitorJSONMakeCommand("query-block-jobs", NULL);
if (!cmd)
return -1;
-ret = qemuMonitorJSONCommand(mon, cmd, &reply);
-if (ret < 0)
+if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
goto cleanup;
if ((data = virJSONValueObjectGet(reply, "return")) == NULL) {
@@ -3780,7 +3779,7 @@ qemuMonitorJSONBlockJobInfo(qemuMonitorPtr mon,
goto cleanup;
}
-for (i = 0, ret = 0; i < nr_results && ret == 0; i++) {
+for (i = ret = 0; i < nr_results && ret == 0; i++) {
virJSONValuePtr entry = virJSONValueArrayGet(data, i);
if (!entry) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3.5 05.5/18] blockjob: add new monitor json conversions
On 09/05/2014 06:00 AM, Peter Krempa wrote: > On 09/05/14 13:36, Eric Blake wrote: >> The previous patch hoisted some bounds checks to the callers; >> but someone that is not aware of the hoisted check could now >> try passing an integer between LLONG_MAX and ULLONG_MAX. As a >> safety measure, add new json conversion modes that let libvirt >> error out early instead of pass bad numbers to qemu, if the >> caller ever makes a mistake due to later refactoring. >> > > Exactly what I had in mind. ACK. Thanks; I've pushed 4, 5, and 5.5 now (still double-checking the rest of the series, and will push as I go). -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 06/18] blockjob: allow finer bandwidth tuning for query
On 09/04/2014 10:11 AM, Peter Krempa wrote: > On 08/31/14 06:02, Eric Blake wrote: >> While reviewing the new virDomainBlockCopy API, Peter Krempa >> pointed out that our existing design of using MiB/s for block >> job bandwidth is rather coarse, especially since qemu tracks >> it in bytes/s; so virDomainBlockCopy only accepts bytes/s. >> But once the new API is implemented for qemu, we will be in >> the situation where it is possible to set a value that cannot >> be accurately reflected back to the user, because the existing >> virDomainGetBlockJobInfo defaults to the coarser units. >> > > ACK. I presume virsh will be modified later in the series. Indeed. I'm still working on the virsh modifications for setting bandwidth in a different scale, but 9/18 was the modification for getting in smarter units. This one is now pushed. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Entering freeze for libvirt-1.2.8
On Fri, Aug 29, 2014 at 12:03:24PM +0200, Daniel Veillard wrote: > On Wed, Aug 27, 2014 at 08:45:29PM +0200, Richard Weinberger wrote: > > On Wed, Aug 27, 2014 at 9:18 AM, Daniel Veillard > > wrote: > > > So I tagged 1.2.8-rc1 in git and made tarball and signed rpms > > > > Can you please sign the tarball too? > > Okay, I went the simplest route of creating an asc for the tarball, > my key is on the mit server: > > user: "Daniel Veillard (Red Hat work email) " > 1024-bit DSA key, ID DE95BC1F, created 2000-05-31 > > I also added asc for the latest 1.2.x releases along the tarballs, Awesome. Thanks for the signatures, that saves some extra steps here. Cheers, -- Guido -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/6] Refactor job statistics
On 09/01/2014 11:05 AM, Jiri Denemark wrote:
> Job statistics data were tracked in several structures and variables.
> Let's make a new qemuDomainJobInfo structure which can be used as a
> single source of statistics data as a preparation for storing data about
> completed a job.
>
> Signed-off-by: Jiri Denemark
> ---
> src/qemu/qemu_domain.c| 157
> --
> src/qemu/qemu_domain.h| 24 ++-
> src/qemu/qemu_driver.c| 119 ---
> src/qemu/qemu_migration.c | 72 -
> 4 files changed, 213 insertions(+), 159 deletions(-)
>
Curious why the decision to use "Ptr" instead of inline struct for
current & completed? When I saw an alloc in the middle of a structure
and then a few free's along the way I began to wonder and attempt to
research the various paths to make sure all accesses knew/checked that
the impending deref would be OK - I marked those I found... I think one
or two may need double check since I assume you know the overall code
paths better. I see there's a path from qemuProcessReconnect() - that's
the libvirtd restart path right? So when the VIR_ALLOC() is done for
current/completed - who's address space is that? Wouldn't that be
address space from the "old" libvirtd?
Maybe I'm overthinking it on a Friday :-)
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index e9506e0..2c709e9 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -163,11 +163,9 @@ qemuDomainObjResetAsyncJob(qemuDomainObjPrivatePtr priv)
> job->asyncOwner = 0;
> job->phase = 0;
> job->mask = DEFAULT_JOB_MASK;
> -job->start = 0;
> job->dump_memory_only = false;
> job->asyncAbort = false;
> -memset(&job->status, 0, sizeof(job->status));
> -memset(&job->info, 0, sizeof(job->info));
> +VIR_FREE(job->current);
This was one of those free paths where there were multiple callers and I
wasn't 100% other accesses needed to be checked w/r/t to this occurring
before an unchecked access in some other path...
> }
>
> void
> @@ -200,6 +198,7 @@ qemuDomainObjTransferJob(virDomainObjPtr obj)
> static void
> qemuDomainObjFreeJob(qemuDomainObjPrivatePtr priv)
> {
> +VIR_FREE(priv->job.current);
> virCondDestroy(&priv->job.cond);
> virCondDestroy(&priv->job.asyncCond);
> }
> @@ -211,6 +210,150 @@ qemuDomainTrackJob(qemuDomainJob job)
> }
>
>
> +int
> +qemuDomainJobInfoUpdateTime(qemuDomainJobInfoPtr jobInfo)
> +{
> +unsigned long long now;
> +
> +if (!jobInfo->started)
> +return 0;
any chance jobInfo == NULL? I think the only path where the caller
doesn't check for current (or completed) is qemuMigrationUpdateJobStatus
> +
> +if (virTimeMillisNow(&now) < 0)
> +return -1;
> +
> +jobInfo->timeElapsed = now - jobInfo->started;
> +return 0;
> +}
> +
> +int
> +qemuDomainJobInfoToInfo(qemuDomainJobInfoPtr jobInfo,
> +virDomainJobInfoPtr info)
> +{
Called with job.current checked
> +info->timeElapsed = jobInfo->timeElapsed;
> +info->timeRemaining = jobInfo->timeRemaining;
> +
> +info->memTotal = jobInfo->status.ram_total;
> +info->memRemaining = jobInfo->status.ram_remaining;
> +info->memProcessed = jobInfo->status.ram_transferred;
> +
> +info->fileTotal = jobInfo->status.disk_total;
> +info->fileRemaining = jobInfo->status.disk_remaining;
> +info->fileProcessed = jobInfo->status.disk_transferred;
> +
> +info->dataTotal = info->memTotal + info->fileTotal;
> +info->dataRemaining = info->memRemaining + info->fileRemaining;
> +info->dataProcessed = info->memProcessed + info->fileProcessed;
> +
> +return 0;
> +}
> +
> +int
> +qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
> + int *type,
> + virTypedParameterPtr *params,
> + int *nparams)
> +{
Called with job.current (and .completed) checked...
> +qemuMonitorMigrationStatus *status = &jobInfo->status;
> +virTypedParameterPtr par = NULL;
> +int maxpar = 0;
> +int npar = 0;
> +
> +if (virTypedParamsAddULLong(&par, &npar, &maxpar,
> +VIR_DOMAIN_JOB_TIME_ELAPSED,
> +jobInfo->timeElapsed) < 0)
> +goto error;
> +
> +if (jobInfo->type == VIR_DOMAIN_JOB_BOUNDED &&
> +virTypedParamsAddULLong(&par, &npar, &maxpar,
> +VIR_DOMAIN_JOB_TIME_REMAINING,
> +jobInfo->timeRemaining) < 0)
> +goto error;
> +
> +if (status->downtime_set &&
> +virTypedParamsAddULLong(&par, &npar, &maxpar,
> +VIR_DOMAIN_JOB_DOWNTIME,
> +status->downtime) < 0)
> +goto error;
> +
> +if (virTypedParamsAddULLong(&par, &npar, &maxpar,
> +VIR_DOMAIN_JOB_DAT
Re: [libvirt] [PATCH 2/6] Add support for fetching statistics of completed jobs
On 09/01/2014 11:05 AM, Jiri Denemark wrote:
> virDomainGetJobStats gains new VIR_DOMAIN_JOB_STATS_COMPLETED flag that
> can be used to fetch statistics of a completed job rather than a
> currently running job.
>
> Signed-off-by: Jiri Denemark
> ---
> include/libvirt/libvirt.h.in | 11 +++
> src/libvirt.c| 8 +++-
> src/qemu/qemu_domain.c | 1 +
> src/qemu/qemu_domain.h | 1 +
> src/qemu/qemu_driver.c | 25 +++--
> src/qemu/qemu_migration.c| 6 ++
> src/qemu/qemu_monitor_json.c | 3 ++-
> 7 files changed, 47 insertions(+), 8 deletions(-)
>
This seems AOK - see my note at the end about qemu_monitor_json.c though.
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 9358314..9670e06 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -4306,6 +4306,17 @@ struct _virDomainJobInfo {
> unsigned long long fileRemaining;
> };
>
> +/**
> + * virDomainGetJobStatsFlags:
> + *
> + * Flags OR'ed together to provide specific behavior when querying domain
> + * job statistics.
> + */
> +typedef enum {
> +VIR_DOMAIN_JOB_STATS_COMPLETED = 1 << 0, /* return stats of a recently
> + * completed job */
> +} virDomainGetJobStatsFlags;
> +
> int virDomainGetJobInfo(virDomainPtr dom,
> virDomainJobInfoPtr info);
> int virDomainGetJobStats(virDomainPtr domain,
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 5d8f01c..6fa0a6b 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -17567,7 +17567,7 @@ virDomainGetJobInfo(virDomainPtr domain,
> virDomainJobInfoPtr info)
> * @type: where to store the job type (one of virDomainJobType)
> * @params: where to store job statistics
> * @nparams: number of items in @params
> - * @flags: extra flags; not used yet, so callers should always pass 0
> + * @flags: bitwise-OR of virDomainGetJobStatsFlags
> *
> * Extract information about progress of a background job on a domain.
> * Will return an error if the domain is not active. The function returns
> @@ -17577,6 +17577,12 @@ virDomainGetJobInfo(virDomainPtr domain,
> virDomainJobInfoPtr info)
> * may receive fields that they do not understand in case they talk to a
> * newer server.
> *
> + * When @flags contains VIR_DOMAIN_JOB_STATS_COMPLETED, the function will
> + * return statistics about a recently completed job. Specifically, this
> + * flag may be used to query statistics of a completed incoming migration.
> + * Statistics of a completed job are automatically destroyed once read or
> + * when libvirtd is restarted.
> + *
^
Yeah - my question from patch 1 with respect to checking access for
job.current especially... The job.completed seems fine.
> * Returns 0 in case of success and -1 in case of failure.
> */
> int
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2c709e9..18a3761 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -199,6 +199,7 @@ static void
> qemuDomainObjFreeJob(qemuDomainObjPrivatePtr priv)
> {
> VIR_FREE(priv->job.current);
> +VIR_FREE(priv->job.completed);
> virCondDestroy(&priv->job.cond);
> virCondDestroy(&priv->job.asyncCond);
> }
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 119590e..365238b 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -124,6 +124,7 @@ struct qemuDomainJobObj {
> unsigned long long mask;/* Jobs allowed during async job */
> bool dump_memory_only; /* use dump-guest-memory to do dump
> */
> qemuDomainJobInfoPtr current; /* async job progress data */
> +qemuDomainJobInfoPtr completed; /* statistics data of a recently
> completed job */
> bool asyncAbort;/* abort of async job requested */
> };
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 265315d..cd6932a 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -11651,9 +11651,10 @@ qemuDomainGetJobStats(virDomainPtr dom,
> {
> virDomainObjPtr vm;
> qemuDomainObjPrivatePtr priv;
> +qemuDomainJobInfoPtr jobInfo;
> int ret = -1;
>
> -virCheckFlags(0, -1);
> +virCheckFlags(VIR_DOMAIN_JOB_STATS_COMPLETED, -1);
>
> if (!(vm = qemuDomObjFromDomain(dom)))
> goto cleanup;
> @@ -11663,13 +11664,19 @@ qemuDomainGetJobStats(virDomainPtr dom,
> if (virDomainGetJobStatsEnsureACL(dom->conn, vm->def) < 0)
> goto cleanup;
>
> -if (!virDomainObjIsActive(vm)) {
> +if (!(flags & VIR_DOMAIN_JOB_STATS_COMPLETED) &&
> +!virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> "%s", _("domain is not running"));
> goto cleanup;
> }
>
> -if (!priv->job.current) {
> +if (flags & VIR_DOMAI
Re: [libvirt] [PATCH 3/6] virsh: Add support for completed job stats
On 09/01/2014 11:05 AM, Jiri Denemark wrote:
> New --completed flag for virsh domjobinfo command.
>
> Signed-off-by: Jiri Denemark
> ---
> tools/virsh-domain.c | 27 ---
> tools/virsh.pod | 5 +++--
> 2 files changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index c75cd73..1ff264e 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -5164,6 +5164,10 @@ static const vshCmdOptDef opts_domjobinfo[] = {
> .flags = VSH_OFLAG_REQ,
> .help = N_("domain name, id or uuid")
> },
> +{.name = "completed",
> + .type = VSH_OT_BOOL,
> + .help = N_("return statistics of a recently completed job")
> +},
> {.name = NULL}
> };
>
> @@ -5195,14 +5199,18 @@ cmdDomjobinfo(vshControl *ctl, const vshCmd *cmd)
> virTypedParameterPtr params = NULL;
> int nparams = 0;
> unsigned long long value;
> +unsigned int flags = 0;
> int rc;
>
> if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
> return false;
>
> +if (vshCommandOptBool(cmd, "completed"))
> +flags |= VIR_DOMAIN_JOB_STATS_COMPLETED;
> +
> memset(&info, 0, sizeof(info));
>
> -rc = virDomainGetJobStats(dom, &info.type, ¶ms, &nparams, 0);
> +rc = virDomainGetJobStats(dom, &info.type, ¶ms, &nparams, flags);
> if (rc == 0) {
> if (virTypedParamsGetULLong(params, nparams,
> VIR_DOMAIN_JOB_TIME_ELAPSED,
> @@ -5239,6 +5247,11 @@ cmdDomjobinfo(vshControl *ctl, const vshCmd *cmd)
> &info.fileRemaining) < 0)
> goto save_error;
> } else if (last_error->code == VIR_ERR_NO_SUPPORT) {
> +if (flags) {
> +vshError(ctl, "%s", _("Optional flags are not supported by the "
> + "daemon"));
> +goto cleanup;
> +}
> vshDebug(ctl, VSH_ERR_DEBUG, "detailed statistics not supported\n");
> vshResetLibvirtError();
> rc = virDomainGetJobInfo(dom, &info);
> @@ -5249,7 +5262,9 @@ cmdDomjobinfo(vshControl *ctl, const vshCmd *cmd)
> vshPrint(ctl, "%-17s %-12s\n", _("Job type:"),
> vshDomainJobToString(info.type));
> if (info.type != VIR_DOMAIN_JOB_BOUNDED &&
> -info.type != VIR_DOMAIN_JOB_UNBOUNDED) {
> +info.type != VIR_DOMAIN_JOB_UNBOUNDED &&
> +(!(flags & VIR_DOMAIN_JOB_STATS_COMPLETED) ||
> + info.type != VIR_DOMAIN_JOB_COMPLETED)) {
> ret = true;
> goto cleanup;
> }
> @@ -5314,7 +5329,13 @@ cmdDomjobinfo(vshControl *ctl, const vshCmd *cmd)
>&value)) < 0) {
> goto save_error;
> } else if (rc) {
> -vshPrint(ctl, "%-17s %-12llu ms\n", _("Expected downtime:"), value);
> +if (info.type == VIR_DOMAIN_JOB_COMPLETED) {
> +vshPrint(ctl, "%-17s %-12llu ms\n",
> + _("Total downtime:"), value);
> +} else {
> +vshPrint(ctl, "%-17s %-12llu ms\n",
> + _("Expected downtime:"), value);
> +}
> }
>
> if ((rc = virTypedParamsGetULLong(params, nparams,
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index ea9267e..3c71db9 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -1112,9 +1112,10 @@ Convert a domain name (or UUID) to a domain id
>
> Abort the currently running domain job.
>
> -=item B I
> +=item B I [I<--completed>]
>
> -Returns information about jobs running on a domain.
> +Returns information about jobs running on a domain. I<--completed> tells
> +virsh to return information about a recently finished job.
Perhaps a bit more about the "once read" or "libvirtd" restart will
cause statistics to be destroyed.
ACK with that
John
>
> =item B I
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] qemu: Transfer migration statistics to destination
On 09/01/2014 11:05 AM, Jiri Denemark wrote:
> When migrating a transient domain or with VIR_MIGRATE_UNDEFINE_SOURCE
> flag, the domain may disappear from source host. And so will migration
> statistics associated with the domain. We need to transfer the
> statistics at the end of a migration so that they can be queried at the
> destination host.
>
> Signed-off-by: Jiri Denemark
> ---
> src/qemu/qemu_migration.c | 190
> +-
> 1 file changed, 187 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 208a21f..f1b3d50 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -80,6 +80,7 @@ enum qemuMigrationCookieFlags {
> QEMU_MIGRATION_COOKIE_FLAG_PERSISTENT,
> QEMU_MIGRATION_COOKIE_FLAG_NETWORK,
> QEMU_MIGRATION_COOKIE_FLAG_NBD,
> +QEMU_MIGRATION_COOKIE_FLAG_STATS,
>
> QEMU_MIGRATION_COOKIE_FLAG_LAST
> };
> @@ -91,7 +92,8 @@ VIR_ENUM_IMPL(qemuMigrationCookieFlag,
>"lockstate",
>"persistent",
>"network",
> - "nbd");
> + "nbd",
> + "statistics");
>
> enum qemuMigrationCookieFeatures {
> QEMU_MIGRATION_COOKIE_GRAPHICS = (1 <<
> QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS),
> @@ -99,6 +101,7 @@ enum qemuMigrationCookieFeatures {
> QEMU_MIGRATION_COOKIE_PERSISTENT = (1 <<
> QEMU_MIGRATION_COOKIE_FLAG_PERSISTENT),
> QEMU_MIGRATION_COOKIE_NETWORK = (1 <<
> QEMU_MIGRATION_COOKIE_FLAG_NETWORK),
> QEMU_MIGRATION_COOKIE_NBD = (1 << QEMU_MIGRATION_COOKIE_FLAG_NBD),
> +QEMU_MIGRATION_COOKIE_STATS = (1 << QEMU_MIGRATION_COOKIE_FLAG_STATS),
> };
>
> typedef struct _qemuMigrationCookieGraphics qemuMigrationCookieGraphics;
> @@ -169,6 +172,9 @@ struct _qemuMigrationCookie {
>
> /* If (flags & QEMU_MIGRATION_COOKIE_NBD) */
> qemuMigrationCookieNBDPtr nbd;
> +
> +/* If (flags & QEMU_MIGRATION_COOKIE_STATS) */
> +qemuDomainJobInfoPtr jobInfo;
> };
>
> static void qemuMigrationCookieGraphicsFree(qemuMigrationCookieGraphicsPtr
> grap)
> @@ -533,6 +539,25 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig,
> }
>
>
> +static int
> +qemuMigrationCookieAddStatistics(qemuMigrationCookiePtr mig,
> + virDomainObjPtr vm)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> +if (!priv->job.completed)
> +return 0;
> +
> +if (!mig->jobInfo && VIR_ALLOC(mig->jobInfo) < 0)
> +return -1;
> +
> +*mig->jobInfo = *priv->job.completed;
> +mig->flags |= QEMU_MIGRATION_COOKIE_STATS;
> +
> +return 0;
> +}
> +
> +
> static void qemuMigrationCookieGraphicsXMLFormat(virBufferPtr buf,
>
> qemuMigrationCookieGraphicsPtr grap)
> {
> @@ -589,6 +614,81 @@ qemuMigrationCookieNetworkXMLFormat(virBufferPtr buf,
> }
>
>
> +static void
> +qemuMigrationCookieStatisticsXMLFormat(virBufferPtr buf,
> + qemuDomainJobInfoPtr jobInfo)
> +{
> +qemuMonitorMigrationStatus *status = &jobInfo->status;
> +
> +virBufferAddLit(buf, "\n");
> +virBufferAdjustIndent(buf, 2);
> +
> +virBufferAsprintf(buf, "<%1$s>%2$llu\n",
> + VIR_DOMAIN_JOB_TIME_ELAPSED,
> + jobInfo->timeElapsed);
> +virBufferAsprintf(buf, "<%1$s>%2$llu\n",
> + VIR_DOMAIN_JOB_TIME_REMAINING,
> + jobInfo->timeRemaining);
qemuDomainJobInfoToParams will use jobInfo->type ==
VIR_DOMAIN_JOB_BOUNDED when printing the above - dies this need to as well?
I would suspect this would be zero anyway, right? Since the job is done.
> +if (status->downtime_set)
> +virBufferAsprintf(buf, "<%1$s>%2$llu\n",
> + VIR_DOMAIN_JOB_DOWNTIME,
> + status->downtime);
What about the VIR_DOMAIN_JOB_DATA_* values? I know they are calculable,
but since they're
> +
> +virBufferAsprintf(buf, "<%1$s>%2$llu\n",
> + VIR_DOMAIN_JOB_MEMORY_TOTAL,
> + status->ram_total);
> +virBufferAsprintf(buf, "<%1$s>%2$llu\n",
> + VIR_DOMAIN_JOB_MEMORY_PROCESSED,
> + status->ram_transferred);
> +virBufferAsprintf(buf, "<%1$s>%2$llu\n",
> + VIR_DOMAIN_JOB_MEMORY_REMAINING,
> + status->ram_remaining);
> +
> +if (status->ram_duplicate_set) {
> +virBufferAsprintf(buf, "<%1$s>%2$llu\n",
> + VIR_DOMAIN_JOB_MEMORY_CONSTANT,
> + status->ram_duplicate);
> +virBufferAsprintf(buf, "<%1$s>%2$llu\n",
> + VIR_DOMAIN_JOB_MEMORY_NORMAL,
> + status->ram_normal);
> +virBufferAsprintf(buf, "<%1$s>%2$llu\n",
> + VIR_DOMAIN_JOB_MEMORY_NORMAL_BYTES,
> +
Re: [libvirt] [PATCH 5/6] qemu: Recompute downtime and total time when migration completes
On 09/01/2014 11:05 AM, Jiri Denemark wrote:
> Total time of a migration and total downtime transfered from a source to
> a destination host do not count with the transfer time to the
> destination host and with the time elapsed before guest CPUs are
> resumed. Thus, source libvirtd remembers when migration started and when
> guest CPUs were paused. Both timestamps are transferred to destination
> libvirtd which uses them to compute total migration time and total
> downtime. This, obviously, requires clock to be synchronized between the
s/This, obviously,/Obviously this/
"requires clock" reads funny... "requires the time" seems closer
> two hosts. The reported times are useless otherwise but they would be
> equally useless if we didn't do this recomputation so don't lose
> anything by doing it.
>
Say nothing of inter-timezone migrations right?
> Signed-off-by: Jiri Denemark
> ---
> src/libvirt.c | 5 -
> src/qemu/qemu_domain.c| 28
> src/qemu/qemu_domain.h| 2 ++
> src/qemu/qemu_migration.c | 15 ++-
> src/qemu/qemu_process.c | 9 -
> tools/virsh.pod | 5 -
> 6 files changed, 60 insertions(+), 4 deletions(-)
>
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 6fa0a6b..61d0543 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -17581,7 +17581,10 @@ virDomainGetJobInfo(virDomainPtr domain,
> virDomainJobInfoPtr info)
> * return statistics about a recently completed job. Specifically, this
> * flag may be used to query statistics of a completed incoming migration.
> * Statistics of a completed job are automatically destroyed once read or
> - * when libvirtd is restarted.
> + * when libvirtd is restarted. Note that time information returned for
> + * completed migrations may be completely irrelevant unless both source and
> + * destination hosts have synchronized time (i.e., NTP daemon is running on
> + * both of them).
> *
> * Returns 0 in case of success and -1 in case of failure.
> */
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 18a3761..cec7828 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -222,11 +222,39 @@ qemuDomainJobInfoUpdateTime(qemuDomainJobInfoPtr
> jobInfo)
> if (virTimeMillisNow(&now) < 0)
> return -1;
>
> +if (now < jobInfo->started) {
> +VIR_WARN("Async job starts in the future");
> +jobInfo->started = 0;
> +return 0;
> +}
> +
> jobInfo->timeElapsed = now - jobInfo->started;
> return 0;
> }
>
> int
> +qemuDomainJobInfoUpdateDowntime(qemuDomainJobInfoPtr jobInfo)
> +{
> +unsigned long long now;
> +
Can jobInfo == NULL? - It's the qemuMigrationWaitForCompletion() path
timing concern from patch 1.
> +if (!jobInfo->stopped)
> +return 0;
> +
> +if (virTimeMillisNow(&now) < 0)
> +return -1;
> +
> +if (now < jobInfo->stopped) {
> +VIR_WARN("Guest's CPUs stopped in the future");
> +jobInfo->stopped = 0;
> +return 0;
> +}
> +
> +jobInfo->status.downtime = now - jobInfo->stopped;
> +jobInfo->status.downtime_set = true;
> +return 0;
> +}
> +
> +int
> qemuDomainJobInfoToInfo(qemuDomainJobInfoPtr jobInfo,
> virDomainJobInfoPtr info)
> {
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 365238b..435a22b 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -105,6 +105,7 @@ typedef qemuDomainJobInfo *qemuDomainJobInfoPtr;
> struct _qemuDomainJobInfo {
> virDomainJobType type;
> unsigned long long started; /* When the async job started */
> +unsigned long long stopped; /* When the domain's CPUs were stopped */
> /* Computed values */
> unsigned long long timeElapsed;
> unsigned long long timeRemaining;
> @@ -390,6 +391,7 @@ bool qemuDomainAgentAvailable(qemuDomainObjPrivatePtr
> priv,
>bool reportError);
>
> int qemuDomainJobInfoUpdateTime(qemuDomainJobInfoPtr jobInfo);
> +int qemuDomainJobInfoUpdateDowntime(qemuDomainJobInfoPtr jobInfo);
Does this also need some sort of ATTRIBUTE_NONNULL(1)?
ACK in general
John
> int qemuDomainJobInfoToInfo(qemuDomainJobInfoPtr jobInfo,
> virDomainJobInfoPtr info);
> int qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index f1b3d50..43b42ac 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -623,6 +623,9 @@ qemuMigrationCookieStatisticsXMLFormat(virBufferPtr buf,
> virBufferAddLit(buf, "\n");
> virBufferAdjustIndent(buf, 2);
>
> +virBufferAsprintf(buf, "%llu\n", jobInfo->started);
> +virBufferAsprintf(buf, "%llu\n", jobInfo->stopped);
> +
> virBufferAsprintf(buf, "<%1$s>%2$llu\n",
>VIR_DOMAIN_JOB_TIME_ELAPSED,
>jobInfo->t
Re: [libvirt] [PATCH v3 08/18] blockjob: add new --raw flag to virsh blockjob
On 09/04/2014 10:23 AM, Peter Krempa wrote: > On 08/31/14 06:02, Eric Blake wrote: >> The current output of 'blockjob [--info]' is a single line >> designed for human consumption; it's not very nice for machine >> parsing. Furthermore, I have plans to modify the line in >> response to the new flag for controlling bandwidth units. >> Solve that by adding a --raw parameter, which outputs >> information closer to the C struct. >> >> $ virsh blockjob testvm1 vda --raw >> type=Block Copy >> bandwidth=1 >> cur=197120 >> end=197120 >> >> The information is indented, because I'd like for a later patch >> to add a mode that iterates over all the vm's disks with status >> for each; in that mode, each block name would be listed unindented >> before information (if any) about that block. I haven't actually finished adding a loop operation yet, but hope to have it in v4. >> * tools/virsh-domain.c (cmdBlockJob): Add --raw parameter; tweak >> error wording. >> * tools/virsh.pod (blockjob): Document it. >> >> Signed-off-by: Eric Blake >> --- >> tools/virsh-domain.c | 31 +++ >> tools/virsh.pod | 19 --- >> 2 files changed, 35 insertions(+), 15 deletions(-) >> > > ACK, Pushed after fixing an accidental double space. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/6] qemu: Transfer recomputed stats back to source
On 09/01/2014 11:05 AM, Jiri Denemark wrote: > After previous commit, migration statistics on source and destination > hosts are not equal because destination updated time statistics. Let's > send the result back so that the same data can be queried on both end of > a migration. My grammar bells and whistles are going off... "After the previous"... "on the source and..." "because the destination" "both ends of the migration" (or both sides) > > Signed-off-by: Jiri Denemark > --- > src/qemu/qemu_migration.c | 23 +-- > 1 file changed, 21 insertions(+), 2 deletions(-) > ACK John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 09/18] blockjob: add new --bytes flag to virsh blockjob
On 09/05/2014 03:05 AM, Peter Krempa wrote:
> On 08/31/14 06:02, Eric Blake wrote:
>> Expose the new flag just added to virDomainGetBlockJobInfo.
>> With --raw, the presence or absence of --bytes determines which
>> flag to use in the single API call. Without --raw, the use of
>> --bytes forces an error if the server doesn't support it,
>> otherwise, the code tries to silently fall back to scaling the
>> MiB/s value.
>>
>> My goal is to eventually also support --bytes in bandwidth mode;
>> but that's a bit further down the road (and needs a new API flag
>> added in libvirt.h first).
the new flag hinted at is patch 14/14; I'm still working on the code for
a v4 patch for actually setting in byte mode.
>
> ACK
>
Pushed with a tweak I found during self-review:
diff --git i/tools/virsh-domain.c w/tools/virsh-domain.c
index 749fa67..2bb3bbc 100644
--- i/tools/virsh-domain.c
+++ w/tools/virsh-domain.c
@@ -2121,7 +2121,7 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd)
if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0)
goto cleanup;
-/* If bytes were requested, or if raw mode is not forcing MiB/s
+/* If bytes were requested, or if raw mode is not forcing a MiB/s
* query and cache can't prove failure, then query bytes/sec. */
if (bytes || !(raw || ctl->blockJobNoBytes)) {
flags |= VIR_DOMAIN_BLOCK_JOB_INFO_BANDWIDTH_BYTES;
@@ -2154,9 +2154,11 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd)
/* Scale to bytes/s unless in raw mode */
if (!raw) {
speed <<= 20;
-if (speed >> 20 != info.bandwidth)
+if (speed >> 20 != info.bandwidth) {
vshError(ctl, _("overflow in converting %ld MiB/s to
bytes\n"),
info.bandwidth);
+goto cleanup;
+}
}
}
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 10/18] blockcopy: allow block device destination
On 09/05/2014 03:23 AM, Peter Krempa wrote: > On 08/31/14 06:02, Eric Blake wrote: >> To date, anyone performing a block copy and pivot ends up with >> the destination being treated as . While this >> works for data access for a block device, it has at least one >> noticeable shortcoming: virDomainGetBlockInfo() reports allocation >> differently for block devices visited as files (the size of the >> device) than for block devices visited as >> (the maximum sector used, as reported by qemu); and this difference >> is significant when trying to manage qcow2 format on block devices >> that can be grown as needed. >> >> > > ACK Pushed. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V2 1/1] libvirtd crash when defining scsi storage pool
On 09/05/2014 01:47 AM, Pradipta Kr. Banerjee wrote: > libvirtd crash when defining scsi storage pool > > libvirtd crashes when there is an existing SCSI pool defined > with adapter type as 'scsi_host' and defining a new SCSI pool with adapter > type as 'fc_host' and parent attribute missing or vice versa. > > For eg If there is an existing SCSI pool with adapter type as 'scsi_host' > defined using the following XML > > > TEST_SCSI_POOL > > > > > /dev/disk/by-path > > > > When defining another SCSI pool with adapter type as 'fc_host' using the > following XML will crash libvirtd > > > TEST_SCSI_FC_POOL > > > > > /dev/disk/by-path > > > > Same is true for the reverse case as well where there exists a SCSI pool with > adapter type as 'fc_host' and another SCSI pool is defined with adapter type > as > 'scsi_host' > > This happens because for fc_host 'name' is optional attribute whereas for > scsi_host its mandatory. However the check in libvirt for finding duplicate > storage pools doesn't take that into account while comparing, resulting into > libvirt crashing > > This patch fixes the issue. This patch is based on the suggestion from John > Ferlan > > Signed-off-by: Pradipta Kr. Banerjee > --- > V2: Incorporate suggestions from John Ferlan > > src/conf/storage_conf.c | 4 > 1 file changed, 4 insertions(+) > I adjusted the commit message slightly and pushed. Thanks - John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH-RFC] qemu: Add network bandwidth setting for ethernet interfaces
On 9/5/14, 1:31 AM, "Martin Kletzander" wrote:
>On Thu, Sep 04, 2014 at 03:02:54PM -0700, Anirban Chakraborty wrote:
>>ethernet interfaces in libvirt currently do not support bandwidth
>>setting.
>>For example, following xml file for an interface will not apply these
>>settings to corresponding qdiscs.
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>-
>>
>>This patch fixes the behavior.
>
>Although this doesn't confuse git, it might confuse something else.
>Leaving it without '' is ok.
>
>>Please review it and if it appears ok, please apply.
>>
>>thanks,
>>Anirban Chakraborty
>>
>
>No need to have this in the commit message, it's kept in the log then.
>
>>
>>Signed-off-by: Anirban Chakraborty
>>---
>> src/qemu/qemu_command.c | 5 +
>> src/qemu/qemu_hotplug.c | 3 +++
>> 2 files changed, 9 insertions(+), 1 deletion(-)
>>
>>diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>index 2184caa..258c6a7 100644
>>--- a/src/qemu/qemu_command.c
>>+++ b/src/qemu/qemu_command.c
>>@@ -7251,6 +7251,11 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>> if (tapfd[0] < 0)
>> goto cleanup;
>> }
>>+ /* Configure network bandwidth for ethernet type network interfaces */
>>+ if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)
>>+ if (virNetDevBandwidthSet(net->ifname,
>>+ virDomainNetGetActualBandwidth(net), false) < 0)
>>+ goto cleanup;
>>
>
>In libvirt, we indent by spaces. This would be caught by running
>'make syntax-check'. Anyway, running 'make syntax-check check' is a
>good practice to follow before formatting the patches.
Got it. Thanks for the pointer.
>
>> if ((actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
>> actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
>>diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>>index a364c52..aeb53c5 100644
>>--- a/src/qemu/qemu_hotplug.c
>>+++ b/src/qemu/qemu_hotplug.c
>>@@ -940,6 +940,9 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>> if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd,
>>&vhostfdSize) < 0)
>> goto cleanup;
>> } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) {
>>+ if (virNetDevBandwidthSet(net->ifname,
>>+ virDomainNetGetActualBandwidth(net), false) < 0)
>>+ goto cleanup;
>
>Same here.
>
>We need to clear the bandwidth settings when shutting down the domain
>or unplugging the device.
If I am not mistaken, currently, the bandwidth setting has been cleared
only for bridged devices (via
networkShutdownNetworkVirtual‹>virNetDevBandwidthClear ). Are you saying
that we should explicitly clear the bandwidth settings for unplugging and
shut down events? As far as my understanding, for both of these events,
the tap interface gets deleted, so there is no need to cleanup qdisc
settings of these devices.
>
>Looking forward to v2,
I am working on it right now and will send it out soon. Thanks for the
feedback.
Anirban
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
