Re: [libvirt] [PATCH 1/3] qemu: agent: fix uninitialized var case in qemuAgentGetFSInfo

[Nikolay Shirokovskiy]

On 08.12.2016 19:38, John Ferlan wrote:
> 
> 
> On 11/24/2016 04:19 AM, Nikolay Shirokovskiy wrote:
>> In case of 0 filesystems *info is not set while according
>> to virDomainGetFSInfo contract user should call free on it even
>> in case of 0 filesystems. Thus we need to properly set
>> it. NULL will be enough as free eats NULLs ok.
>> ---
>>  src/qemu/qemu_agent.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
>> index ec8d47e..c5cf403 100644
>> --- a/src/qemu/qemu_agent.c
>> +++ b/src/qemu/qemu_agent.c
>> @@ -1872,6 +1872,7 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, 
>> virDomainFSInfoPtr **info,
>>  ndata = virJSONValueArraySize(data);
>>  if (!ndata) {
>>  ret = 0;
>> +*info = NULL;
> 
> ACK - although there are more ways above this hunk that allow us to get
> to cleanup without setting *info = NULL;  Currently each of the callers

These are error paths. The caller have no obligations to free info in these 
cases.

> sets the input info to NULL before calling here

qemuDomainGetFSInfo does not set...

> 
> IOW: We could also move that *info = NULL up before the call to
> virAgentMakeCommand
> 

I looked here and there in the libvirt code and found out that it does not zero 
out 
output parameter immediately. I guess it makes sense. Output parameter can be 
actually filled in deep below the call stack. Thus if one starts with 
convention 
to zero out immediately one have to do it in every function on the path.

I guess caller itself can zero out output parameter to simplify its cleanup 
logic.
And some callers are not zero out, they cleanup conditionally for example - 
these 
rely on function to properly set output parameter.

Nikolay

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

Re: [libvirt] [PATCH 2/3] qemu: mark user defined websocket as used

[Nikolay Shirokovskiy]

On 08.12.2016 23:07, John Ferlan wrote:
> 
> 
> On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
>> We need extra state variable to distinguish between autogenerated
>> and user defined cases after auto generation is done.
>> ---
>>  src/conf/domain_conf.h  |  1 +
>>  src/qemu/qemu_process.c | 19 +--
>>  2 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 541b600..9bc4522 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -1468,6 +1468,7 @@ struct _virDomainGraphicsDef {
>>  int port;
>>  bool portReserved;
>>  int websocket;
>> +bool websocketGenerated;
>>  bool autoport;
>>  char *keymap;
>>  virDomainGraphicsAuthDef auth;
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 610..1799f33 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -3574,6 +3574,7 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver,
>>  if (virPortAllocatorAcquire(driver->webSocketPorts, ) < 0)
>>  return -1;
>>  graphics->data.vnc.websocket = port;
>> +graphics->data.vnc.websocketGenerated = true;
>>  }
>>  
>>  return 0;
>> @@ -4065,6 +4066,12 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr 
>> driver,
>>  return -1;
>>  graphics->data.vnc.portReserved = true;
>>  }
>> +if (graphics->data.vnc.websocket != -1 &&   /* auto websocket */
>> +graphics->data.vnc.websocket && /* no websocket */
>> +virPortAllocatorSetUsed(driver->remotePorts,
>> +graphics->data.vnc.websocket,
>> +true) < 0)
>> +return -1;
>>  break;
>>  
>>  case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
>> @@ -6189,8 +6196,16 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>>  false);
>>  graphics->data.vnc.portReserved = false;
>>  }
>> -virPortAllocatorRelease(driver->webSocketPorts,
>> -graphics->data.vnc.websocket);
>> +if (graphics->data.vnc.websocketGenerated) {
>> +virPortAllocatorRelease(driver->webSocketPorts,
>> +graphics->data.vnc.websocket);
>> +graphics->data.vnc.websocketGenerated = false;
>> +graphics->data.vnc.websocket = -1;
> 
> One more question... Should this be 0 instead of -1?
> 
> We set to -1 during Reserve and set the Generated flag indicating that
> the user didn't set to -1, but we did.

Not quite. -1 is valid user input. Reserve does not change websocket value.

> 
> So when we Release the autogenerated port that we created because -1 was
> set, shouldn't we set it back to 0 just like it would have been before
> we decided to set to -1 and set the Generated flag?

We autogenerate only because initial value is -1 which means 'autogenerate' by 
convention.
So if flag is set we should revert back that is set websocket to -1.

> 
> Avoids other code that then may *print* the websocket as -1...

websocket -1 is valid xml telling websocket should be autogenerated.

Nikolay

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

Re: [libvirt] [PATCH v4 9/9] docs: Add vhost-scsi

[John Ferlan]

On 12/07/2016 08:16 PM, Jim Fehlig wrote:
> On 11/22/2016 02:16 PM, John Ferlan wrote:
>>
>>
>> On 11/21/2016 10:58 PM, Eric Farman wrote:
>>> Signed-off-by: Eric Farman 
>>> ---
>>>  docs/formatdomain.html.in | 24 
>>>  1 file changed, 24 insertions(+)
>>>
>>
>> This will get squashed in with the conf patch. I'll also generate the
>> news.html.in entry:
>>
>>   vhost-scsi: Add support scsi_host hostdev passthrough
>>   Add the capability to pass through a scsi_host HBA and the
>>   associated LUNs to the guest.
>>   
>>
>>
>> ACK -
>>
>> I'll push these in a bit, just seeing what I get on my test system... Of
>> course I'm also curious what would happen if I try to pass through a
>> vHBA ;-)...
> 
> I have unsuccessfully tried it, but not sure how to correctly specify
> the vHBA in domXML.
> 
> First, create a vHBA:
> cat vhba.xml
> 
>211b32847342
>201b32847342
>
>  
>   99901001
>   9991
>   
>
> 
> virsh nodedev-create vhba.xml
> 
> (@John: Notice I'm using your "Allow creation of vHBA by
> parent_wwnn/wwpn or fabric_name" series. I'll try to respond with review
> comments on that series tomorrow.)
> 
> Add the vHBA to domain config using a variant of Eric's example:
>   
> 
> 
>   
> 
> Start the VM:
> virsh start test
> error: Failed to start domain test
> error: Path '/sys/kernel/config/target/vhost//naa.5001405df3e54061' is
> not accessible: No such file or directory
> 
> It looks like this patch series only handles scsi_host created with
> targetcli, but I didn't look at all the patches closely. Is it possible
> to use vhost-scsi with NPIV vport (vHBA)? If so, any pointers on how to
> specify the vHBA in  element?
> 

I've been buried by other things, but didn't want to ignore completely...

Unfortunately I haven't found a way to use configure w/ vHBA either - I
really was really hoping something could be done. It would have been so
nice to just use XML that would include the scsi_hostN of the vHBA. I
need to find the time to work through what targetcli does - I think it
creates another scsi_hostN which is essentially what is used.  That'd be
similar enough to what vport_create does. Then it would be (hah!) just a
matter of some code ;-) perhaps...

The example Eric has in the cover used multipath which is not something
I generally configure/use. I found some example using google to not use
multipath, but didn't spend too much time with it. I got it to do
something, but then got pinged to work on something else (you know how
that goes!).

John

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

Re: [libvirt] [PATCH v4 9/9] docs: Add vhost-scsi

[Jim Fehlig]

Eric Farman wrote:
> 
> 
> On 12/07/2016 08:16 PM, Jim Fehlig wrote:
>> On 11/22/2016 02:16 PM, John Ferlan wrote:
>>>
>>>
>>> On 11/21/2016 10:58 PM, Eric Farman wrote:
 Signed-off-by: Eric Farman 
 ---
  docs/formatdomain.html.in | 24 
  1 file changed, 24 insertions(+)

>>>
>>> This will get squashed in with the conf patch. I'll also generate the
>>> news.html.in entry:
>>>
>>>   vhost-scsi: Add support scsi_host hostdev passthrough
>>>   Add the capability to pass through a scsi_host HBA and the
>>>   associated LUNs to the guest.
>>>   
>>>
>>>
>>> ACK -
>>>
>>> I'll push these in a bit, just seeing what I get on my test system... Of
>>> course I'm also curious what would happen if I try to pass through a
>>> vHBA ;-)...
>>
>> I have unsuccessfully tried it, but not sure how to correctly specify
>> the vHBA in domXML.
>>
>> First, create a vHBA:
>> cat vhba.xml
>> 
>>211b32847342
>>201b32847342
>>
>>  
>>   99901001
>>   9991
>>   
>>
>> 
>> virsh nodedev-create vhba.xml
>>
>> (@John: Notice I'm using your "Allow creation of vHBA by
>> parent_wwnn/wwpn or fabric_name" series. I'll try to respond with
>> review comments on that series tomorrow.)
> 
> (@John:  Me too :)
> 
>>
>> Add the vHBA to domain config using a variant of Eric's example:
>>   
>> 
>> 
>>   
>>
>> Start the VM:
>> virsh start test
>> error: Failed to start domain test
>> error: Path '/sys/kernel/config/target/vhost//naa.5001405df3e54061' is
>> not accessible: No such file or directory
> 
> Eh?  Where did "5001..." come from?  Given the above hostdev snippet, I
> would've expected this to show "99900..."
> 
> Also, if you're not using an s390 machine, the address should probably
> be type='pci' or omitted altogether.

Sorry, too much copy and paste between your example and the various configs I
tried. Also too hasty in trying to get the mail out before knocking off for the 
day.

The actual hostdev config is

  


  

and the error

error: Path '/sys/kernel/config/target/vhost//naa.9991' is not
accessible: No such file or directory

>> It looks like this patch series only handles scsi_host created with
>> targetcli, but I didn't look at all the patches closely. 
> 
> Today, yes.  But more specifically, it's only handling scsi_host that
> connects to the vhost_scsi target, thus the
> "/sys/kernel/config/target/vhost/" prefix.  The NPIV vport is at a
> different locale.
> 
>> Is it possible to use vhost-scsi with NPIV vport (vHBA)? If so, any
>> pointers on how to specify the vHBA in  element?
> 
> This vhost-scsi series had to do some trickery rather than relying on
> virsh nodedev-list for data.  Looking at John's series, I suspect
> there's some additional work necessary for the vport-capable sysfs
> entries than what exists with this series.  Some of which I deferred
> from an earlier review comment.

Ok, thanks. I'll see if I can figure out how to make the vports work with
vhost-scsi.

Regards,
Jim

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

Re: [libvirt] [PATCH v2 03/10] daemon: Hook up the virLog{Get, Set}DefaultOutput to the daemon's init routine

[John Ferlan]

