Re: [libvirt] [PATCH v3 3/3] tests : Testcases for loadparm

[John Ferlan]

On 05/23/2017 09:27 AM, Farhan Ali wrote:
> Add testcases for loadparm
> 
> Signed-off-by: Farhan Ali 
> Reviewed-by: Boris Fiuczynski 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Marc Hartmayer 
> ---
>  ...-machine-loadparm-multiple-disks-nets-s390.args | 28 ++
>  ...v-machine-loadparm-multiple-disks-nets-s390.xml | 43 
> ++
>  .../qemuxml2argv-machine-loadparm-net-s390.args| 20 ++
>  .../qemuxml2argv-machine-loadparm-net-s390.xml | 26 +
>  ...xml2argv-machine-loadparm-s390-char-invalid.xml | 26 +
>  ...uxml2argv-machine-loadparm-s390-len-invalid.xml | 26 +
>  .../qemuxml2argv-machine-loadparm-s390.args| 20 ++
>  .../qemuxml2argv-machine-loadparm-s390.xml | 26 +
>  tests/qemuxml2argvtest.c   | 19 ++
>  9 files changed, 234 insertions(+)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-multiple-disks-nets-s390.args
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-multiple-disks-nets-s390.xml
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-net-s390.args
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-net-s390.xml
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390-char-invalid.xml
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390-len-invalid.xml
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390.args
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-machine-loadparm-s390.xml
> 

These all seem reasonable - but could be merged with patch2 which had
the qemu changes.


John

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

Re: [libvirt] [PATCH v3 2/3] qemu : Add loadparm to qemu command line string

[John Ferlan]

On 05/23/2017 09:27 AM, Farhan Ali wrote:
> Introduce a new QEMU capability for loadparm and if the capability is
> supported by QEMU then append the loadparm value to "-machine" string
> of qemu command line.
> 
> Signed-off-by: Farhan Ali 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Boris Fiuczynski 
> Reviewed-by: Marc Hartmayer 
> ---
>  src/qemu/qemu_capabilities.c |  2 ++
>  src/qemu/qemu_capabilities.h |  1 +
>  src/qemu/qemu_command.c  | 37 +
>  3 files changed, 40 insertions(+)
> 

I was wondering why there weren't any changes to the tests/
qemucapabilitiesdata/*s390*.xml files... Until I realized this is a
really new feature for qemu... As in post 2.9 and there is no 2.10
*.replies and *.xml file yet created...

The *.xml files add the  entries for whatever qemu
versions the machine loadparm=xxx flag can be found in order to define
the flag to create the .args.  But you have .args in v3, which is
confusing (it's late too).

See commit '6b5c6314' for a recently added capability for kernel-irqchip
which modified a number of tests/qemucapabilitiesdata/* output .xml files.

There's also "tests/qemucapsprobe /path/to/binary >caps.replies" which
is what's used to generate those .replies files (those could be added as
part of this or the xml2xml commit).

Sometimes the patch order choice is have patch 1 have all the necessary
flag stuff done, patch 2 the xml/rng/docs, patch 3 the qemu, and patch 4
the news.xml.

> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 546dfd7..e3bd445 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -371,6 +371,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>"kernel-irqchip.split",
>"intel-iommu.intremap",
>"intel-iommu.caching-mode",
> +  "loadparm",
>  );
>  
>  
> @@ -3144,6 +3145,7 @@ static struct virQEMUCapsCommandLineProps 
> virQEMUCapsCommandLine[] = {
>  { "drive", "throttling.group", QEMU_CAPS_DRIVE_IOTUNE_GROUP },
>  { "spice", "rendernode", QEMU_CAPS_SPICE_RENDERNODE },
>  { "machine", "kernel_irqchip", QEMU_CAPS_MACHINE_KERNEL_IRQCHIP },
> +{ "machine", "loadparm", QEMU_CAPS_LOADPARM },
>  };
>  
>  static int
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index aa99fda..a18c61c 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -409,6 +409,7 @@ typedef enum {
>  QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT, /* -machine kernel_irqchip=split 
> */
>  QEMU_CAPS_INTEL_IOMMU_INTREMAP, /* intel-iommu.intremap */
>  QEMU_CAPS_INTEL_IOMMU_CACHING_MODE, /* intel-iommu.caching-mode */
> +QEMU_CAPS_LOADPARM, /* -machine loadparm */
>  
>  QEMU_CAPS_LAST /* this must always be the last item */
>  } virQEMUCapsFlags;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index aa66e3d..1291b62 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7226,6 +7226,41 @@ qemuAppendKeyWrapMachineParms(virBuffer *buf, 
> virQEMUCapsPtr qemuCaps,
>  return true;
>  }
>  

The newer style is two lines between functions

> +static void
> +qemuAppendLoadparmMachineParm(virBuffer *buf,
> +  const virDomainDef *def,
> +  virQEMUCapsPtr qemuCaps)
> +{
> +size_t i = 0;
> +
> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX) ||
> +!virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM))
> +return;


Typically when the flag doesn't exist, we elicit an error message and
cause failure. I'm still scratching my head over how this works for test
when the flags aren't in the XML file, but I'm way too tired to think to
hard about it.

> +
> +for (i = 0; i < def->ndisks; i++) {
> +virDomainDiskDefPtr disk = def->disks[i];
> +
> +if (disk->info.bootIndex == 1) {
> +if (disk->info.loadparm)

I think you could combine the if conditions... Unless there's thoughts
that other bootIndex's would work...  Could end up having arch specific
stuff (yuck).

> +virBufferAsprintf(buf, ",loadparm=%s", 
> disk->info.loadparm);> +
> +return;
> +}
> +}
> +
> +/* Network boot device*/
 ^
Need a space before end of comment

> +for (i = 0; i < def->nnets; i++) {
> +virDomainNetDefPtr net = def->nets[i];
> +
> +if (net->info.bootIndex == 1) {
> +if (net->info.loadparm)
> +virBufferAsprintf(buf, ",loadparm=%s", net->info.loadparm);
> +
> +return;
> +}
> +}
> +}
> +

Two lines between functions

>  static int
>  qemuBuildNameCommandLine(virCommandPtr cmd,
>   virQEMUDriverConfigPtr cfg,
> @@ -7315,6 +7350,8 @@ 

Re: [libvirt] [PATCH v3 1/3] conf : Add loadparm boot option for a boot device

[John Ferlan]

On 05/23/2017 09:27 AM, Farhan Ali wrote:
> Update the per device boot schema to add an optional loadparm parameter.
> Extend the virDomainDeviceInfo to support loadparm option.
> Modify the appropriate functions to parse loadparm from boot device xml.
> 

FWIW: I got confused by the time I got to patch 3... the "loadparm" add
added to the -machine qemu command line, but the libvirt XML is added to
the disk device entry.

Perhaps just add a sample here to show where the XML gets added.

> Signed-off-by: Farhan Ali 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Boris Fiuczynski 
> Reviewed-by: Marc Hartmayer 
> ---
>  docs/formatdomain.html.in |  9 --
>  docs/news.xml | 11 +++

Patch ordering - the docs/news.xml should be a single commit and would
be the the last commit.  Still at least you have it there.

>  docs/schemas/domaincommon.rng |  7 +
>  src/conf/device_conf.h|  1 +
>  src/conf/domain_conf.c| 69 
> +--

When you change/add RNG/XML - there should be adjustments to
qemuxml2xmltest.c and of course xml2xml tests.  Many examples of this to
draw from.

>  5 files changed, 93 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 3135db4..fd6f666 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3022,8 +3022,13 @@
>boot
>Specifies that the disk is bootable. The order
>  attribute determines the order in which devices will be tried during
> -boot sequence. The per-device boot elements cannot be
> -used together with general boot elements in
> +boot sequence. On S390 architecture only the first boot device is 
> used.
> +The optional loadparm attribute is an 8 byte parameter
> +which can be queried by guests on S390 via sclp or diag 308. Linux
> +guests on S390 can use loadparm to select a boot entry.
> +Since 3.4.0

Since DV is cutting 3.4.0 tomorrow, this probably slides into 3.5.0.
Given when I realized by patch 2, probably no big deal!

> +The per-device boot elements cannot be used together
> +with general boot elements in
>  BIOS bootloader section.
>  Since 0.8.8
>
> diff --git a/docs/news.xml b/docs/news.xml
> index 52915ee..bd9e43a 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -37,6 +37,17 @@
>  
>
>  
> +  qemu: Add support for loadparm for a boot device
> +
> +
> +  Add an optional boot parameter 'loadparm' for a boot device.
> +  Loadparm is a 8 byte parameter than can be queried by S390 guests

s/a 8/an 8/

s/can be/when present is/ ?

> +  via sclp or diag 308. Linux guests on S390 use it to select a boot
> +  entry.
> +
> +  
> +  
> +
>Improved streams to efficiently transfer sparseness
>  
>  

The rest seems fine -

John

> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index f88e84a..c885aec 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -5026,6 +5026,13 @@
>
>  
>
> +  
> +
> +  
> +[a-zA-Z0-9.\s]{1,8}
> +  
> +
> +  
>
>  
>
> diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
> index a20de85..48782be 100644
> --- a/src/conf/device_conf.h
> +++ b/src/conf/device_conf.h
> @@ -167,6 +167,7 @@ struct _virDomainDeviceInfo {
>   * assignment, never saved and never reported.
>   */
>  int pciConnectFlags; /* enum virDomainPCIConnectFlags */
> +char *loadparm;
>  };
>  
>  
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 8c62673..dbf6108 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3520,6 +3520,7 @@ void virDomainDeviceInfoClear(virDomainDeviceInfoPtr 
> info)
>  memset(>addr, 0, sizeof(info->addr));
>  info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
>  VIR_FREE(info->romfile);
> +VIR_FREE(info->loadparm);
>  }
>  
>  
> @@ -5199,6 +5200,48 @@ virDomainDefValidate(virDomainDefPtr def,
>  return 0;
>  }
>  
> +/**
> + * virDomainDeviceLoadparmIsValid
> + * @loadparm : The string to validate
> + *
> + * The valid set of values for loadparm are [a-zA-Z0-9.]
> + * and blank spaces.
> + * The maximum allowed length is 8 characters.
> + * An empty string is considered invalid
> + */
> +static bool
> +virDomainDeviceLoadparmIsValid(const char *loadparm)
> +{
> +size_t i;
> +
> +if (virStringIsEmpty(loadparm)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("loadparm cannot be an empty string"));
> +return false;
> +}
> +
> +if (strlen(loadparm) > 8) 

Re: [libvirt] [PATCH 1/1] CI: add code coverage analysis

[Claudio André]

Em 25/05/2017 03:49, Martin Kletzander escreveu:
On Thu, May 25, 2017 at 01:14:36AM -0300, claudioandre...@gmail.com 
wrote:

From: Claudio André 

It builds the code coverage report and uploads the coverage data to a 
web service in order to allow to track libvirt's code coverage over 
time.

---


You can add the links from the cover letter right here.  It is below the
'---' marker, so it won't be part of the commit message, but it is also
before the first 'diff --git', so it won't be considered part of the
patch either.  Just like the stats below.



Thanks for the information


+after_success:
+  - 'if [ -n "${COVERAGE}" ]; then
+make -j3 cov;
+coveralls-lcov --repo-token ${COVERALLS_TOKEN} 
coverage/libvirt.info;


I like this, but I have no idea where COVERALLS_TOKEN comes from.


The Token comes from coveralls.io site.
1. create your/libvirt account on coveralls (using GitHub credentials)
2. enable the libvirt repository and you will be able to see a field 
named: REPO TOKEN.
3. then, using Travis website (or Travis CLI tool, I prefer the first 
for this case) you save the token ciphered.


For Coveralls, click here and there and you will find your way to enable 
the repository and get the token (probably 
https://coveralls.io/github/YOUR_NICK/libvirt/settings)
For Travis: https://travis-ci.org/YOUR_NICK/libvirt/settings, you'll see 
"Environment Variables" and the add button.



 How do we make sure that no user branches will post stuff to the
coveralls.io site?


Because Travis save the token ciphered, nobody else can see the token. 
In fact, if you save it using the site, it does not exist even in the 
.travis.yml file.
And if Travis, Coveralls, ..., are compromised, you can easily 
regenerate the Token fast enough to avoid damage.


Claudio
Sorry if the message sounds, umm, pedantic.

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

Re: [libvirt] [PATCH 1/1] CI: add code coverage analysis

[Claudio André]

Em 25/05/2017 05:21, Daniel P. Berrange escreveu:

On Thu, May 25, 2017 at 01:14:36AM -0300, claudioandre...@gmail.com wrote:

@@ -64,7 +65,8 @@ before_install:
  
  # the custom PATH is just to pick up OS-X homebrew & its harmless on Linux

  before_script:
-  - PATH="/usr/local/opt/gettext/bin:/usr/local/opt/rpcgen/bin:$PATH" 
./autogen.sh
+  - PATH="/usr/local/opt/gettext/bin:/usr/local/opt/rpcgen/bin:$PATH" ./autogen.sh 
"$COVERAGE"
+  - gem install coveralls-lcov

Does this actually work on OS-X ? The fact that you only enabled
testing on Trusty suggests not, and if so you shouldn't run this
in the common path.


Yes, it does work on OS-X (I tested on 10.11). I enabled only on Trusty 
because I prefer to use the latest LTS to it (I mean, the latest version 
and LTS).


That said, do you want any change?

Claudio


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

Re: [libvirt] [PATCH] CI: show the log in case of failure

[Claudio André]

Em 25/05/2017 11:15, Martin Kletzander escreveu:

I need to investigate why this doesn't work, I don't have that file


[...]

Just figured that out.  The systems in Travis are just so old that the
automake there doesn't create the test-suite.log =(


The workers available in Travis are really old. That said, I plan to 
workaround your finding and propose a patch:
- to build and test libvirt using a more updated distro (using 
`services: docker`, of course).

- and, if accepted, add ASAN (AddressSanitizer) to CI.

Claudio

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

[libvirt] [PATCH 2/2] lxc: fixing wrong VIR_FREE after a return statement.

[Julio Faracco]

There is a VIR_FREE after a return statement. That code section is never
executed and for this reason the "tty" variable is not being freed. This
commit rearrange the logic.

Signed-off-by: Julio Faracco 
---
 src/lxc/lxc_container.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index a936342..af02b54 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -1143,8 +1143,8 @@ static int lxcContainerSetupDevices(char **ttyPaths, 
size_t nttyPaths)
 return -1;
 
 if (virFileBindMountDevice(ttyPaths[i], tty) < 0) {
-return -1;
 VIR_FREE(tty);
+return -1;
 }
 
 VIR_FREE(tty);
-- 
1.8.3.1

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

[libvirt] [PATCH 1/2] configure: fixing acl missing variable.

[Julio Faracco]

This commit fixes an acl missing variable. The virt-acl.m4 inside the
macro directory does not contain the variable 'with_acl'. So, it is
being set as an empty string "with_acl=''". This is causing a missing
option during the configuration, even if you have acl libs installed.

Signed-off-by: Julio Faracco 
---
 m4/virt-acl.m4 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/m4/virt-acl.m4 b/m4/virt-acl.m4
index f7d1c6d..d548729 100644
--- a/m4/virt-acl.m4
+++ b/m4/virt-acl.m4
@@ -23,8 +23,10 @@ AC_DEFUN([LIBVIRT_CHECK_ACL], [
 
   ACL_CFLAGS=""
   ACL_LIBS=""
+  with_acl=no
   if test "x$ac_cv_header_sys_acl_h:x$with_linux" = "xyes:xyes"; then
 ACL_LIBS="-lacl"
+with_acl=yes
   fi
   AC_SUBST([ACL_CFLAGS])
   AC_SUBST([ACL_LIBS])
-- 
1.8.3.1

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

[libvirt] [PATCH v2 13/14] nodedev: Pass @def by reference to create/assign object

[John Ferlan]

Since the @def is consumed by the assignment function, let's pass by
reference instead of value and really consume it.

Signed-off-by: John Ferlan 
---
 src/conf/virnodedeviceobj.c| 8 
 src/conf/virnodedeviceobj.h| 2 +-
 src/node_device/node_device_hal.c  | 2 +-
 src/node_device/node_device_udev.c | 8 +++-
 src/test/test_driver.c | 5 ++---
 5 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index a7e51ef..1648b33 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -268,13 +268,13 @@ virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs)
 
 virNodeDeviceObjPtr
 virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs,
-  virNodeDeviceDefPtr def)
+  virNodeDeviceDefPtr *def)
 {
 virNodeDeviceObjPtr obj;
 
-if ((obj = virNodeDeviceObjFindByName(devs, def->name))) {
+if ((obj = virNodeDeviceObjFindByName(devs, (*def)->name))) {
 virNodeDeviceDefFree(obj->def);
-obj->def = def;
+VIR_STEAL_PTR(obj->def, *def);
 return obj;
 }
 
@@ -294,7 +294,7 @@ virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs,
 virNodeDeviceObjFree(obj);
 return NULL;
 }
-obj->def = def;
+VIR_STEAL_PTR(obj->def, *def);
 
 return obj;
 
diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h
index 135a424..49c28e7 100644
--- a/src/conf/virnodedeviceobj.h
+++ b/src/conf/virnodedeviceobj.h
@@ -53,7 +53,7 @@ virNodeDeviceObjFindBySysfsPath(virNodeDeviceObjListPtr devs,
 
 virNodeDeviceObjPtr
 virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs,
-  virNodeDeviceDefPtr def);
+  virNodeDeviceDefPtr *def);
 
 void
 virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs,
