Re: [libvirt] [PATCH] libxl: open libxl log stream with libvirtd log_level

2015-09-15 Thread Jim Fehlig 

On 09/15/2015 04:06 PM, John Ferlan wrote:


On 09/15/2015 01:21 PM, Jim Fehlig wrote:

Daniel P. Berrange wrote:

On Tue, Sep 15, 2015 at 10:57:50AM -0600, Jim Fehlig wrote:
   

Daniel P. Berrange wrote:
 

On Tue, Sep 15, 2015 at 09:26:23AM -0600, Jim Fehlig wrote:
   
   

Instead of a hardcoded DEBUG log level, use the overall
daemon log level specified in libvirtd.conf when opening
a log stream with libxl. libxl is very verbose when DEBUG
log level is set, resulting in huge log files that can
potentially fill a disk. Control of libxl verbosity should
be placed in the administrator's hands.

Signed-off-by: Jim Fehlig 
---
  src/libxl/libxl_conf.c | 18 +-
  1 file changed, 17 insertions(+), 1 deletion(-)
 
 

ACK, this makes sense as default behaviour. As a future enhancement
you might also consider supporting a config setting in /etc/libvirt/libxl.conf
to explicitly control the libxl library logging behaviour independantly.
   
   

I had actually thought of adding it there first, but then took this
approach assuming it would be more receptive upstream :-). Personally,
I'm on the fence. I like the idea of a single knob to control log level
throughout the daemon, making it a bit easier on admins. On the other
hand, individual knobs are more friendly to those pouring through logs.
I can add a knob in libxl.conf if preferred.
 

After thinking about it some more, I could actually see value in
create a dedicated virLogSource() instance, solely for libxl
library messages. If we then created a virLogSourceGetPriority()
method, you could query that to see if to turn on logging for
the libxl library. That would ultimately allow you to turn on
debug for just the libxl library if desired, eg

  static virLogSource virLogLibXL = {
  .name = "libxl.libxl_library",
  
  }

LIBVIRT_LOG_FILTERS="1:libxl_library"
   

Ah, good idea. I'll look into it.


Regardless we should just merge your current patch right now.
   

Thanks; done now.

Regards,
Jim


Looks like compilations on Fedora are 'unhappy':

libxl/libxl_conf.c: In function 'libxlDriverConfigNew':
libxl/libxl_conf.c:1560:30: error: 'log_level' may be used uninitialized
in this function [-Werror=maybe-uninitialized]
  (xentoollog_logger *)xtl_createlogger_stdiostream(cfg->logger_file,
   ^
libxl/libxl_conf.c:1499:22: note: 'log_level' was declared here
  xentoollog_level log_level;
   ^
cc1: all warnings being treated as errors
Makefile:8000: recipe for target
'libxl/libvirt_driver_libxl_impl_la-libxl_conf.lo' failed


You can also see failures today in libvirt-daemon-build and
libvirt-daemon-check from:

https://ci.centos.org/view/libvirt-project/


I wasn't sure if initializing log_level to XTL_DEBUG and then changing
if virLogGetDefaultPriority returns something else would be better or
going the "default:" route in the switch statement would be better.


I took the bit more explicit route and went with the former, worried that 
Coverity would be the next thing to complain :-).


I considered initializing log_level to XTL_WARN, matching VIR_LOG_DEFAULT, but 
in the end went with DEBUG. It matches the behavior before this change, and is 
reasonable default if we're already this confused while loading the driver.


Regards,
Jim

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

[libvirt] [PATCH] libxl: fix compiler error introduced by commit ba25c214

2015-09-15 Thread Jim Fehlig 
libxl/libxl_conf.c: In function 'libxlDriverConfigNew':
libxl/libxl_conf.c:1560:30: error: 'log_level' may be used uninitialized
in this function [-Werror=maybe-uninitialized]

Signed-off-by: Jim Fehlig 
---

Pushing under the build-breaker rule.

 src/libxl/libxl_conf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 40fa4b5..35fd495 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1496,7 +1496,7 @@ libxlDriverConfigNew(void)
 {
 libxlDriverConfigPtr cfg;
 char *log_file = NULL;
-xentoollog_level log_level;
+xentoollog_level log_level = XTL_DEBUG;
 char ebuf[1024];
 unsigned int free_mem;
 
-- 
2.5.0

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

Re: [libvirt] [PATCH] libxl: open libxl log stream with libvirtd log_level

2015-09-15 Thread Jim Fehlig 

> On Sep 15, 2015, at 4:06 PM, John Ferlan  wrote:
> 
> 
> 
>> On 09/15/2015 01:21 PM, Jim Fehlig wrote:
>> Daniel P. Berrange wrote:
 On Tue, Sep 15, 2015 at 10:57:50AM -0600, Jim Fehlig wrote:
 
 Daniel P. Berrange wrote:
 
> On Tue, Sep 15, 2015 at 09:26:23AM -0600, Jim Fehlig wrote:
> 
> 
>> Instead of a hardcoded DEBUG log level, use the overall
>> daemon log level specified in libvirtd.conf when opening
>> a log stream with libxl. libxl is very verbose when DEBUG
>> log level is set, resulting in huge log files that can
>> potentially fill a disk. Control of libxl verbosity should
>> be placed in the administrator's hands.
>> 
>> Signed-off-by: Jim Fehlig 
>> ---
>> src/libxl/libxl_conf.c | 18 +-
>> 1 file changed, 17 insertions(+), 1 deletion(-)
> ACK, this makes sense as default behaviour. As a future enhancement
> you might also consider supporting a config setting in 
> /etc/libvirt/libxl.conf
> to explicitly control the libxl library logging behaviour independantly.
 I had actually thought of adding it there first, but then took this
 approach assuming it would be more receptive upstream :-). Personally,
 I'm on the fence. I like the idea of a single knob to control log level
 throughout the daemon, making it a bit easier on admins. On the other
 hand, individual knobs are more friendly to those pouring through logs.
 I can add a knob in libxl.conf if preferred.
>>> 
>>> After thinking about it some more, I could actually see value in
>>> create a dedicated virLogSource() instance, solely for libxl
>>> library messages. If we then created a virLogSourceGetPriority()
>>> method, you could query that to see if to turn on logging for
>>> the libxl library. That would ultimately allow you to turn on
>>> debug for just the libxl library if desired, eg
>>> 
>>> static virLogSource virLogLibXL = {
>>> .name = "libxl.libxl_library",
>>> 
>>> }
>>> 
>>> LIBVIRT_LOG_FILTERS="1:libxl_library"
>> 
>> Ah, good idea. I'll look into it.
>> 
>>> Regardless we should just merge your current patch right now.
>> 
>> Thanks; done now.
>> 
>> Regards,
>> Jim
> 
> Looks like compilations on Fedora are 'unhappy':

Hrm, thanks for bringing it to my attention. I'll send a patch later when I 
have something in front of me besides a mobile device.

Regards,
Jim

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

Re: [libvirt] [PATCH] libxl: open libxl log stream with libvirtd log_level

2015-09-15 Thread John Ferlan 


On 09/15/2015 01:21 PM, Jim Fehlig wrote:
> Daniel P. Berrange wrote:
>> On Tue, Sep 15, 2015 at 10:57:50AM -0600, Jim Fehlig wrote:
>>   
>>> Daniel P. Berrange wrote:
>>> 
 On Tue, Sep 15, 2015 at 09:26:23AM -0600, Jim Fehlig wrote:
   
   
> Instead of a hardcoded DEBUG log level, use the overall
> daemon log level specified in libvirtd.conf when opening
> a log stream with libxl. libxl is very verbose when DEBUG
> log level is set, resulting in huge log files that can
> potentially fill a disk. Control of libxl verbosity should
> be placed in the administrator's hands.
>
> Signed-off-by: Jim Fehlig 
> ---
>  src/libxl/libxl_conf.c | 18 +-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> 
 ACK, this makes sense as default behaviour. As a future enhancement
 you might also consider supporting a config setting in 
 /etc/libvirt/libxl.conf
 to explicitly control the libxl library logging behaviour independantly.
   
   
>>> I had actually thought of adding it there first, but then took this
>>> approach assuming it would be more receptive upstream :-). Personally,
>>> I'm on the fence. I like the idea of a single knob to control log level
>>> throughout the daemon, making it a bit easier on admins. On the other
>>> hand, individual knobs are more friendly to those pouring through logs.
>>> I can add a knob in libxl.conf if preferred.
>>> 
>>
>> After thinking about it some more, I could actually see value in
>> create a dedicated virLogSource() instance, solely for libxl
>> library messages. If we then created a virLogSourceGetPriority()
>> method, you could query that to see if to turn on logging for
>> the libxl library. That would ultimately allow you to turn on
>> debug for just the libxl library if desired, eg
>>
>>  static virLogSource virLogLibXL = {
>>  .name = "libxl.libxl_library",
>>  
>>  }
>>
>> LIBVIRT_LOG_FILTERS="1:libxl_library"
>>   
> 
> Ah, good idea. I'll look into it.
> 
>> Regardless we should just merge your current patch right now.
>>   
> 
> Thanks; done now.
> 
> Regards,
> Jim
> 

Looks like compilations on Fedora are 'unhappy':