On 11/25/2016 08:12 AM, Erik Skultety wrote:
> Now that virLog{Get,Set}DefaultOutput routines are introduced we can wire them
> up to the daemon's logging initialization code. Also, change the order of
> operations a bit so that we still strictly honor our precedence of settings:
> cmdline > env > config now that outputs and filters are not appended anymore.
> 
> Signed-off-by: Erik Skultety 
> ---
>  daemon/libvirtd.c | 96 
> +++
>  src/locking/lock_daemon.c | 90 +++-
>  src/logging/log_daemon.c  | 92 +++--
>  3 files changed, 40 insertions(+), 238 deletions(-)
> 
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index 3902a8b..b2e89fd 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -675,103 +675,33 @@ daemonSetupLogging(struct daemonConfig *config,
>   * Libvirtd's order of precedence is:
>   * cmdline > environment > config
>   *
> - * In order to achieve this, we must process configuration in
> - * different order for the log level versus the filters and
> - * outputs. Because filters and outputs append, we have to look at
> - * the environment first and then only check the config file if
> - * there was no result from the environment. The default output is
> - * then applied only if there was no setting from either of the
> - * first two. Because we don't have a way to determine if the log
> - * level has been set, we must process variables in the opposite
> + * The default output is applied only if there was no setting from either
> + * the config or the environment. Because we don't have a way to 
> determine
> + * if the log level has been set, we must process variables in the 
> opposite
>   * order, each one overriding the previous.
>   */
>  if (config->log_level != 0)
>  virLogSetDefaultPriority(config->log_level);
>  
> +if (virLogSetDefaultOutput("libvirtd.log", godaemon, privileged) < 0)
> +return -1;
> +
> +/* In case the config is empty, the filters become empty and outputs will
> + * be set to default
> + */
> +virLogSetFilters(config->log_filters);
> +virLogSetOutputs(config->log_outputs);

Existing, but each ignores the return value and could be wrapped that
way too. Repeated for all 3 consumers...  Your call on adding the
ignore_value(); around it.

ACK

John

[...]


> 

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

Re: [libvirt] [PATCH v2 02/10] admin: Allow passing NULL to virLogSetFilters

[John Ferlan]

$SUB: "virLogSetOutputs"?


On 11/25/2016 08:12 AM, Erik Skultety wrote:
> Along with an empty string, it should also be possible for users to pass NULL
> to the public APIs which in turn would trigger a routine (future work)
> responsible for defining an appropriate default logging output given the 
> current
> circumstances.
> 
> Signed-off-by: Erik Skultety 
> ---
>  daemon/libvirtd.c | 2 +-
>  src/locking/lock_daemon.c | 2 +-
>  src/logging/log_daemon.c  | 2 +-
>  src/util/virlog.c | 8 +++-
>  src/util/virlog.h | 2 +-
>  5 files changed, 11 insertions(+), 5 deletions(-)
> 

ACK w/ commit msg adjustment

John

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

Re: [libvirt] [PATCH v2 01/10] virlog: Introduce virLog{Get, Set}DefaultOutput

[John Ferlan]

On 11/25/2016 08:11 AM, Erik Skultety wrote:
> These helpers will manage the log destination defaults (fetch/set). The reason
> for this is to stay consistent with the current daemon's behaviour with 
> respect
> to /etc/libvirt/.conf file, since both assignment of an empty string
> or not setting the log output variable at all trigger the daemon's decision on
> the default log destination which depends on whether the daemon runs 
> daemonized
> or not.
> This patch also changes the logic of the selection of the default
> logging output compared to how it is done now. The main difference though is
> that we should only really care if we're running daemonized or not, 
> disregarding
> the fact of (not) having a TTY completely (introduced by commit eba36a3878) as
> that should be of the libvirtd's parent concern (what FD it will pass to it).
> 
>  Before:
>  if (godaemon || !hasTTY):
>  if (journald):
>  use journald
> 
>  if (godaemon):
>  if (privileged):
>  use SYSCONFIG/libvirtd.log
>  else:
>  use XDG_CONFIG_HOME/libvirtd.log
>  else:
>  use stderr
> 
>  After:
>  if (godaemon):
>  if (journald):
>  use journald
> 
>  else:
>  if (privileged):
>  use SYSCONFIG/libvirtd.log
>  else:
>  use XDG_CONFIG_HOME/libvirtd.log
>  else:
>  use stderr
> 
> Signed-off-by: Erik Skultety 
> ---
>  src/libvirt_private.syms |  2 ++
>  src/util/virlog.c| 92 
> 
>  src/util/virlog.h|  2 ++
>  3 files changed, 96 insertions(+)
> 

ACK

John

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

Re: [libvirt] [libvirt-ci] [PATCH] Add builders for Fedora 25

[Martin Kletzander]

On Thu, Dec 08, 2016 at 03:55:43PM -0500, Yash Mankad wrote:

Add builders for Fedora 25 to ci.centos.org



Wouldn't it make sense to remove fedora 23 at the same time?


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

[libvirt] [libvirt-ci] [PATCH] Add builders for Fedora 25

[Yash Mankad]

Add builders for Fedora 25 to ci.centos.org

Signed-off-by: Yash Mankad 
---
 projects/libosinfo.yaml   | 1 +
 projects/libvirt-cim.yaml | 1 +
 projects/libvirt-glib.yaml| 1 +
 projects/libvirt-perl.yaml| 1 +
 projects/libvirt-python.yaml  | 1 +
 projects/libvirt-sandbox.yaml | 1 +
 projects/libvirt-tck.yaml | 1 +
 projects/libvirt.yaml | 3 +++
 projects/osinfo-db-tools.yaml | 1 +
 projects/osinfo-db.yaml   | 1 +
 projects/virt-manager.yaml| 1 +
 projects/virt-viewer.yaml | 1 +
 12 files changed, 14 insertions(+)

diff --git a/projects/libosinfo.yaml b/projects/libosinfo.yaml
index 49639f3..afb2de2 100644
--- a/projects/libosinfo.yaml
+++ b/projects/libosinfo.yaml
@@ -5,6 +5,7 @@
   - libvirt-centos-7
   - libvirt-fedora-23
   - libvirt-fedora-24
+  - libvirt-fedora-25
   - libvirt-fedora-rawhide
 title: libosinfo
 make_env: |
diff --git a/projects/libvirt-cim.yaml b/projects/libvirt-cim.yaml
index d1705af..82a8127 100644
--- a/projects/libvirt-cim.yaml
+++ b/projects/libvirt-cim.yaml
@@ -6,6 +6,7 @@
   - libvirt-centos-7
   - libvirt-fedora-23
   - libvirt-fedora-24
+  - libvirt-fedora-25
   - libvirt-fedora-rawhide
 title: libvirt CIM
 jobs:
diff --git a/projects/libvirt-glib.yaml b/projects/libvirt-glib.yaml
index 38bcded..7d897ab 100644
--- a/projects/libvirt-glib.yaml
+++ b/projects/libvirt-glib.yaml
@@ -5,6 +5,7 @@
   - libvirt-centos-7
   - libvirt-fedora-23
   - libvirt-fedora-24
+  - libvirt-fedora-25
   - libvirt-fedora-rawhide
 title: Libvirt GLib
 jobs:
diff --git a/projects/libvirt-perl.yaml b/projects/libvirt-perl.yaml
index 2bad51d..a9f4740 100644
--- a/projects/libvirt-perl.yaml
+++ b/projects/libvirt-perl.yaml
@@ -5,6 +5,7 @@
   - libvirt-centos-7
   - libvirt-fedora-23
   - libvirt-fedora-24
+  - libvirt-fedora-25
   - libvirt-fedora-rawhide
 title: Libvirt Perl
 jobs:
diff --git a/projects/libvirt-python.yaml b/projects/libvirt-python.yaml
index baca308..c1192d0 100644
--- a/projects/libvirt-python.yaml
+++ b/projects/libvirt-python.yaml
@@ -6,6 +6,7 @@
   - libvirt-centos-7
   - libvirt-fedora-23
   - libvirt-fedora-24
+  - libvirt-fedora-25
   - libvirt-fedora-rawhide
 title: Libvirt Python
 jobs:
diff --git a/projects/libvirt-sandbox.yaml b/projects/libvirt-sandbox.yaml
index eee249c..ebbc5be 100644
--- a/projects/libvirt-sandbox.yaml
+++ b/projects/libvirt-sandbox.yaml
@@ -4,6 +4,7 @@
 machines:
   - libvirt-fedora-23
   - libvirt-fedora-24
+  - libvirt-fedora-25
   - libvirt-fedora-rawhide
 title: Libvirt Sandbox
 jobs:
diff --git a/projects/libvirt-tck.yaml b/projects/libvirt-tck.yaml
index 571f3ce..a7c0233 100644
--- a/projects/libvirt-tck.yaml
+++ b/projects/libvirt-tck.yaml
@@ -4,6 +4,7 @@
 machines:
   - libvirt-fedora-23
   - libvirt-fedora-24
+  - libvirt-fedora-25
   - libvirt-fedora-rawhide
 title: Libvirt TCK
 jobs:
diff --git a/projects/libvirt.yaml b/projects/libvirt.yaml
index 20de27c..b49e01f 100644
--- a/projects/libvirt.yaml
+++ b/projects/libvirt.yaml
@@ -6,6 +6,7 @@
   - libvirt-centos-7
   - libvirt-fedora-23
   - libvirt-fedora-24
+  - libvirt-fedora-25
   - libvirt-fedora-rawhide
 title: Libvirt
 archive_format: xz
@@ -18,6 +19,7 @@
 - libvirt-debian
 - libvirt-fedora-23
 - libvirt-fedora-24
+- libvirt-fedora-25
 - libvirt-fedora-rawhide
 - libvirt-freebsd
   - autotools-syntax-check-job:
@@ -28,6 +30,7 @@
 - libvirt-debian
 - libvirt-fedora-23
 - libvirt-fedora-24
+- libvirt-fedora-25
 - libvirt-fedora-rawhide
   check_env: |
 export VIR_TEST_EXPENSIVE=1
diff --git a/projects/osinfo-db-tools.yaml b/projects/osinfo-db-tools.yaml
index bcb63da..5f275ab 100644
--- a/projects/osinfo-db-tools.yaml
+++ b/projects/osinfo-db-tools.yaml
@@ -5,6 +5,7 @@
   - libvirt-centos-7
   - libvirt-fedora-23
   - libvirt-fedora-24
+  - libvirt-fedora-25
   - libvirt-fedora-rawhide
 title: osinfo database tools
 jobs:
diff --git a/projects/osinfo-db.yaml b/projects/osinfo-db.yaml
index f48aa3f..9539724 100644
--- a/projects/osinfo-db.yaml
+++ b/projects/osinfo-db.yaml
@@ -5,6 +5,7 @@
   - libvirt-centos-7
   - libvirt-fedora-23
   - libvirt-fedora-24
+  - libvirt-fedora-25
   - libvirt-fedora-rawhide
 title: osinfo database
 jobs:
diff --git a/projects/virt-manager.yaml b/projects/virt-manager.yaml
index 737e37d..4485d5f 100644
--- a/projects/virt-manager.yaml
+++ b/projects/virt-manager.yaml
@@ -5,6 +5,7 @@
   - libvirt-centos-7
   - libvirt-fedora-23
   - libvirt-fedora-24
+  - libvirt-fedora-25
   - libvirt-fedora-rawhide
 title: Virtual Machine Manager
 jobs:
diff 

Re: [libvirt] [PATCH 2/3] qemu: mark user defined websocket as used

[John Ferlan]

On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
> We need extra state variable to distinguish between autogenerated
> and user defined cases after auto generation is done.
> ---
>  src/conf/domain_conf.h  |  1 +
>  src/qemu/qemu_process.c | 19 +--
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 541b600..9bc4522 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1468,6 +1468,7 @@ struct _virDomainGraphicsDef {
>  int port;
>  bool portReserved;
>  int websocket;
> +bool websocketGenerated;
>  bool autoport;
>  char *keymap;
>  virDomainGraphicsAuthDef auth;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 610..1799f33 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3574,6 +3574,7 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver,
>  if (virPortAllocatorAcquire(driver->webSocketPorts, ) < 0)
>  return -1;
>  graphics->data.vnc.websocket = port;
> +graphics->data.vnc.websocketGenerated = true;
>  }
>  
>  return 0;
> @@ -4065,6 +4066,12 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr 
> driver,
>  return -1;
>  graphics->data.vnc.portReserved = true;
>  }
> +if (graphics->data.vnc.websocket != -1 &&   /* auto websocket */
> +graphics->data.vnc.websocket && /* no websocket */
> +virPortAllocatorSetUsed(driver->remotePorts,
> +graphics->data.vnc.websocket,
> +true) < 0)
> +return -1;
>  break;
>  
>  case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
> @@ -6189,8 +6196,16 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>  false);
>  graphics->data.vnc.portReserved = false;
>  }
> -virPortAllocatorRelease(driver->webSocketPorts,
> -graphics->data.vnc.websocket);
> +if (graphics->data.vnc.websocketGenerated) {
> +virPortAllocatorRelease(driver->webSocketPorts,
> +graphics->data.vnc.websocket);
> +graphics->data.vnc.websocketGenerated = false;
> +graphics->data.vnc.websocket = -1;

One more question... Should this be 0 instead of -1?

We set to -1 during Reserve and set the Generated flag indicating that
the user didn't set to -1, but we did.

So when we Release the autogenerated port that we created because -1 was
set, shouldn't we set it back to 0 just like it would have been before
we decided to set to -1 and set the Generated flag?

Avoids other code that then may *print* the websocket as -1...

John

> +} else if (graphics->data.vnc.websocket) {
> +virPortAllocatorSetUsed(driver->remotePorts,
> +graphics->data.vnc.websocket,
> +false);
> +}
>  }
>  if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
>  if (graphics->data.spice.autoport) {
> 

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

Re: [libvirt] [PATCH 3/3] qemu: fix xml dump of autogenerated websocket

[John Ferlan]

On 12/08/2016 09:00 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 08.12.2016 16:21, John Ferlan wrote:
>>
>> Perhaps a commit message would answer my question below...
>>
>> On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
>>> ---
>>>  src/conf/domain_conf.c | 5 -
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 6e008e2..fb6ff0b 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -22534,7 +22534,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
>>>  virBufferAsprintf(buf, " autoport='%s'",
>>>def->data.vnc.autoport ? "yes" : "no");
>>>  
>>> -if (def->data.vnc.websocket)
>>> +if (def->data.vnc.websocketGenerated &&
>>
>> Wouldn't websocketGenerated imply an active domain? And a change of the
>> websocket in the active xml to be some value?
>>
>> So is the purpose of this because if you have an active domain you don't
>> want to display the websocket that was generated?
>>
>> And why is that?
>>
>> IOW: What are you trying to ensure with this patch?
>>
> 
> AFAIU this combination - active domain with inactive dump flag is used
> to serialize config in situations described in cover letter - migration
> or saving of a domain. So instead of saving port we save the fact it is 
> autogenerated
> and regenerate on migration destination/restore. One can infer this from 
> socket port logic in near by code.
> 

Oh - didn't read the cover

I can move that explanation in here before pushing.  Although I'll wait
for a response to my recent question in 2/3 before doing so.

John

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

[libvirt] [RFC] qmp: query-device-slots command

[Eduardo Habkost]

This adds a new command to QMP: query-device-slots. It will allow
management software to query possible slots where devices can be
plugged.

This implementation of the command will return:

* Multiple PCI slots per bus, in the case of PCI buses;
* One slot per bus in the case of the other buses;
* One slot for each entry from query-hotpluggable-cpus.

In the example below, I am not sure if the PCIe ports are all
supposed to report all slots, but I didn't find any existing
field in PCIBus that would help me figure out the actual number
of slots in a given PCI bus.

Git tree


This patch needs the previous query-machines series I am working
on. The full tree can be found on the git tree at:

  git://github.com/ehabkost/qemu-hacks.git work/query-machines-bus-info

Example output
--

The following output was returned by QEMU when running it as:

 $ qemu-system-x86_64 -machine q35 \
   -readconfig docs/q35-chipset.cfg \
   -smp 4,maxcpus=8,sockets=2,cores=2,threads=2

 {
"return": [
{
"available": false,
"devices": [
"/machine/unattached/device[30]",
"/machine/unattached/device[29]",
"/machine/unattached/device[28]",
"/machine/unattached/device[27]",
"/machine/unattached/device[26]",
"/machine/unattached/device[25]",
"/machine/unattached/device[24]",
"/machine/unattached/device[23]"
],
"accepted-device-types": [
"i2c-slave"
],
"props": {
"bus": "/machine/unattached/device[22]/i2c"
},
"hotpluggable": false,
"type": "generic"
},
{
"available": false,
"devices": [],
"accepted-device-types": [
"ide-device"
],
"props": {
"bus": "/machine/unattached/device[20]/ide.4"
},
"hotpluggable": false,
"type": "generic"
},
{
"available": false,
"devices": [],
"accepted-device-types": [
"ide-device"
],
"props": {
"bus": "/machine/unattached/device[20]/ide.5"
},
"hotpluggable": false,
"type": "generic"
},
{
"available": false,
"devices": [],
"accepted-device-types": [
"ide-device"
],
"props": {
"bus": "/machine/unattached/device[20]/ide.0"
},
"hotpluggable": false,
"type": "generic"
},
{
"available": false,
"devices": [],
"accepted-device-types": [
"ide-device"
],
"props": {
"bus": "/machine/unattached/device[20]/ide.1"
},
"hotpluggable": false,
"type": "generic"
},
{
"available": false,
"devices": [
"/machine/unattached/device[21]"
],
"accepted-device-types": [
"ide-device"
],
"props": {
"bus": "/machine/unattached/device[20]/ide.2"
},
"hotpluggable": false,
"type": "generic"
},
{
"available": false,
"devices": [],
"accepted-device-types": [
"ide-device"
],
"props": {
"bus": "/machine/unattached/device[20]/ide.3"
},
"hotpluggable": false,
"type": "generic"
},
{
"available": false,
"devices": [
"/machine/unattached/device[8]",
"/machine/q35/ioapic",
"/machine/q35",
"/machine/fw_cfg",
"/machine/unattached/device[1]"
],
"accepted-device-types": [
"sys-bus-device"
],
"props": {
"bus": "/machine/unattached/sysbus"
},
"hotpluggable": false,
"type": "generic"
},
{
"available": false,
"devices": [
"/machine/unattached/device[19]",
"/machine/unattached/device[18]",
"/machine/unattached/device[17]",
"/machine/unattached/device[16]",
"/machine/unattached/device[15]",
"/machine/unattached/device[14]",
"/machine/unattached/device[13]",
"/machine/unattached/device[12]",
"/machine/unattached/device[11]",
"/machine/unattached/device[10]",
"/machine/unattached/device[9]",
"/machine/unattached/device[7]",

Re: [libvirt] [PATCH v2] storage: vz storage pool support

[John Ferlan]

[...]

>>
>> Compare that to NFS, which uses mount which is included in well every
>> distro I can think of. That's a big difference. Also let's face it, NFS
>> has been the essential de facto goto tool to access remote storage for a
>> long time. Personally, I'd rather see the NFS code split out of the
>> *_fs.c backend, but I don't have the desire/time to do it - so it stays
>> as is.
> 
> But netfs pool type is not only for NFS, it also can be used to provide
> cifs and FUSE glusterfs mounts. These three just as vstorage have very
> little difference from local filesystems from pool POV after they are 
> mounted that's why I guess they implemented so tightly.
> 

Sure and they all pass through virStorageBackendFileSystemMount in order
to MOUNT something essentially via the mount command and usage of
specific qualifiers.

Which differs from using VSTORAGE_MOUNT a utility not provided on every
distro AFAIK.

John



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

Re: [libvirt] [PATCH v2] storage: vz storage pool support

[John Ferlan]

>> Compare that to NFS, which uses mount which is included in well every
>> distro I can think of. That's a big difference. Also let's face it, NFS
>> has been the essential de facto goto tool to access remote storage for a
>> long time. Personally, I'd rather see the NFS code split out of the
>> *_fs.c backend, but I don't have the desire/time to do it - so it stays
>> as is.
> 
> To sum this up, you still think that copy and paste isn't a problem here
> and will create more value than do any harm, right?
> 

Not sure what you're inferring by copy and paste - other than perhaps
having to decide for the vstorage backend which storage backend API's it
needs or should support.

The list of API's as I see from the path are:

+
+.startPool = virStorageBackendVzStart,
+.checkPool = virStorageBackendFileSystemCheck,
+.stopPool = virStorageBackendFileSystemStop,
+.findPoolSources = virStorageBackendVzfindPoolSources,
+.buildPool = virStorageBackendFileSystemBuild,
+.deletePool = virStorageBackendFileSystemDelete,
+.refreshPool = virStorageBackendFileSystemRefresh,
+.buildVol = virStorageBackendFileSystemVolBuild,
+.buildVolFrom = virStorageBackendFileSystemVolBuildFrom,
+.createVol = virStorageBackendFileSystemVolCreate,
+.refreshVol = virStorageBackendFileSystemVolRefresh,
+.deleteVol = virStorageBackendFileSystemVolDelete,
+.resizeVol = virStorageBackendFileSystemVolResize,
+.uploadVol = virStorageBackendVolUploadLocal,
+.downloadVol = virStorageBackendVolDownloadLocal,
+.wipeVol = virStorageBackendVolWipeLocal,
+

Other than startPool and findPoolSources, the code will reuse/call the
virStorageBackendFileSystem* API's - so the only copy/paste would be
copyrights, some #include's that would be required only for vstorage,
the VIR_FROM_THIS definition, VIR_LOG_INIT...  Nothing any other backend
wouldn't have to do.

Although I do question "checkPool" - I would think for vstorage that
should check if the environment is "available" somehow *if* you want
pool autostart

Also for stopPool the code will essentially call unmount. So is that
"expected" for vstorage?

Going through each API callout is how you'll be able to tell me/us that
taking that path will work for the vstorage environment.

John

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

[libvirt] [PATCH] news: Add description for perf.branch_instructions

[John Ferlan]

Signed-off-by: John Ferlan 
---
 Pushed as trivial 

 docs/news.html.in | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/docs/news.html.in b/docs/news.html.in
index 958bdd2..faea685 100644
--- a/docs/news.html.in
+++ b/docs/news.html.in
@@ -28,6 +28,12 @@
 
   
   Improvements
+
+  perf: Add perf.branch_instructions
+  Add support to get the count of branch instructions executed
+  by applications running on the platform
+  
+
   
   Bug fixes
   
-- 
2.7.4

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

Re: [libvirt] [PATCH v2] storage: vz storage pool support

[Olga Krishtal]

On 08/12/16 19:39, Olga Krishtal wrote:
Hi all,  I see here a lot of discussions and explanations about 
virtuozzo storage. So, I will just add some information.
Initially I have planned, that admin or user has properly configured 
vstorage somewhere over the
network. (It can be done via DNS records or zeroconf.) At the host 
where pool will be stored
user/admin do cluster discovery and authorization. As Maxim has 
mentioned before - vstorage
does not use the concept of users and groups, providing specific users 
and groups with access to specific parts of a cluster.
So, anyone authorized to access a cluster can access all its data. 
However, you can use additional parameters
during mounting to define mount owner usr name, group name and acecss 
mode (so, you can mount cluster as read-only)

Mistake, performing chown/chmod and etc does nothing.
This means that performing chown/chmod and etc - will have same effect 
as in nfs case.



Of course to perform this operations you need vstorage-client to be 
installed on the host.



On 08/12/16 16:47, Maxim Nestratov wrote:

08-Dec-16 15:17, John Ferlan пишет:



On 12/08/2016 04:19 AM, Maxim Nestratov wrote:

08-Dec-16 02:22, John Ferlan пишет:


[...]

I see what you mean; however, IMO vstorage should be separate. 
Maybe

there's another opinion out there, but since you're requiring
"something" else to be installed in order to get the WITH_VSTORAGE
to be
set to 1, then a separate file is in order.

Not sure they're comparable, but zfs has its own. Having separated
vstorage reduces the chance that some day some incompatible 
logic is

added/altered in the *fs.c (or vice versa).

Ok. I will try.


I think you should consider the *_fs.c code to be the "default" of
sorts. That is default file/dir structure with netfs added in. The
vstorage may just be some file system, but it's not something 
(yet) on

"every" distribution.

I did not understand actually, what you mean  "be the "default" of
sorts."
As I have understood - what I need to do is to create 
backend_vstorage.c

with all create/delete/* functionality.

Sorry - I was trying to think of a better way to explain... The 
'fs' and
'nfs' pool are default of sorts because one can "ls" (on 
UNIX/Linux) or

"dir" (on Windows) and get a list of files.

"ls" and "dir" are inherent to the OS, while in this case vstorage
commands are installed separately.

Once you mounted your vstorage cluster to a local filesystem you can
also "ls" it. Thus, I can't see much difference from nfs here.

So if it's more like NFS, then how does one ensure that the local 
userid

X is the same as the remote userid X? NFS has a root-squashing concept
that results in numerous shall we say "interesting" issues.


Vstorage doesn't have users concept. Authentication is made by a 
password per node just once.
If authentication passes, a key is stored in 
/etc/vstorage/clusters/CLUSTER_NAME/auth_digest.key
Then, permissions are set to a mount point during mounting with -u 
USER -g GROUP -m  MODE options

provided to vstorage-mount command.


Check out the virFileOpen*, virDirCreate, and virFileRemove...

Also what about viFileIsShareFSType? And security_selinux.c code for
NFS? If you use cscope, just search on NFS.

In the virStorageBackendVzStart, I see:

VSTORAGE_MOUNT -c $pool.source.name $pool.target.path


This call certainly lacks user/group/mode parameters and should be 
fixed in the next series.

Do you mean fix the default behavior for non-root users?






where VSTORAGE_MOUNT is a build (configure.ac) definition that is the
"Location or name of vstorage-mount program" which would only be set if
the proper package was installed.

In the virStorageBackendVzfindPoolSources, I see:

VSTORAGE discover

which I assume generates some list of remote "services" (for lack of a
better term) which can be used as/for pool.source.name in order to be
well mounted by the VSTORAGE_MOUNT program.

Compare that to NFS, which uses mount which is included in well every
distro I can think of. That's a big difference. Also let's face it, NFS
has been the essential de facto goto tool to access remote storage 
for a

long time. Personally, I'd rather see the NFS code split out of the
*_fs.c backend, but I don't have the desire/time to do it - so it stays
as is.


To sum this up, you still think that copy and paste isn't a problem 
here and will create more value than do any harm, right?


Maxim

[snip]







--
Best regards,
Olga

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

Re: [libvirt] [PATCH 1/3] qemu: agent: fix uninitialized var case in qemuAgentGetFSInfo

[John Ferlan]

On 11/24/2016 04:19 AM, Nikolay Shirokovskiy wrote:
> In case of 0 filesystems *info is not set while according
> to virDomainGetFSInfo contract user should call free on it even
> in case of 0 filesystems. Thus we need to properly set
> it. NULL will be enough as free eats NULLs ok.
> ---
>  src/qemu/qemu_agent.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index ec8d47e..c5cf403 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -1872,6 +1872,7 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr 
> **info,
>  ndata = virJSONValueArraySize(data);
>  if (!ndata) {
>  ret = 0;
> +*info = NULL;

ACK - although there are more ways above this hunk that allow us to get
to cleanup without setting *info = NULL;  Currently each of the callers
sets the input info to NULL before calling here

IOW: We could also move that *info = NULL up before the call to
virAgentMakeCommand


John

>  goto cleanup;
>  }
>  if (VIR_ALLOC_N(info_ret, ndata) < 0)
> 

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

Re: [libvirt] [PATCH v2] storage: vz storage pool support

[Olga Krishtal]

Hi all,  I see here a lot of discussions and explanations about 
virtuozzo storage. So, I will just add some information.
Initially I have planned, that admin or user has properly configured 
vstorage somewhere over the
network. (It can be done via DNS records or zeroconf.) At the host where 
pool will be stored
user/admin do cluster discovery and authorization. As Maxim has 
mentioned before - vstorage
does not use the concept of users and groups, providing specific users 
and groups with access to specific parts of a cluster.
So, anyone authorized to access a cluster can access all its data. 
However, you can use additional parameters
during mounting to define mount owner usr name, group name and acecss 
mode (so, you can mount cluster as read-only)
This means that performing chown/chmod and etc - will have same effect 
as in nfs case.
Of course to perform this operations you need vstorage-client to be 
installed on the host.



On 08/12/16 16:47, Maxim Nestratov wrote:

08-Dec-16 15:17, John Ferlan пишет:



On 12/08/2016 04:19 AM, Maxim Nestratov wrote:

08-Dec-16 02:22, John Ferlan пишет:


[...]


I see what you mean; however, IMO vstorage should be separate. Maybe
there's another opinion out there, but since you're requiring
"something" else to be installed in order to get the WITH_VSTORAGE
to be
set to 1, then a separate file is in order.

Not sure they're comparable, but zfs has its own. Having separated
vstorage reduces the chance that some day some incompatible logic is
added/altered in the *fs.c (or vice versa).

Ok. I will try.


I think you should consider the *_fs.c code to be the "default" of
sorts. That is default file/dir structure with netfs added in. The
vstorage may just be some file system, but it's not something 
(yet) on

"every" distribution.

I did not understand actually, what you mean  "be the "default" of
sorts."
As I have understood - what I need to do is to create 
backend_vstorage.c

with all create/delete/* functionality.

Sorry - I was trying to think of a better way to explain... The 
'fs' and
'nfs' pool are default of sorts because one can "ls" (on 
UNIX/Linux) or

"dir" (on Windows) and get a list of files.

"ls" and "dir" are inherent to the OS, while in this case vstorage
commands are installed separately.

Once you mounted your vstorage cluster to a local filesystem you can
also "ls" it. Thus, I can't see much difference from nfs here.


So if it's more like NFS, then how does one ensure that the local userid
X is the same as the remote userid X? NFS has a root-squashing concept
that results in numerous shall we say "interesting" issues.


Vstorage doesn't have users concept. Authentication is made by a 
password per node just once.
If authentication passes, a key is stored in 
/etc/vstorage/clusters/CLUSTER_NAME/auth_digest.key
Then, permissions are set to a mount point during mounting with -u 
USER -g GROUP -m  MODE options

provided to vstorage-mount command.


Check out the virFileOpen*, virDirCreate, and virFileRemove...

Also what about viFileIsShareFSType? And security_selinux.c code for
NFS? If you use cscope, just search on NFS.

In the virStorageBackendVzStart, I see:

VSTORAGE_MOUNT -c $pool.source.name $pool.target.path


This call certainly lacks user/group/mode parameters and should be 
fixed in the next series.

Do you mean fix the default behavior for non-root users?






where VSTORAGE_MOUNT is a build (configure.ac) definition that is the
"Location or name of vstorage-mount program" which would only be set if
the proper package was installed.

In the virStorageBackendVzfindPoolSources, I see:

VSTORAGE discover

which I assume generates some list of remote "services" (for lack of a
better term) which can be used as/for pool.source.name in order to be
well mounted by the VSTORAGE_MOUNT program.

Compare that to NFS, which uses mount which is included in well every
distro I can think of. That's a big difference. Also let's face it, NFS
has been the essential de facto goto tool to access remote storage for a
long time. Personally, I'd rather see the NFS code split out of the
*_fs.c backend, but I don't have the desire/time to do it - so it stays
as is.


To sum this up, you still think that copy and paste isn't a problem 
here and will create more value than do any harm, right?


Maxim

[snip]




--
Best regards,
Olga

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

Re: [libvirt] [PATCH 2/3] qemu: don't use vmdef without domain lock

[John Ferlan]

On 11/24/2016 04:19 AM, Nikolay Shirokovskiy wrote:
> Current call to qemuAgentGetFSInfo in qemuDomainGetFSInfo is
> unsafe. Domain lock is dropped and we use vm->def. To fix that
> let's fill in intermediate qemuAgentFsInfo structure in
> qemuAgentGetFSInfo and use vm->def to convert result later when
> lock is hold.
> ---
>  src/qemu/qemu_agent.c| 52 +++
>  src/qemu/qemu_agent.h| 25 -
>  src/qemu/qemu_driver.c   | 88 
> +++-
>  tests/Makefile.am|  1 -
>  tests/qemuagentdata/qemuagent-fsinfo.xml | 39 --
>  tests/qemuagenttest.c| 47 +++--
>  6 files changed, 159 insertions(+), 93 deletions(-)
>  delete mode 100644 tests/qemuagentdata/qemuagent-fsinfo.xml
> 

This failed to compile for me as 'num' wasn't initialized in
qemuDomainGetFSInfo

> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index c5cf403..5230cbc 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -1835,18 +1835,15 @@ qemuAgentSetTime(qemuAgentPtr mon,
>  
>  
>  int
> -qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
> -   virDomainDefPtr vmdef)
> +qemuAgentGetFSInfo(qemuAgentPtr mon, qemuAgentFsInfoPtr **info)
>  {
>  size_t i, j, k;
>  int ret = -1;
>  ssize_t ndata = 0, ndisk;
> -char **alias;
>  virJSONValuePtr cmd;
>  virJSONValuePtr reply = NULL;
>  virJSONValuePtr data;
> -virDomainFSInfoPtr *info_ret = NULL;
> -virPCIDeviceAddress pci_address;
> +qemuAgentFsInfoPtr *info_ret = NULL;
>  
>  cmd = qemuAgentMakeCommand("guest-get-fsinfo", NULL);
>  if (!cmd)
> @@ -1879,6 +1876,8 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr 
> **info,
>  goto cleanup;
>  
>  for (i = 0; i < ndata; i++) {
> +qemuAgentFsDiskAliasPtr alias;
> +
>  /* Reverse the order to arrange in mount order */
>  virJSONValuePtr entry = virJSONValueArrayGet(data, ndata - 1 - i);
>  
> @@ -1941,7 +1940,6 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr 
> **info,
>  int diskaddr[3], pciaddr[4];
>  const char *diskaddr_comp[] = {"bus", "target", "unit"};
>  const char *pciaddr_comp[] = {"domain", "bus", "slot", 
> "function"};
> -virDomainDiskDefPtr diskDef;
>  
>  if (!disk) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -1967,6 +1965,11 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, 
> virDomainFSInfoPtr **info,
>  goto cleanup;
>  }
>  }
> +
> +alias->bus = diskaddr[0];
> +alias->target = diskaddr[1];
> +alias->unit = diskaddr[2];
> +
>  for (k = 0; k < 4; k++) {
>  if (virJSONValueObjectGetNumberInt(
>  pci, pciaddr_comp[k], [k]) < 0) {
> @@ -1977,22 +1980,13 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, 
> virDomainFSInfoPtr **info,
>  }
>  }
>  
> -pci_address.domain = pciaddr[0];
> -pci_address.bus = pciaddr[1];
> -pci_address.slot = pciaddr[2];
> -pci_address.function = pciaddr[3];
> -if (!(diskDef = virDomainDiskByAddress(
> - vmdef, _address,
> - diskaddr[0], diskaddr[1], diskaddr[2])))
> -continue;
> +alias->address.domain = pciaddr[0];
> +alias->address.bus = pciaddr[1];
> +alias->address.slot = pciaddr[2];
> +alias->address.function = pciaddr[3];
>  
> -if (VIR_STRDUP(*alias, diskDef->dst) < 0)
> -goto cleanup;
> -
> -if (*alias) {
> -alias++;
> -info_ret[i]->ndevAlias++;
> -}
> +alias++;
> +info_ret[i]->ndevAlias++;
>  }
>  }
>  
> @@ -2003,7 +1997,7 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr 
> **info,
>   cleanup:
>  if (info_ret) {
>  for (i = 0; i < ndata; i++)
> -virDomainFSInfoFree(info_ret[i]);
> +qemuAgentFsInfoFree(info_ret[i]);
>  VIR_FREE(info_ret);
>  }
>  virJSONValueFree(cmd);
> @@ -2242,3 +2236,17 @@ qemuAgentSetUserPassword(qemuAgentPtr mon,
>  VIR_FREE(password64);
>  return ret;
>  }
> +
> +void
> +qemuAgentFsInfoFree(qemuAgentFsInfoPtr info)
> +{
> +if (!info)
> +return;
> +
> +VIR_FREE(info->mountpoint);
> +VIR_FREE(info->name);
> +VIR_FREE(info->fstype);
> +VIR_FREE(info->devAlias);

You'd have to free each 'ndevAlias' here.

> +
> +VIR_FREE(info);
> +}
> diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
> index 6dd9c70..4b2277c 100644
> --- a/src/qemu/qemu_agent.h
> +++ b/src/qemu/qemu_agent.h
> @@ -75,8 +75,29 @@ int qemuAgentShutdown(qemuAgentPtr mon,
>  int 

Re: [libvirt] [PATCH 3/3] qemu: agent: take monitor lock in qemuAgentNotifyEvent

[John Ferlan]

On 11/24/2016 04:19 AM, Nikolay Shirokovskiy wrote:
> qemuAgentNotifyEvent notify on a lock condition without taking
> the lock. This works but it is a subject to race conditions.
> ---
>  src/qemu/qemu_agent.c | 4 
>  1 file changed, 4 insertions(+)
> 

But the vm is locked prior to any priv->agent dereference and call - so
what path could free priv->agent before we get into this NotifyEvent
code?  I suppose it wouldn't hurt, but we're not entering the agent and
the AgentEOF would require vm lock to clear agent.

John

> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index 5230cbc..ad031d0 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -1248,6 +1248,8 @@ qemuAgentMakeStringsArray(const char **strings, 
> unsigned int len)
>  void qemuAgentNotifyEvent(qemuAgentPtr mon,
>qemuAgentEvent event)
>  {
> +virObjectLock(mon);
> +
>  VIR_DEBUG("mon=%p event=%d await_event=%d", mon, event, 
> mon->await_event);
>  if (mon->await_event == event) {
>  mon->await_event = QEMU_AGENT_EVENT_NONE;
> @@ -1257,6 +1259,8 @@ void qemuAgentNotifyEvent(qemuAgentPtr mon,
>  virCondSignal(>notify);
>  }
>  }
> +
> +virObjectUnlock(mon);
>  }
>  
>  VIR_ENUM_DECL(qemuAgentShutdownMode);
> 

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

Re: [libvirt] [PATCH] qemu: Don't try to find compression program for "raw" memory images

[Michal Privoznik]

On 08.12.2016 11:33, Peter Krempa wrote:
> There's nothing to compress if the requested snapshot memory format is
> set to 'raw' explicitly. After commit 9e14689ea libvirt would try to
> run /sbin/raw to process them emory stream if the qemu.conf option
> snapshot_image_format is set.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1402726
> ---
>  src/qemu/qemu_driver.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 6b177e9..4ef1860 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -3300,6 +3300,9 @@ qemuGetCompressionProgram(const char *imageFormat,
>  if ((ret = qemuSaveCompressionTypeFromString(imageFormat)) < 0)
>  goto error;
> 
> +if (ret == QEMU_SAVE_FORMAT_RAW)
> +return QEMU_SAVE_FORMAT_RAW;
> +
>  if (!(*compresspath = virFindFileInPath(imageFormat)))
>  goto error;
> 

ACK

Michal

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

Re: [libvirt] [PATCH 0/2] Couple of recently found Coverity issues

[Michal Privoznik]

On 07.12.2016 14:20, John Ferlan wrote:
> Nuisance for some, but both have negative repurcussions although one is
> "just" a test.
> 
> John Ferlan (2):
>   tests: Fix virmacmaptest when allocation fails
>   nss: Need to check error condition on virJSONValueArraySize
> 
>  tests/virmacmaptest.c   | 9 +++--
>  tools/nss/libvirt_nss.c | 3 ++-
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 

ACK

Michal

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

Re: [libvirt] [PATCH 2/2] nss: Need to check error condition on virJSONValueArraySize

[Michal Privoznik]

On 07.12.2016 14:20, John Ferlan wrote:
> If the 'nleases < 0' on return, then the subsequent call to
> findLeaseInJSON will not produce the expected results (passed
> in as a size_t, but nleases is a ssize_t).  So check if the
> returned value < 0 and if so, goto cleanup.
> 
> Found by Coverity as a NEGATIVE_RETURNS event
> 
> Signed-off-by: John Ferlan 
> ---
>  tools/nss/libvirt_nss.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c
> index 418c11f..b69e62c 100644
> --- a/tools/nss/libvirt_nss.c
> +++ b/tools/nss/libvirt_nss.c
> @@ -309,7 +309,8 @@ findLease(const char *name,
>  }
>  VIR_DIR_CLOSE(dir);
>  
> -nleases = virJSONValueArraySize(leases_array);
> +if ((nleases = virJSONValueArraySize(leases_array)) < 0)
> +goto cleanup;
>  DEBUG("Read %zd leases", nleases);

Well, this one could happen iff @leases_array wouldn't be a JSON array.
Thus I'm okay with skipping DEBUG() in that case.

Michal

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

Re: [libvirt] [PATCH 07/11] iscsi: Change order of checks in createVport

[Eric Farman]

On 11/18/2016 09:26 AM, John Ferlan wrote:

Move the check for an already existing vHBA to the top of the function.
No sense in first decoding a provided parent if the next thing we're going
to do is fail if a provided wwnn/wwpn already exists.

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

diff --git a/src/storage/storage_backend_scsi.c 
b/src/storage/storage_backend_scsi.c
index cf93fdc..9863880 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -706,20 +706,6 @@ createVport(virConnectPtr conn,
conn, NULLSTR(configFile), NULLSTR(adapter->data.fchost.parent),
adapter->data.fchost.wwnn, adapter->data.fchost.wwpn);

-/* If a parent was provided, then let's make sure it's vhost capable */
-if (adapter->data.fchost.parent) {
-if (virGetSCSIHostNumber(adapter->data.fchost.parent, _host) < 
0)
-return -1;
-
-if (!virIsCapableFCHost(NULL, parent_host)) {
-virReportError(VIR_ERR_XML_ERROR,
-   _("parent '%s' specified for vHBA "
- "is not vport capable"),
-   adapter->data.fchost.parent);
-return -1;
-}
-}
-
  /* If we find an existing HBA/vHBA within the fc_host sysfs
   * using the wwnn/wwpn, then a nodedev is already created for
   * this pool and we don't have to create the vHBA
@@ -736,6 +722,20 @@ createVport(virConnectPtr conn,
  goto cleanup;
  }

+/* If a parent was provided, then let's make sure it's vhost capable */
+if (adapter->data.fchost.parent) {
+if (virGetSCSIHostNumber(adapter->data.fchost.parent, _host) < 
0)
+return -1;
+
+if (!virIsCapableFCHost(NULL, parent_host)) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("parent '%s' specified for vHBA "
+ "is not vport capable"),
+   adapter->data.fchost.parent);
+return -1;
+}
+}
+
  if (!adapter->data.fchost.parent) {


This could be an else now.

 - Eric


  if (!(parent_hoststr = virFindFCHostCapableVport(NULL))) {
  virReportError(VIR_ERR_XML_ERROR, "%s",


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

Re: [libvirt] [PATCH v2] lxc: monitor now holds a reference to the domain

[Michal Privoznik]

On 08.12.2016 16:29, Cédric Bosdonnat wrote:
> If the monitor doesn't hold a reference to the domain object
> the object may be destroyed before the monitor actually stops.
> ---
>  v2: Moved vm ref upper, removed vm unref in error case.
> 
>  src/lxc/lxc_monitor.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/lxc/lxc_monitor.c b/src/lxc/lxc_monitor.c
> index d828d528a..9cab6c203 100644
> --- a/src/lxc/lxc_monitor.c
> +++ b/src/lxc/lxc_monitor.c
> @@ -175,7 +175,7 @@ virLXCMonitorPtr virLXCMonitorNew(virDomainObjPtr vm,
> mon->program) < 0)
>  goto error;
>  
> -mon->vm = vm;
> +mon->vm = virObjectRef(vm);
>  memcpy(>cb, cb, sizeof(mon->cb));
>  
>  virObjectRef(mon);
> @@ -201,6 +201,7 @@ static void virLXCMonitorDispose(void *opaque)
>  if (mon->cb.destroy)
>  (mon->cb.destroy)(mon, mon->vm);
>  virObjectUnref(mon->program);
> +virObjectUnref(mon->vm);
>  }
>  
>  
> 

Yup, this one looks good.

ACK

Michal

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

[libvirt] [PATCH v2] lxc: monitor now holds a reference to the domain

[Cédric Bosdonnat]

If the monitor doesn't hold a reference to the domain object
the object may be destroyed before the monitor actually stops.
---
 v2: Moved vm ref upper, removed vm unref in error case.

 src/lxc/lxc_monitor.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/lxc/lxc_monitor.c b/src/lxc/lxc_monitor.c
index d828d528a..9cab6c203 100644
--- a/src/lxc/lxc_monitor.c
+++ b/src/lxc/lxc_monitor.c
@@ -175,7 +175,7 @@ virLXCMonitorPtr virLXCMonitorNew(virDomainObjPtr vm,
mon->program) < 0)
 goto error;
 
-mon->vm = vm;
+mon->vm = virObjectRef(vm);
 memcpy(>cb, cb, sizeof(mon->cb));
 
 virObjectRef(mon);
@@ -201,6 +201,7 @@ static void virLXCMonitorDispose(void *opaque)
 if (mon->cb.destroy)
 (mon->cb.destroy)(mon, mon->vm);
 virObjectUnref(mon->program);
+virObjectUnref(mon->vm);
 }
 
 
-- 
2.11.0

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

Re: [libvirt] [PATCH] conf: Make scheduler formatting simpler

[Martin Kletzander]

On Thu, Dec 08, 2016 at 08:49:55AM -0500, John Ferlan wrote:



On 11/22/2016 08:53 AM, Martin Kletzander wrote:

Since the great rework of how we store vcpu- and iothread-related
data, we have overly complex part of code that is trying to format the
scheduler tuning data in as less lines as possible by grouping
settings for multiple threads.  That was designed as an input syntax
sugar for users, but we don't need to also use that when formatting
the XML.  Switching to simple enumeration makes the code nicer,
shorter and more welcoming to future changes.

Signed-off-by: Martin Kletzander 
---
 src/conf/domain_conf.c | 209 -
 ...l2xmlout-cputune-iothreadsched-zeropriority.xml |   7 +-
 .../qemuxml2xmlout-cputune-iothreadsched.xml   |   7 +-
 3 files changed, 43 insertions(+), 180 deletions(-)



Yes it certainly does make it easier to read...

One suggestion - change the name of the function to
"virDomainSchedulerFormat" since it makes it easier to find
virDomainSchedulerParse

ACK



OK, good point, thanks, pushed.


John



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 9/9] docs: Add vhost-scsi