diff --git a/src/node_device/node_device_hal.c 
b/src/node_device/node_device_hal.c
index e9031ea..2d996a9 100644
--- a/src/node_device/node_device_hal.c
+++ b/src/node_device/node_device_hal.c
@@ -482,7 +482,7 @@ dev_create(const char *udi)
 /* Some devices don't have a path in sysfs, so ignore failure */
 (void)get_str_prop(ctx, udi, "linux.sysfs_path", );
 
-if (!(obj = virNodeDeviceObjAssignDef(>devs, def))) {
+if (!(obj = virNodeDeviceObjAssignDef(>devs, ))) {
 VIR_FREE(devicePath);
 goto failure;
 }
diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index 83a8fcc..0250aab 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1395,7 +1395,7 @@ udevAddOneDevice(struct udev_device *device)
 
 /* If this is a device change, the old definition will be freed
  * and the current definition will take its place. */
-if (!(obj = virNodeDeviceObjAssignDef(>devs, def)))
+if (!(obj = virNodeDeviceObjAssignDef(>devs, )))
 goto cleanup;
 objdef = virNodeDeviceObjGetDef(obj);
 
@@ -1685,8 +1685,7 @@ udevSetupSystemDev(void)
 udevGetDMIData(>caps->data.system);
 #endif
 
-obj = virNodeDeviceObjAssignDef(>devs, def);
-if (obj == NULL)
+if (!(obj = virNodeDeviceObjAssignDef(>devs, )))
 goto cleanup;
 
 virNodeDeviceObjUnlock(obj);
@@ -1694,8 +1693,7 @@ udevSetupSystemDev(void)
 ret = 0;
 
  cleanup:
-if (ret == -1)
-virNodeDeviceDefFree(def);
+virNodeDeviceDefFree(def);
 
 return ret;
 }
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 206fdf9..84ff1de 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1168,7 +1168,7 @@ testParseNodedevs(testDriverPtr privconn,
 if (!def)
 goto error;
 
-if (!(obj = virNodeDeviceObjAssignDef(>devs, def))) {
+if (!(obj = virNodeDeviceObjAssignDef(>devs, ))) {
 virNodeDeviceDefFree(def);
 goto error;
 }
@@ -5491,9 +5491,8 @@ testNodeDeviceMockCreateVport(testDriverPtr driver,
 caps = caps->next;
 }
 
-if (!(obj = virNodeDeviceObjAssignDef(>devs, def)))
+if (!(obj = virNodeDeviceObjAssignDef(>devs, )))
 goto cleanup;
-def = NULL;
 objdef = virNodeDeviceObjGetDef(obj);
 
 event = virNodeDeviceEventLifecycleNew(objdef->name,
-- 
2.9.4

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

[libvirt] [PATCH v2 09/14] nodedev: Create helper for finding by name in driver

[John Ferlan]

Create nodeDeviceObjFindByName which will perform the corresponding
virNodeDeviceObjFindByName call for various node_device_driver callers
rather than having the same repetitive code.

Signed-off-by: John Ferlan 
---
 src/node_device/node_device_driver.c | 96 +---
 1 file changed, 33 insertions(+), 63 deletions(-)

diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index b599460..4b8a66e 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -229,12 +229,10 @@ nodeConnectListAllNodeDevices(virConnectPtr conn,
 }
 
 
-virNodeDevicePtr
-nodeDeviceLookupByName(virConnectPtr conn,
-   const char *name)
+static virNodeDeviceObjPtr
+nodeDeviceObjFindByName(const char *name)
 {
 virNodeDeviceObjPtr obj;
-virNodeDevicePtr ret = NULL;
 
 nodeDeviceLock();
 obj = virNodeDeviceObjFindByName(>devs, name);
@@ -244,9 +242,22 @@ nodeDeviceLookupByName(virConnectPtr conn,
 virReportError(VIR_ERR_NO_NODE_DEVICE,
_("no node device with matching name '%s'"),
name);
-goto cleanup;
 }
 
+return obj;
+}
+
+
+virNodeDevicePtr
+nodeDeviceLookupByName(virConnectPtr conn,
+   const char *name)
+{
+virNodeDeviceObjPtr obj;
+virNodeDevicePtr ret = NULL;
+
+if (!(obj = nodeDeviceObjFindByName(name)))
+return NULL;
+
 if (virNodeDeviceLookupByNameEnsureACL(conn, obj->def) < 0)
 goto cleanup;
 
@@ -256,8 +267,7 @@ nodeDeviceLookupByName(virConnectPtr conn,
 }
 
  cleanup:
-if (obj)
-virNodeDeviceObjUnlock(obj);
+virNodeDeviceObjUnlock(obj);
 return ret;
 }
 
@@ -324,16 +334,8 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev,
 
 virCheckFlags(0, NULL);
 
-nodeDeviceLock();
-obj = virNodeDeviceObjFindByName(>devs, dev->name);
-nodeDeviceUnlock();
-
-if (!obj) {
-virReportError(VIR_ERR_NO_NODE_DEVICE,
-   _("no node device with matching name '%s'"),
-   dev->name);
-goto cleanup;
-}
+if (!(obj = nodeDeviceObjFindByName(dev->name)))
+return NULL;
 
 if (virNodeDeviceGetXMLDescEnsureACL(dev->conn, obj->def) < 0)
 goto cleanup;
@@ -347,8 +349,7 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev,
 ret = virNodeDeviceDefFormat(obj->def);
 
  cleanup:
-if (obj)
-virNodeDeviceObjUnlock(obj);
+virNodeDeviceObjUnlock(obj);
 return ret;
 }
 
@@ -359,16 +360,8 @@ nodeDeviceGetParent(virNodeDevicePtr dev)
 virNodeDeviceObjPtr obj;
 char *ret = NULL;
 
-nodeDeviceLock();
-obj = virNodeDeviceObjFindByName(>devs, dev->name);
-nodeDeviceUnlock();
-
-if (!obj) {
-virReportError(VIR_ERR_NO_NODE_DEVICE,
-   _("no node device with matching name '%s'"),
-   dev->name);
-goto cleanup;
-}
+if (!(obj = nodeDeviceObjFindByName(dev->name)))
+return NULL;
 
 if (virNodeDeviceGetParentEnsureACL(dev->conn, obj->def) < 0)
 goto cleanup;
@@ -382,8 +375,7 @@ nodeDeviceGetParent(virNodeDevicePtr dev)
 }
 
  cleanup:
-if (obj)
-virNodeDeviceObjUnlock(obj);
+virNodeDeviceObjUnlock(obj);
 return ret;
 }
 
@@ -396,16 +388,8 @@ nodeDeviceNumOfCaps(virNodeDevicePtr dev)
 int ncaps = 0;
 int ret = -1;
 
-nodeDeviceLock();
-obj = virNodeDeviceObjFindByName(>devs, dev->name);
-nodeDeviceUnlock();
-
-if (!obj) {
-virReportError(VIR_ERR_NO_NODE_DEVICE,
-   _("no node device with matching name '%s'"),
-   dev->name);
-goto cleanup;
-}
+if (!(obj = nodeDeviceObjFindByName(dev->name)))
+return -1;
 
 if (virNodeDeviceNumOfCapsEnsureACL(dev->conn, obj->def) < 0)
 goto cleanup;
@@ -427,8 +411,7 @@ nodeDeviceNumOfCaps(virNodeDevicePtr dev)
 ret = ncaps;
 
  cleanup:
-if (obj)
-virNodeDeviceObjUnlock(obj);
+virNodeDeviceObjUnlock(obj);
 return ret;
 }
 
@@ -444,16 +427,8 @@ nodeDeviceListCaps(virNodeDevicePtr dev,
 int ncaps = 0;
 int ret = -1;
 
-nodeDeviceLock();
-obj = virNodeDeviceObjFindByName(>devs, dev->name);
-nodeDeviceUnlock();
-
-if (!obj) {
-virReportError(VIR_ERR_NO_NODE_DEVICE,
-   _("no node device with matching name '%s'"),
-   dev->name);
-goto cleanup;
-}
+if (!(obj = nodeDeviceObjFindByName(dev->name)))
+return -1;
 
 if (virNodeDeviceListCapsEnsureACL(dev->conn, obj->def) < 0)
 goto cleanup;
@@ -483,8 +458,7 @@ nodeDeviceListCaps(virNodeDevicePtr dev,
 ret = ncaps;
 
  cleanup:
-if (obj)
-virNodeDeviceObjUnlock(obj);
+virNodeDeviceObjUnlock(obj);
 if (ret == -1) {
 --ncaps;
 while (--ncaps >= 0)
@@ -619,13 +593,10 @@ 

[libvirt] [PATCH v2 12/14] nodedev: Remove privateData from virNodeDeviceObj

[John Ferlan]

It was only ever used in node_device_hal.c which really never used it
anyway since the NODE_DEV_UDI was never referenced.  Remove free_udi()
and @privData as well as the references to obj->privateData & obj->privateFree.

Signed-off-by: John Ferlan 
---
 src/conf/node_device_conf.h   |  2 --
 src/conf/virnodedeviceobj.c   |  2 --
 src/node_device/node_device_hal.c | 15 ---
 3 files changed, 19 deletions(-)

diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index 5743f9d..0ab2b96 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -318,8 +318,6 @@ struct _virNodeDeviceObj {
 virMutex lock;
 
 virNodeDeviceDefPtr def;   /* device definition */
-void *privateData; /* driver-specific private data */
-void (*privateFree)(void *data);   /* destructor for private data */
 
 };
 
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 1b9d032..a7e51ef 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -248,8 +248,6 @@ virNodeDeviceObjFree(virNodeDeviceObjPtr obj)
 return;
 
 virNodeDeviceDefFree(obj->def);
-if (obj->privateFree)
-(*obj->privateFree)(obj->privateData);
 
 virMutexDestroy(>lock);
 
diff --git a/src/node_device/node_device_hal.c 
b/src/node_device/node_device_hal.c
index cde1c7b..e9031ea 100644
--- a/src/node_device/node_device_hal.c
+++ b/src/node_device/node_device_hal.c
@@ -52,8 +52,6 @@ VIR_LOG_INIT("node_device.node_device_hal");
 
 #define DRV_STATE_HAL_CTX(ds) ((LibHalContext *)((ds)->privateData))
 
-#define NODE_DEV_UDI(obj) ((const char *)((obj)->privateData)
-
 
 static const char *
 hal_name(const char *udi)
@@ -447,12 +445,6 @@ gather_capabilities(LibHalContext *ctx, const char *udi,
 }
 
 static void
-free_udi(void *udi)
-{
-VIR_FREE(udi);
-}
-
-static void
 dev_create(const char *udi)
 {
 LibHalContext *ctx;
@@ -462,12 +454,8 @@ dev_create(const char *udi)
 virNodeDeviceDefPtr objdef;
 const char *name = hal_name(udi);
 int rv;
-char *privData;
 char *devicePath = NULL;
 
-if (VIR_STRDUP(privData, udi) < 0)
-return;
-
 nodeDeviceLock();
 ctx = DRV_STATE_HAL_CTX(driver);
 
@@ -500,8 +488,6 @@ dev_create(const char *udi)
 }
 objdef = virNodeDeviceObjGetDef(obj);
 
-obj->privateData = privData;
-obj->privateFree = free_udi;
 objdef->sysfs_path = devicePath;
 
 virNodeDeviceObjUnlock(obj);
@@ -512,7 +498,6 @@ dev_create(const char *udi)
  failure:
 VIR_DEBUG("FAILED TO ADD dev %s", name);
  cleanup:
-VIR_FREE(privData);
 virNodeDeviceDefFree(def);
 nodeDeviceUnlock();
 }
-- 
2.9.4

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

[libvirt] [PATCH v2 14/14] nodedev: Introduce virNodeDeviceObjNew

[John Ferlan]

Create an allocator for the virNodeDeviceObjPtr - include setting up
the mutex, saving the virNodeDeviceDefPtr, and locking the return object.

Signed-off-by: John Ferlan 
---
 src/conf/virnodedeviceobj.c | 33 ++---
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 1648b33..a84266b 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -33,6 +33,27 @@
 VIR_LOG_INIT("conf.virnodedeviceobj");
 
 
+static virNodeDeviceObjPtr
+virNodeDeviceObjNew(virNodeDeviceDefPtr *def)
+{
+virNodeDeviceObjPtr obj;
+
+if (VIR_ALLOC(obj) < 0)
+return NULL;
+
+if (virMutexInit(>lock) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   "%s", _("cannot initialize mutex"));
+VIR_FREE(obj);
+return NULL;
+}
+virNodeDeviceObjLock(obj);
+VIR_STEAL_PTR(obj->def, *def);
+
+return obj;
+}
+
+
 virNodeDeviceDefPtr
 virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj)
 {
@@ -278,26 +299,16 @@ virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs,
 return obj;
 }
 
-if (VIR_ALLOC(obj) < 0)
+if (!(obj = virNodeDeviceObjNew(def)))
 return NULL;
 
-if (virMutexInit(>lock) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("cannot initialize mutex"));
-VIR_FREE(obj);
-return NULL;
-}
-virNodeDeviceObjLock(obj);
-
 if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, obj) < 0) {
 virNodeDeviceObjUnlock(obj);
 virNodeDeviceObjFree(obj);
 return NULL;
 }
-VIR_STEAL_PTR(obj->def, *def);
 
 return obj;
-
 }
 
 
-- 
2.9.4

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

[libvirt] [PATCH v2 04/14] nodedev: Use switch for virNodeDeviceObjHasCap and virNodeDeviceCapMatch

[John Ferlan]

In order to ensure that whenever something is added to virNodeDevCapType
that both functions are considered for processing of a new capability,
change the if-then-else construct into a switch statement.

Signed-off-by: John Ferlan 
---
 src/conf/virnodedeviceobj.c | 80 +
 1 file changed, 60 insertions(+), 20 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index bbb6eeb..913cdda 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -48,19 +48,41 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *dev,
 while (caps) {
 if (STREQ(cap, virNodeDevCapTypeToString(caps->data.type))) {
 return 1;
-} else if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) {
-if ((STREQ(cap, fc_host_cap) &&
-(caps->data.scsi_host.flags &
- VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)) ||
-(STREQ(cap, vports_cap) &&
-(caps->data.scsi_host.flags &
- VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS)))
-return 1;
-} else if (caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
-if ((STREQ(cap, mdev_types)) &&
-(caps->data.pci_dev.flags &
- VIR_NODE_DEV_CAP_FLAG_PCI_MDEV))
-return 1;
+} else {
+switch (caps->data.type) {
+case VIR_NODE_DEV_CAP_PCI_DEV:
+if ((STREQ(cap, mdev_types)) &&
+(caps->data.pci_dev.flags &
+ VIR_NODE_DEV_CAP_FLAG_PCI_MDEV))
+return 1;
+break;
+
+case VIR_NODE_DEV_CAP_SCSI_HOST:
+if ((STREQ(cap, fc_host_cap) &&
+(caps->data.scsi_host.flags &
+ VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)) ||
+(STREQ(cap, vports_cap) &&
+(caps->data.scsi_host.flags &
+ VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS)))
+return 1;
+break;
+
+case VIR_NODE_DEV_CAP_SYSTEM:
+case VIR_NODE_DEV_CAP_USB_DEV:
+case VIR_NODE_DEV_CAP_USB_INTERFACE:
+case VIR_NODE_DEV_CAP_NET:
+case VIR_NODE_DEV_CAP_SCSI_TARGET:
+case VIR_NODE_DEV_CAP_SCSI:
+case VIR_NODE_DEV_CAP_STORAGE:
+case VIR_NODE_DEV_CAP_FC_HOST:
+case VIR_NODE_DEV_CAP_VPORTS:
+case VIR_NODE_DEV_CAP_SCSI_GENERIC:
+case VIR_NODE_DEV_CAP_DRM:
+case VIR_NODE_DEV_CAP_MDEV_TYPES:
+case VIR_NODE_DEV_CAP_MDEV:
+case VIR_NODE_DEV_CAP_LAST:
+break;
+}
 }
 
 caps = caps->next;
