[libvirt] [PATCHv3] replace 'if' type conditions with 'switch' for VIR_NETWORK_FORWARD_

[Shi Lei]

Signed-off-by: Shi Lei 
---

v2 here: https://www.redhat.com/archives/libvir-list/2018-July/msg01423.html
since v2:
 - typecast def->forward.type to virNetworkForwardType explicitly
  in all the switches rather than change its type to enum in
  the struct definition

v1 here: https://www.redhat.com/archives/libvir-list/2018-July/msg01314.html
since v1:
 - Change the type declaration of _virNetworkForwardDef.type
  from int to virNetworkForwardType
 - use the default case to report out of range error with
  virReportEnumRangeError

 src/conf/domain_conf.c   |  49 ---
 src/conf/network_conf.c  |  75 +++---
 src/conf/virnetworkobj.c |  24 +++-
 src/esx/esx_network_driver.c |  19 ++-
 src/libvirt_private.syms |   1 +
 src/network/bridge_driver.c  | 317 ---
 src/qemu/qemu_process.c  |  10 +-
 7 files changed, 334 insertions(+), 161 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 178c6d2..33b0b4a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -29975,40 +29975,49 @@ virDomainNetResolveActualType(virDomainNetDefPtr 
iface)
 if (!(def = virNetworkDefParseString(xml)))
 goto cleanup;
 