[Eric Farman]

On 12/07/2016 08:16 PM, Jim Fehlig wrote:

On 11/22/2016 02:16 PM, John Ferlan wrote:



On 11/21/2016 10:58 PM, Eric Farman wrote:

Signed-off-by: Eric Farman 
---
 docs/formatdomain.html.in | 24 
 1 file changed, 24 insertions(+)



This will get squashed in with the conf patch. I'll also generate the
news.html.in entry:

  vhost-scsi: Add support scsi_host hostdev passthrough
  Add the capability to pass through a scsi_host HBA and the
  associated LUNs to the guest.
  


ACK -

I'll push these in a bit, just seeing what I get on my test system... Of
course I'm also curious what would happen if I try to pass through a
vHBA ;-)...


I have unsuccessfully tried it, but not sure how to correctly specify 
the vHBA in domXML.


First, create a vHBA:
cat vhba.xml

   211b32847342
   201b32847342
   
 
  99901001
  9991
  
   

virsh nodedev-create vhba.xml

(@John: Notice I'm using your "Allow creation of vHBA by 
parent_wwnn/wwpn or fabric_name" series. I'll try to respond with 
review comments on that series tomorrow.)


(@John:  Me too :)



Add the vHBA to domain config using a variant of Eric's example:
  


  

Start the VM:
virsh start test
error: Failed to start domain test
error: Path '/sys/kernel/config/target/vhost//naa.5001405df3e54061' is 
not accessible: No such file or directory