@@ -468,7 +490,15 @@ virNodeDeviceCapMatch(virNodeDeviceObjPtr devobj,
 if (type == cap->data.type)
 return true;
 
-if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) {
+switch (cap->data.type) {
+case VIR_NODE_DEV_CAP_PCI_DEV:
+if (type == VIR_NODE_DEV_CAP_MDEV_TYPES &&
+(cap->data.pci_dev.flags &
+ VIR_NODE_DEV_CAP_FLAG_PCI_MDEV))
+return true;
+break;
+
+case VIR_NODE_DEV_CAP_SCSI_HOST:
 if (type == VIR_NODE_DEV_CAP_FC_HOST &&
 (cap->data.scsi_host.flags &
  VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST))
@@ -478,13 +508,23 @@ virNodeDeviceCapMatch(virNodeDeviceObjPtr devobj,
 (cap->data.scsi_host.flags &
  VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS))
 return true;
-}
+break;
 
-if (cap->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
-if (type == VIR_NODE_DEV_CAP_MDEV_TYPES &&
-(cap->data.pci_dev.flags &
- VIR_NODE_DEV_CAP_FLAG_PCI_MDEV))
-return true;
+case VIR_NODE_DEV_CAP_SYSTEM:
+case VIR_NODE_DEV_CAP_USB_DEV:
+case VIR_NODE_DEV_CAP_USB_INTERFACE:
+case VIR_NODE_DEV_CAP_NET:
+case VIR_NODE_DEV_CAP_SCSI_TARGET:
+case VIR_NODE_DEV_CAP_SCSI:
+case VIR_NODE_DEV_CAP_STORAGE:
+case VIR_NODE_DEV_CAP_FC_HOST:
+case VIR_NODE_DEV_CAP_VPORTS:
+case VIR_NODE_DEV_CAP_SCSI_GENERIC:
+case VIR_NODE_DEV_CAP_DRM:
+case VIR_NODE_DEV_CAP_MDEV_TYPES:
+case VIR_NODE_DEV_CAP_MDEV:
+case VIR_NODE_DEV_CAP_LAST:
+break;
 }
 }
 
-- 
2.9.4

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

[libvirt] [PATCH v2 07/14] nodedev: Alter param to nodeDeviceUpdateDriverName

[John Ferlan]

Rather than taking an virNodeDeviceObjPtr and dereffing the obj->def,
just pass the def.

Also check for an error in the function to have the calling function goto
cleanup on error.

Signed-off-by: John Ferlan 
---
 src/node_device/node_device_driver.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index 2a461fb..87953f3 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -106,16 +106,16 @@ nodeDeviceUpdateCaps(virNodeDeviceObjPtr dev)
  * udev *and* HAL backends.
  */
 static int
-nodeDeviceUpdateDriverName(virNodeDeviceObjPtr dev)
+nodeDeviceUpdateDriverName(virNodeDeviceDefPtr def)
 {
 char *driver_link = NULL;
 char *devpath = NULL;
 char *p;
 int ret = -1;
 
-VIR_FREE(dev->def->driver);
+VIR_FREE(def->driver);
 
-if (virAsprintf(_link, "%s/driver", dev->def->sysfs_path) < 0)
+if (virAsprintf(_link, "%s/driver", def->sysfs_path) < 0)
 goto cleanup;
 
 /* Some devices don't have an explicit driver, so just return
@@ -132,7 +132,7 @@ nodeDeviceUpdateDriverName(virNodeDeviceObjPtr dev)
 }
 
 p = strrchr(devpath, '/');
-if (p && VIR_STRDUP(dev->def->driver, p + 1) < 0)
+if (p && VIR_STRDUP(def->driver, p + 1) < 0)
 goto cleanup;
 ret = 0;
 
@@ -144,7 +144,7 @@ nodeDeviceUpdateDriverName(virNodeDeviceObjPtr dev)
 #else
 /* XXX: Implement me for non-linux */
 static int
-nodeDeviceUpdateDriverName(virNodeDeviceObjPtr dev ATTRIBUTE_UNUSED)
+nodeDeviceUpdateDriverName(virNodeDeviceDefPtr def ATTRIBUTE_UNUSED)
 {
 return 0;
 }
@@ -338,7 +338,9 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev,
 if (virNodeDeviceGetXMLDescEnsureACL(dev->conn, obj->def) < 0)
 goto cleanup;
 
-nodeDeviceUpdateDriverName(obj);
+if (nodeDeviceUpdateDriverName(obj->def) < 0)
+goto cleanup;
+
 if (nodeDeviceUpdateCaps(obj) < 0)
 goto cleanup;
 
-- 
2.9.4

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

[libvirt] [PATCH v2 11/14] nodedev: Introduce virNodeDeviceObjGetDef

[John Ferlan]

In preparation for privatizing the virNodeDeviceObj - create an accessor
for the @def field and then use it for various callers.

Signed-off-by: John Ferlan 
---
 src/conf/virnodedeviceobj.c  |  7 +
 src/conf/virnodedeviceobj.h  |  2 ++
 src/libvirt_private.syms |  1 +
 src/node_device/node_device_driver.c | 52 ++--
 src/node_device/node_device_hal.c| 11 +---
 src/node_device/node_device_udev.c   | 35 
 src/test/test_driver.c   | 38 +-
 7 files changed, 92 insertions(+), 54 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index a2d09ad..1b9d032 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -33,6 +33,13 @@
 VIR_LOG_INIT("conf.virnodedeviceobj");
 
 
+virNodeDeviceDefPtr
+virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj)
+{
+return obj->def;
+}
+
+
 static int
 virNodeDeviceObjHasCap(const virNodeDeviceObj *obj,
const char *cap)
diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h
index b8b534b..135a424 100644
--- a/src/conf/virnodedeviceobj.h
+++ b/src/conf/virnodedeviceobj.h
@@ -39,6 +39,8 @@ struct _virNodeDeviceDriverState {
 virObjectEventStatePtr nodeDeviceEventState;
 };
 