-if ((def->forward.type == VIR_NETWORK_FORWARD_NONE) ||
-(def->forward.type == VIR_NETWORK_FORWARD_NAT) ||
-(def->forward.type == VIR_NETWORK_FORWARD_ROUTE) ||
-(def->forward.type == VIR_NETWORK_FORWARD_OPEN)) {
+switch ((virNetworkForwardType) def->forward.type) {
+case VIR_NETWORK_FORWARD_NONE:
+case VIR_NETWORK_FORWARD_NAT:
+case VIR_NETWORK_FORWARD_ROUTE:
+case VIR_NETWORK_FORWARD_OPEN:
 /* for these forward types, the actual net type really *is*
  * NETWORK; we just keep the info from the portgroup in
  * iface->data.network.actual
  */
 ret = VIR_DOMAIN_NET_TYPE_NETWORK;
+break;
 
-} else if ((def->forward.type == VIR_NETWORK_FORWARD_BRIDGE) &&
-   def->bridge) {
-
-/*  
- * is VIR_DOMAIN_NET_TYPE_BRIDGE
- */
-
-ret = VIR_DOMAIN_NET_TYPE_BRIDGE;
-
-} else if (def->forward.type == VIR_NETWORK_FORWARD_HOSTDEV) {
-
+case VIR_NETWORK_FORWARD_HOSTDEV:
 ret = VIR_DOMAIN_NET_TYPE_HOSTDEV;
+break;
 
-} else if ((def->forward.type == VIR_NETWORK_FORWARD_BRIDGE) ||
-   (def->forward.type == VIR_NETWORK_FORWARD_PRIVATE) ||
-   (def->forward.type == VIR_NETWORK_FORWARD_VEPA) ||
-   (def->forward.type == VIR_NETWORK_FORWARD_PASSTHROUGH)) {
+case VIR_NETWORK_FORWARD_BRIDGE:
+if (def->bridge) {
+/*  
+ * is VIR_DOMAIN_NET_TYPE_BRIDGE
+ */
+ret = VIR_DOMAIN_NET_TYPE_BRIDGE;
+break;
+}
+
+/* intentionally fall through to the direct case for
+ * VIR_NETWORK_FORWARD_BRIDGE with no bridge device defined
+ */
+ATTRIBUTE_FALLTHROUGH;
 
+case VIR_NETWORK_FORWARD_PRIVATE:
+case VIR_NETWORK_FORWARD_VEPA:
+case VIR_NETWORK_FORWARD_PASSTHROUGH:
 /*  are all
  * VIR_DOMAIN_NET_TYPE_DIRECT.
  */
-
 ret = VIR_DOMAIN_NET_TYPE_DIRECT;
+break;
 
+case VIR_NETWORK_FORWARD_LAST:
+default:
+virReportEnumRangeError(virNetworkForwardType, def->forward.type);
+goto cleanup;
 }
 
  cleanup:
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 630a87f..c08456b 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1874,7 +1874,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
 /* Validate some items in the main NetworkDef that need to align
  * with the chosen forward mode.
  */
-switch (def->forward.type) {
+switch ((virNetworkForwardType) def->forward.type) {
 case VIR_NETWORK_FORWARD_NONE:
 break;
 
@@ -1955,21 +1955,40 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
 goto error;
 }
 break;
+
+case VIR_NETWORK_FORWARD_LAST:
+default:
+virReportEnumRangeError(virNetworkForwardType, def->forward.type);
+goto error;
 }
 
 VIR_FREE(stp);
 
-if (def->mtu &&
-(def->forward.type != VIR_NETWORK_FORWARD_NONE &&
- def->forward.type != VIR_NETWORK_FORWARD_NAT &&
- def->forward.type != VIR_NETWORK_FORWARD_ROUTE &&
- def->forward.type != VIR_NETWORK_FORWARD_OPEN)) {
-virReportError(VIR_ERR_XML_ERROR,
-   _("mtu size only allowed in open, route, nat, "
- "and isolated mode, not in %s (network '%s')"),
-   virNetworkForwardTypeToString(def->forward.type),
-   def->name);
-goto error;
+if (def->mtu) {
+switch ((virNetworkForwardType) def->forward.type) {
+case VIR_NETWORK_FORWARD_NONE:
+case VIR_NETWORK_FORWARD_NAT:
+case VIR_NETWORK_FORWARD_ROUTE:
+case 

[libvirt] [PATCH] qemu: caps: use model "qemu" for s390 tcg cpu-model-expansion

[Collin Walling]

Use model name "qemu" instead of "max" when calling
query-cpu-model-expansion for s390 on tcg.

Signed-off-by: Collin Walling 
---
 src/qemu/qemu_capabilities.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 23b4833..e9b44cc 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2356,7 +2356,10 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
 
 if (tcg || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
 virtType = VIR_DOMAIN_VIRT_QEMU;
-model = "max";
+if (ARCH_IS_S390(qemuCaps->arch))
+model = "qemu";
+else
+model = "max";
 } else {
 virtType = VIR_DOMAIN_VIRT_KVM;
 model = "host";
-- 
2.7.4

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

[libvirt] [PATCH] use model "qemu" for s390 tcg cpu-model-expansion

[Collin Walling]

When calling the query-cpu-model-expansion command via libvirt, we pass in 
model 
name "max" when using TCG. However, this model name is not supported for s390 
on 
TCG. Let's use the model name "qemu" as an alternative.

QEMU Results:

When executing query-cpu-model-expansion via QEMU on s390 with TCG, the name 
"max" 
results in this error:

{ "execute": "query-cpu-model-expansion", "arguments": {"type": "static", 
"model": {"name": "max"}}}

-> {"error": {"class": "GenericError", "desc": "The CPU definition 'max' is 
unknown."}}

An alternative is to use model name "qemu." On a z13.2 machine:

{ "execute": "query-cpu-model-expansion", "arguments": {"type": "static", 
"model": {"name": "qemu"}}}

-> {"return": {"model": {"name": "zEC12.2-base", "props": {"dateh2": false, 
"aen": true, "kmac-tdea-192": false, 
"kmc-tdea-192": false, "parseh": false, "csske": false, "hfpm": false, "hfpue": 
false, "dfp": false, "km-dea": false, 
"emon": false, "kimd-sha-1": false, "cmpsceh": false, "dfpzc": false, "dfphp": 
false, "kmc-dea": false, 
"klmd-sha-1": false, "asnlxr": false, "km-tdea-192": false, "km-tdea-128": 
false, "fpe": false, "kmac-dea": false, 
"kmc-tdea-128": false, "ais": true, "kmac-tdea-128": false, "nonqks": false, 
"pfpo": false, "msa4-base": true, 
"msa3-base": true, "tods": false

I am by all means *not* an expert in TCG, but I noticed this error while I was 
messing
around with things on my system.

Thanks!

Collin Walling (1):
  qemu: caps: use model "qemu" for s390 tcg cpu-model-expansion

 src/qemu/qemu_capabilities.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.7.4

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

Re: [libvirt] [PATCH v2] conf: virDomainDefValidateInternal prohibit some characters in shmem name

[John Ferlan]

On 07/12/2018 09:10 AM, Simon Kobyda wrote:
> XML shmem name will not include character '/', and will not be equal to 
> strings
> "." or "..", as shmem name is used in a path.

Validate that the provided XML shmem name is not directory specific "."
or ".." names as well as ensuring that there is no path separator '/' in
the name.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1192400
> ---
> 
> Changes in V2 
>   - Added error reports
>   - Error situation will happen only if shmem name is equal to
> "." or "..", however their occurence in a name compromised of more
>   characters is allowed.
> 
>  src/conf/domain_conf.c | 22 ++
>  1 file changed, 22 insertions(+)
> 

I believe this actually belongs in virDomainDeviceDefValidateInternal
for case VIR_DOMAIN_DEVICE_SHMEM.

Also, should the docs/schemas/domaincommon.rng be modified? Currently it
has:

  

  

  [^/]*


Consider how other names are limited in their scope. The basictypes.rng
has a number of examples.

Naturally, the problem with changing it is that someone somewhere will
complain, but libvirt used to accept this other format. Right now I
would think the scope a bit too broad.

If we are to limit the name we should also document in
docs/formatdomain.html.in that the shmem name is "limited" in name to
avoid the '/' character, ".", and "..".

BTW: My regex isn't that good, but it would seem '/' is an invalid
character by XML standards even though the code never checked for it.
Using virt-xml-validate   would "validate" whether someone
provides valid XML.


John

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 7ab2953d83..6b34c17de4 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6107,6 +6107,8 @@ virDomainDefLifecycleActionValidate(const virDomainDef 
> *def)
>  static int
>  virDomainDefValidateInternal(const virDomainDef *def)
>  {
> +size_t i;
> +
>  if (virDomainDefCheckDuplicateDiskInfo(def) < 0)
>  return -1;
>  
> @@ -6136,6 +6138,26 @@ virDomainDefValidateInternal(const virDomainDef *def)
>  return -1;
>  }
>  
> +for (i = 0; i < def->nshmems; i++) {
> +if (strchr(def->shmems[i]->name, '/')) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> +   _("shmem name cannot include '/' character"));
> +return -1;
> +}
> +
> +if (STREQ(def->shmems[i]->name, ".")) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> +   _("shmem name cannot be equal to '.'"));
> +return -1;
> +}
> +
> +if (STREQ(def->shmems[i]->name, "..")) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> +   _("shmem name cannot be equal to '..'"));
> +return -1;
> +}
> +}
> +
>  if (virDomainDefLifecycleActionValidate(def) < 0)
>  return -1;
>  
> 

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

Re: [libvirt] [PATCH v2 4/5] check-file-access: Allow specifying action

[John Ferlan]

On 07/23/2018 04:44 AM, Michal Prívozník wrote:
> On 07/21/2018 02:57 PM, John Ferlan wrote:
>>
>>
>> On 07/12/2018 03:37 AM, Michal Privoznik wrote:
>>> The check-file-access.pl script is used to match access list
>>> generated by virtestmock against whitelisted rules stored in
>>> file_access_whitelist.txt. So far the rules are in form:
>>>
>>>   $path: $progname: $testname
>>>
>>> This is not sufficient because the rule does not take into
>>> account 'action' that caused $path to appear in the list of
>>> accessed files. After this commit the rule can be in new form:
>>>
>>>   $path: $action: $progname: $testname
>>>
>>> where $action is one from ("open", "fopen", "access", "stat",
>>> "lstat", "connect"). This way the white list can be fine tuned to
>>> allow say access() but not connect().
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>  tests/check-file-access.pl  | 32 +++-
>>>  tests/file_access_whitelist.txt | 15 ++-
>>>  2 files changed, 37 insertions(+), 10 deletions(-)
>>>
>>
>> Perl scripts, not my area of expertise...  Even worse, regexes.
>>
>> Still I am unclear about this particular one because you stuffed $action
>> in front of something that already existed and it makes me wonder how
>> that affects existing files. [1]
>>
>>
>>> diff --git a/tests/check-file-access.pl b/tests/check-file-access.pl
>>> index 977a2bc533..ea0b7a18a2 100755
>>> --- a/tests/check-file-access.pl
>>> +++ b/tests/check-file-access.pl
>>> @@ -27,18 +27,21 @@ use warnings;
>>>  my $access_file = "test_file_access.txt";
>>>  my $whitelist_file = "file_access_whitelist.txt";
>>>  
>>> +my @known_actions = ("open", "fopen", "access", "stat", "lstat", 
>>> "connect");
>>> +
>>>  my @files;
>>>  my @whitelist;
>>>  
>>>  open FILE, "<", $access_file or die "Unable to open $access_file: $!";
>>>  while () {
>>>  chomp;
>>> -if (/^(\S*):\s*(\S*)(\s*:\s*(.*))?$/) {
>>> +if (/^(\S*):\s*(\S*):\s*(\S*)(\s*:\s*(.*))?$/) {
>>>  my %rec;
>>>  ${rec}{path} = $1;
>>> -${rec}{progname} = $2;
>>> -if (defined $4) {
>>> -${rec}{testname} = $4;
>>> +${rec}{action} = $2;
>>> +${rec}{progname} = $3;
>>> +if (defined $5) {
>>> +${rec}{testname} = $5;
>>>  }
>>>  push (@files, \%rec);
>>>  } else {
>>> @@ -52,7 +55,21 @@ while () {
>>>  chomp;
>>>  if (/^\s*#.*$/) {
>>>  # comment
>>> +} elsif (/^(\S*):\s*(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/ and
>>> +grep /^$2$/, @known_actions) {
>>> +# $path: $action: $progname: $testname
>>> +my %rec;
>>> +${rec}{path} = $1;
>>> +${rec}{action} = $3;
>>> +if (defined $4) {
>>> +${rec}{progname} = $4;
>>> +}
>>> +if (defined $6) {
>>> +${rec}{testname} = $6;
>>> +}
>>> +push (@whitelist, \%rec);
>>>  } elsif (/^(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/) {
>>> +# $path: $progname: $testname
>>>  my %rec;
>>>  ${rec}{path} = $1;
>>>  if (defined $3) {
>>> @@ -79,6 +96,11 @@ for my $file (@files) {
>>>  next;
>>>  }
>>>  
>>> +if (defined %${rule}{action} and
>>> +not %${file}{action} =~ m/^$rule->{action}$/) {
>>> +next;
>>> +}
>>> +
>>>  if (defined %${rule}{progname} and
>>>  not %${file}{progname} =~ m/^$rule->{progname}$/) {
>>>  next;
>>> @@ -95,7 +117,7 @@ for my $file (@files) {
>>>  
>>>  if (not $match) {
>>>  $error = 1;
>>> -print "$file->{path}: $file->{progname}";
>>> +print "$file->{path}: $file->{action}: $file->{progname}";
>>>  print ": $file->{testname}" if defined %${file}{testname};
>>>  print "\n";
>>>  }
>>> diff --git a/tests/file_access_whitelist.txt 
>>> b/tests/file_access_whitelist.txt
>>> index 850b28506e..3fb318cbab 100644
>>> --- a/tests/file_access_whitelist.txt
>>> +++ b/tests/file_access_whitelist.txt
>>> @@ -1,14 +1,17 @@
>>>  # This is a whitelist that allows accesses to files not in our
>>>  # build directory nor source directory. The records are in the
>>> -# following format:
>>> +# following formats:
>>>  #
>>>  #  $path: $progname: $testname
>>> +#  $path: $action: $progname: $testname
>>>  #
>>> -# All these three are evaluated as perl RE. So to allow /dev/sda
>>> -# and /dev/sdb, you can just '/dev/sd[a-b]', or to allow
>>> +# All these variables are evaluated as perl RE. So to allow
>>> +# /dev/sda and /dev/sdb, you can just '/dev/sd[a-b]', or to allow
>>>  # /proc/$pid/status you can '/proc/\d+/status' and so on.
>>> -# Moreover, $progname and $testname can be empty, in which which
>>> -# case $path is allowed for all tests.
>>> +# Moreover, $action, $progname and $testname can be empty, in which
>>> +# which case $path is allowed for all tests. However, $action (if
>>> +# specified) must be one of "open", "fopen", 

Re: [libvirt] [PATCHv2 11/11] qemu_driver: BaselineHypervisorCPU supports S390 using QEMU/QMP

[Collin Walling]

On 07/21/2018 12:05 AM, Chris Venteicher wrote:
> Quoting David Hildenbrand (2018-07-18 02:26:24)
>> On 18.07.2018 00:39, Collin Walling wrote:
>>> On 07/17/2018 05:01 PM, David Hildenbrand wrote:
 On 13.07.2018 18:00, Jiri Denemark wrote:
> On Mon, Jul 09, 2018 at 22:56:55 -0500, Chris Venteicher wrote:
>> Transient S390 configurations require using QEMU to compute CPU Model
>> Baseline and to do CPU Feature Expansion.
>>
>> Start and use a single QEMU instance to do both the baseline and
>> expansion transactions required by BaselineHypervisorCPU.
>>
>> CPU Feature Expansion uses true / false to indicate if property is/isn't
>> included in model. Baseline only returns property list where all
>> enumerated properties are included.
>
> So are you saying on s390 there's no chance there would be a CPU model
> with some feature which is included in the CPU model disabled for some
> reason? Sounds too good to be true :-) (This is the question I referred
> to in one of my replies to the other patches.)

 Giving some background information: When we expand/baseline CPU models,
 we always expand them to the "-base" variants of our CPU models, which
 contain some set of features we expect to be around in all sane
 configurations ("minimal feature set").

 It is very unlikely that we ever have something like
 "z14-base,featx=off", but it could happen
  - When using an emulator (TCG)
  - When running nested and the guest hypervisor is started with such a
strange CPU model
  - When something in the HW is very wrong or eventually removed in the
future (unlikely but possible)

 On some very weird inputs to a baseline request, such a strange model
 can also be the result. But it is very unusual.

 I assume something like "baseline z14-base,featx=off with z14-base" will
 result in "z14-base,featx=off", too.


>>>
>>> That's correct. A CPU model with a feature disabled that is baseline with a 
>>> CPU 
>>> model with that same feature enabled will omit that feature in the QMP 
>>> response.
>>>
>>> e.g. if z14-base has features x, y, z then
>>>
>>> "baseline z14-base,featx=off with z14-base" will result in 
>>> "z14-base,featy=on,featz=on"
> 
> I am runing tests on both S390 and X86 (hacked QEMU to enable baseline).
> 
> I don't see a "false" property in the baseline response in any of the logs.

Right... baseline should not be returning any properties paired with false. It
constructs a third CPU model with properties that can run on both CPUs.

> 
> I did try to slip a "zpci":false into the query-cpu-model-baseline but I 
> still 
> don't get a false in the response.
> 

Sending a property paired with "false" in the JSON object is telling QEMU "I 
want 
to turn off this feature." The feature will then be omitted from the QMP 
response.

> Here is the request/response for reference.
> 
> {"execute":"query-cpu-model-baseline",
>  "arguments":{"modela":{"name":"z14"},
>   
> "modelb":{"name":"z13","props":{"msa5":true,"exrl":true,"zpci":false}}},
>  "id":"libvirt-2"} 
> 
> {"return": {"model": {"name": "z13-base","props": {"aen": true, "aefsi": 
> true, 
> "msa5": true, "msa4": true, "msa3": true, "msa2": true, "msa1": true, 
> "sthyi": 
> true, "edat": true, "ri": true, "edat2": true, "vx": true, "ipter": true, 
> "esop": true, "cte": true, "sea_esop2": true, "te": true, "cmm": true}}}, 
> "id": 
> "libvirt-2"}
> 
>>> Usually we try to not chose a model with stripped off base features ("we
>> try to produce a model that looks sane"), but instead fallback to some
>> very ancient CPU model. E.g.
>>
>> { "execute": "query-cpu-model-baseline", "arguments" : { "modela": {
>> "name": "z14-base", "props": {"msa" : false}}, "modelb": { "name": "z14"}} }
>>
>> -> {"return": {"model": {"name": "z800-base", "props": {"etf2": true,
>> "ldisp": true
>>
>> We might want to change that behavior in the future however (or maybe it
>> already is like this for some corner cases) - assume some base feature
>> gets dropped by HW in a new CPU generation. We don't always want to
>> fallback to a z900 or so when baselining. So one should assume that we
>> can have disabled features here.
>>
>> Especially as there is a BUG in QEMU I'll have to fix:
>>
>> { "execute": "query-cpu-model-baseline", "arguments" : { "modela": {
>> "name": "z14-base", "props": {"esan3" : false}}, "modelb": { "name":
>> "z14"}} }
>>
>> -> Segmentation fault
>>
>> This would have to produce a model with esan3 disabled (very very
>> unlikely to ever happen in real life :) )
>>
>> The result should be something like {"model": {"name": "z900-base",
>> "props": {"esan3": false}}} or an error that they cannot be baselined.
>>
> 
> Seems like were saying I should be filtering out or otherwise property 
> excluding
> any false properties that are returned. Please correct if I have this wrong.> 
> I currently do 

[libvirt] [PATCH v2 0/3] iscsi-direct: first part

[clem]

From: Clementine Hayat 

Hello,

This is the implementation of the iscsi-direct backend storage pool
version 2.
The documentation, some API calls and tests are still missing and will
be comming in a second part.

Best Regards,

-- 
Clementine Hayat

Clementine Hayat (3):
  configure: Introduce libiscsi in build system
  storage: Introduce iscsi_direct pool type
  storage: Implement iscsi_direct pool backend

 configure.ac   |   9 +-
 m4/virt-libiscsi.m4|  30 ++
 m4/virt-storage-iscsi-direct.m4|  44 ++
 src/conf/domain_conf.c |   4 +
 src/conf/storage_conf.c|  33 +-
 src/conf/storage_conf.h|   1 +
 src/conf/virstorageobj.c   |   2 +
 src/storage/Makefile.inc.am|  24 ++
 src/storage/storage_backend.c  |   6 +
 src/storage/storage_backend_iscsi_direct.c | 460 +
 src/storage/storage_backend_iscsi_direct.h |   6 +
 src/storage/storage_driver.c   |   1 +
 tools/virsh-pool.c |   3 +
 13 files changed, 618 insertions(+), 5 deletions(-)
 create mode 100644 m4/virt-libiscsi.m4
 create mode 100644 m4/virt-storage-iscsi-direct.m4
 create mode 100644 src/storage/storage_backend_iscsi_direct.c
 create mode 100644 src/storage/storage_backend_iscsi_direct.h

-- 
2.18.0

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

[libvirt] [PATCH v2 3/3] storage: Implement iscsi_direct pool backend

[clem]

From: Clementine Hayat 

We need here libiscsi for the storgae pool backend.
For the iscsi-direct storage pool, only checkPool and refreshPool should
be necessary.
The pool is state-less and just need the informations within the volume
to work.

Signed-off-by: Clementine Hayat 
---
 m4/virt-storage-iscsi-direct.m4|   3 +
 src/storage/Makefile.inc.am|   2 +
 src/storage/storage_backend_iscsi_direct.c | 408 -
 3 files changed, 410 insertions(+), 3 deletions(-)

diff --git a/m4/virt-storage-iscsi-direct.m4 b/m4/virt-storage-iscsi-direct.m4
index cc2d490352..dab4414169 100644
--- a/m4/virt-storage-iscsi-direct.m4
+++ b/m4/virt-storage-iscsi-direct.m4
@@ -29,6 +29,9 @@ AC_DEFUN([LIBVIRT_STORAGE_CHECK_ISCSI_DIRECT], [
 with_storage_iscsi_direct=$with_libiscsi
   fi
   if test "$with_storage_iscsi_direct" = "yes"; then
+if test "$with_libiscsi" = "no"; then
+  AC_MSG_ERROR([Need libiscsi for iscsi-direct storage driver])
+fi
 AC_DEFINE_UNQUOTED([WITH_STORAGE_ISCSI_DIRECT], [1],
[whether iSCSI backend for storage driver is enabled])
   fi
diff --git a/src/storage/Makefile.inc.am b/src/storage/Makefile.inc.am
index b2714fd960..bd5ea06f8b 100644
--- a/src/storage/Makefile.inc.am
+++ b/src/storage/Makefile.inc.am
@@ -203,6 +203,7 @@ if WITH_STORAGE_ISCSI_DIRECT
 libvirt_storage_backend_iscsi_direct_la_SOURCES = 
$(STORAGE_DRIVER_ISCSI_DIRECT_SOURCES)
 libvirt_storage_backend_iscsi_direct_la_CFLAGS = \
-I$(srcdir)/conf \
+   -I$(srcdir)/secret \
$(LIBISCSI_CFLAGS) \
$(AM_CFLAGS) \
$(NULL)
@@ -211,6 +212,7 @@ storagebackend_LTLIBRARIES += 
libvirt_storage_backend_iscsi-direct.la
 libvirt_storage_backend_iscsi_direct_la_LDFLAGS = $(AM_LDFLAGS_MOD)
 libvirt_storage_backend_iscsi_direct_la_LIBADD = \
libvirt.la \
+   $(LIBISCSI_LIBS) \
../gnulib/lib/libgnu.la \
$(NULL)
 endif WITH_STORAGE_ISCSI_DIRECT
diff --git a/src/storage/storage_backend_iscsi_direct.c 
b/src/storage/storage_backend_iscsi_direct.c
index 94c4c989ff..b9a9bb3eb0 100644
--- a/src/storage/storage_backend_iscsi_direct.c
+++ b/src/storage/storage_backend_iscsi_direct.c
@@ -22,28 +22,430 @@
 
 #include 
 
+#include 
+#include 
+
+#include "datatypes.h"
+#include "secret_util.h"
 #include "storage_backend_iscsi_direct.h"
 #include "storage_util.h"
+#include "viralloc.h"
+#include "virerror.h"
 #include "virlog.h"
+#include "virobject.h"
+#include "virstring.h"
+#include "virtime.h"
+#include "viruuid.h"
 
 #define VIR_FROM_THIS VIR_FROM_STORAGE
 
+#define ISCSI_DEFAULT_TARGET_PORT 3260
+#define VIR_ISCSI_TEST_UNIT_TIMEOUT 30 * 1000
+
 VIR_LOG_INIT("storage.storage_backend_iscsi_direct");
 
+static struct iscsi_context *
+virISCSIDirectCreateContext(const char* initiator_iqn)
+{
+struct iscsi_context *iscsi = NULL;
+
+iscsi = iscsi_create_context(initiator_iqn);
+if (!iscsi)
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Failed to create iscsi context for %s"),
+   initiator_iqn);
+return iscsi;
+}
+
+static char *
+virStorageBackendISCSIDirectPortal(virStoragePoolSourcePtr source)
+{
+char *portal = NULL;
+
+if (source->nhost != 1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Expected exactly 1 host for the storage pool"));
+return NULL;
+}
+if (source->hosts[0].port == 0) {
+ignore_value(virAsprintf(, "%s:%d",
+ source->hosts[0].name,
+ ISCSI_DEFAULT_TARGET_PORT));
+} else if (strchr(source->hosts[0].name, ':')) {
+ignore_value(virAsprintf(, "[%s]:%d",
+ source->hosts[0].name,
+ source->hosts[0].port));
+} else {
+ignore_value(virAsprintf(, "%s:%d",
+ source->hosts[0].name,
+ source->hosts[0].port));
+}
+return portal;
+}
+
+static int
+virStorageBackendISCSIDirectSetAuth(struct iscsi_context *iscsi,
+virStoragePoolSourcePtr source)
+{
+unsigned char *secret_value = NULL;
+size_t secret_size;
+virStorageAuthDefPtr authdef = source->auth;
+int ret = -1;
+virConnectPtr conn = NULL;
+
+if (!authdef || authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE)
+return 0;
+
+VIR_DEBUG("username='%s' authType=%d seclookupdef.type=%d",
+  authdef->username, authdef->authType, 
authdef->seclookupdef.type);
+
+if (authdef->authType != VIR_STORAGE_AUTH_TYPE_CHAP) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("iscsi-direct pool only supports 'chap' auth type"));
+return ret;
+}
+
+if (!(conn = virGetConnectSecret()))
+return ret;
+
+if (virSecretGetSecretString(conn, >seclookupdef,
+ 

[libvirt] [PATCH v2 2/3] storage: Introduce iscsi_direct pool type

[clem]

From: Clementine Hayat 

Introducing the pool as a noop. Integration inside the build
system. Implementation will be in the following commits.

Signed-off-by: Clementine Hayat 
---
 configure.ac   |  6 ++-
 m4/virt-storage-iscsi-direct.m4| 41 +++
 src/conf/domain_conf.c |  4 ++
 src/conf/storage_conf.c| 33 ++--
 src/conf/storage_conf.h|  1 +
 src/conf/virstorageobj.c   |  2 +
 src/storage/Makefile.inc.am| 22 
 src/storage/storage_backend.c  |  6 +++
 src/storage/storage_backend_iscsi_direct.c | 58 ++
 src/storage/storage_backend_iscsi_direct.h |  6 +++
 src/storage/storage_driver.c   |  1 +
 tools/virsh-pool.c |  3 ++
 12 files changed, 178 insertions(+), 5 deletions(-)
 create mode 100644 m4/virt-storage-iscsi-direct.m4
 create mode 100644 src/storage/storage_backend_iscsi_direct.c
 create mode 100644 src/storage/storage_backend_iscsi_direct.h

diff --git a/configure.ac b/configure.ac
index c668630a79..87ac4dc2c3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -564,6 +564,7 @@ LIBVIRT_STORAGE_ARG_DIR
 LIBVIRT_STORAGE_ARG_FS
 LIBVIRT_STORAGE_ARG_LVM
 LIBVIRT_STORAGE_ARG_ISCSI
+LIBVIRT_STORAGE_ARG_ISCSI_DIRECT
 LIBVIRT_STORAGE_ARG_SCSI
 LIBVIRT_STORAGE_ARG_MPATH
 LIBVIRT_STORAGE_ARG_DISK
@@ -578,6 +579,7 @@ if test "$with_libvirtd" = "no"; then
   with_storage_fs=no
   with_storage_lvm=no
   with_storage_iscsi=no
+  with_storage_iscsi_direct=no
   with_storage_scsi=no
   with_storage_mpath=no
   with_storage_disk=no
@@ -598,6 +600,7 @@ LIBVIRT_STORAGE_CHECK_DIR
 LIBVIRT_STORAGE_CHECK_FS
 LIBVIRT_STORAGE_CHECK_LVM
 LIBVIRT_STORAGE_CHECK_ISCSI
+LIBVIRT_STORAGE_CHECK_ISCSI_DIRECT
 LIBVIRT_STORAGE_CHECK_SCSI
 LIBVIRT_STORAGE_CHECK_MPATH
 LIBVIRT_STORAGE_CHECK_DISK
@@ -608,7 +611,7 @@ LIBVIRT_STORAGE_CHECK_ZFS
 LIBVIRT_STORAGE_CHECK_VSTORAGE
 
 with_storage=no
-for backend in dir fs lvm iscsi scsi mpath rbd disk; do
+for backend in dir fs lvm iscsi iscsi_direct scsi mpath rbd disk; do
 if eval test \$with_storage_$backend = yes; then
 with_storage=yes
 break
@@ -936,6 +939,7 @@ LIBVIRT_STORAGE_RESULT_DIR
 LIBVIRT_STORAGE_RESULT_FS
 LIBVIRT_STORAGE_RESULT_LVM
 LIBVIRT_STORAGE_RESULT_ISCSI
+LIBVIRT_STORAGE_RESULT_ISCSI_DIRECT
 LIBVIRT_STORAGE_RESULT_SCSI
 LIBVIRT_STORAGE_RESULT_MPATH
 LIBVIRT_STORAGE_RESULT_DISK
diff --git a/m4/virt-storage-iscsi-direct.m4 b/m4/virt-storage-iscsi-direct.m4
new file mode 100644
index 00..cc2d490352
--- /dev/null
+++ b/m4/virt-storage-iscsi-direct.m4
@@ -0,0 +1,41 @@
+dnl Iscsi-direct storage
+dnl
+dnl Copyright (C) 2018 Clementine Hayat.
+dnl
+dnl This library is free software; you can redistribute it and/or
+dnl modify it under the terms of the GNU Lesser General Public
+dnl License as published by the Free Software Foundation; either
+dnl version 2.1 of the License, or (at your option) any later version.
+dnl
+dnl This library is distributed in the hope that it will be useful,
+dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+dnl Lesser General Public License for more details.
+dnl
+dnl You should have received a copy of the GNU Lesser General Public
+dnl License along with this library.  If not, see
+dnl .
+dnl
+
+AC_DEFUN([LIBVIRT_STORAGE_ARG_ISCSI_DIRECT], [
+  LIBVIRT_ARG_WITH_FEATURE([STORAGE_ISCSI_DIRECT],
+   [iscsi-direct backend for the storage driver],
+   [check])
+])
+
+AC_DEFUN([LIBVIRT_STORAGE_CHECK_ISCSI_DIRECT], [
+  AC_REQUIRE([LIBVIRT_CHECK_LIBISCSI])
+  if test "$with_storage_iscsi_direct" = "check"; then
+with_storage_iscsi_direct=$with_libiscsi
+  fi
+  if test "$with_storage_iscsi_direct" = "yes"; then
+AC_DEFINE_UNQUOTED([WITH_STORAGE_ISCSI_DIRECT], [1],
+   [whether iSCSI backend for storage driver is enabled])
+  fi
+  AM_CONDITIONAL([WITH_STORAGE_ISCSI_DIRECT],
+ [test "$with_storage_iscsi_direct" = "yes"])
+])
+
+AC_DEFUN([LIBVIRT_STORAGE_RESULT_ISCSI_DIRECT], [
+  LIBVIRT_RESULT([iscsi-direct], [$with_storage_iscsi_direct])
+])
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7396616eda..5af27a6ad2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -30163,6 +30163,10 @@ virDomainDiskTranslateSourcePool(virDomainDiskDefPtr 
def)
 
 break;
 
+case VIR_STORAGE_POOL_ISCSI_DIRECT:
+def->src->srcpool->mode = VIR_STORAGE_SOURCE_POOL_MODE_DIRECT;
+break;
+
 case VIR_STORAGE_POOL_ISCSI:
 if (def->startupPolicy) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 5036ab9ef8..ee1586410b 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ 

[libvirt] [PATCH v2 1/3] configure: Introduce libiscsi in build system

[clem]

From: Clementine Hayat 

The minimal required version is 1.18.0 because the synchrounous function
needed were introduced here.

Signed-off-by: Clementine Hayat 
---
 configure.ac|  3 +++
 m4/virt-libiscsi.m4 | 30 ++
 2 files changed, 33 insertions(+)
 create mode 100644 m4/virt-libiscsi.m4

diff --git a/configure.ac b/configure.ac
index a87ca06854..c668630a79 100644
--- a/configure.ac
+++ b/configure.ac
@@ -250,6 +250,7 @@ LIBVIRT_ARG_FIREWALLD
 LIBVIRT_ARG_FUSE
 LIBVIRT_ARG_GLUSTER
 LIBVIRT_ARG_HAL
+LIBVIRT_ARG_LIBISCSI
 LIBVIRT_ARG_LIBPCAP
 LIBVIRT_ARG_LIBSSH
 LIBVIRT_ARG_LIBXML
@@ -290,6 +291,7 @@ LIBVIRT_CHECK_FUSE
 LIBVIRT_CHECK_GLUSTER
 LIBVIRT_CHECK_GNUTLS
 LIBVIRT_CHECK_HAL
+LIBVIRT_CHECK_LIBISCSI
 LIBVIRT_CHECK_LIBNL
 LIBVIRT_CHECK_LIBPARTED
 LIBVIRT_CHECK_LIBPCAP
@@ -970,6 +972,7 @@ LIBVIRT_RESULT_FUSE
 LIBVIRT_RESULT_GLUSTER
 LIBVIRT_RESULT_GNUTLS
 LIBVIRT_RESULT_HAL
+LIBVIRT_RESULT_LIBISCSI
 LIBVIRT_RESULT_LIBNL
 LIBVIRT_RESULT_LIBPCAP
 LIBVIRT_RESULT_LIBSSH
diff --git a/m4/virt-libiscsi.m4 b/m4/virt-libiscsi.m4
new file mode 100644
index 00..2747f00ec4
--- /dev/null
+++ b/m4/virt-libiscsi.m4
@@ -0,0 +1,30 @@
+dnl Libiscsi library
+dnl
+dnl Copyright (C) 2018 Clementine Hayat.
+dnl
+dnl This library is free software; you can redistribute it and/or
+dnl modify it under the terms of the GNU Lesser General Public
+dnl License as published by the Free Software Foundation; either
+dnl version 2.1 of the License, or (at your option) any later version.
+dnl
+dnl This library is distributed in the hope that it will be useful,
+dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+dnl Lesser General Public License for more details.
+dnl
+dnl You should have received a copy of the GNU Lesser General Public
+dnl License along with this library.  If not, see
+dnl .
+dnl
+
+AC_DEFUN([LIBVIRT_ARG_LIBISCSI],[
+  LIBVIRT_ARG_WITH_FEATURE([LIBISCSI], [libiscsi], [check], [1.18.0])
+])
+
+AC_DEFUN([LIBVIRT_CHECK_LIBISCSI],[
+  LIBVIRT_CHECK_PKG([LIBISCSI], [libiscsi], [1.18.0])
+])
+
+AC_DEFUN([LIBVIRT_RESULT_LIBISCSI],[
+  LIBVIRT_RESULT_LIB(LIBISCSI)
+])
-- 
2.18.0

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

Re: [libvirt] [PATCH v2 0/2] Minor fixes for virTypedParams(De)Serialize

[John Ferlan]

On 07/17/2018 08:02 AM, Marc Hartmayer wrote:
> Changelog:
>  + v1->v2:
>- Removed the the allocation change in virTypedParams(De)Serialize
>  (John's comment)
> 
> Marc Hartmayer (2):
>   virTypedParamsSerialize: set remote_params_len at the end
>   virTypedParamsDeserialize: set nparams to 0 in case of an error
> 
>  src/util/virtypedparam.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 

Reviewed-by: John Ferlan 
(series)

John

(and pushed)

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

Re: [libvirt] [PATCH v2 1/2] virsh: Support alias in attach-disk

[Michal Prívozník]

On 07/23/2018 04:23 PM, Han Han wrote:
> On Mon, Jul 23, 2018 at 6:21 PM, Michal Prívozník 
> wrote:
> 
>> On 07/15/2018 12:08 PM, Han Han wrote:
>>> Add --alias to support custom disk alias in virsh attach-disk.
>>> Report error if custom alias doesn't start with 'ua-'.
>>>
>>> Signed-off-by: Han Han 
>>> ---
>>>  tools/virsh-domain.c | 15 ++-
>>>  tools/virsh.pod  |  3 ++-
>>>  2 files changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>>> index 8adec1d9b1..467417852e 100644
>>> --- a/tools/virsh-domain.c
>>> +++ b/tools/virsh-domain.c
>>> @@ -290,6 +290,10 @@ static const vshCmdOptDef opts_attach_disk[] = {
>>>   .type = VSH_OT_STRING,
>>>   .help = N_("wwn of disk device")
>>>  },
>>> +{.name = "alias",
>>> + .type = VSH_OT_STRING,
>>> + .help = N_("custom alias name of disk device")
>>> +},
>>>  {.name = "rawio",
>>>   .type = VSH_OT_BOOL,
>>>   .help = N_("needs rawio capability")
>>> @@ -574,7 +578,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>>>  *subdriver = NULL, *type = NULL, *mode = NULL,
>>>  *iothread = NULL, *cache = NULL, *io = NULL,
>>>  *serial = NULL, *straddr = NULL, *wwn = NULL,
>>> -*targetbus = NULL;
>>> +*targetbus = NULL, *alias = NULL;
>>>  struct DiskAddress diskAddr;
>>>  bool isFile = false, functionReturn = false;
>>>  int ret;
>>> @@ -611,6 +615,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>>>  vshCommandOptStringReq(ctl, cmd, "wwn", ) < 0 ||
>>>  vshCommandOptStringReq(ctl, cmd, "address", ) < 0 ||
>>>  vshCommandOptStringReq(ctl, cmd, "targetbus", ) < 0 ||
>>> +vshCommandOptStringReq(ctl, cmd, "alias", ) < 0 ||
>>>  vshCommandOptStringReq(ctl, cmd, "sourcetype", ) < 0)
>>>  goto cleanup;
>>>
>>> @@ -680,6 +685,14 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>>>  if (serial)
>>>  virBufferAsprintf(, "%s\n", serial);
>>>
>>> +if (alias) {
>>> +if (!STRPREFIX(alias, "ua-")) {
>>> +vshError(ctl, _("Custom alias name should start with ua-"));
>>> +goto cleanup;
>>> +}
>>
>> I don't think this belongs here. I'd let libvirt report an error. The
>> reason for that is to have checks in one place rather than scattered
>> around the code. For instance, imagine that one day we lift the
>> restriction and let users define alias in a free form. Using this
>> version of virsh (and connecting to new libvirtd) they will be unable to
>> do so.
>> Or the other way round - we allow only certain characters to be in user
>> alias. You are not checking them here - you're relying on libvirtd doing
>> so.
>>
>> It is reasonable that libvirtd check the alias name. As I know, currently
> libvirtd
> will ignore the customized alias not starting with 'ua-'. Will we keep
> ignoring
> it or report an error in libvirtd?

Well, currently it reports an error if alias is specified and does not
fill all the requirements. Remember, having "ua-" prefix is just one of
the requirements.

Michal

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

Re: [libvirt] [PATCH v1 15/40] util: firewall: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Sukrit Bhatnagar]

On Mon, 23 Jul 2018 at 18:39, Erik Skultety  wrote:
>
> On Sat, Jul 21, 2018 at 05:36:47PM +0530, Sukrit Bhatnagar wrote:
> > By making use of GNU C's cleanup attribute handled by the
> > VIR_AUTOFREE macro for declaring scalar variables, majority
> > of the VIR_FREE calls can be dropped, which in turn leads to
> > getting rid of most of our cleanup sections.
> >
> > Signed-off-by: Sukrit Bhatnagar 
> > ---
> >  src/util/virfirewall.c | 16 +---
> >  1 file changed, 5 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
> > index dfd792f..b4a4d06 100644
> > --- a/src/util/virfirewall.c
> > +++ b/src/util/virfirewall.c
> > @@ -511,7 +511,7 @@ void virFirewallRuleAddArgFormat(virFirewallPtr 
> > firewall,
> >   virFirewallRulePtr rule,
> >   const char *fmt, ...)
> >  {
> > -char *arg;
> > +VIR_AUTOFREE(char *) arg = NULL;
> >  va_list list;
> >
> >  VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, rule);
> > @@ -525,13 +525,11 @@ void virFirewallRuleAddArgFormat(virFirewallPtr 
> > firewall,
> >
> >  va_end(list);
> >
> > -VIR_FREE(arg);
> >  return;
> >
> >   no_memory:
> >  firewall->err = ENOMEM;
> >  va_end(list);
> > -VIR_FREE(arg);
>
> There could be an additional patch replacing the no_memory label with 
> 'cleanup'
> with the obvious adjustments.

There are many functions in virfirewall.c where no_memory is used
instead of cleanup.
So I should change either all of them, or none of them.

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

Re: [libvirt] [go PATCH 00/37] Fix error reporting thread safety wrt goroutine rescheduling

[Daniel P . Berrangé]

This series has been tested by the KubeVirt devs who confirmed it fixes
the problems they see, so I'll push it shortly.

On Mon, Jul 16, 2018 at 02:23:46PM +0100, Daniel P. Berrangé wrote:
> The Libvirt C API provides the virGetLastError() function to let callers
> aquire the error information after a function fails. This function uses
> a thread local, and so must be called in the same OS thread as the
> function that  failed originally.
> eg you could call
> 
> char *xml = virDomainGetXMLDesc(dom);
> if (!xml) {
> virErrorPtr err = virGetLastError();
> do stuff with err...
> }
> 
> This is all fine, but there is a subtle problem that was overlooked when
> the Go bindings were first created. Specifically a native C API call is
> a goroutine re-scheduling point. So when the Go code does
> 
> var xml = C.virDomainGetXMLDesc(dom);
> if (xml == nil) {
> C.virErrorPtr err = C.virGetLastError();
> do stuff with err...
> }
> 
> All that code runs in the same goroutine, but at the call entry to
> C.virGetLastError, the goroutine might get switched to a different
> OS level thread. As a result virGetLastError() will return either no
> error at all, or an error from a completely different libvirt API call.
> 
> We need to prevent the OS level thread being changed in between the call
> to the real function and the virGetLastError() function.
> 
> Naively you might think we could put a LockOSThread() / UnlockOSThread()
> call around this block of Go code, but that is a very bad idea in
> reality. Until Go 1.10, the LockOSThread() calls did not ref count, so
> if some other code has already locked the thread, when libvirt called
> UnlockOSThread it could do bad things. In addition, after calling
> UnlockOSThread() the Go runtime doesn't trust the OS thread state
> anymore, so will terminate the thread and spawn a new one. IOW using
> LockOSThread() would mean every single libvirt API call would create and
> destroy a new thread which is horrible for performance.
> 
> Thus this patch series takes a different approach. We create a wrapper
> function for every C API exposed by libvirt, that has a 'virErrorPtr'
> parameter. So the Go code can do
> 
>  var C.virErrorPtr err
>  var xml = C.virDomainGetXMLDescWrapper(dom, )
>  if (xml == nil) {
>  ...do stuff with err...
>  }
> 
> The wrapper function is responsible for calling virGetLastError() and
> since this is C code, we're guaranteed its all in the same OS level
> thread.
> 
> Daniel P. Berrangé (37):
>   error: add helper for converting libvirt to go error objects
>   storage volume: add missin blank line
>   Rename *cfuncs.{go,h} to *wrapper.{go,h}
>   Use "Wrapper" or "Helper" as suffix for C functions
>   Change "Compat" suffix to "Wrapper" to have standard naming scheme
>   connect: move wrapper functions out of compat header
>   network: move wrapper functions out of compat header
>   nwfilter binding: move wrapper functions out of compat header
>   node device: move wrapper functions out of compat header
>   secret: move wrapper functions out of compat header
>   stream: move wrapper functions out of compat header
>   storage volume: move wrapper functions out of compat header
>   storage pool: move wrapper functions out of compat header
>   qemu: move wrapper functions out of compat header
>   lxc: move wrapper functions out of compat header
>   domain: move wrapper functions out of compat header
>   make the XXX_wrapper.h header files self-contained
>   Add XXX_wrapper.{h,go} for every remaining file
>   Standardize formatting in all wrapper headers
>   storage vol: fix error reporting thread safety
>   storage pool: fix error reporting thread safety
>   stream: fix error reporting thread safety
>   secret: fix error reporting thread safety
>   nwfilter: fix error reporting thread safety
>   nwfilter binding: fix error reporting thread safety
>   node device: fix error reporting thread safety
>   network: fix error reporting thread safety
>   interface: fix error reporting thread safety
>   domain snapshot: fix error reporting thread safety
>   connect: fix error reporting thread safety
>   domain: fix error reporting thread safety
>   qemu: fix error reporting thread safety
>   lxc: fix error reporting thread safety
>   events: fix error reporting thread safety
>   error: remove GetLastError() function
>   error: make GetNotImplementedError private
>   connect: add missing references on domain object in stats records
> 
>  api_test.go   |5 +-
>  callbacks.go  |4 +-
>  callbacks_cfuncs.go => callbacks_wrapper.go   |6 +-
>  callbacks_cfuncs.h => callbacks_wrapper.h |8 +-
>  connect.go|  741 +++---
>  connect_cfuncs.h  |   34 -
>  connect_compat.go |  206 --
>  connect_compat.h  

[libvirt] [PATCH] nwfilter: Resolve SEGV for NWFilter Snoop processing

[John Ferlan]

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

Commit id fca9afa08 changed the @req->ifname to use
@req->binding->portdevname fillingin the @req->binding
in a similar way that @req->ifname would have been
filled in during virNWFilterDHCPSnoopReq processing.

However, in doing so it did not take into account some
code paths where the @req->binding should be checked
instead of @req->binding->portdevname. These checks
led to SEGVs in some cases during libvirtd reload
processing in virNWFilterSnoopRemAllReqIter (for
stop during nwfilterStateCleanup processing) and
virNWFilterSnoopReqLeaseDel (for start during
nwfilterStateInitialize processing).

In particular, when reading the nwfilter.leases file
a new @req is created, but the @req->binding is not
filled in. That's left to virNWFilterDHCPSnoopReq
processing which checks if the @req already exists
in the @virNWFilterSnoopState.snoopReqs hash table
after adding a virNWFilterSnoopState.ifnameToKey
entry for the @req->binding->portdevname by a
@ref->ikey value.

NB: virNWFilterSnoopIPLeaseInstallRule and
virNWFilterDHCPSnoopThread do not need the
req->binding check since they can only be called
after the filter->binding is created/assigned.

Signed-off-by: John Ferlan 
---
 src/nwfilter/nwfilter_dhcpsnoop.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c 
b/src/nwfilter/nwfilter_dhcpsnoop.c
index c7fd370598..2330ba0479 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -846,7 +846,7 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req,
 int ret = 0;
 virNWFilterSnoopIPLeasePtr ipl;
 char *ipstr = NULL;
-int ipAddrLeft;
+int ipAddrLeft = 0;
 
 /* protect req->start, req->ifname and the lease */
 virNWFilterSnoopReqLock(req);
@@ -867,7 +867,8 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req,
 if (update_leasefile)
 virNWFilterSnoopLeaseFileSave(ipl);
 
-ipAddrLeft = virNWFilterIPAddrMapDelIPAddr(req->binding->portdevname, 
ipstr);
+if (req->binding)
+ipAddrLeft = virNWFilterIPAddrMapDelIPAddr(req->binding->portdevname, 
ipstr);
 
 if (!req->threadkey || !instantiate)
 goto skip_instantiate;
@@ -2037,7 +2038,7 @@ virNWFilterSnoopRemAllReqIter(const void *payload,
 /* protect req->binding->portdevname */
 virNWFilterSnoopReqLock(req);
 
-if (req->binding->portdevname) {
+if (req->binding && req->binding->portdevname) {
 ignore_value(virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey,
 req->binding->portdevname));
 
-- 
2.17.1

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

[libvirt] [PATCH] tests: also skip qemuagenttest with old jansson

[Ján Tomko]

qemuagenttest also depends on JSON object key ordering:
Invalid value of argument 'vcpus' of command 'guest-set-vcpus':
expected '[{"logical-id":1,"online":false}]' got 
'[{"online":false,"logical-id":1}]'

Skip it as well.

Signed-off-by: Ján Tomko 
---
Pushed as a build breaker fix for Debian 8.

 tests/qemuagenttest.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c
index 232b34f9cd..b3d737d8a8 100644
--- a/tests/qemuagenttest.c
+++ b/tests/qemuagenttest.c
@@ -907,8 +907,8 @@ mymain(void)
 {
 int ret = 0;
 
-#if !WITH_JANSSON
-fputs("libvirt not compiled with JSON support, skipping this test\n", 
stderr);
+#if !WITH_STABLE_ORDERING_JANSSON
+fputs("libvirt not compiled with recent enough Jansson, skipping this 
test\n", stderr);
 return EXIT_AM_SKIP;
 #endif
 
-- 
2.16.1

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

Re: [libvirt] [PATCH v1 01/40] util: cgroup: modify virCgroupFree to take virCgroupPtr

[Sukrit Bhatnagar]

On Mon, 23 Jul 2018 at 16:29, Erik Skultety  wrote:
>
> On Sat, Jul 21, 2018 at 05:36:33PM +0530, Sukrit Bhatnagar wrote:
> > Modify virCgroupFree function signature to take a value of type
> > virCgroupPtr instead of virCgroupPtr * as the parameter.
> >
> > Change the argument type in all calls to virCgroupFree function
> > from virCgroupPtr * to virCgroupPtr.
>
> ^This sentence doesn't add any useful information. Instead, the commit message
> should add information about why we're performing this change, i.e. in order 
> to
> enable usage of VIR_AUTOPTR with cgroup module or something alike.
> Also, this patch is oddly placed, IMHO it should come before patch 8, where 
> the
> other work on cgroup module is done.
>
> With that:
> Reviewed-by: Erik Skultety 
>
> ...
>
> > @@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path,
> >  ret = 0;
> >   cleanup:
> >  if (ret != 0)
> > -virCgroupFree(group);
> > -virCgroupFree();
> > +virCgroupFree(*group);
> > +virCgroupFree(parent);
>
> Since you're already touching the code, I'd appreciate another "adjustment"
> patch where occurrences like ^this will be replaced by a VIR_STEAL_PTR, IOW
> where we're passing a reference to a pointer in order to change the original
> pointer, we should use a VIR_STEAL_PTR just before the cleanup section, so 
> that
> we don't need an if (ret != 0) or if (ret < 0) check only to conditionally do
> some cleanup. Feel free to let me know if none of what I just wrote is clear.

I am assuming that you are referring to `group` variable. If so, then I cannot
apply cleanup attribute to function parameters and `group` is one of them.

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

Re: [libvirt] [PATCH v1 21/40] util: pci: use VIR_AUTOPTR for aggregate types

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:53PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOPTR macro for declaring aggregate pointer variables,
> majority of the calls to *Free functions can be dropped, which
> in turn leads to getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar 
> ---
>  src/util/virpci.c | 51 ++-
>  1 file changed, 18 insertions(+), 33 deletions(-)
>
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 1f6ac0b..7b0964c 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -490,8 +490,6 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate 
> predicate,
>  ret = 1;
>  break;
>  }
> -
> -virPCIDeviceFree(check);
>  }
>  VIR_DIR_CLOSE(dir);
>  return ret;

...

> @@ -1679,19 +1675,16 @@ virPCIGetAddrString(unsigned int domain,
>  unsigned int function,
>  char **pciConfigAddr)
>  {
> -virPCIDevicePtr dev = NULL;
> -int ret = -1;
> +VIR_AUTOPTR(virPCIDevice) dev = NULL;
>
>  dev = virPCIDeviceNew(domain, bus, slot, function);
>  if (dev != NULL) {
>  if (VIR_STRDUP(*pciConfigAddr, dev->name) < 0)
> -goto cleanup;
> -ret = 0;
> +return -1;
> +return 0;


if (!dev || VIR_STRDUP(*pciConfigAddr, dev->name) < 0))
return -1;

>  }
>
> - cleanup:
> -virPCIDeviceFree(dev);
> -return ret;
> +return -1;

^return 0;

>  }
>
>  virPCIDevicePtr
> @@ -1962,10 +1955,10 @@ virPCIDeviceListAddCopy(virPCIDeviceListPtr list, 
> virPCIDevicePtr dev)
>
>  if (!copy)
>  return -1;
> -if (virPCIDeviceListAdd(list, copy) < 0) {
> -virPCIDeviceFree(copy);
> +if (virPCIDeviceListAdd(list, copy) < 0)
>  return -1;
> -}
> +
> +copy = NULL;

You'll first need to define copy as VIR_AUTOPTR, if you want to use ^this
assignment, otherwise it's a NOP.

There may be some leftovers from the previous patch that will need to go into
this one, otherwise looks fine.

Erik

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

Re: [libvirt] [PATCH v2 1/2] virsh: Support alias in attach-disk

[Han Han]

On Mon, Jul 23, 2018 at 6:21 PM, Michal Prívozník 
wrote:

> On 07/15/2018 12:08 PM, Han Han wrote:
> > Add --alias to support custom disk alias in virsh attach-disk.
> > Report error if custom alias doesn't start with 'ua-'.
> >
> > Signed-off-by: Han Han 
> > ---
> >  tools/virsh-domain.c | 15 ++-
> >  tools/virsh.pod  |  3 ++-
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> > index 8adec1d9b1..467417852e 100644
> > --- a/tools/virsh-domain.c
> > +++ b/tools/virsh-domain.c
> > @@ -290,6 +290,10 @@ static const vshCmdOptDef opts_attach_disk[] = {
> >   .type = VSH_OT_STRING,
> >   .help = N_("wwn of disk device")
> >  },
> > +{.name = "alias",
> > + .type = VSH_OT_STRING,
> > + .help = N_("custom alias name of disk device")
> > +},
> >  {.name = "rawio",
> >   .type = VSH_OT_BOOL,
> >   .help = N_("needs rawio capability")
> > @@ -574,7 +578,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
> >  *subdriver = NULL, *type = NULL, *mode = NULL,
> >  *iothread = NULL, *cache = NULL, *io = NULL,
> >  *serial = NULL, *straddr = NULL, *wwn = NULL,
> > -*targetbus = NULL;
> > +*targetbus = NULL, *alias = NULL;
> >  struct DiskAddress diskAddr;
> >  bool isFile = false, functionReturn = false;
> >  int ret;
> > @@ -611,6 +615,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
> >  vshCommandOptStringReq(ctl, cmd, "wwn", ) < 0 ||
> >  vshCommandOptStringReq(ctl, cmd, "address", ) < 0 ||
> >  vshCommandOptStringReq(ctl, cmd, "targetbus", ) < 0 ||
> > +vshCommandOptStringReq(ctl, cmd, "alias", ) < 0 ||
> >  vshCommandOptStringReq(ctl, cmd, "sourcetype", ) < 0)
> >  goto cleanup;
> >
> > @@ -680,6 +685,14 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
> >  if (serial)
> >  virBufferAsprintf(, "%s\n", serial);
> >
> > +if (alias) {
> > +if (!STRPREFIX(alias, "ua-")) {
> > +vshError(ctl, _("Custom alias name should start with ua-"));
> > +goto cleanup;
> > +}
>
> I don't think this belongs here. I'd let libvirt report an error. The
> reason for that is to have checks in one place rather than scattered
> around the code. For instance, imagine that one day we lift the
> restriction and let users define alias in a free form. Using this
> version of virsh (and connecting to new libvirtd) they will be unable to
> do so.
> Or the other way round - we allow only certain characters to be in user
> alias. You are not checking them here - you're relying on libvirtd doing
> so.
>
> It is reasonable that libvirtd check the alias name. As I know, currently
libvirtd
will ignore the customized alias not starting with 'ua-'. Will we keep
ignoring
it or report an error in libvirtd?

> The rest looks okay.
>
> Michal
>



-- 
Best regards,
---
Han Han
Quality Engineer
Redhat.

Email: h...@redhat.com
Phone: +861065339333
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 0/2] Add alias support in sub-commands of virsh

[Han Han]

On Mon, Jul 23, 2018 at 6:21 PM, Michal Prívozník 
wrote:

> On 07/15/2018 12:08 PM, Han Han wrote:
> > Fix make syntax-check failure in v1.
> > v1 link: https://www.redhat.com/archives/libvir-list/2018-
> July/msg00929.html
> >
> > Han Han (2):
> >   virsh: Support alias in attach-disk
> >   virsh: Support alias in attach-interface
> >
> >  tools/virsh-domain.c | 30 --
> >  tools/virsh.pod  |  7 +--
> >  2 files changed, 33 insertions(+), 4 deletions(-)
> >
>
> I'm fixing all the issues I've found, ACKing and pushing.
>
> Might be worth providing news.xml entry though. Can you post a patch for
> that please?
>
Sure. The news patch link:
https://www.redhat.com/archives/libvir-list/2018-July/msg01516.html

>
> Michal
>



-- 
Best regards,
---
Han Han
Quality Engineer
Redhat.

Email: h...@redhat.com
Phone: +861065339333
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v1 20/40] util: pci: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:52PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOFREE macro for declaring scalar variables, majority
> of the VIR_FREE calls can be dropped, which in turn leads to
> getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar 
> ---

...

>
> @@ -1278,8 +1247,7 @@ virPCIDeviceUnbindFromStubWithOverride(virPCIDevicePtr 
> dev)
>  static int
>  virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
>  {
> -int ret;
> -char *path;
> +VIR_AUTOFREE(char *) path = NULL;
>
>  /*
>   * Prefer using the device's driver_override interface, falling back
> @@ -1289,12 +1257,9 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
>  return -1;
>
>  if (virFileExists(path))
> -ret = virPCIDeviceUnbindFromStubWithOverride(dev);
> +return virPCIDeviceUnbindFromStubWithOverride(dev);
>  else

Drop the 'else' clause, it's not needed anymore.

...

>  static int
>  virPCIDeviceBindToStub(virPCIDevicePtr dev)
>  {
> -int ret;
> -char *path;
> +VIR_AUTOFREE(char *) path = NULL;
>
>  /*
>   * Prefer using the device's driver_override interface, falling back
> @@ -1511,12 +1464,9 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev)
>  return -1;
>
>  if (virFileExists(path))
> -ret = virPCIDeviceBindToStubWithOverride(dev);
> +return virPCIDeviceBindToStubWithOverride(dev);
>  else
... here too..

...

> @@ -1755,8 +1701,8 @@ virPCIDeviceNew(unsigned int domain,
>  unsigned int function)
>  {
>  virPCIDevicePtr dev;

virPCIDevicePtr ret = NULL;
VIR_AUTOPTR(virPCIDevicePtr) tmp = NULL;

> -char *vendor = NULL;
> -char *product = NULL;
> +VIR_AUTOFREE(char *) vendor = NULL;
> +VIR_AUTOFREE(char *) product = NULL;
>
>  if (VIR_ALLOC(dev) < 0)
>  return NULL;
> @@ -1805,15 +1751,11 @@ virPCIDeviceNew(unsigned int domain,
>
>  VIR_DEBUG("%s %s: initialized", dev->id, dev->name);
>

VIR_STEAL_PTR(ret, dev);

> - cleanup:

Preserve the 'cleanup' label...

> -VIR_FREE(product);
> -VIR_FREE(vendor);
>  return dev;
>
>   error:

Drop 'error' label

>  virPCIDeviceFree(dev);

^this can be dropped too...

> -dev = NULL;
> -goto cleanup;
> +return NULL;

^return ret;

Erik

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

[libvirt] [PATCH] news: Add --alias to virsh attach-disk and attach-interface

[Han Han]

Signed-off-by: Han Han 
---
 docs/news.xml | 9 +
 1 file changed, 9 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 20c2ff7a6f..1e632eb378 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -67,6 +67,15 @@
   only rendering devices within the guest.
 
   
+  
+
+  virsh: Add --alias to attach-disk and attach-interface command.
+
+
+  Add option --alias to set customized device alias name when
+  attach-disk or attach-interface.
+
+  
 
 
 
-- 
2.18.0

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

Re: [libvirt] [PATCH v2 2/5] Forget last daemon/ dir artefacts

[John Ferlan]

On 07/23/2018 04:44 AM, Michal Prívozník wrote:
> On 07/21/2018 02:11 PM, John Ferlan wrote:
>>
>>
>> On 07/12/2018 03:37 AM, Michal Privoznik wrote:
>>> The most important part is LIBVIRTD_PATH env var fix. It is used
>>> in virFileFindResourceFull() from tests. The libvirtd no longer
>>> lives under daemon/.
>>>
>>> Then, libvirtd-fail test was still failing (as expected) but not
>>> because of missing config file but because it was trying to
>>> execute (nonexistent) top_builddir/daemon/libvirtd which
>>> fulfilled expected outcome and thus test did not fail.
>>>
>>> Thirdly, lcov was told to generate coverage for daemon/ dir too.
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>  Makefile.am | 2 +-
>>>  run.in  | 2 +-
>>>  tests/libvirtd-fail | 4 ++--
>>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Makefile.am b/Makefile.am
>>> index 1926e21b7a..709064c6a6 100644
>>> --- a/Makefile.am
>>> +++ b/Makefile.am
>>> @@ -80,7 +80,7 @@ check-access:
>>>  cov: clean-cov
>>> $(MKDIR_P) $(top_builddir)/coverage
>>> $(LCOV) -c -o $(top_builddir)/coverage/libvirt.info.tmp \
>>> - -d $(top_builddir)/src  -d $(top_builddir)/daemon \
>>> + -d $(top_builddir)/src \
>>
>> Since daemon is the former name and this appears to be a clean label
>> target for coverage, perhaps we should keep daemon just to clean up
>> "old" trees...
>>
> 
> No. This is no a clean label. The rule says: in order to make target
> "cov" you need to make target "clean-cov" first as it is dependency. So
> I'm changing the create target not the cleanup. And -d $dir to lcov
> means "include directory $dir to search for .da files" (whatever they
> are - doesn't matter now).
> 
> Michal
> 

Hence the reason I don't like to review Makefile changes ;-)

John

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

Re: [libvirt] [PATCH] qemu: hotplug: Don't leak saved error on failure in qemuHotplugRemoveManagedPR

[John Ferlan]

On 07/23/2018 09:41 AM, Peter Krempa wrote:
> If we'd fail to enter or exit the monitor the saved error would be
> leaked. Introduced in 8498a1e2221 .
> 
> Pointed out by coverity.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_hotplug.c | 14 +-
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 

Tks -

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 07/10] viriscsitest: Test virISCSIConnectionLogin

[John Ferlan]

On 07/23/2018 08:34 AM, Michal Prívozník wrote:
> On 07/23/2018 02:12 PM, John Ferlan wrote:
>>
>>
>> On 07/23/2018 04:01 AM, Michal Prívozník wrote:
>>> On 07/17/2018 09:14 PM, John Ferlan wrote:


 On 07/04/2018 05:23 AM, Michal Privoznik wrote:
> Introduce one basic test that tests the simplest case:
> logging into portal without any IQN.
>
> Signed-off-by: Michal Privoznik 
> ---
>  tests/viriscsitest.c | 46 ++
>  1 file changed, 46 insertions(+)
>
> diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c
> index 3bb3993196..a7287069ba 100644
> --- a/tests/viriscsitest.c
> +++ b/tests/viriscsitest.c
> @@ -94,6 +94,16 @@ static void testIscsiadmCb(const char *const*args,
> args[8] && STREQ(args[8], "nonpersistent") &&
> args[9] == NULL) {
>  ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput));
>>
>> here we do some sort of comparison
> 
> What comparison? The only comparison I see is "args[X] && STREQ(args[X],
> ...)".
> 
> If you're referring to VIR_STRDUP() that is setting the pretended output
> of the iscsiadm command. It's not comparing anything.
> 

Can you just indicate rather than "nada" what the actual deal is here?
That is, in our mocked environment when running this command we don't
expect to get any output from iscsiadm. If this were a real command then
the following would be returned:

...

In my saved iSCSI output file, I have for example:

iscsiadm -m node -T iqn.2013-12.com.example:iscsi-chap-pool -p
192.168.122.1 --login

returning:

Logging in to [iface: default, target:
iqn.2013-12.com.example:iscsi-chap-pool, portal: 192.168.122.1,3260]
(multiple)
Login to [iface: default, target:
iqn.2013-12.com.example:iscsi-chap-pool, portal: 192.168.122.1,3260]
successful.


Hopefully this helps resolves the "confusion" over this.  Like I said
for patch10, no need for a v3, just a diff would be fine, although at
this point it only helps for my really short term memory.

John

>>
> +} else if (args[0] && STREQ(args[0], ISCSIADM) &&
> +   args[1] && STREQ(args[1], "--mode") &&
> +   args[2] && STREQ(args[2], "node") &&
> +   args[3] && STREQ(args[3], "--portal") &&
> +   args[4] && STREQ(args[4], "10.20.30.40:3260,1") &&
> +   args[5] && STREQ(args[5], "--targetname") &&
> +   args[6] && STREQ(args[6], 
> "iqn.2004-06.example:example1:iscsi.test") &&
> +   args[7] && STREQ(args[7], "--login") &&
> +   args[8] == NULL) {
> +/* nada */


 Hmm.. can we place a hold on that R-By - I missed this gem "nada"  -
 why is that?

 If we have *output because we've sent the dryRun, then why not compare?
>>>
>>> I'm not quite sure what you mean. What should be compared?
>>
>> here we do not - instead we just use nada. 
> 
> That is correct because we do need to accept that cmd line but we do not
> need to take any extra step to mimic iscsiadm behaviour we're after.
> 
>> So after reading patch10
>> where nada is again used, I began to doubt/wonder about using it here
>> especially since there are comparisons for all other cases.
>>
>> John
>>
> 
> Michal
> 

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

[libvirt] [PATCH] qemu: hotplug: Don't leak saved error on failure in qemuHotplugRemoveManagedPR

[Peter Krempa]

If we'd fail to enter or exit the monitor the saved error would be
leaked. Introduced in 8498a1e2221 .

Pointed out by coverity.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_hotplug.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index fe703ab4bd..1488f0a7c2 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -354,22 +354,26 @@ qemuHotplugRemoveManagedPR(virQEMUDriverPtr driver,
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
 virErrorPtr orig_err;
-virErrorPreserveLast(_err);
+int ret = -1;

 if (!priv->prDaemonRunning ||
 virDomainDefHasManagedPR(vm->def))
 return 0;

+virErrorPreserveLast(_err);
+
 if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
-return -1;
+goto cleanup;
 ignore_value(qemuMonitorDelObject(priv->mon, 
qemuDomainGetManagedPRAlias()));
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
-return -1;
+goto cleanup;

 qemuProcessKillManagedPRDaemon(vm);
-virErrorRestore(_err);

-return 0;
+ret = 0;
+ cleanup:
+virErrorRestore(_err);
+return ret;
 }


-- 
2.16.2

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

Re: [libvirt] [dbus PATCH v2 04/17] Implement Destroy method for Interface Interface

[Ján Tomko]

On Fri, Jul 20, 2018 at 02:34:48PM -0400, Anya Harter wrote:

Signed-off-by: Anya Harter 
---
data/org.libvirt.Interface.xml |  5 +
src/interface.c| 24 
2 files changed, 29 insertions(+)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [dbus PATCH v2 03/17] Implement Create method for Interface Interface

[Ján Tomko]

On Fri, Jul 20, 2018 at 02:34:47PM -0400, Anya Harter wrote:

Signed-off-by: Anya Harter 
---
data/org.libvirt.Interface.xml |  5 
src/interface.c| 45 ++
2 files changed, 50 insertions(+)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [dbus PATCH v2 02/17] Implement InterfaceDefineXML method for Connect Interface

[Ján Tomko]

On Fri, Jul 20, 2018 at 02:34:46PM -0400, Anya Harter wrote:

Signed-off-by: Anya Harter 
---
data/org.libvirt.Connect.xml |  7 +++
src/connect.c| 30 ++
tests/test_connect.py|  4 
tests/xmldata.py | 12 
4 files changed, 53 insertions(+)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [dbus PATCH v2 01/17] Introduce Interface Interface

[Ján Tomko]

On Fri, Jul 20, 2018 at 02:34:45PM -0400, Anya Harter wrote:

Signed-off-by: Anya Harter 
---
data/Makefile.am   |  1 +
data/org.libvirt.Interface.xml |  7 
src/Makefile.am|  2 ++
src/connect.c  |  6 
src/connect.h  |  1 +
src/interface.c| 65 ++
src/interface.h|  9 +
src/util.c | 35 ++
src/util.h | 15 
9 files changed, 141 insertions(+)
create mode 100644 data/org.libvirt.Interface.xml
create mode 100644 src/interface.c
create mode 100644 src/interface.h



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v1 19/40] util: pci: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:51PM +0530, Sukrit Bhatnagar wrote:
> Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
> src/util/viralloc.h, define a new wrapper around an existing
> cleanup function which will be called when a variable declared
> with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
> viralloc.h include, since that has moved from the source module into
> the header.
>
> When variables of type virPCIDevicePtr and virPCIEDeviceInfoPtr
> are declared using VIR_AUTOPTR, the functions virPCIDeviceFree
> and virPCIEDeviceInfoFree, respectively, will be run automatically
> on them when they go out of scope.
>
> Signed-off-by: Sukrit Bhatnagar 
> ---

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v1 18/40] util: hook: use VIR_AUTOPTR for aggregate types

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:50PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOPTR macro for declaring aggregate pointer variables,
> majority of the calls to *Free functions can be dropped, which
> in turn leads to getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar 
> ---

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v1 17/40] util: hook: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:49PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOFREE macro for declaring scalar variables, majority
> of the VIR_FREE calls can be dropped, which in turn leads to
> getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar 
> ---
>  src/util/virhook.c | 16 +---
>  1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/src/util/virhook.c b/src/util/virhook.c
> index facd74a..51f0eb5 100644
> --- a/src/util/virhook.c
> +++ b/src/util/virhook.c
> @@ -122,8 +122,7 @@ static int virHooksFound = -1;
>  static int
>  virHookCheck(int no, const char *driver)
>  {
> -char *path;
> -int ret;
> +VIR_AUTOFREE(char *) path = NULL;
>
>  if (driver == NULL) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -139,18 +138,15 @@ virHookCheck(int no, const char *driver)
>  }
>
>  if (!virFileExists(path)) {
> -ret = 0;
>  VIR_DEBUG("No hook script %s", path);
> +return 0;
>  } else if (!virFileIsExecutable(path)) {
> -ret = 0;
>  VIR_WARN("Non-executable hook script %s", path);
> +return 0;
>  } else {
> -ret = 1;
>  VIR_DEBUG("Found hook script %s", path);
> +return 1;

^this should be reworked in the following way:

if (!virFileExists) {
VIR_DEBUG;
return 0;
}

if (!virFileIsExecutable) {
VIR_DEBUG;
return 0;
}

VIR_DEBUG;
return 1;

Otherwise looks okay.
Reviewed-by: Erik Skultety 

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

Re: [libvirt] [jenkins-ci PATCH] guests: Make mappings more future proof

[Andrea Bolognani]

On Tue, 2018-07-10 at 14:01 +0200, Andrea Bolognani wrote:
> On Wed, 2018-06-13 at 15:01 +0200, Andrea Bolognani wrote:
> > We can reasonably expect the next major release of CentOS
> > to be somewhat close to current Fedora releases in terms of
> > packaging, and to ditch the soon-to-be-unsupported Python 2
> > in favor of Python 3.
> > 
> > Rewrite some of the mappings based on these expectations.
> > 
> > Signed-off-by: Andrea Bolognani 
> > ---
> >  guests/vars/mappings.yml | 18 +-
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> *** PING HERE ***

Coming up to six weeks on the list... Can anyone please take a look
at this so I can finally drop the corresponding local branch? :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v1 16/40] util: firewall: use VIR_AUTOPTR for aggregate types

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:48PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOPTR macro for declaring aggregate pointer variables,
> majority of the calls to *Free functions can be dropped, which
> in turn leads to getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar 
> ---

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v2 10/10] viriscsitest: Extend virISCSIConnectionLogin test

[John Ferlan]

On 07/23/2018 04:01 AM, Michal Prívozník wrote:
> On 07/17/2018 09:15 PM, John Ferlan wrote:
>>
>>
>> On 07/04/2018 05:23 AM, Michal Privoznik wrote:
>>> Extend this existing test so that a case when IQN is provided is
>>> tested too. Since a special iSCSI interface is created and its
>>> name is randomly generated at runtime we need to link with
>>> virrandommock to have predictable names.
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>  tests/viriscsitest.c | 75 
>>> ++--
>>>  1 file changed, 73 insertions(+), 2 deletions(-)
>>>
>>
>> Should testIscsiadmCbData initialization get { false, false } now (and I
>> should have mention in patch 9 that :
>>
>> +struct testIscsiadmCbData cbData = { 0 };
>>
>> should be
>>
>> +struct testIscsiadmCbData cbData = { false };>
>> I know that 0 and false are synonymous, but syntactical correctness is
>> in play here ;-)
> 
> No. So { 0 } is basically a shortcut for:
> 
> struct testIscsiadmCbData cbData;
> 
> memset(, 0, sizeof(cbData));

Oh yeah, right  for literal interpretation of data.

> 
> It has nothing to do with the struct having only two bools (for now). It
> is used widely in our code, because it has one great advantage: even if
> we add/remove/rearrange items in the struct it is always going to be
> zeroed out whereas if I initialized it like this:
> 
> struct testIscsiadmCbData cbData = {false, false};
> 
> and then somebody would append yet another member to the struct they
> would either:
> 
> a) have to change all the lines where the struct is initialized
> (possibly touching functions that has nothing to do with actual change), or
> b) forget to initialize it completely.
> 
> 
> TL;DR It's just coincidence that the first member is a bool.
> 
>>
>> I also think you're doing two things here:
>>
>> 1. Generating a dryRun output for "existing" test without initiator.iqn
>>
>> 2. Adding tests to test initiator.iqn
>>
> 
> Sure. That's how login works. Firstly it lists output of iscsi
> interfaces, then it creates a new one and then use that to log in. I
> don't see a problem here.
> 

OK, right... Maybe it would have been useful from the literal review POV
to be reminded of this...  While sometimes it's a pain to document the
reason for something - it perhaps prevents this back and forth later on
especially when it's "unique case" code paths.  IIQN isn't really well
documented by any stretch...

>>> diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c
>>> index c6e0e3b8ff..55889d04a3 100644
>>> --- a/tests/viriscsitest.c
>>> +++ b/tests/viriscsitest.c
>>> @@ -60,8 +60,19 @@ const char *iscsiadmSendtargetsOutput =
>>>  "10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar\n"
>>>  "10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven\n";
>>>  
>>> +const char *iscsiadmIfaceDefaultOutput =
>>> +"default tcp\n"
>>> +"iser iser\n";
>>> +
>>> +const char *iscsiadmIfaceIfaceOutput =
>>> +"default tcp\n"
>>> +"iser iser\n"
>>> +"libvirt-iface-03020100 
>>> tcpiqn.2004-06.example:example1:initiator\n";
>>> +
>>> +
>>>  struct testIscsiadmCbData {
>>>  bool output_version;
>>> +bool iface_created;
>>>  };
>>>  
>>>  static void testIscsiadmCb(const char *const*args,
>>> @@ -103,6 +114,62 @@ static void testIscsiadmCb(const char *const*args,
>>> args[7] && STREQ(args[7], "--login") &&
>>> args[8] == NULL) {
>>>  /* nada */
>>
>> Is this "nada"
> 
> Yes, this is "nada" ;-)
> 

Maybe the explanation as to why it's nada and not just leaving nada
there would have helped.

In some instances we're doing output comparison (eventually) and in
others we're not. In the long run it's mainly a matter of being reminded
what the processing is and what (if any) the expected output is.
Sometimes that expected output only occurs on the next query, IIRC.

Creating IIQN's or iSCSI targets is not something I do all that often -
I wonder if you came back to this code 6 months or longer from now
whether you'd (instantly) recall what type of output was generated ;-).
If you would then your short term memory is better than mine! I have to
keep some cheat notes for iSCSI processing letting me know which
commands to use and essentially what they return so I know they
completed as expected.

BTW: This is my favorite from f28 recently:

# iscsiadm -m session -P 3
iSCSI Transport Class version 2.0-870
version 6.2.0.874
Segmentation fault (core dumped)
#

Oy!  at least it's getting further now than at one time where the first
line wasn't even printed.

Don't even get me started about the 'targetcli' command which I find to
be absolutely awful. It's supposed to be better, but I feel like I
stepped back into the 1990's and I'm using OpenVMS NCP/NCL (that'd
require some googling for you I'm sure, but suffice to say it was an
awful^^n syntax).

>>> +} else if (args[0] && STREQ(args[0], ISCSIADM) &&
>>> +   

Re: [libvirt] [PATCH v1 15/40] util: firewall: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:47PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOFREE macro for declaring scalar variables, majority
> of the VIR_FREE calls can be dropped, which in turn leads to
> getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar 
> ---
>  src/util/virfirewall.c | 16 +---
>  1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
> index dfd792f..b4a4d06 100644
> --- a/src/util/virfirewall.c
> +++ b/src/util/virfirewall.c
> @@ -511,7 +511,7 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall,
>   virFirewallRulePtr rule,
>   const char *fmt, ...)
>  {
> -char *arg;
> +VIR_AUTOFREE(char *) arg = NULL;
>  va_list list;
>
>  VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, rule);
> @@ -525,13 +525,11 @@ void virFirewallRuleAddArgFormat(virFirewallPtr 
> firewall,
>
>  va_end(list);
>
> -VIR_FREE(arg);
>  return;
>
>   no_memory:
>  firewall->err = ENOMEM;
>  va_end(list);
> -VIR_FREE(arg);

There could be an additional patch replacing the no_memory label with 'cleanup'
with the obvious adjustments.

With an additional patch in place:
Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v1 14/40] util: firewall: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:46PM +0530, Sukrit Bhatnagar wrote:
> Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
> src/util/viralloc.h, define a new wrapper around an existing
> cleanup function which will be called when a variable declared
> with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
> viralloc.h include, since that has moved from the source module into
> the header.
>
> When a variable of type virFirewallPtr is declared using
> VIR_AUTOPTR, the function virFirewallFree will be run
> automatically on it when it goes out of scope.
>
> Signed-off-by: Sukrit Bhatnagar 
> ---

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v1 13/40] util: mdev: use VIR_AUTOPTR for aggregate types

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:45PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOPTR macro for declaring aggregate pointer variables,
> majority of the calls to *Free functions can be dropped, which
> in turn leads to getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar 
> ---
...

> @@ -494,23 +491,22 @@ int
>  virMediatedDeviceTypeReadAttrs(const char *sysfspath,
> virMediatedDeviceTypePtr *type)
>  {
> -int ret = -1;
> -virMediatedDeviceTypePtr tmp = NULL;
> +VIR_AUTOPTR(virMediatedDeviceType) tmp = NULL;
>
>  #define MDEV_GET_SYSFS_ATTR(attr, dst, cb, optional) \
>  do { \
>  int rc; \
>  if ((rc = cb(dst, "%s/%s", sysfspath, attr)) < 0) { \
>  if (rc != -2 || !optional) \
> -goto cleanup; \
> +return 0; \

Why are you returning success ^here?

With that changed:
Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v1 12/40] util: mdev: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:44PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOFREE macro for declaring scalar variables, majority
> of the VIR_FREE calls can be dropped, which in turn leads to
> getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar 
> ---
...

> @@ -218,34 +208,30 @@ virMediatedDeviceGetPath(virMediatedDevicePtr dev)
>  char *
>  virMediatedDeviceGetIOMMUGroupDev(const char *uuidstr)
>  {
> -char *result_path = NULL;
> -char *iommu_path = NULL;
> +VIR_AUTOFREE(char *) result_path = NULL;
> +VIR_AUTOFREE(char *) iommu_path = NULL;
> +VIR_AUTOFREE(char *) dev_path = virMediatedDeviceGetSysfsPath(uuidstr);
>  char *vfio_path = NULL;
> -char *dev_path = virMediatedDeviceGetSysfsPath(uuidstr);
>
>  if (!dev_path)
>  return NULL;
>
>  if (virAsprintf(_path, "%s/iommu_group", dev_path) < 0)
> -goto cleanup;
> +return NULL;
>
>  if (!virFileExists(iommu_path)) {
>  virReportSystemError(errno, _("failed to access '%s'"), iommu_path);
> -goto cleanup;
> +return NULL;
>  }
>
>  if (virFileResolveLink(iommu_path, _path) < 0) {
>  virReportSystemError(errno, _("failed to resolve '%s'"), iommu_path);
> -goto cleanup;
> +return NULL;
>  }
>
>  if (virAsprintf(_path, "/dev/vfio/%s", last_component(result_path)) 
> < 0)
> -goto cleanup;
> +return vfio_path;

I'd rather you returned NULL ^here.

With that:
Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v1 11/40] util: mdev: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:43PM +0530, Sukrit Bhatnagar wrote:
> Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
> src/util/viralloc.h, define a new wrapper around an existing
> cleanup function which will be called when a variable declared
> with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
> viralloc.h include, since that has moved from the source module into
> the header.
>
> When variables of type virMediatedDevicePtr and virMediatedDeviceTypePtr
> are declared using VIR_AUTOPTR, the functions virMediatedDeviceFree
> and virMediatedDeviceTypeFree, respectively, will be run automatically
> on them when they go out of scope.
>
> Signed-off-by: Sukrit Bhatnagar 
> ---

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [python PATCH 0/3] rpm: misc updates to modernize spec

[Andrea Bolognani]

On Mon, 2018-07-23 at 12:15 +0100, Daniel P. Berrangé wrote:
> 
> Daniel P. Berrangé (3):
>   rpm: use the versioned python2 macro names
>   rpm: add BuildRequires on gcc
>   rpm: update min required rhel/fedora
> 
>  libvirt-python.spec.in | 23 ---
>  1 file changed, 12 insertions(+), 11 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v1 10/40] util: cgroup: use VIR_AUTOPTR for aggregate types

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:42PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOPTR macro for declaring aggregate pointer variables,
> majority of the calls to *Free functions can be dropped, which
> in turn leads to getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar 
> ---
...

> @@ -3505,23 +3470,18 @@ int
>  virCgroupKill(virCgroupPtr group, int signum)
>  {
>  VIR_DEBUG("group=%p path=%s signum=%d", group, group->path, signum);
> -int ret;
>  /* The 'tasks' file in cgroups can contain duplicated
>   * pids, so we use a hash to track which we've already
>   * killed.
>   */
> -virHashTablePtr pids = virHashCreateFull(100,
> +VIR_AUTOPTR(virHashTable) pids = virHashCreateFull(100,
>   NULL,
>   virCgroupPidCode,
>   virCgroupPidEqual,
>   virCgroupPidCopy,
>   NULL);
>

Code misalignment...

...

> @@ -3596,20 +3554,15 @@ virCgroupKillRecursiveInternal(virCgroupPtr group,
>  int
>  virCgroupKillRecursive(virCgroupPtr group, int signum)
>  {
> -int ret;
>  VIR_DEBUG("group=%p path=%s signum=%d", group, group->path, signum);
> -virHashTablePtr pids = virHashCreateFull(100,
> +VIR_AUTOPTR(virHashTable) pids = virHashCreateFull(100,
>   NULL,
>   virCgroupPidCode,
>   virCgroupPidEqual,
>   virCgroupPidCopy,
>   NULL);

...here too...

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v2 07/10] viriscsitest: Test virISCSIConnectionLogin

[Michal Prívozník]

On 07/23/2018 02:12 PM, John Ferlan wrote:
> 
> 
> On 07/23/2018 04:01 AM, Michal Prívozník wrote:
>> On 07/17/2018 09:14 PM, John Ferlan wrote:
>>>
>>>
>>> On 07/04/2018 05:23 AM, Michal Privoznik wrote:
 Introduce one basic test that tests the simplest case:
 logging into portal without any IQN.

 Signed-off-by: Michal Privoznik 
 ---
  tests/viriscsitest.c | 46 ++
  1 file changed, 46 insertions(+)

 diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c
 index 3bb3993196..a7287069ba 100644
 --- a/tests/viriscsitest.c
 +++ b/tests/viriscsitest.c
 @@ -94,6 +94,16 @@ static void testIscsiadmCb(const char *const*args,
 args[8] && STREQ(args[8], "nonpersistent") &&
 args[9] == NULL) {
  ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput));
> 
> here we do some sort of comparison

What comparison? The only comparison I see is "args[X] && STREQ(args[X],
...)".

If you're referring to VIR_STRDUP() that is setting the pretended output
of the iscsiadm command. It's not comparing anything.

> 
 +} else if (args[0] && STREQ(args[0], ISCSIADM) &&
 +   args[1] && STREQ(args[1], "--mode") &&
 +   args[2] && STREQ(args[2], "node") &&
 +   args[3] && STREQ(args[3], "--portal") &&
 +   args[4] && STREQ(args[4], "10.20.30.40:3260,1") &&
 +   args[5] && STREQ(args[5], "--targetname") &&
 +   args[6] && STREQ(args[6], 
 "iqn.2004-06.example:example1:iscsi.test") &&
 +   args[7] && STREQ(args[7], "--login") &&
 +   args[8] == NULL) {
 +/* nada */
>>>
>>>
>>> Hmm.. can we place a hold on that R-By - I missed this gem "nada"  -
>>> why is that?
>>>
>>> If we have *output because we've sent the dryRun, then why not compare?
>>
>> I'm not quite sure what you mean. What should be compared?
> 
> here we do not - instead we just use nada. 

That is correct because we do need to accept that cmd line but we do not
need to take any extra step to mimic iscsiadm behaviour we're after.

> So after reading patch10
> where nada is again used, I began to doubt/wonder about using it here
> especially since there are comparisons for all other cases.
> 
> John
> 

Michal

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

Re: [libvirt] [PATCH] vmx: add support for hostonly interfaces

[Ján Tomko]

On Wed, Jul 11, 2018 at 03:26:45PM +, Javier Uruen Val wrote:

Vmware fusion allows you to configure a network interface
as "hostonly". This uses a private network that is not
normally accessible from the physical networks on the
Mac/PC.



So this is just for guest <-> host communication and the guest cannot
communicate with the world?


We map "hostonly" in the vmx configuration to "network"
in the domain configuration. Some comments in vmx.c
seem to point towards using this mapping and not other
such as "internal" or something else.



Which comments?

If you cannot use a network defined in libvirt's network driver with the
interface, I don't think type='network' is the right match.

Sadly, we don't seem to have documentation for type='internal'.


Added tests and configuration files from actual vmx
machines configured with "hostonly".

Signed-off-by: Javier Uruen Val 
---
src/vmx/vmx.c |  19 ++-
.../vmx2xml-fusion-in-the-wild-2.vmx  | 115 ++
.../vmx2xml-fusion-in-the-wild-2.xml  |  34 ++
tests/vmx2xmltest.c   |   1 +
.../xml2vmx-fusion-in-the-wild-2.vmx  |  36 ++
.../xml2vmx-fusion-in-the-wild-2.xml  |  31 +
tests/xml2vmxtest.c   |   1 +
7 files changed, 231 insertions(+), 6 deletions(-)
create mode 100644 tests/vmx2xmldata/vmx2xml-fusion-in-the-wild-2.vmx
create mode 100644 tests/vmx2xmldata/vmx2xml-fusion-in-the-wild-2.xml
create mode 100755 tests/xml2vmxdata/xml2vmx-fusion-in-the-wild-2.vmx
create mode 100644 tests/xml2vmxdata/xml2vmx-fusion-in-the-wild-2.xml



And some changes to docs/formatdomain.html.in to tell people how to use
it would be beneficial.

Jano


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

[libvirt] [PATCH v2] libvirt_iohelper: record the libvirt_iohelper's error message at virFileWrapperFdFree

[xinhua.Cao]

Currently iohelper's error log is recorded in virFileWrapperFdClose.
In qemuDomainSaveMemory, it usually fails at qemuMigrationSrcToFile,
and then goto cleanup, so the iohelper error log is not recorded,
and so is the another placement. We now record the error log of
iohelper by move it to the virFileWrapperFdFree record.
And for the virCommandWait() - we should call virCommandAbort() in
virFileWrapperFdFree() to make sure no process is left behind. If
virCommandWait() is called then virCommandAbort() is a NO-OP. If it
isn't, then Abort() will kill iohelper. Thanks for Michal for this
advice

Signed-off-by: xinhua.Cao 
---
 src/util/virfile.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index 1faeebb..590fc0e 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -330,9 +330,6 @@ virFileWrapperFdClose(virFileWrapperFdPtr wfd)
 return 0;
 
 ret = virCommandWait(wfd->cmd, NULL);
-if (wfd->err_msg && *wfd->err_msg)
-VIR_WARN("iohelper reports: %s", wfd->err_msg);
-
 return ret;
 }
 
@@ -351,6 +348,10 @@ virFileWrapperFdFree(virFileWrapperFdPtr wfd)
 if (!wfd)
 return;
 
+if (wfd->err_msg && *wfd->err_msg)
+VIR_WARN("iohelper reports: %s", wfd->err_msg);
+
+virCommandAbort(wfd->cmd);
 VIR_FREE(wfd->err_msg);
 
 virCommandFree(wfd->cmd);
-- 
2.8.3


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

Re: [libvirt] [PATCH 24/24] tests: qemuxml2argv: Add CAPS_LATEST version of security-related tests

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:51:16PM +0200, Peter Krempa wrote:

'disk-network-source-auth' and 'disk-netowrk-tlsx509'


network



Signed-off-by: Peter Krempa 
---
.../disk-network-source-auth.x86_64-latest.args| 47 +
.../disk-network-tlsx509.x86_64-latest.args| 59 ++
tests/qemuxml2argvtest.c   |  2 +
3 files changed, 108 insertions(+)
create mode 100644 
tests/qemuxml2argvdata/disk-network-source-auth.x86_64-latest.args
create mode 100644 
tests/qemuxml2argvdata/disk-network-tlsx509.x86_64-latest.args



Reviewed-by: Ján Tomko 

Jano


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 07/10] viriscsitest: Test virISCSIConnectionLogin

[John Ferlan]

On 07/23/2018 04:01 AM, Michal Prívozník wrote:
> On 07/17/2018 09:14 PM, John Ferlan wrote:
>>
>>
>> On 07/04/2018 05:23 AM, Michal Privoznik wrote:
>>> Introduce one basic test that tests the simplest case:
>>> logging into portal without any IQN.
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>  tests/viriscsitest.c | 46 ++
>>>  1 file changed, 46 insertions(+)
>>>
>>> diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c
>>> index 3bb3993196..a7287069ba 100644
>>> --- a/tests/viriscsitest.c
>>> +++ b/tests/viriscsitest.c
>>> @@ -94,6 +94,16 @@ static void testIscsiadmCb(const char *const*args,
>>> args[8] && STREQ(args[8], "nonpersistent") &&
>>> args[9] == NULL) {
>>>  ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput));

here we do some sort of comparison

>>> +} else if (args[0] && STREQ(args[0], ISCSIADM) &&
>>> +   args[1] && STREQ(args[1], "--mode") &&
>>> +   args[2] && STREQ(args[2], "node") &&
>>> +   args[3] && STREQ(args[3], "--portal") &&
>>> +   args[4] && STREQ(args[4], "10.20.30.40:3260,1") &&
>>> +   args[5] && STREQ(args[5], "--targetname") &&
>>> +   args[6] && STREQ(args[6], 
>>> "iqn.2004-06.example:example1:iscsi.test") &&
>>> +   args[7] && STREQ(args[7], "--login") &&
>>> +   args[8] == NULL) {
>>> +/* nada */
>>
>>
>> Hmm.. can we place a hold on that R-By - I missed this gem "nada"  -
>> why is that?
>>
>> If we have *output because we've sent the dryRun, then why not compare?
> 
> I'm not quite sure what you mean. What should be compared?

here we do not - instead we just use nada.  So after reading patch10
where nada is again used, I began to doubt/wonder about using it here
especially since there are comparisons for all other cases.

John

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

Re: [libvirt] [PATCH 23/24] tests: qemuxml2argv: Add CAPS_LATEST version of 'disk-network-sheepdog'

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:51:15PM +0200, Peter Krempa wrote:

Signed-off-by: Peter Krempa 
---
.../disk-network-sheepdog.x86_64-latest.args   | 35 ++
tests/qemuxml2argvtest.c   |  1 +
2 files changed, 36 insertions(+)
create mode 100644 
tests/qemuxml2argvdata/disk-network-sheepdog.x86_64-latest.args



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 22/24] tests: qemuxml2argv: Add CAPS_LATEST version of 'disk-network-gluster'

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:51:14PM +0200, Peter Krempa wrote:

Signed-off-by: Peter Krempa 
---
.../disk-network-gluster.x86_64-latest.args| 44 ++
tests/qemuxml2argvtest.c   |  1 +
2 files changed, 45 insertions(+)
create mode 100644 
tests/qemuxml2argvdata/disk-network-gluster.x86_64-latest.args



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 21/24] tests: qemuxml2argv: Add CAPS_LATEST version of 'disk-readonly' and 'disk-shared'

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:51:13PM +0200, Peter Krempa wrote:

Signed-off-by: Peter Krempa 
---
.../disk-readonly-disk.x86_64-latest.args  | 34 
.../disk-shared.x86_64-latest.args | 37 ++
tests/qemuxml2argvtest.c   |  2 ++
3 files changed, 73 insertions(+)
create mode 100644 tests/qemuxml2argvdata/disk-readonly-disk.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/disk-shared.x86_64-latest.args



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v1 09/40] util: cgroup: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:41PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOFREE macro for declaring scalar variables, majority
> of the VIR_FREE calls can be dropped, which in turn leads to
> getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar 
> ---
...

> @@ -1893,9 +1821,11 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group,
>  long long *requests_write)
>  {
>  long long stats_val;
> -char *str1 = NULL, *str2 = NULL, *p1, *p2;
> +VIR_AUTOFREE(char *) str1 = NULL;
> +VIR_AUTOFREE(char *) str2 = NULL;
> +char *p1;
> +char *p2;

Could have initialized ^these 2 as well for that matter...

...
>
>
> @@ -2002,9 +1927,12 @@ virCgroupGetBlkioIoDeviceServiced(virCgroupPtr group,
>long long *requests_read,
>long long *requests_write)
>  {
> -char *str1 = NULL, *str2 = NULL, *str3 = NULL, *p1, *p2;
> +VIR_AUTOFREE(char *) str1 = NULL;
> +VIR_AUTOFREE(char *) str2 = NULL;
> +VIR_AUTOFREE(char *) str3 = NULL;
> +char *p1;
> +char *p2;

...here too...

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH 20/24] tests: qemuxml2argv: Add CAPS_LATEST version of 'disk-floppy' and 'floppy-drive-fat'

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:51:12PM +0200, Peter Krempa wrote:

Signed-off-by: Peter Krempa 
---
.../disk-floppy.x86_64-latest.args | 35 ++
.../floppy-drive-fat.x86_64-latest.args| 33 
tests/qemuxml2argvtest.c   |  2 ++
3 files changed, 70 insertions(+)
create mode 100644 tests/qemuxml2argvdata/disk-floppy.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/floppy-drive-fat.x86_64-latest.args



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 19/24] tests: qemu: Remove pointless 'disks-many' test

[Ján Tomko]

s/disks/disk/

On Thu, Jul 19, 2018 at 05:51:11PM +0200, Peter Krempa wrote:

We have several cases when a VM has multiple disks in the test files so
having another one without any interresting configuration is not


interesting


necessary.

Signed-off-by: Peter Krempa 
---
tests/qemuxml2argvdata/disk-many.args  | 32 -
tests/qemuxml2argvdata/disk-many.xml   | 48 ---
tests/qemuxml2argvtest.c   |  1 -
tests/qemuxml2xmloutdata/disk-many.xml | 52 --
tests/qemuxml2xmltest.c|  1 -
5 files changed, 134 deletions(-)
delete mode 100644 tests/qemuxml2argvdata/disk-many.args
delete mode 100644 tests/qemuxml2argvdata/disk-many.xml
delete mode 100644 tests/qemuxml2xmloutdata/disk-many.xml



wow, many disk, such test

Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 18/24] tests: qemuxml2argv: Unify testing of 'disk-network-rbd'

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:51:10PM +0200, Peter Krempa wrote:

Move the authentication and ipv6 cases into the main test file. To allow
removal of the separate testing of the secure credential passing via the
'secret' object in qemu, use the DO_TEST_CAPS_VER macro with version
2.5.0 when the secret object is not supported by qemu.

Signed-off-by: Peter Krempa 
---
.../disk-network-rbd-auth-AES.args | 47 -
.../qemuxml2argvdata/disk-network-rbd-auth-AES.xml | 55 ---
tests/qemuxml2argvdata/disk-network-rbd-auth.args  | 32 
tests/qemuxml2argvdata/disk-network-rbd-auth.xml   | 42 ---
tests/qemuxml2argvdata/disk-network-rbd-ipv6.args  | 31 ---
tests/qemuxml2argvdata/disk-network-rbd-ipv6.xml   | 40 --
tests/qemuxml2argvdata/disk-network-rbd.args   | 44 
.../disk-network-rbd.x86_64-2.5.0.args | 55 +++
.../disk-network-rbd.x86_64-latest.args| 61 ++
tests/qemuxml2argvdata/disk-network-rbd.xml| 28 +++---
tests/qemuxml2argvtest.c   |  9 ++--
tests/qemuxml2xmloutdata/disk-network-rbd-auth.xml | 47 -
tests/qemuxml2xmloutdata/disk-network-rbd-ipv6.xml | 45 
tests/qemuxml2xmloutdata/disk-network-rbd.xml  | 30 ---
tests/qemuxml2xmltest.c|  2 -
15 files changed, 165 insertions(+), 403 deletions(-)
delete mode 100644 tests/qemuxml2argvdata/disk-network-rbd-auth-AES.args
delete mode 100644 tests/qemuxml2argvdata/disk-network-rbd-auth-AES.xml
delete mode 100644 tests/qemuxml2argvdata/disk-network-rbd-auth.args
delete mode 100644 tests/qemuxml2argvdata/disk-network-rbd-auth.xml
delete mode 100644 tests/qemuxml2argvdata/disk-network-rbd-ipv6.args
delete mode 100644 tests/qemuxml2argvdata/disk-network-rbd-ipv6.xml
delete mode 100644 tests/qemuxml2argvdata/disk-network-rbd.args
create mode 100644 tests/qemuxml2argvdata/disk-network-rbd.x86_64-2.5.0.args
create mode 100644 tests/qemuxml2argvdata/disk-network-rbd.x86_64-latest.args
delete mode 100644 tests/qemuxml2xmloutdata/disk-network-rbd-auth.xml
delete mode 100644 tests/qemuxml2xmloutdata/disk-network-rbd-ipv6.xml



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 17/24] tests: qemu: Remove pointless 'disk-networ-ceph-env' test

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:51:09PM +0200, Peter Krempa wrote:

The xml2argv variant was unused. The xml2xml variant is redundant in
other tests for RBD.

Signed-off-by: Peter Krempa 
---
.../disk-network-rbd-ceph-env.args | 25 
.../qemuxml2argvdata/disk-network-rbd-ceph-env.xml | 39 ---
.../disk-network-rbd-ceph-env.xml  | 44 --
tests/qemuxml2xmltest.c|  1 -
4 files changed, 109 deletions(-)
delete mode 100644 tests/qemuxml2argvdata/disk-network-rbd-ceph-env.args
delete mode 100644 tests/qemuxml2argvdata/disk-network-rbd-ceph-env.xml
delete mode 100644 tests/qemuxml2xmloutdata/disk-network-rbd-ceph-env.xml



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 16/24] tests: qemuxml2argv: Add 'CAPS_LATEST' version of 'disk-network-nbd'

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:51:08PM +0200, Peter Krempa wrote:

Signed-off-by: Peter Krempa 
---
.../disk-network-nbd.x86_64-latest.args| 46 ++
tests/qemuxml2argvtest.c   |  1 +
2 files changed, 47 insertions(+)
create mode 100644 tests/qemuxml2argvdata/disk-network-nbd.x86_64-latest.args



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 15/24] tests: qemu: Unify nbd disk source testing

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:51:07PM +0200, Peter Krempa wrote:

Signed-off-by: Peter Krempa 
---
.../qemuxml2argvdata/disk-network-nbd-export.args  | 30 
tests/qemuxml2argvdata/disk-network-nbd-export.xml | 37 ---
.../disk-network-nbd-ipv6-export.args  | 29 ---
.../disk-network-nbd-ipv6-export.xml   | 37 ---
tests/qemuxml2argvdata/disk-network-nbd-ipv6.args  | 29 ---
tests/qemuxml2argvdata/disk-network-nbd-ipv6.xml   | 37 ---
tests/qemuxml2argvdata/disk-network-nbd-unix.args  | 30 
tests/qemuxml2argvdata/disk-network-nbd-unix.xml   | 37 ---
tests/qemuxml2argvdata/disk-network-nbd.args   | 18 --
tests/qemuxml2argvdata/disk-network-nbd.xml| 34 ++
tests/qemuxml2argvtest.c   |  4 ---
.../qemuxml2xmloutdata/disk-network-nbd-export.xml | 42 --
.../disk-network-nbd-ipv6-export.xml   | 42 --
tests/qemuxml2xmloutdata/disk-network-nbd-ipv6.xml | 42 --
tests/qemuxml2xmloutdata/disk-network-nbd-unix.xml | 42 --
tests/qemuxml2xmloutdata/disk-network-nbd.xml  | 38 
tests/qemuxml2xmltest.c|  4 ---
17 files changed, 75 insertions(+), 457 deletions(-)
delete mode 100644 tests/qemuxml2argvdata/disk-network-nbd-export.args
delete mode 100644 tests/qemuxml2argvdata/disk-network-nbd-export.xml
delete mode 100644 tests/qemuxml2argvdata/disk-network-nbd-ipv6-export.args
delete mode 100644 tests/qemuxml2argvdata/disk-network-nbd-ipv6-export.xml
delete mode 100644 tests/qemuxml2argvdata/disk-network-nbd-ipv6.args
delete mode 100644 tests/qemuxml2argvdata/disk-network-nbd-ipv6.xml
delete mode 100644 tests/qemuxml2argvdata/disk-network-nbd-unix.args
delete mode 100644 tests/qemuxml2argvdata/disk-network-nbd-unix.xml
delete mode 100644 tests/qemuxml2xmloutdata/disk-network-nbd-export.xml
delete mode 100644 tests/qemuxml2xmloutdata/disk-network-nbd-ipv6-export.xml
delete mode 100644 tests/qemuxml2xmloutdata/disk-network-nbd-ipv6.xml
delete mode 100644 tests/qemuxml2xmloutdata/disk-network-nbd-unix.xml



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH] libvirt_iohelper: record the libvirt_iohelper's error message at virFileWrapperFdFree

[xinhua.Cao]

thanks very much, I will make v2 patch soon.


在 2018/7/23 18:37, Michal Prívozník 写道:

On 07/22/2018 03:40 AM, xinhua.Cao wrote:

Currently iohelper's error log is recorded in virFileWrapperFdClose.
In qemuDomainSaveMemory, it usually fails at qemuMigrationSrcToFile,
and then goto cleanup, so the iohelper error log is not recorded,
and so is the another placement. We now record the error log of
iohelper by move it to the virFileWrapperFdFree record.
There is another problem here, that is, virCommandWait is also not
called, but I can't evaluate this impact. So no changes have been
made here.
---
  src/util/virfile.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index 1faeebb..30456ab 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -330,9 +330,6 @@ virFileWrapperFdClose(virFileWrapperFdPtr wfd)
  return 0;
  
  ret = virCommandWait(wfd->cmd, NULL);

-if (wfd->err_msg && *wfd->err_msg)
-VIR_WARN("iohelper reports: %s", wfd->err_msg);
-
  return ret;
  }
  
@@ -351,6 +348,9 @@ virFileWrapperFdFree(virFileWrapperFdPtr wfd)

  if (!wfd)
  return;
  
+if (wfd->err_msg && *wfd->err_msg)

+VIR_WARN("iohelper reports: %s", wfd->err_msg);
+
  VIR_FREE(wfd->err_msg);
  
  virCommandFree(wfd->cmd);



Makes sense. However, your patch lacks Signed-off-By line so I can't
push it. It's required according to contributor guidelines:

https://libvirt.org/hacking.html

And for the virCommandWait() - we should call virCommandAbort() in
virFileWrapperFdFree() to make sure no process is left behind. If
virCommandWait() is called then virCommandAbort() is a NO-OP. If it
isn't, then Abort() will kill iohelper.

Michal

.




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

Re: [libvirt] [PATCH 14/24] tests: qemuxml2argv: Add 'CAPS_LATEST' version of 'disk-network-iscsi'

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:51:06PM +0200, Peter Krempa wrote:

Signed-off-by: Peter Krempa 
---
.../disk-network-iscsi.x86_64-latest.args  | 63 ++
tests/qemuxml2argvtest.c   |  1 +
2 files changed, 64 insertions(+)
create mode 100644 tests/qemuxml2argvdata/disk-network-iscsi.x86_64-latest.args



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 13/24] tests: qemu: Unify iscsi disk source testing

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:51:05PM +0200, Peter Krempa wrote:

Move various different iSCSI configuration into one test file.

Signed-off-by: Peter Krempa 
---
.../qemuxml2argvdata/disk-network-iscsi-auth.args  | 34 
tests/qemuxml2argvdata/disk-network-iscsi-auth.xml | 43 
tests/qemuxml2argvdata/disk-network-iscsi-lun.args | 29 -
tests/qemuxml2argvdata/disk-network-iscsi-lun.xml  | 28 -
tests/qemuxml2argvdata/disk-network-iscsi.args | 21 --
tests/qemuxml2argvdata/disk-network-iscsi.xml  | 28 +
tests/qemuxml2argvtest.c   |  6 +--
.../qemuxml2xmloutdata/disk-network-iscsi-auth.xml | 47 --
tests/qemuxml2xmloutdata/disk-network-iscsi.xml| 37 -
tests/qemuxml2xmltest.c|  3 +-
10 files changed, 83 insertions(+), 193 deletions(-)
delete mode 100644 tests/qemuxml2argvdata/disk-network-iscsi-auth.args
delete mode 100644 tests/qemuxml2argvdata/disk-network-iscsi-auth.xml
delete mode 100644 tests/qemuxml2argvdata/disk-network-iscsi-lun.args
delete mode 100644 tests/qemuxml2argvdata/disk-network-iscsi-lun.xml
delete mode 100644 tests/qemuxml2xmloutdata/disk-network-iscsi-auth.xml



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 12/24] tests: qemu: Unify disk cache testing

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:51:04PM +0200, Peter Krempa wrote:

Move the 'unsafe' cache test into 'disk-cache' and remove all the
individual cases for one cache mode each.

Signed-off-by: Peter Krempa 
---
tests/qemuxml2argvdata/disk-cache-directsync.args  | 30 
tests/qemuxml2argvdata/disk-cache-directsync.xml   | 37 ---
tests/qemuxml2argvdata/disk-cache-unsafe.args  | 30 
tests/qemuxml2argvdata/disk-cache-unsafe.xml   | 37 ---
tests/qemuxml2argvdata/disk-cache-v2-none.args | 30 
tests/qemuxml2argvdata/disk-cache-v2-none.xml  | 37 ---
tests/qemuxml2argvdata/disk-cache-v2-wb.args   | 30 
tests/qemuxml2argvdata/disk-cache-v2-wb.xml| 37 ---
tests/qemuxml2argvdata/disk-cache-v2-wt.args   | 30 
tests/qemuxml2argvdata/disk-cache-v2-wt.xml| 37 ---
tests/qemuxml2argvdata/disk-cache.args |  3 ++
.../qemuxml2argvdata/disk-cache.x86_64-2.6.0.args  |  3 ++
.../qemuxml2argvdata/disk-cache.x86_64-2.7.0.args  |  4 +++
.../qemuxml2argvdata/disk-cache.x86_64-latest.args |  4 +++
tests/qemuxml2argvdata/disk-cache.xml  |  6 
tests/qemuxml2argvtest.c   |  5 ---
tests/qemuxml2xmloutdata/disk-cache-directsync.xml | 41 --
tests/qemuxml2xmloutdata/disk-cache-unsafe.xml | 41 --
tests/qemuxml2xmloutdata/disk-cache-v2-none.xml| 41 --
tests/qemuxml2xmloutdata/disk-cache-v2-wb.xml  | 41 --
tests/qemuxml2xmloutdata/disk-cache-v2-wt.xml  | 41 --
tests/qemuxml2xmloutdata/disk-cache.xml|  6 
tests/qemuxml2xmltest.c|  5 ---
23 files changed, 26 insertions(+), 550 deletions(-)
delete mode 100644 tests/qemuxml2argvdata/disk-cache-directsync.args
delete mode 100644 tests/qemuxml2argvdata/disk-cache-directsync.xml
delete mode 100644 tests/qemuxml2argvdata/disk-cache-unsafe.args
delete mode 100644 tests/qemuxml2argvdata/disk-cache-unsafe.xml
delete mode 100644 tests/qemuxml2argvdata/disk-cache-v2-none.args
delete mode 100644 tests/qemuxml2argvdata/disk-cache-v2-none.xml
delete mode 100644 tests/qemuxml2argvdata/disk-cache-v2-wb.args
delete mode 100644 tests/qemuxml2argvdata/disk-cache-v2-wb.xml
delete mode 100644 tests/qemuxml2argvdata/disk-cache-v2-wt.args
delete mode 100644 tests/qemuxml2argvdata/disk-cache-v2-wt.xml
delete mode 100644 tests/qemuxml2xmloutdata/disk-cache-directsync.xml
delete mode 100644 tests/qemuxml2xmloutdata/disk-cache-unsafe.xml
delete mode 100644 tests/qemuxml2xmloutdata/disk-cache-v2-none.xml
delete mode 100644 tests/qemuxml2xmloutdata/disk-cache-v2-wb.xml
delete mode 100644 tests/qemuxml2xmloutdata/disk-cache-v2-wt.xml



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 11/24] tests: qemu: Add xml2xml and minimal version of 'disk-cache' test

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:51:03PM +0200, Peter Krempa wrote:

Signed-off-by: Peter Krempa 
---
tests/qemuxml2argvdata/disk-cache.args  | 38 +++
tests/qemuxml2argvtest.c|  1 +
tests/qemuxml2xmloutdata/disk-cache.xml | 54 +
tests/qemuxml2xmltest.c |  1 +
4 files changed, 94 insertions(+)
create mode 100644 tests/qemuxml2argvdata/disk-cache.args
create mode 100644 tests/qemuxml2xmloutdata/disk-cache.xml



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 10/24] tests: qemuxml2argv: Rename disk-write-cache test do disk-cache

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:51:02PM +0200, Peter Krempa wrote:

We'll agregate testing of all cache modes in this test later on.


aggregate



Signed-off-by: Peter Krempa 
---
...k-write-cache.x86_64-2.6.0.args => disk-cache.x86_64-2.6.0.args} | 0
...k-write-cache.x86_64-2.7.0.args => disk-cache.x86_64-2.7.0.args} | 0
...write-cache.x86_64-latest.args => disk-cache.x86_64-latest.args} | 0
tests/qemuxml2argvdata/{disk-write-cache.xml => disk-cache.xml} | 0
tests/qemuxml2argvtest.c| 6 +++---
5 files changed, 3 insertions(+), 3 deletions(-)
rename tests/qemuxml2argvdata/{disk-write-cache.x86_64-2.6.0.args => 
disk-cache.x86_64-2.6.0.args} (100%)
rename tests/qemuxml2argvdata/{disk-write-cache.x86_64-2.7.0.args => 
disk-cache.x86_64-2.7.0.args} (100%)
rename tests/qemuxml2argvdata/{disk-write-cache.x86_64-latest.args => 
disk-cache.x86_64-latest.args} (100%)
rename tests/qemuxml2argvdata/{disk-write-cache.xml => disk-cache.xml} (100%)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 09/24] tests: qemuxml2argv: Add 'CAPS_LATEST' version of 'disk-aio' test

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:51:01PM +0200, Peter Krempa wrote:

Signed-off-by: Peter Krempa 
---
tests/qemuxml2argvdata/disk-aio.x86_64-latest.args | 37 ++
tests/qemuxml2argvtest.c   |  1 +
2 files changed, 38 insertions(+)
create mode 100644 tests/qemuxml2argvdata/disk-aio.x86_64-latest.args



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 08/24] tests: qemuxml2argv: Add 'CAPS_LATEST' version for 'disk-detect-zeroes'

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:51:00PM +0200, Peter Krempa wrote:

This test also excercises options of 'discard'.

Signed-off-by: Peter Krempa 
---
.../disk-detect-zeroes.x86_64-latest.args  | 37 ++
tests/qemuxml2argvtest.c   |  1 +
2 files changed, 38 insertions(+)
create mode 100644 tests/qemuxml2argvdata/disk-detect-zeroes.x86_64-latest.args



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 07/24] tests: qemuxml2argv: Add 'CAPS_LATEST' data for disk-cdrom* tests

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:50:59PM +0200, Peter Krempa wrote:

Signed-off-by: Peter Krempa 
---
.../disk-cdrom-network.x86_64-latest.args  | 41 ++
.../disk-cdrom-tray.x86_64-latest.args | 39 
.../qemuxml2argvdata/disk-cdrom.x86_64-latest.args | 35 ++
tests/qemuxml2argvtest.c   |  3 ++
4 files changed, 118 insertions(+)
create mode 100644 tests/qemuxml2argvdata/disk-cdrom-network.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/disk-cdrom-tray.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/disk-cdrom.x86_64-latest.args



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 06/24] tests: qemuxml2argv: Unify testing of local cdroms

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:50:58PM +0200, Peter Krempa wrote:

Test empty cdroms along with cdroms with medium.

Signed-off-by: Peter Krempa 
---
tests/qemuxml2argvdata/disk-cdrom-empty.args  | 28 ---
tests/qemuxml2argvdata/disk-cdrom-empty.xml   | 36 
tests/qemuxml2argvdata/disk-cdrom.args|  4 ++-
tests/qemuxml2argvdata/disk-cdrom.xml |  6 
tests/qemuxml2argvtest.c  |  1 -
tests/qemuxml2xmloutdata/disk-cdrom-empty.xml | 40 ---
tests/qemuxml2xmloutdata/disk-cdrom.xml   |  6 
tests/qemuxml2xmltest.c   |  1 -
8 files changed, 15 insertions(+), 107 deletions(-)
delete mode 100644 tests/qemuxml2argvdata/disk-cdrom-empty.args
delete mode 100644 tests/qemuxml2argvdata/disk-cdrom-empty.xml
delete mode 100644 tests/qemuxml2xmloutdata/disk-cdrom-empty.xml



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 05/24] tests: qemuxml2argv: Unify network cdrom source testing

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:50:57PM +0200, Peter Krempa wrote:

Unify most of the tests into a common test named disk-cdrom-network by
adding multiple cdroms. The 'http' test is dropped since there can be
only 4 cdroms.

