Re: [libvirt] [Qemu-devel] Close the BlockDriverState when guest eject the media

2014-10-21 Thread Gonglei 
On 2014/10/21 13:53, Weidong Huang wrote:

 On 2014/10/20 19:39, Kevin Wolf wrote:
 
 Am 20.10.2014 um 13:27 hat Weidong Huang geschrieben:
 On 2014/10/20 17:41, Kevin Wolf wrote:

 Am 18.10.2014 um 12:02 hat Weidong Huang geschrieben:
 Hi ALL:

 There are two ways to eject the cdrom tray. One is by the eject's qmp 
 commmand(eject_device).
 The another one is by the guest(bdrv_eject). They have different results.

 Yes, they are different things.

 If a guest opens the tray (using bdrv_eject) and then closes it again,
 with no user interaction in between, the virtual media must still be in
 the drive and the guest must be able to access the same image again.
 Calling bdrv_close() in this case would be a bug.

 The goal of the monitor command eject on the other hand is to remove
 the medium so that the drive is empty. That a device with a closed tray
 has to be opened for this is only secondary.


 Thanks for your reply.

 There is a problem.

 1. Qemu receive the eject command.
 2. Runs eject_request_cb when an eject request is issued from the 
 monitor, the tray
 is closed, and the medium is locked. But the drive is not closed.
 3. Guest agree with opening tray and qemu will call bdrv_eject to complete. 
 The drive is
 still not close.

 So the result of the monitor command eject is not to remove the medium in 
 this situation.

 Now I understand, thanks for explaining.

 But I think libvirt can actually work correctly with what qemu offers
 today. qemu returns an error if the medium cannot be removed with the
 'eject' command and it only sends an eject request to the guest.

 With this error, libvirt can know that the DEVICE_TRAY_MOVED event
 doesn't mean that the medium has removed, but that it needs to issue
 another 'eject' command.

 If this isn't implemented correctly in libvirt today, this needs a
 libvirt fix rather than a qemu one.

 
 
 hi all!
 
 How about fix it in libvirt?
 

Cc'ing Eric for more attention.

Maybe He can give you some suggestion :)

Best regards,
-Gonglei

 eject_device: close the BlockDriverState(bdrv_close(bs))
 bdrv_eject: don't close the BlockDriverState,

 This is ambiguous. So libvirt can't handle some situations.

 libvirt send eject qmp command --- qemu send eject request to guest ---
 guest respond to qemu --- qemu emit tray_open event to libvirt ---
 libvirt will not send change qmp command if media source is null. So
 the media is not be replace to the null.

 What is the problem that libvirt has with the guest opening the tray? I
 don't think libvirt should even care about that case.


 For example, using libvirt to change media by xml below(media source is 
 null):
 disk type='file' device='cdrom'
 driver name='qemu'/
 target dev='hdb' bus='ide'/
 /disk

 libivrt return ok. But media still is in the guest.
 This is confused.

 Kevin

 .

 
 
 
 



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

Re: [libvirt] [Qemu-devel] spec, RFC: TLS support for NBDµ

2014-10-21 Thread Markus Armbruster 
Wouter Verhelst w...@uter.be writes:

 On Mon, Oct 20, 2014 at 01:51:43PM +0200, Markus Armbruster wrote:
 Stefan Hajnoczi stefa...@redhat.com writes:
 
  On Mon, Oct 20, 2014 at 08:58:14AM +0100, Daniel P. Berrange wrote:
  On Sat, Oct 18, 2014 at 07:33:22AM +0100, Richard W.M. Jones wrote:
   On Sat, Oct 18, 2014 at 12:03:23AM +0200, Wouter Verhelst wrote:
Hi all,