+virNodeDeviceDefPtr
+virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj);
 
 virNodeDeviceObjPtr
 virNodeDeviceObjFindByName(virNodeDeviceObjListPtr devs,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d361454..222e3b2 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -955,6 +955,7 @@ virNetworkObjUpdateAssignDef;
 virNodeDeviceObjAssignDef;
 virNodeDeviceObjFindByName;
 virNodeDeviceObjFindBySysfsPath;
+virNodeDeviceObjGetDef;
 virNodeDeviceObjGetNames;
 virNodeDeviceObjGetParentHost;
 virNodeDeviceObjListExport;
diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index d34f399..93aa2af 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -253,16 +253,18 @@ nodeDeviceLookupByName(virConnectPtr conn,
const char *name)
 {
 virNodeDeviceObjPtr obj;
+virNodeDeviceDefPtr def;
 virNodeDevicePtr device = NULL;
 
 if (!(obj = nodeDeviceObjFindByName(name)))
 return NULL;
+def = virNodeDeviceObjGetDef(obj);
 
-if (virNodeDeviceLookupByNameEnsureACL(conn, obj->def) < 0)
+if (virNodeDeviceLookupByNameEnsureACL(conn, def) < 0)
 goto cleanup;
 
 if ((device = virGetNodeDevice(conn, name))) {
-if (VIR_STRDUP(device->parent, obj->def->parent) < 0)
+if (VIR_STRDUP(device->parent, def->parent) < 0)
 virObjectUnref(device);
 }
 
@@ -282,6 +284,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
 virNodeDeviceObjListPtr devs = >devs;
 virNodeDevCapsDefPtr cap = NULL;
 virNodeDeviceObjPtr obj = NULL;
+virNodeDeviceDefPtr def;
 virNodeDevicePtr device = NULL;
 
 virCheckFlags(0, NULL);
@@ -291,7 +294,8 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
 for (i = 0; i < devs->count; i++) {
 obj = devs->objs[i];
 virNodeDeviceObjLock(obj);
-cap = obj->def->caps;
+def = virNodeDeviceObjGetDef(obj);
+cap = def->caps;
 
 while (cap) {
 if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) {
@@ -301,11 +305,11 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
 if (STREQ(cap->data.scsi_host.wwnn, wwnn) &&
 STREQ(cap->data.scsi_host.wwpn, wwpn)) {
 
-if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, 
obj->def) < 0)
+if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, 
def) < 0)
 goto out;
 
-if ((device = virGetNodeDevice(conn, obj->def->name))) 
{
-if (VIR_STRDUP(device->parent, obj->def->parent) < 
0)
+if ((device = virGetNodeDevice(conn, def->name))) {
+if (VIR_STRDUP(device->parent, def->parent) < 0)
 virObjectUnref(device);
 }
 virNodeDeviceObjUnlock(obj);
@@ -330,23 +334,25 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr device,
  unsigned int flags)
 {
 virNodeDeviceObjPtr obj;
+virNodeDeviceDefPtr def;
 char *ret = NULL;
 
 virCheckFlags(0, NULL);
 
 if (!(obj = nodeDeviceObjFindByName(device->name)))
 return NULL;
+def = virNodeDeviceObjGetDef(obj);
 
-if (virNodeDeviceGetXMLDescEnsureACL(device->conn, obj->def) < 0)
+if (virNodeDeviceGetXMLDescEnsureACL(device->conn, def) < 0)
 goto cleanup;
 
-if (nodeDeviceUpdateDriverName(obj->def) < 0)
+if (nodeDeviceUpdateDriverName(def) < 0)
 goto cleanup;
 

[libvirt] [PATCH v2 08/14] nodedev: Alter param to nodeDeviceUpdateCaps

[John Ferlan]

Rather than taking an virNodeDeviceObjPtr and dereffing the obj->def,
just pass the def.

Signed-off-by: John Ferlan 
---
 src/node_device/node_device_driver.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index 87953f3..b599460 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -49,14 +49,14 @@ virNodeDeviceDriverStatePtr driver;
 
 
 static int
-nodeDeviceUpdateCaps(virNodeDeviceObjPtr dev)
+nodeDeviceUpdateCaps(virNodeDeviceDefPtr def)
 {
-virNodeDevCapsDefPtr cap = dev->def->caps;
+virNodeDevCapsDefPtr cap = def->caps;
 
 while (cap) {
 switch (cap->data.type) {
 case VIR_NODE_DEV_CAP_SCSI_HOST:
-nodeDeviceSysfsGetSCSIHostCaps(>def->caps->data.scsi_host);
+nodeDeviceSysfsGetSCSIHostCaps(>caps->data.scsi_host);
 break;
 case VIR_NODE_DEV_CAP_NET:
 if (virNetDevGetLinkInfo(cap->data.net.ifname, >data.net.lnk) 
< 0)
@@ -66,8 +66,8 @@ nodeDeviceUpdateCaps(virNodeDeviceObjPtr dev)
 return -1;
 break;
 case VIR_NODE_DEV_CAP_PCI_DEV:
-   if (nodeDeviceSysfsGetPCIRelatedDevCaps(dev->def->sysfs_path,
-   
>def->caps->data.pci_dev) < 0)
+   if (nodeDeviceSysfsGetPCIRelatedDevCaps(def->sysfs_path,
+   >caps->data.pci_dev) < 
0)
   return -1;
break;
 
@@ -341,7 +341,7 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev,
 if (nodeDeviceUpdateDriverName(obj->def) < 0)
 goto cleanup;
 
-if (nodeDeviceUpdateCaps(obj) < 0)
+if (nodeDeviceUpdateCaps(obj->def) < 0)
 goto cleanup;
 
 ret = virNodeDeviceDefFormat(obj->def);
-- 
2.9.4

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

[libvirt] [PATCH v2 03/14] nodedev: Need to check for vport capable scsi_host for vHBA searches

[John Ferlan]

When searching for an NPIV capable fc_host, not only does there need to
be an "fc_host" capability with the specified wwnn/wwpn or fabric_wwn,
but that scsi_host must be vport capable; otherwise, one could end up
picking an exising vHBA/NPIV which wouldn't be good.

Currently not a problem since scsi_hosts are in an as found forward linked
list and the vport capable scsi_hosts will always appear before a vHBA by
definition. However, in the near term future a hash table will be used to
lookup the devices and that could cause problems for these algorithms.

Signed-off-by: John Ferlan 
---
 src/conf/virnodedeviceobj.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 1352720..bbb6eeb 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -165,7 +165,8 @@ virNodeDeviceFindByWWNs(virNodeDeviceObjListPtr devs,
 virNodeDeviceObjLock(devs->objs[i]);
 if ((cap = virNodeDeviceFindFCCapDef(devs->objs[i])) &&
 STREQ_NULLABLE(cap->data.scsi_host.wwnn, parent_wwnn) &&
-STREQ_NULLABLE(cap->data.scsi_host.wwpn, parent_wwpn))
+STREQ_NULLABLE(cap->data.scsi_host.wwpn, parent_wwpn) &&
+virNodeDeviceFindVPORTCapDef(devs->objs[i]))
 return devs->objs[i];
 virNodeDeviceObjUnlock(devs->objs[i]);
 }
@@ -184,7 +185,8 @@ virNodeDeviceFindByFabricWWN(virNodeDeviceObjListPtr devs,
 virNodeDevCapsDefPtr cap;
 virNodeDeviceObjLock(devs->objs[i]);
 if ((cap = virNodeDeviceFindFCCapDef(devs->objs[i])) &&
-STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, parent_fabric_wwn))
+STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, parent_fabric_wwn) 
&&
+virNodeDeviceFindVPORTCapDef(devs->objs[i]))
 return devs->objs[i];
 virNodeDeviceObjUnlock(devs->objs[i]);
 }
-- 
2.9.4

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

[libvirt] [PATCH v2 10/14] nodedev: Use consistent names for driver variables

[John Ferlan]

A virNodeDeviceObjPtr is an @obj

A virNodeDeviceObjListPtr is a @devs

A virNodeDevicePtr is a @device

Signed-off-by: John Ferlan 
---
 src/node_device/node_device_driver.c | 70 ++--
 src/node_device/node_device_hal.c| 42 +++---
 src/node_device/node_device_udev.c   | 50 +-
 3 files changed, 81 insertions(+), 81 deletions(-)

diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index 4b8a66e..d34f399 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -253,7 +253,7 @@ nodeDeviceLookupByName(virConnectPtr conn,
const char *name)
 {
 virNodeDeviceObjPtr obj;
-virNodeDevicePtr ret = NULL;
+virNodeDevicePtr device = NULL;
 
 if (!(obj = nodeDeviceObjFindByName(name)))
 return NULL;
@@ -261,14 +261,14 @@ nodeDeviceLookupByName(virConnectPtr conn,
 if (virNodeDeviceLookupByNameEnsureACL(conn, obj->def) < 0)
 goto cleanup;
 
-if ((ret = virGetNodeDevice(conn, name))) {
-if (VIR_STRDUP(ret->parent, obj->def->parent) < 0)
-virObjectUnref(ret);
+if ((device = virGetNodeDevice(conn, name))) {
+if (VIR_STRDUP(device->parent, obj->def->parent) < 0)
+virObjectUnref(device);
 }
 
  cleanup:
 virNodeDeviceObjUnlock(obj);
-return ret;
+return device;
 }
 
 
@@ -282,7 +282,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
 virNodeDeviceObjListPtr devs = >devs;
 virNodeDevCapsDefPtr cap = NULL;
 virNodeDeviceObjPtr obj = NULL;
-virNodeDevicePtr dev = NULL;
+virNodeDevicePtr device = NULL;
 
 virCheckFlags(0, NULL);
 
@@ -304,9 +304,9 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
 if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, 
obj->def) < 0)
 goto out;
 
-if ((dev = virGetNodeDevice(conn, obj->def->name))) {
-if (VIR_STRDUP(dev->parent, obj->def->parent) < 0)
-virObjectUnref(dev);
+if ((device = virGetNodeDevice(conn, obj->def->name))) 
{
+if (VIR_STRDUP(device->parent, obj->def->parent) < 
0)
+virObjectUnref(device);
 }
 virNodeDeviceObjUnlock(obj);
 goto out;
@@ -321,12 +321,12 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
 
  out:
 nodeDeviceUnlock();
-return dev;
+return device;
 }
 
 
 char *
-nodeDeviceGetXMLDesc(virNodeDevicePtr dev,
+nodeDeviceGetXMLDesc(virNodeDevicePtr device,
  unsigned int flags)
 {
 virNodeDeviceObjPtr obj;
@@ -334,10 +334,10 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev,
 
 virCheckFlags(0, NULL);
 
-if (!(obj = nodeDeviceObjFindByName(dev->name)))
+if (!(obj = nodeDeviceObjFindByName(device->name)))
 return NULL;
 
-if (virNodeDeviceGetXMLDescEnsureACL(dev->conn, obj->def) < 0)
+if (virNodeDeviceGetXMLDescEnsureACL(device->conn, obj->def) < 0)
 goto cleanup;
 
 if (nodeDeviceUpdateDriverName(obj->def) < 0)
@@ -355,15 +355,15 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev,
 
 
 char *
-nodeDeviceGetParent(virNodeDevicePtr dev)
+nodeDeviceGetParent(virNodeDevicePtr device)
 {
 virNodeDeviceObjPtr obj;
 char *ret = NULL;
 
-if (!(obj = nodeDeviceObjFindByName(dev->name)))
+if (!(obj = nodeDeviceObjFindByName(device->name)))
 return NULL;
 
-if (virNodeDeviceGetParentEnsureACL(dev->conn, obj->def) < 0)
+if (virNodeDeviceGetParentEnsureACL(device->conn, obj->def) < 0)
 goto cleanup;
 
 if (obj->def->parent) {
@@ -381,17 +381,17 @@ nodeDeviceGetParent(virNodeDevicePtr dev)
 
 
 int
-nodeDeviceNumOfCaps(virNodeDevicePtr dev)
+nodeDeviceNumOfCaps(virNodeDevicePtr device)
 {
 virNodeDeviceObjPtr obj;
 virNodeDevCapsDefPtr caps;
 int ncaps = 0;
 int ret = -1;
 
-if (!(obj = nodeDeviceObjFindByName(dev->name)))
+if (!(obj = nodeDeviceObjFindByName(device->name)))
 return -1;
 
-if (virNodeDeviceNumOfCapsEnsureACL(dev->conn, obj->def) < 0)
+if (virNodeDeviceNumOfCapsEnsureACL(device->conn, obj->def) < 0)
 goto cleanup;
 
 for (caps = obj->def->caps; caps; caps = caps->next) {
@@ -418,7 +418,7 @@ nodeDeviceNumOfCaps(virNodeDevicePtr dev)
 
 
 int
-nodeDeviceListCaps(virNodeDevicePtr dev,
+nodeDeviceListCaps(virNodeDevicePtr device,
char **const names,
int maxnames)
 {
@@ -427,10 +427,10 @@ nodeDeviceListCaps(virNodeDevicePtr dev,
 int ncaps = 0;
 int ret = -1;
 
-if (!(obj = nodeDeviceObjFindByName(dev->name)))
+if (!(obj = nodeDeviceObjFindByName(device->name)))
 return -1;
 
-if (virNodeDeviceListCapsEnsureACL(dev->conn, obj->def) < 0)
+

[libvirt] [PATCH v2 06/14] nodedev: Cleanup driver code and prototypes

[John Ferlan]

Alter the node_device_driver source and prototypes to follow more
recent code style guidelines w/r/t spacing between functions, format
of the function, and the prototype definitions.

While the new names for nodeDeviceUpdateCaps, nodeDeviceUpdateDriverName,
and nodeDeviceGetTime don't follow exactly w/r/t a "vir" prefix, they
do follow other driver nomenclature style.

Signed-off-by: John Ferlan 
---
 src/node_device/node_device_driver.c |  41 --
 src/node_device/node_device_driver.h |  93 +
 src/node_device/node_device_udev.c   | 256 +--
 3 files changed, 252 insertions(+), 138 deletions(-)

diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index ba3da62..2a461fb 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -47,7 +47,9 @@
 
 virNodeDeviceDriverStatePtr driver;
 
-static int update_caps(virNodeDeviceObjPtr dev)
+
+static int
+nodeDeviceUpdateCaps(virNodeDeviceObjPtr dev)
 {
 virNodeDevCapsDefPtr cap = dev->def->caps;
 
@@ -103,7 +105,8 @@ static int update_caps(virNodeDeviceObjPtr dev)
  * the driver name for a device each time its entry is used, both for
  * udev *and* HAL backends.
  */
-static int update_driver_name(virNodeDeviceObjPtr dev)
+static int
+nodeDeviceUpdateDriverName(virNodeDeviceObjPtr dev)
 {
 char *driver_link = NULL;
 char *devpath = NULL;
@@ -140,7 +143,8 @@ static int update_driver_name(virNodeDeviceObjPtr dev)
 }
 #else
 /* XXX: Implement me for non-linux */
-static int update_driver_name(virNodeDeviceObjPtr dev ATTRIBUTE_UNUSED)
+static int
+nodeDeviceUpdateDriverName(virNodeDeviceObjPtr dev ATTRIBUTE_UNUSED)
 {
 return 0;
 }
@@ -151,11 +155,14 @@ void nodeDeviceLock(void)
 {
 virMutexLock(>lock);
 }
+
+
 void nodeDeviceUnlock(void)
 {
 virMutexUnlock(>lock);
 }
 
+
 int
 nodeNumOfDevices(virConnectPtr conn,
  const char *cap,
@@ -200,6 +207,7 @@ nodeListDevices(virConnectPtr conn,
 return nnames;
 }
 
+
 int
 nodeConnectListAllNodeDevices(virConnectPtr conn,
   virNodeDevicePtr **devices,
@@ -220,8 +228,10 @@ nodeConnectListAllNodeDevices(virConnectPtr conn,
 return ret;
 }
 
+
 virNodeDevicePtr
-nodeDeviceLookupByName(virConnectPtr conn, const char *name)
+nodeDeviceLookupByName(virConnectPtr conn,
+   const char *name)
 {
 virNodeDeviceObjPtr obj;
 virNodeDevicePtr ret = NULL;
@@ -328,8 +338,8 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev,
 if (virNodeDeviceGetXMLDescEnsureACL(dev->conn, obj->def) < 0)
 goto cleanup;
 
-update_driver_name(obj);
-if (update_caps(obj) < 0)
+nodeDeviceUpdateDriverName(obj);
+if (nodeDeviceUpdateCaps(obj) < 0)
 goto cleanup;
 
 ret = virNodeDeviceDefFormat(obj->def);
@@ -421,8 +431,11 @@ nodeDeviceNumOfCaps(virNodeDevicePtr dev)
 }
 
 
+
 int
-nodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames)
+nodeDeviceListCaps(virNodeDevicePtr dev,
+   char **const names,
+   int maxnames)
 {
 virNodeDeviceObjPtr obj;
 virNodeDevCapsDefPtr caps;
@@ -478,8 +491,9 @@ nodeDeviceListCaps(virNodeDevicePtr dev, char **const 
names, int maxnames)
 return ret;
 }
 
+
 static int
-get_time(time_t *t)
+nodeDeviceGetTime(time_t *t)
 {
 int ret = 0;
 
@@ -522,7 +536,7 @@ find_new_device(virConnectPtr conn, const char *wwnn, const 
char *wwpn)
  * doesn't become invalid.  */
 nodeDeviceUnlock();
 
-get_time();
+nodeDeviceGetTime();
 
 while ((now - start) < LINUX_NEW_DEVICE_WAIT_TIME) {
 
@@ -534,7 +548,7 @@ find_new_device(virConnectPtr conn, const char *wwnn, const 
char *wwpn)
 break;
 
 sleep(5);
-if (get_time() == -1)
+if (nodeDeviceGetTime() == -1)
 break;
 }
 
@@ -543,6 +557,7 @@ find_new_device(virConnectPtr conn, const char *wwnn, const 
char *wwpn)
 return dev;
 }
 
+
 virNodeDevicePtr
 nodeDeviceCreateXML(virConnectPtr conn,
 const char *xmlDesc,
@@ -641,6 +656,7 @@ nodeDeviceDestroy(virNodeDevicePtr dev)
 return ret;
 }
 
+
 int
 nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn,
   virNodeDevicePtr dev,
@@ -662,6 +678,7 @@ nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn,
 return callbackID;
 }
 
+
 int
 nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
 int callbackID)
@@ -682,7 +699,9 @@ nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
 return ret;
 }
 
-int nodedevRegister(void)
+
+int
+nodedevRegister(void)
 {
 #ifdef WITH_UDEV
 return udevNodeRegister();
diff --git a/src/node_device/node_device_driver.h 
b/src/node_device/node_device_driver.h
index bc8af8a..b46f001 100644
--- a/src/node_device/node_device_driver.h
+++ b/src/node_device/node_device_driver.h
@@ -31,37 

[libvirt] [PATCH v2 01/14] test: Adjust cleanup/error paths for nodedev test APIs

[John Ferlan]

 - Rather than "goto cleanup;" on failure to virNodeDeviceObjFindByName
   an @obj, just return directly.  This then allows the cleanup: label code
   to not have to check "if (obj)" before calling virNodeDeviceObjUnlock.
   This also simplifies some exit logic...

 - In testNodeDeviceObjFindByName use an error: label to handle the failure
   and don't do the ncaps++ within the VIR_STRDUP() source target index.
   Only increment ncaps after success. Easier on eyes at error label too.

 - In testNodeDeviceDestroy use "cleanup" rather than "out" for the goto

Signed-off-by: John Ferlan 
---
 src/test/test_driver.c | 75 +++---
 1 file changed, 29 insertions(+), 46 deletions(-)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 2db3f7d..3389edd 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4512,7 +4512,6 @@ testDestroyVport(testDriverPtr privconn,
  const char *wwnn ATTRIBUTE_UNUSED,
  const char *wwpn ATTRIBUTE_UNUSED)
 {
-int ret = -1;
 virNodeDeviceObjPtr obj = NULL;
 virObjectEventPtr event = NULL;
 
@@ -4526,7 +4525,7 @@ testDestroyVport(testDriverPtr privconn,
 if (!(obj = virNodeDeviceObjFindByName(>devs, "scsi_host12"))) {
 virReportError(VIR_ERR_NO_NODE_DEVICE, "%s",
_("no node device with matching name 'scsi_host12'"));
-goto cleanup;
+return -1;
 }
 
 event = virNodeDeviceEventLifecycleNew("scsi_host12",
@@ -4535,13 +4534,8 @@ testDestroyVport(testDriverPtr privconn,
 
 virNodeDeviceObjRemove(>devs, );
 
-ret = 0;
-
- cleanup:
-if (obj)
-virNodeDeviceObjUnlock(obj);
 testObjectEventQueue(privconn, event);
-return ret;
+return 0;
 }
 
 
@@ -5328,16 +5322,14 @@ testNodeDeviceLookupByName(virConnectPtr conn, const 
char *name)
 virNodeDevicePtr ret = NULL;
 
 if (!(obj = testNodeDeviceObjFindByName(driver, name)))
-goto cleanup;
+return NULL;
 
 if ((ret = virGetNodeDevice(conn, name))) {
 if (VIR_STRDUP(ret->parent, obj->def->parent) < 0)
 virObjectUnref(ret);
 }
 
- cleanup:
-if (obj)
-virNodeDeviceObjUnlock(obj);
+virNodeDeviceObjUnlock(obj);
 return ret;
 }
 
@@ -5352,13 +5344,11 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev,
 virCheckFlags(0, NULL);
 
 if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
-goto cleanup;
+return NULL;
 
 ret = virNodeDeviceDefFormat(obj->def);
 
- cleanup:
-if (obj)
-virNodeDeviceObjUnlock(obj);
+virNodeDeviceObjUnlock(obj);
 return ret;
 }
 
@@ -5370,7 +5360,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev)
 char *ret = NULL;
 
 if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
-goto cleanup;
+return NULL;
 
 if (obj->def->parent) {
 ignore_value(VIR_STRDUP(ret, obj->def->parent));
@@ -5379,9 +5369,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev)
"%s", _("no parent for this device"));
 }
 
- cleanup:
-if (obj)
-virNodeDeviceObjUnlock(obj);
+virNodeDeviceObjUnlock(obj);
 return ret;
 }
 
@@ -5393,19 +5381,15 @@ testNodeDeviceNumOfCaps(virNodeDevicePtr dev)
 virNodeDeviceObjPtr obj;
 virNodeDevCapsDefPtr caps;
 int ncaps = 0;
-int ret = -1;
 
 if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
-goto cleanup;
+return -1;
 
 for (caps = obj->def->caps; caps; caps = caps->next)
 ++ncaps;
-ret = ncaps;
 
- cleanup:
-if (obj)
-virNodeDeviceObjUnlock(obj);
-return ret;
+virNodeDeviceObjUnlock(obj);
+return ncaps;
 }
 
 
@@ -5416,26 +5400,25 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char 
**const names, int maxnames)
 virNodeDeviceObjPtr obj;
 virNodeDevCapsDefPtr caps;
 int ncaps = 0;
-int ret = -1;
 
 if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
-goto cleanup;
+return -1;
 
 for (caps = obj->def->caps; caps && ncaps < maxnames; caps = caps->next) {
-if (VIR_STRDUP(names[ncaps++], 
virNodeDevCapTypeToString(caps->data.type)) < 0)
-goto cleanup;
+if (VIR_STRDUP(names[ncaps],
+   virNodeDevCapTypeToString(caps->data.type)) < 0)
+goto error;
+ncaps++;
 }
-ret = ncaps;
 
- cleanup:
-if (obj)
-virNodeDeviceObjUnlock(obj);
-if (ret == -1) {
---ncaps;
-while (--ncaps >= 0)
-VIR_FREE(names[ncaps]);
-}
-return ret;
+virNodeDeviceObjUnlock(obj);
+return ncaps;
+
+ error:
+while (--ncaps >= 0)
+VIR_FREE(names[ncaps]);
+virNodeDeviceObjUnlock(obj);
+return -1;
 }
 
 
@@ -5584,13 +5567,13 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
 virObjectEventPtr event = NULL;
 
 if (!(obj = testNodeDeviceObjFindByName(driver, 

[libvirt] [PATCH v2 00/14] Work towards making virNodeDeviceObjPtr private

[John Ferlan]

v1: https://www.redhat.com/archives/libvir-list/2017-May/msg00718.html

Changes since v1:

 * Adjusted the title of cover letter to more appropriately match what's
   being done.

 * Added new patch 3 to cover issues I've noted in recent code reviews
   where additions to virNodeDevCapType may not be properly 'covered'
   in the virNodeDeviceObjHasCap and virNodeDeviceCapMatch helpers. With
   the switch, it'll be forced.

 * Removed former patch 4 - I'll deal with it later.

 * Added patch 13

 * Patch 14 is the old patch 13o

There's another 8 or so patches waiting to go, but the "next" one in the
series depends on other things currently on list waiting for review.

John Ferlan (14):
  test: Adjust cleanup/error paths for nodedev test APIs
  nodedev: Fix locking in virNodeDeviceObjRemove
  nodedev: Need to check for vport capable scsi_host for vHBA searches
  nodedev: Use switch for virNodeDeviceObjHasCap and
virNodeDeviceCapMatch
  nodedev: Use common naming for virnodedeviceobj
  nodedev: Cleanup driver code and prototypes
  nodedev: Alter param to nodeDeviceUpdateDriverName
  nodedev: Alter param to nodeDeviceUpdateCaps
  nodedev: Create helper for finding by name in driver
  nodedev: Use consistent names for driver variables
  nodedev: Introduce virNodeDeviceObjGetDef
  nodedev: Remove privateData from virNodeDeviceObj
  nodedev: Pass @def by reference to create/assign object
  nodedev: Introduce virNodeDeviceObjNew

 src/conf/node_device_conf.h  |   2 -
 src/conf/virnodedeviceobj.c  | 252 ---
 src/conf/virnodedeviceobj.h  |   4 +-
 src/libvirt_private.syms |   1 +
 src/node_device/node_device_driver.c | 233 -
 src/node_device/node_device_driver.h |  93 +++---
 src/node_device/node_device_hal.c|  56 +++---
 src/node_device/node_device_udev.c   | 321 ---
 src/test/test_driver.c   | 118 +++--
 9 files changed, 609 insertions(+), 471 deletions(-)

-- 
2.9.4

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

[libvirt] [PATCH v2 05/14] nodedev: Use common naming for virnodedeviceobj

[John Ferlan]

A virNodeDeviceObjPtr is an @obj

A virNodeDeviceObjListPtr is an @devs

Signed-off-by: John Ferlan 
---
 src/conf/virnodedeviceobj.c | 130 ++--
 1 file changed, 65 insertions(+), 65 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 913cdda..a2d09ad 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -34,10 +34,10 @@ VIR_LOG_INIT("conf.virnodedeviceobj");
 
 
 static int
-virNodeDeviceObjHasCap(const virNodeDeviceObj *dev,
+virNodeDeviceObjHasCap(const virNodeDeviceObj *obj,
const char *cap)
 {
-virNodeDevCapsDefPtr caps = dev->def->caps;
+virNodeDevCapsDefPtr caps = obj->def->caps;
 const char *fc_host_cap =
 virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_FC_HOST);
 const char *vports_cap =
@@ -100,9 +100,9 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *dev,
  * Pointer to the caps or NULL if not found
  */
 static virNodeDevCapsDefPtr
-virNodeDeviceFindFCCapDef(const virNodeDeviceObj *dev)
+virNodeDeviceFindFCCapDef(const virNodeDeviceObj *obj)
 {
-virNodeDevCapsDefPtr caps = dev->def->caps;
+virNodeDevCapsDefPtr caps = obj->def->caps;
 
 while (caps) {
 if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST &&
@@ -124,9 +124,9 @@ virNodeDeviceFindFCCapDef(const virNodeDeviceObj *dev)
  * Pointer to the caps or NULL if not found
  */
 static virNodeDevCapsDefPtr
-virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *dev)
+virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj)
 {
-virNodeDevCapsDefPtr caps = dev->def->caps;
+virNodeDevCapsDefPtr caps = obj->def->caps;
 
 while (caps) {
 if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST &&
@@ -235,18 +235,18 @@ virNodeDeviceFindByCap(virNodeDeviceObjListPtr devs,
 
 
 void
-virNodeDeviceObjFree(virNodeDeviceObjPtr dev)
+virNodeDeviceObjFree(virNodeDeviceObjPtr obj)
 {
-if (!dev)
+if (!obj)
 return;
 
-virNodeDeviceDefFree(dev->def);
-if (dev->privateFree)
-(*dev->privateFree)(dev->privateData);
+virNodeDeviceDefFree(obj->def);
+if (obj->privateFree)
+(*obj->privateFree)(obj->privateData);
 
-virMutexDestroy(>lock);
+virMutexDestroy(>lock);
 
-VIR_FREE(dev);
+VIR_FREE(obj);
 }
 
 
@@ -265,51 +265,51 @@ virNodeDeviceObjPtr
 virNodeDeviceObjAssignDef(virNodeDeviceObjListPtr devs,
   virNodeDeviceDefPtr def)
 {
-virNodeDeviceObjPtr device;
+virNodeDeviceObjPtr obj;
 
-if ((device = virNodeDeviceObjFindByName(devs, def->name))) {
-virNodeDeviceDefFree(device->def);
-device->def = def;
-return device;
+if ((obj = virNodeDeviceObjFindByName(devs, def->name))) {
+virNodeDeviceDefFree(obj->def);
+obj->def = def;
+return obj;
 }
 
-if (VIR_ALLOC(device) < 0)
+if (VIR_ALLOC(obj) < 0)
 return NULL;
 
-if (virMutexInit(>lock) < 0) {
+if (virMutexInit(>lock) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("cannot initialize mutex"));
-VIR_FREE(device);
+VIR_FREE(obj);
 return NULL;
 }
-virNodeDeviceObjLock(device);
+virNodeDeviceObjLock(obj);
 
-if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, device) < 0) {
-virNodeDeviceObjUnlock(device);
-virNodeDeviceObjFree(device);
+if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, obj) < 0) {
+virNodeDeviceObjUnlock(obj);
+virNodeDeviceObjFree(obj);
 return NULL;
 }
-device->def = def;
+obj->def = def;
 
-return device;
+return obj;
 
 }
 
 
 void
 virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs,
-   virNodeDeviceObjPtr *dev)
+   virNodeDeviceObjPtr *obj)
 {
 size_t i;
 
-virNodeDeviceObjUnlock(*dev);
+virNodeDeviceObjUnlock(*obj);
 
 for (i = 0; i < devs->count; i++) {
 virNodeDeviceObjLock(devs->objs[i]);
-if (devs->objs[i] == *dev) {
+if (devs->objs[i] == *obj) {
 virNodeDeviceObjUnlock(devs->objs[i]);
 virNodeDeviceObjFree(devs->objs[i]);
-*dev = NULL;
+*obj = NULL;
 
 VIR_DELETE_ELEMENT(devs->objs, i, devs->count);
 break;
@@ -332,15 +332,15 @@ virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs,
  *   parent_host value on success (>= 0), -1 otherwise.
  */
 static int
-virNodeDeviceFindFCParentHost(virNodeDeviceObjPtr parent)
+virNodeDeviceFindFCParentHost(virNodeDeviceObjPtr obj)
 {
-virNodeDevCapsDefPtr cap = virNodeDeviceFindVPORTCapDef(parent);
+virNodeDevCapsDefPtr cap = virNodeDeviceFindVPORTCapDef(obj);
 
 if (!cap) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Parent device %s is not capable "
  "of vport operations"),
-   parent->def->name);
+   

[libvirt] [PATCH v2 02/14] nodedev: Fix locking in virNodeDeviceObjRemove

[John Ferlan]

The current mechanism doesn't lock each element in devs->objs as it's
looking at it, rather it keeps locking/unlocking the passed element for
which the removal is being attempted.  Fix things to lock each element
as we're looking at them.

Signed-off-by: John Ferlan 
---
 src/conf/virnodedeviceobj.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 02ac544..1352720 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -281,16 +281,16 @@ virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs,
 virNodeDeviceObjUnlock(*dev);
 
 for (i = 0; i < devs->count; i++) {
-virNodeDeviceObjLock(*dev);
+virNodeDeviceObjLock(devs->objs[i]);
 if (devs->objs[i] == *dev) {
-virNodeDeviceObjUnlock(*dev);
+virNodeDeviceObjUnlock(devs->objs[i]);
 virNodeDeviceObjFree(devs->objs[i]);
 *dev = NULL;
 
 VIR_DELETE_ELEMENT(devs->objs, i, devs->count);
 break;
 }
-virNodeDeviceObjUnlock(*dev);
+virNodeDeviceObjUnlock(devs->objs[i]);
 }
 }
 
-- 
2.9.4

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

Re: [libvirt] [PATCH 6/7] node_device: introduce new capability FC_RPORT

[John Ferlan]

On 05/22/2017 02:38 AM, Bjoern Walk wrote:
> Similar to scsi_host and fc_host, there is a relation between a
> scsi_target and its transport specific fc_remote_port. Let's expose this
> relation and relevant information behind it.
> 
> An example for a virsh nodedev-dumpxml:
> 
> virsh # nodedev-dumpxml scsi_target0_0_0
> 
>   scsi_target0_0_0
>   /sys/devices/[...]/host0/rport-0:0-0/target0:0:0
>   scsi_host0
>   
> target0:0:0
> 
>   rport-0:0-0
>   0x9d73bc45f0e21a86
> 
>   
> 
> 
> Reviewed-by: Boris Fiuczynski 
> Signed-off-by: Bjoern Walk 
> ---
>  docs/schemas/nodedev.rng | 20 
>  src/conf/node_device_conf.c  | 75 
> +++-
>  src/conf/node_device_conf.h  |  7 +++
>  src/node_device/node_device_driver.c |  5 +-
>  src/node_device/node_device_linux_sysfs.c| 56 +
>  src/node_device/node_device_linux_sysfs.h|  2 +
>  src/node_device/node_device_udev.c   |  4 +-
>  tests/nodedevschemadata/scsi_target1_0_0.xml | 12 +
>  tests/nodedevxml2xmltest.c   |  1 +
>  9 files changed, 178 insertions(+), 4 deletions(-)
>  create mode 100644 tests/nodedevschemadata/scsi_target1_0_0.xml
> 
> diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
> index 87bfb0c28..5bcf31787 100644
> --- a/docs/schemas/nodedev.rng
> +++ b/docs/schemas/nodedev.rng
> @@ -458,6 +458,20 @@
>  
>
>  
> +  
> +
> +  fc_remote_port
> +
> +
> +
> +  
> +
> +
> +
> +  

NB: 'wwn' is a string...

> +
> +  
> +
>
>  
>scsi_target
> @@ -466,6 +480,12 @@
>  
>
>  
> +
> +
> +  
> +
> +  
> +
>
>  
>
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index 28d4907f5..ed003d37b 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -563,6 +563,16 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def)
>  case VIR_NODE_DEV_CAP_SCSI_TARGET:
>  virBufferEscapeString(, "%s\n",
>data->scsi_target.name);
> +if (data->scsi_target.flags & VIR_NODE_DEV_CAP_FLAG_FC_RPORT) {
> +virBufferAddLit(, " type='fc_remote_port'>\n");
> +virBufferAdjustIndent(, 2);
> +virBufferAsprintf(, "%s\n",
> +  data->scsi_target.rport);
> +virBufferAsprintf(, "0x%llx\n",
> +  data->scsi_target.wwpn);

If you don't convert to a number, then you don't need to perform this
conversion as it's just a %s print.

> +virBufferAdjustIndent(, -2);
> +virBufferAddLit(, "\n");
> +}
>  break;
>  case VIR_NODE_DEV_CAP_SCSI:
>  virNodeDeviceCapSCSIDefFormat(, data);
> @@ -932,8 +942,10 @@ virNodeDevCapSCSITargetParseXML(xmlXPathContextPtr ctxt,
>  xmlNodePtr node,
>  virNodeDevCapSCSITargetPtr scsi_target)
>  {
> -xmlNodePtr orignode;
> -int ret = -1;
> +xmlNodePtr orignode, *nodes = NULL;
> +int ret = -1, n = 0;
> +size_t i;
> +char *type = NULL, *wwpn = NULL;
>  
>  orignode = ctxt->node;
>  ctxt->node = node;
> @@ -946,10 +958,68 @@ virNodeDevCapSCSITargetParseXML(xmlXPathContextPtr ctxt,
>  goto out;
>  }
>  
> +if ((n = virXPathNodeSet("./capability", ctxt, )) < 0)
> +goto out;
> +
> +for (i = 0; i < n; ++i) {
> +type = virXMLPropString(nodes[i], "type");
> +
> +if (!type) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("missing type for SCSI target capability for 
> '%s'"),
> +   def->name);
> +goto out;
> +}
> +
> +if (STREQ(type, "fc_remote_port")) {
> +xmlNodePtr orignode2;
> +
> +scsi_target->flags |= VIR_NODE_DEV_CAP_FLAG_FC_RPORT;
> +
> +orignode2 = ctxt->node;
> +ctxt->node = nodes[i];

Like the virNodeDevCapSCSIHostParseXML I assume this only parses the
first 'fc_remote_port'... Trying to think whether a VIR_FREE would be
necessary here (and perhaps from SCSIHost code just in case there were
more than one  or  provided...

> +
> +if (virNodeDevCapsDefParseString("string(./rport[1])",
> + ctxt,
> + _target->rport) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("missing rport name for '%s'"), def->name);
> +goto out;
> +}
> +
> +if (virNodeDevCapsDefParseString("string(./wwpn[1])",
> + 

Re: [libvirt] [PATCH 7/7] docs: update news.xml

[John Ferlan]

On 05/22/2017 02:38 AM, Bjoern Walk wrote:
> Mention CCW and fc_remote_port capablities in the news.xml file.
> 
> Reviewed-by: Boris Fiuczynski 
> Signed-off-by: Bjoern Walk 
> ---
>  docs/news.xml | 11 +++
>  1 file changed, 11 insertions(+)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 5/7] util: helper functions for fibre channel devices

[John Ferlan]

On 05/22/2017 02:38 AM, Bjoern Walk wrote:
> We will need some convenient helper functions for managing sysfs-entries
> for fibre channel-backed devices. Let's implement them and make them
> available in the private API.
> 
> Signed-off-by: Bjoern Walk 
> ---
>  po/POTFILES.in   |  1 +
>  src/Makefile.am  |  1 +
>  src/libvirt_private.syms |  5 +++
>  src/util/virfcp.c| 98 
> 
>  src/util/virfcp.h| 34 +
>  5 files changed, 139 insertions(+)
>  create mode 100644 src/util/virfcp.c
>  create mode 100644 src/util/virfcp.h
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 4/7] virsh: nodedev: ability to filter CCW capabilities

[John Ferlan]

On 05/22/2017 02:38 AM, Bjoern Walk wrote:
> Now that the node_device driver is aware of CCW devices, let's hook up
> virsh so that we can filter them properly.
> 
> Reviewed-by: Boris Fiuczynski 
> Reviewed-by: Marc Hartmayer 
> Signed-off-by: Bjoern Walk 
> ---
>  docs/formatnode.html.in   | 12 
>  include/libvirt/libvirt-nodedev.h |  1 +
>  src/conf/node_device_conf.h   |  3 ++-
>  src/conf/virnodedeviceobj.c   |  3 ++-
>  src/libvirt-nodedev.c |  1 +
>  tools/virsh-nodedev.c |  3 ++-
>  tools/virsh.pod   |  2 +-
>  7 files changed, 21 insertions(+), 4 deletions(-)
> 


...

> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
> index 02ac54437..d460f26ec 100644
> --- a/src/conf/virnodedeviceobj.c
> +++ b/src/conf/virnodedeviceobj.c
> @@ -568,7 +568,8 @@ virNodeDeviceMatch(virNodeDeviceObjPtr devobj,
>MATCH(SCSI_GENERIC)  ||
>MATCH(DRM)   ||
>MATCH(MDEV_TYPES)||
> -  MATCH(MDEV)))
> +  MATCH(MDEV)  ||
> +  MATCH(CCW_DEV)))
>  return false;
>  }
>  

But you didn't modify virNodeDeviceCapMatch in order perform the match
that the MATCH does.

Also, another well hidden gem that either Erik Skultety or myself will
fix "eventually" is virNodeDeviceObjHasCap.  See commit id 'e8fcac8ec'
for some context.  Essentially, the virNodeDeviceMatch only matches for
the virNodeDeviceObjListExport API.

If you want to send something to squash in or just a v2 of this patch -
I can handle either.

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 3/7] node_device: detect CCW devices

[John Ferlan]

On 05/22/2017 02:38 AM, Bjoern Walk wrote:
> Make CCW devices available to the node_device driver. The devices are
> already seen by udev so let's implement necessary code for detecting
> them properly.
> 
> Topologically, CCW devices are similar to PCI devices, e.g.:
> 
> +- ccw_0_0_1a2b
> |
> +- scsi_host0
> |
> +- scsi_target0_0_0
> |
> +- scsi_0_0_0_0
> 
> Reviewed-by: Boris Fiuczynski 
> Signed-off-by: Bjoern Walk 
> ---
>  docs/schemas/basictypes.rng   | 31 ++
>  docs/schemas/domaincommon.rng | 30 -
>  docs/schemas/nodedev.rng  | 16 +
>  src/conf/node_device_conf.c   | 75 
> ++-
>  src/conf/node_device_conf.h   | 10 +++
>  src/node_device/node_device_driver.c  |  1 +
>  src/node_device/node_device_udev.c| 35 ++-
>  tests/nodedevschemadata/ccw_0_0_1-invalid.xml | 10 +++
>  tests/nodedevschemadata/ccw_0_0_.xml  | 10 +++
>  tests/nodedevxml2xmltest.c|  1 +
>  tools/virsh-nodedev.c |  2 +
>  11 files changed, 188 insertions(+), 33 deletions(-)
>  create mode 100644 tests/nodedevschemadata/ccw_0_0_1-invalid.xml
>  create mode 100644 tests/nodedevschemadata/ccw_0_0_.xml
> 

...

> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index 9cb63860f..28d4907f5 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c

...

>  
>  static int
> +virNodeDevCapCCWParseXML(xmlXPathContextPtr ctxt,
> + virNodeDeviceDefPtr def,
> + xmlNodePtr node,
> + virNodeDevCapCCWPtr ccw_dev)
> +{
> +xmlNodePtr orignode;
> +int ret = -1;
> +char *cssid = NULL, *ssid = NULL, *devno = NULL;
> +
> +orignode = ctxt->node;
> +ctxt->node = node;
> +
> +   if (!(cssid = virXPathString("string(./cssid[1])", ctxt))) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("missing cssid value for '%s'"), def->name);
> +goto out;
> +}
> +
> +if (virStrToLong_uip(cssid, NULL, 0, _dev->cssid) < 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("invalid cssid value '%s' for '%s'"),
> +   cssid, def->name);
> +goto out;
> +}
> +
> +if (!(ssid = virXPathString("string(./ssid[1])", ctxt))) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("missing ssid value for '%s'"), def->name);
> +goto out;
> +}
> +
> +if (virStrToLong_uip(ssid, NULL, 0, _dev->ssid) < 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("invalid ssid value '%s' for '%s'"),
> +   cssid, def->name);
> +goto out;
> +}
> +
> +if (!(devno = virXPathString("string(./devno[1])", ctxt))) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("missing devno value for '%s'"), def->name);
> +goto out;
> +}
> +
> +if (virStrToLong_uip(devno, NULL, 16, _dev->devno) < 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("invalid devno value '%s' for '%s'"),
> +   devno, def->name);
> +goto out;
> +}