Eh?  Where did "5001..." come from?  Given the above hostdev snippet, I 
would've expected this to show "99900..."


Also, if you're not using an s390 machine, the address should probably 
be type='pci' or omitted altogether.




It looks like this patch series only handles scsi_host created with 
targetcli, but I didn't look at all the patches closely. 


Today, yes.  But more specifically, it's only handling scsi_host that 
connects to the vhost_scsi target, thus the 
"/sys/kernel/config/target/vhost/" prefix.  The NPIV vport is at a 
different locale.


Is it possible to use vhost-scsi with NPIV vport (vHBA)? If so, any 
pointers on how to specify the vHBA in  element?


This vhost-scsi series had to do some trickery rather than relying on 
virsh nodedev-list for data.  Looking at John's series, I suspect 
there's some additional work necessary for the vport-capable sysfs 
entries than what exists with this series.  Some of which I deferred 
from an earlier review comment.


 - Eric



Regards,
Jim



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

Re: [libvirt] [PATCH v3 1/4] Gathering vhostuser interface stats with ovs

[Mehdi Abaakouk]

-if (ret == 0)
-ret = virNetInterfaceStats(path, stats);
-else
+if (net) {
+if (net->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
+ret = virNetDevOpenvswitchInterfaceStats(path, stats);
+} else {
+ret = virNetInterfaceStats(path, stats);
+}
+} else {
 virReportError(VIR_ERR_INVALID_ARG,
_("invalid path, '%s' is not a known interface"), path);
+}



Oh my. Not your fault but this looks ugly. It has even before you've
touched it.

BTW: maybe I am misreading something but my understanding of vhost-user
is that it can be plugged into any type of bridge (e.g. snabb). How does
this work then if we run ovs-vsctl then? 


I don't think you misreading, vhostuser can be created by anything it's
just a unix-socket in this end. And libvirt only known the location of
this socket and not how it have been created.

libvirt was already guessing at getting the statistics, by 'trying' with
/proc/net/dev even that doesn't make any sense for vhostuser interface. Now I
just 'try' with ovs-vsctl but perhaps that doesn't make sense too if the
unix-socket have not been created by openvswitch.

Since libvirt have no real control of how a (host) network interface is
created. It can only guess the statistics location. My change just adds a new
way/tool to guess that.


Do you perhaps have a set of
steps how can I test this feature? Because so far I've used
vhost-user-bridge helper from qemu repo but this will not work with it.


About testing, my setup looks like:

I install the dpdk variant of openvswitch and dpdk tools

I enable dpdk support in ovs with:
(This can be a bit different depending on OS and openvswitch version)

$ ovs-vsctl set Open_vSwitch . "other_config:dpdk-init=true"
$ ovs-vsctl set Open_vSwitch . "other_config:dpdk-alloc-mem=2048"
$ ovs-vsctl set Open_vSwitch . "other_config:dpdk-extra=--vhost-owner 
libvirt-qemu:kvm --vhost-perm 0666"
$ systemctl restart openvswitch-switch

I unbind a network card (virtio network card work well with recent dpdk) on my
host with command like:
$ dpdk-devbind -u :00:04.0
(previously called dpdk_nic_bind, too)

I create a bridge with two interfaces. dpdk0 is the first unbind interface. 
dpdkvhostuser0 is
a vhostuser unix-socket located /var/run/openvswitch/dpdkvhostuser0

$ ovs-vsctl add-br br0 -- set bridge br0 datapath_type=netdev
$ ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk
$ ovs-vsctl add-port br0 dpdkvhostuser0 -- set Interface dpdkvhostuser0 
type=dpdkvhostuser

And I allow packets to pass from/to each interfaces

$ ovs-ofctl del-flows br0
$ ovs-ofctl add-flow br0 in_port=1,action=output:2
$ ovs-ofctl add-flow br0 in_port=2,action=output:1

Then I create a VM with a network interface that looks like:


   
   
   
   
   
   
   



I do some ping command to make statistics filled

Test with:

$ ovs-vsctl get Interface dpdkvhostuser0 statistics
$ virsh domifstat  dpdkvhostuser0