(added rjones from nbdkit fame -- hi there)
   
   [I'm happy to implement whatever you come up with, but I've added
   Florian Weimer to CC who is part of Red Hat's product security group]
   
So I think the following would make sense to allow TLS in NBD.

This would extend the newstyle negotiation by adding two options 
(i.e.,
client requests), one server reply, and one server error as well as
extend one existing reply, in the following manner:

- The two new commands are NBD_OPT_PEEK_EXPORT and NBD_OPT_STARTTLS. 
The
  former would be used to verify if the server will do TLS for a given
  export:

  C: NBD_OPT_PEEK_EXPORT
  S: NBD_REP_SERVER, with an extra field after the export name
 containing flags that describe the export (R/O vs R/W state,
 whether TLS is allowed and/or required).
  
  IMHO the server should never provide *any* information about the exported
  volume(s) until the TLS layer has been fully setup. ie we shouldn't only
  think about the actual block data transfers, we should protect the entire
  NBD protocol even metadata related operations.
 
  This makes sense.
 
 Seconded.

 Mmm. I suppose the NBD_OPT_PEEK_EXPORT message could be defined so that
 it is fine for an export which has the TLS required bit set to provide
 differing information after TLS has been negotiated.

Why is it useful to have a per-export TLS required bit?

Stefan's argument:

  TLS is about the transport, not about a particular NBD export.  The only
  thing that should be communicated is STARTTLS.

makes sense to me.

Wouldn't a per-export TLS required bit enlarge the attack surface?  It
puts the peek export operation into the without authentication
bucket *and* makes it more complex.

The least complex STARTTLS solution seems to be if server wants TLS,
absolutely nothing works but switching to TLS and any feature
negotiation necessary for the client to recognize the need to switch.

Hmm, further down you give a reason why you want to mix encrypted and
unencrypted exports.

 Furthermore, STARTTLS is vulnerable to active attacks: if you can get
 between the peers, you can make them fall back to unencrypted silently.
 How do you plan to guard against that?

 As I've said before in this discussion, encryption downgrade attacks are
 not specific to STARTTLS; as soon as you have have an encrypted and an
 unencrypted variant of a protocol, that becomes a problem. After all,
 if an attacker can modify the communication so that STARTTLS is filtered
 out of the communication, they can most likely also redirect all traffic
 to a decrypting/encrypting proxy.

 The only way to fix that is through userspace; make opportunistic TLS
 (i.e., use it if available, but move on if not) difficult to achieve.

 See also https://www.agwa.name/blog/post/starttls_considered_harmful

 A random blog post by an author who is speaking about STARTTLS in
 general terms is not a good technical argument for why STARTTLS is a bad
 idea in *this* specific case.

Misunderstanding.  I didn't mean to claim STARTTLS is bad.  If I
wanted to say that, I would've said it directly.  I was merely asking
how you plan to guard against downgrade attacks.  I gather your advice
is to make the client (QEMU) insist on TLS, and check the server's
certificate.  Correct?

 If I was defining a new protocol from scratch, I might dump the whole
 thing in TLS to begin with. But that's just not the case, so I have to
 deal with what exists already.

Yes.  You can still start over on a separate port.  However:

 In addition, with the current state of affairs, it is *not possible* to
 swap to an NBD device if you need to pipe its data through a separate
 socket than the one you're handing to the kernel. The result of that is
 that you can't do TLS on a device you want to swap to. This means we
 need to continue to support a protocol that can do TLS for some exports,
 and plain (unencrypted) traffic for other exports, *in the same running
 nbd-server instance*.

I don't understand enough about NBD to challenge this argument.  The sad
consequence is more complexity and a larger attack surface.

Out of curiosity: is this merely an implementation restriction, or is
it more fundamental?

 I did add the NBD_REP_ERR_TLS_REQD message for a server which does not
 export anything unencrypted, so that it can (after the initial few
 exchanges) reply to anything except for STARTTLS with lalala, I'm not
 listening until you encrypt things. However, unless it's fine to ditch
 support for swapping to an NBD device (not an option from where

[libvirt] [libvirt-python PATCH] Fix cannot use VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS flags in domainListGetStats

2014-10-21 Thread Luyao Huang 
When set flags=libvirt.VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, python 
will report
a  error:
OverflowError: signed integer is greater than maximum

Because VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS = 2147483648 (2**31), 
and it set a
signed int in PyArg_ParseTuple function.

if (!PyArg_ParseTuple(args, (char *)OOii:virDomainListGetStats,
  pyobj_conn, py_domlist, stats, flags))

When python = 2.3, 'I' means unsigned int and 'i' means int,so there should 
use 'I'.

From: https://docs.python.org/2/c-api/arg.html

I also found a lot of function use 'i' for a unsigned int, but i didn't change 
them.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
 libvirt-override.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index c887b71..6dacdac 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -8126,7 +8126,7 @@ libvirt_virDomainListGetStats(PyObject *self 
ATTRIBUTE_UNUSED,
 unsigned int flags;
 unsigned int stats;
 
-if (!PyArg_ParseTuple(args, (char *)OOii:virDomainListGetStats,
+if (!PyArg_ParseTuple(args, (char *)OOII:virDomainListGetStats,
   pyobj_conn, py_domlist, stats, flags))
 return NULL;
 
-- 
1.8.3.1

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

Re: [libvirt] [Qemu-devel] spec, RFC: TLS support for NBD

2014-10-21 Thread Daniel P. Berrange 
On Tue, Oct 21, 2014 at 12:10:39AM +0200, Wouter Verhelst wrote:
 On Mon, Oct 20, 2014 at 01:56:43PM +0200, Florian Weimer wrote:
  I cannot comment on whether the proposed STARTTLS command is at the correct
  stage of the NBD protocol.  If there is a protocol description for NBD, I
  can have a look.
 
 To make this discussion in that regard a bit easier, I've committed the
 proposed spec to a separate branch:
 
 https://github.com/yoe/nbd/blob/tlsspec/doc/proto.txt

So, if I'm understanding correctly the new style fixed handshake
currently works like this:

Server starts transmission with version info

  - S: NBDMAGIC
  - S: 0x49484156454F5054
  - S: 0x1

And the client acknowledges with a 32 bits of and first bit set

  - C: 0x1


Now the client has to request one or more options, of which the
NBD_OPT_EXPORT_NAME (0x1) option is mandatory, so it will be the
first thing the client sends next . So it would send

  - C: 0x49484156454F5054
  - C: 0x1
  - C: 0xa  (length of name)
  - C: volumename



You're proposing to add a new option NBD_OPT_STARTTLS (0x5). For this to
work we would need to update the language to say that the NBD_OPT_STARTTLS
must be the first option that the client sends to the server, because we
want the option name to transmit in ciphertext.

So the new protocol startup would be

Server starts transmission with version info

  - S: NBDMAGIC
  - S: 0x49484156454F5054
  - S: 0x1

And the client acknowledges with a 32 bits of zero

  - C: 0x1

Client requests TLS

  - C: 0x49484156454F5054
  - C: 0x5

Server acknowledges TLS:

  - S: 0x3e889045565a9
  - S: 0x5
  - S: 0x1 (REP_ACK)
  - S: 0x0

Now the TLS handshake is performed by client + server



The client can now set other options using the secure
channel, of which the NBD_OPT_EXPORT_NAME (0x1) option
is mandatory, so it will be the first thing the client
sends next . So they would send

  - C: 0x49484156454F5054
  - C: 0x1
  - C: 0xa  (length of name)
  - C: volumename

...etc...


This is secure when both client and server want to use TLS.

This is secure when the client wants TLS and the server does
not want TLS, because the server will reject TLS and the client
will then close the connection.

My concern is the case where the client does not want TLS but
the server does want TLS. In this case the client will immediately
send the NBD_OPT_EXPORT_NAME in clear text over the wire, not giving
the server any chance to reject the use of a clear text channel.

This problem is inherant to the approach of using the NBD protocol
options to negotiate TLS.



One way I see to solve that insecurity, would be for the server
to make use of one of the extra reserved bits in the very first
message it sends. ie we need to negotiate TLS immediately after the
version number / magic acknowledgment, before we actually start
the main body of the protocol

ie, with the new style fixed handshake the server should send

Server starts transmission with version info

  - S: NBDMAGIC
  - S: 0x49484156454F5054
  - S: 0x2 

And the client acknowledges with a 32bits and first two bits set

  - C: 0x2

The questionmark here is what happens when the client sees the
second bit of reserved field set ? 

The protocol docs merely say

  S: 16 bits of zero (bits 1-15 reserved for future use; bit 0 in use to
   signal fixed newstyle (see below))

But don't mention what clients should do upon seeing unknown bits
set in the server's message ?

If clients ignore unknown bits, then we have the same problem where a
client can ignore the TLS bit and start sending option name in the
clear before the server rejects the session.

If clients abort (close connection) on seeing unknown bits, then we
are good.



A 3rd alternative is to actually use a separate magic number, which
should guarantee the client would immediately close upon seeing a
magic number which indicated TLS is required.

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] - make virDomainDetachDeviceFlags respect VIR_DOMAIN_DEVICE_MODIFY_FORCE

2014-10-21 Thread Martin Kletzander 

On Fri, Oct 10, 2014 at 09:05:01AM +0200, Marcin Gibuła wrote:

Hi,

currently, there is no way to force disk detach from KVM guest if guest
does not cooperate. This patch makes virDomainDetachDeviceFlags() respect
VIR_DOMAIN_DEVICE_MODIFY_FORCE flag. When it's on, libvirt will always
call drive_del, regardless if guest responsed to ACPI unplug request or not.

---

diff -ru libvirt-1.2.6-orig/src/qemu/qemu_driver.c 
libvirt-1.2.6/src/qemu/qemu_driver.c
--- libvirt-1.2.6-orig/src/qemu/qemu_driver.c   2014-07-02 05:35:47.0 
+0200
+++ libvirt-1.2.6/src/qemu/qemu_driver.c2014-10-09 13:37:27.863897583 
+0200


Hi there,

thank you for taking the time to submit the patch.  However, I need to
point out few things.  The patch is based on libvirt-1.2.6 and hence
does not apply on top of current master branch.  Also, you might find
git pretty useful for creating the patches.  I wanted to redirect you
to our contributor guidelines [1], but they seem a bit off right from
the start, I'll see if someone will accept my proposal to modernize it
a bit.

Feel free to clone the current repository [2] and base your patch on
that, git format-patch will probably speed up your workflow as well.

To comment on the patch itself, have you tried that it really does
what you claim?  Looking at the patch I feel like it doesn't.  The
function qemuDomainDetachDeviceFlags(), that is the only one that
calls qemuDomainDetachDeviceLive(), also calls virCheckFlags() without
VIR_DOMAIN_DEVICE_MODIFY_FORCE, which means that call to that function
will end with error.

Do you think this is safe for all disks?  Are you trying to solve some
particular problem with this patch?

Martin

[1] http://libvirt.org/hacking.html
[2] located at git://libvirt.org/libvirt.git


@@ -6525,14 +6525,15 @@
static int
qemuDomainDetachDeviceLive(virDomainObjPtr vm,
   virDomainDeviceDefPtr dev,
-   virDomainPtr dom)
+   virDomainPtr dom,
+   int flags)
{
virQEMUDriverPtr driver = dom-conn-privateData;
int ret = -1;

switch (dev-type) {
case VIR_DOMAIN_DEVICE_DISK:
-ret = qemuDomainDetachDeviceDiskLive(driver, vm, dev);
+ret = qemuDomainDetachDeviceDiskLive(driver, vm, dev, flags);
break;
case VIR_DOMAIN_DEVICE_CONTROLLER:
ret = qemuDomainDetachDeviceControllerLive(driver, vm, dev);
@@ -7257,7 +7258,8 @@
virCapsPtr caps = NULL;

virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
-  VIR_DOMAIN_AFFECT_CONFIG, -1);
+  VIR_DOMAIN_AFFECT_CONFIG |
+  VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);

cfg = virQEMUDriverGetConfig(driver);

@@ -7343,7 +7345,7 @@
 VIR_DOMAIN_DEVICE_ACTION_DETACH)  0)
goto endjob;

-if ((ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom))  0)
+if ((ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom, flags))  0)
goto endjob;
/*
 * update domain status forcibly because the domain status may be
diff -ru libvirt-1.2.6-orig/src/qemu/qemu_hotplug.c 
libvirt-1.2.6/src/qemu/qemu_hotplug.c
--- libvirt-1.2.6-orig/src/qemu/qemu_hotplug.c  2014-06-27 05:50:18.0 
+0200
+++ libvirt-1.2.6/src/qemu/qemu_hotplug.c   2014-10-09 13:35:55.566375716 
+0200
@@ -2906,7 +2906,8 @@
static int
qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
- virDomainDiskDefPtr detach)
+ virDomainDiskDefPtr detach,
+ int flags)
{
int ret = -1;
qemuDomainObjPrivatePtr priv = vm-privateData;
@@ -2958,7 +2959,7 @@
qemuDomainObjExitMonitor(driver, vm);

rc = qemuDomainWaitForDeviceRemoval(vm);
-if (rc == 0 || rc == 1)
+if (rc == 0 || rc == 1 || (flags  VIR_DOMAIN_DEVICE_MODIFY_FORCE))
ret = qemuDomainRemoveDiskDevice(driver, vm, detach);
else
ret = 0;
@@ -2971,7 +2972,8 @@
static int
qemuDomainDetachDiskDevice(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
-   virDomainDiskDefPtr detach)
+   virDomainDiskDefPtr detach,
+   int flags)
{
int ret = -1;
qemuDomainObjPrivatePtr priv = vm-privateData;
@@ -3003,7 +3005,7 @@
qemuDomainObjExitMonitor(driver, vm);

rc = qemuDomainWaitForDeviceRemoval(vm);
-if (rc == 0 || rc == 1)
+if (rc == 0 || rc == 1 || (flags  VIR_DOMAIN_DEVICE_MODIFY_FORCE))
ret = qemuDomainRemoveDiskDevice(driver, vm, detach);
else
ret = 0;
@@ -3030,7 +3032,8 @@
int
qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
-   virDomainDeviceDefPtr dev)
+   virDomainDeviceDefPtr dev,
+   int flags)
{

[libvirt] [PATCH] qemu: unref cfg after TerminateMachine has been called

2014-10-21 Thread Martin Kletzander 
Commit 60627f6d added the code that requests driver cfg, but forgot to
unref it.

Signed-off-by: Martin Kletzander mklet...@redhat.com
---
 src/qemu/qemu_cgroup.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index fa894c5..b5bdb36 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -1222,6 +1222,8 @@ qemuRemoveCgroup(virQEMUDriverPtr driver,
 VIR_DEBUG(Failed to terminate cgroup for %s, vm-def-name);
 }

+virObjectUnref(cfg);
+
 return virCgroupRemove(priv-cgroup);
 }

-- 
2.1.2

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

Re: [libvirt] [PATCH] qemu: move setting emulatorpin ahead of monitor showing up

2014-10-21 Thread Martin Kletzander 

On Thu, Oct 16, 2014 at 10:18:48PM +0800, Wang Rui wrote:

From: Zhou yimin zhouyi...@huawei.com

If VM is configured with many devices(including passthrough devices)
and large memory, libvirtd will take seconds(in the worst case) to
wait for monitor. In this period the qemu process may run on any
PCPU though I intend to pin emulator to the specified PCPU in xml
configuration.

Actually qemu process takes high cpu usage during vm startup.
So this is not the strict CPU isolation in this case.



This makes sense and it's also the only TID we can pin before
accessing the monitor.  I worried about this patch breaking other
pinnings, but whatever I tried it just works.

ACK and I'll push it shortly.

Martin


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

[libvirt] [PATCH] docs: Mention repository locations in contributor guidelines

2014-10-21 Thread Martin Kletzander 
Signed-off-by: Martin Kletzander mklet...@redhat.com
---
 HACKING  | 18 +++---
 docs/hacking.html.in |  5 +
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/HACKING b/HACKING
index add0841..f8546cb 100644
--- a/HACKING
+++ b/HACKING
@@ -14,7 +14,11 @@ General tips for contributing patches
 (1) Discuss any large changes on the mailing list first. Post patches early and
 listen to feedback.

-(2) Post patches in unified diff format, with git rename detection enabled. You
+(2) Official upstream repository is kept in git 
(git://libvirt.org/libvirt.git)
+and is browsable along with other libvirt-related repositories (e.g.
+libvirt-python) online http://libvirt.org.
+
+(3) Post patches in unified diff format, with git rename detection enabled. You
 need a one-time setup of:

   git config diff.renames true
@@ -66,7 +70,7 @@ though).



-(3) In your commit message, make the summary line reasonably short (60 
characters
+(4) In your commit message, make the summary line reasonably short (60 
characters
 is typical), followed by a blank line, followed by any longer description of
 why your patch makes sense. If the patch fixes a regression, and you know what
 commit introduced the problem, mentioning that is useful. If the patch
@@ -78,7 +82,7 @@ is up to you if you want to include or omit them in the 
commit message.



-(4) Split large changes into a series of smaller patches, self-contained if
+(5) Split large changes into a series of smaller patches, self-contained if
 possible, with an explanation of each patch and an explanation of how the
 sequence of patches fits together. Moreover, please keep in mind that it's
 required to be able to compile cleanly (*including* make check and make
@@ -89,10 +93,10 @@ things).



-(5) Make sure your patches apply against libvirt GIT. Developers only follow 
GIT
+(6) Make sure your patches apply against libvirt GIT. Developers only follow 
GIT
 and don't care much about released versions.

-(6) Run the automated tests on your code before submitting any changes. In
+(7) Run the automated tests on your code before submitting any changes. In
 particular, configure with compile warnings set to -Werror. This is done
 automatically for a git checkout; from a tarball, use:

@@ -138,7 +142,7 @@ various tests under gdb or Valgrind.



-(7) The Valgrind test should produce similar output to make check. If the 
output
+(8) The Valgrind test should produce similar output to make check. If the 
output
 has traces within libvirt API's, then investigation is required in order to
 determine the cause of the issue. Output such as the following indicates some
 sort of leak:
@@ -214,7 +218,7 @@ to tests/.valgrind.supp in order to suppress the warning:



-(8) Update tests and/or documentation, particularly if you are adding a new
+(9) Update tests and/or documentation, particularly if you are adding a new
 feature or changing the output of a program.


diff --git a/docs/hacking.html.in b/docs/hacking.html.in
index 8f2b9d6..4ab0179 100644
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -11,6 +11,11 @@
   liDiscuss any large changes on the mailing list first.  Post patches
 early and listen to feedback./li

+  liOfficial upstream repository is kept in git
+(codegit://libvirt.org/libvirt.git/code) and is browsable
+along with other libvirt-related repositories
+(e.g. libvirt-python) a href=http://libvirt.org;online/a./li
+
   lipPost patches in unified diff format, with git rename
 detection enabled.  You need a one-time setup of:/p
 pre
-- 
2.1.2

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

Re: [libvirt] [PATCH] qemu: x86_64 is good enough for i686

2014-10-21 Thread Martin Kletzander 

On Thu, Oct 16, 2014 at 09:28:00PM +0200, Lubomir Rintel wrote:

virt-manager on Fedora sets up i686 hosts with /usr/bin/qemu-kvm emulator,
which in turn unconditionally execs qemu-system-x86_64 querying capabilities
then fails:

Error launching details: invalid argument: architecture from emulator 'x86_64' 
doesn't match given architecture 'i686'

Traceback (most recent call last):
 File /usr/share/virt-manager/virtManager/engine.py, line 748, in 
_show_vm_helper
   details = self._get_details_dialog(uri, vm.get_connkey())
 File /usr/share/virt-manager/virtManager/engine.py, line 726, in 
_get_details_dialog
   obj = vmmDetails(conn.get_vm(connkey))
 File /usr/share/virt-manager/virtManager/details.py, line 399, in __init__
   self.init_details()
 File /usr/share/virt-manager/virtManager/details.py, line 784, in 
init_details
   domcaps = self.vm.get_domain_capabilities()
 File /usr/share/virt-manager/virtManager/domain.py, line 518, in 
get_domain_capabilities
   self.get_xmlobj().os.machine, self.get_xmlobj().type)
 File /usr/lib/python2.7/site-packages/libvirt.py, line 3492, in 
getDomainCapabilities
   if ret is None: raise libvirtError ('virConnectGetDomainCapabilities() 
failed', conn=self)
libvirtError: invalid argument: architecture from emulator 'x86_64' doesn't 
match given architecture 'i686'

Journal:

Oct 16 21:08:26 goatlord.localdomain libvirtd[1530]: invalid argument: 
architecture from emulator 'x86_64' doesn't match given architecture 'i686'
---
src/qemu/qemu_driver.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7377320..e4b2b6c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17809,7 +17809,8 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn,

arch_from_caps = virQEMUCapsGetArch(qemuCaps);

-if (arch_from_caps != arch) {
+if (arch_from_caps != arch 
+   (arch_from_caps != VIR_ARCH_X86_64 || arch != VIR_ARCH_I686)) {


You haven't ran make syntax-check, otherwise it would tell you there's
tab with 4 spaces after that and we use spaces only.

It would be nice to add a test case for this particular case.  Anyway
ACK, I'll push it in a while with that line fixed.

However, I wonder if we only limit the 32/64 bit machines by the
processor, because it looks like one can run even the following
combination:

 qemu-system-i686 -cpu Haswell

And this is even with type arch='x86_64' machine='pc-q35-2.2'!  Is
this still 64bit cpu running in 32bit mode?

Martin


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

Re: [libvirt] [PATCH] qemu: x86_64 is good enough for i686

2014-10-21 Thread Daniel P. Berrange 
On Tue, Oct 21, 2014 at 01:33:00PM +0200, Martin Kletzander wrote:
 On Thu, Oct 16, 2014 at 09:28:00PM +0200, Lubomir Rintel wrote:
 virt-manager on Fedora sets up i686 hosts with /usr/bin/qemu-kvm emulator,
 which in turn unconditionally execs qemu-system-x86_64 querying capabilities
 then fails:
 
 Error launching details: invalid argument: architecture from emulator 
 'x86_64' doesn't match given architecture 'i686'
 
 Traceback (most recent call last):
  File /usr/share/virt-manager/virtManager/engine.py, line 748, in 
  _show_vm_helper
details = self._get_details_dialog(uri, vm.get_connkey())
  File /usr/share/virt-manager/virtManager/engine.py, line 726, in 
  _get_details_dialog
obj = vmmDetails(conn.get_vm(connkey))
  File /usr/share/virt-manager/virtManager/details.py, line 399, in __init__
self.init_details()
  File /usr/share/virt-manager/virtManager/details.py, line 784, in 
  init_details
domcaps = self.vm.get_domain_capabilities()
  File /usr/share/virt-manager/virtManager/domain.py, line 518, in 
  get_domain_capabilities
self.get_xmlobj().os.machine, self.get_xmlobj().type)
  File /usr/lib/python2.7/site-packages/libvirt.py, line 3492, in 
  getDomainCapabilities
if ret is None: raise libvirtError ('virConnectGetDomainCapabilities() 
  failed', conn=self)
 libvirtError: invalid argument: architecture from emulator 'x86_64' doesn't 
 match given architecture 'i686'
 
 Journal:
 
 Oct 16 21:08:26 goatlord.localdomain libvirtd[1530]: invalid argument: 
 architecture from emulator 'x86_64' doesn't match given architecture 'i686'
 ---
 src/qemu/qemu_driver.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
 
 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index 7377320..e4b2b6c 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -17809,7 +17809,8 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn,
 
 arch_from_caps = virQEMUCapsGetArch(qemuCaps);
 
 -if (arch_from_caps != arch) {
 +if (arch_from_caps != arch 
 +(arch_from_caps != VIR_ARCH_X86_64 || arch != VIR_ARCH_I686)) {
 
 You haven't ran make syntax-check, otherwise it would tell you there's
 tab with 4 spaces after that and we use spaces only.
 
 It would be nice to add a test case for this particular case.  Anyway
 ACK, I'll push it in a while with that line fixed.
 
 However, I wonder if we only limit the 32/64 bit machines by the
 processor, because it looks like one can run even the following
 combination:
 
  qemu-system-i686 -cpu Haswell
 
 And this is even with type arch='x86_64' machine='pc-q35-2.2'!  Is
 this still 64bit cpu running in 32bit mode?

If libvirt / qemu allows you to request type arch=x86_64 when pointing
emulator to the i686 system emulator binary that is a bug IMHO. We
should reject nonsensical combinations like that.

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] docs: Mention repository locations in contributor guidelines

2014-10-21 Thread Peter Krempa 
On 10/21/14 12:25, Martin Kletzander wrote:
 Signed-off-by: Martin Kletzander mklet...@redhat.com
 ---
  HACKING  | 18 +++---
  docs/hacking.html.in |  5 +
  2 files changed, 16 insertions(+), 7 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] qemu: unref cfg after TerminateMachine has been called

2014-10-21 Thread Peter Krempa 
On 10/21/14 12:25, Martin Kletzander wrote:
 Commit 60627f6d added the code that requests driver cfg, but forgot to

Commit ID doesn't exist in libvirt.git

 unref it.
 
 Signed-off-by: Martin Kletzander mklet...@redhat.com
 ---
  src/qemu/qemu_cgroup.c | 2 ++
  1 file changed, 2 insertions(+)
 
 diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
 index fa894c5..b5bdb36 100644
 --- a/src/qemu/qemu_cgroup.c
 +++ b/src/qemu/qemu_cgroup.c
 @@ -1222,6 +1222,8 @@ qemuRemoveCgroup(virQEMUDriverPtr driver,
  VIR_DEBUG(Failed to terminate cgroup for %s, vm-def-name);
  }
 
 +virObjectUnref(cfg);
 +
  return virCgroupRemove(priv-cgroup);
  }
 

ACK with the commit ID tweaked.

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] Add missing delta from Ubuntu to apparmor profiles

2014-10-21 Thread Stefan Bader 
On 20.10.2014 12:48, Stefan Bader wrote:
 On 19.10.2014 17:07, intrigeri wrote:
 Hi Stefan,

 Stefan Bader wrote (19 Oct 2014 11:07:40 GMT) :
 Yeah, I actually did but it felt a bit hackish but then I am told anything 
 looks
 a bit hackish when it involves autoconf. These are again against upstream
 libvirt mostly because the last touch timestamps always clash otherwise.

 Cool, I've tested this. I've imported these two patches in Debian's
 1.2.9-3 quilt series, made the build system use dh-autoreconf (the
 build system in the tarball wants aclocal 1.13, while Debian sid has
 1.14), and added a build-dep on libapparmor-dev to get the needed
 pkg-config file.

 Attempting to build the resulting source package in a clean sid chroot
 fails here:

   Making all in examples/apparmor
   make[3]: Entering directory 
 '/tmp/buildd/libvirt-1.2.9/debian/build/examples/apparmor'
   make[3]: Circular ../../config.h - ../../config.h dependency dropped.
   ./profile-preprocess ../../../../examples/apparmor/libvirt-qemu.in 
 libvirt-qemu
   ./profile-preprocess ../../../../examples/apparmor/libvirt-lxc.in 
 libvirt-lxc
   ./profile-preprocess 
 ../../../../examples/apparmor/usr.lib.libvirt.virt-aa-helper.in 
 usr.lib.libvirt.virt-aa-helper
   ./profile-preprocess ../../../../examples/apparmor/usr.sbin.libvirtd.in 
 usr.sbin.libvirtd
   make[3]: *** No rule to make target 'local-usr.sbin.libvirtd', needed by 
 'all-am'.  Stop.
   make[3]: *** Waiting for unfinished jobs
   /bin/bash: ./profile-preprocess: No such file or directory
   /bin/bash: ./profile-preprocess: No such file or directory
   Makefile:2068: recipe for target 'libvirt-qemu' failed
   make[3]: *** [libvirt-qemu] Error 127
   Makefile:2068: recipe for target 'libvirt-lxc' failed
   make[3]: *** [libvirt-lxc] Error 127
   /bin/bash: ./profile-preprocess: No such file or directory
   /bin/bash: ./profile-preprocess: No such file or directory
   Makefile:2068: recipe for target 'usr.lib.libvirt.virt-aa-helper' failed
   make[3]: *** [usr.lib.libvirt.virt-aa-helper] Error 127
   Makefile:2068: recipe for target 'usr.sbin.libvirtd' failed
   make[3]: *** [usr.sbin.libvirtd] Error 127
   make[3]: Leaving directory 
 '/tmp/buildd/libvirt-1.2.9/debian/build/examples/apparmor'
   Makefile:1979: recipe for target 'all-recursive' failed
   make[2]: *** [all-recursive] Error 1
   make[2]: Leaving directory '/tmp/buildd/libvirt-1.2.9/debian/build'
   Makefile:1877: recipe for target 'all' failed
   make[1]: *** [all] Error 2
   make[1]: Leaving directory '/tmp/buildd/libvirt-1.2.9/debian/build'
   dh_auto_build: make -j5 returned exit code 2
   debian/rules:126: recipe for target 'build' failed
   make: *** [build] Error 2

 Any hint?
 
 Hm, partially this sounds like the preprocess script is not where it should be
 and the other part looks like not finding any local-usr-sbin. Could likely be
 that I need to do something better to make things work in place (as the 
 upstream
 libvirt instructions suggest) as well as with separate object tree (as it is 
 in
 Debian). I also saw something about circular dependency on config.h which
 probably slipped my attention. For most of the problems I guess adding 
 something
 like $(srcdir) (need to look what this would be actually called) to the
 pre-process scripts path as well as to the .in files..

Turns out that this first attempt was not too good at all. First it does not
help to mis-name the new local .in file. Then, using the wildcard form actually
causes many more files to be touched than intended (the circular reference
hinted that). Lastly I found it might be good to also do something about 
cleanup.
Hope this version works better in general.

-Stefan

From 3715e3a3aa29543e38afc6ec97296866b2977e11 Mon Sep 17 00:00:00 2001
From: Stefan Bader stefan.ba...@canonical.com
Date: Mon, 13 Oct 2014 11:31:59 +0200
Subject: [PATCH 1/2] examples/apparmor: Add ability to add versioned features

Adds APPARMOR_VERSION_NUMBER to config.h which by default is set to the
apparmor library version (major*1000+minor). It can be overriden by
the distro by supplyig --with-apparmor-profiles-version=version.

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 configure.ac   |  22 
 examples/apparmor/Makefile.am  |  18 +++
 examples/apparmor/libvirt-lxc  | 116 -
 examples/apparmor/libvirt-lxc.in   | 116 +
 examples/apparmor/libvirt-qemu | 144 -
 examples/apparmor/libvirt-qemu.in  | 144 +
 examples/apparmor/profile-preprocess   |  21 +++
 examples/apparmor/usr.lib.libvirt.virt-aa-helper   |  48 ---
 .../apparmor/usr.lib.libvirt.virt-aa-helper.in |  48 +++
 examples/apparmor/usr.sbin.libvirtd|  63 -
 examples/apparmor/usr.sbin.libvirtd.in |  63 +
 11 files changed,

Re: [libvirt] [PATCH] qemu: unref cfg after TerminateMachine has been called

2014-10-21 Thread Martin Kletzander 

On Tue, Oct 21, 2014 at 01:49:37PM +0200, Peter Krempa wrote:

On 10/21/14 12:25, Martin Kletzander wrote:

Commit 60627f6d added the code that requests driver cfg, but forgot to


Commit ID doesn't exist in libvirt.git



Yep, my fault, it's 4882618ed13b469d92fa8b2b4a158fdb17dbe9f1, thanks
for findinding that out ;)


unref it.

Signed-off-by: Martin Kletzander mklet...@redhat.com
---
 src/qemu/qemu_cgroup.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index fa894c5..b5bdb36 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -1222,6 +1222,8 @@ qemuRemoveCgroup(virQEMUDriverPtr driver,
 VIR_DEBUG(Failed to terminate cgroup for %s, vm-def-name);
 }

+virObjectUnref(cfg);
+
 return virCgroupRemove(priv-cgroup);
 }



ACK with the commit ID tweaked.

Peter






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


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

Re: [libvirt] [libvirt-python PATCH] Fix cannot use VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS flags in domainListGetStats

2014-10-21 Thread Peter Krempa 
On 10/21/14 11:04, Luyao Huang wrote:
 When set flags=libvirt.VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, 
 python will report
 a  error:
 OverflowError: signed integer is greater than maximum
 
 Because VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS = 2147483648 (2**31), 
 and it set a
 signed int in PyArg_ParseTuple function.
 
 if (!PyArg_ParseTuple(args, (char *)OOii:virDomainListGetStats,
   pyobj_conn, py_domlist, stats, flags))
 
 When python = 2.3, 'I' means unsigned int and 'i' means int,so there should 
 use 'I'.
 
 From: https://docs.python.org/2/c-api/arg.html
 
 I also found a lot of function use 'i' for a unsigned int, but i didn't 
 change them.

Few too long lines and unclear sentences in the commit message.

 
 Signed-off-by: Luyao Huang lhu...@redhat.com
 ---
  libvirt-override.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/libvirt-override.c b/libvirt-override.c
 index c887b71..6dacdac 100644
 --- a/libvirt-override.c
 +++ b/libvirt-override.c
 @@ -8126,7 +8126,7 @@ libvirt_virDomainListGetStats(PyObject *self 
 ATTRIBUTE_UNUSED,
  unsigned int flags;
  unsigned int stats;
  
 -if (!PyArg_ParseTuple(args, (char *)OOii:virDomainListGetStats,
 +if (!PyArg_ParseTuple(args, (char *)OOII:virDomainListGetStats,
pyobj_conn, py_domlist, stats, flags))
  return NULL;
  
 

ACK, I'll clean up the commit message before pushing.

Peter



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

[libvirt] [PATCH v2] Fix cast errors with clang

2014-10-21 Thread Roman Bogorodskiy 
Build with clang fails with:

  CC   util/libvirt_util_la-virsocketaddr.lo
util/virsocketaddr.c:904:17: error: cast from 'struct sockaddr *' to
'struct sockaddr_in *' increases required alignment from 1 to 4
[-Werror,-Wcast-align]
inet4 = (struct sockaddr_in*) res-ai_addr;
^~
util/virsocketaddr.c:909:17: error: cast from 'struct sockaddr *' to
'struct sockaddr_in6 *' increases required alignment from 1 to 4
[-Werror,-Wcast-align]
inet6 = (struct sockaddr_in6*) res-ai_addr;
^~~
2 errors generated.

Fix that by replacing virSocketAddrParseInternal() call with
virSocketAddrParse() in the virSocketAddrIsNumericLocalhost() function.
virSocketAddrParse stores an address in virSocketAddr.
virSocketAddr uses a union to store an address, so it doesn't
need casting.
---
 src/util/virsocketaddr.c | 20 ++--
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
index 5f54e68..da0a9f1 100644
--- a/src/util/virsocketaddr.c
+++ b/src/util/virsocketaddr.c
@@ -890,28 +890,20 @@ virSocketAddrNumericFamily(const char *address)
 bool
 virSocketAddrIsNumericLocalhost(const char *addr)
 {
-struct addrinfo *res;
+virSocketAddr res;
 struct in_addr tmp = { .s_addr = htonl(INADDR_LOOPBACK) };
-struct sockaddr_in *inet4;
-struct sockaddr_in6 *inet6;
 bool ret = false;
 
-if (virSocketAddrParseInternal(res, addr, AF_UNSPEC, false)  0)
+if (virSocketAddrParse(res, addr, AF_UNSPEC)  0)
 return ret;
 
-switch (res-ai_addr-sa_family) {
+switch (res.data.stor.ss_family) {
 case AF_INET:
-inet4 = (struct sockaddr_in*) res-ai_addr;
-ret = memcmp(inet4-sin_addr.s_addr, tmp.s_addr,
- sizeof(inet4-sin_addr.s_addr)) == 0;
-break;
+return memcmp(res.data.inet4.sin_addr.s_addr, tmp.s_addr,
+ sizeof(res.data.inet4.sin_addr.s_addr)) == 0;
 case AF_INET6:
-inet6 = (struct sockaddr_in6*) res-ai_addr;
-ret = IN6_IS_ADDR_LOOPBACK((inet6-sin6_addr));
-break;
+return IN6_IS_ADDR_LOOPBACK(res.data.inet6.sin6_addr);
 }
 
-freeaddrinfo(res);
 return ret;
-
 }
-- 
2.0.2

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

Re: [libvirt] [PATCH] Fix cast errors with clang

2014-10-21 Thread Roman Bogorodskiy 
  Eric Blake wrote:

 On 10/18/2014 10:41 AM, Roman Bogorodskiy wrote:
  Build with clang fails with:
  
CC   util/libvirt_util_la-virsocketaddr.lo
  util/virsocketaddr.c:904:17: error: cast from 'struct sockaddr *' to
  'struct sockaddr_in *' increases required alignment from 1 to 4
  [-Werror,-Wcast-align]
  inet4 = (struct sockaddr_in*) res-ai_addr;
  ^~
  util/virsocketaddr.c:909:17: error: cast from 'struct sockaddr *' to
  'struct sockaddr_in6 *' increases required alignment from 1 to 4
  [-Werror,-Wcast-align]
  inet6 = (struct sockaddr_in6*) res-ai_addr;
  ^~~
  2 errors generated.
  
  Fix by introducing a union of the appropriate sturcts.
 
 s/sturcts/structs/
 
  ---
   src/util/virsocketaddr.c | 17 ++---
   1 file changed, 10 insertions(+), 7 deletions(-)
  
  diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
  index 5f54e68..162108c 100644
  --- a/src/util/virsocketaddr.c
  +++ b/src/util/virsocketaddr.c
  @@ -892,22 +892,25 @@ virSocketAddrIsNumericLocalhost(const char *addr)
   {
   struct addrinfo *res;
   struct in_addr tmp = { .s_addr = htonl(INADDR_LOOPBACK) };
  -struct sockaddr_in *inet4;
  -struct sockaddr_in6 *inet6;
  +union {
  +struct sockaddr *addr;
  +struct sockaddr_in *inet4;
  +struct sockaddr_in6 *inet6;
  +} sa;
 
 Close, but not quite.  The POSIX solution for this is the
 sockaddr_storage type, in sys/socket.h.  You shouldn't need to create
 your own union, but instead reuse sockaddr_storage.

Casting from sockaddr to sockaddr_storage still produces an error with
clang. However, I was reading the related code and noticed a similar
union in the virSocketAddr type, so I decided to use that instead; that
also allowed to reduce size of the code a little.

Roman Bogorodskiy


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

[libvirt] [python PATCH 2/3] Fix parsing of 'flags' argument for bulk stats functions

2014-10-21 Thread Peter Krempa 
From: Luyao Huang lhu...@redhat.com

When 'flags' is set to
'libvirt.VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS,
python will report a  error:

OverflowError: signed integer is greater than maximum

as VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS is defined as 131.
This happens as PyArg_ParseTuple's formatting string containing 'i' as a
modifier expects a signed integer.

With python = 2.3, 'I' means unsigned int and 'i' means int so we
should use 'I' in the formatting string.

See: https://docs.python.org/2/c-api/arg.html

Signed-off-by: Luyao Huang lhu...@redhat.com
Signed-off-by: Peter Krempa pkre...@redhat.com
---
 libvirt-override.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index b4bb571..84f281a 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -8090,7 +8090,7 @@ libvirt_virConnectGetAllDomainStats(PyObject *self 
ATTRIBUTE_UNUSED,
 unsigned int flags;
 unsigned int stats;

-if (!PyArg_ParseTuple(args, (char *)Oii:virConnectGetAllDomainStats,
+if (!PyArg_ParseTuple(args, (char *)OII:virConnectGetAllDomainStats,
   pyobj_conn, stats, flags))
 return NULL;
 conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn);
@@ -8126,7 +8126,7 @@ libvirt_virDomainListGetStats(PyObject *self 
ATTRIBUTE_UNUSED,
 unsigned int flags;
 unsigned int stats;

-if (!PyArg_ParseTuple(args, (char *)OOii:virDomainListGetStats,
+if (!PyArg_ParseTuple(args, (char *)OOII:virDomainListGetStats,
   pyobj_conn, py_domlist, stats, flags))
 return NULL;

-- 
2.1.0

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

[libvirt] [python PATCH 3/3] Fix rest of unsigned integer handling

2014-10-21 Thread Peter Krempa 
As in the previous patch, fix all places where 'flags' is converted as a
signed argument to unsigned including the python code generator.
---
 generator.py|   2 +-
 libvirt-lxc-override.c  |   2 +-
 libvirt-override.c  | 128 
 libvirt-qemu-override.c |   4 +-
 4 files changed, 68 insertions(+), 68 deletions(-)

diff --git a/generator.py b/generator.py
index e8e29b9..01ab441 100755
--- a/generator.py
+++ b/generator.py
@@ -292,7 +292,7 @@ py_types = {
 'int':  ('i', None, int, int),
 'long':  ('l', None, long, long),
 'double':  ('d', None, double, double),
-'unsigned int':  ('i', None, int, int),
+'unsigned int':  ('I', None, int, int),
 'unsigned long':  ('l', None, long, long),
 'long long':  ('L', None, longlong, long long),
 'unsigned long long':  ('L', None, longlong, long long),
diff --git a/libvirt-lxc-override.c b/libvirt-lxc-override.c
index ba97551..d0d9ffd 100644
--- a/libvirt-lxc-override.c
+++ b/libvirt-lxc-override.c
@@ -71,7 +71,7 @@ libvirt_lxc_virDomainLxcOpenNamespace(PyObject *self 
ATTRIBUTE_UNUSED,
 int *fdlist = NULL;
 size_t i;

-if (!PyArg_ParseTuple(args, (char *)Oi:virDomainLxcOpenNamespace,
+if (!PyArg_ParseTuple(args, (char *)OI:virDomainLxcOpenNamespace,
   pyobj_domain, flags))
 return NULL;
 domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
diff --git a/libvirt-override.c b/libvirt-override.c
index 84f281a..a88f137 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -521,7 +521,7 @@ libvirt_virDomainBlockStatsFlags(PyObject *self 
ATTRIBUTE_UNUSED,
 virTypedParameterPtr params;
 const char *path;

-if (!PyArg_ParseTuple(args, (char *)Ozi:virDomainBlockStatsFlags,
+if (!PyArg_ParseTuple(args, (char *)OzI:virDomainBlockStatsFlags,
   pyobj_domain, path, flags))
 return NULL;
 domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
@@ -571,7 +571,7 @@ libvirt_virDomainGetCPUStats(PyObject *self 
ATTRIBUTE_UNUSED, PyObject *args)
 bool totalflag;
 virTypedParameterPtr params = NULL, cpuparams;

-if (!PyArg_ParseTuple(args, (char *)OOi:virDomainGetCPUStats,
+if (!PyArg_ParseTuple(args, (char *)OOI:virDomainGetCPUStats,
   pyobj_domain, totalbool, flags))
 return NULL;
 domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
@@ -888,7 +888,7 @@ libvirt_virDomainGetSchedulerParametersFlags(PyObject *self 
ATTRIBUTE_UNUSED,
 unsigned int flags;
 virTypedParameterPtr params;

-if (!PyArg_ParseTuple(args, (char 
*)Oi:virDomainGetScedulerParametersFlags,
+if (!PyArg_ParseTuple(args, (char 
*)OI:virDomainGetScedulerParametersFlags,
   pyobj_domain, flags))
 return NULL;
 domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
@@ -1012,7 +1012,7 @@ libvirt_virDomainSetSchedulerParametersFlags(PyObject 
*self ATTRIBUTE_UNUSED,
 virTypedParameterPtr params, new_params = NULL;

 if (!PyArg_ParseTuple(args,
-  (char *)OOi:virDomainSetScedulerParametersFlags,
+  (char *)OOI:virDomainSetScedulerParametersFlags,
   pyobj_domain, info, flags))
 return NULL;
 domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
@@ -1087,7 +1087,7 @@ libvirt_virDomainSetBlkioParameters(PyObject *self 
ATTRIBUTE_UNUSED,
 virTypedParameterPtr params = NULL, new_params = NULL;

 if (!PyArg_ParseTuple(args,
-  (char *)OOi:virDomainSetBlkioParameters,
+  (char *)OOI:virDomainSetBlkioParameters,
   pyobj_domain, info, flags))
 return NULL;
 domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
@@ -1159,7 +1159,7 @@ libvirt_virDomainGetBlkioParameters(PyObject *self 
ATTRIBUTE_UNUSED,
 unsigned int flags;
 virTypedParameterPtr params;

-if (!PyArg_ParseTuple(args, (char *)Oi:virDomainGetBlkioParameters,
+if (!PyArg_ParseTuple(args, (char *)OI:virDomainGetBlkioParameters,
   pyobj_domain, flags))
 return NULL;
 domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
@@ -1207,7 +1207,7 @@ libvirt_virDomainSetMemoryParameters(PyObject *self 
ATTRIBUTE_UNUSED,
 virTypedParameterPtr params = NULL, new_params = NULL;

 if (!PyArg_ParseTuple(args,
-  (char *)OOi:virDomainSetMemoryParameters,
+  (char *)OOI:virDomainSetMemoryParameters,
   pyobj_domain, info, flags))
 return NULL;
 domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
@@ -1279,7 +1279,7 @@ libvirt_virDomainGetMemoryParameters(PyObject *self 
ATTRIBUTE_UNUSED,
 unsigned int flags;
 virTypedParameterPtr params;

-if (!PyArg_ParseTuple(args, (char *)Oi:virDomainGetMemoryParameters,
+if (!PyArg_ParseTuple(args, (char

[libvirt] [python PATCH 1/3] Fix function name when parsing arguments in libvirt_virNodeAllocPages

2014-10-21 Thread Peter Krempa 
The override function was copiedpasted from virConnectGetAllDomainStats
and the function name after the collon was not changed. Fix the issue as
an invalid name would appear in the error message.
---
 libvirt-override.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index c887b71..b4bb571 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -8214,7 +8214,7 @@ libvirt_virNodeAllocPages(PyObject *self ATTRIBUTE_UNUSED,
 unsigned int flags = VIR_NODE_ALLOC_PAGES_ADD;
 int c_retval;

-if (!PyArg_ParseTuple(args, (char *)OOiii:virConnectGetAllDomainStats,
+if (!PyArg_ParseTuple(args, (char *)OOiii:virNodeAllocPages,
   pyobj_conn, pyobj_pages,
   startCell, cellCount, flags))
 return NULL;
-- 
2.1.0

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

[libvirt] [python PATCH 0/3] Fix multiple python binding issues

2014-10-21 Thread Peter Krempa 
Luyao Huang (1):
  Fix parsing of 'flags' argument for bulk stats functions

Peter Krempa (2):
  Fix function name when parsing arguments in libvirt_virNodeAllocPages
  Fix rest of unsigned integer handling

 generator.py|   2 +-
 libvirt-lxc-override.c  |   2 +-
 libvirt-override.c  | 132 
 libvirt-qemu-override.c |   4 +-
 4 files changed, 70 insertions(+), 70 deletions(-)

-- 
2.1.0

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

Re: [libvirt] [PATCH] Add support for /run/initctl

2014-10-21 Thread Rick Harris 
Hi all,

A few weeks ago I proposed a patch to fix LXC shutdown for distros that use
`/run/initctl` instead of `/dev/initctl`.

Replying here so that it gets some more visibility.

Would love some feedback here: are people happy with the general approach?

This is my first libvirt patch: is the style/code quality up to snuff?

Thanks all!

-Rick

On Tue, Sep 30, 2014 at 12:01 PM, Rick Harris rconradhar...@gmail.com
wrote:

 Newer versions of Debian use `/run/initctl` instead of `/dev/initctl`. This
 patch updates the code to search for the FIFO from a list of well-known
 locations.

 In the FreeBSD case, as before, we fall-back to the `/etc/.initctl` stub.
 ---
  src/util/virinitctl.c | 38 --
  1 file changed, 24 insertions(+), 14 deletions(-)

 diff --git a/src/util/virinitctl.c b/src/util/virinitctl.c
 index a6fda3b..c4b48d4 100644
 --- a/src/util/virinitctl.c
 +++ b/src/util/virinitctl.c
 @@ -46,12 +46,6 @@
   *  Copyright (C) 1995-2004 Miquel van Smoorenburg
   */

 -# if defined(__FreeBSD_kernel__)
 -#  define VIR_INITCTL_FIFO  /etc/.initctl
 -# else
 -#  define VIR_INITCTL_FIFO  /dev/initctl
 -# endif
 -
  # define VIR_INITCTL_MAGIC 0x03091969
  # define VIR_INITCTL_CMD_START  0
  # define VIR_INITCTL_CMD_RUNLVL 1
 @@ -124,6 +118,13 @@ virInitctlSetRunLevel(virInitctlRunLevel level)
  struct virInitctlRequest req;
  int fd = -1;
  int ret = -1;
 +const char *initctl_fifo = NULL;
 +size_t i = 0;
 +const char *initctl_fifos[] = {
 +/run/initctl,
 +/dev/initctl,
 +/etc/.initctl,
 +};

  memset(req, 0, sizeof(req));

 @@ -133,22 +134,31 @@ virInitctlSetRunLevel(virInitctlRunLevel level)
  /* Yes it is an 'int' field, but wants a numeric character. Go figure
 */
  req.runlevel = '0' + level;

 -if ((fd = open(VIR_INITCTL_FIFO,
 -   O_WRONLY|O_NONBLOCK|O_CLOEXEC|O_NOCTTY))  0) {
 -if (errno == ENOENT) {
 -ret = 0;
 +for (i = 0; i  ARRAY_CARDINALITY(initctl_fifos); i++) {
 +initctl_fifo = initctl_fifos[i];
 +
 +if ((fd = open(initctl_fifo,
 +   O_WRONLY|O_NONBLOCK|O_CLOEXEC|O_NOCTTY)) = 0)
 +break;
 +
 +if (errno != ENOENT) {
 +virReportSystemError(errno,
 + _(Cannot open init control %s),
 + initctl_fifo);
  goto cleanup;
  }
 -virReportSystemError(errno,
 - _(Cannot open init control %s),
 - VIR_INITCTL_FIFO);
 +}
 +
 +/* Ensure we found a valid initctl fifo */
 +if (fd  0) {
 +ret = 0;
  goto cleanup;
  }

  if (safewrite(fd, req, sizeof(req)) != sizeof(req)) {
  virReportSystemError(errno,
   _(Failed to send request to init control
 %s),
 - VIR_INITCTL_FIFO);
 + initctl_fifo);
  goto cleanup;
  }

 --
 1.9.1


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

Re: [libvirt] [PATCH v4] network: Add bandwidth support to ethernet interfaces

2014-10-21 Thread Anirban Chakraborty 


On 10/10/14, 3:23 PM, Anirban Chakraborty abc...@juniper.net wrote:

v4:
Changed function virNetDevSupportBandwidth to use switch statement.
Fixed syntax issues
Fold the two patches into one as the second patch was a one liner

v3:
Addressed issues pointed out in V2
Split into two patches

v2:
Addressed comments raised in review of V1.
Consolidate calls to virNetDevBandwidthSet.
Clear bandwidth settings when the interface is detached or domain
destroyed.

v1:
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.

interface type=ethernet
  mac address=02:36:1d:18:2a:e4/
  model type=virtio/
  script path=/
  target dev=tap361d182a-e4/
  bandwidth
inbound average=984 peak=1024 burst=64/
outbound average=2000 peak=2048 burst=128/
  /bandwidth
/interface

Signed-off-by: Anirban Chakraborty abc...@juniper.net

Ping.

---
 src/conf/domain_conf.h  | 21 +
 src/lxc/lxc_driver.c|  3 +++
 src/lxc/lxc_process.c   | 18 +-
 src/qemu/qemu_command.c | 25 +++--
 src/qemu/qemu_command.h |  2 ++
 src/qemu/qemu_driver.c  |  3 +++
 src/qemu/qemu_hotplug.c | 12 
 src/qemu/qemu_process.c |  3 +++
 src/util/virnetdevmacvlan.c | 10 --
 src/util/virnetdevmacvlan.h |  1 -
 10 files changed, 72 insertions(+), 26 deletions(-)

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index afa3da6..59fdfb2 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2848,4 +2848,25 @@ int virDomainObjSetMetadata(virDomainObjPtr vm,
 bool virDomainDefNeedsPlacementAdvice(virDomainDefPtr def)
 ATTRIBUTE_NONNULL(1);

+static inline bool virNetDevSupportBandwidth(virDomainNetType type)
+{
+switch (type) {
+case VIR_DOMAIN_NET_TYPE_BRIDGE:
+case VIR_DOMAIN_NET_TYPE_NETWORK:
+case VIR_DOMAIN_NET_TYPE_DIRECT:
+case VIR_DOMAIN_NET_TYPE_ETHERNET:
+return true;
+case VIR_DOMAIN_NET_TYPE_USER:
+case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
+case VIR_DOMAIN_NET_TYPE_SERVER:
+case VIR_DOMAIN_NET_TYPE_CLIENT:
+case VIR_DOMAIN_NET_TYPE_MCAST:
+case VIR_DOMAIN_NET_TYPE_INTERNAL:
+case VIR_DOMAIN_NET_TYPE_HOSTDEV:
+case VIR_DOMAIN_NET_TYPE_LAST:
+break;
+}
+return false;
+}
+
 #endif /* __DOMAIN_CONF_H */
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index b3e506f..a6f1f8a 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -72,6 +72,7 @@
 #include viraccessapicheck.h
 #include viraccessapichecklxc.h
 #include virhostdev.h
+#include qemu/qemu_command.h

 #define VIR_FROM_THIS VIR_FROM_LXC

@@ -4634,6 +4635,8 @@ lxcDomainDetachDeviceNetLive(virDomainObjPtr vm,

 detach = vm-def-nets[detachidx];

+qemuDomainClearNetBandwidth(vm);
+
 switch (virDomainNetGetActualType(detach)) {
 case VIR_DOMAIN_NET_TYPE_BRIDGE:
 case VIR_DOMAIN_NET_TYPE_NETWORK:
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index ed30c37..3192011 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -274,11 +274,6 @@ char
*virLXCProcessSetupInterfaceBridged(virConnectPtr conn,
 if (virNetDevSetOnline(parentVeth, true)  0)
 goto cleanup;

-if (virNetDevBandwidthSet(net-ifname,
-  virDomainNetGetActualBandwidth(net),
-  false)  0)
-goto cleanup;
-
 if (net-filter 
 virDomainConfNWFilterInstantiate(conn, vm-uuid, net)  0)
 goto cleanup;
@@ -300,6 +295,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr
conn,
 virNetDevBandwidthPtr bw;
 virNetDevVPortProfilePtr prof;
 virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
+const char *linkdev = virDomainNetGetActualDirectDev(net);

 /* XXX how todo bandwidth controls ?
  * Since the 'net-ifname' is about to be moved to a different
@@ -329,14 +325,13 @@ char
*virLXCProcessSetupInterfaceDirect(virConnectPtr conn,

 if (virNetDevMacVLanCreateWithVPortProfile(
 net-ifname, net-mac,
-virDomainNetGetActualDirectDev(net),
+linkdev,
 virDomainNetGetActualDirectMode(net),
 false, def-uuid,
-virDomainNetGetActualVirtPortProfile(net),
+prof,
 res_ifname,
 VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
-cfg-stateDir,
-virDomainNetGetActualBandwidth(net), 0)  0)
+cfg-stateDir, 0)  0)
 goto cleanup;

 ret = res_ifname;
@@ -450,6 +445,11 @@ static int
virLXCProcessSetupInterfaces(virConnectPtr conn,
 goto cleanup;
 }

+/* set network bandwidth */
+if (virNetDevBandwidthSet(def-nets[i]-ifname,
+virDomainNetGetActualBandwidth(def-nets[i]), false)  0)
+   goto cleanup;
+

Re: [libvirt] Close the BlockDriverState when guest eject the media

2014-10-21 Thread Weidong Huang 
On 2014/10/20 19:39, Kevin Wolf wrote:

 Am 20.10.2014 um 13:27 hat Weidong Huang geschrieben:
 On 2014/10/20 17:41, Kevin Wolf wrote:

 Am 18.10.2014 um 12:02 hat Weidong Huang geschrieben:
 Hi ALL:

 There are two ways to eject the cdrom tray. One is by the eject's qmp 
 commmand(eject_device).
 The another one is by the guest(bdrv_eject). They have different results.

 Yes, they are different things.

 If a guest opens the tray (using bdrv_eject) and then closes it again,
 with no user interaction in between, the virtual media must still be in
 the drive and the guest must be able to access the same image again.
 Calling bdrv_close() in this case would be a bug.

 The goal of the monitor command eject on the other hand is to remove
 the medium so that the drive is empty. That a device with a closed tray
 has to be opened for this is only secondary.


 Thanks for your reply.

 There is a problem.

 1. Qemu receive the eject command.
 2. Runs eject_request_cb when an eject request is issued from the monitor, 
 the tray
 is closed, and the medium is locked. But the drive is not closed.
 3. Guest agree with opening tray and qemu will call bdrv_eject to complete. 
 The drive is
 still not close.

 So the result of the monitor command eject is not to remove the medium in 
 this situation.
 
 Now I understand, thanks for explaining.
 
 But I think libvirt can actually work correctly with what qemu offers
 today. qemu returns an error if the medium cannot be removed with the
 'eject' command and it only sends an eject request to the guest.
 
 With this error, libvirt can know that the DEVICE_TRAY_MOVED event
 doesn't mean that the medium has removed, but that it needs to issue
 another 'eject' command.
 
 If this isn't implemented correctly in libvirt today, this needs a
 libvirt fix rather than a qemu one.
 


hi all!

How about fix it in libvirt?

 eject_device: close the BlockDriverState(bdrv_close(bs))
 bdrv_eject: don't close the BlockDriverState,

 This is ambiguous. So libvirt can't handle some situations.

 libvirt send eject qmp command --- qemu send eject request to guest ---
 guest respond to qemu --- qemu emit tray_open event to libvirt ---
 libvirt will not send change qmp command if media source is null. So
 the media is not be replace to the null.

 What is the problem that libvirt has with the guest opening the tray? I
 don't think libvirt should even care about that case.


 For example, using libvirt to change media by xml below(media source is 
 null):
 disk type='file' device='cdrom'
 driver name='qemu'/
 target dev='hdb' bus='ide'/
 /disk

 libivrt return ok. But media still is in the guest.
 This is confused.
 
 Kevin
 
 .
 



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

Re: [libvirt] [PATCH v2] Fix cast errors with clang

2014-10-21 Thread Eric Blake 
On 10/21/2014 08:22 AM, Roman Bogorodskiy wrote:
 Build with clang fails with:
 
   CC   util/libvirt_util_la-virsocketaddr.lo
 util/virsocketaddr.c:904:17: error: cast from 'struct sockaddr *' to
 'struct sockaddr_in *' increases required alignment from 1 to 4
 [-Werror,-Wcast-align]
 inet4 = (struct sockaddr_in*) res-ai_addr;
 ^~
 util/virsocketaddr.c:909:17: error: cast from 'struct sockaddr *' to
 'struct sockaddr_in6 *' increases required alignment from 1 to 4
 [-Werror,-Wcast-align]
 inet6 = (struct sockaddr_in6*) res-ai_addr;
 ^~~
 2 errors generated.
 
 Fix that by replacing virSocketAddrParseInternal() call with
 virSocketAddrParse() in the virSocketAddrIsNumericLocalhost() function.
 virSocketAddrParse stores an address in virSocketAddr.
 virSocketAddr uses a union to store an address, so it doesn't
 need casting.
 ---
  src/util/virsocketaddr.c | 20 ++--
  1 file changed, 6 insertions(+), 14 deletions(-)
 

  bool ret = false;

You could drop this...

  
 -if (virSocketAddrParseInternal(res, addr, AF_UNSPEC, false)  0)
 +if (virSocketAddrParse(res, addr, AF_UNSPEC)  0)
  return ret;

by making this 'return false'

  
 -switch (res-ai_addr-sa_family) {
 +switch (res.data.stor.ss_family) {
  case AF_INET:
 -inet4 = (struct sockaddr_in*) res-ai_addr;
 -ret = memcmp(inet4-sin_addr.s_addr, tmp.s_addr,
 - sizeof(inet4-sin_addr.s_addr)) == 0;
 -break;
 +return memcmp(res.data.inet4.sin_addr.s_addr, tmp.s_addr,
 + sizeof(res.data.inet4.sin_addr.s_addr)) == 0;
  case AF_INET6:
 -inet6 = (struct sockaddr_in6*) res-ai_addr;
 -ret = IN6_IS_ADDR_LOOPBACK((inet6-sin6_addr));
 -break;
 +return IN6_IS_ADDR_LOOPBACK(res.data.inet6.sin6_addr);
  }
  
 -freeaddrinfo(res);
  return ret;

and also this one. (ret is never assigned anything else in the
function).  Whether or not you make that additional cleanup,

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] [Qemu-devel] spec, RFC: TLS support for NBD

2014-10-21 Thread Wouter Verhelst 
On Tue, Oct 21, 2014 at 10:35:06AM +0100, Daniel P. Berrange wrote:
 On Tue, Oct 21, 2014 at 12:10:39AM +0200, Wouter Verhelst wrote:
  On Mon, Oct 20, 2014 at 01:56:43PM +0200, Florian Weimer wrote:
   I cannot comment on whether the proposed STARTTLS command is at the 
   correct
   stage of the NBD protocol.  If there is a protocol description for NBD, I
   can have a look.
  
  To make this discussion in that regard a bit easier, I've committed the
  proposed spec to a separate branch:
  
  https://github.com/yoe/nbd/blob/tlsspec/doc/proto.txt
 
 So, if I'm understanding correctly the new style fixed handshake
 currently works like this:
 
 Server starts transmission with version info
 
   - S: NBDMAGIC
   - S: 0x49484156454F5054
   - S: 0x1
 
 And the client acknowledges with a 32 bits of and first bit set
 
   - C: 0x1
 
 
 Now the client has to request one or more options, of which the
 NBD_OPT_EXPORT_NAME (0x1) option is mandatory, so it will be the
 first thing the client sends next.

No, on the contrary. NBD_OPT_EXPORT_NAME _must_ be the very last thing the
client sends if the client wants to actually select an export to be
used. This is because for historical reasons, NBD_OPT_EXPORT_NAME can't
get an NBD_REP_ACK, but must immediately introduce the next phase of the
protocol (where data is flowing across the channel).

It should maybe be renamed to something like NBD_OPT_SELECT_AND_GO or
some such, but I'm not sure that makes much sense.

 So it would send
 
   - C: 0x49484156454F5054
   - C: 0x1
   - C: 0xa  (length of name)
   - C: volumename
 
 
 
 You're proposing to add a new option NBD_OPT_STARTTLS (0x5). For this to
 work we would need to update the language to say that the NBD_OPT_STARTTLS
 must be the first option that the client sends to the server, because we
 want the option name to transmit in ciphertext.

Yes, my proposed protocol allows that.

 So the new protocol startup would be
 
 Server starts transmission with version info
 
   - S: NBDMAGIC
   - S: 0x49484156454F5054
   - S: 0x1
 
 And the client acknowledges with a 32 bits of zero
 
   - C: 0x1
 
 Client requests TLS
 
   - C: 0x49484156454F5054
   - C: 0x5
 
 Server acknowledges TLS:
 
   - S: 0x3e889045565a9
   - S: 0x5
   - S: 0x1 (REP_ACK)
   - S: 0x0
 
 Now the TLS handshake is performed by client + server
 
 
 
 The client can now set other options using the secure
 channel, of which the NBD_OPT_EXPORT_NAME (0x1) option
 is mandatory, so it will be the first thing the client

(last thing)

 sends next. So they would send
 
   - C: 0x49484156454F5054
   - C: 0x1
   - C: 0xa  (length of name)
   - C: volumename
 
 etc...
 
 
 This is secure when both client and server want to use TLS.
 
 This is secure when the client wants TLS and the server does
 not want TLS, because the server will reject TLS and the client
 will then close the connection.
 
 My concern is the case where the client does not want TLS but
 the server does want TLS. In this case the client will immediately
 send the NBD_OPT_EXPORT_NAME in clear text over the wire, not giving
 the server any chance to reject the use of a clear text channel.

In that case (if the client doesn't want TLS and also doesn't want to
negotiate anything else apart from a name), the server should disconnect
(anything else will fail to communicate with older clients).

 This problem is inherant to the approach of using the NBD protocol
 options to negotiate TLS.

True.

 One way I see to solve that insecurity, would be for the server
 to make use of one of the extra reserved bits in the very first
 message it sends. ie we need to negotiate TLS immediately after the
 version number / magic acknowledgment, before we actually start
 the main body of the protocol
 
 ie, with the new style fixed handshake the server should send
 
 Server starts transmission with version info
 
   - S: NBDMAGIC
   - S: 0x49484156454F5054
   - S: 0x2 

That would be 0x4, because 0x2 is already used for something else (yes,
yes, details).

 And the client acknowledges with a 32bits and first two bits set
 
   - C: 0x2
 
 The questionmark here is what happens when the client sees the
 second bit of reserved field set ? 
 
 The protocol docs merely say
 
   S: 16 bits of zero (bits 1-15 reserved for future use; bit 0 in use to
signal fixed newstyle (see below))
 
 But don't mention what clients should do upon seeing unknown bits
 set in the server's message ?

If the client doesn't send the bit you proposed, then the server would
know it doesn't understand the TLS variant of the protocol and would
then either fall back to unencrypted or drop the connection (depending
on policy).

I'm not sure using a separate bit for this is necessary, though. The
server is supposed to send NBD_REP_ERR_UNSUP for messages it doesn't
understand, so a client that talks to a server which doesn't support the
STARTTLS message should be able to communicate just fine (if client
policy allows unencrypted communication, yada

Re: [libvirt] [python PATCH 2/3] Fix parsing of 'flags' argument for bulk stats functions

2014-10-21 Thread Eric Blake 
On 10/21/2014 08:34 AM, Peter Krempa wrote:
 From: Luyao Huang lhu...@redhat.com
 
 When 'flags' is set to
 'libvirt.VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS,
 python will report a  error:
 
 OverflowError: signed integer is greater than maximum
 
 as VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS is defined as 131.
 This happens as PyArg_ParseTuple's formatting string containing 'i' as a
 modifier expects a signed integer.
 
 With python = 2.3, 'I' means unsigned int and 'i' means int so we
 should use 'I' in the formatting string.
 
 See: https://docs.python.org/2/c-api/arg.html
 
 Signed-off-by: Luyao Huang lhu...@redhat.com
 Signed-off-by: Peter Krempa pkre...@redhat.com
 ---
  libvirt-override.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 

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] [python PATCH 1/3] Fix function name when parsing arguments in libvirt_virNodeAllocPages

2014-10-21 Thread Eric Blake 
On 10/21/2014 08:34 AM, Peter Krempa wrote:
 The override function was copiedpasted from virConnectGetAllDomainStats
 and the function name after the collon was not changed. Fix the issue as

s/collon/colon/

 an invalid name would appear in the error message.
 ---
  libvirt-override.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 

ACK

 diff --git a/libvirt-override.c b/libvirt-override.c
 index c887b71..b4bb571 100644
 --- a/libvirt-override.c
 +++ b/libvirt-override.c
 @@ -8214,7 +8214,7 @@ libvirt_virNodeAllocPages(PyObject *self 
 ATTRIBUTE_UNUSED,
  unsigned int flags = VIR_NODE_ALLOC_PAGES_ADD;
  int c_retval;
 
 -if (!PyArg_ParseTuple(args, (char *)OOiii:virConnectGetAllDomainStats,
 +if (!PyArg_ParseTuple(args, (char *)OOiii:virNodeAllocPages,
pyobj_conn, pyobj_pages,
startCell, cellCount, flags))
  return NULL;
 

-- 
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] [python PATCH 3/3] Fix rest of unsigned integer handling

2014-10-21 Thread Eric Blake 
On 10/21/2014 08:34 AM, Peter Krempa wrote:
 As in the previous patch, fix all places where 'flags' is converted as a
 signed argument to unsigned including the python code generator.
 ---
  generator.py|   2 +-
  libvirt-lxc-override.c  |   2 +-
  libvirt-override.c  | 128 
 
  libvirt-qemu-override.c |   4 +-
  4 files changed, 68 insertions(+), 68 deletions(-)

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] [Qemu-devel] spec, RFC: TLS support for NBDµ

2014-10-21 Thread Wouter Verhelst 
Hi Markus,

On Tue, Oct 21, 2014 at 10:17:17AM +0200, Markus Armbruster wrote:
 Wouter Verhelst w...@uter.be writes:
  On Mon, Oct 20, 2014 at 01:51:43PM +0200, Markus Armbruster wrote:
[...]
  Furthermore, STARTTLS is vulnerable to active attacks: if you can get
  between the peers, you can make them fall back to unencrypted silently.
  How do you plan to guard against that?
 
  As I've said before in this discussion, encryption downgrade attacks are
  not specific to STARTTLS; as soon as you have have an encrypted and an
  unencrypted variant of a protocol, that becomes a problem. After all,
  if an attacker can modify the communication so that STARTTLS is filtered
  out of the communication, they can most likely also redirect all traffic
  to a decrypting/encrypting proxy.
 
  The only way to fix that is through userspace; make opportunistic TLS
  (i.e., use it if available, but move on if not) difficult to achieve.
 
  See also https://www.agwa.name/blog/post/starttls_considered_harmful
 
  A random blog post by an author who is speaking about STARTTLS in
  general terms is not a good technical argument for why STARTTLS is a bad
  idea in *this* specific case.
 
 Misunderstanding.  I didn't mean to claim STARTTLS is bad.  If I
 wanted to say that, I would've said it directly.  I was merely asking
 how you plan to guard against downgrade attacks.  I gather your advice
 is to make the client (QEMU) insist on TLS, and check the server's
 certificate.  Correct?

My advice is to give both client and server the ability to have TLS
switched on or off, and possibly (but not necessarily so, and certainly
not by default) also the _ability_ to negotiate TLS if the other side
supports it, while not aborting if it doesn't.

  If I was defining a new protocol from scratch, I might dump the whole
  thing in TLS to begin with. But that's just not the case, so I have to
  deal with what exists already.
 
 Yes.  You can still start over on a separate port.  However:

No, I can't, at least not if I want to continue to use an assigned port
without breaking backwards compatibility. When 10809 was assigned to
NBD, IANA made it perfectly clear that I wouldn't get a new port number
for a secure variant.

  In addition, with the current state of affairs, it is *not possible* to
  swap to an NBD device if you need to pipe its data through a separate
  socket than the one you're handing to the kernel. The result of that is
  that you can't do TLS on a device you want to swap to. This means we
  need to continue to support a protocol that can do TLS for some exports,
  and plain (unencrypted) traffic for other exports, *in the same running
  nbd-server instance*.
 
 I don't understand enough about NBD to challenge this argument.  The sad
 consequence is more complexity and a larger attack surface.
 
 Out of curiosity: is this merely an implementation restriction, or is
 it more fundamental?

Well, to some extent, every software issue is merely an implementation
restriction...

The problem is that in order to clear memory when your swap device is on
the other side of a network connection, you need to write to said swap
device, which causes TCP traffic, which means the kernel needs to read
TCP ACK packets (or in the case of NBD, the reply packet to the
NBD_CMD_WRITE request that you sent out) to ensure that the traffic has
in fact reached its destination. Unfortunately, you can't impose
ordering on incoming network traffic, so that means you may need to read
through a whole lot of youtube MPEG traffic or some such in order to
find your one TCP ACK that tells you your swapout has been processed
correctly and you can now clear the memory which you wrote out to swap.

It's fairly obvious how that could lead to a deadlock if you don't know
that this one data stream is not related to swapspace while this other
one here is.

That's why a PF_MEMALLOC kernel-space socket option was created; sockets
marked with that option are related to a swapdevice, so when the kernel
is low on memory, it will throw away all incoming network traffic
_except_ for packets destined for a socket marked with that option, in
the hope that this will allow us to clear up some memory.

Since it's a kernel-space only thing, that means you can't mark sockets
as such from userspace; so if the NBD traffic is wrapped in some other
traffic from which it needs to be unwrapped in userspace, then the
userspace component would basically be a proxy with two sockets -- one
towards the server, one towards the kernel. The socket towards the
kernel would have the PF_MEMALLOC socket option set, but the one towards
the server would not, and *that* is the one which actually has data
flowing in over the network. There's your deadlock again.

Since, with the current state of affairs, the NBD handshake is done in
user space with the actual data pushing stuff later being done in kernel
space, that means you do actually need to unwrap the TLS traffic in
userspace.

I can't think of

Re: [libvirt] libvirt build failure on armhf (Re: [Xen-devel] [libvirt bisection] complete build-armhf-libvirt)

2014-10-21 Thread Jim Fehlig 
Ian Campbell wrote:
 Hi,

 On Fri, 2014-10-17 at 00:42 +0100, xen.org wrote:
   
 *** Found and reproduced problem changeset ***

   Bug is in tree:  libvirt git://libvirt.org/libvirt.git
   Bug introduced:  24c160376275b7d31f71fbde83af8183cbf744a7
   Bug not present: 69f7b67d55316ab7b28fb904b346943497b856a1
 

 The Xen automated build tests have discovered a built error on armhf. I
 don't think it is Xen specific. Our bisector has fingered the changeset
 below. I've had a skim of the git log from there to master and of the
 outstanding patches on libvir list and I don't see anything which is
 obviously a related fix, so I hope this is still useful/relevant.

 An instance of the failure can be seen at
 http://www.chiark.greenend.org.uk/~xensrcts/logs/30765/build-armhf-libvirt/info.html

 The logs say:
 util/virsocketaddr.c: In function 'virSocketAddrIsNumericLocalhost':
 util/virsocketaddr.c:904:17: error: cast increases required alignment of 
 target type [-Werror=cast-align]
 util/virsocketaddr.c:909:17: error: cast increases required alignment of 
 target type [-Werror=cast-align]
   

Hi Ian,

While catching up on libvirt mail today, I not only saw your report, but
also noticed a fix from Roman

https://www.redhat.com/archives/libvir-list/2014-October/msg00559.html

Regards,
Jim

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

[libvirt] [python PATCH] Fix flags cannot get right value for blockCopy function

2014-10-21 Thread Luyao Huang 
When use blockCopy, flags cannot get a right value, because
PyArg_ParseTuple want to get 6 parameters and blockCopy only
pass 5.Flags will get a unpredictable value, this will make
this function cannot be used.And error just like:

unsupported flags (0x7f6c) in function qemuDomainBlockCopy

Signed-off-by: Luyao Huang lhu...@redhat.com
---
 libvirt-override.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index c887b71..4999ac3 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -8175,8 +8175,7 @@ libvirt_virDomainBlockCopy(PyObject *self 
ATTRIBUTE_UNUSED, PyObject *args)
 int c_retval;
 
 if (!PyArg_ParseTuple(args, (char *) Ozz|Oi:virDomainBlockCopy,
-  pyobj_dom, disk, destxml, pyobj_dict, params,
-  flags))
+  pyobj_dom, disk, destxml, pyobj_dict, flags))
 return VIR_PY_INT_FAIL;
 
 if (PyDict_Check(pyobj_dict)) {
-- 
1.8.3.1

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