One would hope they're in range, but since the rng had ranges should you
check here similar to what virDomainDeviceCCWAddressIsValid does?

It's fine this way since this is essentially reporting from udev which
one can only assume (haha) would do validation...

> +
> +ret = 0;
> +
> + out:
> +ctxt->node = orignode;
> +return ret;
> +}
> +
> +

...

> diff --git a/src/node_device/node_device_udev.c 
> b/src/node_device/node_device_udev.c
> index 4ecb0b18f..7744c2637 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1105,6 +1105,33 @@ udevProcessMediatedDevice(struct udev_device *dev,
>  }
>  
>  static int
> +udevProcessCCW(struct udev_device *device, virNodeDeviceDefPtr def)

Although you're following the module syntax, this should follow current
practices for multilines and spacing before/after function... I can
adjust that before pushing if this is all that's necessary though.

> +{
> +int online;
> +char *p;
> +virNodeDevCapDataPtr data = >caps->data;
> +
> +/* process only online devices to keep the list sane */
> +if (udevGetIntSysfsAttr(device, "online", , 0) < 0 || online != 1)
> +return -1;
> +
> +if ((p = strrchr(def->sysfs_path, '/')) == NULL ||
> +virStrToLong_ui(p + 1, , 16, >ccw_dev.cssid) < 0 || p == 
> NULL ||
> +virStrToLong_ui(p + 1, , 16, >ccw_dev.ssid) < 0 || p == NULL 
> ||
> +virStrToLong_ui(p + 1, , 

Re: [libvirt] [PATCH 2/7] node_device: Unlock obj in case of an error too

[John Ferlan]

On 05/22/2017 02:38 AM, Bjoern Walk wrote:
> From: Marc Hartmayer 
> 
> Unlock @obj in case of an error too.
> 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Boris Fiuczynski 
> Signed-off-by: Marc Hartmayer 
> ---
>  src/node_device/node_device_driver.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 

Reviewed-by: John Ferlan 

John

A patch beyond one I've posted also caught this, but I must not have
been thinking about pulling the unlock out for earlier patch...

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

Re: [libvirt] [PATCH 1/7] node_device: Use the iterator variable

[John Ferlan]

On 05/22/2017 02:38 AM, Bjoern Walk wrote:
> From: Marc Hartmayer 
> 
> As the switch statement checks data.type of the iterator variable @cap
> it must use this variable for the update too.

s/must/can...

I can adjust the commit message before pushing though, so no need for a
v2...

> 
> Suggested-by: Bjoern Walk 
> Signed-off-by: Marc Hartmayer 
> ---
>  src/node_device/node_device_driver.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Reviewed-by: John Ferlan 

John

(at least I understand why you jumped on my other series now ;-))



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

[libvirt] Remotable Libvirt

[Peter Volpe]

Hi Everyone,

We are working towards building on the virtualization management 
functionality in cockpit (http://cockpit-project.org/) and wanted to get 
some feedback on the best way to integrate with libvirt.


As a quick overview, cockpit aims to talk to existing remotable system 
APIs. Usually these API’s take the form of dbus, REST or executable 
commands. The majority of cockpit is implemented in javascript. There is 
no cockpit backend that knows how to change a hostname for example. The 
cockpit backend knows how to handle a dbus payload. The javascript 
running in the users browser knows how to use the systemd dbus API at 
org.freedesktop.hostname1 to manage the system hostname.


Right now some of the basics have been implemented by spawning commands 
on the system. This isn't ideal because it involves parsing / screen 
scraping output and doesn't support receiving events so we have to poll 
(ei run the command again) to keep the UI up to date.


As far as I know libvirt doesn't currently have a remoteable API. It 
does have a daemon that communicates with clients via a XDR RPC. 
(https://libvirt.org/internals/rpc.html) However from what I'm hearing 
the RPC is considered an internal implementation and shouldn't be used 
by external applications. Is that still the case? Is there any chance of 
getting talking the daemon directly using the XDR standard for a subset 
of methods blessed as part of the externally supported API?


An alternative is to implement a standards based remotable API, using 
something like dbus or REST, that can be used by external applications.
I imagine that this would be at a bit of a higher level than the current 
RPC and contain at least some of the logic around the actions it 
performs rather than being a direct passthrough to the daemon.


Of course that is a pretty big undertaking and would, in my opinion, 
only be worth it if there is broader interest in the community and use 
cases beyond what cockpit would like to.


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

[libvirt] Remotable Libvirt

[Peter]

Hi Everyone,

We are working towards building on the virtualization management 
functionality in cockpit (http://cockpit-project.org/) and wanted to get 
some feedback on the best way to integrate with libvirt.


As a quick overview, cockpit aims to talk to existing remotable system 
APIs. Usually these API’s take the form of dbus, REST or executable 
commands. The majority of cockpit is implemented in javascript. There is 
no cockpit backend that knows how to change a hostname for example. The 
cockpit backend knows how to handle a dbus payload. The javascript 
running in the users browser knows how to use the systemd dbus API at 
org.freedesktop.hostname1 to manage the system hostname.


Right now some of the basics have been implemented by spawning commands 
on the system. This isn't ideal because it involves parsing / screen 
scraping output and doesn't support receiving events so we have to poll 
(ei run the command again) to keep the UI up to date.


As far as I know libvirt doesn't currently have a remoteable API. It 
does have a daemon that communicates with clients via a XDR RPC. 
(https://libvirt.org/internals/rpc.html) However from what I'm hearing 
the RPC is considered an internal implementation and shouldn't be used 
by external applications. Is that still the case? Is there any chance of 
getting talking the daemon directly using the XDR standard for a subset 
of methods blessed as part of the externally supported API?


An alternative is to implement a standards based remotable API, using 
something like dbus or REST, that can be used by external applications.
I imagine that this would be at a bit of a higher level than the current 
RPC and contain at least some of the logic around the actions it 
performs rather than being a direct passthrough to the daemon.


Of course that is a pretty big undertaking and would, in my opinion, 
only be worth it if there is broader interest in the community and use 
cases beyond what cockpit would like to.


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

Re: [libvirt] RFE: virsh list improvement (--description and --details)

[Przemysław Sztoch]

As you wish. I have made python script for my customer:

https://github.com/psztoch/virt-report

Nevertheless, I think the second version of my patch for the virsh list 
is simple and traightforward, and falls within the scope of a simple 
low-level virsh tool. And of course  should be merged into trunk.


On 09.05.2017 13:25, Andrea Bolognani wrote:

On Mon, 2017-05-08 at 09:16 +0200, Przemysław Sztoch wrote:

Dear Michal and Pavel,
  
We cover about 100 clients who have their own and simple CentOS KVM

installations.
Their technical skills are far from writing python scripts. They expect
simple solutions.

They don't necessarily have to write the Python script
themselves, though :)


Talking to our helpdesk, I found that 70% of libvirt and virtualization
problems are:
A) lack of autostart activation on critical guests; then occasional
failures and reboots affect lack of automatic startup of key services,
B) frequent overcommiting of allocated virtual processors and memory due
to the lack of basic planning and addition operations of local admin
stuff :-(,
C) misconfiguration of qemu-agent, which affects many problems with safe
restart, snapshot, backup, etc. (the "Time" column is a perfect
diagnostic here)
D) leaving unnecessary snapshots that lie unused after many months,
E) live migration attempts that fail to put domain in a transient mode
leave the guests disappearing in unexplained circumstances after kvm
host restart :-)
  
