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

[Anirban Chakraborty]

On 11/20/14, 5:43 AM, "Vasiliy Tolstov"  wrote:

>2014-11-20 7:58 GMT+03:00 Anirban Chakraborty :
>> Your original error message was that qemu couldn¹t find a tap device.
>>So,
>> I am not sure how the above error relates to your original error.
>> Can you write your detail configuration setup and the steps you have
>>taken
>> to get at this error?
>
>
>If i'm not wrong error goes from:
>qemuBuildInterfaceCommandLine builds interface command line and before
>qemu started tries to add tap device to tc, but tap created later via
>qemu (in my case).
>I think that virNetDevBandwidthSet needs to be run after qemu started.

Please look at the code. Tap device is not created for network interface
type ethernet. virNetDevTapCreate creates the tap device, which eventually
gets called from qemuNetworkIfaceConnect and this applies to interface
types network or bridge. For ethernet type interfaces, the onus is on the
user to create the tap device in the first place before configuring
bandwidth for it. 

So, there is no point in changing any code here, if the tap device doesn’t
exist and the vm configuration file has an entry for it, then libvirt is
supposed to throw the error message to let the user know about it.

Anirban


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

[libvirt] [PATCH] docs: fix a typo in formatdomain.html

[Chen Fan]

Signed-off-by: Chen Fan 
---
 docs/formatdomain.html.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 9364eb5..6a15074 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -3413,7 +3413,7 @@
   setting for the attribute is no for security
   reasons and support depends on the guest network device model as
   well as the type of connection on the host - currently it is
-  only supported for the virtio ddevice model and for macvtap
+  only supported for the virtio device model and for macvtap
   connections on the host.
 
 
-- 
1.9.3

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

Re: [libvirt] ANNOUNCE: virt-manager 1.1.0 released

[Jason Helfman]

Hello,

I was able to get this imported for FreeBSD, but had to implement a couple
of patches for it to work:

https://svnweb.freebsd.org/ports/head/deskutils/virt-manager/files/patch-virtManager_config.py?view=markup&pathrev=372972
https://svnweb.freebsd.org/ports/head/deskutils/virt-manager/files/patch-virtManager_console.py?view=markup&pathrev=372972

This was taken obtained from:
https://build.opensuse.org/package/view_file/openSUSE:Factory/virt-manager/virt-manager-Gtk-30.patch#

And is reference by a bug here:
https://bugzilla.suse.com/show_bug.cgi?id=901869

Comments and suggestions are welcome...

-jgh

On Sun, Sep 7, 2014 at 1:59 PM, Cole Robinson  wrote:

> I'm happy to announce the release of virt-manager 1.1.0!
>
> virt-manager is a desktop application for managing KVM, Xen, and LXC
> virtualization via libvirt.
>
> The release can be downloaded from:
>
> http://virt-manager.org/download/
>
> The direct download links are:
>
>
> http://virt-manager.org/download/sources/virt-manager/virt-manager-1.1.0.tar.gz
>
> This release includes:
>
> - Switch to libosinfo as OS metadata database (Giuseppe Scrivano)
> - Use libosinfo for OS detection from CDROM media labels (Giuseppe
>   Scrivano)
> - Use libosinfo for improved OS defaults, like recommended disk size
>   (Giuseppe Scrivano)
> - virt-image tool has been removed, as previously announced
> - Enable Hyper-V enlightenments for Windows VMs
> - Revert virtio-console default, back to plain serial console
> - Experimental q35 option in new VM 'customize' dialog
> - UI for virtual network QoS settings (Giuseppe Scrivano)
> - virt-install: --disk discard= support (Jim Minter)
> - addhardware: Add spiceport UI (Marc-André Lureau)
> - virt-install: --events on_poweroff etc. support (Chen Hanxiao)
> - cli --network portgroup= support and UI support
> - cli --boot initargs= and UI support
> - addhardware: allow setting controller model (Chen Hanxiao)
> - virt-install: support setting hugepage options (Chen Hanxiao)
>
> Thanks to everyone who has contributed to this release through testing,
> bug reporting, submitting patches, and otherwise sending in feedback!
>
> Thanks,
> Cole
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list




-- 
Jason Helfman  | FreeBSD Committer
j...@freebsd.org | http://people.freebsd.org/~jgh  | The Power to Serve
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] network: Let domains be restricted to local DNS

[Josh Stone]

This adds a new "localOnly" attribute on the domain element of the
network xml.  With this set to "yes", DNS requests under that domain
will only be resolved by libvirt's dnsmasq, never forwarded upstream.

This was how it worked before commit f69a6b987d616, and I found that
functionality useful.  For example, I have my host's NetworkManager
dnsmasq configured to forward that domain to libvirt's dnsmasq, so I can
easily resolve guest names from outside.  But if libvirt's dnsmasq
doesn't know a name and forwards it to the host, I'd get an endless
forwarding loop.  Now I can set localOnly="yes" to prevent the loop.

Signed-off-by: Josh Stone 
Cc: Laine Stump 
---
 docs/formatnetwork.html.in | 12 +++-
 docs/schemas/network.rng   |  3 +++
 src/conf/network_conf.c|  5 +
 src/conf/network_conf.h|  1 +
 src/network/bridge_driver.c|  5 +
 .../networkxml2confdata/nat-network-dns-local-domain.conf  | 14 ++
 tests/networkxml2confdata/nat-network-dns-local-domain.xml |  9 +
 tests/networkxml2conftest.c|  1 +
 8 files changed, 49 insertions(+), 1 deletion(-)
 create mode 100644 tests/networkxml2confdata/nat-network-dns-local-domain.conf
 create mode 100644 tests/networkxml2confdata/nat-network-dns-local-domain.xml

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index dc438aee8622..defcdba00930 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -82,7 +82,7 @@
 
 ...
 
-
+
 
 ...
 
@@ -113,6 +113,16 @@
 a  mode of "nat" or "route" (or an
 isolated network with no 
 element). Since 0.4.5
+
+
+  If the optional localOnly attribute on the
+  domain element is "yes", then DNS requests under
+  this domain will only be resolved by the virtual network's own
+  DNS server - they will not be forwarded to the host's upstream
+  DNS server.  If localOnly is "no", and by
+  default, unresolved requests will be forwarded.
+  Since 1.2.11
+
   
   forward
   Inclusion of the forward element indicates that
diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
index 4546f8037580..a1da28092375 100644
--- a/docs/schemas/network.rng
+++ b/docs/schemas/network.rng
@@ -225,6 +225,9 @@
 
   
 
+
+  
+
   
 
 
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 067334e87cb0..61451c39805f 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -2083,6 +2083,11 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
 
 /* Parse network domain information */
 def->domain = virXPathString("string(./domain[1]/@name)", ctxt);
+tmp = virXPathString("string(./domain[1]/@localOnly)", ctxt);
+if (tmp) {
+def->domain_local = STRCASEEQ(tmp, "yes");
+VIR_FREE(tmp);
+}
 
 if ((bandwidthNode = virXPathNode("./bandwidth", ctxt)) != NULL &&
 (def->bandwidth = virNetDevBandwidthParse(bandwidthNode, -1)) == NULL)
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index 660cd2d10cd1..6308a7dcfbf7 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -232,6 +232,7 @@ struct _virNetworkDef {
 
 char *bridge;   /* Name of bridge device */
 char *domain;
+bool domain_local; /* Choose not to forward dns for this domain */
 unsigned long delay;   /* Bridge forward delay (ms) */
 bool stp; /* Spanning tree protocol */
 virMacAddr mac; /* mac address of bridge device */
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 6cb421c52850..dfa375d3aa72 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -912,6 +912,11 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
 }
 
 if (network->def->domain) {
+if (network->def->domain_local) {
+virBufferAsprintf(&configbuf,
+  "local=/%s/\n",
+  network->def->domain);
+}
 virBufferAsprintf(&configbuf,
   "domain=%s\n"
   "expand-hosts\n",
diff --git a/tests/networkxml2confdata/nat-network-dns-local-domain.conf 
b/tests/networkxml2confdata/nat-network-dns-local-domain.conf
new file mode 100644
index ..5f41b9186cbc
--- /dev/null
+++ b/tests/networkxml2confdata/nat-network-dns-local-domain.conf
@@ -0,0 +1,14 @@
+##WARNING:  THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE
+##OVERWRITTEN AND LOST.  Ch

Re: [libvirt] [PATCH v2 0/5] Guest filesystem information API

[Tomoki Sekiyama]

On 11/20/14, 18:54 , "John Ferlan"  wrote:

>On 11/20/2014 01:19 PM, Eric Blake wrote:
>> On 11/20/2014 11:11 AM, John Ferlan wrote:
>> 
 However, for some reason I see this build error after 2/5:

 make[3]: Entering directory
 '/home/zippy/work/libvirt/libvirt.git/tools/wireshark/src'
CC   libvirt_la-packet-libvirt.lo
 In file included from libvirt/protocol.h:5:0,
   from packet-libvirt.h:112,
   from packet-libvirt.c:36:
 ./libvirt/remote.h: In function
'dissect_xdr_remote_typed_param_value':
 ./libvirt/remote.h:470:5: error: unknown type name
'remote_nonnull_string'
   remote_nonnull_string type = 0;
   ^
>> 
>>> Strange - I don't see this.  My ./tools/wireshark/src/libvirt/remote.h
>>> doesn't even have that line. Stranger still, my config.log (after make
>>> clean; ./autogen.sh --system; make j4) has:
>>>
>>> configure:69348: checking for wireshark
>>> configure:69381: result: no
>>>
>>>
>>> Could it be environmental? Could it be some config option? Perhaps
>>> something installed?  I have for wireshark on my f20 system:
>>>
>>> wireshark.x86_64   1.10.10-1.fc20 @updates
>> 
>> You need wireshark-devel before configure will build wireshark
>> interactions into libvirt.
>> 
>> 
>
>So, yes installing wireshark-devel results in a failure for me to.
>
>So, I stopped after 1/5 and modified the "location" of the definitions
>in remote_protocol.x for remote_domain_fsinfo {}.
>
>If move just:
>
>struct remote_domain_fsinfo {
>remote_nonnull_string mountpoint;
>remote_nonnull_string name;
>remote_nonnull_string type;
>remote_nonnull_string dev_aliases;
>/* (const char **) */
>};
>
>to anywhere (I tested) before:
>
>struct remote_network_dhcp_lease {
>remote_nonnull_string iface;
>hyper expirytime;
>int type;
>remote_string mac;
>remote_string iaid;
>remote_nonnull_string ipaddr;
>unsigned int prefix;
>remote_string hostname;
>remote_string clientid;
>};
>
>Then the build succeeds; however, afterwards it fails. I even tried
>changing "dev_aliases" to "devAliases" to see if that would help - it
>didn't.
>
>If I move those dhcp defs to after the get_fsinfo defs, things work fine
>again.  Perhaps there's something in tools/wireshark/util/genxdrstub.pl
>or something in that dhcp_lease def.
>
>I'm not quite sure what the issue is.
>
>John

I could reproduce this issue, and it looks like I can avoid the issue
by renaming the ¹type¹ field to Œfstype¹.
Now investigating why this happens...

Thanks,
Tomoki



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

Re: [libvirt] [libvirt-python PATCH] override: Implement bindings for virDomainGetFSInfo as domain.fsInfo

[Tomoki Sekiyama]

On 11/20/14, 7:24 , "Pavel Hrdina"  wrote:

>On 11/20/2014 01:22 PM, Pavel Hrdina wrote:
>> On 11/18/2014 12:29 AM, Tomoki Sekiyama wrote:
>>> Implement the function which returns a list of tuples, that contains
>>> members
>>> of virDomainFSInfo struct.
>>>
>>> Signed-off-by: Tomoki Sekiyama 
>>> ---
>>>   generator.py |5 +++
>>>   libvirt-override-api.xml |6 
>>>   libvirt-override.c   |   70
>>> ++
>>>   sanitytest.py|5 +++
>>>   4 files changed, 85 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/generator.py b/generator.py
>>> index c66c6d4..39ba2f7 100755
>>> --- a/generator.py
>>> +++ b/generator.py
>>> @@ -476,6 +476,7 @@ skip_impl = (
>>>   'virNetworkGetDHCPLeases',
>>>   'virDomainBlockCopy',
>>>   'virNodeAllocPages',
>>> +'virDomainGetFSInfo',
>>>   )
>>>
>>>   lxc_skip_impl = (
>>> @@ -586,6 +587,7 @@ skip_function = (
>>>
>>>   'virNetworkDHCPLeaseFree', # only useful in C, python code uses
>>> list
>>>   'virDomainStatsRecordListFree', # only useful in C, python uses
>>> dict
>>> +'virDomainFSInfoFree', # only useful in C, python code uses list
>>>   )
>>>
>>>   lxc_skip_function = (
>>> @@ -1107,6 +1109,9 @@ def nameFixup(name, classe, type, file):
>>>   elif name[0:20] == "virDomainGetCPUStats":
>>>   func = name[9:]
>>>   func = func[0:1].lower() + func[1:]
>>> +elif name[0:20] == "virDomainGetFSInfo":
>>
>> There definitely must be name[0:18] as John pointed out.
>>
>>> +func = name[12:]
>>> +func = func[0:2].lower() + func[2:]
>>>   elif name[0:12] == "virDomainGet":
>>>   func = name[12:]
>>>   func = func[0:1].lower() + func[1:]
>>> diff --git a/libvirt-override-api.xml b/libvirt-override-api.xml
>>> index 4fe3c4d..2e807ba 100644
>>> --- a/libvirt-override-api.xml
>>> +++ b/libvirt-override-api.xml
>>> @@ -658,5 +658,11 @@
>>> 
>>> 
>>>   
>>> +
>>> +  Get a list of mapping informaiton for each mounted file
>>> systems within the specified guest and the disks.
>>> +  
>>> +  
>>> +  
>>> +
>>> 
>>>   
>>> diff --git a/libvirt-override.c b/libvirt-override.c
>>> index 8895289..bd6321f 100644
>>> --- a/libvirt-override.c
>>> +++ b/libvirt-override.c
>>> @@ -8266,6 +8266,73 @@ libvirt_virNodeAllocPages(PyObject *self
>>> ATTRIBUTE_UNUSED,
>>>   }
>>>   #endif /* LIBVIR_CHECK_VERSION(1, 2, 8) */
>>>
>>> +#if LIBVIR_CHECK_VERSION(1, 2, 11)
>>> +
>>> +static PyObject *
>>> +libvirt_virDomainGetFSInfo(PyObject *self ATTRIBUTE_UNUSED, PyObject
>>> *args) {
>>> +virDomainPtr domain;
>>> +PyObject *pyobj_domain;
>>> +unsigned int flags;
>>> +virDomainFSInfoPtr *fsinfo = NULL;
>>> +char **dev;
>>> +int c_retval, i;
>>> +PyObject *py_retval;
>>> +
>>> +if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainFSInfo",
>>> +&pyobj_domain, &flags))
>>> +return NULL;
>>> +domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
>>> +
>>> +LIBVIRT_BEGIN_ALLOW_THREADS;
>>> +c_retval = virDomainGetFSInfo(domain, &fsinfo, flags);
>>> +LIBVIRT_END_ALLOW_THREADS;
>>> +
>>> +if (c_retval < 0)
>>> +goto cleanup;
>>> +
>>> +/* convert to a Python list */
>>> +if ((py_retval = PyList_New(c_retval)) == NULL)
>>> +goto cleanup;
>>
>> The PyList_New on success return new reference ... [1]
>>
>>> +
>>> +for (i = 0; i < c_retval; i++) {
>>> +virDomainFSInfoPtr fs = fsinfo[i];
>>> +PyObject *info, *alias;
>>> +
>>> +if (fs == NULL)
>>> +goto cleanup;
>>> +info = PyTuple_New(4);
>>> +if (info == NULL)
>>
>> Wrong indentation, use spaces instead of tabs.
>>
>>> +goto cleanup;
>>> +PyList_SetItem(py_retval, i, info);
>>> +alias = PyList_New(0);
>>> +if (alias == NULL)
>>
>> The same wrong indentation.
>>
>>> +goto cleanup;
>>> +
>>> +PyTuple_SetItem(info, 0,
>>> libvirt_constcharPtrWrap(fs->mountpoint));
>>> +PyTuple_SetItem(info, 1, libvirt_constcharPtrWrap(fs->name));
>>> +PyTuple_SetItem(info, 2, libvirt_constcharPtrWrap(fs->type));
>>> +PyTuple_SetItem(info, 3, alias);
>>> +
>>> +for (dev = fs->devAlias; dev && *dev; dev++)
>>> +if (PyList_Append(alias, libvirt_constcharPtrWrap(*dev))
>>> < 0)
>>> +goto cleanup;
>>> +}
>>> +
>>> +for (i = 0; i < c_retval; i++)
>>> +virDomainFSInfoFree(fsinfo[i]);
>>> +VIR_FREE(fsinfo);
>>> +return py_retval;
>>> +
>>> + cleanup:
>>> +for (i = 0; i < c_retval; i++)
>>> +virDomainFSInfoFree(fsinfo[i]);
>>> +VIR_FREE(fsinfo);
>>> +Py_DECREF(py_retval);
>>
>> [1] ... there you correctly dereference it, but you should use
>> PY_XDECREF because the py_retval could be NULL.
>>
>>> +return VIR_PY_NONE;
>>> +}
>>> +
>>> +#endif /* LIBVIR_CHECK_VERSION(1, 2, 11) */
>>> +
>>>
>>> 
>

Re: [libvirt] [PATCH v2 0/5] Guest filesystem information API

[John Ferlan]

On 11/20/2014 01:19 PM, Eric Blake wrote:
> On 11/20/2014 11:11 AM, John Ferlan wrote:
> 
>>> However, for some reason I see this build error after 2/5:
>>>
>>> make[3]: Entering directory 
>>> '/home/zippy/work/libvirt/libvirt.git/tools/wireshark/src'
>>>CC   libvirt_la-packet-libvirt.lo
>>> In file included from libvirt/protocol.h:5:0,
>>>   from packet-libvirt.h:112,
>>>   from packet-libvirt.c:36:
>>> ./libvirt/remote.h: In function 'dissect_xdr_remote_typed_param_value':
>>> ./libvirt/remote.h:470:5: error: unknown type name 'remote_nonnull_string'
>>>   remote_nonnull_string type = 0;
>>>   ^
> 
>> Strange - I don't see this.  My ./tools/wireshark/src/libvirt/remote.h
>> doesn't even have that line. Stranger still, my config.log (after make
>> clean; ./autogen.sh --system; make j4) has:
>>
>> configure:69348: checking for wireshark
>> configure:69381: result: no
>>
>>
>> Could it be environmental? Could it be some config option? Perhaps
>> something installed?  I have for wireshark on my f20 system:
>>
>> wireshark.x86_64   1.10.10-1.fc20 @updates
> 
> You need wireshark-devel before configure will build wireshark
> interactions into libvirt.
> 
> 

So, yes installing wireshark-devel results in a failure for me to.

So, I stopped after 1/5 and modified the "location" of the definitions
in remote_protocol.x for remote_domain_fsinfo {}.

If move just:

struct remote_domain_fsinfo {
remote_nonnull_string mountpoint;
remote_nonnull_string name;
remote_nonnull_string type;
remote_nonnull_string dev_aliases;
/* (const char **) */
};

to anywhere (I tested) before:

struct remote_network_dhcp_lease {
remote_nonnull_string iface;
hyper expirytime;
int type;
remote_string mac;
remote_string iaid;
remote_nonnull_string ipaddr;
unsigned int prefix;
remote_string hostname;
remote_string clientid;
};

Then the build succeeds; however, afterwards it fails. I even tried
changing "dev_aliases" to "devAliases" to see if that would help - it
didn't.

If I move those dhcp defs to after the get_fsinfo defs, things work fine
again.  Perhaps there's something in tools/wireshark/util/genxdrstub.pl
or something in that dhcp_lease def.

I'm not quite sure what the issue is.

John




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

Re: [libvirt] [PATCH v2 0/5] Guest filesystem information API

[Tomoki Sekiyama]

On 11/19/14, 17:58 , "John Ferlan"  wrote:

>On 11/17/2014 06:26 PM, Tomoki Sekiyama wrote:
>> Hi,
>> 
>> This is v2 of patchset to add virDomainGetFSInfo API.
>> 
>> * changes in v1->v2:
>>  -[all] removed redundant NULL element at the last of returned info
>>array
>>  -[3/5] make error messages in qemu_agent.c consistent with other
>>commands
>>  -[4/5] added a test case for 2 items in info->devAliases
>>  -[5/5] added a pod document for virsh domfsinfo command
>>  (v1: 
>>http://www.redhat.com/archives/libvir-list/2014-October/msg1.html )
>> 
>> * summary
>>   This series implements a new virDomainGetFSInfo API, that returns a
>>list of
>>   mounted filesystems information in the guest, collected via the guest
>>agent.
>> 
>>   The returned info contains mountpoints and disk device alias named in
>>   libvirt, so we can know which mountpoints should be frozen by
>>   virDomainFSFreeze to take snapshots of a part of disks.
>> 
>> ---
>> Tomoki Sekiyama (5):
>>   Implement public API for virDomainGetFSInfo
>>   remote: Implement the remote protocol for virDomainGetFSInfo
>>   qemu: Implement the qemu driver for virDomainGetFSInfo
>>   qemu: add test for qemuAgentGetFSInfo
>>   virsh: expose virDomainGetFSInfo
>> 
>> 
>>  daemon/remote.c  |  117 
>>  include/libvirt/libvirt-domain.h |   21 
>>  src/conf/domain_conf.c   |   71 
>>  src/conf/domain_conf.h   |6 +
>>  src/driver-hypervisor.h  |6 +
>>  src/libvirt.c|   66 +++
>>  src/libvirt_private.syms |1
>>  src/libvirt_public.syms  |6 +
>>  src/qemu/qemu_agent.c|  178
>>++
>>  src/qemu/qemu_agent.h|2
>>  src/qemu/qemu_driver.c   |   48 
>>  src/remote/remote_driver.c   |   92 
>>  src/remote/remote_protocol.x |   32 +
>>  src/remote_protocol-structs  |   21 
>>  src/rpc/gendispatch.pl   |1
>>  tests/Makefile.am|1
>>  tests/qemuagentdata/qemuagent-fsinfo.xml |   39 +++
>>  tests/qemuagenttest.c|  143
>>
>>  tools/virsh-domain.c |   74 
>>  tools/virsh.pod  |9 ++
>>  20 files changed, 933 insertions(+), 1 deletion(-)
>>  create mode 100644 tests/qemuagentdata/qemuagent-fsinfo.xml
>> 
>> --
>> 
>> Tomoki Sekiyama
>> 
>
>
>I reviewed the 'libvirt' specific changes - had a few comments and have
>made changes to my review branch as specified.  As long as you're OK
>with those changes I will get these pushed.

Thanks for the review and fixups! And I apologize I¹ve missed some of your
last comments.

I¹ll send v3 patch including your fixups, and some changes according to
Eric¹s comments (adding length of devAlias array, using @acl
domain:fs_freeze).

Thanks,
Tomoki

>I'm also hoping someone else (eblake?) can look at the remote_protocol.x
>changes to ensure they encompass everything they are supposed to.  Also
>that the usage of QEMU_JOB_QUERY not _MODIFY for the GetFSInfo seems
>more appropriate and is in line with the various remote_protocol.x
>settings (@acl/@generate stuff settings).
>
>I'll look at the python changes separately, although I think phrdina
>understands what needs to change there the best!
>
>John


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

Re: [libvirt] [PATCH v2 2/5] remote: Implement the remote protocol for virDomainGetFSInfo

[Tomoki Sekiyama]

On 11/20/14, 14:12 , "Eric Blake"  wrote:

>On 11/17/2014 04:26 PM, Tomoki Sekiyama wrote:
>> Add daemon and driver code to (de-)serialize virDomainFSInfo.
>> 
>> Signed-off-by: Tomoki Sekiyama 
>> ---
>>  daemon/remote.c  |  117
>>++
>>  src/remote/remote_driver.c   |   92 +
>>  src/remote/remote_protocol.x |   32 +++
>>  src/remote_protocol-structs  |   21 
>>  src/rpc/gendispatch.pl   |1
>>  5 files changed, 262 insertions(+), 1 deletion(-)
>> 
>> diff --git a/daemon/remote.c b/daemon/remote.c
>> index 1d7082e..9b89fd0 100644
>> --- a/daemon/remote.c
>> +++ b/daemon/remote.c
>> @@ -6336,6 +6336,123 @@ remoteDispatchNodeAllocPages(virNetServerPtr
>>server ATTRIBUTE_UNUSED,
>>  }
>>  
>>  
>> +static int
>> +remoteDispatchDomainGetFSInfo(virNetServerPtr server ATTRIBUTE_UNUSED,
>> +  virNetServerClientPtr client,
>> +  virNetMessagePtr msg ATTRIBUTE_UNUSED,
>> +  virNetMessageErrorPtr rerr,
>> +  remote_domain_get_fsinfo_args *args,
>> +  remote_domain_get_fsinfo_ret *ret)
>
>Are we sure we have to write this by hand? [1]
>
>
>> @@ -8171,6 +8262,7 @@ static virHypervisorDriver hypervisor_driver = {
>>  .connectGetDomainCapabilities =
>>remoteConnectGetDomainCapabilities, /* 1.2.7 */
>>  .connectGetAllDomainStats = remoteConnectGetAllDomainStats, /*
>>1.2.8 */
>>  .nodeAllocPages = remoteNodeAllocPages, /* 1.2.9 */
>> +.domainGetFSInfo = remoteDomainGetFSInfo, /* 1.2.10 */
>
>1.2.10 is wrong; the next release is 1.2.11.
>
>>  };
>>  
>>  static virNetworkDriver network_driver = {
>> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
>> index ebf4530..10c8068 100644
>> --- a/src/remote/remote_protocol.x
>> +++ b/src/remote/remote_protocol.x
>> @@ -250,6 +250,12 @@ const REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX =
>>4096;
>>  /* Upper limit of message size for tunable event. */
>>  const REMOTE_DOMAIN_EVENT_TUNABLE_MAX = 2048;
>>  
>> +/* Upper limit on number of mountpoints in fsinfo */
>> +const REMOTE_DOMAIN_FSINFO_MAX = 256;
>> +
>> +/* Upper limit on number of disks per mountpoint in fsinfo */
>> +const REMOTE_DOMAIN_FSINFO_DISKS_MAX = 256;
>> +
>>  /* UUID.  VIR_UUID_BUFLEN definition comes from libvirt.h */
>>  typedef opaque remote_uuid[VIR_UUID_BUFLEN];
>>  
>> @@ -3111,6 +3117,24 @@ struct remote_connect_get_all_domain_stats_args {
>>  struct remote_connect_get_all_domain_stats_ret {
>>  remote_domain_stats_record retStats;
>>  };
>> +
>> +struct remote_domain_fsinfo {
>> +remote_nonnull_string mountpoint;
>> +remote_nonnull_string name;
>> +remote_nonnull_string type;
>> +remote_nonnull_string dev_aliases;
>>/* (const char **) */
>
>Can any of these values ever be NULL because the guest didn't provide
>them?  If so, you want remote_string instead of remote_nonnull_string.
>It wasn't obvious to me in the 1/5 documentation whether values are
>guaranteed to be non-NULL.

These are always filled. If guest agent doesn¹t pass these values,
the qemu driver will return an error.

>> +
>> +/**
>> + * @generate: none
>> + * @acl: domain:read
>> + */
>> +REMOTE_PROC_DOMAIN_GET_FSINFO = 348
>
>[1] Did you try any other values of @generate to see if things get
>transferred correctly without writing it by hand?  Then again, looking
>at the structure you are transferring, it consists of mallocing an array
>of structures which themselves malloc an array of names, so I guess you
>are right that you have to manage it by hand (the generator probably
>doesn't do that).

Right, I¹ve tried code generation, but it couldn¹t handle dynamically
allocated arrays in an array...


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

Re: [libvirt] [PATCH v2 1/5] Implement public API for virDomainGetFSInfo

[Tomoki Sekiyama]

On 11/20/14, 14:09 , "Eric Blake"  wrote:

>On 11/17/2014 04:26 PM, Tomoki Sekiyama wrote:
>> virDomainGetFSInfo returns a list of filesystems information mounted in
>>the
>> guest, which contains mountpoints, device names, filesystem types, and
>> device aliases named by libvirt. This will be useful, for example, to
>> specify mountpoints to fsfreeze when taking snapshot of a part of disks.
>> 
>> Signed-off-by: Tomoki Sekiyama 
>> ---
>>  include/libvirt/libvirt-domain.h |   21 
>>  src/driver-hypervisor.h  |6 +++
>>  src/libvirt.c|   66
>>++
>>  src/libvirt_public.syms  |6 +++
>>  4 files changed, 99 insertions(+)
>> 
>
>> +++ b/include/libvirt/libvirt-domain.h
>> @@ -3456,6 +3456,27 @@ int virDomainFSThaw(virDomainPtr dom,
>>  unsigned int nmountpoints,
>>  unsigned int flags);
>>  
>> +/**
>> + * virDomainFSInfo:
>> + *
>> + * The data structure containing mounted file systems within a guset
>> + *
>> + */
>> +typedef struct _virDomainFSInfo virDomainFSInfo;
>> +typedef virDomainFSInfo *virDomainFSInfoPtr;
>> +struct _virDomainFSInfo {
>> +char *mountpoint; /* path to mount point */
>> +char *name;   /* device name in the guest (e.g. "sda1") */
>> +char *type;   /* filesystem type */
>> +char **devAlias;  /* NULL-terminated array of disk device aliases
>>*/
>> +};
>
>Is it worth also having a size_t ndevAlias that says how long the array
>is?  It may make client life easier if they have an up-front count.

OK, I¹ll add ndevAlias and iterate the devAlias array using that counter.

Thanks,
Tomoki Sekiyama


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

Re: [libvirt] [PATCH v2 0/5] Guest filesystem information API

[Tomoki Sekiyama]

On 11/20/14, 14:17 , "Eric Blake"  wrote:

>On 11/20/2014 05:33 AM, Michal Privoznik wrote:
>
>>> I'm also hoping someone else (eblake?) can look at the
>>>remote_protocol.x
>>> changes to ensure they encompass everything they are supposed to.  Also
>>> that the usage of QEMU_JOB_QUERY not _MODIFY for the GetFSInfo seems
>>> more appropriate and is in line with the various remote_protocol.x
>>> settings (@acl/@generate stuff settings).
>> 
>> 
>> @generate is correct, since both, client and server implementations are
>> provided.
>> @acl looks consistent to the rest. Correct, for querying domain info you
>> need to have read permission and that's it.
>
>Oh, wait.  This is an interaction with the guest agent.  We have already
>stated that ANY action that requires guest cooperation MUST require more
>than plain domain:read privileges (for example, creating a snapshot
>requires domain:fs_freeze if the quiesce flag is present; using
>virDomainShutdownFlags requires domain:write if the guest agent is
>involved).
>
>Since the main use of this API is to query the list of mountpoints that
>then feed virDomainFSFreeze, I think this should be @acl
>domain:fs_freeze, rather than domain:read.  Even if it is a read-only
>operation, it makes more sense to treat this command as a family where a
>user is either given rights for all related freeze APIs or none of them.

OK, I¹ll change this to '@acl domain:fs_freeze¹ and
use QEMU_JOB_QUERY because this interact with qemu-guest-agent.

-- 
Tomoki Sekiyama



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

Re: [libvirt] [PATCH 1/2] virdbus: don't force users to pass int for bool values

[Eric Blake]

On 11/20/2014 08:12 AM, Conrad Meyer wrote:
> Hi Eric,
> 
> I think this change breaks build on FreeBSD:
> 
>   CC   util/libvirt_util_la-virdbus.lo
> util/virdbus.c:956:13: error: cast from 'bool *' to 'dbus_bool_t *' (aka 
> 'unsigned int *') increases required alignment from 1 to 4 
> [-Werror,-Wcast-align]
> GET_NEXT_VAL(dbus_bool_t, bool_val, bool, "%d");
> ^~~
> util/virdbus.c:858:17: note: expanded from macro 'GET_NEXT_VAL'
> x = (dbustype *)(*xptrptr + (*narrayptr - 1));  \
> ^ 1 error generated.

Valid complaint, so I'll have to figure something out to avoid it.  :(
Thanks for the heads up.

-- 
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] storage: qemu: Fix security labelling of new image chain elements

[Eric Blake]

On 11/20/2014 08:23 AM, Peter Krempa wrote:
> When creating a disk image snapshot the libvirt code would blindly copy
> the parents label to the newly created image. This runs into problems
> when you start a VM from an image hosted on NFS (or other storage system
> that doesn't support selinux labels) and the snapshot destination is on
> a storage system that does support selinux labels. Libvirt's code in
> that case generates a different security label for the image hosted on
> NFS. This label is valid only for NFS images and doesn't allow access in
> case of a locally stored image.
> 
> To fix this issue libvirt needs to refrain from copying security
> information in cases where the default domain seclabel is a better
> choice.
> 
> This patch repurposes the now unused @force argument of
> virStorageSourceInitChainElement to denote whether a copy of the
> security labelling stuff should be attempted or not. This allows to
> fine-control the copy operation for cases where we need to keep the
> label of the old disk vs. the cases where we need to keep the label
> unset to use the default domain imagelabel.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1151718
> ---

> + * If @transferLabels is true, security labels from the existing disk are 
> copied
> + * to the new disk. Otherwise the default domain imagelabel label will be 
> used.
>   *
>   * Returns 0 on success, -1 on error.
>   */
>  int
>  virStorageSourceInitChainElement(virStorageSourcePtr newelem,
>   virStorageSourcePtr old,
> - bool force)
> + bool transferLables)

Comment was right, code is not.  s/transferLables/transferLabels/

ACK with that fix.

-- 
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 08/11] qemu: process: Refresh virtio channel guest state when connecting to mon

[Jiri Denemark]

On Thu, Nov 20, 2014 at 22:22:56 +0100, Jiri Denemark wrote:
> On Wed, Nov 19, 2014 at 11:23:21 +0100, Peter Krempa wrote:
> > Use data provided by "query-chardev" to refresh the guest frontend state
> > of virtio channels.
> > ---
> >  src/qemu/qemu_process.c | 68 
> > +++--
> >  1 file changed, 66 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index f19963c..3c3ff66 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -2068,6 +2068,61 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm,
> > 
> > 
> >  static int
> > +qemuProcessRefreshChannelVirtioState(virDomainObjPtr vm,
> > + virHashTablePtr info)
> > +{
> > +size_t i;
> > +qemuMonitorChardevInfoPtr entry;
> > +char id[32];
> > +
> > +for (i = 0; i < vm->def->nchannels; i++) {
> > +virDomainChrDefPtr chr = vm->def->channels[i];
> > +if (chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) {
> > +if (snprintf(id, sizeof(id), "char%s",
> > + chr->info.alias) >= sizeof(id)) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +   _("failed to format device alias "
> > + "for PTY retrieval"));
> > +return -1;
> > +}
> > +
> > +/* port state not reported */
> > +if (!(entry = virHashLookup(info, id)))
> > +continue;

But of course, this should be changed to

 if (!(entry = virHashLookup(info, id)) ||
 !entry->state)

in case you decide to store all devices in the hash as I suggested in my
review to 7/11.

Jirka

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

Re: [libvirt] [PATCH 08/11] qemu: process: Refresh virtio channel guest state when connecting to mon

[Jiri Denemark]

On Wed, Nov 19, 2014 at 11:23:21 +0100, Peter Krempa wrote:
> Use data provided by "query-chardev" to refresh the guest frontend state
> of virtio channels.
> ---
>  src/qemu/qemu_process.c | 68 
> +++--
>  1 file changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index f19963c..3c3ff66 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2068,6 +2068,61 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm,
> 
> 
>  static int
> +qemuProcessRefreshChannelVirtioState(virDomainObjPtr vm,
> + virHashTablePtr info)
> +{
> +size_t i;
> +qemuMonitorChardevInfoPtr entry;
> +char id[32];
> +
> +for (i = 0; i < vm->def->nchannels; i++) {
> +virDomainChrDefPtr chr = vm->def->channels[i];
> +if (chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) {
> +if (snprintf(id, sizeof(id), "char%s",
> + chr->info.alias) >= sizeof(id)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("failed to format device alias "
> + "for PTY retrieval"));
> +return -1;
> +}
> +
> +/* port state not reported */
> +if (!(entry = virHashLookup(info, id)))
> +continue;
> +
> +chr->state = entry->state;
> +}
> +}
> +
> +return 0;
> +}
> +
> +
> +static int
> +qemuProcessReconnectRefreshChannelVirtioState(virQEMUDriverPtr driver,
> +  virDomainObjPtr vm)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +virHashTablePtr info = NULL;
> +int ret = -1;
> +
> +qemuDomainObjEnterMonitor(driver, vm);
> +ret = qemuMonitorGetChardevInfo(priv->mon, &info);
> +qemuDomainObjExitMonitor(driver, vm);
> +
> +if (ret < 0)
> +goto cleanup;
> +
> +ret = qemuProcessRefreshChannelVirtioState(vm, info);
> +
> + cleanup:
> +virHashFree(info);
> +return ret;
> +

Extra empty line.

> +}
> +
> +
> +static int
>  qemuProcessWaitForMonitor(virQEMUDriverPtr driver,
>virDomainObjPtr vm,
>int asyncJob,
> @@ -2110,8 +2165,14 @@ qemuProcessWaitForMonitor(virQEMUDriverPtr driver,
>  qemuDomainObjExitMonitor(driver, vm);
> 
>  VIR_DEBUG("qemuMonitorGetChardevInfo returned %i", ret);
> -if (ret == 0)
> -ret = qemuProcessFindCharDevicePTYsMonitor(vm, qemuCaps, info);
> +if (ret == 0) {
> +if ((ret = qemuProcessFindCharDevicePTYsMonitor(vm, qemuCaps,
> +info)) != 0)

We usually check for errors with ... < 0

> +goto cleanup;
> +
> +if ((ret = qemuProcessRefreshChannelVirtioState(vm, info)) != 0)

Same here.

> +goto cleanup;
> +}
> 
>   cleanup:
>  virHashFree(info);
> @@ -3594,6 +3655,9 @@ qemuProcessReconnect(void *opaque)
>  if (qemuDomainCheckEjectableMedia(driver, obj, QEMU_ASYNC_JOB_NONE) < 0)
>  goto error;
> 
> +if (qemuProcessReconnectRefreshChannelVirtioState(driver, obj) < 0)
> +goto error;
> +
>  if (qemuProcessRecoverJob(driver, obj, conn, &oldjob) < 0)
>  goto error;

ACK

Jirka

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

Re: [libvirt] [PATCH 07/11] qemu: chardev: Extract more information about character devices

[Jiri Denemark]

On Wed, Nov 19, 2014 at 11:23:20 +0100, Peter Krempa wrote:
> Improve the monitor function to also retrieve the guest state of
> character device (if provided) so that we can refresh the state of
> virtio-serial channels and perhaps react to changes in the state in
> future patches.
> 
> This patch changes the returned data from qemuMonitorGetChardevInfo to
> return a structure containing the pty path and the state if the info is
> relevant.
> 
> The change to the testsuite makes sure that the data is parsed
> correctly.
> ---
>  src/qemu/qemu_monitor.c  | 13 +++-
>  src/qemu/qemu_monitor.h  |  6 ++
>  src/qemu/qemu_monitor_json.c | 48 
> +---
>  src/qemu/qemu_monitor_text.c | 17 +++-
>  src/qemu/qemu_process.c  |  8 
>  tests/qemumonitorjsontest.c  | 39 +--
>  6 files changed, 103 insertions(+), 28 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 926619f..c9c84f9 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2982,6 +2982,17 @@ qemuMonitorQueryRxFilter(qemuMonitorPtr mon, const 
> char *alias,
>  }
> 
> 
> +static void
> +qemuMonitorChardevInfoFree(void *data,
> +   const void *name ATTRIBUTE_UNUSED)
> +{
> +qemuMonitorChardevInfoPtr info = data;
> +
> +VIR_FREE(info->ptyPath);
> +VIR_FREE(info);
> +}
> +
> +
>  int
>  qemuMonitorGetChardevInfo(qemuMonitorPtr mon,
>virHashTablePtr *retinfo)
> @@ -2997,7 +3008,7 @@ qemuMonitorGetChardevInfo(qemuMonitorPtr mon,
>  goto error;
>  }
> 
> -if (!(info = virHashCreate(10, virHashValueFree)))
> +if (!(info = virHashCreate(10, qemuMonitorChardevInfoFree)))
>  goto error;
> 
>  if (mon->json)
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 3078be0..21533a4 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -649,6 +649,12 @@ int qemuMonitorRemoveNetdev(qemuMonitorPtr mon,
>  int qemuMonitorQueryRxFilter(qemuMonitorPtr mon, const char *alias,
>   virNetDevRxFilterPtr *filter);
> 
> +typedef struct _qemuMonitorChardevInfo qemuMonitorChardevInfo;
> +typedef qemuMonitorChardevInfo *qemuMonitorChardevInfoPtr;
> +struct _qemuMonitorChardevInfo {
> +char *ptyPath;
> +virDomainChrDeviceState state;
> +};
>  int qemuMonitorGetChardevInfo(qemuMonitorPtr mon,
>virHashTablePtr *retinfo);
> 
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 9c8a6fb..5429382 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -3426,6 +3426,9 @@ qemuMonitorJSONExtractChardevInfo(virJSONValuePtr reply,
>  virJSONValuePtr data;
>  int ret = -1;
>  size_t i;
> +char *ptyPath = NULL;
> +virDomainChrDeviceState state;
> +qemuMonitorChardevInfoPtr entry = NULL;
> 
>  if (!(data = virJSONValueObjectGet(reply, "return"))) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> @@ -3440,44 +3443,65 @@ qemuMonitorJSONExtractChardevInfo(virJSONValuePtr 
> reply,
>  }
> 
>  for (i = 0; i < virJSONValueArraySize(data); i++) {
> -virJSONValuePtr entry = virJSONValueArrayGet(data, i);
> +virJSONValuePtr chardev = virJSONValueArrayGet(data, i);
>  const char *type;
> -const char *id;
> -if (!entry) {
> +const char *alias;
> +bool connected;
> +
> +if (!chardev) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("character device information was missing array 
> element"));
>  goto cleanup;
>  }
> 
> -if (!(type = virJSONValueObjectGetString(entry, "filename"))) {
> +if (!(alias = virJSONValueObjectGetString(chardev, "label"))) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("character device information was missing 
> filename"));
> +   _("character device information was missing 
> alias"));

Shouldn't we report that device information was missing label rather
than alias as we are referring to a field in the QMP event?

>  goto cleanup;
>  }
> 
> -if (!(id = virJSONValueObjectGetString(entry, "label"))) {
> +if (!(type = virJSONValueObjectGetString(chardev, "filename"))) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("character device information was missing 
> filename"));
>  goto cleanup;
>  }
> 
> -if (STRPREFIX(type, "pty:")) {
> -char *path;
> -if (VIR_STRDUP(path, type + strlen("pty:")) < 0)
> +if (STRPREFIX(type, "pty:") &&
> +VIR_STRDUP(ptyPath, type + strlen("pty:")) < 0)
> +goto cleanup;
> +
> +if (virJSONValueObjectGetBoole

Re: [libvirt] [PATCH 06/11] qemu: Add handling for VSERPORT_CHANGE event

[Jiri Denemark]

On Wed, Nov 19, 2014 at 11:23:19 +0100, Peter Krempa wrote:
> New qemu added a new event that is emitted when a virtio serial channel
> is opened in the guest OS. This allows us to update the state of the
> port in the output-only XML element.
> 
> This patch implements the monitor callbacks and necessary handlers to
> update the state in the definition.
> ---
>  src/qemu/qemu_domain.h   |  1 +
>  src/qemu/qemu_driver.c   | 57 
> 
>  src/qemu/qemu_monitor.c  | 14 +++
>  src/qemu/qemu_monitor.h  | 10 
>  src/qemu/qemu_monitor_json.c | 23 ++
>  src/qemu/qemu_process.c  | 44 ++
>  6 files changed, 149 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index ad45a66..e4ea4ce 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -196,6 +196,7 @@ typedef enum {
>  QEMU_PROCESS_EVENT_GUESTPANIC,
>  QEMU_PROCESS_EVENT_DEVICE_DELETED,
>  QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED,
> +QEMU_PROCESS_EVENT_SERIAL_CHANGED,
> 
>  QEMU_PROCESS_EVENT_LAST
>  } qemuProcessEventType;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index a84fd47..31bf6bb 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4334,6 +4334,60 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
>  }
> 
> 
> +static void
> +processSerialChangedEvent(virQEMUDriverPtr driver,
> +  virDomainObjPtr vm,
> +  char *devAlias,
> +  bool connected)
> +{
> +virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +virDomainChrDeviceState newstate;
> +virDomainDeviceDef dev;
> +
> +if (connected)
> +newstate = VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED;
> +else
> +newstate = VIR_DOMAIN_CHR_DEVICE_STATE_DISCONNECTED;
> +
> +VIR_DEBUG("Changing serial port state %s in domain %p %s",
> +  devAlias, vm, vm->def->name);
> +
> +if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> +goto cleanup;
> +
> +if (!virDomainObjIsActive(vm)) {
> +VIR_DEBUG("Domain is not running");
> +goto endjob;
> +}
> +
> +if (virDomainDefFindDevice(vm->def, devAlias, &dev, true) < 0)
> +goto endjob;
> +
> +/* we care only about certain devices */
> +if (dev.type != VIR_DOMAIN_DEVICE_CHR ||
> +dev.data.chr->deviceType != VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL ||
> +dev.data.chr->targetType != 
> VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO)
> +goto endjob;
> +
> +dev.data.chr->state = newstate;
> +
> +if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
> +VIR_WARN("unable to save domain status after removing device %s",

Looks like a copy&paste error :-)

ACK with the warning fixed.

Jirka

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

Re: [libvirt] [PATCH 11/11] qemu: Emit the guest agent lifecycle event

[Eric Blake]

On 11/20/2014 12:03 PM, Peter Krempa wrote:

>>> @@ -4369,6 +4370,10 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
>>>  dev.data.chr->targetType != 
>>> VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO)
>>>  goto endjob;
>>>
>>> +if (STREQ_NULLABLE(dev.data.chr->target.name, 
>>> "org.qemu.guest_agent.0") &&
>>> +(event = virDomainEventAgentLifecycleNewFromObj(vm, newstate, 0)))
>>> +qemuDomainEventQueue(driver, event);
>>> +
>>
>> See my comments on 9/11 - I think that filtering this event to just the
>> org.qemu.guest_agent.0 channel is wrong.  That may be the only channel
>> that libvirt specifically cares about for tracking whether we can send
>> guest agent commands, but it is not the only channel that management may
>> care about.  In fact, naming it virDomainEventAgentLifecycle* may be
>> misleading; isn't it really about virDomainEventChannelLifecycle (where
>> guest-agent is just one use of a channel)?
>>
> 
> I specifically wanted to shoot for agent events as they may be used also
> in a different way than just connect/disconnect. If we want to report
> state of channels too I have a partially finished patch that allows to
> do that.

Ooh, you have a point - we might be able to report multiple states, such as:

no agent connected (reasons include just booted, agent disconnect, ...)
agent connected and idle (reasons include agent connect, last agent
command completed, ...)
agent busy (reasons include other libvirt API, qemu-agent-command
backdoor API, ...)

On the other hand, do we need to trigger an event for every state change
like that?  And virDomainEventRegisterAny() doesn't really provide a
nice way for callers to filter which events they want (they have to
filter on every callback for the events they are actually interested
in).  It's better to start with a small set of events, and add more if
there is a good use case.
> 
> The callback prototype then needs to be different as we need to pass
> also the channel name and possibly other data to identify the channel
> (and/or serial port? )

Yeah, I mentioned that in one of my replies to 9/11; I think the channel
name is sufficient.

-- 
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 05/11] conf: Add channel state for virtio channels to the XML

[Jiri Denemark]

On Wed, Nov 19, 2014 at 11:23:18 +0100, Peter Krempa wrote:
> To track state of virtio channels this patch adds a new output-only
> attribute called 'state' to the  element of virtio channels.
> 
> This will be later populated with the guest state of the channel.
> ---
>  docs/formatdomain.html.in  |  9 -
>  docs/schemas/domaincommon.rng  |  3 ++
>  src/conf/domain_conf.c | 35 --
>  src/conf/domain_conf.h | 12 ++
>  .../qemuxml2argv-channel-virtio-state.args | 17 +
>  .../qemuxml2argv-channel-virtio-state.xml  | 42 +
>  tests/qemuxml2argvtest.c   |  2 +
>  .../qemuxml2xmlout-channel-virtio-state-active.xml | 43 
> ++
>  ...emuxml2xmlout-channel-virtio-state-inactive.xml | 42 +
>  tests/qemuxml2xmltest.c|  1 +
>  10 files changed, 200 insertions(+), 6 deletions(-)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-state.args
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-state.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-state-active.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-state-inactive.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 9364eb5..229783d 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -4911,7 +4911,7 @@ qemu-kvm -net nic,model=? /dev/null
>  
>  
> path='/var/lib/libvirt/qemu/f16x86_64.agent'/>
> -  
> +   status='connected'/>

s/status/state/

>  
>  
>
> @@ -4950,7 +4950,12 @@ qemu-kvm -net nic,model=? /dev/null
>  This is very useful in case of a qemu guest agent, where users don't
>  usually care about the source path since it's libvirt who talks to
>  the guest agent. In case users want to utilize this feature, they 
> should
> -leave  element out.
> +leave  element out.  class="since">Since
> +1.2.11the active XML for a virtio channel may contain an 
> optional

s/the/ the/

> +state attribute that reflects whether a process in the
> +guest is active on the channel. This is an output-only attribute.
> +Possible values for the state attribute are
> +connected and disconnected.
>
>spicevmc
>Paravirtualized SPICE channel. The domain must also have a
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 6863ec6..9d23f3b 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3377,6 +3377,9 @@
>
>  
>
> +  
> +

I think you can be more specific and use

   
 
   connected
   disconnected
 
   

> +  
>  
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 5f4b9f6..f1c07b1 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -410,6 +410,11 @@ VIR_ENUM_IMPL(virDomainNetInterfaceLinkState, 
> VIR_DOMAIN_NET_INTERFACE_LINK_STAT
>"up",
>"down")
> 
> +VIR_ENUM_IMPL(virDomainChrDeviceState, VIR_DOMAIN_CHR_DEVICE_STATE_LAST,
> +  "default",
> +  "connected",
> +  "disconnected");
> +
>  VIR_ENUM_IMPL(virDomainChrSerialTarget,
>VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST,
>"isa-serial",
> @@ -7857,13 +7862,15 @@ virDomainChrTargetTypeFromString(virDomainChrDefPtr 
> def,
> 
>  static int
>  virDomainChrDefParseTargetXML(virDomainChrDefPtr def,
> -  xmlNodePtr cur)
> +  xmlNodePtr cur,
> +  unsigned int flags)
>  {
>  int ret = -1;
>  unsigned int port;
>  char *targetType = virXMLPropString(cur, "type");
>  char *addrStr = NULL;
>  char *portStr = NULL;
> +char *stateStr = NULL;
> 
>  if ((def->targetType =
>   virDomainChrTargetTypeFromString(def, def->deviceType,
> @@ -7920,6 +7927,20 @@ virDomainChrDefParseTargetXML(virDomainChrDefPtr def,
> 
>  case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO:
>  def->target.name = virXMLPropString(cur, "name");
> +
> +if (!(flags & VIR_DOMAIN_XML_INACTIVE) &&
> +(stateStr = virXMLPropString(cur, "state"))) {
> +int tmp;
> +
> +if ((tmp = virDomainChrDeviceStateTypeFromString(stateStr)) 
> < 0) {

You sho

[libvirt] [PATCH v3 5/6] qemu-command: use vram attribute for all video devices

[Pavel Hrdina]

So far we hadn't any option to set video memory size for qemu video
devices. There were only vram (ram for QXL) attribute but it was valid
only for QXL video device.

To provide this feature to users qemu has dedicated device attribute
called 'vgamem_mb' to set the video memory size. We will use the 'vram'
attribute also for setting video memory size for other qemu video
devices.

Only for cirrus device we will ignore the vram value because it has
hardcoded video size in QEMU.

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

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_command.c| 32 ++
 .../qemuxml2argv-video-qxl-device-vgamem.args  |  6 
 .../qemuxml2argv-video-qxl-device-vgamem.xml   | 29 
 .../qemuxml2argv-video-qxl-device.args |  6 
 .../qemuxml2argv-video-qxl-device.xml  | 29 
 .../qemuxml2argv-video-qxl-nodevice.args   |  5 
 .../qemuxml2argv-video-qxl-nodevice.xml| 29 
 .../qemuxml2argv-video-qxl-sec-device-vgamem.args  |  8 ++
 .../qemuxml2argv-video-qxl-sec-device-vgamem.xml   | 32 ++
 .../qemuxml2argv-video-qxl-sec-device.args |  7 +
 .../qemuxml2argv-video-qxl-sec-device.xml  | 32 ++
 .../qemuxml2argv-video-qxl-sec-nodevice.xml| 32 ++
 .../qemuxml2argv-video-vga-device-vgamem.args  |  6 
 .../qemuxml2argv-video-vga-device-vgamem.xml   | 29 
 .../qemuxml2argv-video-vga-device.args |  6 
 .../qemuxml2argv-video-vga-device.xml  | 29 
 .../qemuxml2argv-video-vga-nodevice.args   |  5 
 .../qemuxml2argv-video-vga-nodevice.xml| 29 
 tests/qemuxml2argvtest.c   | 16 +++
 19 files changed, 367 insertions(+)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-vgamem.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-vgamem.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-nodevice.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-nodevice.xml
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-video-qxl-sec-device-vgamem.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-video-qxl-sec-device-vgamem.xml
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-video-qxl-sec-device.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-sec-device.xml
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-video-qxl-sec-nodevice.xml
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-video-vga-device-vgamem.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-video-vga-device-vgamem.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-vga-device.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-vga-device.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-vga-nodevice.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-vga-nodevice.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0c77b57..ac36567 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5031,6 +5031,19 @@ qemuBuildDeviceVideoStr(virDomainDefPtr def,
 /* QEMU accepts bytes for vram_size. */
 virBufferAsprintf(&buf, ",vram_size=%u", video->vram * 1024);
 }
+} else if (video->vram &&
+((video->type == VIR_DOMAIN_VIDEO_TYPE_VGA &&
+  virQEMUCapsGet(qemuCaps, QEMU_CAPS_VGA_VGAMEM)) ||
+ (video->type == VIR_DOMAIN_VIDEO_TYPE_VMVGA &&
+  virQEMUCapsGet(qemuCaps, QEMU_CAPS_VMWARE_SVGA_VGAMEM {
+
+if (video->vram % 1024) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   "%s", _("value for 'vram' must be multiple of 
1024"));
+goto error;
+}
+
+virBufferAsprintf(&buf, ",vgamem_mb=%u", video->vram / 1024);
 }
 
 if (qemuBuildDeviceAddressStr(&buf, def, &video->info, qemuCaps) < 0)
@@ -9360,6 +9373,25 @@ qemuBuildCommandLine(virConnectPtr conn,
dev, vram * 1024);
 }
 }
+
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE) &&
+def->videos[0]->vram &&
+((primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VGA &&
+  virQEMUCapsGet(qemuCaps, QEMU_CAPS_VGA_VGAMEM)) ||
+ (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VMVGA &&
+  virQEMUCapsGet(qemuCaps, 
QEMU_CAPS_VMWARE_SVGA_VGAMEM {
+unsigned in

[libvirt] [PATCH v3 6/6] qemu-command: introduce new vgamem attribute for QXL video device

[Pavel Hrdina]

Add attribute to set vgamem_mb parameter of QXL device for QEMU. This
value sets the size of VGA framebuffer for QXL device. Default value in
QEMU is 8MB so reuse it also in libvirt to not break things.

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

Signed-off-by: Pavel Hrdina 
---
 docs/formatdomain.html.in  |  4 +++-
 docs/schemas/domaincommon.rng  |  5 +
 src/conf/domain_conf.c | 26 ++
 src/conf/domain_conf.h |  1 +
 src/qemu/qemu_command.c| 22 --
 src/qemu/qemu_domain.c | 18 +++
 .../qemuxml2argv-graphics-spice-compression.xml|  4 ++--
 .../qemuxml2argv-graphics-spice-qxl-vga.xml|  4 ++--
 .../qemuxml2argv-graphics-spice.xml|  4 ++--
 .../qemuxml2argv-pcihole64-q35.xml |  2 +-
 tests/qemuxml2argvdata/qemuxml2argv-q35.xml|  2 +-
 .../qemuxml2argv-serial-spiceport.xml  |  2 +-
 .../qemuxml2argv-video-qxl-device-vgamem.args  |  4 ++--
 .../qemuxml2argv-video-qxl-sec-device-vgamem.args  |  6 ++---
 tests/qemuxml2argvtest.c   |  6 +++--
 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml|  2 +-
 16 files changed, 92 insertions(+), 20 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 4e6b919..444e681 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4704,7 +4704,9 @@ qemu-kvm -net nic,model=? /dev/null
   only and specifies the size of the primary bar, while the optional
   attribute vram specifies the secondary bar size.
   If "ram" or "vram" are not supplied a default value is used. The ram
-  should also be rounded to power of two as vram.
+  should also be rounded to power of two as vram. There is also 
optional
+  attribute vgamem (since 
1.2.11)
+  to set the size of VGA framebuffer for fallback mode of QXL device.
 
   
 
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 6863ec6..569e020 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2861,6 +2861,11 @@
   
 
   
+  
+
+  
+
+  
 
   
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index cf55b15..15fd354 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10214,6 +10214,7 @@ virDomainVideoDefParseXML(xmlNodePtr node,
 char *heads = NULL;
 char *vram = NULL;
 char *ram = NULL;
+char *vgamem = NULL;
 char *primary = NULL;
 
 if (VIR_ALLOC(def) < 0)
@@ -10227,6 +10228,7 @@ virDomainVideoDefParseXML(xmlNodePtr node,
 type = virXMLPropString(cur, "type");
 ram = virXMLPropString(cur, "ram");
 vram = virXMLPropString(cur, "vram");
+vgamem = virXMLPropString(cur, "vgamem");
 heads = virXMLPropString(cur, "heads");
 
 if ((primary = virXMLPropString(cur, "primary")) != NULL) {
@@ -10280,6 +10282,19 @@ virDomainVideoDefParseXML(xmlNodePtr node,
 def->vram = virDomainVideoDefaultRAM(dom, def->type);
 }
 
+if (vgamem) {
+if (def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("vgamem attribute only supported for type of 
qxl"));
+goto error;
+}
+if (virStrToLong_ui(vgamem, NULL, 10, &def->vgamem) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("cannot parse video vgamem '%s'"), vgamem);
+goto error;
+}
+}
+
 if (heads) {
 if (virStrToLong_ui(heads, NULL, 10, &def->heads) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -10296,6 +10311,7 @@ virDomainVideoDefParseXML(xmlNodePtr node,
 VIR_FREE(type);
 VIR_FREE(ram);
 VIR_FREE(vram);
+VIR_FREE(vgamem);
 VIR_FREE(heads);
 
 return def;
@@ -10305,6 +10321,7 @@ virDomainVideoDefParseXML(xmlNodePtr node,
 VIR_FREE(type);
 VIR_FREE(ram);
 VIR_FREE(vram);
+VIR_FREE(vgamem);
 VIR_FREE(heads);
 return NULL;
 }
@@ -14610,6 +14627,13 @@ 
virDomainVideoDefCheckABIStability(virDomainVideoDefPtr src,
 return false;
 }
 
+if (src->vgamem != dst->vgamem) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Target video card vgamem %u does not match source 
%u"),
+   dst->vgamem, src->vgamem);
+return false;
+}
+
 if (src->heads != dst->heads) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Target video card heads %u does not match source 
%u"),
@@ -17977,6 +18001,8 @@ v

[libvirt] [PATCH v3 4/6] caps: introduce new QEMU capability for vgamem_mb device property

[Pavel Hrdina]

Allow setting vgamem size for video devices.

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

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_capabilities.c |  33 
 src/qemu/qemu_capabilities.h |   4 +
 tests/qemucapabilitiesdata/caps_1.2.2-1.caps |   4 +
 tests/qemucapabilitiesdata/caps_1.2.2-1.replies  | 206 -
 tests/qemucapabilitiesdata/caps_1.3.1-1.caps |   4 +
 tests/qemucapabilitiesdata/caps_1.3.1-1.replies  | 218 ++-
 tests/qemucapabilitiesdata/caps_1.4.2-1.caps |   4 +
 tests/qemucapabilitiesdata/caps_1.4.2-1.replies  | 218 ++-
 tests/qemucapabilitiesdata/caps_1.5.3-1.caps |   4 +
 tests/qemucapabilitiesdata/caps_1.5.3-1.replies  | 218 ++-
 tests/qemucapabilitiesdata/caps_1.6.0-1.caps |   4 +
 tests/qemucapabilitiesdata/caps_1.6.0-1.replies  | 218 ++-
 tests/qemucapabilitiesdata/caps_1.6.50-1.caps|   4 +
 tests/qemucapabilitiesdata/caps_1.6.50-1.replies | 218 ++-
 tests/qemucapabilitiesdata/caps_2.1.1-1.caps |   4 +
 tests/qemucapabilitiesdata/caps_2.1.1-1.replies  | 218 ++-
 16 files changed, 1530 insertions(+), 49 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 56bd2d5..d2c046d 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -272,6 +272,11 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
   "migrate-rdma",
   "ivshmem",
   "drive-iotune-max",
+
+  "VGA.vgamem_mb", /* 180 */
+  "vmware-svga.vgamem_mb",
+  "qxl.vgamem_mb",
+  "qxl-vga.vgamem_mb",
 );
 
 
@@ -1581,6 +1586,22 @@ static struct virQEMUCapsStringFlags 
virQEMUCapsObjectPropsKVMPit[] = {
 { "lost_tick_policy", QEMU_CAPS_KVM_PIT_TICK_POLICY },
 };
 
+static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVGA[] = {
+{ "vgamem_mb", QEMU_CAPS_VGA_VGAMEM },
+};
+
+static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVmwareSvga[] = {
+{ "vgamem_mb", QEMU_CAPS_VMWARE_SVGA_VGAMEM },
+};
+
+static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsQxl[] = {
+{ "vgamem_mb", QEMU_CAPS_QXL_VGAMEM },
+};
+
+static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsQxlVga[] = {
+{ "vgamem_mb", QEMU_CAPS_QXL_VGA_VGAMEM },
+};
+
 struct virQEMUCapsObjectTypeProps {
 const char *type;
 struct virQEMUCapsStringFlags *props;
@@ -1626,6 +1647,14 @@ static struct virQEMUCapsObjectTypeProps 
virQEMUCapsObjectProps[] = {
   ARRAY_CARDINALITY(virQEMUCapsObjectPropsUSBStorage) },
 { "kvm-pit", virQEMUCapsObjectPropsKVMPit,
   ARRAY_CARDINALITY(virQEMUCapsObjectPropsKVMPit) },
+{ "VGA", virQEMUCapsObjectPropsVGA,
+  ARRAY_CARDINALITY(virQEMUCapsObjectPropsVGA) },
+{ "vmware-svga", virQEMUCapsObjectPropsVmwareSvga,
+  ARRAY_CARDINALITY(virQEMUCapsObjectPropsVmwareSvga) },
+{ "qxl", virQEMUCapsObjectPropsQxl,
+  ARRAY_CARDINALITY(virQEMUCapsObjectPropsQxl) },
+{ "qxl-vga", virQEMUCapsObjectPropsQxlVga,
+  ARRAY_CARDINALITY(virQEMUCapsObjectPropsQxlVga) },
 };
 
 
@@ -1817,6 +1846,10 @@ virQEMUCapsExtractDeviceStr(const char *qemu,
  "-device", "usb-host,?",
  "-device", "scsi-generic,?",
  "-device", "usb-storage,?",
+ "-device", "VGA,?",
+ "-device", "vmware-svga,?",
+ "-device", "qxl,?",
+ "-device", "qxl-vga,?",
  NULL);
 /* qemu -help goes to stdout, but qemu -device ? goes to stderr.  */
 virCommandSetErrorBuffer(cmd, &output);
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index ffe3494..c5542d1 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -219,6 +219,10 @@ typedef enum {
 QEMU_CAPS_MIGRATE_RDMA   = 177, /* have rdma migration */
 QEMU_CAPS_DEVICE_IVSHMEM = 178, /* -device ivshmem */
 QEMU_CAPS_DRIVE_IOTUNE_MAX   = 179, /* -drive bps_max= and friends */
+QEMU_CAPS_VGA_VGAMEM = 180, /* -device VGA.vgamem_mb */
+QEMU_CAPS_VMWARE_SVGA_VGAMEM = 181, /* -device vmware-svga.vgamem_mb */
+QEMU_CAPS_QXL_VGAMEM = 182, /* -device qxl.vgamem_mb */
+QEMU_CAPS_QXL_VGA_VGAMEM = 183, /* -device qxl-vga.vgamem_mb */
 
 QEMU_CAPS_LAST,   /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_1.2.2-1.caps 
b/tests/qemucapabilitiesdata/caps_1.2.2-1.caps
index fc8dfc1..30239df 100644
--- a/tests/qemucapabilitiesdata/caps_1.2.2-1.caps
+++ b/tests/qemucapabilitiesdata/caps_1.2.2-1.caps
@@ -116,4 +116,8 @@
 
 
 
+
+
+
+
   
diff --git a/tests/qemucapabilitiesdata/caps_1.2.2-1.replies 
b/tests/qemucapabilitiesd

[libvirt] [PATCH v3 1/6] internal: add macro to round value to the next closest power of 2

[Pavel Hrdina]

There are two special cases, if the input number is 0 or the number is
larger then 2^31 (for 32bit unsigned int). For the special cases the
return value is 0 because they cannot be rounded.

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

Signed-off-by: Pavel Hrdina 
---
 bootstrap.conf   |  1 +
 src/internal.h   |  7 +++
 tests/utiltest.c | 39 +++
 3 files changed, 47 insertions(+)

diff --git a/bootstrap.conf b/bootstrap.conf
index d24a714..e7ea9c9 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -35,6 +35,7 @@ clock-time
 close
 connect
 configmake
+count-leading-zeros
 count-one-bits
 crypto/md5
 crypto/sha256
diff --git a/src/internal.h b/src/internal.h
index f6a88b2..765b853 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -62,6 +62,7 @@
 
 # include "c-strcase.h"
 # include "ignore-value.h"
+# include "count-leading-zeros.h"
 
 /* On architectures which lack these limits, define them (ie. Cygwin).
  * Note that the libvirt code should be robust enough to handle the
@@ -382,6 +383,12 @@
 /* round up value to the closest multiple of size */
 # define VIR_ROUND_UP(value, size) (VIR_DIV_UP(value, size) * (size))
 
+/* Round up to the next closest power of 2. It will return rounded number or 0
+ * for 0 or number more than 2^31 (for 32bit unsigned int). */
+# define VIR_ROUND_UP_POWER_OF_TWO(value)   \
+((value) > 0 && (value) <= 1U<<(sizeof(unsigned int)*8 - 1) ?   \
+ 1U<<(sizeof(unsigned int)*8 - count_leading_zeros((value) - 1)) : 0)
+
 
 /* Specific error values for use in forwarding programs such as
  * virt-login-shell; these values match what GNU env does.  */
diff --git a/tests/utiltest.c b/tests/utiltest.c
index 8950cf2..2dbd291 100644
--- a/tests/utiltest.c
+++ b/tests/utiltest.c
@@ -143,6 +143,44 @@ testParseVersionString(const void *data ATTRIBUTE_UNUSED)
 
 
 
+struct testRoundData {
+unsigned int input;
+unsigned int output;
+};
+
+static struct testRoundData roundData[] = {
+{ 0, 0 },
+{ 1, 1 },
+{ 1000, 1024 },
+{ 1024, 1024 },
+{ 1025, 2048 },
+{ UINT_MAX, 0 },
+};
+
+static int
+testRoundValueToPowerOfTwo(const void *data ATTRIBUTE_UNUSED)
+{
+unsigned int result;
+size_t i;
+
+for (i = 0; i < ARRAY_CARDINALITY(roundData); i++) {
+result = VIR_ROUND_UP_POWER_OF_TWO(roundData[i].input);
+if (roundData[i].output != result) {
+if (virTestGetDebug() > 0) {
+fprintf(stderr, "\nInput number [%u]\n", roundData[i].input);
+fprintf(stderr, "Expected number [%u]\n", roundData[i].output);
+fprintf(stderr, "Actual number [%u]\n", result);
+}
+
+return -1;
+}
+}
+
+return 0;
+}
+
+
+
 
 static int
 mymain(void)
@@ -162,6 +200,7 @@ mymain(void)
 DO_TEST(IndexToDiskName);
 DO_TEST(DiskNameToIndex);
 DO_TEST(ParseVersionString);
+DO_TEST(RoundValueToPowerOfTwo);
 
 return result == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
-- 
2.0.4

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

[libvirt] [PATCH v3 3/6] QXL: fix setting ram and vram values for QEMU QXL device

[Pavel Hrdina]

QEMU has two different type of QXL display device. The first "qxl-vga"
is for primary video device and second "qxl" is for secondary video
device.

There are also two different ways how to specify those devices on qemu
command line, the first one and obsolete is using "-vga" option and the
current new one is using "-device" option. The "-vga" could be used only
to setup primary video device, so the "-vga qxl" equal to
"-device qxl-vga". Unfortunately the "-vga qxl" doesn't support setting
additional parameters for the device and "-global" option must be used
for this purpose. It's mandatory to use "-global qxl-vga" to set the
parameters of primary video device previously defined with "-vga qxl".

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

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_command.c | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-compression.args | 2 +-
 tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-sasl.args| 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args | 4 ++--
 tests/qemuxml2argvdata/qemuxml2argv-pcihole64-q35.args  | 2 +-
 tests/qemuxml2argvdata/qemuxml2argv-q35.args| 2 +-
 6 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0392f11..0c77b57 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9328,11 +9328,11 @@ qemuBuildCommandLine(virConnectPtr conn,
 
 virCommandAddArgList(cmd, "-vga", vgastr, NULL);
 
+const char *dev = 
qemuDeviceVideoTypeToString(primaryVideoType);
+
 if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_QXL &&
 (def->videos[0]->vram || def->videos[0]->ram) &&
 virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
-const char *dev = (virQEMUCapsGet(qemuCaps, 
QEMU_CAPS_DEVICE_QXL_VGA)
-   ? "qxl-vga" : "qxl");
 unsigned int ram = def->videos[0]->ram;
 unsigned int vram = def->videos[0]->vram;
 
diff --git 
a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-compression.args 
b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-compression.args
index cdc916c..e08ee20 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-compression.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-compression.args
@@ -6,6 +6,6 @@ x509-dir=/etc/pki/libvirt-spice,\
 image-compression=auto_glz,jpeg-wan-compression=auto,\
 zlib-glz-wan-compression=auto,\
 playback-compression=on,streaming-video=filter -vga \
-qxl -global qxl.ram_size=67108864 -global qxl.vram_size=33554432 \
+qxl -global qxl-vga.ram_size=67108864 -global qxl-vga.vram_size=33554432 \
 -device qxl,id=video1,ram_size=67108864,vram_size=33554432,bus=pci.0,addr=0x4 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-sasl.args 
b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-sasl.args
index 0c9df16..4f7f09b 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-sasl.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-sasl.args
@@ -5,5 +5,5 @@ SASL_CONF_PATH=/root/.sasl2 QEMU_AUDIO_DRV=spice \
 /dev/HostVG/QEMUGuest1 \
 -spice port=5903,tls-port=5904,sasl,addr=127.0.0.1,\
 x509-dir=/etc/pki/libvirt-spice,tls-channel=default \
--vga qxl -global qxl.ram_size=67108864 -global \
-qxl.vram_size=33554432 -device 
virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
+-vga qxl -global qxl-vga.ram_size=67108864 -global qxl-vga.vram_size=33554432 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args 
b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args
index 704cec7..97755c3 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args
@@ -7,7 +7,7 @@ plaintext-channel=inputs,\
 image-compression=auto_glz,jpeg-wan-compression=auto,\
 zlib-glz-wan-compression=auto,\
 playback-compression=on,streaming-video=filter,disable-copy-paste,\
-disable-agent-file-xfer -vga qxl -global qxl.ram_size=67108864 \
--global qxl.vram_size=33554432 \
+disable-agent-file-xfer -vga qxl -global qxl-vga.ram_size=67108864 \
+-global qxl-vga.vram_size=33554432 \
 -device qxl,id=video1,ram_size=67108864,vram_size=33554432,bus=pci.0,addr=0x4 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcihole64-q35.args 
b/tests/qemuxml2argvdata/qemuxml2argv-pcihole64-q35.args
index c8045a4..cd2ccca 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-pcihole64-q35.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pcihole64-q35.args
@@ -6,4 +6,4 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test 
QEMU_AUDIO_DRV=none \
 -device pci-bridge,chas

[libvirt] [PATCH v3 0/6] improve setting video memory

[Pavel Hrdina]

As it was pointed out that QEMU needs that the video memory size should be
rounded to power of two this patch series introduces new macro to round number
to power of two. There are two special cases (more description in the commit
message).

This patch series also fixes few issues that I've found with using vram
attribute and possible some bugs (for example if you set vram for QXL to 0, we
pass the 0 to QEMU which is wrong and we shouldn't pass anything).

There were also wrong usage of primary 'qxl-vga' video device and secondary
'qxl' video device. We've been setting wrong primary/secondary parameters.

The main propose is to start using the vram attribute for other QEMU video
devices next to QXL and also introduce vgamem attribute to set the VGA
framebuffer for QXL video device.

v2: https://www.redhat.com/archives/libvir-list/2014-November/msg00525.html
v1: https://www.redhat.com/archives/libvir-list/2014-November/msg00024.html

Pavel Hrdina (6):
  internal: add macro to round value to the next closest power of 2
  video: cleanup usage of vram attribute and update documentation
  QXL: fix setting ram and vram values for QEMU QXL device
  caps: introduce new QEMU capability for vgamem_mb device property
  qemu-command: use vram attribute for all video devices
  qemu-command: introduce new vgamem attribute for QXL video device

 bootstrap.conf |   1 +
 docs/formatdomain.html.in  |  68 ---
 docs/schemas/domaincommon.rng  |   5 +
 src/conf/domain_conf.c |  41 +++-
 src/conf/domain_conf.h |   4 +-
 src/internal.h |   7 +
 src/qemu/qemu_capabilities.c   |  33 
 src/qemu/qemu_capabilities.h   |   4 +
 src/qemu/qemu_command.c|  74 ++-
 src/qemu/qemu_domain.c |  18 ++
 src/xen/xen_driver.c   |   2 +-
 tests/qemucapabilitiesdata/caps_1.2.2-1.caps   |   4 +
 tests/qemucapabilitiesdata/caps_1.2.2-1.replies| 206 ++-
 tests/qemucapabilitiesdata/caps_1.3.1-1.caps   |   4 +
 tests/qemucapabilitiesdata/caps_1.3.1-1.replies| 218 -
 tests/qemucapabilitiesdata/caps_1.4.2-1.caps   |   4 +
 tests/qemucapabilitiesdata/caps_1.4.2-1.replies| 218 -
 tests/qemucapabilitiesdata/caps_1.5.3-1.caps   |   4 +
 tests/qemucapabilitiesdata/caps_1.5.3-1.replies| 218 -
 tests/qemucapabilitiesdata/caps_1.6.0-1.caps   |   4 +
 tests/qemucapabilitiesdata/caps_1.6.0-1.replies| 218 -
 tests/qemucapabilitiesdata/caps_1.6.50-1.caps  |   4 +
 tests/qemucapabilitiesdata/caps_1.6.50-1.replies   | 218 -
 tests/qemucapabilitiesdata/caps_2.1.1-1.caps   |   4 +
 tests/qemucapabilitiesdata/caps_2.1.1-1.replies| 218 -
 ...qemuhotplug-console-compat-2+console-virtio.xml |   2 +-
 .../qemuxml2argv-console-compat-2.xml  |   2 +-
 .../qemuxml2argv-controller-order.xml  |   2 +-
 .../qemuxml2argv-graphics-listen-network.xml   |   2 +-
 .../qemuxml2argv-graphics-listen-network2.xml  |   2 +-
 .../qemuxml2argv-graphics-sdl-fullscreen.xml   |   2 +-
 .../qemuxml2argvdata/qemuxml2argv-graphics-sdl.xml |   2 +-
 .../qemuxml2argv-graphics-spice-agentmouse.xml |   2 +-
 .../qemuxml2argv-graphics-spice-compression.args   |   2 +-
 .../qemuxml2argv-graphics-spice-compression.xml|   4 +-
 .../qemuxml2argv-graphics-spice-qxl-vga.xml|   4 +-
 .../qemuxml2argv-graphics-spice-sasl.args  |   4 +-
 .../qemuxml2argv-graphics-spice-sasl.xml   |   2 +-
 .../qemuxml2argv-graphics-spice-timeout.xml|   2 +-
 .../qemuxml2argv-graphics-spice.args   |   4 +-
 .../qemuxml2argv-graphics-spice.xml|   4 +-
 .../qemuxml2argv-graphics-vnc-policy.xml   |   2 +-
 .../qemuxml2argv-graphics-vnc-sasl.xml |   2 +-
 .../qemuxml2argv-graphics-vnc-socket.xml   |   2 +-
 .../qemuxml2argv-graphics-vnc-tls.xml  |   2 +-
 .../qemuxml2argv-graphics-vnc-websocket.xml|   2 +-
 .../qemuxml2argvdata/qemuxml2argv-graphics-vnc.xml |   2 +-
 .../qemuxml2argv-net-bandwidth.xml |   2 +-
 .../qemuxml2argv-pci-autoadd-addr.xml  |   2 +-
 .../qemuxml2argv-pci-autoadd-idx.xml   |   2 +-
 tests/qemuxml2argvdata/qemuxml2argv-pci-bridge.xml |   2 +-
 .../qemuxml2argv-pcihole64-q35.args|   2 +-
 .../qemuxml2argv-pcihole64-q35.xml |   2 +-
 .../qemuxml2argvdata/qemuxml2argv-pseries-disk.xml |   2 +-
 tests/qemuxml2argvdata/qemuxml2argv-q35.args   |   2 +-
 tests/qemuxml2argvdata/qemuxml2argv-q35.xml|   2 +-
 .../qemuxml2argv-serial-spiceport.args |   2 +-
 .../qemuxml2argv-serial-spiceport.x

[libvirt] [PATCH v3 2/6] video: cleanup usage of vram attribute and update documentation

[Pavel Hrdina]

The vram attribute was introduced to set the video memory but it is
usable only for few hypervisors excluding QEMU/KVM and the old XEN
driver. Only in case of QEMU the vram was used for QXL.

This patch updates the documentation to reflect current code in libvirt
and also changes the cases when we will set the default vram attribute.
It also fixes existing strange default value for VGA devices 9MB to 16MB
because the video ram should be rounded to power of two.

The change of default value could affect migrations but I found out that
QEMU always round the video ram to power of two internally so it's safe
to change the default value to the next closest power of two and also
silently correct every domain XML definition. And it's also safe because
we don't pass the value to QEMU.

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

Signed-off-by: Pavel Hrdina 
---
 docs/formatdomain.html.in  | 66 ++
 src/conf/domain_conf.c | 15 +++--
 src/conf/domain_conf.h |  3 +-
 src/qemu/qemu_command.c| 16 --
 src/xen/xen_driver.c   |  2 +-
 ...qemuhotplug-console-compat-2+console-virtio.xml |  2 +-
 .../qemuxml2argv-console-compat-2.xml  |  2 +-
 .../qemuxml2argv-controller-order.xml  |  2 +-
 .../qemuxml2argv-graphics-listen-network.xml   |  2 +-
 .../qemuxml2argv-graphics-listen-network2.xml  |  2 +-
 .../qemuxml2argv-graphics-sdl-fullscreen.xml   |  2 +-
 .../qemuxml2argvdata/qemuxml2argv-graphics-sdl.xml |  2 +-
 .../qemuxml2argv-graphics-spice-agentmouse.xml |  2 +-
 .../qemuxml2argv-graphics-spice-compression.args   |  2 +-
 .../qemuxml2argv-graphics-spice-compression.xml|  2 +-
 .../qemuxml2argv-graphics-spice-sasl.args  |  2 +-
 .../qemuxml2argv-graphics-spice-sasl.xml   |  2 +-
 .../qemuxml2argv-graphics-spice-timeout.xml|  2 +-
 .../qemuxml2argv-graphics-spice.args   |  2 +-
 .../qemuxml2argv-graphics-spice.xml|  2 +-
 .../qemuxml2argv-graphics-vnc-policy.xml   |  2 +-
 .../qemuxml2argv-graphics-vnc-sasl.xml |  2 +-
 .../qemuxml2argv-graphics-vnc-socket.xml   |  2 +-
 .../qemuxml2argv-graphics-vnc-tls.xml  |  2 +-
 .../qemuxml2argv-graphics-vnc-websocket.xml|  2 +-
 .../qemuxml2argvdata/qemuxml2argv-graphics-vnc.xml |  2 +-
 .../qemuxml2argv-net-bandwidth.xml |  2 +-
 .../qemuxml2argv-pci-autoadd-addr.xml  |  2 +-
 .../qemuxml2argv-pci-autoadd-idx.xml   |  2 +-
 tests/qemuxml2argvdata/qemuxml2argv-pci-bridge.xml |  2 +-
 .../qemuxml2argv-pcihole64-q35.args|  2 +-
 .../qemuxml2argv-pcihole64-q35.xml |  2 +-
 .../qemuxml2argvdata/qemuxml2argv-pseries-disk.xml |  2 +-
 tests/qemuxml2argvdata/qemuxml2argv-q35.args   |  2 +-
 tests/qemuxml2argvdata/qemuxml2argv-q35.xml|  2 +-
 .../qemuxml2argv-serial-spiceport.args |  2 +-
 .../qemuxml2argv-serial-spiceport.xml  |  2 +-
 .../qemuxml2xmlout-graphics-listen-network2.xml|  2 +-
 .../qemuxml2xmlout-graphics-spice-timeout.xml  |  2 +-
 .../qemuxml2xmlout-pci-autoadd-addr.xml|  2 +-
 .../qemuxml2xmlout-pci-autoadd-idx.xml |  2 +-
 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml|  2 +-
 tests/virt-aa-helper-test  |  2 +-
 43 files changed, 103 insertions(+), 75 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 9364eb5..4e6b919 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4651,7 +4651,7 @@ qemu-kvm -net nic,model=? /dev/null
   ...
   
 
@@ -4661,33 +4661,51 @@ qemu-kvm -net nic,model=? /dev/null
 
   video
   
-The video element is the container for describing
-video devices. For backwards compatibility, if no video
-is set but there is a graphics in domain xml, then libvirt
-will add a default video according to the guest type.
-For a guest of type "kvm", the default video for it is:
-type with value "cirrus", vram with value
-"9216", and heads with value "1". By default, the first
-video device in domain xml is the primary one, but the optional
-attribute primary (since 1.0.2)
-with value 'yes' can be used to mark the primary in cases of multiple
-video device. The non-primary must be type of "qxl". The optional
-attribute ram (since
-1.0.2) is allowed for "qxl" type only and specifies
-the size of the primary bar, while vram specifies the
-secondary bar size.  If "ram" or "vram" are not supplied a default

Re: [libvirt] [PATCH v2 0/5] Guest filesystem information API

[Eric Blake]

On 11/20/2014 05:33 AM, Michal Privoznik wrote:

>> I'm also hoping someone else (eblake?) can look at the remote_protocol.x
>> changes to ensure they encompass everything they are supposed to.  Also
>> that the usage of QEMU_JOB_QUERY not _MODIFY for the GetFSInfo seems
>> more appropriate and is in line with the various remote_protocol.x
>> settings (@acl/@generate stuff settings).
> 
> 
> @generate is correct, since both, client and server implementations are
> provided.
> @acl looks consistent to the rest. Correct, for querying domain info you
> need to have read permission and that's it.

Oh, wait.  This is an interaction with the guest agent.  We have already
stated that ANY action that requires guest cooperation MUST require more
than plain domain:read privileges (for example, creating a snapshot
requires domain:fs_freeze if the quiesce flag is present; using
virDomainShutdownFlags requires domain:write if the guest agent is
involved).

Since the main use of this API is to query the list of mountpoints that
then feed virDomainFSFreeze, I think this should be @acl
domain:fs_freeze, rather than domain:read.  Even if it is a read-only
operation, it makes more sense to treat this command as a family where a
user is either given rights for all related freeze APIs or none of them.

-- 
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 0/5] Guest filesystem information API

[Eric Blake]

On 11/19/2014 03:58 PM, John Ferlan wrote:
> 
> 
> On 11/17/2014 06:26 PM, Tomoki Sekiyama wrote:
>> Hi,
>>
>> This is v2 of patchset to add virDomainGetFSInfo API.
>>
>> * changes in v1->v2:
>>  -[all] removed redundant NULL element at the last of returned info array
>>  -[3/5] make error messages in qemu_agent.c consistent with other commands
>>  -[4/5] added a test case for 2 items in info->devAliases
>>  -[5/5] added a pod document for virsh domfsinfo command
>>  (v1: http://www.redhat.com/archives/libvir-list/2014-October/msg1.html )
>>

> I reviewed the 'libvirt' specific changes - had a few comments and have
> made changes to my review branch as specified.  As long as you're OK
> with those changes I will get these pushed.

I think it may be worth a v3 if you like my suggestion on 1/5 of
providing a count argument for the length of the alias array, rather
than forcing the clients to crawl the array themselves to find that length.

> 
> I'm also hoping someone else (eblake?) can look at the remote_protocol.x
> changes to ensure they encompass everything they are supposed to.  Also
> that the usage of QEMU_JOB_QUERY not _MODIFY for the GetFSInfo seems
> more appropriate and is in line with the various remote_protocol.x
> settings (@acl/@generate stuff settings).

On my cursory glance, and reading Michal's review, yes, it looks like
that part was done right.

-- 
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 2/5] remote: Implement the remote protocol for virDomainGetFSInfo

[Eric Blake]

On 11/17/2014 04:26 PM, Tomoki Sekiyama wrote:
> Add daemon and driver code to (de-)serialize virDomainFSInfo.
> 
> Signed-off-by: Tomoki Sekiyama 
> ---
>  daemon/remote.c  |  117 
> ++
>  src/remote/remote_driver.c   |   92 +
>  src/remote/remote_protocol.x |   32 +++
>  src/remote_protocol-structs  |   21 
>  src/rpc/gendispatch.pl   |1 
>  5 files changed, 262 insertions(+), 1 deletion(-)
> 
> diff --git a/daemon/remote.c b/daemon/remote.c
> index 1d7082e..9b89fd0 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -6336,6 +6336,123 @@ remoteDispatchNodeAllocPages(virNetServerPtr server 
> ATTRIBUTE_UNUSED,
>  }
>  
>  
> +static int
> +remoteDispatchDomainGetFSInfo(virNetServerPtr server ATTRIBUTE_UNUSED,
> +  virNetServerClientPtr client,
> +  virNetMessagePtr msg ATTRIBUTE_UNUSED,
> +  virNetMessageErrorPtr rerr,
> +  remote_domain_get_fsinfo_args *args,
> +  remote_domain_get_fsinfo_ret *ret)

Are we sure we have to write this by hand? [1]


> @@ -8171,6 +8262,7 @@ static virHypervisorDriver hypervisor_driver = {
>  .connectGetDomainCapabilities = remoteConnectGetDomainCapabilities, /* 
> 1.2.7 */
>  .connectGetAllDomainStats = remoteConnectGetAllDomainStats, /* 1.2.8 */
>  .nodeAllocPages = remoteNodeAllocPages, /* 1.2.9 */
> +.domainGetFSInfo = remoteDomainGetFSInfo, /* 1.2.10 */

1.2.10 is wrong; the next release is 1.2.11.

>  };
>  
>  static virNetworkDriver network_driver = {
> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index ebf4530..10c8068 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -250,6 +250,12 @@ const REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX = 4096;
>  /* Upper limit of message size for tunable event. */
>  const REMOTE_DOMAIN_EVENT_TUNABLE_MAX = 2048;
>  
> +/* Upper limit on number of mountpoints in fsinfo */
> +const REMOTE_DOMAIN_FSINFO_MAX = 256;
> +
> +/* Upper limit on number of disks per mountpoint in fsinfo */
> +const REMOTE_DOMAIN_FSINFO_DISKS_MAX = 256;
> +
>  /* UUID.  VIR_UUID_BUFLEN definition comes from libvirt.h */
>  typedef opaque remote_uuid[VIR_UUID_BUFLEN];
>  
> @@ -3111,6 +3117,24 @@ struct remote_connect_get_all_domain_stats_args {
>  struct remote_connect_get_all_domain_stats_ret {
>  remote_domain_stats_record retStats;
>  };
> +
> +struct remote_domain_fsinfo {
> +remote_nonnull_string mountpoint;
> +remote_nonnull_string name;
> +remote_nonnull_string type;
> +remote_nonnull_string dev_aliases; /* 
> (const char **) */

Can any of these values ever be NULL because the guest didn't provide
them?  If so, you want remote_string instead of remote_nonnull_string.
It wasn't obvious to me in the 1/5 documentation whether values are
guaranteed to be non-NULL.


> +
> +/**
> + * @generate: none
> + * @acl: domain:read
> + */
> +REMOTE_PROC_DOMAIN_GET_FSINFO = 348

[1] Did you try any other values of @generate to see if things get
transferred correctly without writing it by hand?  Then again, looking
at the structure you are transferring, it consists of mallocing an array
of structures which themselves malloc an array of names, so I guess you
are right that you have to manage it by hand (the generator probably
doesn't do that).

-- 
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 11/11] qemu: Emit the guest agent lifecycle event

[Peter Krempa]

On 11/20/14 20:01, Eric Blake wrote:
> On 11/19/2014 03:23 AM, Peter Krempa wrote:
>> Add code to emit the event on change of the channel state and reconnect
>> to the qemu process.
>> ---
>>  src/qemu/qemu_driver.c  |  5 +
>>  src/qemu/qemu_process.c | 13 ++---
>>  2 files changed, 15 insertions(+), 3 deletions(-)
>>
> 
>> @@ -4369,6 +4370,10 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
>>  dev.data.chr->targetType != 
>> VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO)
>>  goto endjob;
>>
>> +if (STREQ_NULLABLE(dev.data.chr->target.name, "org.qemu.guest_agent.0") 
>> &&
>> +(event = virDomainEventAgentLifecycleNewFromObj(vm, newstate, 0)))
>> +qemuDomainEventQueue(driver, event);
>> +
> 
> See my comments on 9/11 - I think that filtering this event to just the
> org.qemu.guest_agent.0 channel is wrong.  That may be the only channel
> that libvirt specifically cares about for tracking whether we can send
> guest agent commands, but it is not the only channel that management may
> care about.  In fact, naming it virDomainEventAgentLifecycle* may be
> misleading; isn't it really about virDomainEventChannelLifecycle (where
> guest-agent is just one use of a channel)?
> 

I specifically wanted to shoot for agent events as they may be used also
in a different way than just connect/disconnect. If we want to report
state of channels too I have a partially finished patch that allows to
do that.

The callback prototype then needs to be different as we need to pass
also the channel name and possibly other data to identify the channel
(and/or serial port? )

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 09/11] event: Add guest agent lifecycle event

[Eric Blake]

On 11/19/2014 03:23 AM, Peter Krempa wrote:
> As qemu is now able to notify us about change of the channel state used
> for communication with the guest agent we now can more precisely track
> the state of the guest agent.
> 
> To allow notifying management apps this patch implements a new event
> that will be triggered on changes of the guest agent state.
> ---

Actually, after seeing patch 11, I think this event is too narrow.  You
are only reporting if the guest agent on org.qemu.guest_agent.0 changes
state.  But a user can provide more than one channel, and the
VSERPORT_CHANGE event works for every channel.  I think we need to
broaden the definition of the event to allow the event on EVERY virtio
channel, and include the name of _which_ channel had the event, so that
someone using any other channel can also tell if their custom code in
the guest is properly connecting to that channel.


> +/**
> + * virConnectDomainEventAgentLifecycleCallback:
> + * @conn: connection object
> + * @dom: domain on which the event occurred
> + * @state: new state of the guest agent, one of 
> virConnectDomainEventAgentLifecycleState
> + * @reason: reason for state change; currently only 0 is passed denoting 
> change
> + *  in the guest agent socket state
> + * @opaque: application specified data
> + *
> + * This callback occurs when libvirt detects a change in the state of a guest
> + * agent.
> + *
> + * The callback signature to use when registering for an event of type
> + * VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE with 
> virConnectDomainEventRegisterAny()
> + */
> +typedef void (*virConnectDomainEventAgentLifecycleCallback)(virConnectPtr 
> conn,
> +virDomainPtr dom,
> +int state,
> +int reason,
> +void *opaque);

Thus, I think this signature needs an additional const char *channel
name, which provides the name of which agent channel is changing.

> +++ b/src/remote/remote_protocol.x
> @@ -3108,6 +3108,14 @@ struct remote_connect_get_all_domain_stats_args {
>  unsigned int flags;
>  };
> 
> +struct remote_domain_event_callback_agent_lifecycle_msg {
> +int callbackID;
> +remote_nonnull_domain dom;
> +
> +int state;
> +int reason;
> +};

Similarly, you'll need a remote_nonnull_string as the channel name as
part of this RPC call.

-- 
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/5] Implement public API for virDomainGetFSInfo

[Eric Blake]

On 11/17/2014 04:26 PM, Tomoki Sekiyama wrote:
> virDomainGetFSInfo returns a list of filesystems information mounted in the
> guest, which contains mountpoints, device names, filesystem types, and
> device aliases named by libvirt. This will be useful, for example, to
> specify mountpoints to fsfreeze when taking snapshot of a part of disks.
> 
> Signed-off-by: Tomoki Sekiyama 
> ---
>  include/libvirt/libvirt-domain.h |   21 
>  src/driver-hypervisor.h  |6 +++
>  src/libvirt.c|   66 
> ++
>  src/libvirt_public.syms  |6 +++
>  4 files changed, 99 insertions(+)
> 

> +++ b/include/libvirt/libvirt-domain.h
> @@ -3456,6 +3456,27 @@ int virDomainFSThaw(virDomainPtr dom,
>  unsigned int nmountpoints,
>  unsigned int flags);
>  
> +/**
> + * virDomainFSInfo:
> + *
> + * The data structure containing mounted file systems within a guset
> + *
> + */
> +typedef struct _virDomainFSInfo virDomainFSInfo;
> +typedef virDomainFSInfo *virDomainFSInfoPtr;
> +struct _virDomainFSInfo {
> +char *mountpoint; /* path to mount point */
> +char *name;   /* device name in the guest (e.g. "sda1") */
> +char *type;   /* filesystem type */
> +char **devAlias;  /* NULL-terminated array of disk device aliases */
> +};

Is it worth also having a size_t ndevAlias that says how long the array
is?  It may make client life easier if they have an up-front count.

-- 
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 09/11] event: Add guest agent lifecycle event

[Peter Krempa]

On 11/20/14 19:54, Eric Blake wrote:
> On 11/19/2014 03:23 AM, Peter Krempa wrote:
>> As qemu is now able to notify us about change of the channel state used
>> for communication with the guest agent we now can more precisely track
>> the state of the guest agent.
>>
>> To allow notifying management apps this patch implements a new event
>> that will be triggered on changes of the guest agent state.
>> ---
>>  daemon/remote.c  | 36 +++
>>  include/libvirt/libvirt-domain.h | 28 +++
>>  src/conf/domain_event.c  | 78 
>> 
>>  src/conf/domain_event.h  |  9 +
>>  src/libvirt_private.syms |  2 ++
>>  src/remote/remote_driver.c   | 31 
>>  src/remote/remote_protocol.x | 16 -
>>  src/remote_protocol-structs  |  7 
>>  tools/virsh-domain.c | 40 +
>>  9 files changed, 246 insertions(+), 1 deletion(-)
>>
> 
>> +++ b/include/libvirt/libvirt-domain.h
>> @@ -3332,6 +3332,33 @@ typedef void 
>> (*virConnectDomainEventTunableCallback)(virConnectPtr conn,
>>   void *opaque);
>>
>>
>> +typedef enum {
>> +VIR_CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_CONNECTED = 1, /* agent 
>> connected */
>> +VIR_CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_DISCONNECTED = 2, /* agent 
>> disconnected */
>> +} virConnectDomainEventAgentLifecycleState;
> 
> Do we want to explicitly reserve 0 for unknown state?  Do we want to
> provide a _LAST flag so that additions of future states can be detected
> by the increase in the value of _LAST?

Hmm, the 0 value also might be treated as
VIR_CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_REASON_PORT_STATUS or similar
denoting that the reason is that the socket state changed. This might
then change if we detect that the agent sent garbage or other stuff.

Providing an enum for this may be a way then.

Peter

> 
> Everything else looks good.
> 




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 11/11] qemu: Emit the guest agent lifecycle event

[Eric Blake]

On 11/19/2014 03:23 AM, Peter Krempa wrote:
> Add code to emit the event on change of the channel state and reconnect
> to the qemu process.
> ---
>  src/qemu/qemu_driver.c  |  5 +
>  src/qemu/qemu_process.c | 13 ++---
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 

> @@ -4369,6 +4370,10 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
>  dev.data.chr->targetType != 
> VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO)
>  goto endjob;
> 
> +if (STREQ_NULLABLE(dev.data.chr->target.name, "org.qemu.guest_agent.0") 
> &&
> +(event = virDomainEventAgentLifecycleNewFromObj(vm, newstate, 0)))
> +qemuDomainEventQueue(driver, event);
> +

See my comments on 9/11 - I think that filtering this event to just the
org.qemu.guest_agent.0 channel is wrong.  That may be the only channel
that libvirt specifically cares about for tracking whether we can send
guest agent commands, but it is not the only channel that management may
care about.  In fact, naming it virDomainEventAgentLifecycle* may be
misleading; isn't it really about virDomainEventChannelLifecycle (where
guest-agent is just one use of a channel)?

-- 
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 10/11] examples: Add support for the guest agent lifecycle event

[Eric Blake]

On 11/19/2014 03:23 AM, Peter Krempa wrote:
> Add code to support the event in the object-event example.
> ---
>  examples/object-events/event-test.c | 43 
> -
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 

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] [PATCH 09/11] event: Add guest agent lifecycle event

[Eric Blake]

On 11/19/2014 03:23 AM, Peter Krempa wrote:
> As qemu is now able to notify us about change of the channel state used
> for communication with the guest agent we now can more precisely track
> the state of the guest agent.
> 
> To allow notifying management apps this patch implements a new event
> that will be triggered on changes of the guest agent state.
> ---
>  daemon/remote.c  | 36 +++
>  include/libvirt/libvirt-domain.h | 28 +++
>  src/conf/domain_event.c  | 78 
> 
>  src/conf/domain_event.h  |  9 +
>  src/libvirt_private.syms |  2 ++
>  src/remote/remote_driver.c   | 31 
>  src/remote/remote_protocol.x | 16 -
>  src/remote_protocol-structs  |  7 
>  tools/virsh-domain.c | 40 +
>  9 files changed, 246 insertions(+), 1 deletion(-)
> 

> +++ b/include/libvirt/libvirt-domain.h
> @@ -3332,6 +3332,33 @@ typedef void 
> (*virConnectDomainEventTunableCallback)(virConnectPtr conn,
>   void *opaque);
> 
> 
> +typedef enum {
> +VIR_CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_CONNECTED = 1, /* agent 
> connected */
> +VIR_CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_DISCONNECTED = 2, /* agent 
> disconnected */
> +} virConnectDomainEventAgentLifecycleState;

Do we want to explicitly reserve 0 for unknown state?  Do we want to
provide a _LAST flag so that additions of future states can be detected
by the increase in the value of _LAST?

Everything else looks good.

-- 
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 0/5] Guest filesystem information API

[Eric Blake]

On 11/20/2014 11:11 AM, John Ferlan wrote:

>> However, for some reason I see this build error after 2/5:
>>
>> make[3]: Entering directory 
>> '/home/zippy/work/libvirt/libvirt.git/tools/wireshark/src'
>>CC   libvirt_la-packet-libvirt.lo
>> In file included from libvirt/protocol.h:5:0,
>>   from packet-libvirt.h:112,
>>   from packet-libvirt.c:36:
>> ./libvirt/remote.h: In function 'dissect_xdr_remote_typed_param_value':
>> ./libvirt/remote.h:470:5: error: unknown type name 'remote_nonnull_string'
>>   remote_nonnull_string type = 0;
>>   ^

> Strange - I don't see this.  My ./tools/wireshark/src/libvirt/remote.h
> doesn't even have that line. Stranger still, my config.log (after make
> clean; ./autogen.sh --system; make j4) has:
> 
> configure:69348: checking for wireshark
> configure:69381: result: no
> 
> 
> Could it be environmental? Could it be some config option? Perhaps
> something installed?  I have for wireshark on my f20 system:
> 
> wireshark.x86_64   1.10.10-1.fc20 @updates

You need wireshark-devel before configure will build wireshark
interactions into libvirt.


-- 
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 05/12] util: storage: Copy hosts of a storage file only if they exist

[Peter Krempa]

On 11/20/14 16:10, John Ferlan wrote:
> 
> 
> On 11/12/2014 08:47 AM, Peter Krempa wrote:
>> If there are no hosts for a storage source virStorageSourceCopy would
>> try to copy them anyways. As the success of virStorageNetHostDefCopy is
>> determined by returning a pointer and malloc of 0 elements might return
>> NULL according to the implementation, the result of the copy function
>> may vary.
>>
>> Fix this by copying the hosts array only if there are hosts defined.
>> ---
>>  src/util/virstoragefile.c | 9 ++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
> 
> I see 'virStorageSourceNewFromBackingRelative' also has the same issue.
> Could use this opportunity to fix that - or modify the API to adjust the
> return and only do the alloc if source->nhosts > 0.
> 
> I also note that virStorageNetHostDefCopy is only used in
> virstoragefile.c too - cause for statification?
> 
> ACK - I'll let you figure the best option...  Definitely need a check in
> both callers though.
> 
> John
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index 8e9d115..c2d5b46 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -1852,9 +1852,12 @@ virStorageSourceCopy(const virStorageSource *src,
>>  VIR_STRDUP(ret->compat, src->compat) < 0)
>>  goto error;
>>
>> -if (!(ret->hosts = virStorageNetHostDefCopy(src->nhosts, src->hosts)))
>> -goto error;
>> -ret->nhosts = src->nhosts;
>> +if (src->nhosts) {
>> +if (!(ret->hosts = virStorageNetHostDefCopy(src->nhosts, 
>> src->hosts)))

Avoiding the allocation would inhibit checking for failed allocation.
This would require to rewrite it to return an int and return the
allocated array as an argument. I'll stick with adding an explicit check
for now.

>> +goto error;
>> +
>> +ret->nhosts = src->nhosts;
>> +}
>>
>>  if (src->srcpool &&
>>  !(ret->srcpool = virStorageSourcePoolDefCopy(src->srcpool)))
>>




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 01/12] docs: domain: Move docs for storage hosts under the element

[Peter Krempa]

On 11/20/14 16:08, John Ferlan wrote:
> 
> 
> On 11/12/2014 08:47 AM, Peter Krempa wrote:
>> The docs describing the  element that are under the 
>> element in the XML document were incorrectly placed under the 
>> element. Move them to the correct place.
>> ---
>>  docs/formatdomain.html.in | 104 
>> --
>>  1 file changed, 54 insertions(+), 50 deletions(-)
>>
> 
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index d4189e6..4f44bc0 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -1953,6 +1953,60 @@
>>  may have zero or more host sub-elements used to
>>  specify the hosts to connect.
>>  
>> +
>> +
>> +  host
>> +  The host element supports 4 attributes, viz.  
>> "name",
>> +"port", "transport" and "socket", which specify the hostname,
>> +the port number, transport type and path to socket, 
>> respectively.
>> +The meaning of this element and the number of the elements 
>> depend
>> +on the protocol attribute.
> 
> 
> 
>> +
>> +  
>> + Protocol 
>> + Meaning 
>> + Number of hosts 
>> + Default port 
>> +  
>> +  
>> + nbd 
>> + a server running nbd-server 
>> + only one 
>> + 10809 
>> +  
>> +  
>> + iscsi 
>> + an iSCSI server 
>> + only one 
>> + 3260 
>> +  
>> +  
>> + rbd 
>> + monitor servers of RBD 
>> + one or more 
>> + 6789 
>> +  
>> +  
>> + sheepdog 
>> + one of the sheepdog servers (default is 
>> localhost:7000) 
>> + zero or one 
>> + 7000 
>> +  
>> +  
>> + gluster 
>> + a server running glusterd daemon 
>> + only one 
>> + 24007 
>> +  
>> +
> 
> 
> 
> ACK - adding the  is mostly a visual thing - it separates the
> table from the text a bit... not sure if there's another way to do it as
> my html skills are spartan.

I'm going to add aragraphs around the text blocks. That will
automagically add a break before the table and will avoid having empty
paragraph block.

> 
> John
> 
>> +gluster supports "tcp", "rdma", "unix" as valid values for the
>> +transport attribute.  nbd supports "tcp" and "unix".  Others 
>> only
>> +support "tcp".  If nothing is specified, "tcp" is assumed. If 
>> the
>> +transport is "unix", the socket attribute specifies the path to 
>> an
>> +AF_UNIX socket.
>> +  
>> +
>> +
>>  
>>  For a "file" or "volume" disk type which represents a cdrom or 
>> floppy
>>  (the device attribute), it is possible to define




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

[libvirt] Libvirt Live Migration

[Dhia Abbassi]

I'm trying to implement a virtualization API. I was testing migration with
libvirt I got some problems.

When I use the following command :

*virsh migrate --live --persistent --copy-storage-all vm-clone1
qemu+ssh://server_ip/system*

the migration works fine but in the destination host the migrated vm is
paused and I can't unpause it and I need to reboot the vm to be able use it
in the new host. When I try to unoause it Igot the following error message:
<< *Error unpausing domain: internal error: unable to execute QEMU command
'cont': Resetting the Virtual Machine is required *>>

How can I solve this problem, or is there an other way to make a live
migration with libvirt??

Thank you for you consideration.


-- 
Best regards

*Dhia Abbassi*
Full Stack Engineer | Rosafi Holding

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

Re: [libvirt] [PATCH 1/2] virdbus: don't force users to pass int for bool values

[Conrad Meyer]

Hi Eric,

I think this change breaks build on FreeBSD:

  CC   util/libvirt_util_la-virdbus.lo
util/virdbus.c:956:13: error: cast from 'bool *' to 'dbus_bool_t *' (aka 
'unsigned int *') increases required alignment from 1 to 4 
[-Werror,-Wcast-align]
GET_NEXT_VAL(dbus_bool_t, bool_val, bool, "%d");
^~~
util/virdbus.c:858:17: note: expanded from macro 'GET_NEXT_VAL'
x = (dbustype *)(*xptrptr + (*narrayptr - 1));  \
^ 1 error generated.

(CC is Clang.)

Thanks,
Conrad

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

Re: [libvirt] [PATCH v2 0/5] Guest filesystem information API

[John Ferlan]

On 11/20/2014 07:33 AM, Michal Privoznik wrote:
> On 19.11.2014 23:58, John Ferlan wrote:
>>
>>
>> On 11/17/2014 06:26 PM, Tomoki Sekiyama wrote:
>>> Hi,
>>>
>>> This is v2 of patchset to add virDomainGetFSInfo API.
>>>
>>> * changes in v1->v2:
>>>   -[all] removed redundant NULL element at the last of returned info array
>>>   -[3/5] make error messages in qemu_agent.c consistent with other commands
>>>   -[4/5] added a test case for 2 items in info->devAliases
>>>   -[5/5] added a pod document for virsh domfsinfo command
>>>   (v1: 
>>> http://www.redhat.com/archives/libvir-list/2014-October/msg1.html )
>>>
>>> * summary
>>>This series implements a new virDomainGetFSInfo API, that returns a list 
>>> of
>>>mounted filesystems information in the guest, collected via the guest 
>>> agent.
>>>
>>>The returned info contains mountpoints and disk device alias named in
>>>libvirt, so we can know which mountpoints should be frozen by
>>>virDomainFSFreeze to take snapshots of a part of disks.
>>>
>>> ---
>>> Tomoki Sekiyama (5):
>>>Implement public API for virDomainGetFSInfo
>>>remote: Implement the remote protocol for virDomainGetFSInfo
>>>qemu: Implement the qemu driver for virDomainGetFSInfo
>>>qemu: add test for qemuAgentGetFSInfo
>>>virsh: expose virDomainGetFSInfo
>>>
>>>
>>>   daemon/remote.c  |  117 
>>>   include/libvirt/libvirt-domain.h |   21 
>>>   src/conf/domain_conf.c   |   71 
>>>   src/conf/domain_conf.h   |6 +
>>>   src/driver-hypervisor.h  |6 +
>>>   src/libvirt.c|   66 +++
>>>   src/libvirt_private.syms |1
>>>   src/libvirt_public.syms  |6 +
>>>   src/qemu/qemu_agent.c|  178 
>>> ++
>>>   src/qemu/qemu_agent.h|2
>>>   src/qemu/qemu_driver.c   |   48 
>>>   src/remote/remote_driver.c   |   92 
>>>   src/remote/remote_protocol.x |   32 +
>>>   src/remote_protocol-structs  |   21 
>>>   src/rpc/gendispatch.pl   |1
>>>   tests/Makefile.am|1
>>>   tests/qemuagentdata/qemuagent-fsinfo.xml |   39 +++
>>>   tests/qemuagenttest.c|  143 
>>>   tools/virsh-domain.c |   74 
>>>   tools/virsh.pod  |9 ++
>>>   20 files changed, 933 insertions(+), 1 deletion(-)
>>>   create mode 100644 tests/qemuagentdata/qemuagent-fsinfo.xml
>>>
>>> --
>>>
>>> Tomoki Sekiyama
>>>
>>
>>
>> I reviewed the 'libvirt' specific changes - had a few comments and have
>> made changes to my review branch as specified.  As long as you're OK
>> with those changes I will get these pushed.
>>
>> I'm also hoping someone else (eblake?) can look at the remote_protocol.x
>> changes to ensure they encompass everything they are supposed to.  Also
>> that the usage of QEMU_JOB_QUERY not _MODIFY for the GetFSInfo seems
>> more appropriate and is in line with the various remote_protocol.x
>> settings (@acl/@generate stuff settings).
> 
> 
> @generate is correct, since both, client and server implementations are 
> provided.
> @acl looks consistent to the rest. Correct, for querying domain info you 
> need to have read permission and that's it.
> 
> And yes, the job should be _QUERY since we are querying info, not 
> modifying domain in any way.
> 
> However, for some reason I see this build error after 2/5:
> 
> make[3]: Entering directory 
> '/home/zippy/work/libvirt/libvirt.git/tools/wireshark/src'
>CC   libvirt_la-packet-libvirt.lo
> In file included from libvirt/protocol.h:5:0,
>   from packet-libvirt.h:112,
>   from packet-libvirt.c:36:
> ./libvirt/remote.h: In function 'dissect_xdr_remote_typed_param_value':
> ./libvirt/remote.h:470:5: error: unknown type name 'remote_nonnull_string'
>   remote_nonnull_string type = 0;
>   ^
> ./libvirt/remote.h:473:5: warning: implicit declaration of function 
> 'xdr_remote_nonnull_string' [-Wimplicit-function-declaration]
>   if (!xdr_remote_nonnull_string(xdrs, &type))
>   ^
> Makefile:1934: recipe for target 'libvirt_la-packet-libvirt.lo' failed
> make[3]: *** [libvirt_la-packet-libvirt.lo] Error 1
> 
> 
> 

Strange - I don't see this.  My ./tools/wireshark/src/libvirt/remote.h
doesn't even have that line. Stranger still, my config.log (after make
clean; ./autogen.sh --system; make j4) has:

configure:69348: checking for wireshark
configure:69381: result: no


Could it be environmental? Could it be some config option? Perhaps
something installed?  I have for wireshark on my f20 system:

wireshark.x86_64   1.10.10-1.fc20 @updates



John

--
libvir-list mailing list

Re: [libvirt] [PATCH 04/11] qemu: monitor: Rename and improve qemuMonitorGetPtyPaths

[Jiri Denemark]

On Wed, Nov 19, 2014 at 11:23:17 +0100, Peter Krempa wrote:
> To unify future additions that require information from "query-chardev"
> rename qemuMonitorGetPtyPaths and friends to qemuMonitorGetChardevInfo
> and move the allocation of the returned hash into the top level
> function.
> ---
>  src/qemu/qemu_monitor.c  | 31 +++
>  src/qemu/qemu_monitor.h  |  4 ++--
>  src/qemu/qemu_monitor_json.c | 15 +--
>  src/qemu/qemu_monitor_json.h |  4 ++--
>  src/qemu/qemu_monitor_text.c |  6 +++---
>  src/qemu/qemu_monitor_text.h |  4 ++--
>  src/qemu/qemu_process.c  | 28 
>  tests/qemumonitorjsontest.c  | 26 +-
>  8 files changed, 66 insertions(+), 52 deletions(-)

ACK

Jirka

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

Re: [libvirt] [PATCH 03/11] test: xml2xml: Print full filenames if xml2xml test fails

[Jiri Denemark]

On Wed, Nov 19, 2014 at 11:23:16 +0100, Peter Krempa wrote:
> To simplify looking for a problem instrument the XML comparator function
> with possibility to print the filename of the failed/expected XML
> output.
> 
> This is necessary as the VIR_TEST_DIFFERENT macro possibly tests two XML
> files for the inactive/active state and the resulting error may not be
> obvious.
> ---
>  tests/qemuxml2xmltest.c |  2 +-
>  tests/testutils.c   | 35 ++-
>  tests/testutils.h   |  5 +
>  3 files changed, 36 insertions(+), 6 deletions(-)

ACK

Jirka

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

Re: [libvirt] [PATCH 02/11] conf: Annotate source enums for character device struct members

[Jiri Denemark]

On Wed, Nov 19, 2014 at 11:23:15 +0100, Peter Krempa wrote:
> Add a comment to track which values may be present in certain members of
> a struct _virDomainChrDef.
> ---
>  src/conf/domain_conf.h | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 530a3ca..574d3ea 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1100,10 +1100,13 @@ struct _virDomainChrSourceDef {
> 
>  /* A complete character device, both host and domain views.  */
>  struct _virDomainChrDef {
> -int deviceType;
> +int deviceType; /* enum virDomainChrDeviceType */
> 
>  bool targetTypeAttr;
> -int targetType;
> +int targetType; /* enum virDomainChrConsoleTargetType ||
> +   enum virDomainChrChannelTargetType ||
> +   enum virDomainChrSerialTargetType according to 
> deviceType */
> +
>  union {
>  int port; /* parallel, serial, console */
>  virSocketAddrPtr addr; /* guestfwd */

ACK

Jirka

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

Re: [libvirt] [PATCH 01/11] qemu: process: report useful error if alias formatting fails

[Jiri Denemark]

On Wed, Nov 19, 2014 at 11:23:14 +0100, Peter Krempa wrote:
> When retrieving the paths for PTY devices the alias gets formatted into
> a static string. If it doesn't fit we wouldn't report an error.
> ---
>  src/qemu/qemu_process.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 7518138..e58a46d 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1878,8 +1878,12 @@ qemuProcessLookupPTYs(virDomainDefPtr def,
> 
>  if (snprintf(id, sizeof(id), "%s%s",
>   chardevfmt ? "char" : "",
> - chr->info.alias) >= sizeof(id))
> + chr->info.alias) >= sizeof(id)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("failed to format device alias "
> + "for PTY retrieval"));
>  return -1;
> +}
> 
>  path = (const char *) virHashLookup(paths, id);
>  if (path == NULL) {

ACK although I thought we had some neat wrapper around snprintf, or is
my memory playing games with me?

Jirka

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

[libvirt] Plan for next releases

[Daniel Veillard]

  We are getting close to the end of the month and in theory we should
enter freeze around next week if we want to release 1.2.11 early Dec.
However:
  - we have 'only' 150 commits since 1.2.10
  - December is usually heavilly truncated due to Xmas/etc... vacations
  - I'm actually on vacations next week and while I could try to push
the release candidates while on the road it's not ideal

What about pushing the 1.2.11 release to around 15th December (maybe a
bit earlier) then push the following release to the last week of
January, just before FOSDEM so it's available then, and also because Feb
is really short.

  Opinions ? I'm tempted to do that '2 release in 3 months' trick, we
did that in the past for end of year,

   Any objections ?

thanks,

Daniel

-- 
Daniel Veillard  | Open Source and Standards, Red Hat
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

[libvirt] [PATCH] build: avoid 32-bit failure on older gcc

[Eric Blake]

On 32-bit platforms with old gcc (hello RHEL 5 gcc 4.1.2), the
build fails with:
virsh-domain.c: In function 'cmdBlockCopy':
virsh-domain.c:2172: warning: comparison is always false due to limited range 
of data type

Adjust the code to silence the warning.

* tools/virsh-domain.c (cmdBlockCopy): Pacify RHEL 5 gcc.

Signed-off-by: Eric Blake 
---

Pushing under the build-breaker rule.

 tools/virsh-domain.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 7184784..cf45a88 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2169,7 +2169,8 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
 if (bandwidth) {
 /* bandwidth is ulong MiB/s, but the typed parameter is
  * ullong bytes/s; make sure we don't overflow */
-if (bandwidth + 0ULL > ULLONG_MAX >> 20) {
+unsigned long long limit = MIN(ULONG_MAX, ULLONG_MAX >> 20);
+if (bandwidth > limit) {
 virReportError(VIR_ERR_OVERFLOW,
_("bandwidth must be less than %llu"),
ULLONG_MAX >> 20);
-- 
1.9.3

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

[libvirt] [PATCH] build: fix build when not using dbus

[Eric Blake]

Commit c0e7022 breaks on a machine that lacks dbus headers:

In file included from util/virdbus.c:24:0:
util/virdbuspriv.h:31:3: error: unknown type name 'dbus_int16_t'

* src/util/virdbuspriv.h (DBusBasicValue): Only provide fallback
when dbus is compiled.

Signed-off-by: Eric Blake 
---

Pushing under the build-breaker rule (I feel like I'm in whack-a-mole
mode right now, where my fix for one problem causes a failure in
another environment. Sorry for the churn)

 src/util/virdbuspriv.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virdbuspriv.h b/src/util/virdbuspriv.h
index 4247746..74b395f 100644
--- a/src/util/virdbuspriv.h
+++ b/src/util/virdbuspriv.h
@@ -24,7 +24,7 @@

 # include "virdbus.h"

-# if !HAVE_DBUSBASICVALUE
+# if defined(WITH_DBUS) && !HAVE_DBUSBASICVALUE
 /* Copied (and simplified) from dbus 1.6.12, for use with older dbus headers */
 typedef union
 {
-- 
1.9.3

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

[libvirt] [PATCH] drvbhyve: Automatically tear down guest domains on shutdown

[Conrad Meyer]

Reboot requires more sophistication and is left as a future work item --
but at least part of the plumbing is in place.
---
 src/Makefile.am   |   2 +
 src/bhyve/bhyve_monitor.c | 184 ++
 src/bhyve/bhyve_monitor.h |  36 +
 src/bhyve/bhyve_process.c |  14 +++-
 4 files changed, 233 insertions(+), 3 deletions(-)
 create mode 100644 src/bhyve/bhyve_monitor.c
 create mode 100644 src/bhyve/bhyve_monitor.h

diff --git a/src/Makefile.am b/src/Makefile.am
index d8fe624..b6c1701 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -833,6 +833,8 @@ BHYVE_DRIVER_SOURCES =  
\
bhyve/bhyve_domain.h\
bhyve/bhyve_driver.h\
bhyve/bhyve_driver.c\
+   bhyve/bhyve_monitor.c   \
+   bhyve/bhyve_monitor.h   \
bhyve/bhyve_process.c   \
bhyve/bhyve_process.h   \
bhyve/bhyve_utils.h \
diff --git a/src/bhyve/bhyve_monitor.c b/src/bhyve/bhyve_monitor.c
new file mode 100644
index 000..cd3cf6e
--- /dev/null
+++ b/src/bhyve/bhyve_monitor.c
@@ -0,0 +1,184 @@
+/*
+ * bhyve_monitor.c: Tear-down or reboot bhyve domains on guest shutdown
+ *
+ * Copyright (C) 2014 Conrad Meyer
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ * Author: Conrad Meyer 
+ */
+
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include "bhyve_monitor.h"
+#include "bhyve_process.h"
+#include "viralloc.h"
+#include "virerror.h"
+#include "virfile.h"
+#include "virlog.h"
+
+#define VIR_FROM_THIS  VIR_FROM_BHYVE
+
+VIR_LOG_INIT("bhyve.bhyve_monitor");
+
+struct _bhyveMonitor {
+int kq;
+int watch;
+virDomainObjPtr vm;
+bhyveConnPtr driver;
+};
+
+static void
+bhyveMonitorIO(int watch, int kq, int events ATTRIBUTE_UNUSED, void *opaque)
+{
+const struct timespec zerowait = {};
+bhyveMonitorPtr mon = opaque;
+struct kevent kev;
+int rc, status;
+
+if (watch != mon->watch || kq != mon->kq) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("event from unexpected fd %d!=%d / watch %d!=%d"),
+   mon->kq, kq, mon->watch, watch);
+return;
+}
+
+rc = kevent(kq, NULL, 0, &kev, 1, &zerowait);
+if (rc < 0) {
+virReportSystemError(errno, "%s", _("Unable to query kqueue"));
+return;
+}
+
+if (rc == 0)
+return;
+
+if ((kev.flags & EV_ERROR) != 0) {
+virReportSystemError(kev.data, "%s", _("Unable to query kqueue"));
+return;
+}
+
+if (kev.filter == EVFILT_PROC && (kev.fflags & NOTE_EXIT) != 0) {
+if ((pid_t)kev.ident != mon->vm->pid) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+_("event from unexpected proc %ju!=%ju"),
+(uintmax_t)mon->vm->pid, (uintmax_t)kev.ident);
+return;
+}
+
+status = kev.data;
+if (WIFSIGNALED(status) && WCOREDUMP(status)) {
+VIR_ERROR("Guest %s got signal %d and crashed", mon->vm->def->name,
+  WTERMSIG(status));
+virBhyveProcessStop(mon->driver, mon->vm,
+VIR_DOMAIN_SHUTOFF_CRASHED);
+} else if (WIFEXITED(status)) {
+if (WEXITSTATUS(status) == 0) {
+/* 0 - reboot */
+/* TODO: Implementing reboot is a little more complicated. */
+VIR_INFO("Guest %s rebooted; destroying domain.",
+ mon->vm->def->name);
+virBhyveProcessStop(mon->driver, mon->vm,
+VIR_DOMAIN_SHUTOFF_SHUTDOWN);
+} else if (WEXITSTATUS(status) < 3) {
+/* 1 - shutdown, 2 - halt, 3 - triple fault. others - error */
+VIR_INFO("Guest %s shut itself down; destroying domain.",
+ mon->vm->def->name);
+virBhyveProcessStop(mon->driver, mon->vm,
+VIR_DOMAIN_SHUTOFF_SHUTDOWN);
+} else {
+VIR_INFO("Guest %s had an er

Re: [libvirt] [PATCH 0/2] Fix fc_host SCSI pool refresh issues

[John Ferlan]

On 11/19/2014 03:52 PM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1152382
> 
> Fix a couple of fc_host SCSI refresh pool issues.  The first patch I found
> while working on the second.
> 
> It turns out the existing logic to handle 'retval' wasn't correct. Using
> a default of zero, resulted in a couple of cases where success was returned
> when it shouldn't have been. The one that caused the issues was when the
> search of the /sys/bus/scsi/devices/ tree found a new vHBA related device
> and then "assumed" that the 'block' device would be found in the directory.
> That assumption resulted in a 10 second pause in virStorageBackendStablePath
> failing to find a stable path to the device "/dev/(null)" because the
> getBlockDevice returned zero and a NULL block_device and the processLU
> code only failed when the return was < 0.  The processLU code had similar
> issues with retval processing which are also fixed.
> 
> The second patch provides a mechanism to attempt to fill the pool of a 
> libvirt created VPORT_CREATE'd vHBA. The creation takes some time for the
> udev callback mechanism to find and configure. By adding a thread to
> attempt to retry to find the LU's (with any luck) after the udev processing
> has had time to do something means a subsequent 'vol-list' on the pool may
> actually find a volume without needing to attempt a 'pool-refresh' again.
> It's too bad the udev mechanism didn't provide a way to indicate it has
> completed setting up a specific device.
> 
> John Ferlan (2):
>   storage: Fix issue finding LU's when block doesn't exist
>   storage: Add thread to refresh for createVport
> 
>  src/storage/storage_backend_scsi.c | 147 
> -
>  1 file changed, 128 insertions(+), 19 deletions(-)
> 


Pushed - thanks for the review


John

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

Re: [libvirt] [PATCH 1/2] storage: Fix issue finding LU's when block doesn't exist

[John Ferlan]

On 11/20/2014 09:12 AM, Michal Privoznik wrote:

<...snip...>

>> @@ -451,10 +451,10 @@ virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool,
>>   continue;
>>   }
>>
>> -found = true;
>> -VIR_DEBUG("Found LU '%s'", lun_dirent->d_name);
>> +VIR_DEBUG("Found possible LU '%s'", lun_dirent->d_name);
>>
>> -processLU(pool, scanhost, bus, target, lun);
>> +if (processLU(pool, scanhost, bus, target, lun) == 0)
>> +found = true;
> 
> Do we want 'break' here to jump out from the loop too?
> 

No as we need to find "all" LU's - this just indicates that we found at
least one.  The called function will add the LU's to the pool.

John
>>   }
>>
>>   if (!found)
>>
> 
> Either way, that's just an optimization, so ACK.
> 
> Michal
> 

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

[libvirt] [PATCH] storage: qemu: Fix security labelling of new image chain elements

[Peter Krempa]

When creating a disk image snapshot the libvirt code would blindly copy
the parents label to the newly created image. This runs into problems
when you start a VM from an image hosted on NFS (or other storage system
that doesn't support selinux labels) and the snapshot destination is on
a storage system that does support selinux labels. Libvirt's code in
that case generates a different security label for the image hosted on
NFS. This label is valid only for NFS images and doesn't allow access in
case of a locally stored image.

To fix this issue libvirt needs to refrain from copying security
information in cases where the default domain seclabel is a better
choice.

This patch repurposes the now unused @force argument of
virStorageSourceInitChainElement to denote whether a copy of the
security labelling stuff should be attempted or not. This allows to
fine-control the copy operation for cases where we need to keep the
label of the old disk vs. the cases where we need to keep the label
unset to use the default domain imagelabel.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1151718
---
Note:
I've opted not to change to use the default label when doing a block-copy via
the old API as there is no way to specify a new label all other options now
allow to provide a different label via the XML that was provided by the user.

 src/qemu/qemu_driver.c| 14 --
 src/qemu/qemu_process.c   |  2 +-
 src/util/virstoragefile.c | 16 +++-
 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 74d1bdc..0343713 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15861,7 +15861,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
   unsigned long long bandwidth,
   unsigned int granularity,
   unsigned long long buf_size,
-  unsigned int flags)
+  unsigned int flags,
+  bool keepParentLabel)
 {
 virQEMUDriverPtr driver = conn->privateData;
 qemuDomainObjPrivatePtr priv;
@@ -15992,7 +15993,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
 if (mirror->format > 0)
 format = virStorageFileFormatTypeToString(mirror->format);

-if (virStorageSourceInitChainElement(mirror, disk->src, false) < 0)
+if (virStorageSourceInitChainElement(mirror, disk->src,
+ keepParentLabel) < 0)
 goto endjob;

 if (qemuDomainPrepareDiskChainElement(driver, vm, mirror,
@@ -16104,7 +16106,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char 
*path, const char *base,
 flags &= (VIR_DOMAIN_BLOCK_REBASE_SHALLOW |
   VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT);
 ret = qemuDomainBlockCopyCommon(vm, dom->conn, path, dest,
-bandwidth, 0, 0, flags);
+bandwidth, 0, 0, flags, true);
 vm = NULL;
 dest = NULL;

@@ -16177,8 +16179,8 @@ qemuDomainBlockCopy(virDomainPtr dom, const char *disk, 
const char *destxml,
  VIR_DOMAIN_XML_INACTIVE)))
 goto cleanup;

-ret = qemuDomainBlockCopyCommon(vm, dom->conn, disk, dest,
-bandwidth, granularity, buf_size, flags);
+ret = qemuDomainBlockCopyCommon(vm, dom->conn, disk, dest, bandwidth,
+granularity, buf_size, flags, false);
 vm = NULL;

  cleanup:
@@ -16353,7 +16355,7 @@ qemuDomainBlockCommit(virDomainPtr dom,
 goto endjob;
 if (virStorageSourceInitChainElement(mirror,
  disk->src,
- false) < 0)
+ true) < 0)
 goto endjob;
 }

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index b782984..1f22bd0 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1064,7 +1064,7 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon 
ATTRIBUTE_UNUSED,
 copy = virStorageSourceCopy(disk->mirror, false);
 if (virStorageSourceInitChainElement(copy,
  persistDisk->src,
- false) < 0) {
+ true) < 0) {
 VIR_WARN("Unable to update persistent definition "
  "on vm %s after block job",
  vm->def->name);
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index aa97f75..5287c51 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1900,28 +1900,26 @@ virStorageSourceCopy(const virStorageSource *src,
  * virStorageSourceInitChainElement:
  * @newelem

Re: [libvirt] [PATCH 11/12] storage: rbd: qemu: Add support for specifying internal RBD snapshots

[John Ferlan]

On 11/12/2014 08:47 AM, Peter Krempa wrote:
> Some storage systems have internal support for snapshots. Libvirt should
> be able to select a correct snapshot when starting a VM.
> 
> This patch adds a XML element to select a storage source snapshot for
> the RBD protocol which supports this feature.
> ---
>  docs/formatdomain.html.in  | 18 +++---
>  docs/schemas/domaincommon.rng  |  8 +
>  src/conf/domain_conf.c | 38 
> +++---
>  src/conf/domain_conf.h |  1 +
>  src/conf/snapshot_conf.c   |  6 ++--
>  src/qemu/qemu_command.c|  3 ++
>  src/util/virstoragefile.c  |  8 +
>  src/util/virstoragefile.h  |  1 +
>  .../qemuxml2argv-disk-drive-network-rbd.args   |  4 +++
>  .../qemuxml2argv-disk-drive-network-rbd.xml| 17 ++
>  10 files changed, 94 insertions(+), 10 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 4f44bc0..fc35c5a 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1676,6 +1676,7 @@
>
>
>  
> +
>
>
>
> @@ -1949,14 +1950,16 @@
>  is only valid when the specified storage volume is of 'file' or
>  'block' type).
>  
> -When the disk type is "network", the source
> -may have zero or more host sub-elements used to
> -specify the hosts to connect.
> +The source element may contain the following sub 
> elements:
>  
> 
>  
>host
> -  The host element supports 4 attributes, viz.  
> "name",
> +  When the disk type is "network", the 
> source
> +may have zero or more host sub-elements used to
> +specify the hosts to connect.
> +
> +The host element supports 4 attributes, viz.  
> "name",
>  "port", "transport" and "socket", which specify the hostname,
>  the port number, transport type and path to socket, respectively.
>  The meaning of this element and the number of the elements depend
> @@ -2005,6 +2008,13 @@
>  transport is "unix", the socket attribute specifies the path to 
> an
>  AF_UNIX socket.
>
> +  snapshot
> +  
> +The name attribute of snapshot element 
> can
> +optionally specify an internal snapshot name to be used as the
> +source for storage systems such as rbd.
> +(Since 1.2.11 for the qemu driver.)

To follow other usages (adjust last 2 lines):

source for storage protocols.
Supported for 'rbd' since 1.2.11 (QEMU only).


> +  
>  
> 
>  
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 6863ec6..154d222 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1452,6 +1452,14 @@
>  
>
>  
> +
> +  
> +
> +  
> +
> +
> +  
> +
>  
>
>  
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 2c65276..37a8042 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3162,6 +3162,22 @@ 
> virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev,
>  return -1;
>  }
> 
> +/* verify disk source */
> +if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
> +virDomainDiskDefPtr disk = dev->data.disk;
> +
> +/* internal snapshots are currently supported only with rbd: */
> +if (virStorageSourceGetActualType(disk->src) != 
> VIR_STORAGE_TYPE_NETWORK &&
> +disk->src->protocol != VIR_STORAGE_NET_PROTOCOL_RBD) {
> +if (disk->src->snapshot) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> +   _(" element is currently supported "
> + "only with 'rbd' disks"));
> +return -1;
> +}
> +}
> +}
> +
>  return 0;
>  }
> 
> @@ -5316,10 +5332,14 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node,
> 
>  int
>  virDomainDiskSourceParse(xmlNodePtr node,
> + xmlXPathContextPtr ctxt,
>   virStorageSourcePtr src)
>  {
>  int ret = -1;
>  char *protocol = NULL;
> +xmlNodePtr saveNode = ctxt->node;
> +
> +ctxt->node = node;
> 
>  switch ((virStorageType)src->type) {
>  case VIR_STORAGE_TYPE_FILE:
> @@ -5372,6 +5392,9 @@ virDomainDiskSourceParse(xmlNodePtr node,
>  tmp[0] = '\0'

Re: [libvirt] [PATCH 09/12] util: storagefile: Split out parsing of NBD string into a separate func

[John Ferlan]

On 11/12/2014 08:47 AM, Peter Krempa wrote:
> Split out the code so that the function looks homogenous after adding
> more protocol specific parsers.
> ---
>  src/util/virstoragefile.c | 141 
> --
>  1 file changed, 86 insertions(+), 55 deletions(-)
> 
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index f267d1a..58be237 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -2353,77 +2353,109 @@ virStorageSourceParseRBDColonString(const char 
> *rbdstr,
> 
> 
>  static int
> -virStorageSourceParseBackingColon(virStorageSourcePtr src,
> -  const char *path)
> +virStorageSourceParseNBDColonString(const char *nbdstr,
> +virStorageSourcePtr src)
>  {
>  char **backing = NULL;
>  int ret = -1;
> 
> -if (!(backing = virStringSplit(path, ":", 0)))
> +if (!(backing = virStringSplit(nbdstr, ":", 0)))
>  goto cleanup;
> 
> -if (!backing[0] ||
> -(src->protocol = virStorageNetProtocolTypeFromString(backing[0])) < 
> 0) {
> +/* we know that backing[0] now equals to "nbd" */
> +
> +if (VIR_ALLOC_N(src->hosts, 1) < 0)
> +goto cleanup;
> +
> +src->nhosts = 1;
> +src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
> +
> +/* format: [] denotes optional sections, uppercase are variable strings
> + * nbd:unix:/PATH/TO/SOCKET[:exportname=EXPORTNAME]
> + * nbd:HOSTNAME:PORT[:exportname=EXPORTNAME]
> + */
> +if (!backing[1]) {

IOW: If someone provided just "nbd:", right?

>  virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("invalid backing protocol '%s'"),
> -   NULLSTR(backing[0]));
> +   _("missing remote information in '%s' for protocol 
> nbd"),
> +   nbdstr);
>  goto cleanup;
> -}
> +} else if (STREQ(backing[1], "unix")) {
> +if (!backing[2]) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("missing unix socket path in nbd backing string 
> %s"),
> +   nbdstr);
> +goto cleanup;
> +}
> 
> -switch ((virStorageNetProtocol) src->protocol) {
> -case VIR_STORAGE_NET_PROTOCOL_NBD:
> -if (VIR_ALLOC_N(src->hosts, 1) < 0)
> +if (VIR_STRDUP(src->hosts->socket, backing[2]) < 0)
>  goto cleanup;
> -src->nhosts = 1;
> -src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
> 
> -/* format: [] denotes optional sections, uppercase are variable 
> strings
> - * nbd:unix:/PATH/TO/SOCKET[:exportname=EXPORTNAME]
> - * nbd:HOSTNAME:PORT[:exportname=EXPORTNAME]
> - */
> +   } else {
>  if (!backing[1]) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("missing remote information in '%s' for 
> protocol nbd"),
> -   path);
> +   _("missing host name in nbd string '%s'"),
> +   nbdstr);

How could this ever have happened?

We have :
if (!backing[1])
error
else if (STREQ(backing[1], "unix"))
...
else
if (!backing[1])
different error

>  goto cleanup;
> -} else if (STREQ(backing[1], "unix")) {
> -if (!backing[2]) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("missing unix socket path in nbd backing 
> string %s"),
> -   path);
> -goto cleanup;
> -}
> +}
> 
> -if (VIR_STRDUP(src->hosts->socket, backing[2]) < 0)
> -goto cleanup;
> +if (VIR_STRDUP(src->hosts->name, backing[1]) < 0)
> +goto cleanup;
> 
> -   } else {
> -if (!backing[1]) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("missing host name in nbd string '%s'"),
> -   path);
> -goto cleanup;
> -}
> +if (!backing[2]) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("missing port in nbd string '%s'"),
> +   nbdstr);
> +goto cleanup;
> +}
> 
> -if (VIR_STRDUP(src->hosts->name, backing[1]) < 0)
> -goto cleanup;
> +if (VIR_STRDUP(src->hosts->port, backing[2]) < 0)
> +goto cleanup;
> +}
> 
> -if (!backing[2]) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("missing port in nbd string '%s'"),
> -   path);
> -goto cleanup;
> -}
> +if (backing[3] && STRPREFIX(backing[3], "exportname=")) {
> +if (VIR_STRDUP(src->path, backing[3] + strlen("exportna

Re: [libvirt] [PATCH 12/12] storage: rbd: Implement support for passing config file option

[John Ferlan]

On 11/12/2014 08:47 AM, Peter Krempa wrote:
> To be able to express some use cases of the RBD backing with libvirt, we
> need to be able to specify a config file for the RBD client to qemu as
> that is one of the commonly used options.
> ---
>  docs/formatdomain.html.in  |  8 
>  docs/schemas/domaincommon.rng  |  8 
>  src/conf/domain_conf.c | 18 
> --
>  src/qemu/qemu_command.c|  3 +++
>  src/util/virstoragefile.c  |  5 +
>  src/util/virstoragefile.h  |  2 ++
>  .../qemuxml2argv-disk-drive-network-rbd.args   |  2 ++
>  .../qemuxml2argv-disk-drive-network-rbd.xml|  8 
>  8 files changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index fc35c5a..62bca45 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1677,6 +1677,7 @@
>
>  
>  
> +
>
>
>
> @@ -2015,6 +2016,13 @@
>  source for storage systems such as rbd.
>  (Since 1.2.11 for the qemu driver.)
>
> +  config
> +  
> +The file attribute of config element
> +allows to specify a configuration file for storage backends.
> +
> +(Since 1.2.11 for 'rbd' disks in qemu)

So this reads funny/strange... In particular, "...attribute of config
element allows to specify..."

Perhaps,

"The file attribute for the config element
provides a fully qualified path to a configuration file to be provided
as a parameter to the client of a networked storage protocol. Supported
for 'rbd' since 1.2.11 (QEMU only)."


NOTE: Going for consistent usage w/ 11/12... previously snapshot had
"storage systems" and this has "storage backends".  Since both seem to
be keyed of which storage protocol is used, I went with storage protocol.

> +  
>  
> 
>  
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 154d222..6b2845a 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1460,6 +1460,14 @@
>  
>
>  
> +
> +  
> +
> +  
> +
> +
> +  
> +
>  
>
>  
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 37a8042..a9a26a4 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3166,7 +3166,8 @@ 
> virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev,
>  if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
>  virDomainDiskDefPtr disk = dev->data.disk;
> 
> -/* internal snapshots are currently supported only with rbd: */
> +/* internal snapshots and config files are currently supported
> + * only with rbd: */
>  if (virStorageSourceGetActualType(disk->src) != 
> VIR_STORAGE_TYPE_NETWORK &&
>  disk->src->protocol != VIR_STORAGE_NET_PROTOCOL_RBD) {
>  if (disk->src->snapshot) {
> @@ -3175,6 +3176,13 @@ 
> virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev,
>   "only with 'rbd' disks"));
>  return -1;
>  }
> +
> +if (disk->src->configFile) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> +   _(" element is currently supported "
> + "only with 'rbd' disks"));
> +return -1;
> +}
>  }
>  }
> 
> @@ -5395,6 +5403,9 @@ virDomainDiskSourceParse(xmlNodePtr node,
>  /* snapshot currently works only for remote disks */
>  src->snapshot = virXPathString("string(./snapshot/@name)", ctxt);
> 
> +/* config file currently only works with remote disks */
> +src->configFile = virXPathString("string(./config/@file)", ctxt);
> +
>  if (virDomainStorageHostParse(node, &src->hosts, &src->nhosts) < 0)
>  goto cleanup;
>  break;
> @@ -16179,7 +16190,7 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf,
> 
>  VIR_FREE(path);
> 
> -if (src->nhosts == 0 && !src->snapshot) {
> +if (src->nhosts == 0 && !src->snapshot && !src->configFile) {
>  virBufferAddLit(buf, "/>\n");
>  } else {
>  virBufferAddLit(buf, ">\n");
> @@ -16205,6 +16216,9 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf,
>  virBufferEscapeString(buf, "\n",
>src->snapshot);
> 

if (sr

Re: [libvirt] [PATCH 00/12] Unbreak vm's backed by RBD disks

[John Ferlan]

On 11/12/2014 08:47 AM, Peter Krempa wrote:
> After recent refactors, starting a VM whose disk is backed by RBD storage 
> would
> fail as the parser for the backing file specification string was not
> implemented in the metadata crawler.
> 
> Reuse qemu's parser to do this and fix a few things around.
> 
> 

In general ACK series - although I did make comments to specific patches
- some for simple typos/nits and a couple for minor adjustments which
should be addressable without the need for a v2 (patch 5, 8, 9, 11, & 12).

Nice to know about virstoragetest.c - I can see the need to add perhaps
some iscsi options there (learned something new today)


John
> 
> Peter Krempa (12):
>   docs: domain: Move docs for storage hosts under the  element
>   test: virstoragetest: Add testing of network disk details
>   util: buffer: Clarify scope of the escape operation in virBufferEscape
>   util: storage: Add notice for extension of struct virStorageSource
>   util: storage: Copy hosts of a storage file only if they exist
>   qemu: Refactor qemuBuildNetworkDriveURI to take a virStorageSourcePtr
>   tests: Reflow the expected output from RBD disk test
>   util: split out qemuParseRBDString into a common helper
>   util: storagefile: Split out parsing of NBD string into a separate
> func
>   storage: Allow parsing of RBD backing strings when building backing
> chain
>   storage: rbd: qemu: Add support for specifying internal RBD snapshots
>   storage: rbd: Implement support for passing config file option
> 
>  docs/formatdomain.html.in  | 128 +
>  docs/schemas/domaincommon.rng  |  16 ++
>  src/conf/domain_conf.c |  52 +++-
>  src/conf/domain_conf.h |   1 +
>  src/conf/snapshot_conf.c   |   6 +-
>  src/libvirt_private.syms   |   1 +
>  src/qemu/qemu_command.c| 268 +-
>  src/util/virbuffer.c   |   5 +-
>  src/util/virstoragefile.c  | 313 
> +
>  src/util/virstoragefile.h  |  14 +-
>  .../qemuxml2argv-disk-drive-network-rbd.args   |  16 +-
>  .../qemuxml2argv-disk-drive-network-rbd.xml|  25 ++
>  tests/virstoragetest.c |  65 -
>  13 files changed, 587 insertions(+), 323 deletions(-)
> 

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

Re: [libvirt] [PATCH] Fix API docs for header file re-organization

[Daniel P. Berrange]

On Tue, Nov 18, 2014 at 04:48:32AM +0530, Nehal J Wani wrote:
> I guess, references of libvirt-libvirt.html were not removed from
> docs/Makefile.am
> 
> Following patch fixes it (But I am not sure if it is the right way to go):
> 
> diff --git a/docs/Makefile.am b/docs/Makefile.am
> index 910bfef..5485ee9 100644
> --- a/docs/Makefile.am
> +++ b/docs/Makefile.am
> @@ -31,7 +31,6 @@ apihtml =\
>html/libvirt-libvirt-domain-snapshot.html\
>html/libvirt-libvirt-event.html\
>html/libvirt-libvirt-host.html\
> -  html/libvirt-libvirt.html\
>html/libvirt-libvirt-interface.html\
>html/libvirt-libvirt-network.html\
>html/libvirt-libvirt-nodedev.html\
> @@ -51,7 +50,6 @@ devhelphtml =\
>devhelp/libvirt.devhelp\
>devhelp/index.html\
>devhelp/general.html\
> -  devhelp/libvirt-libvirt.html\
>devhelp/libvirt-virterror.html
> 
>  css = \
> @@ -269,7 +267,6 @@ $(addprefix $(srcdir)/,$(devhelphtml)):
> $(srcdir)/libvirt-api.xml $(devhelpxsl)
> 
> 
>  python_generated_files = \
> -$(srcdir)/html/libvirt-libvirt.html \
>  $(srcdir)/html/libvirt-libvirt-lxc.html \
>  $(srcdir)/html/libvirt-libvirt-qemu.html \
>  $(srcdir)/html/libvirt-virterror.html \

ACK, and pushed


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 08/12] util: split out qemuParseRBDString into a common helper

[John Ferlan]

On 11/12/2014 08:47 AM, Peter Krempa wrote:
> To allow reuse this non-trivial parser code in the backing store parser
> this part of the command line parser needs to be split out into a
> separate funciton.
> ---
>  src/libvirt_private.syms  |   1 +
>  src/qemu/qemu_command.c   | 133 +++---
>  src/util/virstoragefile.c | 144 
> ++
>  src/util/virstoragefile.h |   3 +
>  4 files changed, 156 insertions(+), 125 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index b8f35e8..b33722e 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1991,6 +1991,7 @@ virStorageSourceInitChainElement;
>  virStorageSourceIsEmpty;
>  virStorageSourceIsLocalStorage;
>  virStorageSourceNewFromBacking;
> +virStorageSourceParseRBDColonString;
>  virStorageSourcePoolDefFree;
>  virStorageSourcePoolModeTypeFromString;
>  virStorageSourcePoolModeTypeToString;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 19e8f9d..021ec07 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2540,137 +2540,20 @@ qemuGetSecretString(virConnectPtr conn,
>  }
> 
> 
> -static int qemuAddRBDHost(virDomainDiskDefPtr disk, char *hostport)
> +static int
> +qemuParseRBDString(virDomainDiskDefPtr disk)
>  {
> -char *port;
> -size_t skip;
> -char **parts;
> -
> -if (VIR_EXPAND_N(disk->src->hosts, disk->src->nhosts, 1) < 0)
> -return -1;
> -
> -if ((port = strchr(hostport, ']'))) {
> -/* ipv6, strip brackets */
> -hostport += 1;
> -skip = 3;
> -} else {
> -port = strstr(hostport, "\\:");
> -skip = 2;
> -}
> -
> -if (port) {
> -*port = '\0';
> -port += skip;
> -if (VIR_STRDUP(disk->src->hosts[disk->src->nhosts - 1].port, port) < 
> 0)
> -goto error;
> -} else {
> -if (VIR_STRDUP(disk->src->hosts[disk->src->nhosts - 1].port, "6789") 
> < 0)
> -goto error;
> -}
> -
> -parts = virStringSplit(hostport, "\\:", 0);
> -if (!parts)
> -goto error;
> -disk->src->hosts[disk->src->nhosts-1].name = virStringJoin((const char 
> **)parts, ":");
> -virStringFreeList(parts);
> -if (!disk->src->hosts[disk->src->nhosts-1].name)
> -goto error;
> +char *source = disk->src->path;
> +int ret;
> 
> -disk->src->hosts[disk->src->nhosts-1].transport = 
> VIR_STORAGE_NET_HOST_TRANS_TCP;
> -disk->src->hosts[disk->src->nhosts-1].socket = NULL;
> +disk->src->path = NULL;
> 
> -return 0;
> +ret = virStorageSourceParseRBDColonString(source, disk->src);
> 
> - error:
> -VIR_FREE(disk->src->hosts[disk->src->nhosts-1].port);
> -VIR_FREE(disk->src->hosts[disk->src->nhosts-1].name);
> -return -1;
> +VIR_FREE(source);
> +return ret;
>  }
> 
> -/* disk->src initially has everything after the rbd: prefix */
> -static int qemuParseRBDString(virDomainDiskDefPtr disk)
> -{
> -char *options = NULL;
> -char *p, *e, *next;
> -virStorageAuthDefPtr authdef = NULL;
> -
> -p = strchr(disk->src->path, ':');
> -if (p) {
> -if (VIR_STRDUP(options, p + 1) < 0)
> -goto error;
> -*p = '\0';
> -}
> -
> -/* options */
> -if (!options)
> -return 0; /* all done */
> -
> -p = options;
> -while (*p) {
> -/* find : delimiter or end of string */
> -for (e = p; *e && *e != ':'; ++e) {
> -if (*e == '\\') {
> -e++;
> -if (*e == '\0')
> -break;
> -}
> -}
> -if (*e == '\0') {
> -next = e;/* last kv pair */
> -} else {
> -next = e + 1;
> -*e = '\0';
> -}
> -
> -if (STRPREFIX(p, "id=")) {
> -const char *secrettype;
> -/* formulate authdef for disk->src->auth */
> -if (VIR_ALLOC(authdef) < 0)
> -goto error;
> -
> -if (VIR_STRDUP(authdef->username, p + strlen("id=")) < 0)
> -goto error;
> -secrettype = 
> virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_CEPH);
> -if (VIR_STRDUP(authdef->secrettype, secrettype) < 0)
> -goto error;
> -disk->src->auth = authdef;
> -authdef = NULL;
> -
> -/* Cannot formulate a secretType (eg, usage or uuid) given
> - * what is provided.
> - */
> -}
> -if (STRPREFIX(p, "mon_host=")) {
> -char *h, *sep;
> -
> -h = p + strlen("mon_host=");
> -while (h < e) {
> -for (sep = h; sep < e; ++sep) {
> -if (*sep == '\\' && (sep[1] == ',' ||
> - sep[1] == ';' ||
> - sep[1] == ' ')) {
> -*sep = '\0

Re: [libvirt] [PATCH 06/12] qemu: Refactor qemuBuildNetworkDriveURI to take a virStorageSourcePtr

[John Ferlan]

On 11/12/2014 08:47 AM, Peter Krempa wrote:
> Instead of spliting out various fields, pass the complete structure and

s/spliting/splitting

> let the function pick various things of it.
> 
> As one of the callers isn't using virStorageSourcePtr to store the data,
> this patch adds glue code that fills the data into a dummy
> virStorageSourcePtr before calling the func.
> 
> This change will help when adding new fields that need output processing
> in the future.
> ---
>  src/qemu/qemu_command.c | 129 
> +++-
>  1 file changed, 62 insertions(+), 67 deletions(-)
> 

ACK

John

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

Re: [libvirt] [PATCH 05/12] util: storage: Copy hosts of a storage file only if they exist

[John Ferlan]

On 11/12/2014 08:47 AM, Peter Krempa wrote:
> If there are no hosts for a storage source virStorageSourceCopy would
> try to copy them anyways. As the success of virStorageNetHostDefCopy is
> determined by returning a pointer and malloc of 0 elements might return
> NULL according to the implementation, the result of the copy function
> may vary.
> 
> Fix this by copying the hosts array only if there are hosts defined.
> ---
>  src/util/virstoragefile.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 

I see 'virStorageSourceNewFromBackingRelative' also has the same issue.
Could use this opportunity to fix that - or modify the API to adjust the
return and only do the alloc if source->nhosts > 0.

I also note that virStorageNetHostDefCopy is only used in
virstoragefile.c too - cause for statification?

ACK - I'll let you figure the best option...  Definitely need a check in
both callers though.

John
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 8e9d115..c2d5b46 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1852,9 +1852,12 @@ virStorageSourceCopy(const virStorageSource *src,
>  VIR_STRDUP(ret->compat, src->compat) < 0)
>  goto error;
> 
> -if (!(ret->hosts = virStorageNetHostDefCopy(src->nhosts, src->hosts)))
> -goto error;
> -ret->nhosts = src->nhosts;
> +if (src->nhosts) {
> +if (!(ret->hosts = virStorageNetHostDefCopy(src->nhosts, 
> src->hosts)))
> +goto error;
> +
> +ret->nhosts = src->nhosts;
> +}
> 
>  if (src->srcpool &&
>  !(ret->srcpool = virStorageSourcePoolDefCopy(src->srcpool)))
> 

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

Re: [libvirt] [PATCH 03/12] util: buffer: Clarify scope of the escape operation in virBufferEscape

[John Ferlan]

On 11/12/2014 08:47 AM, Peter Krempa wrote:
> The escaping is applied only to the string, not the formating argument.
> State this fact in the docs.

s/formating/format


ACK

John
> ---
>  src/util/virbuffer.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 

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

Re: [libvirt] [PATCH 04/12] util: storage: Add notice for extension of struct virStorageSource

[John Ferlan]

On 11/12/2014 08:47 AM, Peter Krempa wrote:
> As we now have a deep copy function for struct virStorageSource add a
> notice that extensions of the structure require also appropriate changes
> to the virStorageSourceCopy func.
> ---
>  src/util/virstoragefile.h | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index 2583e10..7f3f353 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -228,9 +228,11 @@ typedef virStorageDriverData *virStorageDriverDataPtr;
>  typedef struct _virStorageSource virStorageSource;
>  typedef virStorageSource *virStorageSourcePtr;
> 
> -/* Stores information related to a host resource.  In the case of
> - * backing chains, multiple source disks join to form a single guest
> - * view.  */
> +/* Stores information related to a host resource.  In the case of backing
> + * chains, multiple source disks join to form a single guest view.
> + *
> + * IMPORTANT: When adding fields to this struct it's also necessary to add
> + * apropriate code to thevirStorageSourceCopy deep copy function */

s/apropriate/appropriate
s/thevir.../the vir.../

Too bad there wasn't some way to automagically detect this...

ACK

John

>  struct _virStorageSource {
>  int type; /* virStorageType */
>  char *path;
> 

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

[libvirt] [PATCH v3] qemu: Pass file descriptor when using TPM passthrough

[Stefan Berger]

Pass the TPM file descriptor to QEMU via command line.
Instead of passing /dev/tpm0 we now pass /dev/fdset/10 and the additional
parameters -add-fd set=10,fd=20.

This addresses the use case when QEMU is started with non-root privileges
and QEMU cannot open /dev/tpm0 for example.

One problem is that for the passing of the file descriptor set to work,
virCommandReorderFDs must not be called on the virCommand. This is prevented
by setting a flag in the virCommandPassFDGetFDIndex that is checked to be
clear when virCommandReorderFDs is run.

Signed-off-by: Stefan Berger 

v2->v3: Fixed some memory leaks
---
 src/libvirt_private.syms |   1 +
 src/qemu/qemu_command.c  | 136 ---
 src/util/vircommand.c|  33 
 src/util/vircommand.h|   3 ++
 4 files changed, 166 insertions(+), 7 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index aeec440..3194e8b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1164,6 +1164,7 @@ virCommandNewArgList;
 virCommandNewArgs;
 virCommandNonblockingFDs;
 virCommandPassFD;
+virCommandPassFDGetFDIndex;
 virCommandPassListenFDs;
 virCommandRawStatus;
 virCommandRequireHandshake;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 8ed7934..17debba 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -159,6 +159,58 @@ VIR_ENUM_IMPL(qemuNumaPolicy, VIR_DOMAIN_NUMATUNE_MEM_LAST,
   "interleave");
 
 /**
+ * qemuVirCommandGetFDSet:
+ * @cmd: the command to modify
+ * @fd: fd to reassign to the child
+ *
+ * Get the parameters for the QEMU -add-fd command line option
+ * for the given file descriptor. The file descriptor must previously
+ * have been 'transferred' in a virCommandPassFD() call.
+ * This function for example returns "set=10,fd=20".
+ */
+static char *
+qemuVirCommandGetFDSet(virCommandPtr cmd, int fd)
+{
+char *result = NULL;
+int idx = virCommandPassFDGetFDIndex(cmd, fd);
+
+if (idx >= 0) {
+ignore_value(virAsprintf(&result, "set=%d,fd=%d", idx, fd) < 0);
+} else {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("file descriptor %d has not been transferred"), fd);
+}
+
+return result;
+}
+
+/**
+ * qemuVirCommandGetDevSet:
+ * @cmd: the command to modify
+ * @fd: fd to reassign to the child
+ *
+ * Get the parameters for the QEMU path= parameter where a file
+ * descriptor is accessed via a file descriptor set, for example
+ * /dev/fdset/10. The file descriptor must previously have been
+ * 'transferred' in a virCommandPassFD() call.
+ */
+static char *
+qemuVirCommandGetDevSet(virCommandPtr cmd, int fd)
+{
+char *result = NULL;
+int idx = virCommandPassFDGetFDIndex(cmd, fd);
+
+if (idx >= 0) {
+ignore_value(virAsprintf(&result, "/dev/fdset/%d", idx) < 0);
+} else {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("file descriptor %d has not been transferred"), fd);
+}
+return result;
+}
+
+
+/**
  * qemuPhysIfaceConnect:
  * @def: the definition of the VM (needed by 802.1Qbh and audit)
  * @driver: pointer to the driver instance
@@ -5926,14 +5978,20 @@ qemuBuildRNGDeviceArgs(virCommandPtr cmd,
 
 
 static char *qemuBuildTPMBackendStr(const virDomainDef *def,
+virCommandPtr cmd,
 virQEMUCapsPtr qemuCaps,
-const char *emulator)
+const char *emulator,
+int *tpmfd, int *cancelfd)
 {
 const virDomainTPMDef *tpm = def->tpm;
 virBuffer buf = VIR_BUFFER_INITIALIZER;
 const char *type = virDomainTPMBackendTypeToString(tpm->type);
-char *cancel_path;
+char *cancel_path = NULL;
 const char *tpmdev;
+char *devset = NULL, *cancel_devset = NULL;
+
+*tpmfd = -1;
+*cancelfd = -1;
 
 virBufferAsprintf(&buf, "%s,id=tpm-%s", type, tpm->info.alias);
 
@@ -5946,11 +6004,49 @@ static char *qemuBuildTPMBackendStr(const virDomainDef 
*def,
 if (!(cancel_path = virTPMCreateCancelPath(tpmdev)))
 goto error;
 
-virBufferAddLit(&buf, ",path=");
-virBufferEscape(&buf, ',', ",", "%s", tpmdev);
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ADD_FD)) {
+*tpmfd = open(tpmdev, O_RDWR);
+if (*tpmfd < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Could not open TPM device %s"), tpmdev);
+goto error;
+}
+
+virCommandPassFD(cmd, *tpmfd,
+ VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+devset = qemuVirCommandGetDevSet(cmd, *tpmfd);
+if (devset == NULL)
+goto error;
+
+*cancelfd = open(cancel_path, O_WRONLY);
+if (*cancelfd < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+

Re: [libvirt] [PATCH 01/12] docs: domain: Move docs for storage hosts under the element

[John Ferlan]

On 11/12/2014 08:47 AM, Peter Krempa wrote:
> The docs describing the  element that are under the 
> element in the XML document were incorrectly placed under the 
> element. Move them to the correct place.
> ---
>  docs/formatdomain.html.in | 104 
> --
>  1 file changed, 54 insertions(+), 50 deletions(-)
> 

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index d4189e6..4f44bc0 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1953,6 +1953,60 @@
>  may have zero or more host sub-elements used to
>  specify the hosts to connect.
>  
> +
> +
> +  host
> +  The host element supports 4 attributes, viz.  
> "name",
> +"port", "transport" and "socket", which specify the hostname,
> +the port number, transport type and path to socket, respectively.
> +The meaning of this element and the number of the elements depend
> +on the protocol attribute.



> +
> +  
> + Protocol 
> + Meaning 
> + Number of hosts 
> + Default port 
> +  
> +  
> + nbd 
> + a server running nbd-server 
> + only one 
> + 10809 
> +  
> +  
> + iscsi 
> + an iSCSI server 
> + only one 
> + 3260 
> +  
> +  
> + rbd 
> + monitor servers of RBD 
> + one or more 
> + 6789 
> +  
> +  
> + sheepdog 
> + one of the sheepdog servers (default is localhost:7000) 
> 
> + zero or one 
> + 7000 
> +  
> +  
> + gluster 
> + a server running glusterd daemon 
> + only one 
> + 24007 
> +  
> +



ACK - adding the  is mostly a visual thing - it separates the
table from the text a bit... not sure if there's another way to do it as
my html skills are spartan.

John

> +gluster supports "tcp", "rdma", "unix" as valid values for the
> +transport attribute.  nbd supports "tcp" and "unix".  Others only
> +support "tcp".  If nothing is specified, "tcp" is assumed. If the
> +transport is "unix", the socket attribute specifies the path to 
> an
> +AF_UNIX socket.
> +  
> +
> +
>  
>  For a "file" or "volume" disk type which represents a cdrom or floppy
>  (the device attribute), it is possible to define
> @@ -2308,56 +2362,6 @@
>  characters.
>  Since 1.0.1
>
> -  host
> -  The host element supports 4 attributes, viz.  "name",
> -"port", "transport" and "socket", which specify the hostname, the 
> port
> - number, transport type and path to socket, respectively. The meaning
> - of this element and the number of the elements depend on the 
> protocol
> - attribute.
> -
> -  
> - Protocol 
> - Meaning 
> - Number of hosts 
> - Default port 
> -  
> -  
> - nbd 
> - a server running nbd-server 
> - only one 
> - 10809 
> -  
> -  
> - iscsi 
> - an iSCSI server 
> - only one 
> - 3260 
> -  
> -  
> - rbd 
> - monitor servers of RBD 
> - one or more 
> - 6789 
> -  
> -  
> - sheepdog 
> - one of the sheepdog servers (default is localhost:7000) 
> 
> - zero or one 
> - 7000 
> -  
> -  
> - gluster 
> - a server running glusterd daemon 
> - only one 
> - 24007 
> -  
> -
> -gluster supports "tcp", "rdma", "unix" as valid values for the
> -transport attribute.  nbd supports "tcp" and "unix".  Others only
> -support "tcp".  If nothing is specified, "tcp" is assumed. If the
> -transport is "unix", the socket attribute specifies the path to an
> -AF_UNIX socket.
> -  
>address
>If present, the address element ties the disk
>  to a given slot of a controller (the
> 

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

Re: [libvirt] [PATCH 2/2] storage: Add thread to refresh for createVport

[Michal Privoznik]

On 19.11.2014 21:52, John Ferlan wrote:

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

When libvirt create's the vport (VPORT_CREATE) for the vHBA, there isn't
enough "time" between the creation and the running of the following
backend->refreshPool after a backend->startPool in order to find the LU's.
Population of LU's happens asynchronously when udevEventHandleCallback
discovers the "new" vHBA port.  Creation of the infrastructure by udev
is an iterative process creating and discovering actual storage devices and
adjusting the environment.

Because of the time it takes to discover and set things up, the backend
refreshPool call done after the startPool call will generally fail to
find any devices. This leaves the newly started pool appear empty when
querying via 'vol-list' after startup. The "workaround" has always been
to run pool-refresh after startup (or any time thereafter) in order to
find the LU's. Depending on how quickly run after startup, this too may
not find any LUs in the pool. Eventually though given enough time and
retries it will find something if LU's exist for the vHBA.

This patch adds a thread to be executed after the VPORT_CREATE which will
attempt to find the LU's without requiring the external run of refresh-pool.
It does this by waiting for 5 seconds and searching for the LU's. If any
are found, then the thread completes; otherwise, it will retry once more
in another 5 seconds.  If none are found in that second pass, the thread
gives up.

Things learned while investigating this... No need to try and fill the
pool too quickly or too many times. Over the course of creation, the udev
code may 'add', 'change', and 'delete' the same device. So if the refresh
code runs and finds something, it may display it only to have a subsequent
refresh appear to "lose" the device. The udev processing doesn't seem to
have a way to indicate that it's all done with the creation processing of a
newly found vHBA. Only the Lone Ranger has silver bullets to fix everything.

Signed-off-by: John Ferlan 
---
  src/storage/storage_backend_scsi.c | 131 +
  1 file changed, 120 insertions(+), 11 deletions(-)


ACK

Michal

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

Re: [libvirt] [PATCH 1/2] storage: Fix issue finding LU's when block doesn't exist

[Michal Privoznik]

On 19.11.2014 21:52, John Ferlan wrote:

Fix a problem in the getBlockDevice and processLU where retval initialized
to zero causing some failures to erroneously continue through to the
virStorageBackendSCSINewLun with an attempt to find a path for "/dev/(null)".
This would fail approximately 10 seconds later with debug message:

virStorageBackendSCSINewLun:203 :
  No stable path found for '/dev/(null)' in '/dev/disk/by-path'

The root cause of the issue is for many /sys/bus/scsi/devices/
there is no "block*" device found for the vHBA's, where "" are
the various paths created for the vHBA, such as "17:0:0:0", "17:0:1:0",
"17:0:2:0", "17:0:3:0", etc.  If the block device isn't found, then the
directory should just be ignored rather than attempting to process it.

The bug was that in getBlockDevice the assumption was "block" would exist
and either getNewStyleBlockDevice or getOldStyleBlockDevice would fill in
@block_device. However, if 'block*' doesn't exist, then the code returned
NULL for block_device *and* a good (zero) retval value.  This caused the
processLU code to attempt the virStorageBackendSCSINewLun which failed
"at some point in time" in the future.

After this change - on test system the refresh-pool didn't have a noticable
pause of about 20 seconds - it completed within a second since no longer
was there an attempt/need to find "/dev/(null)".

Additionally, the virStorageBackendSCSIFindLU's shouldn't be declaring
found unless the processLU actually returns success. This will be
important in the followup patch which relies on whether a LU was found.

Signed-off-by: John Ferlan 
---
  src/storage/storage_backend_scsi.c | 18 +-
  1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/storage/storage_backend_scsi.c 
b/src/storage/storage_backend_scsi.c
index c952b71..5e4f36e 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -321,7 +321,7 @@ getBlockDevice(uint32_t host,
  char *lun_path = NULL;
  DIR *lun_dir = NULL;
  struct dirent *lun_dirent = NULL;
-int retval = 0;
+int retval = -1;
  int direrr;

  if (virAsprintf(&lun_path, "/sys/bus/scsi/devices/%u:%u:%u:%u",
@@ -333,7 +333,6 @@ getBlockDevice(uint32_t host,
  virReportSystemError(errno,
   _("Failed to opendir sysfs path '%s'"),
   lun_path);
-retval = -1;
  goto out;
  }

@@ -368,7 +367,7 @@ processLU(virStoragePoolObjPtr pool,
uint32_t lun)
  {
  char *type_path = NULL;
-int retval = 0;
+int retval = -1;
  int device_type;
  char *block_device = NULL;

@@ -379,7 +378,6 @@ processLU(virStoragePoolObjPtr pool,
  virReportError(VIR_ERR_INTERNAL_ERROR,
 _("Failed to determine if %u:%u:%u:%u is a Direct-Access 
LUN"),
 host, bus, target, lun);
-retval = -1;
  goto out;
  }

@@ -396,17 +394,19 @@ processLU(virStoragePoolObjPtr pool,
  VIR_DEBUG("%u:%u:%u:%u is a Direct-Access LUN",
host, bus, target, lun);

-if (getBlockDevice(host, bus, target, lun, &block_device) < 0)
+if (getBlockDevice(host, bus, target, lun, &block_device) < 0) {
+VIR_DEBUG("Failed to find block device for this LUN");
  goto out;
+}

  if (virStorageBackendSCSINewLun(pool,
  host, bus, target, lun,
  block_device) < 0) {
  VIR_DEBUG("Failed to create new storage volume for %u:%u:%u:%u",
host, bus, target, lun);
-retval = -1;
  goto out;
  }
+retval = 0;

  VIR_DEBUG("Created new storage volume for %u:%u:%u:%u successfully",
host, bus, target, lun);
@@ -451,10 +451,10 @@ virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool,
  continue;
  }

-found = true;
-VIR_DEBUG("Found LU '%s'", lun_dirent->d_name);
+VIR_DEBUG("Found possible LU '%s'", lun_dirent->d_name);

-processLU(pool, scanhost, bus, target, lun);
+if (processLU(pool, scanhost, bus, target, lun) == 0)
+found = true;


Do we want 'break' here to jump out from the loop too?


  }

  if (!found)



Either way, that's just an optimization, so ACK.

Michal

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

[libvirt] [PATCHv5 1/3] leaseshelper: improvements to support all events

[Peter Krempa]

From: Nehal J Wani 

This patch enables the helper program to detect event(s) triggered when
there is a change in lease length or expiry and client-id. This
transfers complete control of leases database to libvirt and obsoletes
use of the lease database file (.leases). That file will
not be created, read, or written.  This is achieved by adding the option
--leasefile-ro to dnsmasq and passing a custom env var to leaseshelper,
which helps us map events related to leases with their corresponding
network bridges, no matter what the event be.

Also, this requires the addition of a new non-lease entry in our custom
lease database: "server-duid". It is required to identify a DHCPv6
server.

Now that dnsmasq doesn't maintain its own leases database, it relies on
our helper program to tell it about previous leases and server duid.
Thus, this patch makes our leases program honor an extra action: "init",
in which it sends the known info in a particular format to dnsmasq
by printing it to stdout.

The drawback of this change is that upgrade to this new approach does
not transfer the existing leases for the network if the leaseshelper
wasn't already used.
---

Notes:
Version 5:
- fixed crash in the "init" operation
- mentioned the loss of previous records in the commit message

 src/network/bridge_driver.c |   3 +
 src/network/leaseshelper.c  | 160 ++--
 2 files changed, 128 insertions(+), 35 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 6cb421c..6ecbc37 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1280,7 +1280,10 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr 
network,

 cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps));
 virCommandAddArgFormat(cmd, "--conf-file=%s", configfile);
+/* Libvirt gains full control of leases database */
+virCommandAddArgFormat(cmd, "--leasefile-ro");
 virCommandAddArgFormat(cmd, "--dhcp-script=%s", leaseshelper_path);
+virCommandAddEnvPair(cmd, "VIR_BRIDGE_NAME", network->def->bridge);

 *cmdout = cmd;
 ret = 0;
diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c
index 5b3c9c3..806e82d 100644
--- a/src/network/leaseshelper.c
+++ b/src/network/leaseshelper.c
@@ -50,6 +50,12 @@
  */
 #define VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX (32 * 1024 * 1024)

+/*
+ * Use this when passing possibly-NULL strings to printf-a-likes.
+ * Required for unknown parameters during init call.
+ */
+#define EMPTY_STR(s) ((s) ? (s) : "*")
+
 static const char *program_name;

 /* Display version information. */
@@ -65,7 +71,7 @@ usage(int status)
 if (status) {
 fprintf(stderr, _("%s: try --help for more details\n"), program_name);
 } else {
-printf(_("Usage: %s add|old|del mac|clientid ip [hostname]\n"
+printf(_("Usage: %s add|old|del|init mac|clientid ip [hostname]\n"
  "Designed for use with 'dnsmasq --dhcp-script'\n"
  "Refer to man page of dnsmasq for more details'\n"),
program_name);
@@ -89,6 +95,7 @@ enum virLeaseActionFlags {
 VIR_LEASE_ACTION_ADD,   /* Create new lease */
 VIR_LEASE_ACTION_OLD,   /* Lease already exists, renew it */
 VIR_LEASE_ACTION_DEL,   /* Delete the lease */
+VIR_LEASE_ACTION_INIT,  /* Tell dnsmasq of existing leases on restart 
*/

 VIR_LEASE_ACTION_LAST
 };
@@ -96,7 +103,7 @@ enum virLeaseActionFlags {
 VIR_ENUM_DECL(virLeaseAction);

 VIR_ENUM_IMPL(virLeaseAction, VIR_LEASE_ACTION_LAST,
-  "add", "old", "del");
+  "add", "old", "del", "init");

 int
 main(int argc, char **argv)
@@ -107,12 +114,15 @@ main(int argc, char **argv)
 char *custom_lease_file = NULL;
 const char *ip = NULL;
 const char *mac = NULL;
+const char *ip_tmp = NULL;
+const char *leases_str = NULL;
+const char *server_duid_tmp = NULL;
 const char *iaid = virGetEnvAllowSUID("DNSMASQ_IAID");
 const char *clientid = virGetEnvAllowSUID("DNSMASQ_CLIENT_ID");
 const char *interface = virGetEnvAllowSUID("DNSMASQ_INTERFACE");
 const char *exptime_tmp = virGetEnvAllowSUID("DNSMASQ_LEASE_EXPIRES");
 const char *hostname = virGetEnvAllowSUID("DNSMASQ_SUPPLIED_HOSTNAME");
-const char *leases_str = NULL;
+const char *server_duid = virGetEnvAllowSUID("DNSMASQ_SERVER_DUID");
 long long currtime = 0;
 long long expirytime = 0;
 size_t i = 0;
@@ -120,7 +130,6 @@ main(int argc, char **argv)
 int pid_file_fd = -1;
 int rv = EXIT_FAILURE;
 int custom_lease_file_len = 0;
-bool add = false;
 bool delete = false;
 virJSONValuePtr lease_new = NULL;
 virJSONValuePtr lease_tmp = NULL;
@@ -156,16 +165,17 @@ main(int argc, char **argv)
 }
 }

-if (argc != 4 && argc != 5) {
+if (argc != 4 && argc != 5 && argc != 2) {
 /* Refer man page of dnsmasq --dhcp-script for more details */
 usage(EXIT_FAI

[libvirt] [PATCHv5 0/3] leaseshelper: Couple of improvements

[Peter Krempa]

Nehal J Wani (1):
  leaseshelper: improvements to support all events

Peter Krempa (2):
  leaseshelper: Refactor control flow
  network: dnsmasq: Don't format lease file path

 src/network/bridge_driver.c|  16 +-
 src/network/bridge_driver.h|   3 -
 src/network/leaseshelper.c | 246 +++--
 tests/networkxml2confdata/dhcp6-nat-network.conf   |   1 -
 tests/networkxml2confdata/dhcp6-network.conf   |   1 -
 tests/networkxml2confdata/isolated-network.conf|   1 -
 .../nat-network-dns-srv-record-minimal.conf|   1 -
 .../nat-network-dns-srv-record.conf|   1 -
 .../nat-network-dns-txt-record.conf|   1 -
 tests/networkxml2confdata/nat-network.conf |   1 -
 tests/networkxml2confdata/netboot-network.conf |   1 -
 .../networkxml2confdata/netboot-proxy-network.conf |   1 -
 tests/networkxml2conftest.c|  12 -
 13 files changed, 181 insertions(+), 105 deletions(-)

-- 
2.1.0

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

[libvirt] [PATCHv5 3/3] network: dnsmasq: Don't format lease file path

[Peter Krempa]

Now that we don't use the leases file at all for leases just don't
format it into the config and use the leaseshelper to do all the
lifting.
---
 src/network/bridge_driver.c | 13 ++---
 src/network/bridge_driver.h |  3 ---
 tests/networkxml2confdata/dhcp6-nat-network.conf|  1 -
 tests/networkxml2confdata/dhcp6-network.conf|  1 -
 tests/networkxml2confdata/isolated-network.conf |  1 -
 .../nat-network-dns-srv-record-minimal.conf |  1 -
 tests/networkxml2confdata/nat-network-dns-srv-record.conf   |  1 -
 tests/networkxml2confdata/nat-network-dns-txt-record.conf   |  1 -
 tests/networkxml2confdata/nat-network.conf  |  1 -
 tests/networkxml2confdata/netboot-network.conf  |  1 -
 tests/networkxml2confdata/netboot-proxy-network.conf|  1 -
 tests/networkxml2conftest.c | 12 
 12 files changed, 2 insertions(+), 35 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 6ecbc37..9355003 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -209,9 +209,6 @@ networkDnsmasqLeaseFileNameDefault(const char *netname)
 return leasefile;
 }

-networkDnsmasqLeaseFileNameFunc networkDnsmasqLeaseFileName =
-networkDnsmasqLeaseFileNameDefault;
-
 static char *
 networkDnsmasqLeaseFileNameCustom(const char *bridge)
 {
@@ -273,7 +270,7 @@ networkRemoveInactive(virNetworkObjPtr net)
 goto cleanup;
 }

-if (!(leasefile = networkDnsmasqLeaseFileName(def->name)))
+if (!(leasefile = networkDnsmasqLeaseFileNameDefault(def->name)))
 goto cleanup;

 if (!(customleasefile = networkDnsmasqLeaseFileNameCustom(def->bridge)))
@@ -1183,14 +1180,8 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
 ipdef = (ipdef == ipv6def) ? NULL : ipv6def;
 }

-if (nbleases > 0) {
-char *leasefile = networkDnsmasqLeaseFileName(network->def->name);
-if (!leasefile)
-goto cleanup;
-virBufferAsprintf(&configbuf, "dhcp-leasefile=%s\n", leasefile);
-VIR_FREE(leasefile);
+if (nbleases > 0)
 virBufferAsprintf(&configbuf, "dhcp-lease-max=%d\n", nbleases);
-}

 /* this is done once per interface */
 if (networkBuildDnsmasqHostsList(dctx, dns) < 0)
diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h
index decc08f..2f801ee 100644
--- a/src/network/bridge_driver.h
+++ b/src/network/bridge_driver.h
@@ -64,7 +64,4 @@ int networkDnsmasqConfContents(virNetworkObjPtr network,

 typedef char *(*networkDnsmasqLeaseFileNameFunc)(const char *netname);

-/* this allows the testsuite to replace the lease filename resolver function */
-extern networkDnsmasqLeaseFileNameFunc networkDnsmasqLeaseFileName;
-
 #endif /* __VIR_NETWORK__DRIVER_H */
diff --git a/tests/networkxml2confdata/dhcp6-nat-network.conf 
b/tests/networkxml2confdata/dhcp6-nat-network.conf
index f270a43..922eb7a 100644
--- a/tests/networkxml2confdata/dhcp6-nat-network.conf
+++ b/tests/networkxml2confdata/dhcp6-nat-network.conf
@@ -11,7 +11,6 @@ interface=virbr0
 dhcp-range=192.168.122.2,192.168.122.254
 dhcp-no-override
 dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff
-dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases
 dhcp-lease-max=493
 dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile
 addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts
diff --git a/tests/networkxml2confdata/dhcp6-network.conf 
b/tests/networkxml2confdata/dhcp6-network.conf
index f0a9660..064515f 100644
--- a/tests/networkxml2confdata/dhcp6-network.conf
+++ b/tests/networkxml2confdata/dhcp6-network.conf
@@ -11,7 +11,6 @@ except-interface=lo
 bind-dynamic
 interface=virbr0
 dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff
-dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases
 dhcp-lease-max=240
 dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile
 addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts
diff --git a/tests/networkxml2confdata/isolated-network.conf 
b/tests/networkxml2confdata/isolated-network.conf
index 6ba34ae..fbdf75a 100644
--- a/tests/networkxml2confdata/isolated-network.conf
+++ b/tests/networkxml2confdata/isolated-network.conf
@@ -12,7 +12,6 @@ dhcp-option=3
 no-resolv
 dhcp-range=192.168.152.2,192.168.152.254
 dhcp-no-override
-dhcp-leasefile=/var/lib/libvirt/dnsmasq/private.leases
 dhcp-lease-max=253
 dhcp-hostsfile=/var/lib/libvirt/dnsmasq/private.hostsfile
 addn-hosts=/var/lib/libvirt/dnsmasq/private.addnhosts
diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf 
b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf
index e60411b..08ed672 100644
--- a/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf
+++ b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf
@@ -15,7 +15,6 @@ listen-address=10.24.10.1
 sr

[libvirt] [PATCHv5 2/3] leaseshelper: Refactor control flow

[Peter Krempa]

Untangle a few conditions into a case statement and improve reporting of
invaid commands.
---
 src/network/leaseshelper.c | 100 ++---
 1 file changed, 58 insertions(+), 42 deletions(-)

diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c
index 806e82d..96a1de2 100644
--- a/src/network/leaseshelper.c
+++ b/src/network/leaseshelper.c
@@ -180,7 +180,11 @@ main(int argc, char **argv)

 ip = argv[3];
 mac = argv[2];
-action = virLeaseActionTypeFromString(argv[1]);
+
+if ((action = virLeaseActionTypeFromString(argv[1])) < 0) {
+fprintf(stderr, _("Unsupported action: %s\n"), argv[1]);
+exit(EXIT_FAILURE);
+}

 /* In case hostname is known, it is the 5th argument */
 if (argc == 5)
@@ -230,9 +234,47 @@ main(int argc, char **argv)
 goto cleanup;
 }

-if (action == VIR_LEASE_ACTION_ADD ||
-action == VIR_LEASE_ACTION_OLD ||
-action == VIR_LEASE_ACTION_DEL) {
+switch ((enum virLeaseActionFlags) action) {
+case  VIR_LEASE_ACTION_ADD:
+case VIR_LEASE_ACTION_OLD:
+if (!mac)
+break;
+delete = true;
+
+/* Create new lease */
+if (!(lease_new = virJSONValueNewObject())) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("failed to create json"));
+goto cleanup;
+}
+
+if (!exptime ||
+virStrToLong_ll(exptime, NULL, 10, &expirytime) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Unable to convert lease expiry time to long 
long: %s"),
+   NULLSTR(exptime));
+goto cleanup;
+}
+
+if (iaid && virJSONValueObjectAppendString(lease_new, "iaid", iaid) < 
0)
+goto cleanup;
+if (ip && virJSONValueObjectAppendString(lease_new, "ip-address", ip) 
< 0)
+goto cleanup;
+if (mac && virJSONValueObjectAppendString(lease_new, "mac-address", 
mac) < 0)
+goto cleanup;
+if (hostname && virJSONValueObjectAppendString(lease_new, "hostname", 
hostname) < 0)
+goto cleanup;
+if (clientid && virJSONValueObjectAppendString(lease_new, "client-id", 
clientid) < 0)
+goto cleanup;
+if (server_duid && virJSONValueObjectAppendString(lease_new, 
"server-duid", server_duid) < 0)
+goto cleanup;
+if (expirytime && virJSONValueObjectAppendNumberLong(lease_new, 
"expiry-time", expirytime) < 0)
+goto cleanup;
+
+break;
+
+case VIR_LEASE_ACTION_DEL:
+delete = true;
 /* Custom ipv6 leases *will not* be created if the env-var DNSMASQ_MAC
  * is not set. In the special case, when the $(interface).status file
  * is not already present and dnsmasq is (re)started, the corresponding
@@ -246,46 +288,15 @@ main(int argc, char **argv)
  * the new lease will be created irrespective of whether the MACID is
  * known or not.
  */
-if (mac || action == VIR_LEASE_ACTION_DEL) {
+if (mac) {
 /* Delete the corresponding lease, if it already exists */
 delete = true;
-if (action == VIR_LEASE_ACTION_ADD ||
-action == VIR_LEASE_ACTION_OLD) {
-/* Create new lease */
-if (!(lease_new = virJSONValueNewObject())) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("failed to create json"));
-goto cleanup;
-}
-
-if (!exptime ||
-virStrToLong_ll(exptime, NULL, 10, &expirytime) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Unable to convert lease expiry time to 
long long: %s"),
-   NULLSTR(exptime));
-goto cleanup;
-}
-
-if (iaid && virJSONValueObjectAppendString(lease_new, "iaid", 
iaid) < 0)
-goto cleanup;
-if (ip && virJSONValueObjectAppendString(lease_new, 
"ip-address", ip) < 0)
-goto cleanup;
-if (mac && virJSONValueObjectAppendString(lease_new, 
"mac-address", mac) < 0)
-goto cleanup;
-if (hostname && virJSONValueObjectAppendString(lease_new, 
"hostname", hostname) < 0)
-goto cleanup;
-if (clientid && virJSONValueObjectAppendString(lease_new, 
"client-id", clientid) < 0)
-goto cleanup;
-if (server_duid && virJSONValueObjectAppendString(lease_new, 
"server-duid", server_duid) < 0)
-goto cleanup;
-if (expirytime && 
virJSONValueObjectAppendNumberLong(lease_new, "expiry-time", expirytime) < 0)
-goto cleanup;
-}
 }
-} e

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

[Vasiliy Tolstov]

2014-11-20 7:58 GMT+03:00 Anirban Chakraborty :
> Your original error message was that qemu couldn¹t find a tap device. So,
> I am not sure how the above error relates to your original error.
> Can you write your detail configuration setup and the steps you have taken
> to get at this error?


If i'm not wrong error goes from:
qemuBuildInterfaceCommandLine builds interface command line and before
qemu started tries to add tap device to tc, but tap created later via
qemu (in my case).
I think that virNetDevBandwidthSet needs to be run after qemu started.


-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru
jabber: v...@selfip.ru

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

Re: [libvirt] [PATCH 0/8] Add XML validation to the APIs

[Daniel P. Berrange]

On Thu, Nov 20, 2014 at 12:02:12PM +0100, Martin Kletzander wrote:
> On Wed, Nov 19, 2014 at 12:51:22PM +, Daniel P. Berrange wrote:
> >On Wed, Nov 19, 2014 at 09:45:39AM +, Daniel P. Berrange wrote:
> >>On Wed, Nov 19, 2014 at 08:23:22AM +0100, Martin Kletzander wrote:
> >>> On Tue, Nov 18, 2014 at 05:59:47PM +, Daniel P. Berrange wrote:
> >>> >This proof of concept patch extends the virDomainDefineXML
> >>> >and virDomainCreateXML APIs so that they can validate
> >>> >the user supplied XML document against the RNG schemas.
> >>> >
> >>> >The virsh command will enable validation by default, it
> >>> >must be turned off with --skip-validation if desired.
> >>> >
> >>> >This series is not complete
> >>> >
> >>> >- The network, interface, storage pool, etc APIs are
> >>> >  not wired up to support validation.
> >>> >- Only the QEMU virt driver is wired up to validate
> >>> >- The virsh edit command is not wired up to validate
> >>> >
> >>> >It is enough to demonstrate it working with 'virsh define'
> >>> >and the QEMU driver though.
> >>> >
> >>> >The biggest problem I see is the really awful error
> >>> >messages we get back from libxml2 when validation
> >>> >fails :-( They are essentially useless :-(
> >
> >>> >
> >>>
> >>> This is one of the things why I'm not convinced this work is worth
> >>> it.  It may be nice if we tell the user their XML is invalid instead
> >>> of silently losing information.  But error message similar to "invalid
> >>> element in interleave" doesn't help much when you are adding 100-line
> >>> XML.  There are some better validators, but requiring those would be
> >>> too cumbersome.
> >>
> >>At least when using 'virsh edit' you would know what element you
> >>just changed / added. So if you got get a generic 'validation failed'
> >>error you have a pretty good idea of where in teh document you made
> >>the mistake. So I think it'd be useful in that scenario. The error
> >>reporting is more of a problem for the apps where they're passing in
> >>a big XML document to define the guest and basically anything could
> >>be wrong.
> >
> >So, it seems not all of the error messages are so awful. It does a bad
> >job of reporting unknown elements, but if you have an unknown attribute
> >it does better:
> >
> > "Invalid attribute foo for element name"
> >
> >If you give an invalid value for an attribute which is an enum it
> >is semi-usefull
> >
> > "Element domain failed to validate attributes"
> >
> >So this does seem somewhat more useful to have in libvirt
> >
> 
> As I said, I'm not against this, I agree that it will still be useful.
> 
> If you meant this as an RFC, then I misunderstood that and I should've
> just wrote that as an initial PoC it's fine with me :)
> 
> Do you want me to finish the review?

Actually if you want to review patches 4, 5, 6, 7 that would be useful.
Those are general refactoring of the way we handle flags with the XML
parsers/formatters. The 7th patch was awful to create and will be a
rebase nightmare if we leave it too long.


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

[libvirt] [PATCH] build: fix build with older dbus headers

[Eric Blake]

Compilation on a RHEL 5 host failed, due to the older dbus headers
present on that machine, and triggered by commit 2aa167ca:

util/virdbus.c: In function 'virDBusMessageIterDecode':
util/virdbus.c:952: error: 'DBusBasicValue' undeclared (first use in this 
function)

* m4/virt-dbus.m4 (LIBVIRT_CHECK_DBUS): Check for DBusBasicValue.
* src/util/virdbuspriv.h (DBusBasicValue): Provide fallback.

Signed-off-by: Eric Blake 
---

Pushing under the build-breaker rule.

 m4/virt-dbus.m4|  3 ++-
 src/util/virdbuspriv.h | 17 -
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/m4/virt-dbus.m4 b/m4/virt-dbus.m4
index 4ef0c82..3f9b306 100644
--- a/m4/virt-dbus.m4
+++ b/m4/virt-dbus.m4
@@ -1,6 +1,6 @@
 dnl The libdbus.so library
 dnl
-dnl Copyright (C) 2012-2013 Red Hat, Inc.
+dnl Copyright (C) 2012-2014 Red Hat, Inc.
 dnl
 dnl This library is free software; you can redistribute it and/or
 dnl modify it under the terms of the GNU Lesser General Public
@@ -26,6 +26,7 @@ AC_DEFUN([LIBVIRT_CHECK_DBUS],[
 CFLAGS="$CFLAGS $DBUS_CFLAGS"
 LIBS="$LIBS $DBUS_LIBS"
 AC_CHECK_FUNCS([dbus_watch_get_unix_fd])
+AC_CHECK_TYPES([DBusBasicValue], [], [], [[#include ]])
 CFLAGS="$old_CFLAGS"
 LIBS="$old_LIBS"
   fi
diff --git a/src/util/virdbuspriv.h b/src/util/virdbuspriv.h
index d45fb25..4247746 100644
--- a/src/util/virdbuspriv.h
+++ b/src/util/virdbuspriv.h
@@ -1,7 +1,7 @@
 /*
  * virdbuspriv.h: internal APIs for testing DBus code
  *
- * Copyright (C) 2012-2013 Red Hat, Inc.
+ * Copyright (C) 2012-2014 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -24,6 +24,21 @@

 # include "virdbus.h"

+# if !HAVE_DBUSBASICVALUE
+/* Copied (and simplified) from dbus 1.6.12, for use with older dbus headers */
+typedef union
+{
+  dbus_int16_t  i16;   /**< as int16 */
+  dbus_uint16_t u16;   /**< as int16 */
+  dbus_int32_t  i32;   /**< as int32 */
+  dbus_uint32_t u32;   /**< as int32 */
+  dbus_bool_t   bool_val; /**< as boolean */
+  dbus_int64_t  i64;   /**< as int64 */
+  dbus_uint64_t u64;   /**< as int64 */
+  double dbl;  /**< as double */
+  unsigned char byt;   /**< as byte */
+} DBusBasicValue;
+# endif

 int virDBusMessageEncodeArgs(DBusMessage* msg,
  const char *types,
-- 
1.9.3

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

Re: [libvirt] [libvirt PATCH] rpc: do not fail if the pid of the connecting process is not set

[Giuseppe Scrivano]

Martin Kletzander  writes:

> On Wed, Nov 19, 2014 at 06:29:48PM +0100, Giuseppe Scrivano wrote:
>>getsockopt(sock->fd, SOL_SOCKET, SO_PEERCRED, ...) sets the pid to 0
>>when the process that opens the connection is in another container.
>>
>>Signed-off-by: Giuseppe Scrivano 
>>---
>> src/rpc/virnetsocket.c | 8 ++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>>diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
>>index 882fbec..6b019cc 100644
>>--- a/src/rpc/virnetsocket.c
>>+++ b/src/rpc/virnetsocket.c
>>@@ -1251,10 +1251,14 @@ int virNetSocketGetUNIXIdentity(virNetSocketPtr sock,
>> goto cleanup;
>> }
>>
>>-if (virProcessGetStartTime(cr.pid, timestamp) < 0)
>>+*timestamp = -1;
>>+if (cr.pid && virProcessGetStartTime(cr.pid, timestamp) < 0)
>> goto cleanup;
>>
>>-*pid = cr.pid;
>>+if (cr.pid)
>>+*pid = cr.pid;
>>+else
>>+*pid = -1;
>> *uid = cr.uid;
>> *gid = cr.gid;
>>
>>--
>>1.9.3
>>
>
> ACK,
>
> Martin

Thanks, pushed now.

Giuseppe

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

Re: [libvirt] [PATCH] util: don't log failure when older iptables lacks -w

[Eric Blake]

On 11/20/2014 12:38 AM, Peter Krempa wrote:
> On 11/20/14 00:21, Eric Blake wrote:
>> Commit dc33e6e4 caused older platforms like Fedora 20 to emit
>> scary log messages at startup:
>>
>> 2014-11-19 23:12:58.800+: 28906: error : virCommandWait:2532 : internal 
>> error: Child process (/usr/sbin/iptables -w -L -n) unexpected exit status 2: 
>> iptables v1.4.19.1: unknown option "-w"
>> Try `iptables -h' or 'iptables --help' for more information.
>>
>> Since we are probing and expect to handle the case where -w is not
>> supported, we should not let virCommand log it as an error.
>>
>> * src/util/virfirewall.c (virFirewallCheckUpdateLock): Handle
>> non-zero status ourselves.
>>
>> Signed-off-by: Eric Blake 
>> ---
>>  src/util/virfirewall.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
> 
> ACK

Thanks; 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 06/11] qemu: Add handling for VSERPORT_CHANGE event

[Peter Krempa]

On 11/20/14 14:11, Eric Blake wrote:
> On 11/20/2014 12:47 AM, Peter Krempa wrote:
>> On 11/20/14 08:44, Wang Rui wrote:
>>> On 2014/11/19 18:23, Peter Krempa wrote:
 New qemu added a new event that is emitted when a virtio serial channel
 is opened in the guest OS. This allows us to update the state of the
 port in the output-only XML element.

> 
>>> Hi, Peter
>>> IIUC, QEMU emitted the event and libvirt saved the state for the next time
>>> being queryed. 'the output-only XML element' and 'SaveStatus' means the 
>>> state
>>> is not saved persistently.
>>
>> This saves the state of the port into the status XML and
>>
>>>
>>> In case of libvirtd being restarted after state is saved, we'll lose it. 
>>> Could
>>> we handle this case?
>>
>> the status XML is the piece that is reloaded on libvirtd restart for
>> running VMs. For inactive VMs this doesn't make sense to report.
> 
> When reconnecting to the QMP monitor after a libvirtd restart, we need
> to query for whether the agent state has changed in the time that
> libvirt was not receiving events.  At the same time as VSERPORT_CHANGE
> was added as an event, Laszlo also enhanced the 'query-chardev' command
> to also poll the current connectedness of the agent channel (qemu commit
> 32a97ea).  This series needs to call that command on reconnect instead
> of trusting that the state saved prior to libvirtd restart is still valid.
> 

See patch 8/11.

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 06/11] qemu: Add handling for VSERPORT_CHANGE event

[Eric Blake]

On 11/20/2014 12:47 AM, Peter Krempa wrote:
> On 11/20/14 08:44, Wang Rui wrote:
>> On 2014/11/19 18:23, Peter Krempa wrote:
>>> New qemu added a new event that is emitted when a virtio serial channel
>>> is opened in the guest OS. This allows us to update the state of the
>>> port in the output-only XML element.
>>>

>> Hi, Peter
>> IIUC, QEMU emitted the event and libvirt saved the state for the next time
>> being queryed. 'the output-only XML element' and 'SaveStatus' means the state
>> is not saved persistently.
> 
> This saves the state of the port into the status XML and
> 
>>
>> In case of libvirtd being restarted after state is saved, we'll lose it. 
>> Could
>> we handle this case?
> 
> the status XML is the piece that is reloaded on libvirtd restart for
> running VMs. For inactive VMs this doesn't make sense to report.

When reconnecting to the QMP monitor after a libvirtd restart, we need
to query for whether the agent state has changed in the time that
libvirt was not receiving events.  At the same time as VSERPORT_CHANGE
was added as an event, Laszlo also enhanced the 'query-chardev' command
to also poll the current connectedness of the agent channel (qemu commit
32a97ea).  This series needs to call that command on reconnect instead
of trusting that the state saved prior to libvirtd restart is still valid.

-- 
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] [libvirt PATCH] rpc: do not fail if the pid of the connecting process is not set

[Martin Kletzander]

On Wed, Nov 19, 2014 at 06:29:48PM +0100, Giuseppe Scrivano wrote:

getsockopt(sock->fd, SOL_SOCKET, SO_PEERCRED, ...) sets the pid to 0
when the process that opens the connection is in another container.

Signed-off-by: Giuseppe Scrivano 
---
src/rpc/virnetsocket.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 882fbec..6b019cc 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -1251,10 +1251,14 @@ int virNetSocketGetUNIXIdentity(virNetSocketPtr sock,
goto cleanup;
}

-if (virProcessGetStartTime(cr.pid, timestamp) < 0)
+*timestamp = -1;
+if (cr.pid && virProcessGetStartTime(cr.pid, timestamp) < 0)
goto cleanup;

-*pid = cr.pid;
+if (cr.pid)
+*pid = cr.pid;
+else
+*pid = -1;
*uid = cr.uid;
*gid = cr.gid;

--
1.9.3



ACK,

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 v3 03/12] parallels: move parallelsDomNotFoundError to parallels_utils.h

[Peter Krempa]

On 11/18/14 14:16, Dmitry Guryanov wrote:
> Move macro parallelsDomNotFoundError to file
> parallels_utils.h, because it will be used in
> parallels_sdk.c.
> 
> Signed-off-by: Dmitry Guryanov 
> ---
>  src/parallels/parallels_driver.c | 8 
>  src/parallels/parallels_utils.h  | 8 
>  2 files changed, 8 insertions(+), 8 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: Really fix crash in tunnelled migration

[Peter Krempa]

On 11/20/14 13:46, Jiri Denemark wrote:
> Oops, I forgot to squash one more instance of the same check into the
> previous commit (v1.2.10-144-g52691f9).
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1147331
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_migration.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

ACK



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

[libvirt] [PATCH] qemu: Really fix crash in tunnelled migration

[Jiri Denemark]

Oops, I forgot to squash one more instance of the same check into the
previous commit (v1.2.10-144-g52691f9).

https://bugzilla.redhat.com/show_bug.cgi?id=1147331
Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_migration.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 89313df..a1b1458 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2757,7 +2757,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
 QEMU_ASYNC_JOB_MIGRATION_IN) < 0)
 goto stop;
 
-if (STREQ(protocol, "rdma") &&
+if (STREQ_NULLABLE(protocol, "rdma") &&
 virProcessSetMaxMemLock(vm->pid, vm->def->mem.hard_limit << 10) < 0) {
 goto stop;
 }
-- 
2.1.3

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

Re: [libvirt] [PATCH v3 01/12] parallels: move IS_CT macro to parallels_utils.h

[Peter Krempa]

On 11/18/14 14:16, Dmitry Guryanov wrote:
> This macro will be used in paralles_sdk.c so move
> it to common header.
> 
> Signed-off-by: Dmitry Guryanov 
> ---
>  src/parallels/parallels_driver.c | 2 --
>  src/parallels/parallels_utils.h  | 2 ++
>  2 files changed, 2 insertions(+), 2 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] [PATCHv3] virsh: Fix msg: blockjob is aborted from another client

[Peter Krempa]

On 11/20/14 12:52, Erik Skultety wrote:
> When a block{pull, copy, commit} is aborted via keyboard interrupt,
> the job is properly canceled followed by proper error message.
> However, when the job receives an abort from another client connected
> to the same domain, the error message incorrectly indicates that
> a blockjob has been finished successfully, though the abort request
> took effect. This patch introduces a new blockjob abort handler, which
> is registered when the client calls block{copy,commit,pull} routine,
> providing its caller the status of the finished blockjob.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1135442
> ---
>  tools/virsh-domain.c | 71 
> +---
>  1 file changed, 68 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index a7e9151..0891821 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -1709,6 +1709,17 @@ static void vshCatchInt(int sig ATTRIBUTE_UNUSED,
>  intCaught = 1;
>  }
>  
> +static void
> +vshBlockJobStatusHandler(virConnectPtr conn ATTRIBUTE_UNUSED,
> + virDomainPtr dom ATTRIBUTE_UNUSED,
> + const char *disk ATTRIBUTE_UNUSED,
> + int type ATTRIBUTE_UNUSED,
> + int status,
> + void *opaque)
> +{
> +*(int *) opaque = status;
> +}
> +
>  /*
>   * "blockcommit" command
>   */
> @@ -1808,6 +1819,8 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
>  const char *path = NULL;
>  bool quit = false;
>  int abort_flags = 0;
> +int status = -1;
> +int cb_id = -1;
>  
>  blocking |= vshCommandOptBool(cmd, "timeout") || pivot || finish;
>  if (blocking) {
> @@ -1837,6 +1850,17 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
>  return false;
>  }
>  
> +virConnectDomainEventGenericCallback cb =
> +VIR_DOMAIN_EVENT_CALLBACK(vshBlockJobStatusHandler);
> +
> +if ((cb_id = virConnectDomainEventRegisterAny(ctl->conn,
> + dom,
> + VIR_DOMAIN_EVENT_ID_BLOCK_JOB,
> + cb,
> + &status,
> + NULL)) < 0)

The lines above are misaligned after you added the condition.

> +vshResetLibvirtError();
> +
>  if (!blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_COMMIT, &dom))
>  goto cleanup;
>  

I've fixed all three instances and pushed the patch.

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 v2 0/5] Guest filesystem information API

[Michal Privoznik]

On 19.11.2014 23:58, John Ferlan wrote:



On 11/17/2014 06:26 PM, Tomoki Sekiyama wrote:

Hi,

This is v2 of patchset to add virDomainGetFSInfo API.

* changes in v1->v2:
  -[all] removed redundant NULL element at the last of returned info array
  -[3/5] make error messages in qemu_agent.c consistent with other commands
  -[4/5] added a test case for 2 items in info->devAliases
  -[5/5] added a pod document for virsh domfsinfo command
  (v1: http://www.redhat.com/archives/libvir-list/2014-October/msg1.html )

* summary
   This series implements a new virDomainGetFSInfo API, that returns a list of
   mounted filesystems information in the guest, collected via the guest agent.

   The returned info contains mountpoints and disk device alias named in
   libvirt, so we can know which mountpoints should be frozen by
   virDomainFSFreeze to take snapshots of a part of disks.

---
Tomoki Sekiyama (5):
   Implement public API for virDomainGetFSInfo
   remote: Implement the remote protocol for virDomainGetFSInfo
   qemu: Implement the qemu driver for virDomainGetFSInfo
   qemu: add test for qemuAgentGetFSInfo
   virsh: expose virDomainGetFSInfo


  daemon/remote.c  |  117 
  include/libvirt/libvirt-domain.h |   21 
  src/conf/domain_conf.c   |   71 
  src/conf/domain_conf.h   |6 +
  src/driver-hypervisor.h  |6 +
  src/libvirt.c|   66 +++
  src/libvirt_private.syms |1
  src/libvirt_public.syms  |6 +
  src/qemu/qemu_agent.c|  178 ++
  src/qemu/qemu_agent.h|2
  src/qemu/qemu_driver.c   |   48 
  src/remote/remote_driver.c   |   92 
  src/remote/remote_protocol.x |   32 +
  src/remote_protocol-structs  |   21 
  src/rpc/gendispatch.pl   |1
  tests/Makefile.am|1
  tests/qemuagentdata/qemuagent-fsinfo.xml |   39 +++
  tests/qemuagenttest.c|  143 
  tools/virsh-domain.c |   74 
  tools/virsh.pod  |9 ++
  20 files changed, 933 insertions(+), 1 deletion(-)
  create mode 100644 tests/qemuagentdata/qemuagent-fsinfo.xml

--

Tomoki Sekiyama




I reviewed the 'libvirt' specific changes - had a few comments and have
made changes to my review branch as specified.  As long as you're OK
with those changes I will get these pushed.

I'm also hoping someone else (eblake?) can look at the remote_protocol.x
changes to ensure they encompass everything they are supposed to.  Also
that the usage of QEMU_JOB_QUERY not _MODIFY for the GetFSInfo seems
more appropriate and is in line with the various remote_protocol.x
settings (@acl/@generate stuff settings).



@generate is correct, since both, client and server implementations are 
provided.
@acl looks consistent to the rest. Correct, for querying domain info you 
need to have read permission and that's it.


And yes, the job should be _QUERY since we are querying info, not 
modifying domain in any way.


However, for some reason I see this build error after 2/5:

make[3]: Entering directory 
'/home/zippy/work/libvirt/libvirt.git/tools/wireshark/src'

  CC   libvirt_la-packet-libvirt.lo
In file included from libvirt/protocol.h:5:0,
 from packet-libvirt.h:112,
 from packet-libvirt.c:36:
./libvirt/remote.h: In function 'dissect_xdr_remote_typed_param_value':
./libvirt/remote.h:470:5: error: unknown type name 'remote_nonnull_string'
 remote_nonnull_string type = 0;
 ^
./libvirt/remote.h:473:5: warning: implicit declaration of function 
'xdr_remote_nonnull_string' [-Wimplicit-function-declaration]

 if (!xdr_remote_nonnull_string(xdrs, &type))
 ^
Makefile:1934: recipe for target 'libvirt_la-packet-libvirt.lo' failed
make[3]: *** [libvirt_la-packet-libvirt.lo] Error 1


Michal


Michal

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

[libvirt] [PATCH] qemu: Fix crash in tunnelled migration

[Jiri Denemark]

Any attempt to start a tunnelled migration with libvirtd that supports
RDMA migration (specifically commit v1.2.8-226-ged22a47) crashes
libvirtd on the destination host.

The crash is inevitable because qemuMigrationPrepareAny is always called
with NULL protocol in case of tunnelled migration.

https://bugzilla.redhat.com/show_bug.cgi?id=1147331
Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_migration.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 100600e..89313df 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2707,7 +2707,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
QEMU_MIGRATION_COOKIE_NBD)))
 goto cleanup;
 
-if (STREQ(protocol, "rdma") && !vm->def->mem.hard_limit) {
+if (STREQ_NULLABLE(protocol, "rdma") && !vm->def->mem.hard_limit) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("cannot start RDMA migration with no memory hard "
  "limit set"));
-- 
2.1.3

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

Re: [libvirt] [PATCH] qemu: Fix crash in tunnelled migration

[Peter Krempa]

On 11/20/14 13:27, Jiri Denemark wrote:
> Any attempt to start a tunnelled migration with libvirtd that supports
> RDMA migration (specifically commit v1.2.8-226-ged22a47) crashes
> libvirtd on the destination host.
> 
> The crash is inevitable because qemuMigrationPrepareAny is always called
> with NULL protocol in case of tunnelled migration.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1147331
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_migration.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 100600e..89313df 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -2707,7 +2707,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
> QEMU_MIGRATION_COOKIE_NBD)))
>  goto cleanup;
>  
> -if (STREQ(protocol, "rdma") && !vm->def->mem.hard_limit) {
> +if (STREQ_NULLABLE(protocol, "rdma") && !vm->def->mem.hard_limit) {
>  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> _("cannot start RDMA migration with no memory hard "
>   "limit set"));
> 

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] [libvirt-python PATCH] override: Implement bindings for virDomainGetFSInfo as domain.fsInfo

[Pavel Hrdina]

On 11/20/2014 01:22 PM, Pavel Hrdina wrote:

On 11/18/2014 12:29 AM, Tomoki Sekiyama wrote:

Implement the function which returns a list of tuples, that contains
members
of virDomainFSInfo struct.

Signed-off-by: Tomoki Sekiyama 
---
  generator.py |5 +++
  libvirt-override-api.xml |6 
  libvirt-override.c   |   70
++
  sanitytest.py|5 +++
  4 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/generator.py b/generator.py
index c66c6d4..39ba2f7 100755
--- a/generator.py
+++ b/generator.py
@@ -476,6 +476,7 @@ skip_impl = (
  'virNetworkGetDHCPLeases',
  'virDomainBlockCopy',
  'virNodeAllocPages',
+'virDomainGetFSInfo',
  )

  lxc_skip_impl = (
@@ -586,6 +587,7 @@ skip_function = (

  'virNetworkDHCPLeaseFree', # only useful in C, python code uses
list
  'virDomainStatsRecordListFree', # only useful in C, python uses
dict
+'virDomainFSInfoFree', # only useful in C, python code uses list
  )

  lxc_skip_function = (
@@ -1107,6 +1109,9 @@ def nameFixup(name, classe, type, file):
  elif name[0:20] == "virDomainGetCPUStats":
  func = name[9:]
  func = func[0:1].lower() + func[1:]
+elif name[0:20] == "virDomainGetFSInfo":


There definitely must be name[0:18] as John pointed out.


+func = name[12:]
+func = func[0:2].lower() + func[2:]
  elif name[0:12] == "virDomainGet":
  func = name[12:]
  func = func[0:1].lower() + func[1:]
diff --git a/libvirt-override-api.xml b/libvirt-override-api.xml
index 4fe3c4d..2e807ba 100644
--- a/libvirt-override-api.xml
+++ b/libvirt-override-api.xml
@@ -658,5 +658,11 @@


  
+
+  Get a list of mapping informaiton for each mounted file
systems within the specified guest and the disks.
+  
+  
+  
+

  
diff --git a/libvirt-override.c b/libvirt-override.c
index 8895289..bd6321f 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -8266,6 +8266,73 @@ libvirt_virNodeAllocPages(PyObject *self
ATTRIBUTE_UNUSED,
  }
  #endif /* LIBVIR_CHECK_VERSION(1, 2, 8) */

+#if LIBVIR_CHECK_VERSION(1, 2, 11)
+
+static PyObject *
+libvirt_virDomainGetFSInfo(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args) {
+virDomainPtr domain;
+PyObject *pyobj_domain;
+unsigned int flags;
+virDomainFSInfoPtr *fsinfo = NULL;
+char **dev;
+int c_retval, i;
+PyObject *py_retval;
+
+if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainFSInfo",
+&pyobj_domain, &flags))
+return NULL;
+domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
+
+LIBVIRT_BEGIN_ALLOW_THREADS;
+c_retval = virDomainGetFSInfo(domain, &fsinfo, flags);
+LIBVIRT_END_ALLOW_THREADS;
+
+if (c_retval < 0)
+goto cleanup;
+
+/* convert to a Python list */
+if ((py_retval = PyList_New(c_retval)) == NULL)
+goto cleanup;


The PyList_New on success return new reference ... [1]


+
+for (i = 0; i < c_retval; i++) {
+virDomainFSInfoPtr fs = fsinfo[i];
+PyObject *info, *alias;
+
+if (fs == NULL)
+goto cleanup;
+info = PyTuple_New(4);
+if (info == NULL)


Wrong indentation, use spaces instead of tabs.


+goto cleanup;
+PyList_SetItem(py_retval, i, info);
+alias = PyList_New(0);
+if (alias == NULL)


The same wrong indentation.


+goto cleanup;
+
+PyTuple_SetItem(info, 0,
libvirt_constcharPtrWrap(fs->mountpoint));
+PyTuple_SetItem(info, 1, libvirt_constcharPtrWrap(fs->name));
+PyTuple_SetItem(info, 2, libvirt_constcharPtrWrap(fs->type));
+PyTuple_SetItem(info, 3, alias);
+
+for (dev = fs->devAlias; dev && *dev; dev++)
+if (PyList_Append(alias, libvirt_constcharPtrWrap(*dev))
< 0)
+goto cleanup;
+}
+
+for (i = 0; i < c_retval; i++)
+virDomainFSInfoFree(fsinfo[i]);
+VIR_FREE(fsinfo);
+return py_retval;
+
+ cleanup:
+for (i = 0; i < c_retval; i++)
+virDomainFSInfoFree(fsinfo[i]);
+VIR_FREE(fsinfo);
+Py_DECREF(py_retval);


[1] ... there you correctly dereference it, but you should use
PY_XDECREF because the py_retval could be NULL.


+return VIR_PY_NONE;
+}
+
+#endif /* LIBVIR_CHECK_VERSION(1, 2, 11) */
+

/
   **
   *The registration stuff*
@@ -8459,6 +8526,9 @@ static PyMethodDef libvirtMethods[] = {
  #if LIBVIR_CHECK_VERSION(1, 2, 9)
  {(char *) "virNodeAllocPages", libvirt_virNodeAllocPages,
METH_VARARGS, NULL},
  #endif /* LIBVIR_CHECK_VERSION(1, 2, 9) */
+#if LIBVIR_CHECK_VERSION(1, 2, 11)
+{(char *) "virDomainGetFSInfo", libvirt_virDomainGetFSInfo,
METH_VARARGS, NULL},
+#endif /* LIBVIR_CHECK_VERSION(1, 2, 11) */
  {NULL, NULL, 0, NULL}
  };

diff --git a/sanitytest.py b/sanitytest

Re: [libvirt] [libvirt-python PATCH] override: Implement bindings for virDomainGetFSInfo as domain.fsInfo

[Pavel Hrdina]

On 11/18/2014 12:29 AM, Tomoki Sekiyama wrote:

Implement the function which returns a list of tuples, that contains members
of virDomainFSInfo struct.

Signed-off-by: Tomoki Sekiyama 
---
  generator.py |5 +++
  libvirt-override-api.xml |6 
  libvirt-override.c   |   70 ++
  sanitytest.py|5 +++
  4 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/generator.py b/generator.py
index c66c6d4..39ba2f7 100755
--- a/generator.py
+++ b/generator.py
@@ -476,6 +476,7 @@ skip_impl = (
  'virNetworkGetDHCPLeases',
  'virDomainBlockCopy',
  'virNodeAllocPages',
+'virDomainGetFSInfo',
  )

  lxc_skip_impl = (
@@ -586,6 +587,7 @@ skip_function = (

  'virNetworkDHCPLeaseFree', # only useful in C, python code uses list
  'virDomainStatsRecordListFree', # only useful in C, python uses dict
+'virDomainFSInfoFree', # only useful in C, python code uses list
  )

  lxc_skip_function = (
@@ -1107,6 +1109,9 @@ def nameFixup(name, classe, type, file):
  elif name[0:20] == "virDomainGetCPUStats":
  func = name[9:]
  func = func[0:1].lower() + func[1:]
+elif name[0:20] == "virDomainGetFSInfo":


There definitely must be name[0:18] as John pointed out.


+func = name[12:]
+func = func[0:2].lower() + func[2:]
  elif name[0:12] == "virDomainGet":
  func = name[12:]
  func = func[0:1].lower() + func[1:]
diff --git a/libvirt-override-api.xml b/libvirt-override-api.xml
index 4fe3c4d..2e807ba 100644
--- a/libvirt-override-api.xml
+++ b/libvirt-override-api.xml
@@ -658,5 +658,11 @@


  
+
+  Get a list of mapping informaiton for each mounted file systems within 
the specified guest and the disks.
+  
+  
+  
+

  
diff --git a/libvirt-override.c b/libvirt-override.c
index 8895289..bd6321f 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -8266,6 +8266,73 @@ libvirt_virNodeAllocPages(PyObject *self 
ATTRIBUTE_UNUSED,
  }
  #endif /* LIBVIR_CHECK_VERSION(1, 2, 8) */

+#if LIBVIR_CHECK_VERSION(1, 2, 11)
+
+static PyObject *
+libvirt_virDomainGetFSInfo(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) {
+virDomainPtr domain;
+PyObject *pyobj_domain;
+unsigned int flags;
+virDomainFSInfoPtr *fsinfo = NULL;
+char **dev;
+int c_retval, i;
+PyObject *py_retval;
+
+if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainFSInfo",
+&pyobj_domain, &flags))
+return NULL;
+domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
+
+LIBVIRT_BEGIN_ALLOW_THREADS;
+c_retval = virDomainGetFSInfo(domain, &fsinfo, flags);
+LIBVIRT_END_ALLOW_THREADS;
+
+if (c_retval < 0)
+goto cleanup;
+
+/* convert to a Python list */
+if ((py_retval = PyList_New(c_retval)) == NULL)
+goto cleanup;


The PyList_New on success return new reference ... [1]


+
+for (i = 0; i < c_retval; i++) {
+virDomainFSInfoPtr fs = fsinfo[i];
+PyObject *info, *alias;
+
+if (fs == NULL)
+goto cleanup;
+info = PyTuple_New(4);
+   if (info == NULL)


Wrong indentation, use spaces instead of tabs.


+goto cleanup;
+PyList_SetItem(py_retval, i, info);
+alias = PyList_New(0);
+   if (alias == NULL)


The same wrong indentation.


+goto cleanup;
+
+PyTuple_SetItem(info, 0, libvirt_constcharPtrWrap(fs->mountpoint));
+PyTuple_SetItem(info, 1, libvirt_constcharPtrWrap(fs->name));
+PyTuple_SetItem(info, 2, libvirt_constcharPtrWrap(fs->type));
+PyTuple_SetItem(info, 3, alias);
+
+for (dev = fs->devAlias; dev && *dev; dev++)
+if (PyList_Append(alias, libvirt_constcharPtrWrap(*dev)) < 0)
+goto cleanup;
+}
+
+for (i = 0; i < c_retval; i++)
+virDomainFSInfoFree(fsinfo[i]);
+VIR_FREE(fsinfo);
+return py_retval;
+
+ cleanup:
+for (i = 0; i < c_retval; i++)
+virDomainFSInfoFree(fsinfo[i]);
+VIR_FREE(fsinfo);
+Py_DECREF(py_retval);


[1] ... there you correctly dereference it, but you should use 
PY_XDECREF because the py_retval could be NULL.



+return VIR_PY_NONE;
+}
+
+#endif /* LIBVIR_CHECK_VERSION(1, 2, 11) */
+
  /
   **
   *The registration stuff  *
@@ -8459,6 +8526,9 @@ static PyMethodDef libvirtMethods[] = {
  #if LIBVIR_CHECK_VERSION(1, 2, 9)
  {(char *) "virNodeAllocPages", libvirt_virNodeAllocPages, METH_VARARGS, 
NULL},
  #endif /* LIBVIR_CHECK_VERSION(1, 2, 9) */
+#if LIBVIR_CHECK_VERSION(1, 2, 11)
+{(char *) "virDomainGetFSInfo", libvirt_virDomainGetFSInfo, METH_VARARGS, 
NULL},
+#endif /* LIBVIR_CHECK_VERSION(1, 2, 11) */
  {NULL, NULL, 0, NULL}
  };

diff --git a/sanityte

[libvirt] [PATCHv3] virsh: Fix msg: blockjob is aborted from another client

[Erik Skultety]

When a block{pull, copy, commit} is aborted via keyboard interrupt,
the job is properly canceled followed by proper error message.
However, when the job receives an abort from another client connected
to the same domain, the error message incorrectly indicates that
a blockjob has been finished successfully, though the abort request
took effect. This patch introduces a new blockjob abort handler, which
is registered when the client calls block{copy,commit,pull} routine,
providing its caller the status of the finished blockjob.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1135442
---
 tools/virsh-domain.c | 71 +---
 1 file changed, 68 insertions(+), 3 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index a7e9151..0891821 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1709,6 +1709,17 @@ static void vshCatchInt(int sig ATTRIBUTE_UNUSED,
 intCaught = 1;
 }
 
+static void
+vshBlockJobStatusHandler(virConnectPtr conn ATTRIBUTE_UNUSED,
+ virDomainPtr dom ATTRIBUTE_UNUSED,
+ const char *disk ATTRIBUTE_UNUSED,
+ int type ATTRIBUTE_UNUSED,
+ int status,
+ void *opaque)
+{
+*(int *) opaque = status;
+}
+
 /*
  * "blockcommit" command
  */
@@ -1808,6 +1819,8 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
 const char *path = NULL;
 bool quit = false;
 int abort_flags = 0;
+int status = -1;
+int cb_id = -1;
 
 blocking |= vshCommandOptBool(cmd, "timeout") || pivot || finish;
 if (blocking) {
@@ -1837,6 +1850,17 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
 return false;
 }
 
+virConnectDomainEventGenericCallback cb =
+VIR_DOMAIN_EVENT_CALLBACK(vshBlockJobStatusHandler);
+
+if ((cb_id = virConnectDomainEventRegisterAny(ctl->conn,
+ dom,
+ VIR_DOMAIN_EVENT_ID_BLOCK_JOB,
+ cb,
+ &status,
+ NULL)) < 0)
+vshResetLibvirtError();
+
 if (!blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_COMMIT, &dom))
 goto cleanup;
 
@@ -1878,7 +1902,7 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
  intCaught ? "interrupted" : "timeout");
 intCaught = 0;
 timeout = 0;
-quit = true;
+status = VIR_DOMAIN_BLOCK_JOB_CANCELED;
 if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) {
 vshError(ctl, _("failed to abort job for disk %s"), path);
 goto cleanup;
@@ -1890,6 +1914,9 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
 }
 }
 
+if (status == VIR_DOMAIN_BLOCK_JOB_CANCELED)
+quit = true;
+
 if (verbose && !quit) {
 /* printf [100 %] */
 vshPrintJobProgress(_("Block Commit"), 0, 1);
@@ -1920,6 +1947,8 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
 virDomainFree(dom);
 if (blocking)
 sigaction(SIGINT, &old_sig_action, NULL);
+if (cb_id >= 0)
+virConnectDomainEventDeregisterAny(ctl->conn, cb_id);
 return ret;
 }
 
@@ -2043,6 +2072,8 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
 char *xmlstr = NULL;
 virTypedParameterPtr params = NULL;
 int nparams = 0;
+int status = -1;
+int cb_id = -1;
 
 if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0)
 return false;
@@ -2083,6 +2114,17 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
 return false;
 }
 
+virConnectDomainEventGenericCallback cb =
+VIR_DOMAIN_EVENT_CALLBACK(vshBlockJobStatusHandler);
+
+if ((cb_id = virConnectDomainEventRegisterAny(ctl->conn,
+ dom,
+ VIR_DOMAIN_EVENT_ID_BLOCK_JOB,
+ cb,
+ &status,
+ NULL)) < 0)
+vshResetLibvirtError();
+
 if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
 goto cleanup;
 
@@ -2216,7 +2258,7 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
  intCaught ? "interrupted" : "timeout");
 intCaught = 0;
 timeout = 0;
-quit = true;
+status = VIR_DOMAIN_BLOCK_JOB_CANCELED;
 if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) {
 vshError(ctl, _("failed to abort job for disk %s"), path);
 goto cleanup;
@@ -2228,6 +2270,9 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
 }
 }
 
+if (status == VIR_DOMAIN_BLOCK_JOB_CANCELED)
+quit = true;
+
 if (!quit && pivot) {
 abort_flags |= VIR

Re: [libvirt] [PATCH 0/8] Add XML validation to the APIs

[Daniel P. Berrange]

On Thu, Nov 20, 2014 at 12:02:12PM +0100, Martin Kletzander wrote:
> On Wed, Nov 19, 2014 at 12:51:22PM +, Daniel P. Berrange wrote:
> >On Wed, Nov 19, 2014 at 09:45:39AM +, Daniel P. Berrange wrote:
> >>On Wed, Nov 19, 2014 at 08:23:22AM +0100, Martin Kletzander wrote:
> >>> On Tue, Nov 18, 2014 at 05:59:47PM +, Daniel P. Berrange wrote:
> >>> >This proof of concept patch extends the virDomainDefineXML
> >>> >and virDomainCreateXML APIs so that they can validate
> >>> >the user supplied XML document against the RNG schemas.
> >>> >
> >>> >The virsh command will enable validation by default, it
> >>> >must be turned off with --skip-validation if desired.
> >>> >
> >>> >This series is not complete
> >>> >
> >>> >- The network, interface, storage pool, etc APIs are
> >>> >  not wired up to support validation.
> >>> >- Only the QEMU virt driver is wired up to validate
> >>> >- The virsh edit command is not wired up to validate
> >>> >
> >>> >It is enough to demonstrate it working with 'virsh define'
> >>> >and the QEMU driver though.
> >>> >
> >>> >The biggest problem I see is the really awful error
> >>> >messages we get back from libxml2 when validation
> >>> >fails :-( They are essentially useless :-(
> >
> >>> >
> >>>
> >>> This is one of the things why I'm not convinced this work is worth
> >>> it.  It may be nice if we tell the user their XML is invalid instead
> >>> of silently losing information.  But error message similar to "invalid
> >>> element in interleave" doesn't help much when you are adding 100-line
> >>> XML.  There are some better validators, but requiring those would be
> >>> too cumbersome.
> >>
> >>At least when using 'virsh edit' you would know what element you
> >>just changed / added. So if you got get a generic 'validation failed'
> >>error you have a pretty good idea of where in teh document you made
> >>the mistake. So I think it'd be useful in that scenario. The error
> >>reporting is more of a problem for the apps where they're passing in
> >>a big XML document to define the guest and basically anything could
> >>be wrong.
> >
> >So, it seems not all of the error messages are so awful. It does a bad
> >job of reporting unknown elements, but if you have an unknown attribute
> >it does better:
> >
> > "Invalid attribute foo for element name"
> >
> >If you give an invalid value for an attribute which is an enum it
> >is semi-usefull
> >
> > "Element domain failed to validate attributes"
> >
> >So this does seem somewhat more useful to have in libvirt
> >
> 
> As I said, I'm not against this, I agree that it will still be useful.
> 
> If you meant this as an RFC, then I misunderstood that and I should've
> just wrote that as an initial PoC it's fine with me :)
> 
> Do you want me to finish the review?

No, I should probably finish doing the rest of the work first if people
agree using this API flag is the right way forward.


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 0/8] Add XML validation to the APIs

[Martin Kletzander]

On Wed, Nov 19, 2014 at 12:51:22PM +, Daniel P. Berrange wrote:

On Wed, Nov 19, 2014 at 09:45:39AM +, Daniel P. Berrange wrote:

On Wed, Nov 19, 2014 at 08:23:22AM +0100, Martin Kletzander wrote:
> On Tue, Nov 18, 2014 at 05:59:47PM +, Daniel P. Berrange wrote:
> >This proof of concept patch extends the virDomainDefineXML
> >and virDomainCreateXML APIs so that they can validate
> >the user supplied XML document against the RNG schemas.
> >
> >The virsh command will enable validation by default, it
> >must be turned off with --skip-validation if desired.
> >
> >This series is not complete
> >
> >- The network, interface, storage pool, etc APIs are
> >  not wired up to support validation.
> >- Only the QEMU virt driver is wired up to validate
> >- The virsh edit command is not wired up to validate
> >
> >It is enough to demonstrate it working with 'virsh define'
> >and the QEMU driver though.
> >
> >The biggest problem I see is the really awful error
> >messages we get back from libxml2 when validation
> >fails :-( They are essentially useless :-(



> >
>
> This is one of the things why I'm not convinced this work is worth
> it.  It may be nice if we tell the user their XML is invalid instead
> of silently losing information.  But error message similar to "invalid
> element in interleave" doesn't help much when you are adding 100-line
> XML.  There are some better validators, but requiring those would be
> too cumbersome.

At least when using 'virsh edit' you would know what element you
just changed / added. So if you got get a generic 'validation failed'
error you have a pretty good idea of where in teh document you made
the mistake. So I think it'd be useful in that scenario. The error
reporting is more of a problem for the apps where they're passing in
a big XML document to define the guest and basically anything could
be wrong.


So, it seems not all of the error messages are so awful. It does a bad
job of reporting unknown elements, but if you have an unknown attribute
it does better:

 "Invalid attribute foo for element name"

If you give an invalid value for an attribute which is an enum it
is semi-usefull

 "Element domain failed to validate attributes"

So this does seem somewhat more useful to have in libvirt



As I said, I'm not against this, I agree that it will still be useful.

If you meant this as an RFC, then I misunderstood that and I should've
just wrote that as an initial PoC it's fine with me :)

Do you want me to finish the review?

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 v4] leaseshelper: improvements to support all events

[Peter Krempa]

On 11/20/14 11:24, Daniel P. Berrange wrote:
> On Thu, Nov 20, 2014 at 10:56:53AM +0100, Peter Krempa wrote:
>> On 11/18/14 18:16, Nehal J Wani wrote:
>>> This patch enables the helper program to detect event(s) triggered when 
>>> there
>>> is a change in lease length or expiry and client-id. This transfers complete
>>> control of leases database to libvirt and obsoletes use of the lease 
>>> database
>>> file (.leases). That file will not be created, read, or 
>>> written.
>>> This is achieved by adding the option --leasefile-ro to dnsmasq and passing 
>>> a
>>> custom env var to leaseshelper, which helps us map events related to leases
>>> with their corresponding network bridges, no matter what the event be.
>>>
>>> Also, this requires the addition of a new non-lease entry in our custom 
>>> lease
>>> database: "server-duid". It is required to identify a DHCPv6 server.
>>>
>>> Now that dnsmasq doesn't maintain its own leases database, it relies on our
>>> helper program to tell it about previous leases and server duid. Thus, this
>>> patch makes our leases program honor an extra action: "init", in which it 
>>> sends
>>> the known info in a particular format to dnsmasq by printing it to stdout.
>>>
>>> ---
>>>
>>>  This is compatible with libvirt 1.2.6 as only additions have been
>>>  introduced, and the old leases file (*.status) will still be supported.
>>>
>>>  v4: * Removed extra data structures for segregating ipv4 and ipv6 leases.
>>>
>>>  v3: * Add server-duid as an entry in the lease object for every ipv6 lease.
>>>  * Remove unnecessary variables and double copies.
>>>  * Take value from DNSMASQ_OLD_HOSTNAME if hostname is not known.
>>>  http://www.redhat.com/archives/libvir-list/2014-August/msg00028.html
>>>
>>>  v2: http://www.redhat.com/archives/libvir-list/2014-July/msg01109.html
>>>
>>>  v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html
>>>
>>>
>>> ---
>>>  src/network/bridge_driver.c |   3 +
>>>  src/network/leaseshelper.c  | 142 
>>> +++-
>>>  2 files changed, 117 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>>> index 6cb421c..6ecbc37 100644
>>> --- a/src/network/bridge_driver.c
>>> +++ b/src/network/bridge_driver.c
>>> @@ -1280,7 +1280,10 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr 
>>> network,
>>>  
>>>  cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps));
>>>  virCommandAddArgFormat(cmd, "--conf-file=%s", configfile);
>>> +/* Libvirt gains full control of leases database */
>>> +virCommandAddArgFormat(cmd, "--leasefile-ro");
>>
>> One other thing that is worrying is that if you specify this option and
>> the leaseshelper implements the init operation we'd no longer load a
>> possibly pre-existing leases file that was present previous to the
>> upgrade to this handling.
>>
>> This would cause the guests to be re-addressed from the moment of the
>> upgrade. (exactly once).
>>
>> The question here is. Do we care about this slight breakage? I'm not
>> entirely decided yet. I probably wouldn't care.
>>
>> A possible workaround will be to check whether the old leases file
>> exists and refrain from starting with the --leasefile-ro option in that
>> case.
> 
> When libvirtd is upgraded, we don't restart dnsmasq so it will continue
> with its current config. Only once you shutdown the virtual network and
> restart it (eg most likely upon next reboot) would the config change
> loosing the leases.  That probably isn't the end of the world because
> with DHCP there is never a strong guarantee of getting the same IP
> address unless fixed mappings are provided.

Yep, and addresses that have to remain static can be configured explicitly.

> 
> If there's an easy way to notice the old file and convert it to the new
> file format we should do it, but if it is hard, then its not the end of
> the world.

AFAIK the leaseshelper was introduced so that we don't ever have to
parse the dnsmasq lease file so having to parse it ourselves so parsing
it wouldn't be viable. I was thinking of not using the new initial lease
reloading in case an existing lease file is present if we would want to
keep the mapping, but even that increases the complexity.

Thus I'm glad we don't have to strictly keep the leases file and users
will deal with this slight issue just once at upgrade.

> 
> Regards,
> Daniel
> 

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 v4] leaseshelper: improvements to support all events

[Daniel P. Berrange]

On Thu, Nov 20, 2014 at 10:56:53AM +0100, Peter Krempa wrote:
> On 11/18/14 18:16, Nehal J Wani wrote:
> > This patch enables the helper program to detect event(s) triggered when 
> > there
> > is a change in lease length or expiry and client-id. This transfers complete
> > control of leases database to libvirt and obsoletes use of the lease 
> > database
> > file (.leases). That file will not be created, read, or 
> > written.
> > This is achieved by adding the option --leasefile-ro to dnsmasq and passing 
> > a
> > custom env var to leaseshelper, which helps us map events related to leases
> > with their corresponding network bridges, no matter what the event be.
> > 
> > Also, this requires the addition of a new non-lease entry in our custom 
> > lease
> > database: "server-duid". It is required to identify a DHCPv6 server.
> > 
> > Now that dnsmasq doesn't maintain its own leases database, it relies on our
> > helper program to tell it about previous leases and server duid. Thus, this
> > patch makes our leases program honor an extra action: "init", in which it 
> > sends
> > the known info in a particular format to dnsmasq by printing it to stdout.
> > 
> > ---
> > 
> >  This is compatible with libvirt 1.2.6 as only additions have been
> >  introduced, and the old leases file (*.status) will still be supported.
> > 
> >  v4: * Removed extra data structures for segregating ipv4 and ipv6 leases.
> > 
> >  v3: * Add server-duid as an entry in the lease object for every ipv6 lease.
> >  * Remove unnecessary variables and double copies.
> >  * Take value from DNSMASQ_OLD_HOSTNAME if hostname is not known.
> >  http://www.redhat.com/archives/libvir-list/2014-August/msg00028.html
> > 
> >  v2: http://www.redhat.com/archives/libvir-list/2014-July/msg01109.html
> > 
> >  v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html
> > 
> > 
> > ---
> >  src/network/bridge_driver.c |   3 +
> >  src/network/leaseshelper.c  | 142 
> > +++-
> >  2 files changed, 117 insertions(+), 28 deletions(-)
> > 
> > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> > index 6cb421c..6ecbc37 100644
> > --- a/src/network/bridge_driver.c
> > +++ b/src/network/bridge_driver.c
> > @@ -1280,7 +1280,10 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr 
> > network,
> >  
> >  cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps));
> >  virCommandAddArgFormat(cmd, "--conf-file=%s", configfile);
> > +/* Libvirt gains full control of leases database */
> > +virCommandAddArgFormat(cmd, "--leasefile-ro");
> 
> One other thing that is worrying is that if you specify this option and
> the leaseshelper implements the init operation we'd no longer load a
> possibly pre-existing leases file that was present previous to the
> upgrade to this handling.
> 
> This would cause the guests to be re-addressed from the moment of the
> upgrade. (exactly once).
> 
> The question here is. Do we care about this slight breakage? I'm not
> entirely decided yet. I probably wouldn't care.
> 
> A possible workaround will be to check whether the old leases file
> exists and refrain from starting with the --leasefile-ro option in that
> case.

When libvirtd is upgraded, we don't restart dnsmasq so it will continue
with its current config. Only once you shutdown the virtual network and
restart it (eg most likely upon next reboot) would the config change
loosing the leases.  That probably isn't the end of the world because
with DHCP there is never a strong guarantee of getting the same IP
address unless fixed mappings are provided.

If there's an easy way to notice the old file and convert it to the new
file format we should do it, but if it is hard, then its not the end of
the world.

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 1/2] virdbus: don't force users to pass int for bool values

[Daniel P. Berrange]

On Wed, Nov 19, 2014 at 02:42:51PM -0700, Eric Blake wrote:
> On 11/17/2014 05:36 PM, Eric Blake wrote:
> > Use of an 'int' to represent a 'bool' value is confusing.  Just
> > because dbus made the mistake of cementing their 4-byte wire
> > format of dbus_bool_t into their API doesn't mean we have to
> > repeat the mistake.  With a little bit of finesse, we can
> > guarantee that we provide a large-enough value to the DBus
> > code, while still copying only the relevant one-byte bool
> > to the client code, and isolate the rest of our code base from
> > the DBus stupidity.
> > 
> > [Have I ever mentioned that the assymetry between type promotions
> > of values passed through var-args on setting, vs. no promotions
> > when passing pointers through var-args for getting, makes life
> > awkward, not just for DBus interactions, but also for printf vs.
> > scanf?  Then again, using scanf is often the wrong thing to do...]
> > 
> > * src/util/virdbus.c (GET_NEXT_VAL): Add parameter.
> > (virDBusMessageIterDecode): Adjust all clients.
> > * src/util/virpolkit.c (virPolkitCheckAuth): Use nicer type.
> > * tests/virdbustest.c (testMessageSimple, testMessageStruct):
> > Test new behavior.
> > 
> 
> > -# define GET_NEXT_VAL(dbustype, vargtype, fmt)  \
> > +# define GET_NEXT_VAL(dbustype, member, vargtype, fmt)  \
> >  do {\
> >  dbustype *x;\
> > +DBusBasicValue v;   \
> 
> Blech. DBusBasicValue wasn't defined in the older dbus-types.h from
> 1.1.2 as shipped on RHEL 5, causing a compilation failure there.  I'm
> working on a fix...

Its just a dumb union, so we can either provide the definition ourselves,
or just define our of  virDBusValue union and ignore DBusBasicValue

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] [PATCHv2] virsh: Fix msg: blockjob is aborted from another client

[Peter Krempa]

On 11/19/14 14:54, Erik Skultety wrote:
> When a block{pull, copy, commit} is aborted via keyboard interrupt,
> the job is properly canceled followed by proper error message.
> However, when the job receives an abort from another client connected
> to the same domain, the error message incorrectly indicates that
> a blockjob has been finished successfully, though the abort request
> took effect. This patch introduces a new blockjob abort handler, which
> is registered when the client calls block{copy,commit,pull} routine,
> providing its caller the status of the finished blockjob.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1135442
> ---
>  tools/virsh-domain.c | 65 
> +---
>  1 file changed, 62 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index a7e9151..90960e9 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c

> @@ -1808,6 +1819,8 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
>  const char *path = NULL;
>  bool quit = false;
>  int abort_flags = 0;
> +int status = -1;
> +int cb_id = -1;
>  
>  blocking |= vshCommandOptBool(cmd, "timeout") || pivot || finish;
>  if (blocking) {
> @@ -1837,6 +1850,16 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
>  return false;
>  }
>  
> +virConnectDomainEventGenericCallback cb =
> +VIR_DOMAIN_EVENT_CALLBACK(vshBlockJobStatusHandler);
> +
> +cb_id = virConnectDomainEventRegisterAny(ctl->conn,
> + dom,
> + VIR_DOMAIN_EVENT_ID_BLOCK_JOB,
> + cb,
> + &status,
> + NULL);

You should call vshResetLibvirtError in case you want to ignore failure
of registration of the callback.

> +
>  if (!blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_COMMIT, &dom))
>  goto cleanup;
>  

> @@ -1920,6 +1946,7 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
>  virDomainFree(dom);
>  if (blocking)
>  sigaction(SIGINT, &old_sig_action, NULL);
> +virConnectDomainEventDeregisterAny(ctl->conn, cb_id);

And deregister it only if the callback id isn't -1 as it will again
overwrite the saved error.

>  return ret;
>  }
>  

Otherwise looks good.

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] cpu-driver: Fix the cross driver function call

[Daniel Hansel]

For Intel and PowerPC the implementation is calling a cpu driver
function across driver layers (i.e. from qemu driver directly to cpu
driver).
The correct behavior is to use libvirt API functionality to perform such
a inter-driver call.

This patch introduces a new cpu driver API function getModels() to
retrieve the cpu models. The currect implementation to process the
cpu_map XML content is transferred to the INTEL and PowerPC cpu driver
specific API functions.
Additionally processing the cpu_map XML file is not safe due to the fact
that the cpu map does not exist for all architectures. Therefore it is
better to encapsulate the processing in the architecture specific cpu
drivers.

Signed-off-by: Daniel Hansel 
Reviewed-by: Boris Fiuczynski 
Reviewed-by: Viktor Mihajlovski 
---
 src/cpu/cpu.c | 68 +--
 src/cpu/cpu.h |  4 +++
 src/cpu/cpu_powerpc.c | 37 
 src/cpu/cpu_x86.c | 33 +
 4 files changed, 86 insertions(+), 56 deletions(-)

diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
index 08bec5e..788f688 100644
--- a/src/cpu/cpu.c
+++ b/src/cpu/cpu.c
@@ -724,43 +724,6 @@ cpuModelIsAllowed(const char *model,
 return false;
 }
 
-struct cpuGetModelsData
-{
-char **data;
-size_t len;  /* It includes the last element of DATA, which is NULL. */
-};
-
-static int
-cpuGetArchModelsCb(cpuMapElement element,
-   xmlXPathContextPtr ctxt,
-   void *cbdata)
-{
-char *name;
-struct cpuGetModelsData *data = cbdata;
-if (element != CPU_MAP_ELEMENT_MODEL)
-return 0;
-
-name = virXPathString("string(@name)", ctxt);
-if (name == NULL)
-return -1;
-
-if (!data->data) {
-VIR_FREE(name);
-data->len++;
-return 0;
-}
-
-return VIR_INSERT_ELEMENT(data->data, data->len - 1, data->len, name);
-}
-
-
-static int
-cpuGetArchModels(const char *arch, struct cpuGetModelsData *data)
-{
-return cpuMapLoad(arch, cpuGetArchModelsCb, data);
-}
-
-
 /**
  * cpuGetModels:
  *
@@ -774,18 +737,17 @@ cpuGetArchModels(const char *arch, struct 
cpuGetModelsData *data)
 int
 cpuGetModels(const char *archName, char ***models)
 {
-struct cpuGetModelsData data;
-virArch arch;
 struct cpuArchDriver *driver;
-data.data = NULL;
-data.len = 1;
+virArch arch;
+
+VIR_DEBUG("arch=%s", archName);
 
 arch = virArchFromString(archName);
 if (arch == VIR_ARCH_NONE) {
 virReportError(VIR_ERR_INVALID_ARG,
_("cannot find architecture %s"),
archName);
-goto error;
+return -1;
 }
 
 driver = cpuGetSubDriver(arch);
@@ -793,21 +755,15 @@ cpuGetModels(const char *archName, char ***models)
 virReportError(VIR_ERR_INVALID_ARG,
_("cannot find a driver for the architecture %s"),
archName);
-goto error;
+return -1;
 }
 
-if (models && VIR_ALLOC_N(data.data, data.len) < 0)
-goto error;
-
-if (cpuGetArchModels(driver->name, &data) < 0)
-goto error;
-
-if (models)
-*models = data.data;
-
-return data.len - 1;
+if (!driver->getModels) {
+virReportError(VIR_ERR_NO_SUPPORT,
+   _("CPU driver for %s has no CPU model support"),
+   virArchToString(arch));
+return -1;
+}
 
- error:
-virStringFreeList(data.data);
-return -1;
+return driver->getModels(models);
 }
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
index 339964c..09e9538 100644
--- a/src/cpu/cpu.h
+++ b/src/cpu/cpu.h
@@ -100,6 +100,9 @@ typedef char *
 typedef virCPUDataPtr
 (*cpuArchDataParse) (const char *xmlStr);
 
+typedef int
+(*cpuArchGetModels) (char ***models);
+
 struct cpuArchDriver {
 const char *name;
 const virArch *arch;
@@ -115,6 +118,7 @@ struct cpuArchDriver {
 cpuArchHasFeaturehasFeature;
 cpuArchDataFormat   dataFormat;
 cpuArchDataParsedataParse;
+cpuArchGetModelsgetModels;
 };
 
 
diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c
index 67cb9ff..155d93e 100644
--- a/src/cpu/cpu_powerpc.c
+++ b/src/cpu/cpu_powerpc.c
@@ -649,6 +649,42 @@ ppcBaseline(virCPUDefPtr *cpus,
 goto cleanup;
 }
 
+static int
+ppcGetModels(char ***models)
+{
+struct ppc_map *map;
+struct ppc_model *model;
+char *name;
+size_t nmodels = 0;
+
+if (!(map = ppcLoadMap()))
+goto error;
+
+if (models && VIR_ALLOC_N(*models, 0) < 0)
+goto error;
+
+model = map->models;
+while (model != NULL) {
+if (VIR_STRDUP(name, model->name) < 0)
+goto error;
+
+if (VIR_INSERT_ELEMENT(*models, 0, nmodels, name) < 0)
+goto error;
+
+model = model->next;
+}
+
+ cleanup:
+ppcMapFree(map);
+
+return nmodels;
+
+ error:
+virStringFreeList(*models);
+nmodels = -1;
+goto

Re: [libvirt] [PATCH v4] leaseshelper: improvements to support all events

[Peter Krempa]

On 11/18/14 18:16, Nehal J Wani wrote:
> This patch enables the helper program to detect event(s) triggered when there
> is a change in lease length or expiry and client-id. This transfers complete
> control of leases database to libvirt and obsoletes use of the lease database
> file (.leases). That file will not be created, read, or written.
> This is achieved by adding the option --leasefile-ro to dnsmasq and passing a
> custom env var to leaseshelper, which helps us map events related to leases
> with their corresponding network bridges, no matter what the event be.
> 
> Also, this requires the addition of a new non-lease entry in our custom lease
> database: "server-duid". It is required to identify a DHCPv6 server.
> 
> Now that dnsmasq doesn't maintain its own leases database, it relies on our
> helper program to tell it about previous leases and server duid. Thus, this
> patch makes our leases program honor an extra action: "init", in which it 
> sends
> the known info in a particular format to dnsmasq by printing it to stdout.
> 
> ---
> 
>  This is compatible with libvirt 1.2.6 as only additions have been
>  introduced, and the old leases file (*.status) will still be supported.
> 
>  v4: * Removed extra data structures for segregating ipv4 and ipv6 leases.
> 
>  v3: * Add server-duid as an entry in the lease object for every ipv6 lease.
>  * Remove unnecessary variables and double copies.
>  * Take value from DNSMASQ_OLD_HOSTNAME if hostname is not known.
>  http://www.redhat.com/archives/libvir-list/2014-August/msg00028.html
> 
>  v2: http://www.redhat.com/archives/libvir-list/2014-July/msg01109.html
> 
>  v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html
> 
> 
> ---
>  src/network/bridge_driver.c |   3 +
>  src/network/leaseshelper.c  | 142 
> +++-
>  2 files changed, 117 insertions(+), 28 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 6cb421c..6ecbc37 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1280,7 +1280,10 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr 
> network,
>  
>  cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps));
>  virCommandAddArgFormat(cmd, "--conf-file=%s", configfile);
> +/* Libvirt gains full control of leases database */
> +virCommandAddArgFormat(cmd, "--leasefile-ro");

One other thing that is worrying is that if you specify this option and
the leaseshelper implements the init operation we'd no longer load a
possibly pre-existing leases file that was present previous to the
upgrade to this handling.

This would cause the guests to be re-addressed from the moment of the
upgrade. (exactly once).

The question here is. Do we care about this slight breakage? I'm not
entirely decided yet. I probably wouldn't care.

A possible workaround will be to check whether the old leases file
exists and refrain from starting with the --leasefile-ro option in that
case.


>  virCommandAddArgFormat(cmd, "--dhcp-script=%s", leaseshelper_path);
> +virCommandAddEnvPair(cmd, "VIR_BRIDGE_NAME", network->def->bridge);
>  
>  *cmdout = cmd;
>  ret = 0;

Thanks for opinions.

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 v4] leaseshelper: improvements to support all events

[Peter Krempa]

On 11/18/14 18:16, Nehal J Wani wrote:
> This patch enables the helper program to detect event(s) triggered when there
> is a change in lease length or expiry and client-id. This transfers complete
> control of leases database to libvirt and obsoletes use of the lease database
> file (.leases). That file will not be created, read, or written.
> This is achieved by adding the option --leasefile-ro to dnsmasq and passing a
> custom env var to leaseshelper, which helps us map events related to leases
> with their corresponding network bridges, no matter what the event be.
> 
> Also, this requires the addition of a new non-lease entry in our custom lease
> database: "server-duid". It is required to identify a DHCPv6 server.
> 
> Now that dnsmasq doesn't maintain its own leases database, it relies on our
> helper program to tell it about previous leases and server duid. Thus, this
> patch makes our leases program honor an extra action: "init", in which it 
> sends
> the known info in a particular format to dnsmasq by printing it to stdout.
> 
> ---
> 
>  This is compatible with libvirt 1.2.6 as only additions have been
>  introduced, and the old leases file (*.status) will still be supported.
> 
>  v4: * Removed extra data structures for segregating ipv4 and ipv6 leases.
> 
>  v3: * Add server-duid as an entry in the lease object for every ipv6 lease.
>  * Remove unnecessary variables and double copies.
>  * Take value from DNSMASQ_OLD_HOSTNAME if hostname is not known.
>  http://www.redhat.com/archives/libvir-list/2014-August/msg00028.html
> 
>  v2: http://www.redhat.com/archives/libvir-list/2014-July/msg01109.html
> 
>  v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html
> 
> 
> ---
>  src/network/bridge_driver.c |   3 +
>  src/network/leaseshelper.c  | 142 
> +++-
>  2 files changed, 117 insertions(+), 28 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 6cb421c..6ecbc37 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1280,7 +1280,10 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr 
> network,
>  
>  cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps));
>  virCommandAddArgFormat(cmd, "--conf-file=%s", configfile);
> +/* Libvirt gains full control of leases database */
> +virCommandAddArgFormat(cmd, "--leasefile-ro");

As we are now starting dnsmasq with the libvirt managed database, we
should get rid of the lease file configuration option present in the
config file.

>  virCommandAddArgFormat(cmd, "--dhcp-script=%s", leaseshelper_path);
> +virCommandAddEnvPair(cmd, "VIR_BRIDGE_NAME", network->def->bridge);
>  
>  *cmdout = cmd;
>  ret = 0;
> diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c
> index 5b3c9c3..fdd04a3 100644
> --- a/src/network/leaseshelper.c
> +++ b/src/network/leaseshelper.c
> @@ -50,6 +50,12 @@
>   */
>  #define VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX (32 * 1024 * 1024)
>  
> +/*
> + * Use this when passing possibly-NULL strings to printf-a-likes.
> + * Required for unknown parameters during init call.
> + */
> +#define EMPTY_STR(s) ((s) ? (s) : "*")
> +
>  static const char *program_name;
>  
>  /* Display version information. */
> @@ -65,7 +71,7 @@ usage(int status)
>  if (status) {
>  fprintf(stderr, _("%s: try --help for more details\n"), 
> program_name);
>  } else {
> -printf(_("Usage: %s add|old|del mac|clientid ip [hostname]\n"
> +printf(_("Usage: %s add|old|del|init mac|clientid ip [hostname]\n"
>   "Designed for use with 'dnsmasq --dhcp-script'\n"
>   "Refer to man page of dnsmasq for more details'\n"),
> program_name);
> @@ -89,6 +95,7 @@ enum virLeaseActionFlags {
>  VIR_LEASE_ACTION_ADD,   /* Create new lease */
>  VIR_LEASE_ACTION_OLD,   /* Lease already exists, renew it */
>  VIR_LEASE_ACTION_DEL,   /* Delete the lease */
> +VIR_LEASE_ACTION_INIT,  /* Tell dnsmasq of existing leases on 
> restart */
>  
>  VIR_LEASE_ACTION_LAST
>  };
> @@ -96,7 +103,7 @@ enum virLeaseActionFlags {
>  VIR_ENUM_DECL(virLeaseAction);
>  
>  VIR_ENUM_IMPL(virLeaseAction, VIR_LEASE_ACTION_LAST,
> -  "add", "old", "del");
> +  "add", "old", "del", "init");
>  
>  int
>  main(int argc, char **argv)
> @@ -107,12 +114,15 @@ main(int argc, char **argv)
>  char *custom_lease_file = NULL;
>  const char *ip = NULL;
>  const char *mac = NULL;
> +const char *ip_tmp = NULL;
> +const char *leases_str = NULL;
> +const char *server_duid_tmp = NULL;
>  const char *iaid = virGetEnvAllowSUID("DNSMASQ_IAID");
>  const char *clientid = virGetEnvAllowSUID("DNSMASQ_CLIENT_ID");
>  const char *interface = virGetEnvAllowSUID("DNSMASQ_INTERFACE");
>  const char *exptime_tmp = virGetEnvAllowSUID("DNSMASQ_LEASE_EXPIRES");
>  const char *ho

Re: [libvirt] [PATCH 06/11] qemu: Add handling for VSERPORT_CHANGE event

[Wang Rui]

On 2014/11/20 15:47, Peter Krempa wrote:
> the status XML is the piece that is reloaded on libvirtd restart for
> running VMs. For inactive VMs this doesn't make sense to report.

Thanks.
I missed status XML in /var/run/libvirt/.

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