Cheers,
--
Mehdi Abaakouk
mail: sil...@sileht.net
irc: sileht

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

[libvirt] [PATCH] libvirt.spec: Package libnss_libvirt_guest.so.2

[Michal Privoznik]

In 22f7ceb695a I've introduced another NSS module
but forgot to package it in libvirt-nss.rpm.

Signed-off-by: Michal Privoznik 
---

Pushed under trivial rule.

 libvirt.spec.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 4bb0699cb..51ffbb55c 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1858,6 +1858,7 @@ exit 0
 
 %files nss
 %{_libdir}/libnss_libvirt.so.2
+%{_libdir}/libnss_libvirt_guest.so.2
 
 %if %{with_lxc}
 %files login-shell
-- 
2.11.0

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

Re: [libvirt] [PATCH 1/2] qemu: Allow saving QEMU libvirt state to a pipe

[Chen Hanxiao]

At 2016-12-08 20:08:11, "Peter Krempa"  wrote:
>On Sat, Dec 03, 2016 at 17:45:47 +0800, Chen Hanxiao wrote:
>> From: Chen Hanxiao 
>> 
>> Base upon patches from Roy Keene 
>> 
>> Currently qemuDomainSaveMemory can save vm's config
>> and memory to fd.
>> It writes a magic QEMU_SAVE_PARTIAL firstly,
>> then re-open it to change QEMU_SAVE_PARTIAL as QEMU_SAVE_MAGIC
>> after a success write.
>> 
>> For pipes this is not possible, attempting to re-open the pipe
>> will not connect you to the same consumer.
>> Seeking is also not possible on a pipe.
>> 
>> This patch introduce VIR_DOMAIN_SAVE_PIPE.
>> If set, write QEMU_SAVE_MAGIC directly.
>> Try to write a regular file with VIR_DOMAIN_SAVE_PIPE
>> is not supportted.
>> 
>> This is useful to me for saving a VM state directly to
>> Ceph RBD images without having an intermediate file.
>> 
>> Cc: Roy Keene 
>> Signed-off-by: Chen Hanxiao 
>> ---
>>  include/libvirt/libvirt-domain.h |  1 +
>>  src/qemu/qemu_driver.c   | 71 
>> ++--
>>  2 files changed, 54 insertions(+), 18 deletions(-)
>> 
>> diff --git a/include/libvirt/libvirt-domain.h 
>> b/include/libvirt/libvirt-domain.h
>> index a8435ab..c3e4c15 100644
>> --- a/include/libvirt/libvirt-domain.h
>> +++ b/include/libvirt/libvirt-domain.h
>> @@ -1169,6 +1169,7 @@ typedef enum {
>>  VIR_DOMAIN_SAVE_BYPASS_CACHE = 1 << 0, /* Avoid file system cache 
>> pollution */
>>  VIR_DOMAIN_SAVE_RUNNING  = 1 << 1, /* Favor running over paused */
>>  VIR_DOMAIN_SAVE_PAUSED   = 1 << 2, /* Favor paused over running */
>> +VIR_DOMAIN_SAVE_PIPE = 1 << 3, /* Output is a pipe */
>
>It doesn't have necessarily to be a pipe.

We need a flag to specify that
we need to write QEMU_SAVE_MAGIC directly.
Any suggestion for the name of this flag?

>
>>  } virDomainSaveRestoreFlags;
>>  
>>  int virDomainSave   (virDomainPtr domain,
[snip]
>> +
>> +/*
>> + * Determine if this file is a PIPE, which could not be reopen.
>> + */
>> +if (virFileExists(path)) {
>> +fd = qemuOpenFile(driver, vm, path, O_RDONLY | O_NONBLOCK, NULL, 
>> NULL);
>> +if (fd < 0)
>> +goto cleanup;
>> +if (fstat(fd, ) < 0)
>> +goto cleanup;
>> +if (S_ISFIFO(statbuf.st_mode)) {
>
>You should not try to check this. If the user wishes to write the
>complete header right away, then we should obey it and not have to check
>prior to do so.

My concern is that
when we write to a pipe/fifo, if no one read it, we will hang.
We should prevent from doing this
only when we specify a flag.

Regards,
- Chen
>
>> +if (flags & VIR_DOMAIN_SAVE_PIPE) {
>> +canReopen = false;
>> +} else {
>> +virReportSystemError(EINVAL, _("%s is not PIPE"), path);
>> +goto cleanup;
>> +}
>> +}
>> +VIR_FORCE_CLOSE(fd);
>> +}
>> +
>>  fd = qemuOpenFile(driver, vm, path,
>>O_WRONLY | O_TRUNC | O_CREAT | directFlag,
>>, );

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

Re: [libvirt] [PATCH v2] storage: vz storage pool support

[Nikolay Shirokovskiy]

On 08.12.2016 15:17, John Ferlan wrote:
> 
> 
> On 12/08/2016 04:19 AM, Maxim Nestratov wrote:
>> 08-Dec-16 02:22, John Ferlan пишет:
>>
>>> [...]
>>>
> I see what you mean; however, IMO vstorage should be separate. Maybe
> there's another opinion out there, but since you're requiring
> "something" else to be installed in order to get the WITH_VSTORAGE
> to be
> set to 1, then a separate file is in order.
>
> Not sure they're comparable, but zfs has its own. Having separated
> vstorage reduces the chance that some day some incompatible logic is
> added/altered in the *fs.c (or vice versa).
 Ok. I will try.

> I think you should consider the *_fs.c code to be the "default" of
> sorts. That is default file/dir structure with netfs added in. The
> vstorage may just be some file system, but it's not something (yet) on
> "every" distribution.
 I did not understand actually, what you mean  "be the "default" of
 sorts."
 As I have understood - what I need to do is to create backend_vstorage.c
 with all create/delete/* functionality.

>>> Sorry - I was trying to think of a better way to explain... The 'fs' and
>>> 'nfs' pool are default of sorts because one can "ls" (on UNIX/Linux) or
>>> "dir" (on Windows) and get a list of files.
>>>
>>> "ls" and "dir" are inherent to the OS, while in this case vstorage
>>> commands are installed separately.
>>
>> Once you mounted your vstorage cluster to a local filesystem you can
>> also "ls" it. Thus, I can't see much difference from nfs here.
>>
> 
> So if it's more like NFS, then how does one ensure that the local userid
> X is the same as the remote userid X? NFS has a root-squashing concept
> that results in numerous shall we say "interesting" issues.
> 
> Check out the virFileOpen*, virDirCreate, and virFileRemove...
> 
> Also what about viFileIsShareFSType? And security_selinux.c code for
> NFS? If you use cscope, just search on NFS.
> 
> In the virStorageBackendVzStart, I see:
> 
>VSTORAGE_MOUNT -c $pool.source.name $pool.target.path
> 
> where VSTORAGE_MOUNT is a build (configure.ac) definition that is the
> "Location or name of vstorage-mount program" which would only be set if
> the proper package was installed.
> 
> In the virStorageBackendVzfindPoolSources, I see:
> 
>VSTORAGE discover
> 
> which I assume generates some list of remote "services" (for lack of a
> better term) which can be used as/for pool.source.name in order to be
> well mounted by the VSTORAGE_MOUNT program.
> 
> Compare that to NFS, which uses mount which is included in well every
> distro I can think of. That's a big difference. Also let's face it, NFS
> has been the essential de facto goto tool to access remote storage for a
> long time. Personally, I'd rather see the NFS code split out of the
> *_fs.c backend, but I don't have the desire/time to do it - so it stays
> as is.

But netfs pool type is not only for NFS, it also can be used to provide
cifs and FUSE glusterfs mounts. These three just as vstorage have very
little difference from local filesystems from pool POV after they are 
mounted that's why I guess they implemented so tightly.

> 
>>> When you create a vstorage "file" is that done via touch? or edit some
>>> path or some other "common" OS command?  Or is there a vstorage command
>>> that needs to be used.  If the former, then using the common
>>> storage_backend API's should be possible.
>>
>> vstorage is just another "remote filesystem" type of distributed
>> software defined storage. In terms of starting to use it, it doesn't
>> differ from nfs - you should mount it and then you can use any POSIX
>> calls to control files and directories resided on it.
> 
> Here's a sample nfs pool xml I have - I changed the source/target path
> and didn't define a host.
> 
> 
>   nfs
>   0
>   0
>   0
>   
> 
> 
> 
>   
>   
> /path/to/target
> 
>   0700
>   0
>   0
> 
>   
> 
> 
> That is vastly different than what was in the cover:
> 
> 
>   VZ
>   5f45665b-66fa-4b18-84d1-248774cff3a1
>   107374182400
>   1441144832
>   105933037568
>   
> vz7-vzstorage
>   
>   
> /vzstorage_pool
> 
>   0700
>   0
>   0
> 
>   
> 
> 
> What causes "vz7-vzstorage" to be defined? Something from the 'VSTORAGE'
> command. I would think that is that essentially similar to how
> glusterfs, rbd, or sheepdog uses a source  field. Note that each
> of those include a  definition, although
> this vstorage XML doesn't.
> 
> Thus it seems vzstorage is really not a "local" filesystem, correct? If
> so, then should one really be allowing "local" things like chown, chmod,
> etc. to be executed? What kind of "configuration and/or validation of
> trust" takes place via vstorage provided tools in order to allow a user
> on the local host to access the storage on the remote host.
> 
> John
>>
>> Maxim
>>>
>>> John
>>>
>>>
> Also I forgot to mention yesterday - you need 

Re: [libvirt] [PATCH] lxc: monitor now holds a reference to the domain

[Michal Privoznik]

On 06.12.2016 15:39, Cédric Bosdonnat wrote:
> If the monitor doesn't hold a reference to the domain object
> the object may be destroyed before the monitor actually stops.
> ---
>  src/lxc/lxc_monitor.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/lxc/lxc_monitor.c b/src/lxc/lxc_monitor.c
> index d828d52..de63f9e 100644
> --- a/src/lxc/lxc_monitor.c
> +++ b/src/lxc/lxc_monitor.c
> @@ -179,6 +179,7 @@ virLXCMonitorPtr virLXCMonitorNew(virDomainObjPtr vm,
>  memcpy(>cb, cb, sizeof(mon->cb));
>  
>  virObjectRef(mon);
> +virObjectRef(vm);

You can move this a few lines above: mon->vm = virObjectRef(vm); if you
want.

>  virNetClientSetCloseCallback(mon->client, virLXCMonitorEOFNotify, mon,
>   virLXCMonitorCloseFreeCallback);
>  
> @@ -188,6 +189,7 @@ virLXCMonitorPtr virLXCMonitorNew(virDomainObjPtr vm,
>  
>   error:
>  virObjectUnref(mon);
> +virObjectUnref(vm);

This doesn't feel right. Imagine something bad happened before @vm got
ref'd (the first hunk). The control jumps over to error label and unref
@vm even though it hasn't been ref'd yet.
Or worse - @mon has exactly one reference (we are creating it here in
this function), therefore the above unref(mon) causes the MonitorDispose
callback to be called, which unrefs the @vm again. Fortunately, this
scenario is currently impossible as there's no 'goto error' after
@mon->vm is set, but you get my point.

>  mon = NULL;
>  goto cleanup;
>  }
> @@ -201,6 +203,7 @@ static void virLXCMonitorDispose(void *opaque)
>  if (mon->cb.destroy)
>  (mon->cb.destroy)(mon, mon->vm);
>  virObjectUnref(mon->program);
> +virObjectUnref(mon->vm);
>  }
>  
>  
> 

ACK if you drop the 2nd hunk.

Michal

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

Re: [libvirt] [PATCH 3/3] qemu: fix xml dump of autogenerated websocket

[Nikolay Shirokovskiy]

On 08.12.2016 16:21, John Ferlan wrote:
> 
> Perhaps a commit message would answer my question below...
> 
> On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
>> ---
>>  src/conf/domain_conf.c | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 6e008e2..fb6ff0b 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -22534,7 +22534,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
>>  virBufferAsprintf(buf, " autoport='%s'",
>>def->data.vnc.autoport ? "yes" : "no");
>>  
>> -if (def->data.vnc.websocket)
>> +if (def->data.vnc.websocketGenerated &&
> 
> Wouldn't websocketGenerated imply an active domain? And a change of the
> websocket in the active xml to be some value?
> 
> So is the purpose of this because if you have an active domain you don't
> want to display the websocket that was generated?
> 
> And why is that?
> 
> IOW: What are you trying to ensure with this patch?
> 

AFAIU this combination - active domain with inactive dump flag is used
to serialize config in situations described in cover letter - migration
or saving of a domain. So instead of saving port we save the fact it is 
autogenerated
and regenerate on migration destination/restore. One can infer this from 
socket port logic in near by code.

Nikolay

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

Re: [libvirt] [PATCH v3 4/4] domain_conf: autodetect vhostuser ifname

[Mehdi Abaakouk]

On Thu, Dec 08, 2016 at 01:51:10PM +0100, Michal Privoznik wrote:

On 18.11.2016 23:51, Mehdi Abaakouk wrote:

From: Mehdi Abaakouk 


I don't think this is a good idea. For instance, for the following XML:

 
   
   
   
 

this code would produce ifname of "vhost0.sock", which is obviously
wrong. Moreover, tests are failing with this change. Not only that, for
auto-filling values in XML we have so called post parse callbacks.
Historically we put everything into XML parsing code and it ended up
being this one big mess. So we worked hard to split it and although we
are not there 100%, we are getting there slowly.


I agree I have proposed that to start a discussion, but I don't really
like it. It work in my case because openvswitch vhostuser unix-socket path
is hardcoded to /var/run/openvswitch/. But if you
vhostuser unix-socket have been created outside openvswitch that
obviously doesn't pick a good name, and it's even not really useful to
set a default name.

Perhaps should I move this logic to virnetdevopenvswitch.c.
I means virNetDevOpenvswitchInterfaceStats() should take a look to the
source path when the ifname is unset, WDT ?



 vhostuser_path = NULL;

 if (STREQ(vhostuser_mode, "server")) {
@@ -9842,6 +9851,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
 VIR_FREE(localaddr);
 VIR_FREE(localport);
 virNWFilterHashTableFree(filterparams);
+virStringFreeListCount(tokens, ntokens);



Ah, due to me not reviewing the patches early, this function was renamed
in c2a5a4e7ea930.



 return def;




Michal

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


--
Mehdi Abaakouk
mail: sil...@sileht.net
irc: sileht

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

Re: [libvirt] [PATCH 2/3] qemu: mark user defined websocket as used

[Nikolay Shirokovskiy]

On 08.12.2016 16:21, John Ferlan wrote:
> 
> 
> On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
>> We need extra state variable to distinguish between autogenerated
>> and user defined cases after auto generation is done.
>> ---
>>  src/conf/domain_conf.h  |  1 +
>>  src/qemu/qemu_process.c | 19 +--
>>  2 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 541b600..9bc4522 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -1468,6 +1468,7 @@ struct _virDomainGraphicsDef {
>>  int port;
>>  bool portReserved;
>>  int websocket;
>> +bool websocketGenerated;
>>  bool autoport;
>>  char *keymap;
>>  virDomainGraphicsAuthDef auth;
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 610..1799f33 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -3574,6 +3574,7 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver,
>>  if (virPortAllocatorAcquire(driver->webSocketPorts, ) < 0)
>>  return -1;
>>  graphics->data.vnc.websocket = port;
>> +graphics->data.vnc.websocketGenerated = true;
>>  }
>>  
>>  return 0;
>> @@ -4065,6 +4066,12 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr 
>> driver,
>>  return -1;
>>  graphics->data.vnc.portReserved = true;
>>  }
>> +if (graphics->data.vnc.websocket != -1 &&   /* auto websocket */
>> +graphics->data.vnc.websocket && /* no websocket */
> 
> Some would prefer no comments because the logic should be self
> explanatory.  IDC, but would rather see the comment before rather than
> within the "if" statement.
> 
> In any case, why isn't this just:
> 
> if (graphics->data.vnc.websocket > 0) {

This is better of course )

> 
> note: no comments.
> 
> IOW: If a user defined a specific port, set that in the remotePorts.
> 
> ACK in general - I can adjust the check before pushing based on your
> feedback. I could also wait for a v2 if you want to create an Unreserve
> helper with switch/case too.
> 

So I'm on with you change.

Nikolay

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

Re: [libvirt] [PATCH] conf: Make scheduler formatting simpler

[John Ferlan]

On 11/22/2016 08:53 AM, Martin Kletzander wrote:
> Since the great rework of how we store vcpu- and iothread-related
> data, we have overly complex part of code that is trying to format the
> scheduler tuning data in as less lines as possible by grouping
> settings for multiple threads.  That was designed as an input syntax
> sugar for users, but we don't need to also use that when formatting
> the XML.  Switching to simple enumeration makes the code nicer,
> shorter and more welcoming to future changes.
> 
> Signed-off-by: Martin Kletzander 
> ---
>  src/conf/domain_conf.c | 209 
> -
>  ...l2xmlout-cputune-iothreadsched-zeropriority.xml |   7 +-
>  .../qemuxml2xmlout-cputune-iothreadsched.xml   |   7 +-
>  3 files changed, 43 insertions(+), 180 deletions(-)
> 

Yes it certainly does make it easier to read...

One suggestion - change the name of the function to
"virDomainSchedulerFormat" since it makes it easier to find
virDomainSchedulerParse

ACK

John

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

Re: [libvirt] [PATCH 1/3] qemu: refactor: use switch for enum in qemuProcessGraphicsReservePorts

[Nikolay Shirokovskiy]

On 08.12.2016 16:21, John Ferlan wrote:
> 
> 
> On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
>> ---
>>  src/qemu/qemu_process.c | 30 +-
>>  1 file changed, 21 insertions(+), 9 deletions(-)
>>
> 
> ACK - although I'll reword commit a bit:
> 
> qemu: Refactor qemuProcessGraphicsReservePorts
> 
> Use switch for enums rather than if/else conditions.

Ok.

> 
> FWIW: After reading patch 2, why not alter the code in qemuProcessStop
> in order to have a switch too?  In fact that code could be split out
> into a qemuProcessGraphicsUnreservePorts function.  If you want to make
> a v2 to do then, then let me know.

I'd better offload the change you suggest to a different series/patch.

This patch on the other hand is correlated with next one - you can not add 
extra vnc logic
to qemuProcessGraphicsReservePorts without changing its overall control flow.
So does this patch.

Nikolay

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

Re: [libvirt] [PATCH v2] storage: vz storage pool support

[Maxim Nestratov]

08-Dec-16 15:17, John Ferlan пишет:



On 12/08/2016 04:19 AM, Maxim Nestratov wrote:

08-Dec-16 02:22, John Ferlan пишет:


[...]


I see what you mean; however, IMO vstorage should be separate. Maybe
there's another opinion out there, but since you're requiring
"something" else to be installed in order to get the WITH_VSTORAGE
to be
set to 1, then a separate file is in order.

Not sure they're comparable, but zfs has its own. Having separated
vstorage reduces the chance that some day some incompatible logic is
added/altered in the *fs.c (or vice versa).

Ok. I will try.


I think you should consider the *_fs.c code to be the "default" of
sorts. That is default file/dir structure with netfs added in. The
vstorage may just be some file system, but it's not something (yet) on
"every" distribution.

I did not understand actually, what you mean  "be the "default" of
sorts."
As I have understood - what I need to do is to create backend_vstorage.c
with all create/delete/* functionality.


Sorry - I was trying to think of a better way to explain... The 'fs' and
'nfs' pool are default of sorts because one can "ls" (on UNIX/Linux) or
"dir" (on Windows) and get a list of files.

"ls" and "dir" are inherent to the OS, while in this case vstorage
commands are installed separately.

Once you mounted your vstorage cluster to a local filesystem you can
also "ls" it. Thus, I can't see much difference from nfs here.


So if it's more like NFS, then how does one ensure that the local userid
X is the same as the remote userid X? NFS has a root-squashing concept
that results in numerous shall we say "interesting" issues.


Vstorage doesn't have users concept. Authentication is made by a password per 
node just once.
If authentication passes, a key is stored in 
/etc/vstorage/clusters/CLUSTER_NAME/auth_digest.key
Then, permissions are set to a mount point during mounting with -u USER -g 
GROUP -m  MODE options
provided to vstorage-mount command.


Check out the virFileOpen*, virDirCreate, and virFileRemove...

Also what about viFileIsShareFSType? And security_selinux.c code for
NFS? If you use cscope, just search on NFS.

In the virStorageBackendVzStart, I see:

VSTORAGE_MOUNT -c $pool.source.name $pool.target.path


This call certainly lacks user/group/mode parameters and should be fixed in the 
next series.



where VSTORAGE_MOUNT is a build (configure.ac) definition that is the
"Location or name of vstorage-mount program" which would only be set if
the proper package was installed.

In the virStorageBackendVzfindPoolSources, I see:

VSTORAGE discover

which I assume generates some list of remote "services" (for lack of a
better term) which can be used as/for pool.source.name in order to be
well mounted by the VSTORAGE_MOUNT program.

Compare that to NFS, which uses mount which is included in well every
distro I can think of. That's a big difference. Also let's face it, NFS
has been the essential de facto goto tool to access remote storage for a
long time. Personally, I'd rather see the NFS code split out of the
*_fs.c backend, but I don't have the desire/time to do it - so it stays
as is.


To sum this up, you still think that copy and paste isn't a problem here and 
will create more value than do any harm, right?

Maxim

[snip]

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

[libvirt] [PATCH 2/2] cgroup: reduce complexity of controller disabling

[Boris Fiuczynski]

This patch reduces the complexity of the filtering algorithm in
virCgroupDetect by first correcting the controller mask and then
checking for potential co-mounts without any correlating
controller mask modifications.

If you agree that this patch removes complexity and improves
readability it could simply be squashed into the first patch
of this series.

Signed-off-by: Boris Fiuczynski 
Reviewed-by: Bjoern Walk 
Reviewed-by: Marc Hartmayer 
---
 src/util/vircgroup.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 47691e2..80ce43c 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -656,11 +656,8 @@ virCgroupDetect(virCgroupPtr group,
 
 if (controllers >= 0) {
 VIR_DEBUG("Filtering controllers %d", controllers);
+/* First mark requested but non-existing controllers to be ignored */
 for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) {
-VIR_DEBUG("Controller '%s' wanted=%s, mount='%s'",
-  virCgroupControllerTypeToString(i),
-  (1 << i) & controllers ? "yes" : "no",
-  NULLSTR(group->controllers[i].mountPoint));
 if (((1 << i) & controllers)) {
 /* Remove non-existent controllers  */
 if (!group->controllers[i].mountPoint) {
@@ -668,9 +665,15 @@ virCgroupDetect(virCgroupPtr group,
   virCgroupControllerTypeToString(i));
 controllers &= ~(1 << i);
 }
-} else {
-if (!group->controllers[i].mountPoint)
-continue; /* without controller co-mounting is impossible 
*/
+}
+}
+for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) {
+VIR_DEBUG("Controller '%s' wanted=%s, mount='%s'",
+  virCgroupControllerTypeToString(i),
+  (1 << i) & controllers ? "yes" : "no",
+  NULLSTR(group->controllers[i].mountPoint));
+if (!((1 << i) & controllers) &&
+group->controllers[i].mountPoint) {
 /* Check whether a request to disable a controller
  * clashes with co-mounting of controllers */
 for (j = 0; j < VIR_CGROUP_CONTROLLER_LAST; j++) {
-- 
2.5.5

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

[libvirt] [PATCH 0/2] cgroup: unavailable controller prevents controller disabling

[Boris Fiuczynski]

The problem description is covered in patch one.
I added patch two as an optional enhancement removing some complexity and
improving readability. If this finds common agreement it should simply be
squashed into the first patch of this series.

Boris Fiuczynski (2):
  cgroup: unavailable controller prevents controller disabling
  cgroup: reduce complexity of controller disabling

 src/util/vircgroup.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

-- 
2.5.5

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

[libvirt] [PATCH 1/2] cgroup: unavailable controller prevents controller disabling

[Boris Fiuczynski]

The cgroup controller filtering in virCgroupDetect does not work
properly if the following conditions are met:
1) the host system does not have a cgroup controller which
libvirt requests (unavailable controller) and
2) libvirt is configured to disable a controller (disabled controller) and
3) the disabled controller is located before the unavailable controller
in virCgroupController.