libxl/libxl_conf.c: In function 'libxlDriverConfigNew':
libxl/libxl_conf.c:1560:30: error: 'log_level' may be used uninitialized
in this function [-Werror=maybe-uninitialized]
 (xentoollog_logger *)xtl_createlogger_stdiostream(cfg->logger_file,
  ^
libxl/libxl_conf.c:1499:22: note: 'log_level' was declared here
 xentoollog_level log_level;
  ^
cc1: all warnings being treated as errors
Makefile:8000: recipe for target
'libxl/libvirt_driver_libxl_impl_la-libxl_conf.lo' failed


You can also see failures today in libvirt-daemon-build and
libvirt-daemon-check from:

https://ci.centos.org/view/libvirt-project/


I wasn't sure if initializing log_level to XTL_DEBUG and then changing
if virLogGetDefaultPriority returns something else would be better or
going the "default:" route in the switch statement would be better.

John


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

[libvirt] [PATCH] qemu: Check for existence of auto-generated socket path before removing

2015-09-15 Thread John Ferlan 
Commit id 'f1f68ca33' added code to remove the directory paths for
auto-generated sockets, but that code could be called before the
paths were created resulting in generating error messages from
virFileDeleteTree indicating that the file doesn't exist. So just
add a check before attemping the directory delete for existence.

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

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ce2c70c..b55eb52 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5269,13 +5269,13 @@ void qemuProcessStop(virQEMUDriverPtr driver,
 
 ignore_value(virAsprintf(&tmppath, "%s/domain-%s",
  cfg->libDir, vm->def->name));
-if (tmppath)
+if (tmppath && virFileExists(tmppath))
 virFileDeleteTree(tmppath);
 VIR_FREE(tmppath);
 
 ignore_value(virAsprintf(&tmppath, "%s/domain-%s",
  cfg->channelTargetDir, vm->def->name));
-if (tmppath)
+if (tmppath && virFileExists(tmppath))
 virFileDeleteTree(tmppath);
 VIR_FREE(tmppath);
 
-- 
2.1.0

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

Re: [libvirt] [PATCH] libxl: open libxl log stream with libvirtd log_level

2015-09-15 Thread Jim Fehlig 
Daniel P. Berrange wrote:
> On Tue, Sep 15, 2015 at 10:57:50AM -0600, Jim Fehlig wrote:
>   
>> Daniel P. Berrange wrote:
>> 
>>> On Tue, Sep 15, 2015 at 09:26:23AM -0600, Jim Fehlig wrote:
>>>   
>>>   
 Instead of a hardcoded DEBUG log level, use the overall
 daemon log level specified in libvirtd.conf when opening
 a log stream with libxl. libxl is very verbose when DEBUG
 log level is set, resulting in huge log files that can
 potentially fill a disk. Control of libxl verbosity should
 be placed in the administrator's hands.

 Signed-off-by: Jim Fehlig 
 ---
  src/libxl/libxl_conf.c | 18 +-
  1 file changed, 17 insertions(+), 1 deletion(-)
 
 
>>> ACK, this makes sense as default behaviour. As a future enhancement
>>> you might also consider supporting a config setting in 
>>> /etc/libvirt/libxl.conf
>>> to explicitly control the libxl library logging behaviour independantly.
>>>   
>>>   
>> I had actually thought of adding it there first, but then took this
>> approach assuming it would be more receptive upstream :-). Personally,
>> I'm on the fence. I like the idea of a single knob to control log level
>> throughout the daemon, making it a bit easier on admins. On the other
>> hand, individual knobs are more friendly to those pouring through logs.
>> I can add a knob in libxl.conf if preferred.
>> 
>
> After thinking about it some more, I could actually see value in
> create a dedicated virLogSource() instance, solely for libxl
> library messages. If we then created a virLogSourceGetPriority()
> method, you could query that to see if to turn on logging for
> the libxl library. That would ultimately allow you to turn on
> debug for just the libxl library if desired, eg
>
>  static virLogSource virLogLibXL = {
>  .name = "libxl.libxl_library",
>  
>  }
>
> LIBVIRT_LOG_FILTERS="1:libxl_library"
>   

Ah, good idea. I'll look into it.

> Regardless we should just merge your current patch right now.
>   

Thanks; done now.

Regards,
Jim

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

Re: [libvirt] [PATCH] libxl: open libxl log stream with libvirtd log_level

2015-09-15 Thread Daniel P. Berrange 
On Tue, Sep 15, 2015 at 10:57:50AM -0600, Jim Fehlig wrote:
> Daniel P. Berrange wrote:
> > On Tue, Sep 15, 2015 at 09:26:23AM -0600, Jim Fehlig wrote:
> >   
> >> Instead of a hardcoded DEBUG log level, use the overall
> >> daemon log level specified in libvirtd.conf when opening
> >> a log stream with libxl. libxl is very verbose when DEBUG
> >> log level is set, resulting in huge log files that can
> >> potentially fill a disk. Control of libxl verbosity should
> >> be placed in the administrator's hands.
> >>
> >> Signed-off-by: Jim Fehlig 
> >> ---
> >>  src/libxl/libxl_conf.c | 18 +-
> >>  1 file changed, 17 insertions(+), 1 deletion(-)
> >> 
> >
> > ACK, this makes sense as default behaviour. As a future enhancement
> > you might also consider supporting a config setting in 
> > /etc/libvirt/libxl.conf
> > to explicitly control the libxl library logging behaviour independantly.
> >   
> 
> I had actually thought of adding it there first, but then took this
> approach assuming it would be more receptive upstream :-). Personally,
> I'm on the fence. I like the idea of a single knob to control log level
> throughout the daemon, making it a bit easier on admins. On the other
> hand, individual knobs are more friendly to those pouring through logs.
> I can add a knob in libxl.conf if preferred.

After thinking about it some more, I could actually see value in
create a dedicated virLogSource() instance, solely for libxl
library messages. If we then created a virLogSourceGetPriority()
method, you could query that to see if to turn on logging for
the libxl library. That would ultimately allow you to turn on
debug for just the libxl library if desired, eg

 static virLogSource virLogLibXL = {
 .name = "libxl.libxl_library",
 
 }

LIBVIRT_LOG_FILTERS="1:libxl_library"

Regardless we should just merge your current patch right now.

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

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

Re: [libvirt] [PATCH] libxl: open libxl log stream with libvirtd log_level

2015-09-15 Thread Jim Fehlig 
Daniel P. Berrange wrote:
> On Tue, Sep 15, 2015 at 09:26:23AM -0600, Jim Fehlig wrote:
>   
>> Instead of a hardcoded DEBUG log level, use the overall
>> daemon log level specified in libvirtd.conf when opening
>> a log stream with libxl. libxl is very verbose when DEBUG
>> log level is set, resulting in huge log files that can
>> potentially fill a disk. Control of libxl verbosity should
>> be placed in the administrator's hands.
>>
>> Signed-off-by: Jim Fehlig 
>> ---
>>  src/libxl/libxl_conf.c | 18 +-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>> 
>
> ACK, this makes sense as default behaviour. As a future enhancement
> you might also consider supporting a config setting in /etc/libvirt/libxl.conf
> to explicitly control the libxl library logging behaviour independantly.
>   

I had actually thought of adding it there first, but then took this
approach assuming it would be more receptive upstream :-). Personally,
I'm on the fence. I like the idea of a single knob to control log level
throughout the daemon, making it a bit easier on admins. On the other
hand, individual knobs are more friendly to those pouring through logs.
I can add a knob in libxl.conf if preferred.

Regards,
Jim

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

Re: [libvirt] [PATCH v2] qemu: Fix using guest architecture as lookup key