Signed-off-by: Peter Krempa 
---
tests/qemuxml2argvdata/disk-cdrom-network-ftp.args | 27 
tests/qemuxml2argvdata/disk-cdrom-network-ftp.xml  | 36 --
.../qemuxml2argvdata/disk-cdrom-network-ftps.args  | 27 
tests/qemuxml2argvdata/disk-cdrom-network-ftps.xml | 36 --
.../qemuxml2argvdata/disk-cdrom-network-http.args  | 27 
tests/qemuxml2argvdata/disk-cdrom-network-http.xml | 36 --
.../qemuxml2argvdata/disk-cdrom-network-tftp.args  | 27 
tests/qemuxml2argvdata/disk-cdrom-network-tftp.xml | 36 --
...-network-https.args => disk-cdrom-network.args} |  9 ++
...om-network-https.xml => disk-cdrom-network.xml} | 27 
tests/qemuxml2argvtest.c   |  6 +---
11 files changed, 37 insertions(+), 257 deletions(-)
delete mode 100644 tests/qemuxml2argvdata/disk-cdrom-network-ftp.args
delete mode 100644 tests/qemuxml2argvdata/disk-cdrom-network-ftp.xml
delete mode 100644 tests/qemuxml2argvdata/disk-cdrom-network-ftps.args
delete mode 100644 tests/qemuxml2argvdata/disk-cdrom-network-ftps.xml
delete mode 100644 tests/qemuxml2argvdata/disk-cdrom-network-http.args
delete mode 100644 tests/qemuxml2argvdata/disk-cdrom-network-http.xml
delete mode 100644 tests/qemuxml2argvdata/disk-cdrom-network-tftp.args
delete mode 100644 tests/qemuxml2argvdata/disk-cdrom-network-tftp.xml
rename tests/qemuxml2argvdata/{disk-cdrom-network-https.args => 
disk-cdrom-network.args} (58%)
rename tests/qemuxml2argvdata/{disk-cdrom-network-https.xml => 
disk-cdrom-network.xml} (52%)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 04/24] tests: qemuxml2argv: Remove tests obsoleted by assuming support for '-device'

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:50:56PM +0200, Peter Krempa wrote:

Few disk tests were testing support for pure -drive command line
generation for disks now that we assume it for all qemu versions the
cases are obsolete.

Replacements:
disk-readonly-no-device -> disk-readonly-disk
disk-floppy-tray-no-device -> disk-floppy-tray
disk-cdrom-tray-no-device -> disk-cdrom-tray

Signed-off-by: Peter Krempa 
---
.../disk-cdrom-tray-no-device-cap.args | 29 -
.../disk-cdrom-tray-no-device-cap.xml  | 32 ---
.../disk-floppy-tray-no-device-cap.args| 31 --
.../disk-floppy-tray-no-device-cap.xml | 37 --
.../qemuxml2argvdata/disk-readonly-no-device.args  | 30 --
tests/qemuxml2argvdata/disk-readonly-no-device.xml | 32 ---
tests/qemuxml2argvtest.c   |  3 --
7 files changed, 194 deletions(-)
delete mode 100644 tests/qemuxml2argvdata/disk-cdrom-tray-no-device-cap.args
delete mode 100644 tests/qemuxml2argvdata/disk-cdrom-tray-no-device-cap.xml
delete mode 100644 tests/qemuxml2argvdata/disk-floppy-tray-no-device-cap.args
delete mode 100644 tests/qemuxml2argvdata/disk-floppy-tray-no-device-cap.xml
delete mode 100644 tests/qemuxml2argvdata/disk-readonly-no-device.args
delete mode 100644 tests/qemuxml2argvdata/disk-readonly-no-device.xml



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 02/24] tests: qemuxml2xml: Remove duplicate test disk-copy-on-read.xml

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:50:54PM +0200, Peter Krempa wrote:

We also have disk-copy_on_read.xml which also tests the command line.

Signed-off-by: Peter Krempa 
---
tests/qemuxml2argvdata/disk-copy-on-read.xml   | 30 -
tests/qemuxml2xmloutdata/disk-copy-on-read.xml | 36 --
tests/qemuxml2xmltest.c|  1 -
3 files changed, 67 deletions(-)
delete mode 100644 tests/qemuxml2argvdata/disk-copy-on-read.xml
delete mode 100644 tests/qemuxml2xmloutdata/disk-copy-on-read.xml



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 03/24] tests: qemuxml2argv: Add 'CAPS_LATEST' version of "disk-copy_on_read"

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:50:55PM +0200, Peter Krempa wrote:

Signed-off-by: Peter Krempa 
---
.../disk-copy_on_read.x86_64-latest.args   | 41 ++
tests/qemuxml2argvtest.c   |  1 +
2 files changed, 42 insertions(+)
create mode 100644 tests/qemuxml2argvdata/disk-copy_on_read.x86_64-latest.args



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 01/24] tests: qemu: Drop 'drive' from disk tests

[Ján Tomko]

On Thu, Jul 19, 2018 at 05:50:53PM +0200, Peter Krempa wrote:

Signed-off-by: Peter Krempa 
---
...ress-conflict.xml => disk-address-conflict.xml} |  0
...-drive-boot-cdrom.args => disk-boot-cdrom.args} |  0


[...]


rename tests/qemuxml2xmloutdata/{disk-drive-network-vxhs.xml => 
disk-network-vxhs.xml} (100%)
rename tests/qemuxml2xmloutdata/{disk-virtio-drive-queues.xml => 
disk-virtio-queues.xml} (100%)



Reviewed-by: Ján Tomko 

Jano


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 RESEND 00/12] PCI passthrough support on s390

[Laine Stump]

On Mon, Jul 23, 2018, 2:37 PM Ján Tomko  wrote:

>
> But I'd like to hear from some other developer who touched the PCI code
> last (Laine? Andrea?)
>

I'm looking at this on the screen of my phone as I stare out the hotel
window at an afternoon rainstorm over Razgrad, Bulgaria, and won't be back
to my office for another week, so I'm afraid I'm not in much of a position
to make any useful comment :-)

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

Re: [libvirt] [PATCH v1 07/40] util: hash: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:39PM +0530, Sukrit Bhatnagar wrote:
> Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
> src/util/viralloc.h, define a new wrapper around an existing
> cleanup function which will be called when a variable declared
> with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
> viralloc.h include, since that has moved from the source module into
> the header.
>
> When variables of type virHashTablePtr are declared using
> VIR_AUTOPTR, the function virHashFree will be run
> automatically on it when it goes out of scope.
>
> Signed-off-by: Sukrit Bhatnagar 
> ---

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v1 06/40] util: buffer: use VIR_AUTOPTR for aggregate types

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:38PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOPTR macro for declaring aggregate pointer variables,
> majority of the calls to *Free functions can be dropped, which
> in turn leads to getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar 
> ---

Try considering the idea I had about the virBufferEscapePairFree function.
Other than that, the patch is fine.

Erik

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

Re: [libvirt] [PATCH v1 05/40] util: buffer: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:37PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOFREE macro for declaring scalar variables, majority
> of the VIR_FREE calls can be dropped, which in turn leads to
> getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar 
> ---

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v2 RESEND 04/12] qemu: Enable PCI multi bus for S390 guests

[Ján Tomko]

On Tue, Jul 10, 2018 at 04:02:18PM +0800, Yi Min Zhao wrote:

QEMU on s390 supports PCI multibus since forever. But zPCI, as extension
of PCI device on s390, is the significant capability. Only when zPCI
capability is existing, we consider QEMU supports PCI multibus properly.
So let enable PCI multibus only if zPCI is supported.



This comment is now outdated, since we assume multibus in all cases
(even though it only makes sense with ZPCI)

Jano


Signed-off-by: Yi Min Zhao 
Reviewed-by: Boris Fiuczynski 
Reviewed-by: Stefan Zimmermann 
Reviewed-by: Bjoern Walk 
---
src/qemu/qemu_capabilities.c | 4 
1 file changed, 4 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 07f58fd014..7cba7eecdc 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1760,6 +1760,10 @@ bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr qemuCaps,
return false;
}

+/* S390 supports PCI-multibus. */
+if (ARCH_IS_S390(def->os.arch))
+return true;
+
/* If 'virt' supports PCI, it supports multibus.
 * No extra conditions here for simplicity.
 */
--
Yi Min

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


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

Re: [libvirt] [PATCH v1 03/40] util: buffer: typedef and Free helper for struct _virBufferEscapePair

[Erik Skultety]

On Mon, Jul 23, 2018 at 01:19:49PM +0200, Erik Skultety wrote:
> On Sat, Jul 21, 2018 at 05:36:35PM +0530, Sukrit Bhatnagar wrote:
> > Create typedefs virBufferEscapePair and virBufferEscapePairPtr
> > for struct _virBufferEscapePair for cleaner code and for use
> > with cleanup macros.
> >
> > Also create a dedicated Free helper virBufferEscapePairFree.
>
> Usually, a sentence like ^this indicates, that the change itself should come
> separately.
>
> >
> > Signed-off-by: Sukrit Bhatnagar 
> > ---
> >  src/util/virbuffer.c | 16 ++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
> > index 3d6defb..8076cd3 100644
> > --- a/src/util/virbuffer.c
> > +++ b/src/util/virbuffer.c
> > @@ -648,11 +648,23 @@ virBufferEscape(virBufferPtr buf, char escape, const 
> > char *toescape,
> >  }
> >
> >
> > +typedef struct _virBufferEscapePair virBufferEscapePair;
> > +typedef virBufferEscapePair *virBufferEscapePairPtr;
>
> ^This should come as a separate patch.
>
> > +
> >  struct _virBufferEscapePair {
> >  char escape;
> >  char *toescape;
> >  };
> >
> > +static void
> > +virBufferEscapePairFree(virBufferEscapePairPtr pair)
> > +{
> > +if (!pair)
> > +return;
> > +
> > +VIR_FREE(pair);
> > +}
>
> Patch doesn't compile, please make sure that each patch can pass
> make check && make syntax-check.
>
> ^This function should be introduced in the following patch.

An idea to consider would be to do VIR_STRDUP on the toescape strings, so that
this free helper would do VIR_FREE(toescape), that might make more sense in
terms of logic. It's also safer, since we can't predict the lifetime of the
string passed by the caller, they might as well free it immediately after
returning from a helper, assuming the helper created their own copy of the
string.

Erik

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

Re: [libvirt] [PATCH v2 RESEND 00/12] PCI passthrough support on s390

[Ján Tomko]

On Tue, Jul 10, 2018 at 04:02:14PM +0800, Yi Min Zhao wrote:

Abstract

The PCI representation in QEMU has recently been extended for S390
allowing configuration of zPCI attributes like uid (user-defined
identifier) and fid (PCI function identifier).
The details can be found here:
https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg07262.html

To support the new zPCI feature of the S390 platform, two new XML
attributes, @uid and @fid, are introduced for device addresses of type
'pci', i.e.:
 
   
   
 
   
   
 

uid and fid are optional attributes. If they are defined by the user,
unique values within the guest domain must be used. If they are not
specified and the architecture requires them, they are automatically
generated with non-conflicting values.

Current implementation is the most seamless one for the user as it
unites the address specific data of a PCI device on one XML element.
It could accommodate both specifying our special parameters (uid and fid)
and re-using standard statements (domain, bus, slot and function) for
PCI devices. User can still specify bus/slot/function for the virtualized
PCI devices in the XML.

Thus uid/fid act as an extension to the PCI address and are stored in
a new structure 'virZPCIDeviceAddress' which is a member of common PCI
Address structure. Additionally, two hashtables are used for assignment
and reservation of uid/fid.

In support of extending the PCI address, a new PCI address extension flag is
introduced. This extension flag allows is not only dedicated for the S390
platform but also other architectures needing certain extensions to PCI
address space.

Code Base
=
commit in master:
767f9e1449b1a36111532847f0c62dc758263c42
qemu: validate: Enforce compile time switch type checking for videos

Change Log
==
v1->v2:
1. Separate test commit and merge testcases into corresponding commits that
  introduce the functionalities firstly.
2. Spare some checks for zpci device.
3. Add vsock and controller support.
4. Add uin32 type schema.
5. Rename zpciuid and zpcifid to zpci_uid and zpci_fid.
6. Always return multibus support on S390.

Yi Min Zhao (12):
 conf: Add definitions for 'uid' and 'fid' PCI address attributes
 qemu: Introduce zPCI capability
 conf: Introduce a new PCI address extension flag
 qemu: Enable PCI multi bus for S390 guests
 qemu: Auto add pci-root for s390/s390x guests
 conf: Introduce address caching for PCI extensions
 conf: Introduce parser, formatter for uid and fid
 conf: Allocate/release 'uid' and 'fid' in PCI address
 qemu: Generate and use zPCI device in QEMU command line
 qemu: Add hotpluging support for PCI devices on S390 guests
 docs: Add 'uid' and 'fid' information
 news: Update news for PCI address extension attributes



The patches look OK to me, therefore:

Reviewed-by: Ján Tomko 

But I'd like to hear from some other developer who touched the PCI code
last (Laine? Andrea?)

Jano


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

Re: [libvirt] [PATCH v1 04/40] util: buffer: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:36PM +0530, Sukrit Bhatnagar wrote:
> Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
> src/util/viralloc.h, define a new wrapper around an existing
> cleanup function which will be called when a variable declared
> with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
> viralloc.h include, since that has moved from the source module into
> the header.
>
> When variables of type virBufferPtr and virBufferEscapePairPtr
> are declared using VIR_AUTOPTR, the functions virBufferFreeAndReset
> and virBufferEscapePairFree, respectively, will be run automatically
> on them when they go out of scope.
>
> Signed-off-by: Sukrit Bhatnagar 
> ---
>  src/util/virbuffer.c | 2 +-
>  src/util/virbuffer.h | 9 +++--
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
> index 8076cd3..65f4a08 100644
> --- a/src/util/virbuffer.c
> +++ b/src/util/virbuffer.c
> @@ -31,7 +31,6 @@
>  #define __VIR_BUFFER_C__
>
>  #include "virbuffer.h"
> -#include "viralloc.h"
>  #include "virerror.h"
>  #include "virstring.h"
>
> @@ -664,6 +663,7 @@ virBufferEscapePairFree(virBufferEscapePairPtr pair)
>
>  VIR_FREE(pair);
>  }
> +VIR_DEFINE_AUTOPTR_FUNC(virBufferEscapePair, virBufferEscapePairFree)

I'm wondering whether we shouldn't export virBufferEscapePair struct to other
potential callers just to make things cleaner by having the
VIR_DEFINE_AUTOPTR_FUNC macros coupled together, but that can be done ad-hoc
once someone needs it, I'm fine with this change too.

>
>
>  /**
> diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h
> index e95ee87..3b31060 100644
> --- a/src/util/virbuffer.h
> +++ b/src/util/virbuffer.h
> @@ -23,10 +23,13 @@
>  #ifndef __VIR_BUFFER_H__
>  # define __VIR_BUFFER_H__
>
> -# include "internal.h"
> -
>  # include 
>
> +# include "internal.h"
> +

^This newline is redundant.

> +# include "viralloc.h"
> +
> +
>  /**
>   * virBuffer:
>   *
> @@ -119,4 +122,6 @@ int virBufferGetIndent(const virBuffer *buf, bool 
> dynamic);
>  void virBufferTrim(virBufferPtr buf, const char *trim, int len);
>  void virBufferAddStr(virBufferPtr buf, const char *str);
>
> +VIR_DEFINE_AUTOPTR_FUNC(virBuffer, virBufferFreeAndReset)
> +
>  #endif /* __VIR_BUFFER_H__ */
> --
> 1.8.3.1
>

If you squash the static definition of virBufferEscapePairFree into this patch:

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v1 03/40] util: buffer: typedef and Free helper for struct _virBufferEscapePair

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:35PM +0530, Sukrit Bhatnagar wrote:
> Create typedefs virBufferEscapePair and virBufferEscapePairPtr
> for struct _virBufferEscapePair for cleaner code and for use
> with cleanup macros.
>
> Also create a dedicated Free helper virBufferEscapePairFree.

Usually, a sentence like ^this indicates, that the change itself should come
separately.

>
> Signed-off-by: Sukrit Bhatnagar 
> ---
>  src/util/virbuffer.c | 16 ++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
> index 3d6defb..8076cd3 100644
> --- a/src/util/virbuffer.c
> +++ b/src/util/virbuffer.c
> @@ -648,11 +648,23 @@ virBufferEscape(virBufferPtr buf, char escape, const 
> char *toescape,
>  }
>
>
> +typedef struct _virBufferEscapePair virBufferEscapePair;
> +typedef virBufferEscapePair *virBufferEscapePairPtr;

^This should come as a separate patch.

> +
>  struct _virBufferEscapePair {
>  char escape;
>  char *toescape;
>  };
>
> +static void
> +virBufferEscapePairFree(virBufferEscapePairPtr pair)
> +{
> +if (!pair)
> +return;
> +
> +VIR_FREE(pair);
> +}

Patch doesn't compile, please make sure that each patch can pass
make check && make syntax-check.

^This function should be introduced in the following patch.