As an example: The memory controller is unavailable and the cpuset
controller is configured to be disabled.
In this scenario trying to start a domain results in the error
error: Controller 'cpuset' is not wanted, but 'memory' is co-mounted: Invalid 
argument

This error occurs when virCgroupDetect is called with a valid parent group.
The resulting group created by virCgroupCopyMounts holds for cpuset and
memory controller empty mount points. The filtering of disabled controllers
checks for co-mounts by comparing the mount points. The cpuset controller
causes the filtering to occur before the memory controller is marked as to be
ignored by modifying the controller mask since it is unavailable.
Therefore the co-mount detection logic compares the cpuset and memory controller
mount points and since both are empty the memory controller is regarded
erroneously as being co-mounted.

Signed-off-by: Boris Fiuczynski 
Reviewed-by: Marc Hartmayer 
Reviewed-by: Bjoern Walk 
---
 src/util/vircgroup.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index b6affe3..47691e2 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -669,6 +669,8 @@ virCgroupDetect(virCgroupPtr group,
 controllers &= ~(1 << i);
 }
 } else {
+if (!group->controllers[i].mountPoint)
+continue; /* without controller co-mounting is impossible 
*/
 /* Check whether a request to disable a controller
  * clashes with co-mounting of controllers */
 for (j = 0; j < VIR_CGROUP_CONTROLLER_LAST; j++) {
-- 
2.5.5

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

Re: [libvirt] [PATCH 2/3] qemu: mark user defined websocket as used

[John Ferlan]

On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
> We need extra state variable to distinguish between autogenerated
> and user defined cases after auto generation is done.
> ---
>  src/conf/domain_conf.h  |  1 +
>  src/qemu/qemu_process.c | 19 +--
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 541b600..9bc4522 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1468,6 +1468,7 @@ struct _virDomainGraphicsDef {
>  int port;
>  bool portReserved;
>  int websocket;
> +bool websocketGenerated;
>  bool autoport;
>  char *keymap;
>  virDomainGraphicsAuthDef auth;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 610..1799f33 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3574,6 +3574,7 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver,
>  if (virPortAllocatorAcquire(driver->webSocketPorts, ) < 0)
>  return -1;
>  graphics->data.vnc.websocket = port;
> +graphics->data.vnc.websocketGenerated = true;
>  }
>  
>  return 0;
> @@ -4065,6 +4066,12 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr 
> driver,
>  return -1;
>  graphics->data.vnc.portReserved = true;
>  }
> +if (graphics->data.vnc.websocket != -1 &&   /* auto websocket */
> +graphics->data.vnc.websocket && /* no websocket */

Some would prefer no comments because the logic should be self
explanatory.  IDC, but would rather see the comment before rather than
within the "if" statement.

In any case, why isn't this just:

if (graphics->data.vnc.websocket > 0) {

note: no comments.

IOW: If a user defined a specific port, set that in the remotePorts.

ACK in general - I can adjust the check before pushing based on your
feedback. I could also wait for a v2 if you want to create an Unreserve
helper with switch/case too.

John

> +virPortAllocatorSetUsed(driver->remotePorts,
> +graphics->data.vnc.websocket,
> +true) < 0)
> +return -1;
>  break;
>  
>  case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
> @@ -6189,8 +6196,16 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>  false);
>  graphics->data.vnc.portReserved = false;
>  }
> -virPortAllocatorRelease(driver->webSocketPorts,
> -graphics->data.vnc.websocket);
> +if (graphics->data.vnc.websocketGenerated) {
> +virPortAllocatorRelease(driver->webSocketPorts,
> +graphics->data.vnc.websocket);
> +graphics->data.vnc.websocketGenerated = false;
> +graphics->data.vnc.websocket = -1;
> +} else if (graphics->data.vnc.websocket) {
> +virPortAllocatorSetUsed(driver->remotePorts,
> +graphics->data.vnc.websocket,
> +false);
> +}
>  }
>  if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
>  if (graphics->data.spice.autoport) {
> 

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

Re: [libvirt] [PATCH 1/3] qemu: refactor: use switch for enum in qemuProcessGraphicsReservePorts

[John Ferlan]

On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
> ---
>  src/qemu/qemu_process.c | 30 +-
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 

ACK - although I'll reword commit a bit:

qemu: Refactor qemuProcessGraphicsReservePorts

Use switch for enums rather than if/else conditions.

John

FWIW: After reading patch 2, why not alter the code in qemuProcessStop
in order to have a switch too?  In fact that code could be split out
into a qemuProcessGraphicsUnreservePorts function.  If you want to make
a v2 to do then, then let me know.

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

Re: [libvirt] [PATCH 3/3] qemu: fix xml dump of autogenerated websocket

[John Ferlan]

Perhaps a commit message would answer my question below...

On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
> ---
>  src/conf/domain_conf.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6e008e2..fb6ff0b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -22534,7 +22534,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
>  virBufferAsprintf(buf, " autoport='%s'",
>def->data.vnc.autoport ? "yes" : "no");
>  
> -if (def->data.vnc.websocket)
> +if (def->data.vnc.websocketGenerated &&

Wouldn't websocketGenerated imply an active domain? And a change of the
websocket in the active xml to be some value?

So is the purpose of this because if you have an active domain you don't
want to display the websocket that was generated?

And why is that?

IOW: What are you trying to ensure with this patch?

John
> +(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE))
> +virBufferAddLit(buf, " websocket='-1'");
> +else if (def->data.vnc.websocket)
>  virBufferAsprintf(buf, " websocket='%d'", 
> def->data.vnc.websocket);
>  
>  virDomainGraphicsListenDefFormatAddr(buf, glisten, flags);
> 

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

Re: [libvirt] [PATCH] node_device: Check return value for udev_new()

[Boris Fiuczynski]

On 12/08/2016 01:48 PM, Martin Kletzander wrote:

On Thu, Dec 01, 2016 at 01:52:03PM +0100, Marc Hartmayer wrote:

On Wed, Nov 30, 2016 at 04:25 PM +0100, Martin Kletzander
 wrote:

On Wed, Nov 30, 2016 at 01:45:32PM +0100, Marc Hartmayer wrote:

The comment was actually wrong as
https://www.freedesktop.org/software/systemd/man/udev_new.html
mentions that on failure NULL is returned.

Signed-off-by: Marc Hartmayer 
Reviewed-by: Bjoern Walk 
Reviewed-by: Boris Fiuczynski 
---
src/node_device/node_device_udev.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 4b81312..4b0a875 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1491,13 +1491,11 @@ static int nodeStateInitialize(bool privileged,
if (udevPCITranslateInit(privileged) < 0)
goto cleanup;

-/*
- *
http://www.kernel.org/pub/linux/utils/kernel/hotplug/libudev/libudev-udev.html#udev-new

- *
- * indicates no return value other than success, so we don't check
- * its return value.
- */
udev = udev_new();
+if (!udev) {
+virReportOOMError();
+goto cleanup;
+}


Is that true for other udevs and not just systemd-udev?


I'm not sure about other versions of udev but the NULL pointer is already
handled in udevStatInitialize() for udev_new() in a likewise manner.


Does it really mean just an OOM error?


It fails for systemd-udev if malloc/calloc fails => this is most
likely a OOM error at this point.


Couldn't we add a proper error message?


In udevStateInitialize() the error handling reports an internal error
but as the original error is caused by OOM I think we have to use
virReportOOMError().



OK, it doesn't make sense for it to return NULL anyway, so I'm OK with
that, but I'd rather use internal error as we're not completely sure all
udev implementations can fail only due to not enough memory.  Plus the
internal error will give more information in the logs.

Martin
Shouldn't the OOM error method already to be used if only one udev 
implementation could fail due to not enough memory?






--
Beste Grüße / Kind regards
  Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294




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




--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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

Re: [libvirt] [PATCH v3 1/4] Gathering vhostuser interface stats with ovs

[Michal Privoznik]

On 18.11.2016 23:51, Mehdi Abaakouk wrote:
> From: Mehdi Abaakouk 
> 
> When vhostuser interfaces are used, the interface statistics
> are not available in /proc/net/dev.
> 
> This change looks at the openvswitch interfaces statistics
> tables to provide this information for vhostuser interface.
> 
> Note that in openvswitch world drop/error doesn't always make sense
> for some interface type. When these informations are not available we
> set them to 0 on the virDomainInterfaceStats.
> ---
>  src/libvirt_private.syms|   1 +
>  src/qemu/qemu_driver.c  |  29 ---
>  src/util/virnetdevopenvswitch.c | 106 
> 
>  src/util/virnetdevopenvswitch.h |   4 ++
>  4 files changed, 133 insertions(+), 7 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index baff82b..aa27f78 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2034,6 +2034,7 @@ virNetDevMidonetUnbindPort;
>  # util/virnetdevopenvswitch.h
>  virNetDevOpenvswitchAddPort;
>  virNetDevOpenvswitchGetMigrateData;
> +virNetDevOpenvswitchInterfaceStats;
>  virNetDevOpenvswitchRemovePort;
>  virNetDevOpenvswitchSetMigrateData;
>  
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d039255..87ca09d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -66,6 +66,7 @@
>  #include "virhostcpu.h"
>  #include "virhostmem.h"
>  #include "virstats.h"
> +#include "virnetdevopenvswitch.h"
>  #include "capabilities.h"
>  #include "viralloc.h"
>  #include "viruuid.h"
> @@ -10975,6 +10976,7 @@ qemuDomainInterfaceStats(virDomainPtr dom,
>   virDomainInterfaceStatsPtr stats)
>  {
>  virDomainObjPtr vm;
> +virDomainNetDefPtr net = NULL;
>  size_t i;
>  int ret = -1;
>  
> @@ -10994,16 +10996,21 @@ qemuDomainInterfaceStats(virDomainPtr dom,
>  for (i = 0; i < vm->def->nnets; i++) {
>  if (vm->def->nets[i]->ifname &&
>  STREQ(vm->def->nets[i]->ifname, path)) {
> -ret = 0;
> +net = vm->def->nets[i];
>  break;
>  }
>  }
>  
> -if (ret == 0)
> -ret = virNetInterfaceStats(path, stats);
> -else
> +if (net) {
> +if (net->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
> +ret = virNetDevOpenvswitchInterfaceStats(path, stats);
> +} else {
> +ret = virNetInterfaceStats(path, stats);
> +}
> +} else {
>  virReportError(VIR_ERR_INVALID_ARG,
> _("invalid path, '%s' is not a known interface"), 
> path);
> +}
>  

Oh my. Not your fault but this looks ugly. It has even before you've
touched it.

BTW: maybe I am misreading something but my understanding of vhost-user
is that it can be plugged into any type of bridge (e.g. snabb). How does
this work then if we run ovs-vsctl then? Do you perhaps have a set of
steps how can I test this feature? Because so far I've used
vhost-user-bridge helper from qemu repo but this will not work with it.

>   cleanup:
>  virDomainObjEndAPI();
> @@ -19140,9 +19147,17 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver 
> ATTRIBUTE_UNUSED,
>  QEMU_ADD_NAME_PARAM(record, maxparams,
>  "net", "name", i, dom->def->nets[i]->ifname);
>  
> -if (virNetInterfaceStats(dom->def->nets[i]->ifname, ) < 0) {
> -virResetLastError();
> -continue;
> +if (dom->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
> +if (virNetDevOpenvswitchInterfaceStats(dom->def->nets[i]->ifname,
> +   ) < 0) {
> +virResetLastError();
> +continue;
> +}
> +} else {
> +if (virNetInterfaceStats(dom->def->nets[i]->ifname, ) < 0) {
> +virResetLastError();
> +continue;
> +}
>  }
>  
>  QEMU_ADD_NET_PARAM(record, maxparams, i,
> diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> index 9283bbb..db8b542 100644
> --- a/src/util/virnetdevopenvswitch.c
> +++ b/src/util/virnetdevopenvswitch.c
> @@ -24,6 +24,8 @@
>  
>  #include 
>  
> +#include 
> +
>  #include "virnetdevopenvswitch.h"
>  #include "vircommand.h"
>  #include "viralloc.h"
> @@ -270,3 +272,107 @@ int virNetDevOpenvswitchSetMigrateData(char *migrate, 
> const char *ifname)
>  virCommandFree(cmd);
>  return ret;
>  }
> +
> +/**
> + * virNetDevOpenvswitchInterfaceStats:
> + * @ifname: the name of the interface
> + * @stats: the retreived domain interface stat
> + *
> + * Retrieves the OVS interfaces stats
> + *
> + * Returns 0 in case of success or -1 in case of failure
> + */
> +int
> +virNetDevOpenvswitchInterfaceStats(const char *ifname,
> +   virDomainInterfaceStatsPtr stats)
> +{
> +virCommandPtr cmd = NULL;
> +char 

Re: [libvirt] [PATCH v3 4/4] domain_conf: autodetect vhostuser ifname

[Michal Privoznik]

On 18.11.2016 23:51, Mehdi Abaakouk wrote:
> From: Mehdi Abaakouk 
> 
> This change puts the socket filename as ifname for vhostuser netwok
> interface.
> 
> The filename is the name of the openvswitch interface, this allows the
> qemuDomainInterfaceStats to work out of the box without having to
> manually set the ifname.
> ---
>  src/conf/domain_conf.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6e008e2..0f91ab3 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -9098,6 +9098,8 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>  virDomainActualNetDefPtr actual = NULL;
>  xmlNodePtr oldnode = ctxt->node;
>  int ret, val;
> +char **tokens = NULL;
> +size_t ntokens = 0;
>  
>  if (VIR_ALLOC(def) < 0)
>  return NULL;
> @@ -9394,6 +9396,13 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>  
>  def->data.vhostuser->type = VIR_DOMAIN_CHR_TYPE_UNIX;
>  def->data.vhostuser->data.nix.path = vhostuser_path;
> +
> +if (!ifname && (tokens = virStringSplitCount(vhostuser_path, "/", 0,
> + ))) {
> +if (VIR_STRDUP(ifname, tokens[ntokens-1]) < 0)
> +goto error;
> +}
> +

I don't think this is a good idea. For instance, for the following XML:

  



  

this code would produce ifname of "vhost0.sock", which is obviously
wrong. Moreover, tests are failing with this change. Not only that, for
auto-filling values in XML we have so called post parse callbacks.
Historically we put everything into XML parsing code and it ended up
being this one big mess. So we worked hard to split it and although we
are not there 100%, we are getting there slowly.

>  vhostuser_path = NULL;
>  
>  if (STREQ(vhostuser_mode, "server")) {
> @@ -9842,6 +9851,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>  VIR_FREE(localaddr);
>  VIR_FREE(localport);
>  virNWFilterHashTableFree(filterparams);
> +virStringFreeListCount(tokens, ntokens);


Ah, due to me not reviewing the patches early, this function was renamed
in c2a5a4e7ea930.

>  
>  return def;
>  
> 

Michal

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

Re: [libvirt] [PATCH v3 3/4] Move virstat.c code to virnetdevtap.c

[Michal Privoznik]

On 18.11.2016 23:51, Mehdi Abaakouk wrote:
> From: Mehdi Abaakouk 
> 
> This is just a code move of virstat.c to virnetdevtap.c
> ---
>  po/POTFILES.in |   1 -
>  src/Makefile.am|   1 -
>  src/libvirt_private.syms   |   4 +-
>  src/libxl/libxl_driver.c   |   2 +-
>  src/lxc/lxc_driver.c   |   1 -
>  src/openvz/openvz_driver.c |   2 +-
>  src/qemu/qemu_driver.c |   2 +-
>  src/uml/uml_driver.c   |   1 -
>  src/util/virnetdevtap.c| 143 
>  src/util/virnetdevtap.h|   3 +
>  src/util/virstats.c| 178 
> -
>  src/util/virstats.h|  32 
>  src/xen/xen_hypervisor.c   |   2 +-
>  13 files changed, 151 insertions(+), 221 deletions(-)
>  delete mode 100644 src/util/virstats.c
>  delete mode 100644 src/util/virstats.h

ACK

Michal

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

Re: [libvirt] [PATCH v3 2/4] virstat: fix signature of virstat helper

[Michal Privoznik]

On 18.11.2016 23:51, Mehdi Abaakouk wrote:
> From: Mehdi Abaakouk 
> 
> In preparation to the code move to virnetdevtap.c, this change:
> 
> * renames virNetInterfaceStats to virNetDevTapInterfaceStats
> * changes 'path' to 'ifname', to use the same vocable as other
>   method in virnetdevtap.c.
> * Add the attributes checker
> ---
>  src/libvirt_private.syms   |  2 +-
>  src/libxl/libxl_driver.c   |  2 +-
>  src/lxc/lxc_driver.c   |  2 +-
>  src/openvz/openvz_driver.c |  2 +-
>  src/qemu/qemu_driver.c |  4 ++--
>  src/util/virstats.c| 22 +++---
>  src/util/virstats.h|  5 +++--
>  src/xen/xen_hypervisor.c   |  2 +-
>  8 files changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index aa27f78..0036cbd 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2367,7 +2367,7 @@ virSocketAddrSetIPv6AddrNetOrder;
>  virSocketAddrSetPort;
>  
>  # util/virstats.h
> -virNetInterfaceStats;
> +virNetDevTapInterfaceStats;
>  
>  # util/virstorageencryption.h
>  virStorageEncryptionFormat;
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index b2f3b16..67f0e58 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -4982,7 +4982,7 @@ libxlDomainInterfaceStats(virDomainPtr dom,
>  }
>  
>  if (ret == 0)
> -ret = virNetInterfaceStats(path, stats);
> +ret = virNetDevTapInterfaceStats(path, stats);
>  else
>  virReportError(VIR_ERR_INVALID_ARG,
> _("'%s' is not a known interface"), path);
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 4a0165a..526d40d 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -2893,7 +2893,7 @@ lxcDomainInterfaceStats(virDomainPtr dom,
>  }
>  
>  if (ret == 0)
> -ret = virNetInterfaceStats(path, stats);
> +ret = virNetDevTapInterfaceStats(path, stats);
>  else
>  virReportError(VIR_ERR_INVALID_ARG,
> _("Invalid path, '%s' is not a known interface"), 
> path);
> diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
> index 38a562e..7bd3acf 100644
> --- a/src/openvz/openvz_driver.c
> +++ b/src/openvz/openvz_driver.c
> @@ -2024,7 +2024,7 @@ openvzDomainInterfaceStats(virDomainPtr dom,
>  }
>  
>  if (ret == 0)
> -ret = virNetInterfaceStats(path, stats);
> +ret = virNetDevTapInterfaceStats(path, stats);
>  else
>  virReportError(VIR_ERR_INVALID_ARG,
> _("invalid path, '%s' is not a known interface"), 
> path);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 87ca09d..38208b1 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -11005,7 +11005,7 @@ qemuDomainInterfaceStats(virDomainPtr dom,
>  if (net->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
>  ret = virNetDevOpenvswitchInterfaceStats(path, stats);
>  } else {
> -ret = virNetInterfaceStats(path, stats);
> +ret = virNetDevTapInterfaceStats(path, stats);
>  }
>  } else {
>  virReportError(VIR_ERR_INVALID_ARG,
> @@ -19154,7 +19154,7 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver 
> ATTRIBUTE_UNUSED,
>  continue;
>  }
>  } else {
> -if (virNetInterfaceStats(dom->def->nets[i]->ifname, ) < 0) {
> +if (virNetDevTapInterfaceStats(dom->def->nets[i]->ifname, ) 
> < 0) {
>  virResetLastError();
>  continue;
>  }
> diff --git a/src/util/virstats.c b/src/util/virstats.c
> index c4725ed..95b4c38 100644
> --- a/src/util/virstats.c
> +++ b/src/util/virstats.c
> @@ -50,10 +50,10 @@
>   */
>  #ifdef __linux__
>  int
> -virNetInterfaceStats(const char *path,
> - virDomainInterfaceStatsPtr stats)
> +virNetDevTapInterfaceStats(const char *ifname,
> +   virDomainInterfaceStatsPtr stats)
>  {
> -int path_len;
> +int ifname_len;
>  FILE *fp;
>  char line[256], *colon;
>  
> @@ -64,7 +64,7 @@ virNetInterfaceStats(const char *path,
>  return -1;
>  }
>  
> -path_len = strlen(path);
> +ifname_len = strlen(ifname);
>  
>  while (fgets(line, sizeof(line), fp)) {
>  long long dummy;
> @@ -84,8 +84,8 @@ virNetInterfaceStats(const char *path,
>  colon = strchr(line, ':');
>  if (!colon) continue;
>  *colon = '\0';
> -if (colon-path_len >= line &&
> -STREQ(colon-path_len, path)) {
> +if (colon-ifname_len >= line &&
> +STREQ(colon-ifname_len, ifname)) {

While touching this you can fix the spaces around '-' sign.

>  /* IMPORTANT NOTE!
>   * /proc/net/dev vif.nn sees the network from the point
>   * of view of dom0 / hypervisor.  So bytes TRANSMITTED by dom0

Re: [libvirt] [PATCH] node_device: Check return value for udev_new()

[Martin Kletzander]

On Thu, Dec 01, 2016 at 01:52:03PM +0100, Marc Hartmayer wrote:

On Wed, Nov 30, 2016 at 04:25 PM +0100, Martin Kletzander  
wrote:

On Wed, Nov 30, 2016 at 01:45:32PM +0100, Marc Hartmayer wrote:

The comment was actually wrong as
https://www.freedesktop.org/software/systemd/man/udev_new.html
mentions that on failure NULL is returned.

Signed-off-by: Marc Hartmayer 
Reviewed-by: Bjoern Walk 
Reviewed-by: Boris Fiuczynski 
---
src/node_device/node_device_udev.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index 4b81312..4b0a875 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1491,13 +1491,11 @@ static int nodeStateInitialize(bool privileged,
if (udevPCITranslateInit(privileged) < 0)
goto cleanup;

-/*
- * 
http://www.kernel.org/pub/linux/utils/kernel/hotplug/libudev/libudev-udev.html#udev-new
- *
- * indicates no return value other than success, so we don't check
- * its return value.
- */
udev = udev_new();
+if (!udev) {
+virReportOOMError();
+goto cleanup;
+}


Is that true for other udevs and not just systemd-udev?


I'm not sure about other versions of udev but the NULL pointer is already
handled in udevStatInitialize() for udev_new() in a likewise manner.


Does it really mean just an OOM error?


It fails for systemd-udev if malloc/calloc fails => this is most
likely a OOM error at this point.


Couldn't we add a proper error message?


In udevStateInitialize() the error handling reports an internal error
but as the original error is caused by OOM I think we have to use
virReportOOMError().



OK, it doesn't make sense for it to return NULL anyway, so I'm OK with
that, but I'd rather use internal error as we're not completely sure all
udev implementations can fail only due to not enough memory.  Plus the
internal error will give more information in the logs.

Martin



--
Beste Grüße / Kind regards
  Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294



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

Re: [libvirt] [PATCH v2] storage: vz storage pool support

[John Ferlan]

On 12/08/2016 04:19 AM, Maxim Nestratov wrote:
> 08-Dec-16 02:22, John Ferlan пишет:
> 
>> [...]
>>
 I see what you mean; however, IMO vstorage should be separate. Maybe
 there's another opinion out there, but since you're requiring
 "something" else to be installed in order to get the WITH_VSTORAGE
 to be
 set to 1, then a separate file is in order.

 Not sure they're comparable, but zfs has its own. Having separated
 vstorage reduces the chance that some day some incompatible logic is
 added/altered in the *fs.c (or vice versa).
>>> Ok. I will try.
>>>
 I think you should consider the *_fs.c code to be the "default" of
 sorts. That is default file/dir structure with netfs added in. The
 vstorage may just be some file system, but it's not something (yet) on
 "every" distribution.
>>> I did not understand actually, what you mean  "be the "default" of
>>> sorts."
>>> As I have understood - what I need to do is to create backend_vstorage.c
>>> with all create/delete/* functionality.
>>>
>> Sorry - I was trying to think of a better way to explain... The 'fs' and
>> 'nfs' pool are default of sorts because one can "ls" (on UNIX/Linux) or
>> "dir" (on Windows) and get a list of files.
>>
>> "ls" and "dir" are inherent to the OS, while in this case vstorage
>> commands are installed separately.
> 
> Once you mounted your vstorage cluster to a local filesystem you can
> also "ls" it. Thus, I can't see much difference from nfs here.
> 

So if it's more like NFS, then how does one ensure that the local userid
X is the same as the remote userid X? NFS has a root-squashing concept
that results in numerous shall we say "interesting" issues.

Check out the virFileOpen*, virDirCreate, and virFileRemove...

Also what about viFileIsShareFSType? And security_selinux.c code for
NFS? If you use cscope, just search on NFS.

In the virStorageBackendVzStart, I see:

   VSTORAGE_MOUNT -c $pool.source.name $pool.target.path

where VSTORAGE_MOUNT is a build (configure.ac) definition that is the
"Location or name of vstorage-mount program" which would only be set if
the proper package was installed.

In the virStorageBackendVzfindPoolSources, I see:

   VSTORAGE discover

which I assume generates some list of remote "services" (for lack of a
better term) which can be used as/for pool.source.name in order to be
well mounted by the VSTORAGE_MOUNT program.

Compare that to NFS, which uses mount which is included in well every
distro I can think of. That's a big difference. Also let's face it, NFS
has been the essential de facto goto tool to access remote storage for a
long time. Personally, I'd rather see the NFS code split out of the
*_fs.c backend, but I don't have the desire/time to do it - so it stays
as is.

>> When you create a vstorage "file" is that done via touch? or edit some
>> path or some other "common" OS command?  Or is there a vstorage command
>> that needs to be used.  If the former, then using the common
>> storage_backend API's should be possible.
> 
> vstorage is just another "remote filesystem" type of distributed
> software defined storage. In terms of starting to use it, it doesn't
> differ from nfs - you should mount it and then you can use any POSIX
> calls to control files and directories resided on it.

Here's a sample nfs pool xml I have - I changed the source/target path
and didn't define a host.


  nfs
  0
  0
  0
  



  
  
/path/to/target

  0700
  0
  0

  


That is vastly different than what was in the cover:


  VZ
  5f45665b-66fa-4b18-84d1-248774cff3a1
  107374182400
  1441144832
  105933037568
  
vz7-vzstorage
  
  
/vzstorage_pool

  0700
  0
  0

  


What causes "vz7-vzstorage" to be defined? Something from the 'VSTORAGE'
command. I would think that is that essentially similar to how
glusterfs, rbd, or sheepdog uses a source  field. Note that each
of those include a  definition, although
this vstorage XML doesn't.

Thus it seems vzstorage is really not a "local" filesystem, correct? If
so, then should one really be allowing "local" things like chown, chmod,
etc. to be executed? What kind of "configuration and/or validation of
trust" takes place via vstorage provided tools in order to allow a user
on the local host to access the storage on the remote host.

John
> 
> Maxim
>>
>> John
>>
>>
 Also I forgot to mention yesterday - you need to update the
 docs/formatstorage.html.in at the very least and also the storage
 driver
 page docs/storage.html.in.
 In addition there are storagepool tests (xml2xml) that would need to be
 updated to validate the new storage pool type. The tests would "show"
 how the pool XML would appear and validate whatever parsing has been
 done.
>>> I know. Will fix.
>>>
>> [...]
> 

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

Re: [libvirt] [PATCH 1/2] qemu: Allow saving QEMU libvirt state to a pipe

[Peter Krempa]

On Sat, Dec 03, 2016 at 17:45:47 +0800, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
> Base upon patches from Roy Keene 
> 
> Currently qemuDomainSaveMemory can save vm's config
> and memory to fd.
> It writes a magic QEMU_SAVE_PARTIAL firstly,
> then re-open it to change QEMU_SAVE_PARTIAL as QEMU_SAVE_MAGIC
> after a success write.
> 
> For pipes this is not possible, attempting to re-open the pipe
> will not connect you to the same consumer.
> Seeking is also not possible on a pipe.
> 
> This patch introduce VIR_DOMAIN_SAVE_PIPE.
> If set, write QEMU_SAVE_MAGIC directly.
> Try to write a regular file with VIR_DOMAIN_SAVE_PIPE
> is not supportted.
> 
> This is useful to me for saving a VM state directly to
> Ceph RBD images without having an intermediate file.
> 
> Cc: Roy Keene 
> Signed-off-by: Chen Hanxiao 
> ---
>  include/libvirt/libvirt-domain.h |  1 +
>  src/qemu/qemu_driver.c   | 71 
> ++--
>  2 files changed, 54 insertions(+), 18 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-domain.h 
> b/include/libvirt/libvirt-domain.h
> index a8435ab..c3e4c15 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -1169,6 +1169,7 @@ typedef enum {
>  VIR_DOMAIN_SAVE_BYPASS_CACHE = 1 << 0, /* Avoid file system cache 
> pollution */
>  VIR_DOMAIN_SAVE_RUNNING  = 1 << 1, /* Favor running over paused */
>  VIR_DOMAIN_SAVE_PAUSED   = 1 << 2, /* Favor paused over running */
> +VIR_DOMAIN_SAVE_PIPE = 1 << 3, /* Output is a pipe */

It doesn't have necessarily to be a pipe.

>  } virDomainSaveRestoreFlags;
>  
>  int virDomainSave   (virDomainPtr domain,
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 3517aa2..58422ac 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -3054,14 +3054,15 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
>  virQEMUSaveHeader header;
>  bool bypassSecurityDriver = false;
>  bool needUnlink = false;
> +bool canReopen = true;
>  int ret = -1;
>  int fd = -1;
>  int directFlag = 0;
>  virFileWrapperFdPtr wrapperFd = NULL;
>  unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING;
> +struct stat statbuf;
>  
>  memset(, 0, sizeof(header));
> -memcpy(header.magic, QEMU_SAVE_PARTIAL, sizeof(header.magic));
>  header.version = QEMU_SAVE_VERSION;
>  header.was_running = was_running ? 1 : 0;
>  header.compressed = compressed;
> @@ -3077,6 +3078,27 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
>  goto cleanup;
>  }
>  }
> +
> +/*
> + * Determine if this file is a PIPE, which could not be reopen.
> + */
> +if (virFileExists(path)) {
> +fd = qemuOpenFile(driver, vm, path, O_RDONLY | O_NONBLOCK, NULL, 
> NULL);
> +if (fd < 0)
> +goto cleanup;
> +if (fstat(fd, ) < 0)
> +goto cleanup;
> +if (S_ISFIFO(statbuf.st_mode)) {

You should not try to check this. If the user wishes to write the
complete header right away, then we should obey it and not have to check
prior to do so.

> +if (flags & VIR_DOMAIN_SAVE_PIPE) {
> +canReopen = false;
> +} else {
> +virReportSystemError(EINVAL, _("%s is not PIPE"), path);
> +goto cleanup;
> +}
> +}
> +VIR_FORCE_CLOSE(fd);
> +}
> +
>  fd = qemuOpenFile(driver, vm, path,
>O_WRONLY | O_TRUNC | O_CREAT | directFlag,
>, );


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

[libvirt] [PATCH] qemu: Don't try to find compression program for "raw" memory images

[Peter Krempa]

There's nothing to compress if the requested snapshot memory format is
set to 'raw' explicitly. After commit 9e14689ea libvirt would try to
run /sbin/raw to process them emory stream if the qemu.conf option
snapshot_image_format is set.

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

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6b177e9..4ef1860 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3300,6 +3300,9 @@ qemuGetCompressionProgram(const char *imageFormat,
 if ((ret = qemuSaveCompressionTypeFromString(imageFormat)) < 0)
 goto error;

+if (ret == QEMU_SAVE_FORMAT_RAW)
+return QEMU_SAVE_FORMAT_RAW;
+
 if (!(*compresspath = virFindFileInPath(imageFormat)))
 goto error;

-- 
2.10.2

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

Re: [libvirt] [PATCH] Deal with gnutls 3.5.6 regression

[Daniel P. Berrange]

On Mon, Dec 05, 2016 at 12:04:36PM +, Daniel P. Berrange wrote:
> I was not originally planning to do anything for the gnutls 3.5.6
> regression:
> 
>   https://www.redhat.com/archives/libvir-list/2016-November/msg00816.html
> 
> but there's still no immediate sign of the new 3.5.7 release,
> so while I still don't want to workaround the bug in libvirt,
> we can at least blacklist that version of gnutls in the test
> suite, so 'make check' passes on affected systems while we're
> waiting for 3.5.7 to arrive.

3.5.7 has just hit Fedora updates-testing, but I figure we
might as well still blacklist 3.5.6 in our tests

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

[libvirt] [PATCH 1/2] vz: set PVMT_DONT_CREATE_DISK migration flag

[Pavel Glushchak]

This flag tells backend not to create instance
disks making behavior the same as in qemu driver.
Disk files have to be created beforehand on target
host manually or by upper management layer i.e.
OpenStack Nova.

Signed-off-by: Pavel Glushchak 
---
 src/vz/vz_sdk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index d13c86a..4cd988a 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -4740,7 +4740,7 @@ int prlsdkSwitchToSnapshot(virDomainObjPtr dom, const 
char *uuid, bool paused)
  * connection to dispatcher
  */
 
-#define PRLSDK_MIGRATION_FLAGS (PSL_HIGH_SECURITY)
+#define PRLSDK_MIGRATION_FLAGS (PSL_HIGH_SECURITY | PVMT_DONT_CREATE_DISK)
 
 int prlsdkMigrate(virDomainObjPtr dom, virURIPtr uri,
   const unsigned char *session_uuid,
-- 
2.7.4

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

[libvirt] [PATCH 2/2] vz: added VIR_MIGRATE_NON_SHARED_INC migration flag support

[Pavel Glushchak]

This flag is used in Virtuozzo backend implicitly, thus
we need to support it and don't fail if it's set.

Signed-off-by: Pavel Glushchak 
---
 src/vz/vz_driver.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 08f7961..0a84cee 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -2899,7 +2899,8 @@ vzEatCookie(const char *cookiein, int cookieinlen, 
unsigned int flags)
 VIR_MIGRATE_PEER2PEER |   \
 VIR_MIGRATE_LIVE |\
 VIR_MIGRATE_UNDEFINE_SOURCE | \
-VIR_MIGRATE_PERSIST_DEST)
+VIR_MIGRATE_PERSIST_DEST |\
+VIR_MIGRATE_NON_SHARED_INC)
 
 #define VZ_MIGRATION_PARAMETERS \
 VIR_MIGRATE_PARAM_DEST_XML, VIR_TYPED_PARAM_STRING, \
-- 
2.7.4

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

Re: [libvirt] [PATCH v2] storage: vz storage pool support

[Maxim Nestratov]

08-Dec-16 02:22, John Ferlan пишет:


[...]


I see what you mean; however, IMO vstorage should be separate. Maybe
there's another opinion out there, but since you're requiring
"something" else to be installed in order to get the WITH_VSTORAGE to be
set to 1, then a separate file is in order.

Not sure they're comparable, but zfs has its own. Having separated
vstorage reduces the chance that some day some incompatible logic is
added/altered in the *fs.c (or vice versa).

Ok. I will try.


I think you should consider the *_fs.c code to be the "default" of
sorts. That is default file/dir structure with netfs added in. The
vstorage may just be some file system, but it's not something (yet) on
"every" distribution.

I did not understand actually, what you mean  "be the "default" of sorts."
As I have understood - what I need to do is to create backend_vstorage.c
with all create/delete/* functionality.


Sorry - I was trying to think of a better way to explain... The 'fs' and
'nfs' pool are default of sorts because one can "ls" (on UNIX/Linux) or
"dir" (on Windows) and get a list of files.

"ls" and "dir" are inherent to the OS, while in this case vstorage
commands are installed separately.


Once you mounted your vstorage cluster to a local filesystem you can also "ls" it. Thus, I can't see much difference 
from nfs here.



When you create a vstorage "file" is that done via touch? or edit some
path or some other "common" OS command?  Or is there a vstorage command
that needs to be used.  If the former, then using the common
storage_backend API's should be possible.


vstorage is just another "remote filesystem" type of distributed software defined storage. In terms of starting to use 
it, it doesn't differ from nfs - you should mount it and then you can use any POSIX calls to control files and 
directories resided on it.


Maxim


John



Also I forgot to mention yesterday - you need to update the
docs/formatstorage.html.in at the very least and also the storage driver
page docs/storage.html.in.
In addition there are storagepool tests (xml2xml) that would need to be
updated to validate the new storage pool type. The tests would "show"
how the pool XML would appear and validate whatever parsing has been
done.

I know. Will fix.


[...]


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

Re: [libvirt] [PATCH 0/2] Allow saving QEMU libvirt state to a pipe

[Chen Hanxiao]

At 2016-12-03 17:45:46, "Chen Hanxiao"  wrote:
>This series introduce flag VIR_DOMAIN_SAVE_PIPE
>to enable command 'save' to write to PIPE.
>
>Base upon patches from Roy Keene 
>with some fixes.
>
>Change from original patch:
>1) Check whether the specified path is a PIPE.
>2) Rebase on upstream.
>3) Add doc for virsh command
>
>Chen Hanxiao (2):
>  qemu: Allow saving QEMU libvirt state to a pipe
>  virsh: introduce flage --pipe for save command
>
> include/libvirt/libvirt-domain.h |  1 +
> src/qemu/qemu_driver.c   | 71 ++--
> tools/virsh-domain.c |  6 
> tools/virsh.pod  |  6 +++-
> 4 files changed, 65 insertions(+), 19 deletions(-)
>

ping

Regards,
- Chen

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

[libvirt] [PATCH] virsh: maxvcpus: Always fall back to the old command if domain caps fail

[Peter Krempa]

1ec22be5 added code that detects the maximum cpu count according to
domain capabilities. The code fell back to the old command only if the
API was not supported. If the API fails for other reasons the command
would fail. There's no point in not trying the old API in such case.

https://bugzilla.redhat.com/show_bug.cgi?id=1402690
---
 tools/virsh-host.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/virsh-host.c b/tools/virsh-host.c
index 2fd3686..24ebde2 100644
--- a/tools/virsh-host.c
+++ b/tools/virsh-host.c
@@ -623,9 +623,6 @@ cmdMaxvcpus(vshControl *ctl, const vshCmd *cmd)

 ignore_value(virXPathInt("string(./vcpu[1]/@max)", ctxt, ));
 } else {
-if (last_error && last_error->code != VIR_ERR_NO_SUPPORT)
-goto cleanup;
-
vshResetLibvirtError();
 }

-- 
2.10.2

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