2015-09-15 Thread Daniel P. Berrange 
On Tue, Sep 15, 2015 at 04:44:13PM +0200, Andrea Bolognani wrote:
> When looking for a QEMU binary suitable for running ppc64le guests
> we have to take into account the fact that we use the QEMU target
> as key for the hash, so direct comparison is not good enough.
> 
> Factor out the logic from virQEMUCapsFindBinaryForArch() to a new
> virQEMUCapsFindTarget() function and use that both when looking
> for QEMU binaries available on the system and when looking up
> QEMU capabilities later.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1260753
> ---
>  src/qemu/qemu_capabilities.c | 100 
> ---
>  1 file changed, 65 insertions(+), 35 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 4ad1bdb..6757907 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -386,6 +386,28 @@ static const char *virQEMUCapsArchToString(virArch arch)
>  return virArchToString(arch);
>  }
>  
> +/* Given a host and guest architectures, find a suitable QEMU target.
> + *
> + * This is meant to be used as a second attempt if qemu-system-$guestarch
> + * can't be found, eg. on a x86_64 host you want to use qemu-system-i386,
> + * if available, instead of qemu-system-x86_64 to run i686 guests */
> +static virArch
> +virQEMUCapsFindTarget(virArch hostarch,
> +  virArch guestarch)
> +{
> +/* Both ppc64 and ppc64le guests can use the ppc64 target */
> +if (ARCH_IS_PPC64(guestarch))
> +guestarch = VIR_ARCH_PPC64;
> +
> +/* armv7l guests on aarch64 hosts can use the aarch64 target
> + * i686 guests on x86_64 hosts can use the x86_64 target */
> +if ((guestarch == VIR_ARCH_ARMV7L && hostarch == VIR_ARCH_AARCH64) ||
> +(guestarch == VIR_ARCH_I686 && hostarch == VIR_ARCH_X86_64)) {
> +return hostarch;
> +}
> +
> +return guestarch;
> +}
>  
>  static virCommandPtr
>  virQEMUCapsProbeCommand(const char *qemu,
> @@ -690,51 +712,52 @@ virQEMUCapsProbeCPUModels(virQEMUCapsPtr qemuCaps, 
> uid_t runUid, gid_t runGid)
>  return ret;
>  }
>  
> -
>  static char *
> -virQEMUCapsFindBinaryForArch(virArch hostarch,
> - virArch guestarch)
> +virQEMUCapsFindBinary(const char *format,
> +  const char *archstr)
>  {
> -char *ret;
> -const char *archstr;
> -char *binary;
> -
> -if (ARCH_IS_PPC64(guestarch))
> -archstr = virQEMUCapsArchToString(VIR_ARCH_PPC64);
> -else
> -archstr = virQEMUCapsArchToString(guestarch);
> +char *ret = NULL;
> +char *binary = NULL;
>  
> -if (virAsprintf(&binary, "qemu-system-%s", archstr) < 0)
> -return NULL;
> +if (virAsprintf(&binary, format, archstr) < 0)
> +goto out;
>  
>  ret = virFindFileInPath(binary);
>  VIR_FREE(binary);
> -if (ret && !virFileIsExecutable(ret))
> -VIR_FREE(ret);
> +if (ret && virFileIsExecutable(ret))
> +goto out;
>  
> -if (guestarch == VIR_ARCH_ARMV7L &&
> -!ret &&
> -hostarch == VIR_ARCH_AARCH64) {
> -ret = virFindFileInPath("qemu-system-aarch64");
> -if (ret && !virFileIsExecutable(ret))
> -VIR_FREE(ret);
> -}
> +VIR_FREE(ret);
>  
> -if (guestarch == VIR_ARCH_I686 &&
> -!ret &&
> -hostarch == VIR_ARCH_X86_64) {
> -ret = virFindFileInPath("qemu-system-x86_64");
> -if (ret && !virFileIsExecutable(ret))
> -VIR_FREE(ret);
> -}
> + out:
> +return ret;
> +}
> +
> +static char *
> +virQEMUCapsFindBinaryForArch(virArch hostarch,
> + virArch guestarch)
> +{
> +char *ret = NULL;
> +const char *archstr;
> +
> +/* First attempt: try the guest architecture as it is */
> +archstr = virQEMUCapsArchToString(guestarch);
> +if ((ret = virQEMUCapsFindBinary("qemu-system-%s", archstr)) != NULL)
> +goto out;
>  
> -if (guestarch == VIR_ARCH_I686 &&
> -!ret) {
> -ret = virFindFileInPath("qemu");
> -if (ret && !virFileIsExecutable(ret))
> -VIR_FREE(ret);
> +/* Second attempt: use some arch-specific rules */
> +archstr = virQEMUCapsArchToString(virQEMUCapsFindTarget(hostarch,
> +guestarch));

Nitpick, we could avoid the second search if virQEMUCapsFindTarget()
returns a value that == the original guestarch.

> +if ((ret = virQEMUCapsFindBinary("qemu-system-%s", archstr)) != NULL)
> +goto out;
> +
> +/* Third attempt, i686 only: try 'qemu' */
> +if (guestarch == VIR_ARCH_I686) {
> +if ((ret = virQEMUCapsFindBinary("%s", "qemu")) != NULL)
> +goto out;
>  }
>  
> + out:
>  return ret;
>  }
>  
> @@ -3793,10 +3816,17 @@ virQEMUCapsCacheLookupByArch(virQEMUCapsCachePtr 
> cache,
>  
>  virMutexLock(&cache->lock);
>  ret = virHashSearch(cache->binar

Re: [libvirt] [PATCH] libxl: open libxl log stream with libvirtd log_level

2015-09-15 Thread Daniel P. Berrange 
On Tue, Sep 15, 2015 at 09:26:23AM -0600, Jim Fehlig wrote:
> Instead of a hardcoded DEBUG log level, use the overall
> daemon log level specified in libvirtd.conf when opening
> a log stream with libxl. libxl is very verbose when DEBUG
> log level is set, resulting in huge log files that can
> potentially fill a disk. Control of libxl verbosity should
> be placed in the administrator's hands.
> 
> Signed-off-by: Jim Fehlig 
> ---
>  src/libxl/libxl_conf.c | 18 +-
>  1 file changed, 17 insertions(+), 1 deletion(-)

ACK, this makes sense as default behaviour. As a future enhancement
you might also consider supporting a config setting in /etc/libvirt/libxl.conf
to explicitly control the libxl library logging behaviour independantly.

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

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

Re: [libvirt] [Xen-devel] [PATCH] libxl: open libxl log stream with libvirtd log_level

2015-09-15 Thread Ian Campbell 
On Tue, 2015-09-15 at 09:26 -0600, Jim Fehlig wrote:
> Instead of a hardcoded DEBUG log level, use the overall
> daemon log level specified in libvirtd.conf when opening
> a log stream with libxl. libxl is very verbose when DEBUG
> log level is set, resulting in huge log files that can
> potentially fill a disk. Control of libxl verbosity should
> be placed in the administrator's hands.
> 
> Signed-off-by: Jim Fehlig 

FWIW this seems like a good idea to me:
Acked-by: Ian Campbell 

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

[libvirt] [PATCH] libxl: open libxl log stream with libvirtd log_level

2015-09-15 Thread Jim Fehlig 
Instead of a hardcoded DEBUG log level, use the overall
daemon log level specified in libvirtd.conf when opening
a log stream with libxl. libxl is very verbose when DEBUG
log level is set, resulting in huge log files that can
potentially fill a disk. Control of libxl verbosity should
be placed in the administrator's hands.

Signed-off-by: Jim Fehlig 
---
 src/libxl/libxl_conf.c | 18 +-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index a76ad5a..40fa4b5 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1496,6 +1496,7 @@ libxlDriverConfigNew(void)
 {
 libxlDriverConfigPtr cfg;
 char *log_file = NULL;
+xentoollog_level log_level;
 char ebuf[1024];
 unsigned int free_mem;
 
@@ -1540,9 +1541,24 @@ libxlDriverConfigNew(void)
 }
 VIR_FREE(log_file);
 
+switch (virLogGetDefaultPriority()) {
+case VIR_LOG_DEBUG:
+log_level = XTL_DEBUG;
+break;
+case VIR_LOG_INFO:
+log_level = XTL_INFO;
+break;
+case VIR_LOG_WARN:
+log_level = XTL_WARN;
+break;
+case VIR_LOG_ERROR:
+log_level = XTL_ERROR;
+break;
+}
+
 cfg->logger =
 (xentoollog_logger *)xtl_createlogger_stdiostream(cfg->logger_file,
-  XTL_DEBUG, XTL_STDIOSTREAM_SHOW_DATE);
+  log_level, XTL_STDIOSTREAM_SHOW_DATE);
 if (!cfg->logger) {
 VIR_ERROR(_("cannot create logger for libxenlight, disabling driver"));
 goto error;
-- 
2.5.0

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

Re: [libvirt] [PATCH] qemu: The ppc64 target can run both ppc64 and ppc64le guests

2015-09-15 Thread Andrea Bolognani 
On Tue, 2015-09-15 at 13:48 +0100, Daniel P. Berrange wrote:
> The virQEMUCapsFindBinaryForArch metho already has a bunch of logic
> to
> deal with fact that one binary can support multiple different
> architectures. For example, x86_64 can support i686. It looks like we
> hit the same problem as ppc64/ppc64le in that case and you have not
> handled it here. It would be desirable to avoid having to have the
> same
> compat logic in two places too.

I've just sent out a v2[1] which takes the logic you're referring
to out of virQEMUCapsFindBinaryForArch() and into a separate
function which is then reused when looking up capabilities.

Of course it's a slightly larger patch this time around :)

Cheers.


[1] https://www.redhat.com/archives/libvir-list/2015-September/msg00475
.html
-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

[libvirt] [PATCH v2] qemu: Fix using guest architecture as lookup key

2015-09-15 Thread Andrea Bolognani 
When looking for a QEMU binary suitable for running ppc64le guests
we have to take into account the fact that we use the QEMU target
as key for the hash, so direct comparison is not good enough.

Factor out the logic from virQEMUCapsFindBinaryForArch() to a new
virQEMUCapsFindTarget() function and use that both when looking
for QEMU binaries available on the system and when looking up
QEMU capabilities later.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1260753
---
 src/qemu/qemu_capabilities.c | 100 ---
 1 file changed, 65 insertions(+), 35 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 4ad1bdb..6757907 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -386,6 +386,28 @@ static const char *virQEMUCapsArchToString(virArch arch)
 return virArchToString(arch);
 }
 
+/* Given a host and guest architectures, find a suitable QEMU target.
+ *
+ * This is meant to be used as a second attempt if qemu-system-$guestarch
+ * can't be found, eg. on a x86_64 host you want to use qemu-system-i386,
+ * if available, instead of qemu-system-x86_64 to run i686 guests */
+static virArch
+virQEMUCapsFindTarget(virArch hostarch,
+  virArch guestarch)
+{
+/* Both ppc64 and ppc64le guests can use the ppc64 target */
+if (ARCH_IS_PPC64(guestarch))
+guestarch = VIR_ARCH_PPC64;
+
+/* armv7l guests on aarch64 hosts can use the aarch64 target
+ * i686 guests on x86_64 hosts can use the x86_64 target */
+if ((guestarch == VIR_ARCH_ARMV7L && hostarch == VIR_ARCH_AARCH64) ||
+(guestarch == VIR_ARCH_I686 && hostarch == VIR_ARCH_X86_64)) {
+return hostarch;
+}
+
+return guestarch;
+}
 
 static virCommandPtr
 virQEMUCapsProbeCommand(const char *qemu,
@@ -690,51 +712,52 @@ virQEMUCapsProbeCPUModels(virQEMUCapsPtr qemuCaps, uid_t 
runUid, gid_t runGid)
 return ret;
 }
 
-
 static char *
-virQEMUCapsFindBinaryForArch(virArch hostarch,
- virArch guestarch)
+virQEMUCapsFindBinary(const char *format,
+  const char *archstr)
 {
-char *ret;
-const char *archstr;
-char *binary;
-
-if (ARCH_IS_PPC64(guestarch))
-archstr = virQEMUCapsArchToString(VIR_ARCH_PPC64);
-else
-archstr = virQEMUCapsArchToString(guestarch);
+char *ret = NULL;
+char *binary = NULL;
 
-if (virAsprintf(&binary, "qemu-system-%s", archstr) < 0)
-return NULL;
+if (virAsprintf(&binary, format, archstr) < 0)
+goto out;
 
 ret = virFindFileInPath(binary);
 VIR_FREE(binary);
-if (ret && !virFileIsExecutable(ret))
-VIR_FREE(ret);
+if (ret && virFileIsExecutable(ret))
+goto out;
 
-if (guestarch == VIR_ARCH_ARMV7L &&
-!ret &&
-hostarch == VIR_ARCH_AARCH64) {
-ret = virFindFileInPath("qemu-system-aarch64");
-if (ret && !virFileIsExecutable(ret))
-VIR_FREE(ret);
-}
+VIR_FREE(ret);
 
-if (guestarch == VIR_ARCH_I686 &&
-!ret &&
-hostarch == VIR_ARCH_X86_64) {
-ret = virFindFileInPath("qemu-system-x86_64");
-if (ret && !virFileIsExecutable(ret))
-VIR_FREE(ret);
-}
+ out:
+return ret;
+}
+
+static char *
+virQEMUCapsFindBinaryForArch(virArch hostarch,
+ virArch guestarch)
+{
+char *ret = NULL;
+const char *archstr;
+
+/* First attempt: try the guest architecture as it is */
+archstr = virQEMUCapsArchToString(guestarch);
+if ((ret = virQEMUCapsFindBinary("qemu-system-%s", archstr)) != NULL)
+goto out;
 
-if (guestarch == VIR_ARCH_I686 &&
-!ret) {
-ret = virFindFileInPath("qemu");
-if (ret && !virFileIsExecutable(ret))
-VIR_FREE(ret);
+/* Second attempt: use some arch-specific rules */
+archstr = virQEMUCapsArchToString(virQEMUCapsFindTarget(hostarch,
+guestarch));
+if ((ret = virQEMUCapsFindBinary("qemu-system-%s", archstr)) != NULL)
+goto out;
+
+/* Third attempt, i686 only: try 'qemu' */
+if (guestarch == VIR_ARCH_I686) {
+if ((ret = virQEMUCapsFindBinary("%s", "qemu")) != NULL)
+goto out;
 }
 
+ out:
 return ret;
 }
 
@@ -3793,10 +3816,17 @@ virQEMUCapsCacheLookupByArch(virQEMUCapsCachePtr cache,
 
 virMutexLock(&cache->lock);
 ret = virHashSearch(cache->binaries, virQEMUCapsCompareArch, &data);
-VIR_DEBUG("Returning caps %p for arch %s", ret, virArchToString(arch));
+if (!ret) {
+/* If the first attempt at findind capabilities has failed, try
+ * again using the QEMU target as lookup key instead */
+data.arch = virQEMUCapsFindTarget(virArchFromHost(), data.arch);
+ret = virHashSearch(cache->binaries, virQEMUCapsCompareArch, &data);
+}
 virObjectRef(ret);
 virMutexUnlo

Re: [libvirt] [PATCH 1/4] tests: split out common qemu driver initialization

2015-09-15 Thread Martin Kletzander 

On Tue, Sep 15, 2015 at 01:59:54PM +0200, Ján Tomko wrote:

On Tue, Sep 15, 2015 at 10:40:36AM +0200, Martin Kletzander wrote:

On Tue, Sep 15, 2015 at 10:05:19AM +0200, Ján Tomko wrote:
>From: Pavel Fedin 
>
>Two utility functions are introduced for proper initialization and
>cleanup of the driver.
>
>Signed-off-by: Pavel Fedin 
>Signed-off-by: Ján Tomko 
>---
> tests/domainsnapshotxml2xmltest.c | 10 +++---
> tests/qemuagenttest.c | 11 ++-
> tests/qemuargv2xmltest.c  | 12 ++--
> tests/qemuhotplugtest.c   |  9 ++---
> tests/qemuxml2argvtest.c  | 11 ++-
> tests/qemuxml2xmltest.c   |  8 +++-
> tests/qemuxmlnstest.c | 11 +++
> tests/testutilsqemu.c | 30 ++
> tests/testutilsqemu.h |  2 ++
> 9 files changed, 53 insertions(+), 51 deletions(-)
>
>diff --git a/tests/domainsnapshotxml2xmltest.c 
b/tests/domainsnapshotxml2xmltest.c
>index 3955a19..b66af3e 100644
>--- a/tests/domainsnapshotxml2xmltest.c
>+++ b/tests/domainsnapshotxml2xmltest.c
>@@ -152,13 +152,10 @@ mymain(void)
> {
> int ret = 0;
>
>-if ((driver.caps = testQemuCapsInit()) == NULL)
>+if (qemuTestDriverInit(&driver) < 0)
> return EXIT_FAILURE;
>
>-if (!(driver.xmlopt = virQEMUDriverCreateXMLConf(&driver))) {
>-virObjectUnref(driver.caps);
>-return EXIT_FAILURE;
>-}
>+driver.config->allowDiskFormatProbing = true;
>

Why is this needed?



These tests did not have a driver.config before, but they do after
switching to qemuTestDriverInit.

In qemuDomainDeviceDefPostParse, the code setting the format to raw when
format probing is off is wrapped in if (cfg). After this patch, it no
longer gets skipped.

The alternative would be to adjust the test files to work without
probing.



Although I would normally vote for that, I'm kind of on the edge.
Setting it to "true" by default is somewhat easier to fix, from what I
checked, but that's not we want since that's not reasonable production
default.  Could you at least put a TODO or XXX as a comment before the
' = true' line?  ACK with that (at least we can track it even though
it's not something crucial.


>diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
>index a2f4299..84dfa75 100644
>--- a/tests/testutilsqemu.c
>+++ b/tests/testutilsqemu.c



>+
>+return 0;
>+
>+ error:
>+virObjectUnref(driver->caps);
>+virObjectUnref(driver->config);
>+virObjectUnref(driver->xmlopt);

qemuTestDriverFree would be nicer

>+return -ENOMEM;

also -1 would do here.



I have squashed these two changes to my local branch.

Jan


>+}
>+
>+void qemuTestDriverFree(virQEMUDriver *driver)
>+{
>+virObjectUnref(driver->xmlopt);
>+virObjectUnref(driver->caps);
>+virObjectUnref(driver->config);
>+}
> #endif

ACK with allowDiskFormatProbing removed and the two mentioned nits
fixed, otherwise please explain the format probing if you want that
it too.





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: The ppc64 target can run both ppc64 and ppc64le guests

2015-09-15 Thread Daniel P. Berrange 
On Tue, Sep 15, 2015 at 01:53:52PM +0200, Andrea Bolognani wrote:
> When looking for a QEMU binary suitable for running ppc64le guests
> we have to take into account the fact that we use the QEMU target
> as key for the hash, so direct comparison is not good enough:
> normalize everything that can run on the ppc64 target to
> VIR_ARCH_PPC64 instead.
> 
> This approach was already used in virQEMUCapsFindBinaryForArch(),
> and I've added a comment there to clarify why it's needed.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1260753
> ---
>  src/qemu/qemu_capabilities.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 4ad1bdb..a06141e 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -699,6 +699,7 @@ virQEMUCapsFindBinaryForArch(virArch hostarch,
>  const char *archstr;
>  char *binary;
>  
> +/* qemu-system-ppc64 can run both ppc64 and ppc64le guests */
>  if (ARCH_IS_PPC64(guestarch))
>  archstr = virQEMUCapsArchToString(VIR_ARCH_PPC64);
>  else
> @@ -3791,6 +3792,10 @@ virQEMUCapsCacheLookupByArch(virQEMUCapsCachePtr cache,
>  virQEMUCapsPtr ret = NULL;
>  struct virQEMUCapsSearchData data = { .arch = arch };
>  
> +/* QEMU's ppc64 target can run both ppc64 and ppc64le guests */
> +if (ARCH_IS_PPC64(data.arch))
> +data.arch = VIR_ARCH_PPC64;
> +
>  virMutexLock(&cache->lock);
>  ret = virHashSearch(cache->binaries, virQEMUCapsCompareArch, &data);
>  VIR_DEBUG("Returning caps %p for arch %s", ret, virArchToString(arch));

The virQEMUCapsFindBinaryForArch metho already has a bunch of logic to
deal with fact that one binary can support multiple different
architectures. For example, x86_64 can support i686. It looks like we
hit the same problem as ppc64/ppc64le in that case and you have not
handled it here. It would be desirable to avoid having to have the same
compat logic in two places too.


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

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

Re: [libvirt] [PATCH 1/4] tests: split out common qemu driver initialization

2015-09-15 Thread Ján Tomko 
On Tue, Sep 15, 2015 at 10:40:36AM +0200, Martin Kletzander wrote:
> On Tue, Sep 15, 2015 at 10:05:19AM +0200, Ján Tomko wrote:
> >From: Pavel Fedin 
> >
> >Two utility functions are introduced for proper initialization and
> >cleanup of the driver.
> >
> >Signed-off-by: Pavel Fedin 
> >Signed-off-by: Ján Tomko 
> >---
> > tests/domainsnapshotxml2xmltest.c | 10 +++---
> > tests/qemuagenttest.c | 11 ++-
> > tests/qemuargv2xmltest.c  | 12 ++--
> > tests/qemuhotplugtest.c   |  9 ++---
> > tests/qemuxml2argvtest.c  | 11 ++-
> > tests/qemuxml2xmltest.c   |  8 +++-
> > tests/qemuxmlnstest.c | 11 +++
> > tests/testutilsqemu.c | 30 ++
> > tests/testutilsqemu.h |  2 ++
> > 9 files changed, 53 insertions(+), 51 deletions(-)
> >
> >diff --git a/tests/domainsnapshotxml2xmltest.c 
> >b/tests/domainsnapshotxml2xmltest.c
> >index 3955a19..b66af3e 100644
> >--- a/tests/domainsnapshotxml2xmltest.c
> >+++ b/tests/domainsnapshotxml2xmltest.c
> >@@ -152,13 +152,10 @@ mymain(void)
> > {
> > int ret = 0;
> >
> >-if ((driver.caps = testQemuCapsInit()) == NULL)
> >+if (qemuTestDriverInit(&driver) < 0)
> > return EXIT_FAILURE;
> >
> >-if (!(driver.xmlopt = virQEMUDriverCreateXMLConf(&driver))) {
> >-virObjectUnref(driver.caps);
> >-return EXIT_FAILURE;
> >-}
> >+driver.config->allowDiskFormatProbing = true;
> >
> 
> Why is this needed?
> 

These tests did not have a driver.config before, but they do after
switching to qemuTestDriverInit.

In qemuDomainDeviceDefPostParse, the code setting the format to raw when
format probing is off is wrapped in if (cfg). After this patch, it no
longer gets skipped.

The alternative would be to adjust the test files to work without
probing.

> >diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
> >index a2f4299..84dfa75 100644
> >--- a/tests/testutilsqemu.c
> >+++ b/tests/testutilsqemu.c

> >+
> >+return 0;
> >+
> >+ error:
> >+virObjectUnref(driver->caps);
> >+virObjectUnref(driver->config);
> >+virObjectUnref(driver->xmlopt);
> 
> qemuTestDriverFree would be nicer
> 
> >+return -ENOMEM;
> 
> also -1 would do here.
> 

I have squashed these two changes to my local branch.

Jan

> >+}
> >+
> >+void qemuTestDriverFree(virQEMUDriver *driver)
> >+{
> >+virObjectUnref(driver->xmlopt);
> >+virObjectUnref(driver->caps);
> >+virObjectUnref(driver->config);
> >+}
> > #endif
> 
> ACK with allowDiskFormatProbing removed and the two mentioned nits
> fixed, otherwise please explain the format probing if you want that
> it too.




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

[libvirt] [PATCH] qemu: The ppc64 target can run both ppc64 and ppc64le guests

2015-09-15 Thread Andrea Bolognani 
When looking for a QEMU binary suitable for running ppc64le guests
we have to take into account the fact that we use the QEMU target
as key for the hash, so direct comparison is not good enough:
normalize everything that can run on the ppc64 target to
VIR_ARCH_PPC64 instead.

This approach was already used in virQEMUCapsFindBinaryForArch(),
and I've added a comment there to clarify why it's needed.

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

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 4ad1bdb..a06141e 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -699,6 +699,7 @@ virQEMUCapsFindBinaryForArch(virArch hostarch,
 const char *archstr;
 char *binary;
 
+/* qemu-system-ppc64 can run both ppc64 and ppc64le guests */
 if (ARCH_IS_PPC64(guestarch))
 archstr = virQEMUCapsArchToString(VIR_ARCH_PPC64);
 else
@@ -3791,6 +3792,10 @@ virQEMUCapsCacheLookupByArch(virQEMUCapsCachePtr cache,
 virQEMUCapsPtr ret = NULL;
 struct virQEMUCapsSearchData data = { .arch = arch };
 
+/* QEMU's ppc64 target can run both ppc64 and ppc64le guests */
+if (ARCH_IS_PPC64(data.arch))
+data.arch = VIR_ARCH_PPC64;
+
 virMutexLock(&cache->lock);
 ret = virHashSearch(cache->binaries, virQEMUCapsCompareArch, &data);
 VIR_DEBUG("Returning caps %p for arch %s", ret, virArchToString(arch));
-- 
2.4.3

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

[libvirt] [PATCH 1/4] tests: split out common qemu driver initialization

2015-09-15 Thread Ján Tomko 
From: Pavel Fedin 

Two utility functions are introduced for proper initialization and
cleanup of the driver.

Signed-off-by: Pavel Fedin 
Signed-off-by: Ján Tomko 
---
 tests/domainsnapshotxml2xmltest.c | 10 +++---
 tests/qemuagenttest.c | 11 ++-
 tests/qemuargv2xmltest.c  | 12 ++--
 tests/qemuhotplugtest.c   |  9 ++---
 tests/qemuxml2argvtest.c  | 11 ++-
 tests/qemuxml2xmltest.c   |  8 +++-
 tests/qemuxmlnstest.c | 11 +++
 tests/testutilsqemu.c | 30 ++
 tests/testutilsqemu.h |  2 ++
 9 files changed, 53 insertions(+), 51 deletions(-)

diff --git a/tests/domainsnapshotxml2xmltest.c 
b/tests/domainsnapshotxml2xmltest.c
index 3955a19..b66af3e 100644
--- a/tests/domainsnapshotxml2xmltest.c
+++ b/tests/domainsnapshotxml2xmltest.c
@@ -152,13 +152,10 @@ mymain(void)
 {
 int ret = 0;
 
-if ((driver.caps = testQemuCapsInit()) == NULL)
+if (qemuTestDriverInit(&driver) < 0)
 return EXIT_FAILURE;
 
-if (!(driver.xmlopt = virQEMUDriverCreateXMLConf(&driver))) {
-virObjectUnref(driver.caps);
-return EXIT_FAILURE;
-}
+driver.config->allowDiskFormatProbing = true;
 
 if (VIR_ALLOC(testSnapshotXMLVariableLineRegex) < 0)
 goto cleanup;
@@ -227,8 +224,7 @@ mymain(void)
 if (testSnapshotXMLVariableLineRegex)
 regfree(testSnapshotXMLVariableLineRegex);
 VIR_FREE(testSnapshotXMLVariableLineRegex);
-virObjectUnref(driver.caps);
-virObjectUnref(driver.xmlopt);
+qemuTestDriverFree(&driver);
 
 return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c
index 52cc834..1ebc030 100644
--- a/tests/qemuagenttest.c
+++ b/tests/qemuagenttest.c
@@ -31,6 +31,8 @@
 
 #define VIR_FROM_THIS VIR_FROM_NONE
 
+static virQEMUDriver driver;
+
 static int
 testQemuAgentFSFreeze(const void *data)
 {
@@ -909,7 +911,6 @@ static int
 mymain(void)
 {
 int ret = 0;
-virDomainXMLOptionPtr xmlopt;
 
 #if !WITH_YAJL
 fputs("libvirt not compiled with yajl, skipping this test\n", stderr);
@@ -917,13 +918,13 @@ mymain(void)
 #endif
 
 if (virThreadInitialize() < 0 ||
-!(xmlopt = virQEMUDriverCreateXMLConf(NULL)))
+qemuTestDriverInit(&driver) < 0)
 return EXIT_FAILURE;
 
 virEventRegisterDefaultImpl();
 
-#define DO_TEST(name)   \
-if (virtTestRun(# name, testQemuAgent ## name, xmlopt) < 0) \
+#define DO_TEST(name)  \
+if (virtTestRun(# name, testQemuAgent ## name, driver.xmlopt) < 0) \
 ret = -1
 
 DO_TEST(FSFreeze);
@@ -938,7 +939,7 @@ mymain(void)
 
 DO_TEST(Timeout); /* Timeout should always be called last */
 
-virObjectUnref(xmlopt);
+qemuTestDriverFree(&driver);
 
 return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
 }
diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c
index ea85913..96453e5 100644
--- a/tests/qemuargv2xmltest.c
+++ b/tests/qemuargv2xmltest.c
@@ -145,15 +145,9 @@ mymain(void)
 {
 int ret = 0;
 
-driver.config = virQEMUDriverConfigNew(false);
-if (driver.config == NULL)
+if (qemuTestDriverInit(&driver) < 0)
 return EXIT_FAILURE;
 
-if ((driver.caps = testQemuCapsInit()) == NULL)
-return EXIT_FAILURE;
-
-if (!(driver.xmlopt = virQEMUDriverCreateXMLConf(&driver)))
-return EXIT_FAILURE;
 
 # define DO_TEST_FULL(name, flags)  \
 do {\
@@ -298,9 +292,7 @@ mymain(void)
 DO_TEST("machine-deakeywrap-off-argv");
 DO_TEST("machine-keywrap-none-argv");
 
-virObjectUnref(driver.config);
-virObjectUnref(driver.caps);
-virObjectUnref(driver.xmlopt);
+qemuTestDriverFree(&driver);
 
 return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 368a5e7..3cf7f36 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -338,14 +338,11 @@ mymain(void)
 #endif
 
 if (virThreadInitialize() < 0 ||
-!(driver.caps = testQemuCapsInit()) ||
-!(driver.xmlopt = virQEMUDriverCreateXMLConf(&driver)))
+qemuTestDriverInit(&driver) < 0)
 return EXIT_FAILURE;
 
 virEventRegisterDefaultImpl();
 
-if (!(driver.config = virQEMUDriverConfigNew(false)))
-return EXIT_FAILURE;
 VIR_FREE(driver.config->spiceListen);
 VIR_FREE(driver.config->vncListen);
 /* some dummy values from 'config file' */
@@ -486,9 +483,7 @@ mymain(void)
"device_del", QMP_DEVICE_DELETED("scsi0-0-0-5") QMP_OK,
"human-monitor-command", HMP(""));
 
-virObjectUnref(driver.caps);
-virObjectUnref(driver.xmlopt);
-virObjectUnref(driver.config);
+qemuTestDriverFree(&driver);
 return

Re: [libvirt] [PATCH 1/4] tests: split out common qemu driver initialization

2015-09-15 Thread Martin Kletzander 

On Tue, Sep 15, 2015 at 10:05:19AM +0200, Ján Tomko wrote:

From: Pavel Fedin 

Two utility functions are introduced for proper initialization and
cleanup of the driver.

Signed-off-by: Pavel Fedin 
Signed-off-by: Ján Tomko 
---
tests/domainsnapshotxml2xmltest.c | 10 +++---
tests/qemuagenttest.c | 11 ++-
tests/qemuargv2xmltest.c  | 12 ++--
tests/qemuhotplugtest.c   |  9 ++---
tests/qemuxml2argvtest.c  | 11 ++-
tests/qemuxml2xmltest.c   |  8 +++-
tests/qemuxmlnstest.c | 11 +++
tests/testutilsqemu.c | 30 ++
tests/testutilsqemu.h |  2 ++
9 files changed, 53 insertions(+), 51 deletions(-)

diff --git a/tests/domainsnapshotxml2xmltest.c 
b/tests/domainsnapshotxml2xmltest.c
index 3955a19..b66af3e 100644
--- a/tests/domainsnapshotxml2xmltest.c
+++ b/tests/domainsnapshotxml2xmltest.c
@@ -152,13 +152,10 @@ mymain(void)
{
int ret = 0;

-if ((driver.caps = testQemuCapsInit()) == NULL)
+if (qemuTestDriverInit(&driver) < 0)
return EXIT_FAILURE;

-if (!(driver.xmlopt = virQEMUDriverCreateXMLConf(&driver))) {
-virObjectUnref(driver.caps);
-return EXIT_FAILURE;
-}
+driver.config->allowDiskFormatProbing = true;



Why is this needed?


diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 5a20ebc..3552309 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -302,11 +302,10 @@ mymain(void)
int ret = 0;
struct testInfo info;

-if ((driver.caps = testQemuCapsInit()) == NULL)
+if (qemuTestDriverInit(&driver) < 0)
return EXIT_FAILURE;

-if (!(driver.xmlopt = virQEMUDriverCreateXMLConf(&driver)))
-return EXIT_FAILURE;
+driver.config->allowDiskFormatProbing = true;



same here


diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index a2f4299..84dfa75 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -526,4 +526,34 @@ qemuTestParseCapabilities(const char *capsFile)
xmlXPathFreeContext(ctxt);
return NULL;
}
+
+int qemuTestDriverInit(virQEMUDriver *driver)
+{
+driver->config = virQEMUDriverConfigNew(false);
+if (!driver->config)
+return -ENOMEM;
+
+driver->caps = testQemuCapsInit();
+if (!driver->caps)
+goto error;
+
+driver->xmlopt = virQEMUDriverCreateXMLConf(driver);
+if (!driver->xmlopt)
+goto error;
+
+return 0;
+
+ error:
+virObjectUnref(driver->caps);
+virObjectUnref(driver->config);
+virObjectUnref(driver->xmlopt);


qemuTestDriverFree would be nicer


+return -ENOMEM;


also -1 would do here.


+}
+
+void qemuTestDriverFree(virQEMUDriver *driver)
+{
+virObjectUnref(driver->xmlopt);
+virObjectUnref(driver->caps);
+virObjectUnref(driver->config);
+}
#endif


ACK with allowDiskFormatProbing removed and the two mentioned nits
fixed, otherwise please explain the format probing if you want that
it too.


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

Re: [libvirt] [PATCH v3] Ignore virtio-mmio disks in qemuAssignDevicePCISlots()

2015-09-15 Thread Martin Kletzander 

On Wed, Sep 09, 2015 at 03:02:53PM +0300, Pavel Fedin wrote:

Fixes the following error when attempting to add a disk with bus='virtio'
to a machine which actually supports virtio-mmio (caught with ARM virt):

virtio disk cannot have an address of type 'virtio-mmio'

The problem has been likely introduced by
e8d55172544c1fafe31a9e09346bdebca4f0d6f9. Before that
qemuAssignDevicePCISlots() was never called for ARM "virt" machine.



ACK && Pushed


Signed-off-by: Pavel Fedin 
---
v2 => v3
- Bring back qemuCaps to qemuAssignDevicePCISlots(), was lost in
a3ecd63e928ff39d73c1c14b0fb3be8addbc977b
- Swap conditions so as not to call virQEMUCapsGet() every time

v1 => v2
- Added check for QEMU_CAPS_DEVICE_VIRTIO_MMIO, this leaves the
 error message for machines which actually do not support
 virtio-mmio. In this case the user may still attempt to add
 such a disk manually.
---
src/qemu/qemu_command.c | 11 +--
src/qemu/qemu_command.h |  1 +
2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ec5e3d4..ea1bb28 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2241,7 +2241,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0)
goto cleanup;

-if (qemuAssignDevicePCISlots(def, addrs) < 0)
+if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
goto cleanup;

for (i = 1; i < addrs->nbuses; i++) {
@@ -2274,7 +2274,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
if (qemuValidateDevicePCISlotsChipsets(def, qemuCaps, addrs) < 0)
goto cleanup;

-if (qemuAssignDevicePCISlots(def, addrs) < 0)
+if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
goto cleanup;

for (i = 0; i < def->ncontrollers; i++) {
@@ -2406,6 +2406,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
 */
int
qemuAssignDevicePCISlots(virDomainDefPtr def,
+ virQEMUCapsPtr qemuCaps,
 virDomainPCIAddressSetPtr addrs)
{
size_t i, j;
@@ -2598,6 +2599,12 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)
continue;

+/* Also ignore virtio-mmio disks if our machine allows them */
+if (def->disks[i]->info.type ==
+VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO &&
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MMIO))
+continue;
+
if (def->disks[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
   _("virtio disk cannot have an address of type '%s'"),
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 767d31f..4aa7f2d 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -284,6 +284,7 @@ virDomainPCIAddressSetPtr 
qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
bool dryRun);

int qemuAssignDevicePCISlots(virDomainDefPtr def,
+ virQEMUCapsPtr qemuCaps,
 virDomainPCIAddressSetPtr addrs);

int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps);
--
1.9.5.msysgit.0




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

[libvirt] [PATCH] Minor typo fixes in documentation

2015-09-15 Thread Christian Loehle 

Signed-off-by: Christian Loehle 

diff --git a/docs/formatcaps.html.in b/docs/formatcaps.html.in
index 4245d8a..c187187 100644
--- a/docs/formatcaps.html.in
+++ b/docs/formatcaps.html.in
@@ -162,7 +162,7 @@
   
   
   
-
+
   
 
   
diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in
index 64365b2..17d1360 100644
--- a/docs/internals/command.html.in
+++ b/docs/internals/command.html.in
@@ -68,8 +68,8 @@
   There is now a high level API that provides a safe and
   flexible way to spawn commands, which prevents the most
   common errors & is easy to code against.  This
-  code is provided in the src/util/command.h
-  header which can be imported using #include "command.h"
+  code is provided in the src/util/vircommand.h
+  header which can be imported using #include "vircommand.h"
 
 
 Defining commands in libvirt
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 964a4d7..1edd73e 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -268,7 +268,7 @@ virDomainCreateXMLWithFiles(virConnectPtr conn, const char *xmlDesc,
  *
  * Deprecated after 0.4.6.
  * Renamed to virDomainCreateXML() providing identical functionality.
- * This existing name will left indefinitely for API compatibility.
+ * This existing name will be left indefinitely for API compatibility.
  *
  * Returns a new domain object or NULL in case of failure
  */
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 4/4] Removed unneeded check

2015-09-15 Thread Ján Tomko 
From: Pavel Fedin 

Since test suite now correctly creates capabilities cache, the hack is not
needed any more.

Signed-off-by: Pavel Fedin 
Signed-off-by: Ján Tomko 
---
 src/qemu/qemu_domain.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c8b0ccd..26ca12d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1039,10 +1039,7 @@ qemuDomainDefPostParse(virDomainDefPtr def,
 return ret;
 
 
-/* This condition is actually a (temporary) hack for test suite which
- * does not create capabilities cache */
-if (driver && driver->qemuCapsCache)
-qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, 
def->emulator);
+qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator);
 
 /* Add implicit PCI root controller if the machine has one */
 switch (def->os.arch) {
@@ -1241,10 +1238,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
 if (driver)
 cfg = virQEMUDriverGetConfig(driver);
 
-/* This condition is actually a (temporary) hack for test suite which
- * does not create capabilities cache */
-if (driver && driver->qemuCapsCache)
-qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, 
def->emulator);
+qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator);
 
 if (dev->type == VIR_DOMAIN_DEVICE_NET &&
 dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
-- 
2.4.6

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

[libvirt] [PATCH 0/4] Implement mockup capabilities cache in QEMU tests

2015-09-15 Thread Ján Tomko 
v2: https://www.redhat.com/archives/libvir-list/2015-September/msg00374.html
v3:
 * split out the movement of existing initialization code to qemuTestDriverInit
   from the cache addition
 * qemuTestDriverInit now adds an empty capability cache, so most tests are 
unchanged
 * renamed 'cleanup' to 'error' in qemuTestDriverInit, since it's only called 
on error paths
 * fixed indentation issues to pass syntax-check
 * removed mode changes to 755

Pavel Fedin (4):
  tests: split out common qemu driver initialization
  Implement infrastracture for mocking up QEMU capabilities cache
  Use mockup cache
  Removed unneeded check

 src/qemu/qemu_capabilities.c  | 16 -
 src/qemu/qemu_capspriv.h  | 36 
 src/qemu/qemu_domain.c| 10 ++
 tests/domainsnapshotxml2xmltest.c | 10 ++
 tests/qemuagenttest.c | 11 +++---
 tests/qemuargv2xmltest.c  | 12 ++-
 tests/qemuhotplugtest.c   | 32 +-
 tests/qemuxml2argvtest.c  | 17 +-
 tests/qemuxml2xmltest.c   |  8 ++---
 tests/qemuxmlnstest.c | 17 +-
 tests/testutilsqemu.c | 70 +++
 tests/testutilsqemu.h |  7 
 12 files changed, 170 insertions(+), 76 deletions(-)
 create mode 100644 src/qemu/qemu_capspriv.h

-- 
2.4.6

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

Re: [libvirt] [PATCH] conf/qemu: enforce NUMA nodes only for x86 memory hotplug

2015-09-15 Thread Nikunj A Dadhania 
Peter Krempa  writes:

> On Tue, Aug 18, 2015 at 15:35:11 +0530, Nikunj A Dadhania wrote:
>> libvirt enforces at least one NUMA node for memory hotplug support on
>> all architectures. While it might be required for some x86 guest,
>> PowerPC can hotplug memory on non-NUMA system.
>> 
>> The generic checks are replaced with arch specific check and xml
>> validation too does not enforce "node" for non-x86 arch.
>> 
>> CC: Peter Krempa 
>> Signed-off-by: Nikunj A Dadhania 
>> ---
>>  src/conf/domain_conf.c  |  9 ++---
>>  src/qemu/qemu_command.c | 28 +---
>>  2 files changed, 23 insertions(+), 14 deletions(-)
>> 
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index fd0450f..4cb2d4a 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>
> ...
>
>> @@ -12437,7 +12438,7 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
>>  xmlNodePtr save = ctxt->node;
>>  ctxt->node = node;
>>  
>> -if (virXPathUInt("string(./node)", ctxt, &def->targetNode) < 0) {
>> +if (virXPathUInt("string(./node)", ctxt, &def->targetNode) < 0 && 
>> ARCH_IS_X86(domDef->os.arch)) {
>
> The parser code should not be made architecture dependant. In this case
> we will need to adjust the code in a way that it will set a known value
> in case the numa node was not provided in the device XML and the check
> itself will need to be moved into the post parse callback so that the
> decision can be made on a per-hypervisor basis.

Sure, the only requirement is node should not be made mandatory in
parser code.

>
>>  virReportError(VIR_ERR_XML_ERROR, "%s",
>> _("invalid or missing value of memory device node"));
>>  goto cleanup;
>
> ...
>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index ae03618..51160e7 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -4979,8 +4979,12 @@ qemuBuildMemoryBackendStr(unsigned long long size,
>>  *backendProps = NULL;
>>  *backendType = NULL;
>>  
>> -/* memory devices could provide a invalid guest node */
>> -if (guestNode >= virDomainNumaGetNodeCount(def->numa)) {
>> +/* memory devices could provide a invalid guest node. Moreover,
>> + * x86 guests needs at least one numa node to support memory
>> + * hotplug
>> + */
>> +if ((virDomainNumaGetNodeCount(def->numa) == 0 && 
>> ARCH_IS_X86(def->os.arch)) ||
>> +guestNode > virDomainNumaGetNodeCount(def->numa)) {
>
> If we make this ARCH dependent here it will be hard to adjust it again
> in the future. Also I think we should whitelist PPC rather than
> blacklisting x86, since other ARCHes and OSes might have the same
> problem here.

Sure.

>
>>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> _("can't add memory backend for guest node '%d' as "
>>   "the guest has only '%zu' NUMA nodes configured"),
>> @@ -4991,10 +4995,12 @@ qemuBuildMemoryBackendStr(unsigned long long size,
>>  if (!(props = virJSONValueNewObject()))
>>  return -1;
>>  
>> -memAccess = virDomainNumaGetNodeMemoryAccessMode(def->numa, guestNode);
>> -if (virDomainNumatuneGetMode(def->numa, guestNode, &mode) < 0 &&
>> -virDomainNumatuneGetMode(def->numa, -1, &mode) < 0)
>> -mode = VIR_DOMAIN_NUMATUNE_MEM_STRICT;
>> +if (virDomainNumaGetNodeCount(def->numa)) {
>> +memAccess = virDomainNumaGetNodeMemoryAccessMode(def->numa, 
>> guestNode);
>> +if (virDomainNumatuneGetMode(def->numa, guestNode, &mode) < 0 &&
>> +virDomainNumatuneGetMode(def->numa, -1, &mode) < 0)
>> +mode = VIR_DOMAIN_NUMATUNE_MEM_STRICT;
>> +}
>>  
>>  if (pagesize == 0) {
>>  /* Find the huge page size we want to use */
>> @@ -9238,11 +9244,11 @@ qemuBuildCommandLine(virConnectPtr conn,
>>  goto error;
>>  }
>>  
>> -/* due to guest support, qemu would silently enable NUMA with one 
>> node
>> - * once the memory hotplug backend is enabled. To avoid possible
>> - * confusion we will enforce user originated numa configuration 
>> along
>> - * with memory hotplug. */
>> -if (virDomainNumaGetNodeCount(def->numa) == 0) {
>> +/* x86 windows guest needs at least one numa node to be
>> + * present. While its not possible to detect what guest os is
>> + * running, enforce this limitation only to x86 architecture.
>
> Actually, qemu would add the numa node anyways, so the libvirt XML would
> not correspond to the configuration the guest sees and to avoid that we
> enforce the numa node.

I did not see this in my testing for PPC64.

>
>> + */
>> +if (ARCH_IS_X86(def->os.arch) && 
>> virDomainNumaGetNodeCount(def->numa) == 0) {
>>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> _("At least one numa node has to be configured 
>> when "
>>

Re: [libvirt] [PATCH 2/4] Implement infrastracture for mocking up QEMU capabilities cache

2015-09-15 Thread Martin Kletzander 

On Tue, Sep 15, 2015 at 10:05:20AM +0200, Ján Tomko wrote:

From: Pavel Fedin 

The main purpose of this patch is to introduce test mode to
virQEMUCapsCacheLookup(). This is done by adding a global variable, which
effectively overrides binary name. This variable is supposed to be set by
test suite.

The second addition is qemuTestCapsCacheInsert() function which allows the
test suite to actually populate the cache.

Signed-off-by: Pavel Fedin 
Signed-off-by: Ján Tomko 
---
src/qemu/qemu_capabilities.c | 16 +++-
src/qemu/qemu_capspriv.h | 36 
tests/testutilsqemu.c| 40 
tests/testutilsqemu.h|  5 +
4 files changed, 88 insertions(+), 9 deletions(-)
create mode 100644 src/qemu/qemu_capspriv.h

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 4ad1bdb..9a819d2 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -42,6 +42,7 @@
#include "virstring.h"
#include "qemu_hostdev.h"
#include "qemu_domain.h"
+#include "qemu_capspriv.h"

#include 
#include 
@@ -331,15 +332,6 @@ struct _virQEMUCaps {
unsigned int *machineMaxCpus;
};

-struct _virQEMUCapsCache {
-virMutex lock;
-virHashTablePtr binaries;
-char *libDir;
-char *cacheDir;
-uid_t runUid;
-gid_t runGid;
-};
-
struct virQEMUCapsSearchData {
virArch arch;
};
@@ -3718,11 +3710,17 @@ virQEMUCapsCacheNew(const char *libDir,
return NULL;
}

+const char *qemuTestCapsName;

virQEMUCapsPtr
virQEMUCapsCacheLookup(virQEMUCapsCachePtr cache, const char *binary)
{
virQEMUCapsPtr ret = NULL;
+
+/* This is used only by test suite!!! */
+if (qemuTestCapsName)
+binary = qemuTestCapsName;
+
virMutexLock(&cache->lock);
ret = virHashLookup(cache->binaries, binary);
if (ret &&
diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h
new file mode 100644
index 000..9288dbd
--- /dev/null
+++ b/src/qemu/qemu_capspriv.h
@@ -0,0 +1,36 @@
+/*
+ * qemu_capspriv.h: private declarations for QEMU capabilities generation
+ *
+ * Copyright (C) 2015 Samsung Electronics Co. Ltd
+ * Copyright (C) 2015 Pavel Fedin
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ * Author: Pavel Fedin 
+ */
+
+#ifndef __QEMU_CAPSPRIV_H__
+# define __QEMU_CAPSPRIV_H__
+
+struct _virQEMUCapsCache {
+virMutex lock;
+virHashTablePtr binaries;
+char *libDir;
+char *cacheDir;
+uid_t runUid;
+gid_t runGid;
+};
+
+#endif
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index 84dfa75..275d22a 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -8,6 +8,7 @@
# include "cpu_conf.h"
# include "qemu/qemu_driver.h"
# include "qemu/qemu_domain.h"
+# include "qemu/qemu_capspriv.h"
# include "virstring.h"

# define VIR_FROM_THIS VIR_FROM_QEMU
@@ -527,6 +528,32 @@ qemuTestParseCapabilities(const char *capsFile)
return NULL;
}

+int qemuTestCapsCacheInsert(virQEMUCapsCachePtr cache, const char *binary,
+virQEMUCapsPtr caps)
+{
+int ret;
+
+if (caps) {
+/* Our caps were created artificially, so we don't want
+ * virQEMUCapsCacheFree() to attempt to deallocate them */
+virObjectRef(caps);
+} else {
+caps = virQEMUCapsNew();
+if (!caps)
+return -ENOMEM;
+}
+
+/* We can have repeating names for our test data sets,
+ * so make sure there's no old copy */
+virHashRemoveEntry(cache->binaries, binary);
+
+ret = virHashAddEntry(cache->binaries, binary, caps);
+if (ret < 0)
+virObjectUnref(caps);
+
+return ret;
+}
+
int qemuTestDriverInit(virQEMUDriver *driver)
{
driver->config = virQEMUDriverConfigNew(false);
@@ -537,13 +564,24 @@ int qemuTestDriverInit(virQEMUDriver *driver)
if (!driver->caps)
goto error;

+/* Using /dev/null for libDir and cacheDir automatically produces errors
+ * upon attempt to use any of them */
+driver->qemuCapsCache = virQEMUCapsCacheNew("/dev/null", "/dev/null", 0, 
0);
+if (!driver->qemuCapsCache)
+goto error;
+
driver->xmlopt = virQEMUDriverCreateXMLConf(driver);
if (!driver->xmlopt)
goto error;

+qemuTestCapsName = "empty";
+if (qemuTestCapsCacheInsert(driver->qemuCapsCache, "empty", NULL) < 0)
+

[libvirt] [PATCH 2/4] Implement infrastracture for mocking up QEMU capabilities cache

2015-09-15 Thread Ján Tomko 
From: Pavel Fedin 

The main purpose of this patch is to introduce test mode to
virQEMUCapsCacheLookup(). This is done by adding a global variable, which
effectively overrides binary name. This variable is supposed to be set by
test suite.

The second addition is qemuTestCapsCacheInsert() function which allows the
test suite to actually populate the cache.

Signed-off-by: Pavel Fedin 
Signed-off-by: Ján Tomko 
---
 src/qemu/qemu_capabilities.c | 16 +++-
 src/qemu/qemu_capspriv.h | 36 
 tests/testutilsqemu.c| 40 
 tests/testutilsqemu.h|  5 +
 4 files changed, 88 insertions(+), 9 deletions(-)
 create mode 100644 src/qemu/qemu_capspriv.h

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 4ad1bdb..9a819d2 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -42,6 +42,7 @@
 #include "virstring.h"
 #include "qemu_hostdev.h"
 #include "qemu_domain.h"
+#include "qemu_capspriv.h"
 
 #include 
 #include 
@@ -331,15 +332,6 @@ struct _virQEMUCaps {
 unsigned int *machineMaxCpus;
 };
 
-struct _virQEMUCapsCache {
-virMutex lock;
-virHashTablePtr binaries;
-char *libDir;
-char *cacheDir;
-uid_t runUid;
-gid_t runGid;
-};
-
 struct virQEMUCapsSearchData {
 virArch arch;
 };
@@ -3718,11 +3710,17 @@ virQEMUCapsCacheNew(const char *libDir,
 return NULL;
 }
 
+const char *qemuTestCapsName;
 
 virQEMUCapsPtr
 virQEMUCapsCacheLookup(virQEMUCapsCachePtr cache, const char *binary)
 {
 virQEMUCapsPtr ret = NULL;
+
+/* This is used only by test suite!!! */
+if (qemuTestCapsName)
+binary = qemuTestCapsName;
+
 virMutexLock(&cache->lock);
 ret = virHashLookup(cache->binaries, binary);
 if (ret &&
diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h
new file mode 100644
index 000..9288dbd
--- /dev/null
+++ b/src/qemu/qemu_capspriv.h
@@ -0,0 +1,36 @@
+/*
+ * qemu_capspriv.h: private declarations for QEMU capabilities generation
+ *
+ * Copyright (C) 2015 Samsung Electronics Co. Ltd
+ * Copyright (C) 2015 Pavel Fedin
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ * Author: Pavel Fedin 
+ */
+
+#ifndef __QEMU_CAPSPRIV_H__
+# define __QEMU_CAPSPRIV_H__
+
+struct _virQEMUCapsCache {
+virMutex lock;
+virHashTablePtr binaries;
+char *libDir;
+char *cacheDir;
+uid_t runUid;
+gid_t runGid;
+};
+
+#endif
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index 84dfa75..275d22a 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -8,6 +8,7 @@
 # include "cpu_conf.h"
 # include "qemu/qemu_driver.h"
 # include "qemu/qemu_domain.h"
+# include "qemu/qemu_capspriv.h"
 # include "virstring.h"
 
 # define VIR_FROM_THIS VIR_FROM_QEMU
@@ -527,6 +528,32 @@ qemuTestParseCapabilities(const char *capsFile)
 return NULL;
 }
 
+int qemuTestCapsCacheInsert(virQEMUCapsCachePtr cache, const char *binary,
+virQEMUCapsPtr caps)
+{
+int ret;
+
+if (caps) {
+/* Our caps were created artificially, so we don't want
+ * virQEMUCapsCacheFree() to attempt to deallocate them */
+virObjectRef(caps);
+} else {
+caps = virQEMUCapsNew();
+if (!caps)
+return -ENOMEM;
+}
+
+/* We can have repeating names for our test data sets,
+ * so make sure there's no old copy */
+virHashRemoveEntry(cache->binaries, binary);
+
+ret = virHashAddEntry(cache->binaries, binary, caps);
+if (ret < 0)
+virObjectUnref(caps);
+
+return ret;
+}
+
 int qemuTestDriverInit(virQEMUDriver *driver)
 {
 driver->config = virQEMUDriverConfigNew(false);
@@ -537,13 +564,24 @@ int qemuTestDriverInit(virQEMUDriver *driver)
 if (!driver->caps)
 goto error;
 
+/* Using /dev/null for libDir and cacheDir automatically produces errors
+ * upon attempt to use any of them */
+driver->qemuCapsCache = virQEMUCapsCacheNew("/dev/null", "/dev/null", 0, 
0);
+if (!driver->qemuCapsCache)
+goto error;
+
 driver->xmlopt = virQEMUDriverCreateXMLConf(driver);
 if (!driver->xmlopt)
 goto error;
 
+qemuTestCapsName = "empty";
+if (qemuTestCapsCacheInsert(driver->qemuCapsCache, "empty", NULL) < 0)
+goto err

Re: [libvirt] [PATCH 2/3] Use mockup cache

2015-09-15 Thread Ján Tomko 
On Wed, Sep 09, 2015 at 05:05:37PM +0300, Pavel Fedin wrote:
>  Hello!
> 
> > Looks good to me, but domainsnapshotxml2xmltest crashes with this series
> > applied.
> 
>  Fixed and posted v2
> 
> > qemuTestDriverInit could be introduced in a separate patch before
> > qemuTestCapsCacheInsert, only moving the existing initializations from
> > all the separate tests.
> 
>  Sorry, totally no time to do this rework now.
> 

Okay, I took your v2, split out the patch, fixed some cosmetic issues
and sent v3:

https://www.redhat.com/archives/libvir-list/2015-September/msg00460.html

Jan


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

[libvirt] [PATCH 3/4] Use mockup cache

2015-09-15 Thread Ján Tomko 
From: Pavel Fedin 

Use the new API in order to correctly add capability sets to the cache
before parsing XML files

Signed-off-by: Pavel Fedin 
---
 tests/qemuhotplugtest.c  | 23 +++
 tests/qemuxml2argvtest.c |  6 ++
 tests/qemuxmlnstest.c|  6 ++
 3 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 3cf7f36..109d820 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -57,7 +57,7 @@ static int
 qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
  virDomainObjPtr *vm,
  const char *domxml,
- bool event)
+ bool event, const char *testname)
 {
 int ret = -1;
 qemuDomainObjPrivatePtr priv = NULL;
@@ -65,12 +65,6 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
 if (!(*vm = virDomainObjNew(xmlopt)))
 goto cleanup;
 
-if (!((*vm)->def = virDomainDefParseString(domxml,
-   driver.caps,
-   driver.xmlopt,
-   VIR_DOMAIN_DEF_PARSE_INACTIVE)))
-goto cleanup;
-
 priv = (*vm)->privateData;
 
 if (!(priv->qemuCaps = virQEMUCapsNew()))
@@ -85,6 +79,18 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
 if (event)
 virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT);
 
+qemuTestCapsName = testname;
+ret = qemuTestCapsCacheInsert(driver.qemuCapsCache, testname,
+  priv->qemuCaps);
+if (ret < 0)
+goto cleanup;
+
+if (!((*vm)->def = virDomainDefParseString(domxml,
+   driver.caps,
+   driver.xmlopt,
+   VIR_DOMAIN_DEF_PARSE_INACTIVE)))
+goto cleanup;
+
 if (qemuDomainAssignAddresses((*vm)->def, priv->qemuCaps, *vm) < 0)
 goto cleanup;
 
@@ -243,7 +249,8 @@ testQemuHotplug(const void *data)
 vm = test->vm;
 } else {
 if (qemuHotplugCreateObjects(driver.xmlopt, &vm, domain_xml,
- test->deviceDeletedEvent) < 0)
+ test->deviceDeletedEvent,
+ test->domain_filename) < 0)
 goto cleanup;
 }
 
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index cd12356..1fc767e 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -423,6 +423,12 @@ testCompareXMLToArgvHelper(const void *data)
 if (virQEMUCapsGet(info->extraFlags, QEMU_CAPS_ENABLE_FIPS))
 flags |= FLAG_FIPS;
 
+qemuTestCapsName = info->name;
+result = qemuTestCapsCacheInsert(driver.qemuCapsCache, info->name,
+ info->extraFlags);
+if (result < 0)
+goto cleanup;
+
 result = testCompareXMLToArgvFiles(xml, args, info->extraFlags,
info->migrateFrom, info->migrateFd,
flags);
diff --git a/tests/qemuxmlnstest.c b/tests/qemuxmlnstest.c
index 40e32dc..65bf1d3 100644
--- a/tests/qemuxmlnstest.c
+++ b/tests/qemuxmlnstest.c
@@ -179,6 +179,12 @@ testCompareXMLToArgvHelper(const void *data)
 abs_srcdir, info->name) < 0)
 goto cleanup;
 
+qemuTestCapsName = info->name;
+result = qemuTestCapsCacheInsert(driver.qemuCapsCache, info->name,
+ info->extraFlags);
+if (result < 0)
+goto cleanup;
+
 result = testCompareXMLToArgvFiles(xml, args, info->extraFlags,
info->migrateFrom, info->migrateFd,
info->json, info->expectError);
-- 
2.4.6

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