Virtually all the above problems of everyday life, our helpdesk is now

able to diagnose by command:
virsh list --details --managed-save
By the way, they can easily update the documentation with one compact list.
  
I do not understand your dislike for the proposed changes. All the

members of our team and teams of our partners have been very
enthusiastic about the new functionality.
You govern, so you have to decide. ;-)

The problem with your proposal is that it doesn't fit
neatly in a generic tool like virsh.

My suggestion would be to implement the script yourself,
then ship it to your clients and instruct them to run

   # virt-diagnostics.py

or whatever you end up calling it to obtain the
information you care about.

Doing so will allow you to have plenty of freedom when
it comes to tailoring the output for your specific needs
instead of having to keep the tool generic, which you
would have to do if you wanted it to be shipped with
libvirt, and it won't be any harder for your clients to
run it.

--
Andrea Bolognani / Red Hat / Virtualization


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

[libvirt] [PATCH 11/08] interface: Alter virInterfaceObjListAssignDef @def param

[John Ferlan]

Rather than pass by value, let's pass by reference since the object ends
up "owning" the XML definition, let's make that ownership a bit more real.

Signed-off-by: John Ferlan 
---

Since 09 and 10 weren't reviewed yet, I'll add one more.

 src/conf/virinterfaceobj.c | 12 +++-
 src/conf/virinterfaceobj.h |  2 +-
 src/test/test_driver.c |  5 ++---
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c
index 51c3c82..f7352d2 100644
--- a/src/conf/virinterfaceobj.c
+++ b/src/conf/virinterfaceobj.c
@@ -235,8 +235,10 @@ virInterfaceObjListClone(virInterfaceObjListPtr interfaces)
 }
 
 VIR_FREE(xml);
-if (!(obj = virInterfaceObjListAssignDef(dest, backup)))
+if (!(obj = virInterfaceObjListAssignDef(dest, ))) {
+virInterfaceDefFree(backup);
 goto error;
+}
 virInterfaceObjEndAPI();
 }
 
@@ -250,13 +252,13 @@ virInterfaceObjListClone(virInterfaceObjListPtr 
interfaces)
 
 virInterfaceObjPtr
 virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces,
- virInterfaceDefPtr def)
+ virInterfaceDefPtr *def)
 {
 virInterfaceObjPtr obj;
 
-if ((obj = virInterfaceObjListFindByName(interfaces, def->name))) {
+if ((obj = virInterfaceObjListFindByName(interfaces, (*def)->name))) {
 virInterfaceDefFree(obj->def);
-obj->def = def;
+VIR_STEAL_PTR(obj->def, *def);
 
 return obj;
 }
@@ -270,7 +272,7 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr 
interfaces,
 return NULL;
 }
 
-obj->def = def;
+VIR_STEAL_PTR(obj->def, *def);
 return virObjectRef(obj);
 
 }
diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h
index 2b9e1b2..ee9 100644
--- a/src/conf/virinterfaceobj.h
+++ b/src/conf/virinterfaceobj.h
@@ -65,7 +65,7 @@ virInterfaceObjListClone(virInterfaceObjListPtr interfaces);
 
 virInterfaceObjPtr
 virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces,
- virInterfaceDefPtr def);
+ virInterfaceDefPtr *def);
 
 void
 virInterfaceObjListRemove(virInterfaceObjListPtr interfaces,
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index fb95319..4b4a782 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1021,7 +1021,7 @@ testParseInterfaces(testDriverPtr privconn,
 if (!def)
 goto error;
 
-if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces, def))) {
+if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces, ))) {
 virInterfaceDefFree(def);
 goto error;
 }
@@ -3903,9 +3903,8 @@ testInterfaceDefineXML(virConnectPtr conn,
 if (!(def = virInterfaceDefParseString(xmlStr)))
 goto cleanup;
 
-if ((obj = virInterfaceObjListAssignDef(privconn->ifaces, def)) == NULL)
+if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces, )))
 goto cleanup;
-def = NULL;
 objdef = virInterfaceObjGetDef(obj);
 
 ret = virGetInterface(conn, objdef->name, objdef->mac);
-- 
2.9.4

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

Re: [libvirt] [virt-tools-list] Problem with firewalls on Arch Linux

[Dan]

On Thu, May 25, 2017 at 09:42:42AM -0400, Cole Robinson wrote:
> On 05/25/2017 06:08 AM, Derek Richline wrote:
> > Hi all:
> > 
> > I noticed that Virt Manager will not start the network "default" unless
> > it detects a running firewalld. Arch Linux only ships with iptables by
> > default. Can Virt-Manager be patched to not check as aggressively for a
> > firewall (or just issue a warning)?
> > 
> 
I had to manually install ebtalbes on archlinux when testing libvirt by "make
check". 
> virt-manager doesn't have any specific firewalld checking so this is likely
> coming from libvirt. I don't know anything about arch packaging, but if the
> libvirt package doesn't pull in firewalld then maybe libvirt should be built
> --without-firewalld
> 
> What libvirt version is this? And what's the exact error message you are 
> seeing?
> 
> - Cole
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

[libvirt] [PATCH] node_device: fix memory leak in nodeDeviceSysfsGetSCSIHostCaps

[Yi Wang]

The @tmp is allocated in virVHBAGetConfig in virVHBAIsVportCapable
condition, it will lost when virVHBAGetConfig called again.

Signed-off-by: Yi Wang 
---
 src/node_device/node_device_linux_sysfs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/node_device/node_device_linux_sysfs.c 
b/src/node_device/node_device_linux_sysfs.c
index 1b7aa94..a9c7c9c 100644
--- a/src/node_device/node_device_linux_sysfs.c
+++ b/src/node_device/node_device_linux_sysfs.c
@@ -95,7 +95,8 @@ nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr 
scsi_host)
 goto cleanup;
 }
 
- if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host,
+VIR_FREE(tmp);
+if (!(tmp = virVHBAGetConfig(NULL, scsi_host->host,
   "npiv_vports_inuse"))) {
 VIR_WARN("Failed to read npiv_vports_inuse for host%d",
  scsi_host->host);
-- 
1.8.3.1


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

Re: [libvirt] [PATCH] CI: show the log in case of failure

[Martin Kletzander]

On Thu, May 25, 2017 at 02:35:53PM +0200, Martin Kletzander wrote:

On Thu, May 25, 2017 at 02:19:45AM -0300, claudioandre...@gmail.com wrote:

From: Claudio André 

Disclosure the content of the 'test-suite.log' file (if available) in case of 
failures inside Travis-CI. This is needed to understand what happened and to 
provide hints about the proper fix (if applicable).
---
.travis.yml | 8 
1 file changed, 8 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index 266..5a3e765 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -83,3 +83,11 @@ matrix:
# many unit tests fail & so does syntax-check, so skip for now
# one day we must fix it though
- make -j3
+
+after_failure:
+  - echo 
''
+  - 'if [ -f $(pwd)/tests/test-suite.log ]; then
+cat $(pwd)/tests/test-suite.log;
+else
+echo "=== NO LOG FILE FOUND ===";
+fi'


I need to investigate why this doesn't work, I don't have that file
there after 'make check'.



Just figured that out.  The systems in Travis are just so old that the
automake there doesn't create the test-suite.log =(


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

[libvirt] [PATCH] util: fix memory leak in virSocketAddrFormatFull

[Yi Wang]

The @ipv6_host allocated in virAsprintf may be lost when virAsprintf
addrstr failed.

Signed-off-by: Yi Wang 
---
 src/util/virsocketaddr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
index 9dffbc7..95b5274 100644
--- a/src/util/virsocketaddr.c
+++ b/src/util/virsocketaddr.c
@@ -417,8 +417,10 @@ virSocketAddrFormatFull(const virSocketAddr *addr,
 
 if (virAsprintf(, "%s%s%s",
 ipv6_host ? ipv6_host : host,
-separator ? separator : ":", port) == -1)
+separator ? separator : ":", port) == -1) {
+VIR_FREE(ipv6_host);
 goto error;
+}
 
 VIR_FREE(ipv6_host);
 } else {
-- 
1.8.3.1


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

Re: [libvirt] [virt-tools-list] Problem with firewalls on Arch Linux

[Cole Robinson]

On 05/25/2017 06:08 AM, Derek Richline wrote:
> Hi all:
> 
> I noticed that Virt Manager will not start the network "default" unless
> it detects a running firewalld. Arch Linux only ships with iptables by
> default. Can Virt-Manager be patched to not check as aggressively for a
> firewall (or just issue a warning)?
> 

virt-manager doesn't have any specific firewalld checking so this is likely
coming from libvirt. I don't know anything about arch packaging, but if the
libvirt package doesn't pull in firewalld then maybe libvirt should be built
--without-firewalld

What libvirt version is this? And what's the exact error message you are seeing?

- Cole

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

Re: [libvirt] [PATCH v2 3/3] virQEMUDriverDomainABIStability: Check for memoryBacking

[Peter Krempa]

On Wed, May 24, 2017 at 16:06:56 +0100, Daniel Berrange wrote:
> On Wed, May 24, 2017 at 04:45:57PM +0200, Michal Privoznik wrote:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1450349
> > 
> > Problem is, qemu fails to load guest memory image if these
> > attribute change on migration/restore from an image.
> 
> [snip]
> 
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 35fd79de8..c1ff8ca8a 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -5797,6 +5797,46 @@ qemuDomainUpdateMemoryDeviceInfo(virQEMUDriverPtr 
> > driver,
> >  }
> >  
> >  
> > +static bool
> > +qemuDomainABIStabilityCheck(const virDomainDef *src,
> > +const virDomainDef *dst)
> > +{
> > +if (src->mem.source != dst->mem.source) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +   _("Target memoryBacking source '%s' doesn't "
> > + "match source memoryBacking source'%s'"),
> > +   virDomainMemorySourceTypeToString(dst->mem.source),
> > +   virDomainMemorySourceTypeToString(src->mem.source));
> > +return false;
> > +}
> > +
> > +if (src->mem.access != dst->mem.access) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +   _("Target memoryBacking access '%s' doesn't "
> > + "match access memoryBacking access'%s'"),
> > +   virDomainMemoryAccessTypeToString(dst->mem.access),
> > +   virDomainMemoryAccessTypeToString(src->mem.access));
> > +return false;
> > +}
> > +
> > +if (src->mem.allocation != dst->mem.allocation) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +   _("Target memoryBacking allocation '%s' doesn't "
> > + "match allocation memoryBacking allocation'%s'"),
> > +   
> > virDomainMemoryAllocationTypeToString(dst->mem.allocation),
> > +   
> > virDomainMemoryAllocationTypeToString(src->mem.allocation));
> > +return false;
> > +}
> 
> 
> Do we really need to blacklist all of these changes. I can understand that
> changing the memory source would affect migration ABI, as it causes us to
> use the memory backend command line config differently.
> 
> Assuming that matches though, I'm sceptical that changing 'access' or
> 'allocation' affects ABI.