> +
>
>  /**
>   * virBufferEscapeN:
> @@ -678,8 +690,8 @@ virBufferEscapeN(virBufferPtr buf,
>  char *escaped = NULL;
>  char *out;
>  const char *cur;
> -struct _virBufferEscapePair escapeItem;
> -struct _virBufferEscapePair *escapeList = NULL;
> +virBufferEscapePair escapeItem;
> +virBufferEscapePairPtr escapeList = NULL;

^This should come together with the typedef above.

Erik

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

Re: [libvirt] [PATCH v1 02/40] util: error: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:34PM +0530, Sukrit Bhatnagar wrote:
> Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
> src/util/viralloc.h, define a new wrapper around an existing
> cleanup function which will be called when a variable declared
> with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant
> viralloc.h include, since that has moved from the source module into
> the header.
>
> The type virError and the declaration of virFreeError is done in
> include/libvirt/virterror.h which is pulled in by internal.h.

^This sentence is not needed.

Reviewed-by: Erik Skultety 

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

[libvirt] [python PATCH 3/3] rpm: update min required rhel/fedora

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---
 libvirt-python.spec.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libvirt-python.spec.in b/libvirt-python.spec.in
index 8709317..21dc713 100644
--- a/libvirt-python.spec.in
+++ b/libvirt-python.spec.in
@@ -3,8 +3,8 @@
 # This spec file assumes you are building on a Fedora or RHEL version
 # that's still supported by the vendor. It may work on other distros
 # or versions, but no effort will be made to ensure that going forward
-%define min_rhel 6
-%define min_fedora 25
+%define min_rhel 7
+%define min_fedora 27
 
 %if (0%{?fedora} && 0%{?fedora} >= %{min_fedora}) || (0%{?rhel} && 0%{?rhel} 
>= %{min_rhel})
 %define supported_platform 1
-- 
2.17.1

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

[libvirt] [python PATCH 2/3] rpm: add BuildRequires on gcc

[Daniel P . Berrangé]

The gcc RPM is no longer part of the default build root.

Signed-off-by: Daniel P. Berrangé 
---
 libvirt-python.spec.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libvirt-python.spec.in b/libvirt-python.spec.in
index 589855e..8709317 100644
--- a/libvirt-python.spec.in
+++ b/libvirt-python.spec.in
@@ -55,6 +55,7 @@ BuildRequires: python3-devel
 BuildRequires: python3-nose
 BuildRequires: python3-lxml
 %endif
+BuildRequires: gcc
 
 # Don't want provides for python shared objects
 %if %{with_python2}
-- 
2.17.1

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

[libvirt] [python PATCH 0/3] rpm: misc updates to modernize spec

[Daniel P . Berrangé]

Daniel P. Berrangé (3):
  rpm: use the versioned python2 macro names
  rpm: add BuildRequires on gcc
  rpm: update min required rhel/fedora

 libvirt-python.spec.in | 23 ---
 1 file changed, 12 insertions(+), 11 deletions(-)

-- 
2.17.1

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

[libvirt] [python PATCH 1/3] rpm: use the versioned python2 macro names

[Daniel P . Berrangé]

The use of non-versioned python2 macro names is deprecated in Fedora

Signed-off-by: Daniel P. Berrangé 
---
 libvirt-python.spec.in | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/libvirt-python.spec.in b/libvirt-python.spec.in
index f3940cc..589855e 100644
--- a/libvirt-python.spec.in
+++ b/libvirt-python.spec.in
@@ -58,7 +58,7 @@ BuildRequires: python3-lxml
 
 # Don't want provides for python shared objects
 %if %{with_python2}
-%{?filter_provides_in: %filter_provides_in %{python_sitearch}/.*\.so}
+%{?filter_provides_in: %filter_provides_in %{python2_sitearch}/.*\.so}
 %endif
 %if %{with_python3}
 %{?filter_provides_in: %filter_provides_in %{python3_sitearch}/.*\.so}
@@ -118,7 +118,7 @@ exit 1
 %endif
 
 %if %{with_python2}
-CFLAGS="$RPM_OPT_FLAGS" %{__python} setup.py build
+CFLAGS="$RPM_OPT_FLAGS" %{__python2} setup.py build
 %endif
 %if %{with_python3}
 CFLAGS="$RPM_OPT_FLAGS" %{__python3} setup.py build
@@ -126,7 +126,7 @@ CFLAGS="$RPM_OPT_FLAGS" %{__python3} setup.py build
 
 %install
 %if %{with_python2}
-%{__python} setup.py install --skip-build --root=%{buildroot}
+%{__python2} setup.py install --skip-build --root=%{buildroot}
 %endif
 %if %{with_python3}
 %{__python3} setup.py install --skip-build --root=%{buildroot}
@@ -134,7 +134,7 @@ CFLAGS="$RPM_OPT_FLAGS" %{__python3} setup.py build
 
 %check
 %if %{with_python2}
-%{__python} setup.py test
+%{__python2} setup.py test
 %endif
 %if %{with_python3}
 %{__python3} setup.py test
@@ -143,11 +143,11 @@ CFLAGS="$RPM_OPT_FLAGS" %{__python3} setup.py build
 %if %{with_python2}
 %files -n python2-libvirt
 %doc ChangeLog AUTHORS NEWS README COPYING COPYING.LESSER examples/
-%{python_sitearch}/libvirt.py*
-%{python_sitearch}/libvirt_qemu.py*
-%{python_sitearch}/libvirt_lxc.py*
-%{python_sitearch}/libvirtmod*
-%{python_sitearch}/*egg-info
+%{python2_sitearch}/libvirt.py*
+%{python2_sitearch}/libvirt_qemu.py*
+%{python2_sitearch}/libvirt_lxc.py*
+%{python2_sitearch}/libvirtmod*
+%{python2_sitearch}/*egg-info
 %endif
 
 %if %{with_python3}
-- 
2.17.1

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

Re: [libvirt] [PATCH v2 0/8] virStr*cpy*() related fixes and cleanups

[Michal Prívozník]

On 07/20/2018 04:44 PM, Andrea Bolognani wrote:
> Changes from [v1]:
> 
>   * catch a bunch more suboptimal uses of the original API;
> 
>   * improve the API so that it's both more sensible and fits
> better with the rest of libvirt.
> 
> [v1] https://www.redhat.com/archives/libvir-list/2018-July/msg01044.html
> 
> Andrea Bolognani (8):
>   src: Use virStrcpyStatic() to avoid truncation
>   src: Use virStrcpyStatic() wherever possible
>   src: Use VIR_STRDUP() wherever possible
>   src: Use virStrcpy() wherever possible
>   src: Don't rely on strncpy()-like behavior
>   src: Make virStr*cpy*() functions return an int
>   esx: Use memcpy() in esxVI_CURL_Debug()
>   util: Improve virStrncpy() implementation
> 
>  docs/hacking.html.in  | 29 +-
>  src/conf/capabilities.c   |  2 +-
>  src/conf/netdev_vport_profile_conf.c  |  2 +-
>  src/conf/nwfilter_conf.c  |  3 +-
>  src/esx/esx_driver.c  |  8 +--
>  src/esx/esx_vi.c  |  6 +-
>  src/esx/esx_vi_types.c|  2 +-
>  src/hyperv/hyperv_driver.c|  3 +-
>  src/libxl/libxl_conf.c|  2 +-
>  src/locking/lock_driver_sanlock.c | 25 
>  src/lxc/lxc_driver.c  |  8 +--
>  src/nwfilter/nwfilter_dhcpsnoop.c |  2 +-
>  src/nwfilter/nwfilter_ebiptables_driver.c |  6 +-
>  src/nwfilter/nwfilter_learnipaddr.c   |  2 +-
>  src/openvz/openvz_conf.c  |  8 +--
>  src/qemu/qemu_agent.c |  2 +-
>  src/qemu/qemu_command.c   |  2 +-
>  src/qemu/qemu_monitor.c   |  2 +-
>  src/remote/remote_daemon_dispatch.c   |  4 +-
>  src/remote/remote_driver.c|  4 +-
>  src/rpc/virnetlibsshsession.c |  4 +-
>  src/rpc/virnetsocket.c|  4 +-
>  src/security/security_apparmor.c  |  2 +-
>  src/security/virt-aa-helper.c |  2 +-
>  src/test/test_driver.c|  4 +-
>  src/uml/uml_driver.c  |  4 +-
>  src/util/virfdstream.c|  4 +-
>  src/util/virhostcpu.c |  4 +-
>  src/util/virhostmem.c |  6 +-
>  src/util/virlog.c |  5 +-
>  src/util/virnetdev.c  | 12 ++--
>  src/util/virnetdevbridge.c|  6 +-
>  src/util/virnetdevtap.c   |  4 +-
>  src/util/virnetdevvportprofile.c  |  6 +-
>  src/util/virstring.c  | 70 +++
>  src/util/virstring.h  |  4 +-
>  src/util/virtypedparam.c  |  8 +--
>  src/xenapi/xenapi_driver.c|  4 +-
>  src/xenconfig/xen_common.c| 38 ++--
>  src/xenconfig/xen_sxpr.c  |  2 +-
>  src/xenconfig/xen_xl.c| 41 ++---
>  src/xenconfig/xen_xm.c|  2 +-
>  42 files changed, 179 insertions(+), 179 deletions(-)
> 

ACK series.

Michal

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

Re: [libvirt] [PATCH v1 01/40] util: cgroup: modify virCgroupFree to take virCgroupPtr

[Erik Skultety]

On Sat, Jul 21, 2018 at 05:36:33PM +0530, Sukrit Bhatnagar wrote:
> Modify virCgroupFree function signature to take a value of type
> virCgroupPtr instead of virCgroupPtr * as the parameter.
>
> Change the argument type in all calls to virCgroupFree function
> from virCgroupPtr * to virCgroupPtr.

^This sentence doesn't add any useful information. Instead, the commit message
should add information about why we're performing this change, i.e. in order to
enable usage of VIR_AUTOPTR with cgroup module or something alike.
Also, this patch is oddly placed, IMHO it should come before patch 8, where the
other work on cgroup module is done.

With that:
Reviewed-by: Erik Skultety 

...

> @@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path,
>  ret = 0;
>   cleanup:
>  if (ret != 0)
> -virCgroupFree(group);
> -virCgroupFree();
> +virCgroupFree(*group);
> +virCgroupFree(parent);

Since you're already touching the code, I'd appreciate another "adjustment"
patch where occurrences like ^this will be replaced by a VIR_STEAL_PTR, IOW
where we're passing a reference to a pointer in order to change the original
pointer, we should use a VIR_STEAL_PTR just before the cleanup section, so that
we don't need an if (ret != 0) or if (ret < 0) check only to conditionally do
some cleanup. Feel free to let me know if none of what I just wrote is clear.

Erik

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

Re: [libvirt] [PATCH] libvirt_iohelper: record the libvirt_iohelper's error message at virFileWrapperFdFree

[Michal Prívozník]

On 07/22/2018 03:40 AM, xinhua.Cao wrote:
> Currently iohelper's error log is recorded in virFileWrapperFdClose.
> In qemuDomainSaveMemory, it usually fails at qemuMigrationSrcToFile,
> and then goto cleanup, so the iohelper error log is not recorded,
> and so is the another placement. We now record the error log of
> iohelper by move it to the virFileWrapperFdFree record.
> There is another problem here, that is, virCommandWait is also not
> called, but I can't evaluate this impact. So no changes have been
> made here.
> ---
>  src/util/virfile.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 1faeebb..30456ab 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -330,9 +330,6 @@ virFileWrapperFdClose(virFileWrapperFdPtr wfd)
>  return 0;
>  
>  ret = virCommandWait(wfd->cmd, NULL);
> -if (wfd->err_msg && *wfd->err_msg)
> -VIR_WARN("iohelper reports: %s", wfd->err_msg);
> -
>  return ret;
>  }
>  
> @@ -351,6 +348,9 @@ virFileWrapperFdFree(virFileWrapperFdPtr wfd)
>  if (!wfd)
>  return;
>  
> +if (wfd->err_msg && *wfd->err_msg)
> +VIR_WARN("iohelper reports: %s", wfd->err_msg);
> +
>  VIR_FREE(wfd->err_msg);
>  
>  virCommandFree(wfd->cmd);
> 

Makes sense. However, your patch lacks Signed-off-By line so I can't
push it. It's required according to contributor guidelines:

https://libvirt.org/hacking.html

And for the virCommandWait() - we should call virCommandAbort() in
virFileWrapperFdFree() to make sure no process is left behind. If
virCommandWait() is called then virCommandAbort() is a NO-OP. If it
isn't, then Abort() will kill iohelper.

Michal

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

Re: [libvirt] [PATCH] tests: fix TLS handshake failure with TLS 1.3

[Michal Prívozník]

On 07/18/2018 08:21 PM, Daniel P. Berrangé wrote:
> When gnutls negotiates TLS 1.3 instead of 1.2, the order of messages
> sent by the handshake changes. This exposed a logic bug in the test
> suite which caused us to wait for the server to see handshake
> completion, but not wait for the client to see completion. The result
> was the client didn't receive the certificate for verification and the
> test failed.
> 
> This is exposed in Fedora 29 rawhide which has just enabled TLS 1.3 in
> its GNUTLS builds.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  tests/virnettlssessiontest.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

ACK

Michal

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

Re: [libvirt] [PATCH v2 1/2] virsh: Support alias in attach-disk

[Michal Prívozník]

On 07/15/2018 12:08 PM, Han Han wrote:
> Add --alias to support custom disk alias in virsh attach-disk.
> Report error if custom alias doesn't start with 'ua-'.
> 
> Signed-off-by: Han Han 
> ---
>  tools/virsh-domain.c | 15 ++-
>  tools/virsh.pod  |  3 ++-
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 8adec1d9b1..467417852e 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -290,6 +290,10 @@ static const vshCmdOptDef opts_attach_disk[] = {
>   .type = VSH_OT_STRING,
>   .help = N_("wwn of disk device")
>  },
> +{.name = "alias",
> + .type = VSH_OT_STRING,
> + .help = N_("custom alias name of disk device")
> +},
>  {.name = "rawio",
>   .type = VSH_OT_BOOL,
>   .help = N_("needs rawio capability")
> @@ -574,7 +578,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>  *subdriver = NULL, *type = NULL, *mode = NULL,
>  *iothread = NULL, *cache = NULL, *io = NULL,
>  *serial = NULL, *straddr = NULL, *wwn = NULL,
> -*targetbus = NULL;
> +*targetbus = NULL, *alias = NULL;
>  struct DiskAddress diskAddr;
>  bool isFile = false, functionReturn = false;
>  int ret;
> @@ -611,6 +615,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>  vshCommandOptStringReq(ctl, cmd, "wwn", ) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "address", ) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "targetbus", ) < 0 ||
> +vshCommandOptStringReq(ctl, cmd, "alias", ) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "sourcetype", ) < 0)
>  goto cleanup;
>  
> @@ -680,6 +685,14 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>  if (serial)
>  virBufferAsprintf(, "%s\n", serial);
>  
> +if (alias) {
> +if (!STRPREFIX(alias, "ua-")) {
> +vshError(ctl, _("Custom alias name should start with ua-"));
> +goto cleanup;
> +}

I don't think this belongs here. I'd let libvirt report an error. The
reason for that is to have checks in one place rather than scattered
around the code. For instance, imagine that one day we lift the
restriction and let users define alias in a free form. Using this
version of virsh (and connecting to new libvirtd) they will be unable to
do so.
Or the other way round - we allow only certain characters to be in user
alias. You are not checking them here - you're relying on libvirtd doing so.

The rest looks okay.

Michal

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

Re: [libvirt] [PATCH v2 2/2] virsh: Support alias in attach-interface

[Michal Prívozník]

On 07/15/2018 12:08 PM, Han Han wrote:
> Add --alias to support custom alias in virsh attach-interface.
> Report error if custom alias doesn't start with 'ua-'.
> 
> Signed-off-by: Han Han 
> ---
>  tools/virsh-domain.c | 15 ++-
>  tools/virsh.pod  |  4 +++-
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 467417852e..7fb419f6b5 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -842,6 +842,10 @@ static const vshCmdOptDef opts_attach_interface[] = {
>   .type = VSH_OT_STRING,
>   .help = N_("model type")
>  },
> +{.name = "alias",
> + .type = VSH_OT_STRING,
> + .help = N_("custom alias name of interface device")
> +},
>  {.name = "inbound",
>   .type = VSH_OT_STRING,
>   .help = N_("control domain's incoming traffics")
> @@ -915,7 +919,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
>  virDomainPtr dom = NULL;
>  const char *mac = NULL, *target = NULL, *script = NULL,
> *type = NULL, *source = NULL, *model = NULL,
> -   *inboundStr = NULL, *outboundStr = NULL;
> +   *inboundStr = NULL, *outboundStr = NULL, *alias = NULL;
>  virNetDevBandwidthRate inbound, outbound;
>  virDomainNetType typ;
>  int ret;
> @@ -945,6 +949,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
>  vshCommandOptStringReq(ctl, cmd, "mac", ) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "script", ) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "model", ) < 0 ||
> +vshCommandOptStringReq(ctl, cmd, "alias", ) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "inbound", ) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "outbound", ) < 0)
>  goto cleanup;
> @@ -1042,6 +1047,14 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
>  if (model != NULL)
>  virBufferAsprintf(, "\n", model);
>  
> +if (alias != NULL) {
> +if (!STRPREFIX(alias, "ua-")) {
> +vshError(ctl, _("Custom alias name should start with ua-"));
> +goto cleanup;
> +}
> +virBufferAsprintf(, "\n", alias);

Same here. This check is not desired here. To extend my reasoning in 1/2
- we are not checking if provided MAC address really is MAC address. For
instance:

# attach-interface --print-xml fedora network default --mac blah

Michal

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

Re: [libvirt] [PATCH v2 0/2] Add alias support in sub-commands of virsh

[Michal Prívozník]

On 07/15/2018 12:08 PM, Han Han wrote:
> Fix make syntax-check failure in v1.
> v1 link: https://www.redhat.com/archives/libvir-list/2018-July/msg00929.html
> 
> Han Han (2):
>   virsh: Support alias in attach-disk
>   virsh: Support alias in attach-interface
> 
>  tools/virsh-domain.c | 30 --
>  tools/virsh.pod  |  7 +--
>  2 files changed, 33 insertions(+), 4 deletions(-)
> 

I'm fixing all the issues I've found, ACKing and pushing.

Might be worth providing news.xml entry though. Can you post a patch for
that please?

Michal

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

Re: [libvirt] [PATCHv4 13/15] build: switch --with-qemu default from yes to check

[Andrea Bolognani]

On Mon, 2018-07-23 at 10:27 +0100, Daniel P. Berrangé wrote:
> On Mon, Jul 23, 2018 at 09:38:47AM +0200, Andrea Bolognani wrote:
> > Partially answering myself (morning coffee hasn't quite kicked in
> > just yet), the MinGW .spec doesn't list yajl as a BuildRequires,
> > which means that either we're not hitting any of the code paths you
> > mention on that platform, or we've been producing completely useless
> > Windows builds for a while now :)
> 
> The remote driver works fine without JSON being required.

That being the case, not having jansson as mandatory sounds like the
proper thing to do, at least from the theoretical point of view: even
if you build without it, you can still get *some* functionality out
of libvirt, so it's kinda hard to argue for it being a strict
requirement the same way something like libxml2 is.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [dbus PATCH] Install daemon under @sbindir@

[Andrea Bolognani]

On Mon, 2018-07-23 at 10:43 +0200, Pino Toscano wrote:
> On Monday, 23 July 2018 10:34:29 CEST Andrea Bolognani wrote:
> > The libvirt-dbus daemon is not supposed to be invoked
> > explicitly by the user, but rather to be spawned on-demand
> > by the D-Bus daemon: as such, @sbindir@ is a more suitable
> > location in which to install it.
> 
> sbindir is still not correct though, since it is not in the normal
> $PATH of users, but still root's one.
> 
> Most probably what you are looking for is libexecdir, which is the
> place that fits your requirement.

Yeah, I considered that one too, but being consistent with where
libvirt is installing its own daemons is probably a better
compromise in this case.

Also note that while /usr/sbin is not in regular users' $PATH in
Debian, it is in Fedora :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCHv2] replace 'if' type conditions with 'switch' for VIR_NETWORK_FORWARD_

[Shi Lei]

On Mon, Jul 23, 2018 at 4:47 PM, Erik wrote:
> On Mon, Jul 23, 2018 at 12:03:57PM +0800, Shi Lei wrote:
> > v1 patch here: 
> > https://www.redhat.com/archives/libvir-list/2018-July/msg01314.html
> >
> > since v1:
> > 1. Change the type declaration of _virNetworkForwardDef.type
> >  from int to virNetworkForwardType
> 
> Maybe I wasn't 100% clear in my response to v1, you have to do the typecast
> explicitly in all the switches rather than change the type to an enum in the
> struct definition, since the compiler can decide the enum to be unsigned which
> means that the return value from the virNetworkForwardTypeFromString call in
> virNetworkForwardDefParseXML might get usigned-wrapped, ergo the check for a
> non-existent type would be a contradiction.

OK.

> 
> > 2. use the default case to report out of range error with
> >  virReportEnumRangeError
> 
> Another thing, it's cool that you incorporated the diff summary. However, this
> shouldn't ever be part of the commit message, rather it should be mentioned as
> a note in the note section below the "dashed line"

OK.

> 
> >
> > Signed-off-by: Shi Lei 
> > ---
> >  src/conf/domain_conf.c   |  49 ---
> >  src/conf/network_conf.c  |  73 +++---
> >  src/conf/network_conf.h  |   2 +-
> >  src/conf/virnetworkobj.c |  24 +++-
> >  src/esx/esx_network_driver.c |  19 ++-
> >  src/libvirt_private.syms |   1 +
> >  src/network/bridge_driver.c  | 309 
> > ---
> >  src/qemu/qemu_process.c  |   8 ++
> >  8 files changed, 329 insertions(+), 156 deletions(-)
> >
> 
> ...
> 
> > +if (virNetworkObjIsActive(obj)) {
> > +switch (def->forward.type) {
> > +case VIR_NETWORK_FORWARD_NONE:
> > +case VIR_NETWORK_FORWARD_NAT:
> > +case VIR_NETWORK_FORWARD_ROUTE:
> > +/* Only three of the L3 network types that are configured by
> > + * libvirt need to have iptables rules reloaded. The 4th L3
> > + * network type, forward='open', doesn't need this because it
> > + * has no iptables rules.
> > + */
> > +networkRemoveFirewallRules(def);
> > +/* No need to check return value since already logged 
> > internally */
> 
> You can drop ^this comment, the fact that we're purposefully ignoring the 
> return
> value should tell the reader something ;).

OK. I'll drop it.

> 
> > +ignore_value(networkAddFirewallRules(def));
> > +break;
> > +
> 
> Thanks,
> Erik
> 

Thanks!
Shi Lei

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

Re: [libvirt] [PATCHv4 13/15] build: switch --with-qemu default from yes to check

[Daniel P . Berrangé]

On Mon, Jul 23, 2018 at 09:38:47AM +0200, Andrea Bolognani wrote:
> On Mon, 2018-07-23 at 09:27 +0200, Andrea Bolognani wrote:
> > On Fri, 2018-07-20 at 11:33 +0100, Daniel P. Berrangé wrote:
> > > On Fri, Jul 20, 2018 at 12:24:50PM +0200, Ján Tomko wrote:
> > > > I do not oppose reverting this bit and failing by default if we don't
> > > > have a JSON library (as Andrea mentioned, more drivers might possibly
> > > > require a working JSON implementation).
> > > 
> > > We use virjson.h throughout src/util and src/rpc so that code is
> > > not even driver specific, however, there are not currently mingw
> > > pacakges for jansson in Fedora at least. Maybe it will work, but
> > > we would need someone todo the work to add that to Fedora before
> > > we can consider making this mandatory.
> > 
> > So does the MinGW build work at all without jansson, considering its
> > functionality is fairly reduced in scope compared to the Unix builds?
> 
> Partially answering myself (morning coffee hasn't quite kicked in
> just yet), the MinGW .spec doesn't list yajl as a BuildRequires,
> which means that either we're not hitting any of the code paths you
> mention on that platform, or we've been producing completely useless
> Windows builds for a while now :)

The remote driver works fine without JSON being required.

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

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

Re: [libvirt] [PATCHv2] replace 'if' type conditions with 'switch' for VIR_NETWORK_FORWARD_

[Erik Skultety]

On Mon, Jul 23, 2018 at 12:03:57PM +0800, Shi Lei wrote:
> v1 patch here: 
> https://www.redhat.com/archives/libvir-list/2018-July/msg01314.html
>
> since v1:
> 1. Change the type declaration of _virNetworkForwardDef.type
>  from int to virNetworkForwardType

Maybe I wasn't 100% clear in my response to v1, you have to do the typecast
explicitly in all the switches rather than change the type to an enum in the
struct definition, since the compiler can decide the enum to be unsigned which
means that the return value from the virNetworkForwardTypeFromString call in
virNetworkForwardDefParseXML might get usigned-wrapped, ergo the check for a
non-existent type would be a contradiction.

> 2. use the default case to report out of range error with
>  virReportEnumRangeError

Another thing, it's cool that you incorporated the diff summary. However, this
shouldn't ever be part of the commit message, rather it should be mentioned as
a note in the note section below the "dashed line"

>
> Signed-off-by: Shi Lei 
> ---
>  src/conf/domain_conf.c   |  49 ---
>  src/conf/network_conf.c  |  73 +++---
>  src/conf/network_conf.h  |   2 +-
>  src/conf/virnetworkobj.c |  24 +++-
>  src/esx/esx_network_driver.c |  19 ++-
>  src/libvirt_private.syms |   1 +
>  src/network/bridge_driver.c  | 309 
> ---
>  src/qemu/qemu_process.c  |   8 ++
>  8 files changed, 329 insertions(+), 156 deletions(-)
>

...

> +if (virNetworkObjIsActive(obj)) {
> +switch (def->forward.type) {
> +case VIR_NETWORK_FORWARD_NONE:
> +case VIR_NETWORK_FORWARD_NAT:
> +case VIR_NETWORK_FORWARD_ROUTE:
> +/* Only three of the L3 network types that are configured by
> + * libvirt need to have iptables rules reloaded. The 4th L3
> + * network type, forward='open', doesn't need this because it
> + * has no iptables rules.
> + */
> +networkRemoveFirewallRules(def);
> +/* No need to check return value since already logged internally 
> */

You can drop ^this comment, the fact that we're purposefully ignoring the return
value should tell the reader something ;).

> +ignore_value(networkAddFirewallRules(def));
> +break;
> +

Thanks,
Erik

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

Re: [libvirt] [PATCH v2 2/5] Forget last daemon/ dir artefacts

[Michal Prívozník]

On 07/21/2018 02:11 PM, John Ferlan wrote:
> 
> 
> On 07/12/2018 03:37 AM, Michal Privoznik wrote:
>> The most important part is LIBVIRTD_PATH env var fix. It is used
>> in virFileFindResourceFull() from tests. The libvirtd no longer
>> lives under daemon/.
>>
>> Then, libvirtd-fail test was still failing (as expected) but not
>> because of missing config file but because it was trying to
>> execute (nonexistent) top_builddir/daemon/libvirtd which
>> fulfilled expected outcome and thus test did not fail.
>>
>> Thirdly, lcov was told to generate coverage for daemon/ dir too.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  Makefile.am | 2 +-
>>  run.in  | 2 +-
>>  tests/libvirtd-fail | 4 ++--
>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index 1926e21b7a..709064c6a6 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -80,7 +80,7 @@ check-access:
>>  cov: clean-cov
>>  $(MKDIR_P) $(top_builddir)/coverage
>>  $(LCOV) -c -o $(top_builddir)/coverage/libvirt.info.tmp \
>> -  -d $(top_builddir)/src  -d $(top_builddir)/daemon \
>> +  -d $(top_builddir)/src \
> 
> Since daemon is the former name and this appears to be a clean label
> target for coverage, perhaps we should keep daemon just to clean up
> "old" trees...
> 

No. This is no a clean label. The rule says: in order to make target
"cov" you need to make target "clean-cov" first as it is dependency. So
I'm changing the create target not the cleanup. And -d $dir to lcov
means "include directory $dir to search for .da files" (whatever they
are - doesn't matter now).

Michal

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