If it does, a comment should state when it's required. (e.g. access may
need to be checked, since shared access may force usage of
memory-backend-file).

Allocation is indeed weird.


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

Re: [libvirt] [PATCH v2 2/3] virDomainXMLOption: Introduce virDomainABIStabilityDomain

[Peter Krempa]

On Wed, May 24, 2017 at 16:45:56 +0200, Michal Privoznik wrote:
> While checking for ABI stability, drivers might pose additional
> checks that are not valid for general case. For instance, qemu
> driver might check some memory backing attributes because of how
> qemu works. But those attributes may work well in other drivers.
> 
> Signed-off-by: Michal Privoznik 
> ---

[...]

> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index 5daa8d11a..cdba60b4d 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -1194,7 +1194,8 @@ virDomainSnapshotIsExternal(virDomainSnapshotObjPtr 
> snap)
>  }
>  
>  int
> -virDomainSnapshotRedefinePrep(virDomainPtr domain,
> +virDomainSnapshotRedefinePrep(virDomainXMLOptionPtr xmlopt,

xmlopt usually isn't the first argument.

> +  virDomainPtr domain,
>virDomainObjPtr vm,
>virDomainSnapshotDefPtr *defptr,
>virDomainSnapshotObjPtr *snap,

[...]

> diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c
> index 37d1a6fd9..380c3a1de 100644
> --- a/src/xenapi/xenapi_driver.c
> +++ b/src/xenapi/xenapi_driver.c
> @@ -200,7 +200,7 @@ xenapiConnectOpen(virConnectPtr conn, virConnectAuthPtr 
> auth,
>  }
>  
>  if (!(privP->xmlopt = virDomainXMLOptionNew(,
> -NULL, NULL))) {
> +NULL, NULL, NULL))) {
>  xenapiSessionErrorHandler(conn, VIR_ERR_INTERNAL_ERROR,
>_("Failed to create XML conf object"));
>  goto error;

You've missed some:

security/virt-aa-helper.c: In function 'get_definition':
security/virt-aa-helper.c:670:25: error: too few arguments to function 
'virDomainXMLOptionNew'
 if (!(ctl->xmlopt = virDomainXMLOptionNew(NULL, NULL, NULL))) {
 ^
In file included from security/security_driver.h:26:0,
 from security/virt-aa-helper.c:45:
./conf/domain_conf.h:2548:23: note: declared here
 virDomainXMLOptionPtr virDomainXMLOptionNew(virDomainDefParserConfigPtr config,

also

src/vz/vz_driver.c:!(driver->xmlopt = 
virDomainXMLOptionNew(,
src/vz/vz_driver.c- 
,
src/vz/vz_driver.c- NULL)) ||
src/vz/vz_driver.c-!(driver->domains = virDomainObjListNew()) ||

also

tests/bhyveargv2xmltest.c:if ((driver.xmlopt = virDomainXMLOptionNew(NULL, 
NULL, NULL)) == NULL)
tests/bhyveargv2xmltest.c-return EXIT_FAILURE;
tests/bhyveargv2xmltest.c-
tests/bhyveargv2xmltest.c-# define DO_TEST_FULL(name, flags)
\

ACK if you put xmlopt to a saner place in the argument list of
virDomainSnapshotRedefinePrep and fix the missing stuff above.


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

Re: [libvirt] [PATCH] CI: show the log in case of failure

[Martin Kletzander]

On Thu, May 25, 2017 at 02:19:45AM -0300, claudioandre...@gmail.com wrote:

From: Claudio André 

Disclosure the content of the 'test-suite.log' file (if available) in case of 
failures inside Travis-CI. This is needed to understand what happened and to 
provide hints about the proper fix (if applicable).
---
.travis.yml | 8 
1 file changed, 8 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index 266..5a3e765 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -83,3 +83,11 @@ matrix:
# many unit tests fail & so does syntax-check, so skip for now
# one day we must fix it though
- make -j3
+
+after_failure:
+  - echo 
''
+  - 'if [ -f $(pwd)/tests/test-suite.log ]; then
+cat $(pwd)/tests/test-suite.log;
+else
+echo "=== NO LOG FILE FOUND ===";
+fi'


I need to investigate why this doesn't work, I don't have that file
there after 'make check'.


--
2.11.0

--
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 v2 RFC 0/4] qemu: replace nested job with interruptible async job state

[Nikolay Shirokovskiy]

ping

On 02.05.2017 13:06, Nikolay Shirokovskiy wrote:
> This is the next version of RFC [1] 'drop nested job concept'. Actually
> it is quite different from the first. Patches that accomodate callers to
> use functions to enter/exit monitor with without driver and asyncJob arguments
> are moved out of this series. I guess this can be done as soon as this series
> is upstream. Most of approach details are in the first patch.
> 
> Pros:
> - better async job and concurrent regular job isolation
> - more straightforward implementation
> - more simple to use - no need to pass async job argument down the stack
> - safer to use - no warnings to be fixed if function is started to be
>   used from asynchronous job context.
> 
> TODO:
> - replace qemuDomainObjEnterMonitorAsync with qemuDomainObjEnterMonitor
> - accomodate callers to qemuDomainObjExitMonitor became returning void
> - remove passing driver and asyncJob down the stack
> - reflect changes in THREADS.txt
> 
> 
> [1] https://www.redhat.com/archives/libvir-list/2016-November/msg01357.html
> 
> Nikolay Shirokovskiy (4):
>   qemu: replace nested job with interruptible async job state
>   qemu: remove liveness check from qemuDomainObjExitMonitor
>   qemu: remove nesting job usage from qemuProcessStop
>   qemu: remove the rest of nested job parts
> 
>  src/conf/domain_conf.c|  19 
>  src/conf/domain_conf.h|   1 -
>  src/libvirt_private.syms  |   1 -
>  src/qemu/qemu_domain.c| 253 
> +-
>  src/qemu/qemu_domain.h|  29 --
>  src/qemu/qemu_driver.c|   2 +-
>  src/qemu/qemu_migration.c |  70 +++--
>  src/qemu/qemu_process.c   |  31 ++
>  8 files changed, 225 insertions(+), 181 deletions(-)
> 

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

Re: [libvirt] [PATCH] qemu: Use correct variable in qemuDomainSetBlockIoTune

[Peter Krempa]

On Thu, May 25, 2017 at 13:47:56 +0200, Ján Tomko wrote:
> On Thu, May 25, 2017 at 01:17:08PM +0200, Peter Krempa wrote:
> > 'param' contains the correct element from 'params'.
> > 
> > If the group name would not be the first element libvirtd would crash.
> > 
> 
> Is there a public bug you could link here?

There is now. I'll link it prior to pushing.


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

Re: [libvirt] [PATCH] qemu: Use correct variable in qemuDomainSetBlockIoTune

[Ján Tomko]

On Thu, May 25, 2017 at 01:17:08PM +0200, Peter Krempa wrote:

'param' contains the correct element from 'params'.

If the group name would not be the first element libvirtd would crash.



Is there a public bug you could link here?


Introduced in c53bd25b13.
---
src/qemu/qemu_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)



ACK

Jan


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

Re: [libvirt] [PATCH] qemu: Use correct variable in qemuDomainSetBlockIoTune

[Pavel Hrdina]

On Thu, May 25, 2017 at 01:17:08PM +0200, Peter Krempa wrote:
> 'param' contains the correct element from 'params'.
> 
> If the group name would not be the first element libvirtd would crash.
> 
> Introduced in c53bd25b13.
> ---
>  src/qemu/qemu_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Pavel Hrdina 


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

[libvirt] [PATCH] qemu: Use correct variable in qemuDomainSetBlockIoTune

[Peter Krempa]

'param' contains the correct element from 'params'.

If the group name would not be the first element libvirtd would crash.

Introduced in c53bd25b13.
---
 src/qemu/qemu_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index cd513ff9f..67f54282a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17443,7 +17443,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,

 /* NB: Cannot use macro since this is a value.s not a value.ul */
 if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) {
-if (VIR_STRDUP(info.group_name, params->value.s) < 0)
+if (VIR_STRDUP(info.group_name, param->value.s) < 0)
 goto endjob;
 set_fields |= QEMU_BLOCK_IOTUNE_SET_GROUP_NAME;
 if (virTypedParamsAddString(, ,
-- 
2.12.2

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

[libvirt] [PATCH v3] Regression: Report correct CPUs present on executing virsh nodecpumap

[Nitesh Konkar]

On executing the virsh nodecpumap command, the number
of CPUs present was shown as (last cpu online id + 1). This
patch fixes the issue by reporting the current number of
CPUs present.

Signed-off-by: Nitesh Konkar 
---
 src/util/virhostcpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index aa9cfeac2..c485a9721 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -1093,7 +1093,7 @@ virHostCPUGetMap(unsigned char **cpumap,
 if (online)
 *online = virBitmapCountBits(cpus);
 
-ret = virBitmapSize(cpus);
+ret = virHostCPUGetCount();
 
  cleanup:
 if (ret < 0 && cpumap)
-- 
2.11.0

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

Re: [libvirt] [PATCH v2] Regression: Report correct CPUs present on executing virsh nodecpumap

[Nitesh Konkar]

On Thu, May 25, 2017 at 2:14 PM, Ján Tomko  wrote:

> On Thu, May 25, 2017 at 02:04:16PM +0530, Nitesh Konkar wrote:
>
>> On executing the virsh nodecpumap command, the number
>> of CPUs present was shown as (last cpu online id + 1). This
>> patch fixes the issue by reporting the current number of
>> CPUs present.
>>
>> Signed-off-by: Nitesh Konkar 
>> ---
>> src/util/virhostcpu.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
>> index aa9cfeac2..6d7e8b4f4 100644
>> --- a/src/util/virhostcpu.c
>> +++ b/src/util/virhostcpu.c
>> @@ -1093,7 +1093,7 @@ virHostCPUGetMap(unsigned char **cpumap,
>> if (online)
>> *online = virBitmapCountBits(cpus);
>>
>> -ret = virBitmapSize(cpus);
>> +ret = virHostCPUParseCountLinux();
>>
>
> This should be virHostCPUGetCount, just like we have a few lines above.
> That way the function has a chance of working on FreeBSD too.
>
Yea agree. I missed out on checking for FreeBSD.
Nitesh.

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

Re: [libvirt] [PATCH v2] Regression: Report correct CPUs present on executing virsh nodecpumap

[Ján Tomko]

On Thu, May 25, 2017 at 02:04:16PM +0530, Nitesh Konkar wrote:

On executing the virsh nodecpumap command, the number
of CPUs present was shown as (last cpu online id + 1). This
patch fixes the issue by reporting the current number of
CPUs present.

Signed-off-by: Nitesh Konkar 
---
src/util/virhostcpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index aa9cfeac2..6d7e8b4f4 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -1093,7 +1093,7 @@ virHostCPUGetMap(unsigned char **cpumap,
if (online)
*online = virBitmapCountBits(cpus);

-ret = virBitmapSize(cpus);
+ret = virHostCPUParseCountLinux();


This should be virHostCPUGetCount, just like we have a few lines above.
That way the function has a chance of working on FreeBSD too.

Jan


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] Regression: Report correct CPUs present on executing virsh nodecpumap

[Nitesh Konkar]

Yes I did, limited to my knowledge.

On Thu, May 25, 2017 at 2:08 PM, Nitesh Konkar <
niteshkonkar.libv...@gmail.com> wrote:

> Yes I did.
>
> On Thu, May 25, 2017 at 2:07 PM, Peter Krempa  wrote:
>
>> On Thu, May 25, 2017 at 14:04:16 +0530, Nitesh Konkar wrote:
>> > On executing the virsh nodecpumap command, the number
>> > of CPUs present was shown as (last cpu online id + 1). This
>> > patch fixes the issue by reporting the current number of
>> > CPUs present.
>> >
>> > Signed-off-by: Nitesh Konkar 
>> > ---
>> >  src/util/virhostcpu.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Did you test it? I did not.
>>
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] Regression: Report correct CPUs present on executing virsh nodecpumap

[Peter Krempa]

On Thu, May 25, 2017 at 14:04:16 +0530, Nitesh Konkar wrote:
> On executing the virsh nodecpumap command, the number
> of CPUs present was shown as (last cpu online id + 1). This
> patch fixes the issue by reporting the current number of
> CPUs present.
> 
> Signed-off-by: Nitesh Konkar 
> ---
>  src/util/virhostcpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Did you test it? I did not.


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

[libvirt] [PATCH v2] Regression: Report correct CPUs present on executing virsh nodecpumap

[Nitesh Konkar]

On executing the virsh nodecpumap command, the number
of CPUs present was shown as (last cpu online id + 1). This
patch fixes the issue by reporting the current number of
CPUs present.

Signed-off-by: Nitesh Konkar 
---
 src/util/virhostcpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index aa9cfeac2..6d7e8b4f4 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -1093,7 +1093,7 @@ virHostCPUGetMap(unsigned char **cpumap,
 if (online)
 *online = virBitmapCountBits(cpus);
 
-ret = virBitmapSize(cpus);
+ret = virHostCPUParseCountLinux();
 
  cleanup:
 if (ret < 0 && cpumap)
-- 
2.11.0

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

Re: [libvirt] [PATCH 1/1] CI: add code coverage analysis

[Daniel P. Berrange]

On Thu, May 25, 2017 at 01:14:36AM -0300, claudioandre...@gmail.com wrote:
> From: Claudio André 
> 
> It builds the code coverage report and uploads the coverage data to a web 
> service in order to allow to track libvirt's code coverage over time.
> ---
>  .travis.yml | 11 ++-
>  README.md   |  1 +
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 266..24b8d6c 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -39,6 +39,7 @@ addons:
>- dnsmasq-base
>- librbd-dev
>- w3c-dtd-xhtml
> +  - lcov
>  
>  notifications:
>irc:
> @@ -64,7 +65,8 @@ before_install:
>  
>  # the custom PATH is just to pick up OS-X homebrew & its harmless on Linux
>  before_script:
> -  - PATH="/usr/local/opt/gettext/bin:/usr/local/opt/rpcgen/bin:$PATH" 
> ./autogen.sh
> +  - PATH="/usr/local/opt/gettext/bin:/usr/local/opt/rpcgen/bin:$PATH" 
> ./autogen.sh "$COVERAGE"
> +  - gem install coveralls-lcov

Does this actually work on OS-X ? The fact that you only enabled
testing on Trusty suggests not, and if so you shouldn't run this
in the common path.


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] [PATCH] Regression: Report correct CPUs present on executing virsh nodecpumap

[Peter Krempa]

On Wed, May 24, 2017 at 22:05:54 +0530, Nitesh Konkar wrote:
> On Wed, May 24, 2017 at 8:09 PM, Ján Tomko  wrote:
> 
> > On Wed, May 24, 2017 at 07:46:00PM +0530, Nitesh Konkar wrote:
> >
> >> Recent changes to virbitmap.c file created a regression
> >> where on executing the virsh nodecpumap command, the number
> >> of CPUs present was shown as (last cpu online id + 1). This
> >> patch fixes the issue.
> >>
> >> Signed-off-by: Nitesh Konkar 
> >> ---
> >> src/Makefile.am  |  2 ++
> >> src/util/virbitmap.c | 10 --
> >> 2 files changed, 10 insertions(+), 2 deletions(-)

[...]

> However, currently we get the following output, which is invalid.
> 
> # virsh nodecpumap
> CPUs present:   73 <--- should be 80.
> CPUs online:10
> CPU map:
> y---y---y---y---y---y---y---y---y---y
> 
> # lscpu
> Architecture:  ppc64le
> Byte Order:Little Endian
> CPU(s):80
> On-line CPU(s) list:   0,8,16,24,32,40,48,56,64,72
> Off-line CPU(s) list:  
> 1-7,9-15,17-23,25-31,33-39,41-47,49-55,57-63,65-71,73-79
> Thread(s) per core:1
> Core(s) per socket:5
> Socket(s): 2
> NUMA node(s):  2
> Model: 2.1 (pvr 004b 0201)
> Model name:POWER8E (raw), altivec supported
> L1d cache: 64K
> L1i cache: 32K
> L2 cache:  512K
> L3 cache:  8192K
> NUMA node0 CPU(s): 0,8,16,24,32
> NUMA node1 CPU(s): 40,48,56,64,72
> 
> # ls /sys/devices/system/cpu | grep cpu[0-9] | wc -l
> 80

This should be the proper fix:

diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index aa9cfeac2..6d7e8b4f4 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -1093,7 +1093,7 @@ virHostCPUGetMap(unsigned char **cpumap,
 if (online)
 *online = virBitmapCountBits(cpus);

-ret = virBitmapSize(cpus);
+ret = virHostCPUParseCountLinux();

  cleanup:
 if (ret < 0 && cpumap)



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

Re: [libvirt] [PATCH] CI: show the log in case of failure

[Martin Kletzander]

On Thu, May 25, 2017 at 02:19:45AM -0300, claudioandre...@gmail.com wrote:

From: Claudio André 

Disclosure the content of the 'test-suite.log' file (if available) in


s/Disclosure/Disclose/

ACK && Pushed.


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

Re: [libvirt] [PATCH 1/1] CI: add code coverage analysis

[Martin Kletzander]

On Thu, May 25, 2017 at 01:14:36AM -0300, claudioandre...@gmail.com wrote:

From: Claudio André 

It builds the code coverage report and uploads the coverage data to a web 
service in order to allow to track libvirt's code coverage over time.
---


You can add the links from the cover letter right here.  It is below the
'---' marker, so it won't be part of the commit message, but it is also
before the first 'diff --git', so it won't be considered part of the
patch either.  Just like the stats below.


.travis.yml | 11 ++-
README.md   |  1 +
2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index 266..24b8d6c 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -39,6 +39,7 @@ addons:
  - dnsmasq-base
  - librbd-dev
  - w3c-dtd-xhtml
+  - lcov

notifications:
  irc:
@@ -64,7 +65,8 @@ before_install:

# the custom PATH is just to pick up OS-X homebrew & its harmless on Linux
before_script:
-  - PATH="/usr/local/opt/gettext/bin:/usr/local/opt/rpcgen/bin:$PATH" 
./autogen.sh
+  - PATH="/usr/local/opt/gettext/bin:/usr/local/opt/rpcgen/bin:$PATH" ./autogen.sh 
"$COVERAGE"
+  - gem install coveralls-lcov
script:
  - VIR_TEST_DEBUG=1 make -j3 && make -j3 syntax-check && make -j3 check

@@ -77,9 +79,16 @@ matrix:
  dist: trusty
- compiler: gcc
  dist: trusty
+  env: COVERAGE="--enable-test-coverage"
- compiler: clang
  os: osx
  script:
# many unit tests fail & so does syntax-check, so skip for now
# one day we must fix it though
- make -j3
+
+after_success:
+  - 'if [ -n "${COVERAGE}" ]; then
+make -j3 cov;
+coveralls-lcov --repo-token ${COVERALLS_TOKEN} coverage/libvirt.info;


I like this, but I have no idea where COVERALLS_TOKEN comes from.  How
do we make sure that no user branches will post stuff to the
coveralls.io site?  I think I can safely guess how the rest works, and
that seems OK.


+fi'
diff --git a/README.md b/README.md
index 1035bcf..021f71b 100644
--- a/README.md
+++ b/README.md
@@ -1,4 +1,5 @@
[![Build 
Status](https://travis-ci.org/libvirt/libvirt.svg)](https://travis-ci.org/libvirt/libvirt)
+[![Coverage 
Status](https://coveralls.io/repos/github/libvirt/libvirt/badge.svg)](https://coveralls.io/github/libvirt/libvirt)

Libvirt API for virtualization
==
--
2.11.0

--
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] Regression: Report correct CPUs present on executing virsh nodecpumap

[Peter Krempa]

On Wed, May 24, 2017 at 22:05:54 +0530, Nitesh Konkar wrote:
> On Wed, May 24, 2017 at 8:09 PM, Ján Tomko  wrote:
> 
> > On Wed, May 24, 2017 at 07:46:00PM +0530, Nitesh Konkar wrote:
> >
> >> Recent changes to virbitmap.c file created a regression
> >> where on executing the virsh nodecpumap command, the number
> >> of CPUs present was shown as (last cpu online id + 1). This
> >> patch fixes the issue.
> >>
> >> Signed-off-by: Nitesh Konkar 
> >> ---
> >> src/Makefile.am  |  2 ++
> >> src/util/virbitmap.c | 10 --
> >> 2 files changed, 10 insertions(+), 2 deletions(-)

[...]

> >> @@ -565,9 +566,14 @@ virBitmapParseUnlimited(const char *str)
> >> const char *cur = str;
> >> char *tmp;
> >> size_t i;
> >> -int start, last;
> >> +int start, last, bitmapSize;
> >> +
> >> +bitmapSize = virHostCPUGetCount();
> >>
> >
> > NACK.
> >
> > This function should be able to parse any bitmap, regardless of how many
> > CPUs the host has.
> >
> > Jan
> >
> Hi Jan,
> 
> However, currently we get the following output, which is invalid.
> 
> # virsh nodecpumap
> CPUs present:   73 <--- should be 80.
> CPUs online:10
> CPU map:
> y---y---y---y---y---y---y---y---y---y


Yes, that is true, but your fix is wrong.


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

Re: [libvirt] [PATCH v2] lib: Add note that bulk stats API queries may overrun RPC buffers

[Michal Privoznik]

On 05/24/2017 09:30 PM, Martin Kletzander wrote:
> On Wed, May 24, 2017 at 05:12:06PM +0200, Michal Privoznik wrote:
>> On 05/24/2017 04:58 PM, Richard W.M. Jones wrote:
>>> On Wed, May 24, 2017 at 04:16:45PM +0200, Michal Privoznik wrote:
 On 05/24/2017 02:47 PM, Richard W.M. Jones wrote:
>
> On Wed, May 24, 2017 at 12:49:58PM +0200, Michal Privoznik wrote:
>> That's quite exact. I mean the word 'guessing'. We can't really
>> provide
>> reliable way of dealing with what you're suggesting (unless we cut
>> the
>> limit really small) nor we can guarantee atomicity. Therefore I
>> think it
>> would be a waste of time to work on this. Yes, it can be done, but
>> the
>> benefits are pretty small IMO.
>
> Why is atomicity a problem?

 The atomicity steps in depending on what level we are talking about
 serialization. If we do it the way Martin suggested (= on public API
 level) then the list of domains may change between two iterations of
 the
 suggested loop. Thus the result obtained would not reflect the reality.
 However, if we are talking on RPC level, then there's no atomicity
 problem as RPC is 'blind' to the data (it doesn't care what data is
 sent).

> Just structure the libvirtd
> messages so that you have:
>
>   COLLECT_THE_STATS
> - saves the stats into an internal buffer in libvirtd
>   and returns a handle and a number of stat items
>   RETURN_THE_STATS
> - returns partial subset of previously collected stats,
>   called multiple times to transfer the data back to libvirt
>   FREE_THE_STATS
> - free the internal buffer

 This is exactly what I was proposing in my e-mail that I linked in the
 other thread. You just wrote it more cute. Yet again, what's the
 gain/advantage of this over one big message?
>>>
>>> In the libguestfs case there is an actual security concern.  The
>>> daemon runs in the untrusted context of the guest, where a malicious
>>> guest filesystem or program could (in some cases quite easily) send
>>> back arbitrarily large messages to the library, tieing up infinite
>>> resources on the host.
>>>
>>> In libvirt the danger is possibly more on the other side (modified
>>> library sends infinitely large message to libvirtd).  There's also an
>>> issue of libvirt connecting to a compromised remote host.
>>>
>>> Whether having a maximum message size prevents all such attacks I'm
>>> less clear about, but it probably helps against simple ones.
>>
>> I'm not a security expert but I view both approach the same from
>> security POV. I mean, I'm all up for limits. Don't get me wrong there.
>> It's just that we have two options:
>>
>> a) one API call = one RPC call
>> b) one API call split across multiple RPC messages
>>
>> So, it's obvious that in case a) we need bigger limit for the RPC
>> message to fit all the data. In case b) the limit for the RPC message
>> can be smaller, but in turn we need limit for the maximum messages per
>> one API call. Both of these approaches consume about the same memory on
>> src & dst (I think option b) does consume slightly more because of all
>> the extra headers sent with each message, but that's not the point).
>> Now, if I were an attacker I can very well send one big message as well
>> as N small messages to fill up the buffers. Therefore I think the attack
>> surface is the same for both of these approaches.
>>
>> a) is easier to implement IMO.
>>
> 
> You are forgetting one thing.  If b) is only utilized on the way from
> the daemon to the client, then you can keep the number of messages
> per RPC call unlimited without any security concern.  The only thing
> that would have to get compromised is the daemon and if that happens,
> there are bigger issues than DoS'ing the client.  It would actually
> solve all of our current issues (not talking about client starting a
> domain with 10,000 disks, of course).

That's the same as dropping the limit for one message if it's send from
server to client. But I don't think that's a good idea. The limits are
there not just to protect us from attackers (that's actually transport
layer's job) but to protect us from bugs too, i.e. if we have a bug and
one side would start sending garbage, the other side is protected from
consuming all memory available. Therefore I don't think dropping limits
in whatever direction is a good idea.

But I don't want to stop any attempts in this area. Feel free to work
and propose patches that split large messages into smaller ones. Just a
couple of hints: since RPC is invisible to users, I think the solution
should be transparent to users. That means no flag passed to an API. No
such flag as SPLIT_LARGE_MESSAGES. Also, this API fixed here is probably
most likely to hit the limit, but others are good candidates fir hitting
the limit too. Therefore, if implementing this it should be generic
enough that actually 

[libvirt] [V4 RESEND PATCH] Expose resource control capabilites on cache bank

[Eli Qiao]

* This patch amends the cache bank capability as follow:


  

  
  

  


For CDP enabled on x86 arch, we will have:

  


  
...

* Added a new testdata directory `linux-resctrl-cdp` to test CDP enabled
case.

* Along with vircaps2xmltest case updated.

Signed-off-by: Eli Qiao 
---
 docs/schemas/capability.rng|  20 +++
 src/conf/capabilities.c| 138 -
 src/conf/capabilities.h|  12 ++
 .../vircaps2xmldata/linux-resctrl-cdp/resctrl/cpus |   1 +
 .../linux-resctrl-cdp/resctrl/info/L3CODE/cbm_mask |   1 +
 .../resctrl/info/L3CODE/min_cbm_bits   |   1 +
 .../resctrl/info/L3CODE/num_closids|   1 +
 .../linux-resctrl-cdp/resctrl/info/L3DATA/cbm_mask |   1 +
 .../resctrl/info/L3DATA/min_cbm_bits   |   1 +
 .../resctrl/info/L3DATA/num_closids|   1 +
 .../linux-resctrl-cdp/resctrl/manualres/cpus   |   1 +
 .../linux-resctrl-cdp/resctrl/manualres/schemata   |   2 +
 .../linux-resctrl-cdp/resctrl/manualres/tasks  |   0
 .../linux-resctrl-cdp/resctrl/schemata |   2 +
 .../linux-resctrl-cdp/resctrl/tasks|   0
 tests/vircaps2xmldata/linux-resctrl-cdp/system |   1 +
 .../vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml |  55 
 tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml   |   8 +-
 tests/vircaps2xmltest.c|  10 +-
 19 files changed, 252 insertions(+), 4 deletions(-)
 create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/cpus
 create mode 100755 
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/cbm_mask
 create mode 100755 
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/min_cbm_bits
 create mode 100755 
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3CODE/num_closids
 create mode 100755 
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/cbm_mask
 create mode 100755 
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/min_cbm_bits
 create mode 100755 
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/info/L3DATA/num_closids
 create mode 100644 
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/cpus
 create mode 100644 
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/schemata
 create mode 100644 
tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/manualres/tasks
 create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/schemata
 create mode 100644 tests/vircaps2xmldata/linux-resctrl-cdp/resctrl/tasks
 create mode 12 tests/vircaps2xmldata/linux-resctrl-cdp/system
 create mode 100644 tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml

diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
index 26f0aa2..927fc18 100644
--- a/docs/schemas/capability.rng
+++ b/docs/schemas/capability.rng
@@ -277,6 +277,26 @@
   
 
   
+  
+
+  
+
+  
+  
+
+  
+  
+
+  both
+  code
+  data
+
+  
+  
+
+  
+
+  
 
   
 
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index a91a72a..0888605 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -51,6 +51,7 @@
 #define VIR_FROM_THIS VIR_FROM_CAPABILITIES
 
 #define SYSFS_SYSTEM_PATH "/sys/devices/system"
+#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl"
 
 VIR_LOG_INIT("conf.capabilities")
 
@@ -872,6 +873,9 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
 virCapsHostCacheBankPtr *caches)
 {
 size_t i = 0;
+size_t j = 0;
+int indent = virBufferGetIndent(buf, false);
+virBuffer controlBuf = VIR_BUFFER_INITIALIZER;
 
 if (!ncaches)
 return 0;
@@ -893,13 +897,35 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
  */
 virBufferAsprintf(buf,
   "\n",
+  "size='%llu' unit='%s' cpus='%s'",
   bank->id, bank->level,
   virCacheTypeToString(bank->type),
   bank->size >> (kilos * 10),
   kilos ? "KiB" : "B",
   cpus_str);
 
+virBufferAdjustIndent(, indent + 4);
+for (j = 0; j < bank->ncontrols; j++) {
+bool min_kilos = !(bank->controls[j]->min % 1024);
+virBufferAsprintf(,
+  "\n",
+  bank->controls[j]->min >> (min_kilos * 10),
+  min_kilos ? "KiB" : "B",
+  virCacheTypeToString(bank->controls[j]->scope),
+  bank->controls[j]->max_allocation);
+}
+
+if (virBufferUse()) {
+

Re: [libvirt] [PATCH 2/2] virsh: Don't attempt polkit processing for non local authn/authz

[Michal Privoznik]

On 05/24/2017 05:45 PM, John Ferlan wrote:
> 
> 
> On 05/24/2017 10:38 AM, Michal Privoznik wrote:
>> On 05/11/2017 05:04 PM, John Ferlan wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1374126
>>>
>>> Due to how the processing for authentication using polkit works, the
>>> virshConnect code must first "attempt" an virConnectOpenAuth and then
>>> check for a "special" return error code VIR_ERR_AUTH_UNAVAILABLE in
>>> order to attempt to "retry" the authentication after performing a creation
>>> of a pkttyagent to handle the challenge/response for the client.
>>>
>>> However, attempting to use a remote connection, (such as perhaps
>>> "qemu+ssh://someuser@localhost/system"), will cause a never ending
>>> loop since attempting to generate a pkttyagent would fail for the
>>> network client connection resulting in a never ending loop since the
>>> return code is always VIR_ERR_AUTH_UNAVAILABLE from virPolkitCheckAuth.
>>> The only way out of the loop is a forced quit (e.g. ctrl-c) as the
>>> @authfail wouldn't be incremented as a result of a failed authn from
>>> pkttyagent.
>>>
>>> So rather than take any extra step for which the only result will be
>>> a failure, let's check if there is a URI and if it's not using ":///",
>>> then just fail.
>>>
>>> This resolves the never ending loop and will generate an error:
>>>
>>> error: failed to connect to the hypervisor
>>> error: authentication unavailable: no polkit agent available to 
>>> authenticate action 'org.libvirt.unix.manage'
>>>
>>> NB: If the authentication was for a sufficiently privileged client, such as
>>> qemu+ssh://root@localhost/system, then the remoteDispatchAuthList "allows"
>>> the authentication to use libvirt since @callerUid would be 0.
>>>
>>> Signed-off-by: John Ferlan 
>>> ---
>>>  tools/virsh.c | 5 +
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/tools/virsh.c b/tools/virsh.c
>>> index 1f5c2b1..be368ba 100644
>>> --- a/tools/virsh.c
>>> +++ b/tools/virsh.c
>>> @@ -166,6 +166,11 @@ virshConnect(vshControl *ctl, const char *uri, bool 
>>> readonly)
>>>  if (readonly)
>>>  goto cleanup;
>>>  
>>> +/* No URI or indication of a requesting a remote connection, then
>>> + * polkit will not work for the authentication/authorization */
>>> +if (!uri || !(strstr(uri, ":///")))
>>> +goto cleanup;
>>
>> But we can get here even without polkit, right? Therefore it'd be much
>> more safer to check err && err->domain == VIR_FROM_POLKIT.
>>
> 
> True - that's simple enough to adjust... 
> 
> So instead of:
> 
> err = virGetLastError();
> if (err && err->domain == VIR_FROM_POLKIT &&
> err->code == VIR_ERR_AUTH_UNAVAILABLE) {
> if (!pkagent && !(pkagent = virPolkitAgentCreate()))
> goto cleanup;
> } else if (err && err->domain == VIR_FROM_POLKIT &&
>err->code == VIR_ERR_AUTH_FAILED) {
> authfail++;
> } else {
> goto cleanup;
> }
> 
> I could have
> 
> if (err && err->domain == VIR_FROM_POLKIT) {
> /* No URI or indication of a requesting a remote connection, then
>  * polkit will not work for the authentication/authorization */
> if (!uri || !(strstr(uri, ":///")))
> goto cleanup;
> if (err->code == VIR_ERR_AUTH_UNAVAILABLE) {
> if (!pkagent && !(pkagent = virPolkitAgentCreate()))
> goto cleanup;
> } else if (err->code == VIR_ERR_AUTH_FAILED) {
> authfail++;
> } else {
> goto cleanup;
> }
> } else {
> goto cleanup;
> }
> 
> 
> So is there a preferred approach?  See the cover letter...

Aha. So these two patches are mutually exclusive? I haven't read it
until now O:-)
Well, I like the first one better because it's more simple. It fixes
just what is broken and is not introducing any bug.

Michal

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